From 1487fd01f2608d9dd42a1181f82d0611ed753c33 Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Wed, 22 Mar 2017 17:29:05 +0100 Subject: [PATCH] Remove is_waiting_for_repo_regen and use buildroot_ready instead. Do not get buildroot dependencies from the buildrequires recursively. Do not validate Koji tags to inherit. --- module_build_service/builder.py | 27 +------------ module_build_service/pdc.py | 68 +++++++++++++-------------------- module_build_service/utils.py | 7 ++-- tests/test_build/test_build.py | 3 -- tests/test_pdc.py | 16 ++++++++ tests/test_utils/test_utils.py | 8 +--- 6 files changed, 50 insertions(+), 79 deletions(-) diff --git a/module_build_service/builder.py b/module_build_service/builder.py index a766ea5c..cb6a4bbc 100644 --- a/module_build_service/builder.py +++ b/module_build_service/builder.py @@ -332,12 +332,6 @@ class GenericBuilder(six.with_metaclass(ABCMeta)): """ raise NotImplementedError() - @abstractmethod - def is_waiting_for_repo_regen(self): - """ - :return: True if there is repo regeneration pending for a module. - """ - raise NotImplementedError() class KojiModuleBuilder(GenericBuilder): """ Koji specific builder class """ @@ -378,20 +372,6 @@ class KojiModuleBuilder(GenericBuilder): return "" % ( self.module_str, self.tag_name) - def is_waiting_for_repo_regen(self): - """ - Returns true when there is 'newRepo' task for our tag. - """ - tasks = [] - state = [koji.TASK_STATES[s] for s in ('FREE', 'OPEN', 'ASSIGNED')] - for task in self.koji_session.listTasks(opts={'state': state, - 'decode': True, - 'method': 'newRepo'}): - tag = task['request'][0] - if tag == self.tag_name: - return True - return False - @module_build_service.utils.retry(wait_on=(IOError, koji.GenericError)) def buildroot_ready(self, artifacts=None): """ @@ -750,7 +730,7 @@ chmod 644 %buildroot/%_rpmconfigdir/macros.d/macros.modules raise SystemError("Unknown tag: %s" % tag) return taginfo - @module_build_service.utils.validate_koji_tag(['tag_name', 'parent_tags'], post='') + @module_build_service.utils.validate_koji_tag(['tag_name'], post='') def _koji_add_many_tag_inheritance(self, tag_name, parent_tags): tag = self._get_tag(tag_name) # highest priority num is at the end @@ -1079,9 +1059,6 @@ class CoprModuleBuilder(GenericBuilder): def list_tasks_for_components(self, component_builds=None, state='active'): pass - def is_waiting_for_repo_regen(self): - return False - def build(self, artifact_name, source): """ :param artifact_name : A package name. We can't guess it since macros @@ -1579,8 +1556,6 @@ mdpolicy=group:primary def list_tasks_for_components(self, component_builds=None, state='active'): pass - def is_waiting_for_repo_regen(self): - return True def build_from_scm(artifact_name, source, config, build_srpm, diff --git a/module_build_service/pdc.py b/module_build_service/pdc.py index 2cc6c9a8..8c5b0d61 100644 --- a/module_build_service/pdc.py +++ b/module_build_service/pdc.py @@ -274,54 +274,40 @@ def module_depsolving_wrapper(session, modules, strict=True): """ log.debug("module_depsolving_wrapper(%r, strict=%r)" % (modules, strict)) - # We're going to modify this list, so don't screw with the passed in value. - module_list = copy.deepcopy(modules) - # Name this, just for readability... - original_modules = modules - - # Keep track of which modules we've queried for before, to avoid that. - seen = list() - # This is the set we're going to build up and return. module_tags = set() - while not all([d in seen for d in module_list]): - for module_dict in module_list: - # Make sure not to spam PDC with unnecessary queries. - if module_dict in seen: - continue - seen.append(module_dict) + # We want to take only single + for module_dict in modules: + # Get enhanced info on this module from pdc. + info = get_module(session, module_dict, strict) - # Get enhanced info on this module from pdc. - info = get_module(session, module_dict, strict) + # Take note of the tag of this module, but only if it is a dep and + # not in the original list. + # XXX - But, for now go ahead and include it because that's how this + # code used to work. + module_tags.add(info['koji_tag']) - # Take note of the tag of this module, but only if it is a dep and - # not in the original list. - #if module_dict not in original_modules: - # module_tags.add(info['koji_tag']) - # XXX - But, for now go ahead and include it because that's how this - # code used to work. + # Now, when we look for the deps of this module, use the mmd.xmd + # attributes because they contain the promise of *exactly* which + # versions of which deps we say we're going to build *this* module + # against. + if not info['modulemd']: + raise ValueError("No PDC modulemd found for %r" % info) + mmd = _extract_modulemd(info['modulemd']) + + # Queue up the next tier of deps that we should look at.. + for name, details in mmd.xmd['mbs']['buildrequires'].items(): + modified_dep = { + 'name': name, + 'version': details['stream'], + 'release': details['version'], + # Only return details about module builds that finished + 'active': True, + } + info = get_module(session, modified_dep, strict) module_tags.add(info['koji_tag']) - # Now, when we look for the deps of this module, use the mmd.xmd - # attributes because they contain the promise of *exactly* which - # versions of which deps we say we're going to build *this* module - # against. - if not info['modulemd']: - raise ValueError("No PDC modulemd found for %r" % info) - mmd = _extract_modulemd(info['modulemd']) - - # Queue up the next tier of deps that we should look at.. - for name, details in mmd.xmd['mbs']['buildrequires'].items(): - modified_dep = { - 'name': name, - 'version': details['stream'], - 'release': details['version'], - # Only return details about module builds that finished - 'active': True, - } - module_list.append(modified_dep) - return list(module_tags) diff --git a/module_build_service/utils.py b/module_build_service/utils.py index f4533877..3dd30517 100644 --- a/module_build_service/utils.py +++ b/module_build_service/utils.py @@ -290,9 +290,10 @@ def start_next_batch_build(config, module, session, builder, components=None): # Find out if there is repo regeneration in progress for this module. # If there is, wait until the repo is regenerated before starting a new # batch. - if builder.is_waiting_for_repo_regen(): - log.debug("Not starting new batch, repo regeneration is in progress " - "for module %s" % module) + artifacts = [c.nvr for c in module.current_batch()] + if not builder.buildroot_ready(artifacts): + log.info("Not starting new batch, not all of %r are in the buildroot. " + "Waiting." % artifacts) return [] module.batch += 1 diff --git a/tests/test_build/test_build.py b/tests/test_build/test_build.py index 300529bd..3c5fb232 100644 --- a/tests/test_build/test_build.py +++ b/tests/test_build/test_build.py @@ -145,9 +145,6 @@ class TestModuleBuilder(GenericBuilder): if TestModuleBuilder.on_tag_artifacts_cb: TestModuleBuilder.on_tag_artifacts_cb(self, artifacts) - def is_waiting_for_repo_regen(self): - return False - @property def module_build_tag(self): return {"name": self.tag_name + "-build"} diff --git a/tests/test_pdc.py b/tests/test_pdc.py index 769ce213..3e2fa0af 100644 --- a/tests/test_pdc.py +++ b/tests/test_pdc.py @@ -68,6 +68,22 @@ class TestPDCModule(unittest.TestCase): ] self.assertEqual(set(result), set(expected)) + def test_get_module_depsolving_wrapper_recursive(self): + query = [{ + 'name': 'testmodule', + 'version': 'master', + 'release': '20170322155247', + 'active': False, + }] + result = mbs_pdc.module_depsolving_wrapper(self.pdc, query) + expected = [ + u'module-base-runtime-master-20170315134803', + # Should the list of deps should not include the original tag? + # Probably not. + u'module-testmodule-master-20170322155247' + ] + self.assertEqual(set(result), set(expected)) + def test_resolve_profiles(self): current_dir = os.path.dirname(__file__) yaml_path = os.path.join( diff --git a/tests/test_utils/test_utils.py b/tests/test_utils/test_utils.py index 55e9ffe3..c5cee88f 100644 --- a/tests/test_utils/test_utils.py +++ b/tests/test_utils/test_utils.py @@ -396,9 +396,6 @@ class DummyModuleBuilder(GenericBuilder): def tag_artifacts(self, artifacts): pass - def is_waiting_for_repo_regen(self): - return False - @property def module_build_tag(self): return {"name": self.tag_name + "-build"} @@ -446,7 +443,6 @@ class TestBatches(unittest.TestCase): module_build.batch = 1 builder = mock.MagicMock() - builder.is_waiting_for_repo_regen.return_value = False further_work = module_build_service.utils.start_next_batch_build( conf, module_build, db.session, builder) @@ -505,13 +501,13 @@ class TestBatches(unittest.TestCase): def test_start_next_batch_build_repo_building(self, default_buildroot_groups): """ Test that start_next_batch_build does not start new batch when - builder.is_waiting_for_repo_regen() returns True. + builder.buildroot_ready() returns False. """ module_build = models.ModuleBuild.query.filter_by(id=2).one() module_build.batch = 1 builder = mock.MagicMock() - builder.is_waiting_for_repo_regen.return_value = True + builder.buildroot_ready.return_value = False further_work = module_build_service.utils.start_next_batch_build( conf, module_build, db.session, builder)