From bb0dde993683b30ab8303fbea010e322bcbd9e18 Mon Sep 17 00:00:00 2001 From: mprahl Date: Thu, 9 Nov 2017 15:36:39 -0500 Subject: [PATCH] Only complete the module build when *all components are tagged* and a KojiRepoRegen message is received --- .../scheduler/handlers/components.py | 11 +--- .../scheduler/handlers/modules.py | 1 + .../scheduler/handlers/repos.py | 12 ++++ .../scheduler/handlers/tags.py | 3 +- tests/__init__.py | 32 +++++++++- tests/test_build/test_build.py | 64 +++++++++++++++---- tests/test_scheduler/test_repo_done.py | 21 ++++++ tests/test_scheduler/test_tag_tagged.py | 14 ++-- 8 files changed, 129 insertions(+), 29 deletions(-) diff --git a/module_build_service/scheduler/handlers/components.py b/module_build_service/scheduler/handlers/components.py index 5fb49087..ac6e8825 100644 --- a/module_build_service/scheduler/handlers/components.py +++ b/module_build_service/scheduler/handlers/components.py @@ -118,17 +118,12 @@ def _finalize(config, session, msg, state): log.info("Batch done. Tagging %i components." % len( built_components_in_batch)) log.debug("%r" % built_components_in_batch) - install = bool(component_build.package == 'module-build-macros') - builder.buildroot_add_artifacts(built_components_in_batch, install=install) + builder.buildroot_add_artifacts( + built_components_in_batch, install=component_build.build_time_only) # Do not tag packages which belong to -build tag to final tag. - if not install: + if not component_build.build_time_only: builder.tag_artifacts(built_components_in_batch) - else: - # For components which should not be tagged in final Koji tag, - # set the build_time_only to True so we do not wait for the - # non-existing tagging task to finish. - component_build.build_time_only = True session.commit() elif (any([c.state != koji.BUILD_STATES['BUILDING'] diff --git a/module_build_service/scheduler/handlers/modules.py b/module_build_service/scheduler/handlers/modules.py index 9e50dd97..9c448522 100644 --- a/module_build_service/scheduler/handlers/modules.py +++ b/module_build_service/scheduler/handlers/modules.py @@ -329,6 +329,7 @@ def wait(config, session, msg): state_reason=reason, nvr=nvr, batch=1, + build_time_only=True ) session.add(component_build) diff --git a/module_build_service/scheduler/handlers/repos.py b/module_build_service/scheduler/handlers/repos.py index b4692938..34157358 100644 --- a/module_build_service/scheduler/handlers/repos.py +++ b/module_build_service/scheduler/handlers/repos.py @@ -54,6 +54,18 @@ def done(config, session, msg): log.info("Ignoring repo regen for already failed %r" % module_build) return + # Get the list of untagged components in current/previous batches which + # have been built successfully + if config.system in ('koji', 'test') and module_build.component_builds: + untagged_components = [ + c for c in module_build.up_to_current_batch() + if (not c.tagged or (not c.tagged_in_final and not c.build_time_only)) and + c.state == koji.BUILD_STATES['COMPLETE'] + ] + if untagged_components: + log.info("Ignoring repo regen, because not all components are tagged.") + return + # If there are no components in this module build, then current_batch will be empty if not module_build.component_builds: current_batch = [] diff --git a/module_build_service/scheduler/handlers/tags.py b/module_build_service/scheduler/handlers/tags.py index 18ba8954..c13f9590 100644 --- a/module_build_service/scheduler/handlers/tags.py +++ b/module_build_service/scheduler/handlers/tags.py @@ -34,8 +34,7 @@ logging.basicConfig(level=logging.DEBUG) def tagged(config, session, msg): """ Called whenever koji tags a build to tag. """ - - if not config.system == "koji": + if config.system not in ("koji", "test"): return [] # Find our ModuleBuild associated with this tagged artifact. diff --git a/tests/__init__.py b/tests/__init__.py index 1577071f..fc2116a1 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -88,6 +88,8 @@ def init_data(): component_one_build_one.nvr = 'nginx-1.10.1-2.module_nginx_1_2' component_one_build_one.batch = 1 component_one_build_one.module_id = 1 + index * 3 + component_one_build_one.tagged = True + component_one_build_one.tagged_in_final = True component_two_build_one = ComponentBuild() component_two_build_one.package = 'module-build-macros' @@ -101,6 +103,8 @@ def init_data(): 'module-build-macros-01-1.module_nginx_1_2' component_two_build_one.batch = 2 component_two_build_one.module_id = 1 + index * 3 + component_two_build_one.tagged = True + component_two_build_one.tagged_in_final = True build_two = ModuleBuild() build_two.name = 'postgressql' @@ -132,6 +136,8 @@ def init_data(): component_one_build_two.nvr = 'postgresql-9.5.3-4.module_postgresql_1_2' component_one_build_two.batch = 2 component_one_build_two.module_id = 2 + index * 3 + component_one_build_two.tagged = True + component_one_build_two.tagged_in_final = True component_two_build_two = ComponentBuild() component_two_build_two.package = 'module-build-macros' @@ -145,6 +151,8 @@ def init_data(): 'module-build-macros-01-1.module_postgresql_1_2' component_two_build_two.batch = 1 component_two_build_two.module_id = 2 + index * 3 + component_one_build_two.tagged = True + component_one_build_two.build_time_only = True build_three = ModuleBuild() build_three.name = 'testmodule' @@ -188,6 +196,8 @@ def init_data(): 'module-build-macros-01-1.module_postgresql_1_2' component_two_build_three.batch = 1 component_two_build_three.module_id = 3 + index * 3 + component_two_build_three.tagged = True + component_two_build_three.build_time_only = True with make_session(conf) as session: session.add(build_one) @@ -235,6 +245,9 @@ def scheduler_init_data(communicator_state=None): component_one_build_one.format = 'rpms' component_one_build_one.task_id = 12312345 component_one_build_one.state = communicator_state + if component_one_build_one.state == 1: + component_one_build_one.tagged = True + component_one_build_one.tagged_in_final = True component_one_build_one.nvr = 'communicator-1.10.1-2.module_starcommand_1_3' component_one_build_one.batch = 2 component_one_build_one.module_id = 1 @@ -247,6 +260,9 @@ def scheduler_init_data(communicator_state=None): component_two_build_one.format = 'rpms' component_two_build_one.task_id = 12312321 component_two_build_one.state = 1 + if component_two_build_one.state == 1: + component_two_build_one.tagged = True + component_two_build_one.tagged_in_final = True component_two_build_one.nvr = \ 'module-build-macros-01-1.module_starcommand_1_3' component_two_build_one.batch = 2 @@ -297,6 +313,8 @@ def test_reuse_component_init_data(): component_one_build_one.batch = 2 component_one_build_one.module_id = 1 component_one_build_one.ref = '4ceea43add2366d8b8c5a622a2fb563b625b9abf' + component_one_build_one.tagged = True + component_one_build_one.tagged_in_final = True component_two_build_one = module_build_service.models.ComponentBuild() component_two_build_one.package = 'perl-List-Compare' @@ -311,6 +329,8 @@ def test_reuse_component_init_data(): component_two_build_one.batch = 2 component_two_build_one.module_id = 1 component_two_build_one.ref = '76f9d8c8e87eed0aab91034b01d3d5ff6bd5b4cb' + component_two_build_one.tagged = True + component_two_build_one.tagged_in_final = True component_three_build_one = module_build_service.models.ComponentBuild() component_three_build_one.package = 'tangerine' @@ -325,6 +345,8 @@ def test_reuse_component_init_data(): component_three_build_one.batch = 3 component_three_build_one.module_id = 1 component_three_build_one.ref = 'fbed359411a1baa08d4a88e0d12d426fbf8f602c' + component_three_build_one.tagged = True + component_three_build_one.tagged_in_final = True component_four_build_one = module_build_service.models.ComponentBuild() component_four_build_one.package = 'module-build-macros' @@ -338,6 +360,8 @@ def test_reuse_component_init_data(): 'module-build-macros-0.1-1.module_testmodule_master_20170109091357' component_four_build_one.batch = 1 component_four_build_one.module_id = 1 + component_four_build_one.tagged = True + component_four_build_one.build_time_only = True mmd = modulemd.ModuleMetadata() mmd.loads(yaml) @@ -351,7 +375,7 @@ def test_reuse_component_init_data(): build_two.koji_tag = 'module-testmodule' build_two.scmurl = ('git://pkgs.stg.fedoraproject.org/modules/testmodule.' 'git?#55f4a0a') - build_two.batch = 0 + build_two.batch = 1 build_two.owner = 'Tom Brady' build_two.time_submitted = datetime(2017, 2, 19, 16, 8, 18) build_two.time_modified = datetime(2017, 2, 19, 16, 8, 18) @@ -399,6 +423,8 @@ def test_reuse_component_init_data(): 'module-build-macros-0.1-1.module_testmodule_master_20170219191323' component_four_build_two.batch = 1 component_four_build_two.module_id = 2 + component_four_build_two.tagged = True + component_four_build_two.build_time_only = True with make_session(conf) as session: session.add(build_one) @@ -466,7 +492,9 @@ def test_reuse_shared_userspace_init_data(): scmurl=full_url, batch=batch, ref=pkgref, - state=1 + state=1, + tagged=True, + tagged_in_final=True ) session.add(build) diff --git a/tests/test_build/test_build.py b/tests/test_build/test_build.py index 67d4500f..96cae09d 100644 --- a/tests/test_build/test_build.py +++ b/tests/test_build/test_build.py @@ -37,7 +37,7 @@ import module_build_service.utils from module_build_service.errors import Forbidden from module_build_service import db, models, conf, build_logs -from mock import patch, PropertyMock +from mock import patch, PropertyMock, Mock from tests import app, test_reuse_component_init_data, clean_database import json @@ -86,6 +86,14 @@ class FakeSCM(object): return path.join(self.sourcedir, self.name + ".yaml") +def _on_build_cb(cls, artifact_name, source): + # Tag the build in the -build tag + cls._send_tag(artifact_name) + if not artifact_name.startswith("module-build-macros"): + # Tag the build in the final tag + cls._send_tag(artifact_name, build=False) + + class FakeModuleBuilder(GenericBuilder): """ Fake module builder which succeeds for every build. @@ -99,7 +107,7 @@ class FakeModuleBuilder(GenericBuilder): INSTANT_COMPLETE = False DEFAULT_GROUPS = None - on_build_cb = None + on_build_cb = _on_build_cb on_cancel_cb = None on_buildroot_add_artifacts_cb = None on_tag_artifacts_cb = None @@ -109,16 +117,18 @@ class FakeModuleBuilder(GenericBuilder): self.module_str = module self.tag_name = tag_name self.config = config + self.on_build_cb = _on_build_cb @classmethod def reset(cls): FakeModuleBuilder.BUILD_STATE = "COMPLETE" FakeModuleBuilder.INSTANT_COMPLETE = False - FakeModuleBuilder.on_build_cb = None + FakeModuleBuilder.on_build_cb = _on_build_cb FakeModuleBuilder.on_cancel_cb = None FakeModuleBuilder.on_buildroot_add_artifacts_cb = None FakeModuleBuilder.on_tag_artifacts_cb = None FakeModuleBuilder.DEFAULT_GROUPS = None + FakeModuleBuilder.backend = 'test' def buildroot_connect(self, groups): default_groups = FakeModuleBuilder.DEFAULT_GROUPS or { @@ -157,6 +167,19 @@ class FakeModuleBuilder(GenericBuilder): def tag_artifacts(self, artifacts): if FakeModuleBuilder.on_tag_artifacts_cb: FakeModuleBuilder.on_tag_artifacts_cb(self, artifacts) + for nvr in artifacts: + # tag_artifacts received a list of NVRs, but the tag message expects the + # component name + artifact = models.ComponentBuild.query.filter_by(nvr=nvr).one().package + self._send_tag(artifact) + if not artifact.startswith('module-build-macros'): + self._send_tag(artifact, build=False) + + @property + def koji_session(self): + session = Mock() + session.newRepo.return_value = 123 + return session @property def module_build_tag(self): @@ -169,6 +192,18 @@ class FakeModuleBuilder(GenericBuilder): ) module_build_service.scheduler.consumer.work_queue_put(msg) + def _send_tag(self, artifact, build=True): + if build: + tag = self.tag_name + "-build" + else: + tag = self.tag_name + msg = module_build_service.messaging.KojiTagChange( + msg_id='a faked internal message', + tag=tag, + artifact=artifact + ) + module_build_service.scheduler.consumer.work_queue_put(msg) + def _send_build_change(self, state, source, build_id): # build_id=1 and task_id=1 are OK here, because we are building just # one RPM at the time. @@ -620,6 +655,11 @@ class TestBuild(unittest.TestCase): FakeModuleBuilder.BUILD_STATE = "FAILED" else: FakeModuleBuilder.BUILD_STATE = "COMPLETE" + # Tag the build in the -build tag + cls._send_tag(artifact_name) + if not artifact_name.startswith("module-build-macros"): + # Tag the build in the final tag + cls._send_tag(artifact_name, build=False) FakeModuleBuilder.on_build_cb = on_build_cb @@ -677,7 +717,9 @@ class TestBuild(unittest.TestCase): def on_build_cb(cls, artifact_name, source): # Next components *after* the module-build-macros will fail # to build. - if not artifact_name.startswith("module-build-macros"): + if artifact_name.startswith("module-build-macros"): + cls._send_tag(artifact_name) + else: FakeModuleBuilder.BUILD_STATE = "FAILED" FakeModuleBuilder.on_build_cb = on_build_cb @@ -858,6 +900,8 @@ class TestBuild(unittest.TestCase): component_one.batch = 2 component_one.module_id = 1 component_one.ref = '4ceea43add2366d8b8c5a622a2fb563b625b9abf' + component_one.tagged = True + component_one.tagged_in_final = True # Failed component component_two = models.ComponentBuild() component_two.package = 'perl-List-Compare' @@ -1000,10 +1044,12 @@ class TestBuild(unittest.TestCase): @patch("module_build_service.config.Config.system", - new_callable=PropertyMock, return_value="test") + new_callable=PropertyMock, return_value="testlocal") class TestLocalBuild(unittest.TestCase): def setUp(self): + FakeModuleBuilder.on_build_cb = None + FakeModuleBuilder.backend = 'testlocal' GenericBuilder.register_backend_class(FakeModuleBuilder) self.client = app.test_client() clean_database() @@ -1014,13 +1060,7 @@ class TestLocalBuild(unittest.TestCase): def tearDown(self): FakeModuleBuilder.reset() - - # Necessary to restart the twisted reactor for the next test. - import sys - del sys.modules['twisted.internet.reactor'] - del sys.modules['moksha.hub.reactor'] - del sys.modules['moksha.hub'] - import moksha.hub.reactor # noqa + cleanup_moksha() self.vcr.__exit__() for i in range(20): try: diff --git a/tests/test_scheduler/test_repo_done.py b/tests/test_scheduler/test_repo_done.py index 9b6e6ab6..d1ba07d5 100644 --- a/tests/test_scheduler/test_repo_done.py +++ b/tests/test_scheduler/test_repo_done.py @@ -110,6 +110,27 @@ class TestRepoDone(unittest.TestCase): self.assertEquals(component_build.state_reason, 'Failed to submit artifact communicator to Koji') + @mock.patch('module_build_service.scheduler.handlers.repos.log.info') + def test_erroneous_regen_repo_received(self, mock_log_info): + """ Test that when an unexpected KojiRepoRegen message is received, the module doesn't + complete or go to the next build batch. + """ + scheduler_init_data(1) + msg = module_build_service.messaging.KojiRepoChange( + 'some_msg_id', 'module-starcommand-1.3-build') + component_build = module_build_service.models.ComponentBuild.query\ + .filter_by(package='communicator').one() + component_build.tagged = False + db.session.add(component_build) + db.session.commit() + module_build_service.scheduler.handlers.repos.done( + config=conf, session=db.session, msg=msg) + mock_log_info.assert_called_once_with( + 'Ignoring repo regen, because not all components are tagged.') + module_build = module_build_service.models.ModuleBuild.query.get(1) + # Make sure the module build didn't transition since all the components weren't tagged + self.assertEqual(module_build.state, module_build_service.models.BUILD_STATES['build']) + @mock.patch('module_build_service.builder.KojiModuleBuilder.list_tasks_for_components', return_value=[]) @mock.patch('module_build_service.builder.KojiModuleBuilder.buildroot_ready', return_value=True) diff --git a/tests/test_scheduler/test_tag_tagged.py b/tests/test_scheduler/test_tag_tagged.py index 26d0e7c9..fca70e02 100644 --- a/tests/test_scheduler/test_tag_tagged.py +++ b/tests/test_scheduler/test_tag_tagged.py @@ -266,6 +266,10 @@ class TestTagTagged(unittest.TestCase): module_build = module_build_service.models.ModuleBuild.query.filter_by(id=2).one() module_build.batch = 2 + mbm = module_build_service.models.ComponentBuild.query.filter_by( + module_id=2, package='module-build-macros').one() + mbm.tagged = False + db.session.add(mbm) for c in module_build.current_batch(): c.state = koji.BUILD_STATES["COMPLETE"] db.session.commit() @@ -300,16 +304,16 @@ class TestTagTagged(unittest.TestCase): # to tag. self.assertTrue(not koji_session.newRepo.called) - # Tag the component from first batch to the buildroot. - msg = module_build_service.messaging.KojiTagChange( - 'id', 'module-testmodule-build', "module-build-macros") - module_build_service.scheduler.handlers.tags.tagged( - config=conf, session=db.session, msg=msg) # Tag the component from first batch to final tag. msg = module_build_service.messaging.KojiTagChange( 'id', 'module-testmodule', "module-build-macros") module_build_service.scheduler.handlers.tags.tagged( config=conf, session=db.session, msg=msg) + # Tag the component from first batch to the buildroot. + msg = module_build_service.messaging.KojiTagChange( + 'id', 'module-testmodule-build', "module-build-macros") + module_build_service.scheduler.handlers.tags.tagged( + config=conf, session=db.session, msg=msg) # newRepo should be called now - all components have been tagged. koji_session.newRepo.assert_called_once_with("module-testmodule-build")