From 780ed117b443162e700a8c53bdf600edae8657ce Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Mon, 25 Mar 2019 14:54:17 +0100 Subject: [PATCH] Find compatible base modules based on the virtual streams. Before this commit, the compatible base modules for Module Stream Expansion have been found without any limitation, just based on the stream version. It was therefore possible that `platform:lp29.0.0` was found as compatible module for `platform:f29.1.0` although those platform streams are not compatible at all. In this commit, the module can be treated as compatible only if it has the same virtual stream as the input module. The idea behind this is that both `platform:f29.0.0` and `platform:f29.1.0` should include the `virtual_streams: [f29]` in their XMD section which tells MBS that they are actually compatible. The `lp29` stream will not have the same virtual stream (most likely it won't have any virtual stream at all). The `virtual_streams` is already used for this use-case in `MMDResolver`, but it was not used to limit the inputs to `MMDResolver` which is what this commit is doing. This commit also fixes the issue in `get_last_builds_in_stream_version_lte` which was simply broken if multiple stream_versions of single base module existed and their builds had different version. In this case, only builds with single (randomly chosen) version were returned. --- module_build_service/models.py | 40 +++++++++++++++++++---------- module_build_service/utils/mse.py | 41 ++++++++++++++++++++++++++++++ tests/__init__.py | 13 +++++++++- tests/test_models/test_models.py | 24 ++++++++++++++++- tests/test_utils/test_utils_mse.py | 36 +++++++++++++++++++++++--- tests/test_views/test_views.py | 13 ++++++---- 6 files changed, 142 insertions(+), 25 deletions(-) diff --git a/module_build_service/models.py b/module_build_service/models.py index 8fa6a80a..1fe0326d 100644 --- a/module_build_service/models.py +++ b/module_build_service/models.py @@ -358,21 +358,33 @@ class ModuleBuild(MBSBase): # the minimal `stream_version` is 280000. min_stream_version = (stream_version // 10000) * 10000 - # Prepare the subquery to find out all unique name:stream records. - subq = session.query( - func.max(sqlalchemy.cast(ModuleBuild.version, db.BigInteger)).label("maxversion") - ).filter_by(name=name, state=BUILD_STATES["ready"]).filter( - stream_version <= stream_version).filter( - stream_version >= min_stream_version).subquery('t2') + query = session.query(ModuleBuild)\ + .filter(ModuleBuild.name == name)\ + .filter(ModuleBuild.state == BUILD_STATES["ready"])\ + .filter(ModuleBuild.stream_version <= stream_version)\ + .filter(ModuleBuild.stream_version >= min_stream_version)\ + .order_by(ModuleBuild.version.desc()) + builds = query.all() - # Use the subquery to actually return all the columns for its results. - query = session.query(ModuleBuild).join( - subq, and_( - ModuleBuild.name == name, - ModuleBuild.stream_version <= stream_version, - ModuleBuild.stream_version >= min_stream_version, - sqlalchemy.cast(ModuleBuild.version, db.BigInteger) == subq.c.maxversion)) - return query.all() + # In case there are multiple versions of single name:stream build, we want to return + # the latest version only. The `builds` are ordered by "version" desc, so we + # can just get the first (greatest) version of name:stream. + # TODO: Is there a way how to do that nicely in the SQL query itself? + seen = {} # {"n:s": v, ...} + ret = [] + for build in builds: + ns = "%s:%s" % (build.name, build.stream) + if ns in seen and seen[ns] != build.version: + # Skip the builds if we already handled this nsv before. + continue + elif ns in seen and seen[ns] == build.version: + # Different context of the NSV + ret.append(build) + continue + + seen[ns] = build.version + ret.append(build) + return ret @staticmethod def get_build_by_koji_tag(session, tag): diff --git a/module_build_service/utils/mse.py b/module_build_service/utils/mse.py index caa86555..0a75db1f 100644 --- a/module_build_service/utils/mse.py +++ b/module_build_service/utils/mse.py @@ -218,11 +218,52 @@ def _get_base_module_mmds(mmd): if ns in seen: continue + # Get the MMD for particular buildrequired name:stream to find out the virtual + # streams according to which we can find the compatible streams later. + # The returned MMDs are all the module builds for name:stream in the highest + # version. Given the base module does not depend on other modules, it can appear + # only in single context and therefore the `mmds` should always contain just + # zero or one module build. + mmds = resolver.get_module_modulemds(name, stream) + if not mmds: + continue + stream_mmd = mmds[0] + + # In case there are no virtual_streams in the buildrequired name:stream, + # it is clear that there are no compatible streams, so return just this + # `stream_mmd`. + xmd = stream_mmd.get_xmd() + if "mbs" not in xmd.keys() or "virtual_streams" not in xmd["mbs"].keys(): + seen.add(ns) + ret.append(stream_mmd) + continue + + # Get the list of compatible virtual streams. + virtual_streams = xmd["mbs"]["virtual_streams"] + mmds = resolver.get_module_modulemds(name, stream, stream_version_lte=True) 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_name(), mmd_.get_stream()]) + xmd = mmd_.get_xmd() + if "mbs" not in xmd.keys() or "virtual_streams" not in xmd["mbs"].keys(): + # This module does not contain any virtual stream, so it cannot + # be compatible with our buildrequired module. + continue + + # Check if some virtual stream from buildrequired module exists in + # this module. That would mean the modules are compatible. + mmd_virtual_streams = xmd["mbs"]["virtual_streams"] + for virtual_stream in virtual_streams: + if virtual_stream in mmd_virtual_streams: + break + else: + # No stream from `virtual_streams` exist in `mmd_virtual_streams`, so this + # module is not compatible with our buildrequired module. + continue + mmd_ns = ":".join([mmd_.get_name(), mmd_.get_stream()]) # An extra precaution to ensure there are no duplicates in the event the sorting # above wasn't flawless diff --git a/tests/__init__.py b/tests/__init__.py index 8352a2e9..e9f43042 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -126,7 +126,14 @@ def init_data(data_size=10, contexts=False, multiple_stream_versions=False, scra for stream in ["f28.0.0", "f29.0.0", "f29.1.0", "f29.2.0"]: mmd.set_name("platform") mmd.set_stream(stream) + + # Set the virtual_streams based on "fXY" to mark the platform streams + # with the same major stream_version compatible. + xmd = glib.from_variant_dict(mmd.get_xmd()) + xmd['mbs']['virtual_streams'] = [stream[:3]] + mmd.set_xmd(glib.dict_values(xmd)) import_mmd(db.session, mmd) + # Just to possibly confuse tests by adding another base module. mmd.set_name("bootstrap") mmd.set_stream(stream) @@ -724,7 +731,7 @@ def reuse_shared_userspace_init_data(): def make_module(nsvc, requires_list=None, build_requires_list=None, base_module=None, - filtered_rpms=None, xmd=None, store_to_db=True): + filtered_rpms=None, xmd=None, store_to_db=True, virtual_streams=None): """ Creates new models.ModuleBuild defined by `nsvc` string with requires and buildrequires set according to ``requires_list`` and ``build_requires_list``. @@ -742,6 +749,7 @@ def make_module(nsvc, requires_list=None, build_requires_list=None, base_module= default key/value pairs are added if not present. :param bool store_to_db: whether to store created module metadata to the database. + :param list virtual_streams: List of virtual streams provided by this module. :return: New Module Build if set to store module metadata to database, otherwise the module metadata is returned. :rtype: ModuleBuild or Modulemd.Module @@ -795,6 +803,9 @@ def make_module(nsvc, requires_list=None, build_requires_list=None, base_module= if 'mse' not in xmd_mbs: xmd_mbs['mse'] = 'true' + if virtual_streams: + xmd_mbs['virtual_streams'] = virtual_streams + mmd.set_xmd(glib.dict_values(xmd)) if not store_to_db: diff --git a/tests/test_models/test_models.py b/tests/test_models/test_models.py index 2b1c9863..e98d19e2 100644 --- a/tests/test_models/test_models.py +++ b/tests/test_models/test_models.py @@ -24,7 +24,7 @@ import os from module_build_service.utils import to_text_type from tests.test_models import init_data, module_build_from_modulemd -from tests import (init_data as init_data_contexts, clean_database) +from tests import (init_data as init_data_contexts, clean_database, make_module) from module_build_service import conf, Modulemd from module_build_service.models import ComponentBuild, ModuleBuild, make_session @@ -142,3 +142,25 @@ class TestModelsGetStreamsContexts: builds = set(["%s:%s:%s:%s" % (build.name, build.stream, str(build.version), build.context) for build in builds]) assert builds == set(['platform:f29.0.0:3:00000000', 'platform:f29.1.0:3:00000000']) + + def test_get_last_builds_in_stream_version_lte_different_versions(self): + """ + Tests that get_last_builds_in_stream_version_lte works in case the + name:stream_ver modules have different versions. + """ + clean_database(False) + make_module("platform:f29.1.0:10:old_version", {}, {}, virtual_streams=["f29"]) + make_module("platform:f29.1.0:15:c11.another", {}, {}, virtual_streams=["f29"]) + make_module("platform:f29.1.0:15:c11", {}, {}, virtual_streams=["f29"]) + make_module("platform:f29.2.0:0:old_version", {}, {}, virtual_streams=["f29"]) + make_module("platform:f29.2.0:1:c11", {}, {}, virtual_streams=["f29"]) + make_module("platform:f29.3.0:15:old_version", {}, {}, virtual_streams=["f29"]) + make_module("platform:f29.3.0:20:c11", {}, {}, virtual_streams=["f29"]) + + with make_session(conf) as session: + builds = ModuleBuild.get_last_builds_in_stream_version_lte( + session, "platform", 290200) + builds = set(["%s:%s:%s:%s" % (build.name, build.stream, str(build.version), + build.context) for build in builds]) + assert builds == set(['platform:f29.1.0:15:c11', 'platform:f29.1.0:15:c11.another', + 'platform:f29.2.0:1:c11']) diff --git a/tests/test_utils/test_utils_mse.py b/tests/test_utils/test_utils_mse.py index e82ccd40..49bc78e5 100644 --- a/tests/test_utils/test_utils_mse.py +++ b/tests/test_utils/test_utils_mse.py @@ -337,9 +337,9 @@ class TestUtilsModuleStreamExpansion: and lorem:1 modules which require base:f29 module requiring platform:f29 module :). """ - f290000 = make_module("platform:f29.0.0:0:c11", {}, {}) - f290100 = make_module("platform:f29.1.0:0:c11", {}, {}) - f290200 = make_module("platform:f29.2.0:0:c11", {}, {}) + f290000 = make_module("platform:f29.0.0:0:c11", {}, {}, virtual_streams=["f29"]) + f290100 = make_module("platform:f29.1.0:0:c11", {}, {}, virtual_streams=["f29"]) + f290200 = make_module("platform:f29.2.0:0:c11", {}, {}, virtual_streams=["f29"]) make_module("gtk:1:0:c2", {"platform": ["f29"]}, {}, f290000) make_module("gtk:1:1:c2", {"platform": ["f29"]}, {}, f290100) make_module("gtk:1:2:c2", {"platform": ["f29"]}, {}, f290100) @@ -362,7 +362,7 @@ class TestUtilsModuleStreamExpansion: os.path.join(base_dir, 'staged_data', 'testmodule_v2.yaml'), True) deps = mmd.get_dependencies() brs = deps[0].get_buildrequires() - brs['platform'].set(['platform:f29.1.0', 'platform:f29.2.0']) + brs['platform'].set(['f29.1.0', 'f29.2.0']) deps[0].set_buildrequires(brs) mmd.set_dependencies(deps) @@ -375,3 +375,31 @@ class TestUtilsModuleStreamExpansion: for mmd_ in mmds: actual.add('{}:{}'.format(mmd_.get_name(), mmd_.get_stream())) assert actual == expected + + @pytest.mark.parametrize('virtual_streams', (None, ["f29"], ["lp29"])) + def test__get_base_module_mmds_virtual_streams(self, virtual_streams): + """Ensure the correct results are returned without duplicates.""" + init_data(data_size=1, multiple_stream_versions=True) + mmd = module_build_service.utils.load_mmd( + os.path.join(base_dir, 'staged_data', 'testmodule_v2.yaml'), True) + deps = mmd.get_dependencies() + brs = deps[0].get_buildrequires() + brs['platform'].set(['f29.2.0']) + deps[0].set_buildrequires(brs) + mmd.set_dependencies(deps) + + make_module("platform:lp29.1.1:12:c11", {}, {}, virtual_streams=virtual_streams) + + mmds = module_build_service.utils.mse._get_base_module_mmds(mmd) + if virtual_streams == ["f29"]: + expected = set(['platform:f29.0.0', 'platform:f29.1.0', 'platform:f29.2.0', + 'platform:lp29.1.1']) + 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) + # Verify the expected ones were returned + actual = set() + for mmd_ in mmds: + actual.add('{}:{}'.format(mmd_.get_name(), mmd_.get_stream())) + assert actual == expected diff --git a/tests/test_views/test_views.py b/tests/test_views/test_views.py index db5b9739..8ec579a0 100644 --- a/tests/test_views/test_views.py +++ b/tests/test_views/test_views.py @@ -1289,6 +1289,9 @@ class TestViews: @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM') def test_submit_build_invalid_basemodule_stream(self, mocked_scm, mocked_get_user): + # By default tests do not provide platform:f28.0.0, but just platform:f28. + # Therefore we want to enable multiple_stream_versions. + init_data(2, multiple_stream_versions=True) FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml', '620ec77321b2ea7b0d67d82992dda3e1d67055b4') @@ -1302,12 +1305,12 @@ class TestViews: rv = self.client.post('/module-build-service/1/module-builds/', data=json.dumps(data)) result = json.loads(rv.data) assert result == { - 'error': 'Bad Request', - 'status': 400, - 'message': ('No dependency combination was satisfied. Please verify the ' - 'buildrequires in your modulemd have previously been built.') + 'error': 'Unprocessable Entity', + 'status': 422, + 'message': ('None of the base module (platform) streams in the buildrequires ' + 'section could be found') } - assert rv.status_code == 400 + assert rv.status_code == 422 @patch('module_build_service.auth.get_user', return_value=user) @patch('module_build_service.scm.SCM')