From 8e41f9f3350b09eb1e30004ff91c33405f1b43cd Mon Sep 17 00:00:00 2001 From: mprahl Date: Thu, 19 Oct 2017 17:12:31 -0400 Subject: [PATCH] Try to reuse all components in the batch before starting it --- module_build_service/utils.py | 67 ++++++++++++++++++++--------- tests/test_scheduler/test_poller.py | 4 +- tests/test_utils/test_utils.py | 55 ++++++++++++++++++++--- 3 files changed, 98 insertions(+), 28 deletions(-) diff --git a/module_build_service/utils.py b/module_build_service/utils.py index 6c895307..6417e8b5 100644 --- a/module_build_service/utils.py +++ b/module_build_service/utils.py @@ -156,30 +156,20 @@ def continue_batch_build(config, module, session, builder, components=None): log.debug("Cannot continue building module %s. No component to build." % module) return [] - # Get the list of components to be build in this batch. We are not - # building all `unbuilt_components`, because we can a) meet - # the num_concurrent_builds threshold or b) reuse previous build. + # Get the list of components to be built in this batch. We are not building + # all `unbuilt_components`, because we can meet the num_concurrent_builds + # threshold further_work = [] components_to_build = [] for c in unbuilt_components: - # Check to see if we can reuse a previous component build - # instead of rebuilding it - previous_component_build = get_reusable_component( - session, module, c.package) - # If a component build can't be reused, we need to check - # the concurrent threshold. - if (not previous_component_build and - at_concurrent_component_threshold(config, session)): + # Check the concurrent build threshold. + if at_concurrent_component_threshold(config, session): log.info('Concurrent build threshold met') break - if previous_component_build: - further_work += reuse_component(c, previous_component_build) - continue - - # We set state to BUILDING here, because we are going to build the - # component anyway and at_concurrent_component_threshold() works - # by counting components in BUILDING state. + # We set state to "BUILDING" here because at this point we are committed + # to build the component and at_concurrent_component_threshold() works by + # counting the number of components in the "BUILDING" state. c.state = koji.BUILD_STATES['BUILDING'] components_to_build.append(c) @@ -229,6 +219,9 @@ def start_next_batch_build(config, module, session, builder, components=None): has_unbuilt_components_in_batch = False has_building_components_in_batch = False has_failed_components = False + # This is used to determine if it's worth checking if a component can be reused + # later on in the code + all_reused_in_prev_batch = True for c in module.component_builds: if c.state in [None, koji.BUILD_STATES['BUILDING']]: has_unbuilt_components = True @@ -242,6 +235,9 @@ def start_next_batch_build(config, module, session, builder, components=None): koji.BUILD_STATES['CANCELED']]): has_failed_components = True + if c.batch == module.batch and not c.reused_component_id: + all_reused_in_prev_batch = False + # Do not start new batch if there are no components to build. if not has_unbuilt_components: log.debug("Not starting new batch, there is no component to build " @@ -294,6 +290,8 @@ def start_next_batch_build(config, module, session, builder, components=None): "Waiting." % artifacts) return [] + # Although this variable isn't necessary, it is easier to read code later on with it + prev_batch = module.batch module.batch += 1 # The user can either pass in a list of components to 'seed' the batch, or @@ -317,8 +315,37 @@ def start_next_batch_build(config, module, session, builder, components=None): log.info("Starting build of next batch %d, %s" % (module.batch, unbuilt_components)) - return continue_batch_build( - config, module, session, builder, unbuilt_components) + # Attempt to reuse any components possible in the batch before attempting to build any + further_work = [] + # Try to figure out if it's even worth checking if the components can be + # reused by checking to see if the previous batch had all their builds + # reused except for when the previous batch was 1 because that always + # has the module-build-macros component built + unbuilt_components_after_reuse = [] + components_reused = False + if prev_batch == 1 or all_reused_in_prev_batch: + for c in unbuilt_components: + previous_component_build = get_reusable_component( + session, module, c.package) + + if previous_component_build: + components_reused = True + further_work += reuse_component(c, previous_component_build) + else: + unbuilt_components_after_reuse.append(c) + # Commit the changes done by reuse_component + if components_reused: + session.commit() + + # If all the components were reused in the batch then make a KojiRepoChange + # message and return + if components_reused and not unbuilt_components_after_reuse: + further_work.append(module_build_service.messaging.KojiRepoChange( + 'start_build_batch: fake msg', builder.module_build_tag['name'])) + return further_work + + return further_work + continue_batch_build( + config, module, session, builder, unbuilt_components_after_reuse) def pagination_metadata(p_query, request_args): diff --git a/tests/test_scheduler/test_poller.py b/tests/test_scheduler/test_poller.py index d8ae55b3..2f1d031e 100644 --- a/tests/test_scheduler/test_poller.py +++ b/tests/test_scheduler/test_poller.py @@ -47,7 +47,8 @@ class TestPoller(unittest.TestCase): def tearDown(self): init_data() - def test_process_paused_module_builds(self, create_builder, + @patch('module_build_service.utils.start_build_component') + def test_process_paused_module_builds(self, start_build_component, create_builder, koji_get_session, global_consumer, dbg): """ @@ -82,6 +83,7 @@ class TestPoller(unittest.TestCase): components = module_build.current_batch() for component in components: self.assertEqual(component.state, koji.BUILD_STATES["BUILDING"]) + self.assertEqual(len(start_build_component.mock_calls), 2) def test_trigger_new_repo_when_failed(self, create_builder, koji_get_session, global_consumer, diff --git a/tests/test_utils/test_utils.py b/tests/test_utils/test_utils.py index a7067c9f..14e67a46 100644 --- a/tests/test_utils/test_utils.py +++ b/tests/test_utils/test_utils.py @@ -772,7 +772,48 @@ class TestBatches(unittest.TestCase): # Check that packages have been tagged just once. self.assertEqual(len(DummyModuleBuilder.TAGGED_COMPONENTS), 2) - def test_start_next_batch_continue(self, default_buildroot_groups): + @patch('module_build_service.utils.start_build_component') + def test_start_next_batch_build_reuse_some(self, mock_sbc, default_buildroot_groups): + """ + Tests that start_next_batch_build: + 1) Increments module.batch. + 2) Can reuse all components in the batch that it can. + 3) Returns proper further_work messages for reused components. + 4) Builds the remaining components + 5) Handling the further_work messages lead to proper tagging of + reused components. + """ + module_build = models.ModuleBuild.query.filter_by(id=2).one() + module_build.batch = 1 + plc_component = models.ComponentBuild.query.filter_by( + module_id=2, package='perl-List-Compare').one() + plc_component.ref = '5ceea46add2366d8b8c5a623a2fb563b625b9abd' + + builder = mock.MagicMock() + further_work = module_build_service.utils.start_next_batch_build( + conf, module_build, db.session, builder) + + # Batch number should increase. + self.assertEqual(module_build.batch, 2) + + # Make sure we only have one message returned for the one reused component + self.assertEqual(len(further_work), 1) + # The KojiBuildChange message in further_work should have build_new_state + # set to COMPLETE, but the current component build state in the DB should be set + # to BUILDING, so KojiBuildChange message handler handles the change + # properly. + self.assertEqual(further_work[0].build_new_state, koji.BUILD_STATES['COMPLETE']) + component_build = models.ComponentBuild.from_component_event(db.session, further_work[0]) + self.assertEqual(component_build.state, koji.BUILD_STATES['BUILDING']) + self.assertEqual(component_build.package, 'perl-Tangerine') + self.assertIsNotNone(component_build.reused_component_id) + # Make sure perl-List-Compare is set to the build state as well but not reused + self.assertEqual(plc_component.state, koji.BUILD_STATES['BUILDING']) + self.assertIsNone(plc_component.reused_component_id) + mock_sbc.assert_called_once() + + @patch('module_build_service.utils.start_build_component') + def test_start_next_batch_continue(self, mock_sbc, default_buildroot_groups): """ Tests that start_next_batch_build does not start new batch when there are unbuilt components in the current one. @@ -780,9 +821,10 @@ class TestBatches(unittest.TestCase): module_build = models.ModuleBuild.query.filter_by(id=2).one() module_build.batch = 2 - # Mark the component as BUILDING. + # The component was reused when the batch first started building_component = module_build.current_batch()[0] building_component.state = koji.BUILD_STATES['BUILDING'] + building_component.reused_component_id = 123 db.session.commit() builder = mock.MagicMock() @@ -791,11 +833,10 @@ class TestBatches(unittest.TestCase): # Batch number should not increase. self.assertEqual(module_build.batch, 2) - - # Single component should be reused this time, second message is fake - # KojiRepoChange. - self.assertEqual(len(further_work), 2) - self.assertEqual(further_work[0].build_name, "perl-List-Compare") + # Make sure start build was called for the second component which wasn't reused + mock_sbc.assert_called_once() + # No further work should be returned + self.assertEqual(len(further_work), 0) def test_start_next_batch_build_repo_building(self, default_buildroot_groups): """