PR#39: Prevent cyclic rebuild of modules by checking db records - freshmaker

freshmaker

#39 Prevent cyclic rebuild of modules by checking db records

Merged 8 years ago by qwan. Opened 8 years ago by qwan.

qwan/freshmaker prevent-cyclic-rebuilds into master

Prevent cyclic rebuild of modules by checking db records

Qixiang Wan • 8 years ago

aa04fc0

New api get_root_dep_of to get the root dep build

Qixiang Wan • 8 years ago

eb6f45b

Record dep_of when a module build is triggered

Qixiang Wan • 8 years ago

706e5be

freshmaker/handlers/mbs/module_state_change.py

file modified

+18 -7

		`@@ -22,7 +22,7 @@`
		`# Written by Jan Kaluza <jkaluza@redhat.com>`

		`from freshmaker import log, conf, utils, db, models`
		`- from freshmaker.types import ArtifactType`
		`+ from freshmaker.types import ArtifactType, ArtifactBuildState`
		`from freshmaker.mbs import MBS`
		`from freshmaker.pdc import PDC`
		`from freshmaker.handlers import BaseHandler`
		`@@ -50,19 +50,21 @@`
		`build_id = event.build_id`
		`build_state = event.build_state`

		`+ module_build = None`
		`# update build state if the build is submitted by Freshmaker`
		`builds = db.session.query(models.ArtifactBuild).filter_by(build_id=build_id,`
		`type=ArtifactType.MODULE.value).all()`
		`if len(builds) > 1:`
		`raise RuntimeError("Found duplicate module build '%s' in db" % build_id)`
		`if len(builds) == 1:`
		`- build = builds.pop()`
		`+ # we can find this build in DB`
		`+ module_build = builds.pop()`
		`if build_state in [MBS.BUILD_STATES['ready'], MBS.BUILD_STATES['failed']]:`
		`log.info("Module build '%s' state changed in MBS, updating it in db.", build_id)`
		`if build_state == MBS.BUILD_STATES['ready']:`
		`- build.state = models.BUILD_STATES['done']`
		`+ module_build.state = ArtifactBuildState.DONE.value`
		`if build_state == MBS.BUILD_STATES['failed']:`
		`- build.state = models.BUILD_STATES['failed']`
		`+ module_build.state = ArtifactBuildState.FAILED.value`
		`db.session.commit()`

		`# Rebuild depending modules when state of MBSModuleStateChangeEvent is 'ready'`
		`@@ -70,6 +72,15 @@`
		`log.info("Triggering rebuild of modules depending on %s:%s "`
		`"in MBS", module_name, module_stream)`

		`+ if module_build:`
		`+ # we have this build recorded in DB, check to prevent`
		`+ # cyclic build loop`
		`+ root_dep = module_build.get_root_dep_of()`
		`+ if root_dep and root_dep.name == module_name:`
		`+ log.info("Skipping the rebuild triggered by %s:%s as it will"`
		`+ "result in cyclic build loop.", module_name, module_stream)`
		`+ return []`
		`+`
		`pdc = PDC(conf)`
		`modules = pdc.get_latest_modules(build_dep_name=module_name,`
		`build_dep_stream=module_stream,`
		`@@ -85,8 +96,8 @@`
		`# bump module repo first`
		`commit_msg = "Bump to rebuild because of %s update" % module_name`
		`rev = utils.bump_distgit_repo('modules', name, branch=version, commit_msg=commit_msg, logger=log)`
		`- build_id = self.build_module(name, version, rev)`
		`- if build_id is not None:`
		`- self.record_build(event, name, 'module', build_id)`
		`+ new_build_id = self.build_module(name, version, rev)`
		`+ if new_build_id is not None:`
		`+ self.record_build(event, name, 'module', new_build_id, dep_of=module_build)`

		`return []`

freshmaker/models.py

file modified

+10

		`@@ -149,3 +149,13 @@`
		`return "<ArtifactBuild %s, type %s, state %s, event %s>" % (`
		`self.name, ArtifactType(self.type).name,`
		`ArtifactBuildState(self.state).name, self.event.message_id)`
		`+`
		`+ def get_root_dep_of(self):`
		`+ dep_of = self.dep_of`
		`+ while dep_of:`
		`+ dep = dep_of.dep_of`
		`+ if dep:`
		`+ dep_of = dep`
		`+ else:`
		`+ break`
		`+ return dep_of`

tests/test_mbs_module_state_change_handler.py

file modified

+87

		`@@ -175,6 +175,93 @@`

		`handler.build_module.assert_not_called()`

		`+ @mock.patch('freshmaker.handlers.mbs.module_state_change.PDC')`
		`+ @mock.patch('freshmaker.handlers.mbs.module_state_change.utils')`
		`+ @mock.patch('freshmaker.handlers.mbs.module_state_change.log')`
		`+ def test_handler_not_fall_into_cyclic_rebuild_loop(self, log, utils, PDC):`
		`+ """`
		`+ Tests handler will not fall into cyclic rebuild loop when there is`
		`+ build dep loop of modules.`
		`+ """`
		`+ # in this case, we have:`
		`+ # 1. module2 depends on module1`
		`+ # 2. module3 depends on module2`
		`+ # 3. module1 depends on module3`
		`+ #`
		`+ # when we receives a modult built event of module1, the expect result is:`
		`+ # 1. module2 get rebuild because module1 is built`
		`+ # 2. module3 get rebuild because module2 is built`
		`+ # 3. module1 get rebuild because module3 is built`
		`+ # 4. stop here`
		`+`
		`+ utils.bump_distgit_repo.return_value = 'abcd'`
		`+`
		`+ mod1_info = helpers.PDCModuleInfo('module1', 'master', '20170412010101')`
		`+ mod1_info.add_build_dep('module3', 'master')`
		`+ mod1 = mod1_info.produce()`
		`+`
		`+ mod2_info = helpers.PDCModuleInfo('module2', 'master', '20170412010102')`
		`+ mod2_info.add_build_dep('module1', 'master')`
		`+ mod2 = mod2_info.produce()`
		`+`
		`+ mod3_info = helpers.PDCModuleInfo('module3', 'master', '20170412010103')`
		`+ mod3_info.add_build_dep('module2', 'master')`
		`+ mod3 = mod3_info.produce()`
		`+`
		`+ pdc = PDC.return_value`
		`+ handler = MBSModuleStateChangeHandler()`
		`+`
		`+ # Assume we have build of module1 recorded in DB already, it doesn't has`
		`+ # any dep_of as it was initial triggered by an event which is not`
		`+ # associated with any build in our DB.`
		`+ event = models.Event.create(db.session, "initial_msg_id")`
		`+ models.ArtifactBuild.create(db.session, event, "module1", "module", '123')`
		`+ db.session.commit()`
		`+`
		`+ # we received module built event of module1`
		`+ msg = helpers.ModuleStateChangeMessage('module1', 'master', state='ready', build_id=123).produce()`
		`+ event = self.get_event_from_msg(msg)`
		`+ pdc.get_latest_modules.return_value = [mod2]`
		`+ handler.build_module = mock.Mock()`
		`+ handler.build_module.return_value = 124`
		`+`
		`+ # this will trigger module rebuild of module2`
		`+ handler.handle(event)`
		`+ handler.build_module.assert_called_once_with('module2', 'master', 'abcd')`
		`+`
		`+ # we received module built event of module2`
		`+ msg = helpers.ModuleStateChangeMessage('module2', 'master', state='ready', build_id=124).produce()`
		`+ event = self.get_event_from_msg(msg)`
		`+ pdc.get_latest_modules.return_value = [mod3]`
		`+ handler.build_module = mock.Mock()`
		`+ handler.build_module.return_value = 125`
		`+`
		`+ # this will trigger module rebuild of module3`
		`+ handler.handle(event)`
		`+ handler.build_module.assert_called_once_with('module3', 'master', 'abcd')`
		`+`
		`+ # we received module built event of module3`
		`+ msg = helpers.ModuleStateChangeMessage('module3', 'master', state='ready', build_id=125).produce()`
		`+ event = self.get_event_from_msg(msg)`
		`+ pdc.get_latest_modules.return_value = [mod1]`
		`+ handler.build_module = mock.Mock()`
		`+ handler.build_module.return_value = 126`
		`+`
		`+ # this will trigger module rebuild of module1`
		`+ handler.handle(event)`
		`+ handler.build_module.assert_called_once_with('module1', 'master', 'abcd')`
		`+`
		`+ # we received module built event of module1`
		`+ msg = helpers.ModuleStateChangeMessage('module1', 'master', state='ready', build_id=126).produce()`
		`+ event = self.get_event_from_msg(msg)`
		`+ pdc.get_latest_modules.return_value = [mod2]`
		`+ handler.build_module = mock.Mock()`
		`+`
		`+ # but this time we should not rebuild module2`
		`+ handler.handle(event)`
		`+ handler.build_module.assert_not_called()`
		`+ log.info.assert_has_calls([mock.call('Skipping the rebuild triggered by %s:%s as it willresult in cyclic build loop.', 'module1', u'master')])`
		`+`

		`if __name__ == '__main__':`
		`unittest.main()`

tests/test_models.py

file modified

+13

		`@@ -63,3 +63,16 @@`
		`self.assertEqual(e.builds[1].state, 0)`
		`self.assertEqual(e.builds[1].build_id, 1235)`
		`self.assertEqual(e.builds[1].dep_of.name, "ed")`
		`+`
		`+ def test_get_root_dep_of(self):`
		`+ event = Event.create(db.session, "test_msg_id", "test", TestingEvent)`
		`+ build1 = ArtifactBuild.create(db.session, event, "ed", "module", 1234)`
		`+ build2 = ArtifactBuild.create(db.session, event, "mksh", "module", 1235, build1)`
		`+ build3 = ArtifactBuild.create(db.session, event, "runtime", "module", 1236, build2)`
		`+ build4 = ArtifactBuild.create(db.session, event, "perl-runtime", "module", 1237, build3)`
		`+ db.session.commit()`
		`+ db.session.expire_all()`
		`+ self.assertEqual(build1.get_root_dep_of(), None)`
		`+ self.assertEqual(build2.get_root_dep_of(), build1)`
		`+ self.assertEqual(build3.get_root_dep_of(), build1)`
		`+ self.assertEqual(build4.get_root_dep_of(), build1)`

qwan commented 8 years ago

Handler should not fall into cyclic rebuild loop when there is build dep loop of modules. For example if we have:
1. Module B depends on Module A
2. Module C depends on Module B
3. Module A depends on Module C
and now we receives a module built event of Module A, then we should:
1. Rebuild Module B because of A is built.
2. Rebuild Module C when Module B is built in MBS.
3. Rebuild Module A when Module C is built in MBS.
4. Stop here, so we won't rebuild Module B.