From f5de140521cdc425d3975203f9943ce091076d26 Mon Sep 17 00:00:00 2001 From: mprahl Date: Oct 23 2017 19:33:57 +0000 Subject: Schedule components based on build time --- diff --git a/module_build_service/builder/KojiModuleBuilder.py b/module_build_service/builder/KojiModuleBuilder.py index 78f9cfe..184203d 100644 --- a/module_build_service/builder/KojiModuleBuilder.py +++ b/module_build_service/builder/KojiModuleBuilder.py @@ -753,3 +753,13 @@ chmod 644 %buildroot/%_sysconfdir/rpm/macros.zz-modules tasks.append(task) return tasks + + def get_average_build_time(self, component): + """ + Get the average build time of the component from Koji + :param component: a ComponentBuild object + :return: a float of the average build time in seconds + """ + # If the component has not been built before, then None is returned. Instead, let's + # return 0.0 so the type is consistent + return self.koji_session.getAverageBuildDuration(component.package) or 0.0 diff --git a/module_build_service/builder/MockModuleBuilder.py b/module_build_service/builder/MockModuleBuilder.py index af2515e..3dff301 100644 --- a/module_build_service/builder/MockModuleBuilder.py +++ b/module_build_service/builder/MockModuleBuilder.py @@ -93,6 +93,7 @@ class MockModuleBuilder(GenericBuilder): self.config = config self.groups = [] self.yum_conf = MockModuleBuilder.yum_config_template + self.koji_session = None # Auto-detect arch (if possible) or fallback to the configured one if conf.arch_autodetect: @@ -539,3 +540,23 @@ class SCMBuilder(BaseBuilder): if source.startswith(host): return cmds raise KeyError("No defined commands for {}".format(source)) + + def get_average_build_time(self, component): + """ + Get the average build time of the component from Koji + :param component: a ComponentBuild object + :return: a float of the average build time in seconds + """ + # We currently don't track build times in MBS directly, so we can use Koji to get a decent + # estimate + if not self.koji_session: + # If Koji is not configured on the system, then just return 0.0 for components + try: + self.koji_session = KojiModuleBuilder.get_session(self.config, self.owner) + # If the component has not been built before, then None is returned. Instead, + # let's return 0.0 so the type is consistent + return self.koji_session.getAverageBuildDuration(component.package) or 0.0 + except: + log.debug('The Koji call to getAverageBuildDuration failed. Is Koji properly ' + 'configured?') + return 0.0 diff --git a/module_build_service/builder/base.py b/module_build_service/builder/base.py index 7d474d5..88104e0 100644 --- a/module_build_service/builder/base.py +++ b/module_build_service/builder/base.py @@ -312,3 +312,13 @@ class GenericBuilder(six.with_metaclass(ABCMeta)): for component builds. """ raise NotImplementedError() + + @classmethod + def get_average_build_time(self, component): + """ + Placeholder function for the builders to report the average time it takes to build the + specified component. If this function is not overridden, then 0.0 is returned. + :param component: a ComponentBuild object + :return: a float of 0.0 + """ + return 0.0 diff --git a/module_build_service/utils.py b/module_build_service/utils.py index 6417e8b..0b811c8 100644 --- a/module_build_service/utils.py +++ b/module_build_service/utils.py @@ -161,6 +161,12 @@ def continue_batch_build(config, module, session, builder, components=None): # threshold further_work = [] components_to_build = [] + # Sort the unbuilt_components so that the components that take the longest to build are + # first + log.info('Sorting the unbuilt components by their average build time') + unbuilt_components.sort(key=lambda c: builder.get_average_build_time(c), reverse=True) + log.info('Done sorting the unbuilt components by their average build time') + for c in unbuilt_components: # Check the concurrent build threshold. if at_concurrent_component_threshold(config, session): diff --git a/tests/test_scheduler/test_repo_done.py b/tests/test_scheduler/test_repo_done.py index 5259d99..212eeff 100644 --- a/tests/test_scheduler/test_repo_done.py +++ b/tests/test_scheduler/test_repo_done.py @@ -58,12 +58,14 @@ class TestRepoDone(unittest.TestCase): module_build_service.scheduler.handlers.repos.done( config=conf, session=db.session, msg=msg) + @mock.patch('module_build_service.builder.KojiModuleBuilder.get_average_build_time', + return_value=0.0) @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) @mock.patch('module_build_service.builder.KojiModuleBuilder.get_session') @mock.patch('module_build_service.builder.KojiModuleBuilder.build') @mock.patch('module_build_service.builder.KojiModuleBuilder.buildroot_connect') - def test_a_single_match(self, connect, build_fn, get_session, ready, list_tasks_fn): + def test_a_single_match(self, connect, build_fn, get_session, ready, list_tasks_fn, mock_gabt): """ Test that when a repo msg hits us and we have a single match. """ get_session.return_value = mock.Mock(), 'development' @@ -77,12 +79,15 @@ class TestRepoDone(unittest.TestCase): artifact_name='communicator', source='git://pkgs.domain.local/rpms/communicator?#da95886c8a443b36a9ce31abda1f9bed22f2f9c2') + @mock.patch('module_build_service.builder.KojiModuleBuilder.get_average_build_time', + return_value=0.0) @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) @mock.patch('module_build_service.builder.KojiModuleBuilder.get_session') @mock.patch('module_build_service.builder.KojiModuleBuilder.build') @mock.patch('module_build_service.builder.KojiModuleBuilder.buildroot_connect') - def test_a_single_match_build_fail(self, connect, build_fn, config, ready, list_tasks_fn): + def test_a_single_match_build_fail(self, connect, build_fn, config, ready, list_tasks_fn, + mock_gabt): """ Test that when a KojiModuleBuilder.build fails, the build is marked as failed with proper state_reason. """ diff --git a/tests/test_utils/test_utils.py b/tests/test_utils/test_utils.py index 14e67a4..2becfc5 100644 --- a/tests/test_utils/test_utils.py +++ b/tests/test_utils/test_utils.py @@ -813,6 +813,42 @@ class TestBatches(unittest.TestCase): mock_sbc.assert_called_once() @patch('module_build_service.utils.start_build_component') + def test_start_next_batch_build_smart_scheduling(self, mock_sbc, default_buildroot_groups): + """ + Tests that components with the longest build time will be scheduled first + """ + module_build = models.ModuleBuild.query.filter_by(id=2).one() + module_build.batch = 1 + pt_component = models.ComponentBuild.query.filter_by( + module_id=2, package='perl-Tangerine').one() + pt_component.ref = '6ceea46add2366d8b8c5a623b2fb563b625bfabe' + plc_component = models.ComponentBuild.query.filter_by( + module_id=2, package='perl-List-Compare').one() + plc_component.ref = '5ceea46add2366d8b8c5a623a2fb563b625b9abd' + + builder = mock.MagicMock() + # The call order of get_average_build_time should be by the component's ID. Having this + # side_effect tells continue_batch_build to build the second component in the build batch + # first and the first component in the build batch second. + builder.get_average_build_time.side_effect = [1234.56, 2345.67] + 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 don't have any messages returned since no components should be reused + self.assertEqual(len(further_work), 0) + # Make sure both components are set to the build state but not reused + self.assertEqual(pt_component.state, koji.BUILD_STATES['BUILDING']) + self.assertIsNone(pt_component.reused_component_id) + self.assertEqual(plc_component.state, koji.BUILD_STATES['BUILDING']) + self.assertIsNone(plc_component.reused_component_id) + # Test the order of the scheduling + expected_calls = [mock.call(builder, plc_component), mock.call(builder, pt_component)] + self.assertEqual(mock_sbc.mock_calls, expected_calls) + + @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