From 95febc2e1bc16d11ac5c73ffaa444709fca4fad1 Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Thu, 20 Jun 2019 09:05:03 +0200 Subject: [PATCH 1/2] Allow modules built against platform in 'garbage' state to be used as build dependency. --- module_build_service/models.py | 5 +- module_build_service/resolver/DBResolver.py | 61 ++++++++++++++------ module_build_service/resolver/MBSResolver.py | 44 +++++++++++--- module_build_service/resolver/base.py | 10 +++- module_build_service/utils/mse.py | 55 +++++++++++------- tests/test_resolver/test_db.py | 10 ++-- tests/test_resolver/test_mbs.py | 12 ++-- tests/test_utils/test_utils_mse.py | 33 +++++++---- 8 files changed, 157 insertions(+), 73 deletions(-) diff --git a/module_build_service/models.py b/module_build_service/models.py index b3fae068..80c2f16a 100644 --- a/module_build_service/models.py +++ b/module_build_service/models.py @@ -460,7 +460,7 @@ class ModuleBuild(MBSBase): @staticmethod def get_last_builds_in_stream_version_lte( - session, name, stream_version=None, virtual_streams=None): + session, name, stream_version=None, virtual_streams=None, states=None): """ Returns the latest builds in "ready" state for given name:stream limited by `stream_version`. The `stream_version` is int generated by `get_stream_version(...)` @@ -475,10 +475,11 @@ class ModuleBuild(MBSBase): :param list virtual_streams: A list of the virtual streams to filter on. The filtering uses "or" logic. When falsy, no filtering occurs. """ + states = states or [BUILD_STATES["ready"]] query = ( session.query(ModuleBuild) .filter(ModuleBuild.name == name) - .filter(ModuleBuild.state == BUILD_STATES["ready"]) + .filter(ModuleBuild.state.in_(states)) .order_by(sqlalchemy.cast(ModuleBuild.version, db.BigInteger).desc()) ) diff --git a/module_build_service/resolver/DBResolver.py b/module_build_service/resolver/DBResolver.py index ad62bb32..ba6f59ed 100644 --- a/module_build_service/resolver/DBResolver.py +++ b/module_build_service/resolver/DBResolver.py @@ -93,8 +93,6 @@ class DBResolver(GenericResolver): version=None, context=None, strict=False, - stream_version_lte=False, - virtual_streams=None, ): """ Gets the module modulemds from the resolver. @@ -106,11 +104,6 @@ class DBResolver(GenericResolver): be returned. :kwarg strict: Normally this function returns [] if no module can be found. If strict=True, then a UnprocessableEntity is raised. - :kwarg stream_version_lte: If True and if the `stream` can be transformed to - "stream version", the returned list will include all the modules with stream version - less than or equal the stream version computed from `stream`. - :kwarg virtual_streams: a list of the virtual streams to filter on. The filtering uses "or" - logic. When falsy, no filtering occurs. :return: List of Modulemd metadata instances matching the query """ if version and context: @@ -121,17 +114,7 @@ class DBResolver(GenericResolver): with models.make_session(self.config) as session: if not version and not context: - if stream_version_lte and ( - len(str(models.ModuleBuild.get_stream_version(stream, right_pad=False))) >= 5 - ): - stream_version = models.ModuleBuild.get_stream_version(stream) - builds = models.ModuleBuild.get_last_builds_in_stream_version_lte( - session, name, stream_version, virtual_streams) - elif not stream_version_lte and virtual_streams: - builds = models.ModuleBuild.get_last_builds_in_stream_version_lte( - session, name, None, virtual_streams) - else: - builds = models.ModuleBuild.get_last_builds_in_stream(session, name, stream) + builds = models.ModuleBuild.get_last_builds_in_stream(session, name, stream) else: raise NotImplementedError( "This combination of name/stream/version/context is not implemented") @@ -141,6 +124,48 @@ class DBResolver(GenericResolver): "Cannot find any module builds for %s:%s" % (name, stream)) return [build.mmd() for build in builds] + def get_compatible_base_module_modulemds( + self, name, stream, stream_version_lte, virtual_streams, states + ): + """ + Returns the Modulemd metadata of base modules compatible with base module + defined by `name` and `stream`. The compatibility is found out using the + stream version in case the stream is in "x.y.z" format and is limited to + single major version of stream version. + + If `virtual_streams` are defined, the compatibility is also extended to + all base module streams which share the same virtual stream. + + :param name: Name of the base module. + :param stream: Stream of the base module. + :param stream_version_lte: If True, the compatible streams are limited + by the stream version computed from `stream`. If False, even the + modules with higher stream version are returned. + :param virtual_streams: List of virtual streams. If set, also modules + with incompatible stream version are returned in case they share + one of the virtual streams. + :param states: List of states the returned compatible modules should + be in. + """ + builds = [] + with models.make_session(self.config) as session: + if stream_version_lte: + stream_in_xyz_format = len(str(models.ModuleBuild.get_stream_version( + stream, right_pad=False))) >= 5 + if not stream_in_xyz_format: + log.warning( + "Cannot get compatible base modules, because stream_version_let is used, " + "but stream %r is not in x.y.z format." % stream) + return [] + stream_version = models.ModuleBuild.get_stream_version(stream) + builds = models.ModuleBuild.get_last_builds_in_stream_version_lte( + session, name, stream_version, virtual_streams, states) + else: + builds = models.ModuleBuild.get_last_builds_in_stream_version_lte( + session, name, None, virtual_streams, states) + + return [build.mmd() for build in builds] + def get_buildrequired_modulemds(self, name, stream, base_module_nsvc): """ Returns modulemd metadata of all module builds with `name` and `stream` buildrequiring diff --git a/module_build_service/resolver/MBSResolver.py b/module_build_service/resolver/MBSResolver.py index 3d8bda1d..c27633cc 100644 --- a/module_build_service/resolver/MBSResolver.py +++ b/module_build_service/resolver/MBSResolver.py @@ -46,7 +46,7 @@ class MBSResolver(GenericResolver): self.mbs_prod_url = config.mbs_url self._generic_error = "Failed to query MBS with query %r returned HTTP status %s" - def _query_from_nsvc(self, name, stream, version=None, context=None, state="ready"): + def _query_from_nsvc(self, name, stream, version=None, context=None, states=None): """ Generates dict with MBS params query. @@ -55,10 +55,11 @@ class MBSResolver(GenericResolver): :param str version/int: Version of the module to query. :param str context: Context of the module to query. """ + states = states or ["ready"] query = { "name": name, "stream": stream, - "state": state, + "state": states, "verbose": True, "order_desc_by": "version", } @@ -69,7 +70,7 @@ class MBSResolver(GenericResolver): return query def _get_modules( - self, name, stream, version=None, context=None, state="ready", strict=False, **kwargs + self, name, stream, version=None, context=None, states=None, strict=False, **kwargs ): """Query and return modules from MBS with specific info @@ -85,7 +86,7 @@ class MBSResolver(GenericResolver): :rtype: list[dict] :raises UnprocessableEntity: if no modules are found and ``strict`` is True. """ - query = self._query_from_nsvc(name, stream, version, context, state) + query = self._query_from_nsvc(name, stream, version, context, states) query["page"] = 1 query["per_page"] = 10 query.update(kwargs) @@ -118,8 +119,8 @@ class MBSResolver(GenericResolver): else: return modules - def get_module(self, name, stream, version, context, state="ready", strict=False): - rv = self._get_modules(name, stream, version, context, state, strict) + def get_module(self, name, stream, version, context, states=None, strict=False): + rv = self._get_modules(name, stream, version, context, states, strict) if rv: return rv[0] @@ -173,6 +174,7 @@ class MBSResolver(GenericResolver): strict=False, stream_version_lte=False, virtual_streams=None, + states=None, ): """ Gets the module modulemds from the resolver. @@ -207,7 +209,8 @@ class MBSResolver(GenericResolver): if virtual_streams: extra_args["virtual_stream"] = virtual_streams - modules = self._get_modules(name, stream, version, context, strict=strict, **extra_args) + modules = self._get_modules(name, stream, version, context, strict=strict, states=states, + **extra_args) if not modules: return [] @@ -226,6 +229,33 @@ class MBSResolver(GenericResolver): mmds.append(load_mmd(yaml)) return mmds + def get_compatible_base_module_modulemds( + self, name, stream, stream_version_lte, virtual_streams, states + ): + """ + Returns the Modulemd metadata of base modules compatible with base module + defined by `name` and `stream`. The compatibility is found out using the + stream version in case the stream is in "x.y.z" format and is limited to + single major version of stream version. + + If `virtual_streams` are defined, the compatibility is also extended to + all base module streams which share the same virtual stream. + + :param name: Name of the base module. + :param stream: Stream of the base module. + :param stream_version_lte: If True, the compatible streams are limited + by the stream version computed from `stream`. If False, even the + modules with higher stream version are returned. + :param virtual_streams: List of virtual streams. If set, also modules + with incompatible stream version are returned in case they share + one of the virtual streams. + :param states: List of states the returned compatible modules should + be in. + """ + return self.get_module_modulemds( + name, stream, stream_version_lte=stream_version_lte, virtual_streams=virtual_streams, + states=states) + def get_buildrequired_modulemds(self, name, stream, base_module_nsvc): """ Returns modulemd metadata of all module builds with `name` and `stream` buildrequiring diff --git a/module_build_service/resolver/base.py b/module_build_service/resolver/base.py index d1074a1a..7f8dbb6a 100644 --- a/module_build_service/resolver/base.py +++ b/module_build_service/resolver/base.py @@ -117,9 +117,13 @@ class GenericResolver(six.with_metaclass(ABCMeta)): stream, version=None, context=None, - strict=False, - stream_version_lte=None, - virtual_streams=None, + strict=False + ): + raise NotImplementedError() + + @abstractmethod + def get_compatible_base_module_modulemds( + self, name, stream, stream_version_lte, virtual_streams, states ): raise NotImplementedError() diff --git a/module_build_service/utils/mse.py b/module_build_service/utils/mse.py index 1ba956e5..b9b427ec 100644 --- a/module_build_service/utils/mse.py +++ b/module_build_service/utils/mse.py @@ -211,11 +211,12 @@ def _get_base_module_mmds(mmd): old versions of the base module based on the stream version. :param Modulemd mmd: Input modulemd metadata. - :rtype: list of Modulemd - :return: List of MMDs of base modules buildrequired by `mmd`. + :rtype: dict with lists of Modulemd + :return: Dict with "ready" or "garbage" state name as a key and list of MMDs of base modules + buildrequired by `mmd` as a value. """ seen = set() - ret = [] + ret = {"ready": [], "garbage": []} resolver = module_build_service.resolver.system_resolver for deps in mmd.get_dependencies(): @@ -258,30 +259,34 @@ def _get_base_module_mmds(mmd): # `stream_mmd`. if not virtual_streams: seen.add(ns) - ret.append(stream_mmd) + ret["ready"].append(stream_mmd) continue virtual_streams = xmd["mbs"]["virtual_streams"] if conf.allow_only_compatible_base_modules: stream_version_lte = True + states = ["ready"] else: stream_version_lte = False + states = ["ready", "garbage"] - mmds = resolver.get_module_modulemds( - name, stream, stream_version_lte=stream_version_lte, - virtual_streams=virtual_streams) - ret_chunk = [] - # Add the returned mmds to the `seen` set to avoid querying those individually if - # they are part of the buildrequire streams for this base module - for mmd_ in mmds: - mmd_ns = ":".join([mmd_.get_module_name(), mmd_.get_stream_name()]) - # An extra precaution to ensure there are no duplicates in the event the sorting - # above wasn't flawless - if mmd_ns not in seen: - ret_chunk.append(mmd_) - seen.add(mmd_ns) - ret += ret_chunk + for state in states: + mmds = resolver.get_compatible_base_module_modulemds( + name, stream, stream_version_lte, virtual_streams, + [models.BUILD_STATES[state]]) + ret_chunk = [] + # Add the returned mmds to the `seen` set to avoid querying those + # individually if they are part of the buildrequire streams for this + # base module. + for mmd_ in mmds: + mmd_ns = ":".join([mmd_.get_module_name(), mmd_.get_stream_name()]) + # An extra precaution to ensure there are no duplicates in the event the + # sorting above wasn't flawless + if mmd_ns not in seen: + ret_chunk.append(mmd_) + seen.add(mmd_ns) + ret[state] += ret_chunk # Just in case it was queried for but MBS didn't find it seen.add(ns) break @@ -322,7 +327,7 @@ def get_mmds_required_by_module_recursively( # Get the MMDs of all compatible base modules based on the buildrequires. base_module_mmds = _get_base_module_mmds(mmd) - if not base_module_mmds: + if not base_module_mmds["ready"]: base_module_choices = " or ".join(conf.base_module_names) raise UnprocessableEntity( "None of the base module ({}) streams in the buildrequires section could be found" @@ -330,16 +335,22 @@ def get_mmds_required_by_module_recursively( ) # Add base modules to `mmds`. - for base_module in base_module_mmds: + for base_module in base_module_mmds["ready"]: ns = ":".join([base_module.get_module_name(), base_module.get_stream_name()]) mmds.setdefault(ns, []) mmds[ns].append(base_module) + # The currently submitted module build must be build only against "ready" base modules, + # but its dependencies might have been built against some old platform which is already + # EOL ("garbage" state). In order to find such old module builds, we need to include + # also EOL platform streams. + all_base_module_mmds = base_module_mmds["ready"] + base_module_mmds["garbage"] + # Get all the buildrequires of the module of interest. for deps in mmd.get_dependencies(): deps_dict = deps_to_dict(deps, 'buildtime') mmds = _get_mmds_from_requires( - deps_dict, mmds, False, default_streams, raise_if_stream_ambigous, base_module_mmds) + deps_dict, mmds, False, default_streams, raise_if_stream_ambigous, all_base_module_mmds) # Now get the requires of buildrequires recursively. for mmd_key in list(mmds.keys()): @@ -352,7 +363,7 @@ def get_mmds_required_by_module_recursively( True, default_streams, raise_if_stream_ambigous, - base_module_mmds, + all_base_module_mmds, ) # Make single list from dict of lists. diff --git a/tests/test_resolver/test_db.py b/tests/test_resolver/test_db.py index 5180b4ea..0ca873c7 100644 --- a/tests/test_resolver/test_db.py +++ b/tests/test_resolver/test_db.py @@ -78,16 +78,18 @@ class TestDBModule: assert nsvcs == set(["testmodule:master:20170109091357:123"]) @pytest.mark.parametrize("stream_versions", [False, True]) - def test_get_module_modulemds_stream_versions(self, stream_versions): + def test_get_compatible_base_module_modulemds_stream_versions(self, stream_versions): tests.init_data(1, multiple_stream_versions=True) resolver = mbs_resolver.GenericResolver.create(tests.conf, backend="db") - result = resolver.get_module_modulemds( - "platform", "f29.1.0", stream_version_lte=stream_versions) + result = resolver.get_compatible_base_module_modulemds( + "platform", "f29.1.0", stream_version_lte=stream_versions, virtual_streams=["f29"], + states=[models.BUILD_STATES["ready"]]) nsvcs = set([mmd.get_nsvc() for mmd in result]) if stream_versions: assert nsvcs == set(["platform:f29.1.0:3:00000000", "platform:f29.0.0:3:00000000"]) else: - assert nsvcs == set(["platform:f29.1.0:3:00000000"]) + assert nsvcs == set(["platform:f29.1.0:3:00000000", "platform:f29.0.0:3:00000000", + "platform:f29.2.0:3:00000000"]) @pytest.mark.parametrize("empty_buildrequires", [False, True]) def test_get_module_build_dependencies(self, empty_buildrequires, db_session): diff --git a/tests/test_resolver/test_mbs.py b/tests/test_resolver/test_mbs.py index 24048a04..ff022886 100644 --- a/tests/test_resolver/test_mbs.py +++ b/tests/test_resolver/test_mbs.py @@ -73,7 +73,7 @@ class TestMBSModule: "order_desc_by": "version", "page": 1, "per_page": 10, - "state": "ready", + "state": ["ready"], "virtual_stream": ["f28"], } mock_session.get.assert_called_once_with(mbs_url, params=expected_query) @@ -129,7 +129,7 @@ class TestMBSModule: "order_desc_by": "version", "page": 1, "per_page": 10, - "state": "ready", + "state": ["ready"], } mock_session.get.assert_called_once_with(mbs_url, params=expected_query) assert nsvcs == expected @@ -187,7 +187,7 @@ class TestMBSModule: "order_desc_by": "version", "page": 1, "per_page": 10, - "state": "ready", + "state": ["ready"], }, { "name": "platform", @@ -198,7 +198,7 @@ class TestMBSModule: "order_desc_by": "version", "page": 1, "per_page": 10, - "state": "ready", + "state": ["ready"], }, ] @@ -260,7 +260,7 @@ class TestMBSModule: "order_desc_by": "version", "page": 1, "per_page": 10, - "state": "ready", + "state": ["ready"], } mock_session.get.assert_called_once_with(mbs_url, params=expected_query) assert set(result) == expected @@ -336,7 +336,7 @@ class TestMBSModule: "order_desc_by": "version", "page": 1, "per_page": 10, - "state": "ready", + "state": ["ready"], } mock_session.get.assert_called_once_with(mbs_url, params=expected_query) diff --git a/tests/test_utils/test_utils_mse.py b/tests/test_utils/test_utils_mse.py index d693a628..449cf496 100644 --- a/tests/test_utils/test_utils_mse.py +++ b/tests/test_utils/test_utils_mse.py @@ -22,7 +22,7 @@ from mock import patch, PropertyMock import pytest import module_build_service.utils -from module_build_service import Modulemd +from module_build_service import Modulemd, models from module_build_service.errors import StreamAmbigous from tests import db, clean_database, make_module, init_data, read_staged_data @@ -418,10 +418,10 @@ class TestUtilsModuleStreamExpansion: mmds = module_build_service.utils.mse._get_base_module_mmds(mmd) expected = set(["platform:f29.0.0", "platform:f29.1.0", "platform:f29.2.0"]) # Verify no duplicates were returned before doing set operations - assert len(mmds) == len(expected) + assert len(mmds["ready"]) == len(expected) # Verify the expected ones were returned actual = set() - for mmd_ in mmds: + for mmd_ in mmds["ready"]: actual.add("{}:{}".format(mmd_.get_module_name(), mmd_.get_stream_name())) assert actual == expected @@ -447,10 +447,10 @@ class TestUtilsModuleStreamExpansion: else: expected = set(["platform:f29.0.0", "platform:f29.1.0", "platform:f29.2.0"]) # Verify no duplicates were returned before doing set operations - assert len(mmds) == len(expected) + assert len(mmds["ready"]) == len(expected) # Verify the expected ones were returned actual = set() - for mmd_ in mmds: + for mmd_ in mmds["ready"]: actual.add("{}:{}".format(mmd_.get_module_name(), mmd_.get_stream_name())) assert actual == expected @@ -461,6 +461,13 @@ class TestUtilsModuleStreamExpansion: def test__get_base_module_mmds_virtual_streams_only_major_versions(self, cfg): """Ensure the correct results are returned without duplicates.""" init_data(data_size=1, multiple_stream_versions=["foo28", "foo29", "foo30"]) + + # Mark platform:foo28 as garbage to test that it is still considered as compatible. + platform = models.ModuleBuild.query.filter_by(name="platform", stream="foo28").first() + platform.state = "garbage" + db.session.add(platform) + db.session.commit() + mmd = module_build_service.utils.load_mmd(read_staged_data("testmodule_v2")) deps = mmd.get_dependencies()[0] new_deps = Modulemd.Dependencies() @@ -471,12 +478,16 @@ class TestUtilsModuleStreamExpansion: mmd.add_dependencies(new_deps) mmds = module_build_service.utils.mse._get_base_module_mmds(mmd) - expected = set(["platform:foo28", "platform:foo29", "platform:foo30"]) + expected = {} + expected["ready"] = set(["platform:foo29", "platform:foo30"]) + expected["garbage"] = set(["platform:foo28"]) # Verify no duplicates were returned before doing set operations assert len(mmds) == len(expected) - # Verify the expected ones were returned - actual = set() - for mmd_ in mmds: - actual.add("{}:{}".format(mmd_.get_module_name(), mmd_.get_stream_name())) - assert actual == expected + for k in expected.keys(): + assert len(mmds[k]) == len(expected[k]) + # Verify the expected ones were returned + actual = set() + for mmd_ in mmds[k]: + actual.add("{}:{}".format(mmd_.get_module_name(), mmd_.get_stream_name())) + assert actual == expected[k] From 8301aeb9eb461b4637b241b22de4a3690e9dd862 Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Tue, 9 Jul 2019 09:51:12 +0200 Subject: [PATCH 2/2] Return the latest base module in case stream_version_lte is used, but stream is not in x.y.z format. This seems to be better behaviour than simply rejecting the module build completely. MBS still shows warning in the log that it cannot find any compatible module, but the build continues with the base module requested in the submitted modulemd. --- module_build_service/resolver/DBResolver.py | 25 +++++++-------------- module_build_service/resolver/base.py | 9 +------- module_build_service/utils/mse.py | 2 +- 3 files changed, 10 insertions(+), 26 deletions(-) diff --git a/module_build_service/resolver/DBResolver.py b/module_build_service/resolver/DBResolver.py index ba6f59ed..e1f23d4a 100644 --- a/module_build_service/resolver/DBResolver.py +++ b/module_build_service/resolver/DBResolver.py @@ -86,14 +86,7 @@ class DBResolver(GenericResolver): if module: return load_mmd(module.modulemd) - def get_module_modulemds( - self, - name, - stream, - version=None, - context=None, - strict=False, - ): + def get_module_modulemds(self, name, stream, version=None, context=None, strict=False): """ Gets the module modulemds from the resolver. :param name: a string of the module's name @@ -149,20 +142,18 @@ class DBResolver(GenericResolver): """ builds = [] with models.make_session(self.config) as session: + stream_version = None if stream_version_lte: stream_in_xyz_format = len(str(models.ModuleBuild.get_stream_version( stream, right_pad=False))) >= 5 - if not stream_in_xyz_format: + if stream_in_xyz_format: + stream_version = models.ModuleBuild.get_stream_version(stream) + else: log.warning( - "Cannot get compatible base modules, because stream_version_let is used, " + "Cannot get compatible base modules, because stream_version_lte is used, " "but stream %r is not in x.y.z format." % stream) - return [] - stream_version = models.ModuleBuild.get_stream_version(stream) - builds = models.ModuleBuild.get_last_builds_in_stream_version_lte( - session, name, stream_version, virtual_streams, states) - else: - builds = models.ModuleBuild.get_last_builds_in_stream_version_lte( - session, name, None, virtual_streams, states) + builds = models.ModuleBuild.get_last_builds_in_stream_version_lte( + session, name, stream_version, virtual_streams, states) return [build.mmd() for build in builds] diff --git a/module_build_service/resolver/base.py b/module_build_service/resolver/base.py index 7f8dbb6a..db4e0606 100644 --- a/module_build_service/resolver/base.py +++ b/module_build_service/resolver/base.py @@ -111,14 +111,7 @@ class GenericResolver(six.with_metaclass(ABCMeta)): raise NotImplementedError() @abstractmethod - def get_module_modulemds( - self, - name, - stream, - version=None, - context=None, - strict=False - ): + def get_module_modulemds(self, name, stream, version=None, context=None, strict=False): raise NotImplementedError() @abstractmethod diff --git a/module_build_service/utils/mse.py b/module_build_service/utils/mse.py index b9b427ec..20740391 100644 --- a/module_build_service/utils/mse.py +++ b/module_build_service/utils/mse.py @@ -340,7 +340,7 @@ def get_mmds_required_by_module_recursively( mmds.setdefault(ns, []) mmds[ns].append(base_module) - # The currently submitted module build must be build only against "ready" base modules, + # The currently submitted module build must be built only against "ready" base modules, # but its dependencies might have been built against some old platform which is already # EOL ("garbage" state). In order to find such old module builds, we need to include # also EOL platform streams.