From 9e50e9e2685c1ca07ba5e8d68b739da0e31c678b Mon Sep 17 00:00:00 2001 From: Luiz Carvalho Date: Jul 12 2019 18:12:15 +0000 Subject: Do not start newRepo task if one in progress Signed-off-by: Luiz Carvalho Check task status on producer - to be squashed Signed-off-by: Luiz Carvalho --- diff --git a/module_build_service/scheduler/handlers/tags.py b/module_build_service/scheduler/handlers/tags.py index 77b7f73..1743ad2 100644 --- a/module_build_service/scheduler/handlers/tags.py +++ b/module_build_service/scheduler/handlers/tags.py @@ -89,10 +89,16 @@ def tagged(config, session, msg): if c.state == koji.BUILD_STATES["BUILDING"] or not c.state ] if unbuilt_components: - repo_tag = builder.module_build_tag["name"] - log.info("All components in batch tagged, regenerating repo for tag %s", repo_tag) - task_id = builder.koji_session.newRepo(repo_tag) - module_build.new_repo_task_id = task_id + if not _is_new_repo_generating(module_build, builder.koji_session): + repo_tag = builder.module_build_tag["name"] + log.info("All components in batch tagged, regenerating repo for tag %s", repo_tag) + task_id = builder.koji_session.newRepo(repo_tag) + module_build.new_repo_task_id = task_id + else: + log.info( + "newRepo task %s for %r already in progress, not starting another one", + str(module_build.new_repo_task_id), module_build, + ) else: # In case this is the last batch, we do not need to regenerate the # buildroot, because we will not build anything else in it. It @@ -106,3 +112,18 @@ def tagged(config, session, msg): session.commit() return further_work + + +def _is_new_repo_generating(module_build, koji_session): + """ Return whether or not a new repo is already being generated. """ + if not module_build.new_repo_task_id: + return False + + log.debug( + 'Checking status of newRepo task "%d" for %s', module_build.new_repo_task_id, module_build) + task_info = koji_session.getTaskInfo(module_build.new_repo_task_id) + + active_koji_states = [ + koji.TASK_STATES["FREE"], koji.TASK_STATES["OPEN"], koji.TASK_STATES["ASSIGNED"]] + + return task_info["state"] in active_koji_states diff --git a/module_build_service/scheduler/producer.py b/module_build_service/scheduler/producer.py index 82a8df0..44848c7 100644 --- a/module_build_service/scheduler/producer.py +++ b/module_build_service/scheduler/producer.py @@ -275,19 +275,16 @@ class MBSProducer(PollingProducer): # then no possible event will start off new component builds. # But do not try to start new builds when we are waiting for the # repo-regen. - if ( - not module_build.current_batch(koji.BUILD_STATES["BUILDING"]) - and not module_build.new_repo_task_id - ): - log.info(" Processing the paused module build %r", module_build) + if not module_build.current_batch(koji.BUILD_STATES["BUILDING"]): # Initialize the builder... builder = GenericBuilder.create_from_module(session, module_build, config) - - further_work = module_build_service.utils.start_next_batch_build( - config, module_build, session, builder) - for event in further_work: - log.info(" Scheduling faked event %r" % event) - module_build_service.scheduler.consumer.work_queue_put(event) + if _has_missed_new_repo_message(module_build, builder.koji_session): + log.info(" Processing the paused module build %r", module_build) + further_work = module_build_service.utils.start_next_batch_build( + config, module_build, session, builder) + for event in further_work: + log.info(" Scheduling faked event %r" % event) + module_build_service.scheduler.consumer.work_queue_put(event) # Check if we have met the threshold. if module_build_service.utils.at_concurrent_component_threshold(config, session): @@ -319,8 +316,6 @@ class MBSProducer(PollingProducer): ) taginfo = koji_session.getTag(module_build.koji_tag + "-build") module_build.new_repo_task_id = koji_session.newRepo(taginfo["name"]) - else: - module_build.new_repo_task_id = 0 session.commit() @@ -496,3 +491,18 @@ class MBSProducer(PollingProducer): build.state_reason += " (Error occured while querying Greenwave)" build.time_modified = datetime.utcnow() session.commit() + + +def _has_missed_new_repo_message(module_build, koji_session): + """ + Returns whether or not a new repo message has probably been missed. + """ + if not module_build.new_repo_task_id: + # A newRepo task has incorrectly not been requested. Treat it as a missed + # message so module build can recover. + return True + log.debug( + 'Checking status of newRepo task "%d" for %s', module_build.new_repo_task_id, module_build) + task_info = koji_session.getTaskInfo(module_build.new_repo_task_id) + # Other final sates, FAILED and CANCELED, are handled by retrigger_new_repo_on_failure + return task_info["state"] == koji.TASK_STATES["CLOSED"] diff --git a/tests/test_build/test_build.py b/tests/test_build/test_build.py index 925db2c..3f42fdc 100644 --- a/tests/test_build/test_build.py +++ b/tests/test_build/test_build.py @@ -116,6 +116,7 @@ class FakeModuleBuilder(GenericBuilder): on_buildroot_add_artifacts_cb = None on_tag_artifacts_cb = None on_buildroot_add_repos_cb = None + on_get_task_info_cb = None @module_build_service.utils.validate_koji_tag("tag_name") def __init__(self, owner, module, config, tag_name, components): @@ -237,6 +238,8 @@ class FakeModuleBuilder(GenericBuilder): return 123 session.newRepo = _newRepo + if self.on_get_task_info_cb: + session.getTaskInfo = self.on_get_task_info_cb return session @property @@ -459,6 +462,10 @@ class TestBuild(BaseTestBuild): def on_tag_artifacts_cb(cls, artifacts, dest_tag=True): assert tag_groups.pop(0) == set(artifacts) + def on_get_task_info_cb(cls, task_id): + return {"state": koji.TASK_STATES["CLOSED"]} + + FakeModuleBuilder.on_get_task_info_cb = on_get_task_info_cb FakeModuleBuilder.on_finalize_cb = on_finalize_cb FakeModuleBuilder.on_tag_artifacts_cb = on_tag_artifacts_cb diff --git a/tests/test_scheduler/test_poller.py b/tests/test_scheduler/test_poller.py index 0951a04..04c242f 100644 --- a/tests/test_scheduler/test_poller.py +++ b/tests/test_scheduler/test_poller.py @@ -105,6 +105,60 @@ class TestPoller: assert len(start_build_component.mock_calls) == expected_build_calls + @pytest.mark.parametrize('task_state, expect_start_build_component', ( + (None, True), # Indicates a newRepo task has not been triggered yet. + (koji.TASK_STATES["CLOSED"], True), + (koji.TASK_STATES["OPEN"], False), + )) + @patch("module_build_service.utils.batches.start_build_component") + def test_process_paused_module_builds_with_new_repo_task( + self, start_build_component, create_builder, global_consumer, dbg, task_state, + expect_start_build_component + ): + """ + Tests general use-case of process_paused_module_builds. + """ + consumer = mock.MagicMock() + consumer.incoming = queue.Queue() + global_consumer.return_value = consumer + + builder = mock.MagicMock() + create_builder.return_value = builder + + # Change the batch to 2, so the module build is in state where + # it is not building anything, but the state is "build". + module_build = models.ModuleBuild.query.get(3) + module_build.batch = 2 + module_build.time_modified = datetime.utcnow() - timedelta(days=5) + if task_state: + koji_session = mock.MagicMock() + koji_session.getTaskInfo.return_value = {"state": task_state} + builder.koji_session = koji_session + module_build.new_repo_task_id = 123 + db.session.commit() + + # Poll :) + hub = mock.MagicMock() + poller = MBSProducer(hub) + poller.poll() + + # Refresh our module_build object. + module_build = models.ModuleBuild.query.get(3) + db.session.refresh(module_build) + + if expect_start_build_component: + expected_state = koji.BUILD_STATES["BUILDING"] + expected_build_calls = 2 + else: + expected_state = None + expected_build_calls = 0 + + components = module_build.current_batch() + for component in components: + assert component.state == expected_state + + assert len(start_build_component.mock_calls) == expected_build_calls + @patch.dict("sys.modules", krbV=mock.MagicMock()) @patch("module_build_service.builder.KojiModuleBuilder.KojiClientSession") def test_trigger_new_repo_when_failed( @@ -178,7 +232,7 @@ class TestPoller: db.session.refresh(module_build) assert not koji_session.newRepo.called - assert module_build.new_repo_task_id == 0 + assert module_build.new_repo_task_id == 123456 def test_process_paused_module_builds_waiting_for_repo( self, create_builder, global_consumer, dbg diff --git a/tests/test_scheduler/test_tag_tagged.py b/tests/test_scheduler/test_tag_tagged.py index 0c3484d..afa4aac 100644 --- a/tests/test_scheduler/test_tag_tagged.py +++ b/tests/test_scheduler/test_tag_tagged.py @@ -31,6 +31,7 @@ from tests import reuse_component_init_data from tests import conf, db import koji +import pytest class TestTagTagged: @@ -504,3 +505,114 @@ class TestTagTagged: # newRepo task_id should be stored in database, so we can check its # status later in poller. assert module_build.new_repo_task_id == 123456 + + @pytest.mark.parametrize('task_state, expect_new_repo', ( + (None, True), # Indicates a newRepo task has not been triggered yet. + (koji.TASK_STATES["CLOSED"], True), + (koji.TASK_STATES["CANCELED"], True), + (koji.TASK_STATES["FAILED"], True), + (koji.TASK_STATES["FREE"], False), + (koji.TASK_STATES["OPEN"], False), + (koji.TASK_STATES["ASSIGNED"], False), + )) + @patch( + "module_build_service.builder.GenericBuilder.default_buildroot_groups", + return_value={"build": [], "srpm-build": []}, + ) + @patch("module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session") + @patch("module_build_service.builder.GenericBuilder.create_from_module") + def test_newrepo_not_duplicated( + self, create_builder, koji_get_session, dbg, task_state, expect_new_repo + ): + """ + Test that newRepo is not called if a task is already in progress. + """ + koji_session = mock.MagicMock() + koji_session.getTag = lambda tag_name: {"name": tag_name} + koji_session.getTaskInfo.return_value = {"state": task_state} + koji_session.newRepo.return_value = 123456 + koji_get_session.return_value = koji_session + + builder = mock.MagicMock() + builder.koji_session = koji_session + builder.buildroot_ready.return_value = False + builder.module_build_tag = { + "name": "module-testmodule-master-20170219191323-c40c156c-build" + } + create_builder.return_value = builder + + module_build = module_build_service.models.ModuleBuild.query.get(3) + assert module_build + + # Set previous components as COMPLETE and tagged. + module_build.batch = 1 + for c in module_build.up_to_current_batch(): + c.state = koji.BUILD_STATES["COMPLETE"] + c.tagged = True + c.tagged_in_final = True + + module_build.batch = 2 + for c in module_build.current_batch(): + if c.package == "perl-Tangerine": + c.nvr = "perl-Tangerine-0.23-1.module+0+d027b723" + elif c.package == "perl-List-Compare": + c.nvr = "perl-List-Compare-0.53-5.module+0+d027b723" + c.state = koji.BUILD_STATES["COMPLETE"] + + if task_state is not None: + module_build.new_repo_task_id = 123456 + + db.session.commit() + + # Tag the first component to the buildroot. + msg = module_build_service.messaging.KojiTagChange( + "id", + "module-testmodule-master-20170219191323-c40c156c-build", + "perl-Tangerine", + "perl-Tangerine-0.23-1.module+0+d027b723", + ) + module_build_service.scheduler.handlers.tags.tagged( + config=conf, session=db.session, msg=msg) + # Tag the first component to the final tag. + msg = module_build_service.messaging.KojiTagChange( + "id", + "module-testmodule-master-20170219191323-c40c156c", + "perl-Tangerine", + "perl-Tangerine-0.23-1.module+0+d027b723", + ) + module_build_service.scheduler.handlers.tags.tagged( + config=conf, session=db.session, msg=msg) + # Tag the second component to the buildroot. + msg = module_build_service.messaging.KojiTagChange( + "id", + "module-testmodule-master-20170219191323-c40c156c-build", + "perl-List-Compare", + "perl-List-Compare-0.53-5.module+0+d027b723", + ) + module_build_service.scheduler.handlers.tags.tagged( + config=conf, session=db.session, msg=msg) + # Tag the second component to the final tag. + msg = module_build_service.messaging.KojiTagChange( + "id", + "module-testmodule-master-20170219191323-c40c156c", + "perl-List-Compare", + "perl-List-Compare-0.53-5.module+0+d027b723", + ) + module_build_service.scheduler.handlers.tags.tagged( + config=conf, session=db.session, msg=msg) + + # All components are tagged, newRepo should be called if there are no active tasks. + if expect_new_repo: + koji_session.newRepo.assert_called_once_with( + "module-testmodule-master-20170219191323-c40c156c-build") + else: + assert not koji_session.newRepo.called + + # Refresh our module_build object. + db.session.expunge(module_build) + module_build = module_build_service.models.ModuleBuild.query.filter_by(id=3).one() + + # newRepo task_id should be stored in database, so we can check its + # status later in poller. + assert module_build.new_repo_task_id == 123456 + koji_session.newRepo.reset_mock()