From 7761bbf5b41cea3800d171440915f5b64d4d1c88 Mon Sep 17 00:00:00 2001 From: "Owen W. Taylor" Date: Fri, 20 Nov 2020 15:49:14 -0500 Subject: [PATCH] Tweak behavior of get_compatible_base_module_modulemds Fix some oddities in the DBResolver implementation of get_compatible_base_module_modulemds() and make the MBSResolver version - which was previously just buggy - match that. (Tests for the MBSResolver version are added in a subsequent commit.) * If an empty virtual_streams argument was passed in, *all* streams were considered compatible. Throw an exception in this case - it should be considered an error. * If stream_version_lte=True, but the stream from the base module wasn't in the form FOOx.y.z, then throw an exception. This was previously treated like stream_version_lte=False, which is just a recipe for confusion and mistakes. test_get_reusable_module_use_latest_build() is rewritten to comprehensively test all possibilities, including the case that changed above. test_add_default_modules_compatible_platforms() is changed to run under allow_only_compatible_base_modules=False, since it expected Fedora-style virtual streams (versions not in FOOx.y.z form, all share the same stream), which doesn't make sense with allow_only_compatible_base_modules=True. --- module_build_service/common/errors.py | 4 + module_build_service/resolver/DBResolver.py | 25 +-- module_build_service/resolver/MBSResolver.py | 114 ++++++------- tests/test_resolver/test_db.py | 33 +++- tests/test_scheduler/test_default_modules.py | 7 + tests/test_scheduler/test_reuse.py | 158 ++++++++++++------- 6 files changed, 222 insertions(+), 119 deletions(-) diff --git a/module_build_service/common/errors.py b/module_build_service/common/errors.py index 44946e06..8f116761 100644 --- a/module_build_service/common/errors.py +++ b/module_build_service/common/errors.py @@ -21,6 +21,10 @@ class UnprocessableEntity(ValueError): pass +class StreamNotXyz(UnprocessableEntity): + pass + + class Conflict(ValueError): pass diff --git a/module_build_service/resolver/DBResolver.py b/module_build_service/resolver/DBResolver.py index 47ae1a9d..0be8d156 100644 --- a/module_build_service/resolver/DBResolver.py +++ b/module_build_service/resolver/DBResolver.py @@ -6,7 +6,7 @@ import sqlalchemy from sqlalchemy.orm import aliased from module_build_service.common import log, models -from module_build_service.common.errors import UnprocessableEntity +from module_build_service.common.errors import StreamNotXyz, UnprocessableEntity from module_build_service.common.utils import load_mmd from module_build_service.resolver.base import GenericResolver @@ -111,26 +111,31 @@ class DBResolver(GenericResolver): :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 virtual_streams: List of virtual streams. + Modules are returned if they share one of the virtual streams. :param states: List of states the returned compatible modules should be in. :return list: List of Modulemd objects. """ + if not virtual_streams: + raise RuntimeError("Virtual stream list must not be empty") + name = base_module_mmd.get_module_name() stream = base_module_mmd.get_stream_name() builds = [] + stream_version = None if stream_version_lte: stream_in_xyz_format = len(str(models.ModuleBuild.get_stream_version( stream, right_pad=False))) >= 5 - if stream_in_xyz_format: - stream_version = models.ModuleBuild.get_stream_version(stream) - else: - log.warning( - "Cannot get compatible base modules, because stream_version_lte is used, " - "but stream %r is not in x.y.z format." % stream) + if not stream_in_xyz_format: + raise StreamNotXyz( + "Cannot get compatible base modules, because stream of resolved " + "base module %s:%s is not in x.y.z format." % ( + base_module_mmd.get_module_name(), stream + )) + stream_version = models.ModuleBuild.get_stream_version(stream) + builds = models.ModuleBuild.get_last_builds_in_stream_version_lte( self.db_session, name, stream_version, virtual_streams, states) diff --git a/module_build_service/resolver/MBSResolver.py b/module_build_service/resolver/MBSResolver.py index 3cd8dce9..714fdfe0 100644 --- a/module_build_service/resolver/MBSResolver.py +++ b/module_build_service/resolver/MBSResolver.py @@ -9,7 +9,7 @@ import kobo.rpmlib from module_build_service.common import models from module_build_service.common.config import conf -from module_build_service.common.errors import UnprocessableEntity +from module_build_service.common.errors import StreamNotXyz, UnprocessableEntity from module_build_service.common.request_utils import requests_session from module_build_service.common.utils import load_mmd, import_mmd from module_build_service.resolver.KojiResolver import KojiResolver @@ -38,11 +38,12 @@ class MBSResolver(KojiResolver): states = states or ["ready"] query = { "name": name, - "stream": stream, "state": states, "verbose": True, "order_desc_by": "version", } + if stream is not None: + query["stream"] = stream if version is not None: query["version"] = str(version) if context is not None: @@ -145,55 +146,11 @@ class MBSResolver(KojiResolver): if data["items"]: return load_mmd(data["items"][0]["modulemd"]) - def get_module_modulemds( - self, - name, - stream, - version=None, - context=None, - strict=False, - stream_version_lte=False, - virtual_streams=None, - states=None, - ): - """ - Gets the module modulemds from the resolver. - :param name: a string of the module's name - :param stream: a string of the module's stream - :param version: a string or int of the module's version. When None, latest version will - be returned. - :param context: a string of the module's context. When None, all contexts will - 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 - """ - yaml = None - - local_modules = models.ModuleBuild.local_modules(self.db_session, name, stream) - if local_modules: - return [m.mmd() for m in local_modules] - - extra_args = {} - 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) - extra_args["stream_version_lte"] = stream_version - - if virtual_streams: - extra_args["virtual_stream"] = virtual_streams - - modules = self._get_modules(name, stream, version, context, strict=strict, states=states, - **extra_args) + def _modules_to_modulemds(self, modules, strict): if not modules: return [] + yaml = None mmds = [] for module in modules: if module: @@ -209,6 +166,34 @@ class MBSResolver(KojiResolver): mmds.append(load_mmd(yaml)) return mmds + 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 + :param stream: a string of the module's stream + :param version: a string or int of the module's version. When None, latest version will + be returned. + :param context: a string of the module's context. When None, all contexts will + be returned. + :kwarg strict: Normally this function returns [] if no module can be + found. If strict=True, then a UnprocessableEntity is raised. + :return: List of Modulemd metadata instances matching the query + """ + local_modules = models.ModuleBuild.local_modules(self.db_session, name, stream) + if local_modules: + return [m.mmd() for m in local_modules] + + modules = self._get_modules(name, stream, version, context, strict=strict) + + return self._modules_to_modulemds(modules, strict) + def get_compatible_base_module_modulemds( self, base_module_mmd, stream_version_lte, virtual_streams, states ): @@ -225,18 +210,37 @@ class MBSResolver(KojiResolver): :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 virtual_streams: List of virtual streams. + Modules are returned if they share one of the virtual streams. :param states: List of states the returned compatible modules should be in. :return list: List of Modulemd objects. """ + if not virtual_streams: + raise RuntimeError("Virtual stream list must not be empty") + name = base_module_mmd.get_module_name() - stream = base_module_mmd.get_stream_name() - return self.get_module_modulemds( - name, stream, stream_version_lte=stream_version_lte, virtual_streams=virtual_streams, - states=states) + + extra_args = {} + if stream_version_lte: + stream = base_module_mmd.get_stream_name() + stream_in_xyz_format = len(str(models.ModuleBuild.get_stream_version( + stream, right_pad=False))) >= 5 + + if not stream_in_xyz_format: + raise StreamNotXyz( + "Cannot get compatible base modules, because stream of resolved " + "base module %s:%s is not in x.y.z format." % ( + base_module_mmd.get_module_name(), stream + )) + + stream_version = models.ModuleBuild.get_stream_version(stream) + extra_args["stream_version_lte"] = stream_version + + extra_args["virtual_stream"] = virtual_streams + + modules = self._get_modules(name, None, states=states, **extra_args) + return self._modules_to_modulemds(modules, False) def get_buildrequired_modulemds(self, name, stream, base_module_mmd): """ diff --git a/tests/test_resolver/test_db.py b/tests/test_resolver/test_db.py index adb1f29f..a946d9a7 100644 --- a/tests/test_resolver/test_db.py +++ b/tests/test_resolver/test_db.py @@ -11,7 +11,7 @@ import pytest from module_build_service.builder.MockModuleBuilder import load_local_builds from module_build_service.common import models from module_build_service.common.config import conf -from module_build_service.common.errors import UnprocessableEntity +from module_build_service.common.errors import StreamNotXyz, UnprocessableEntity from module_build_service.common.models import ModuleBuild from module_build_service.common.modulemd import Modulemd from module_build_service.common.utils import import_mmd, load_mmd, mmd_to_str @@ -80,6 +80,37 @@ class TestDBModule: "platform:f29.2.0:3:00000000" } + @pytest.mark.parametrize("provide_test_data", + [{"multiple_stream_versions": True}], indirect=True) + def test_get_compatible_base_module_modulemds_no_virtual(self, provide_test_data): + resolver = mbs_resolver.GenericResolver.create(db_session, conf, backend="db") + + platform = db_session.query(ModuleBuild).filter_by(name="platform", stream="f29.1.0").one() + platform_mmd = platform.mmd() + + with pytest.raises(RuntimeError, match=r"Virtual stream list must not be empty"): + resolver.get_compatible_base_module_modulemds( + platform_mmd, stream_version_lte=True, virtual_streams=[], + states=[models.BUILD_STATES["ready"]] + ) + + def test_get_compatible_base_module_modulemds_stream_not_xyz( + self, require_platform_and_default_arch, caplog + ): + resolver = mbs_resolver.GenericResolver.create(db_session, conf, backend="db") + + platform = db_session.query(ModuleBuild).filter_by(name="platform", stream="f28").one() + platform_mmd = platform.mmd() + + with pytest.raises( + StreamNotXyz, + match=(r"Cannot get compatible base modules, because stream " + r"of resolved base module platform:f28 is not in x\.y\.z format") + ): + resolver.get_compatible_base_module_modulemds( + platform_mmd, stream_version_lte=True, virtual_streams=["f29"], + states=[models.BUILD_STATES["ready"]]) + @pytest.mark.parametrize("empty_buildrequires", [False, True]) def test_get_module_build_dependencies(self, empty_buildrequires, reuse_component_init_data): """ diff --git a/tests/test_scheduler/test_default_modules.py b/tests/test_scheduler/test_default_modules.py index 90fbf78a..f0f6e5bd 100644 --- a/tests/test_scheduler/test_default_modules.py +++ b/tests/test_scheduler/test_default_modules.py @@ -90,6 +90,13 @@ def test_add_default_modules_platform_not_available(require_empty_database): default_modules.add_default_modules(mmd) +# The fedora-style base module versioning here, with f27/f28 sharing the same +# virtual_stream doesn't work with allow_only_compatible_base_modules, since +# that expects FOOx.y.z versioning. +@patch( + "module_build_service.common.config.Config.allow_only_compatible_base_modules", + new=False +) @patch("module_build_service.scheduler.default_modules._get_default_modules") def test_add_default_modules_compatible_platforms(mock_get_dm, require_empty_database): """ diff --git a/tests/test_scheduler/test_reuse.py b/tests/test_scheduler/test_reuse.py index 0238252b..aed2cf14 100644 --- a/tests/test_scheduler/test_reuse.py +++ b/tests/test_scheduler/test_reuse.py @@ -2,11 +2,14 @@ # SPDX-License-Identifier: MIT from __future__ import absolute_import +from datetime import timedelta + import mock import pytest from sqlalchemy.orm.session import make_transient from module_build_service.common import models +from module_build_service.common.errors import StreamNotXyz from module_build_service.common.modulemd import Modulemd from module_build_service.common.utils import import_mmd, load_mmd, mmd_to_str from module_build_service.scheduler.db_session import db_session @@ -293,80 +296,129 @@ class TestUtilsModuleReuse: assert reusable_module.id == build_module.reused_module_id assert reusable_module.id == reused_module.id - @pytest.mark.parametrize("allow_ocbm", (True, False)) + _EXCEPTION = object() + + @pytest.mark.parametrize("allow_ocbm,build_req,available_req,expected", [ + # When allow_only_compatible_base_modules is false, only modules + # built against the exact same base module stream can be used + (False, "f29.2.0", ("f29.2.0",), "f29.2.0"), + (False, "f29.2.0", ("f29.0.0",), None), + # When it is true, any base module x.y2.z2 where y2.z2 <= y1.z1 + # is a candidate + (True, "f29.2.0", ("f29.2.0", "f29.0.0", "f29.3.0"), "f29.2.0"), + (True, "f29.2.0", ("f29.1.0", "f29.0.0", "f29.3.0"), "f29.1.0"), + # But if the major is different, they don't (the code below adds "f28.0.0" + # with the "f29" virtual stream, so it would be a candidate otherwise) + (True, "f29.2.0", ("f28.0.0",), None), + # the virtual stream must also match (+ is used to skip virtual stream addition) + (True, "f29.2.0", ("+f29.1.0",), None), + # If the buildrequired base module isn't in x.y.z form, we raise an + # exception + (True, "f29", ("f29",), _EXCEPTION), + (True, "f29", ("f29.0.0",), _EXCEPTION), + ]) @mock.patch( "module_build_service.common.config.Config.allow_only_compatible_base_modules", new_callable=mock.PropertyMock, ) - def test_get_reusable_module_use_latest_build(self, cfg, allow_ocbm): + def test_get_reusable_module_use_latest_build( + self, cfg, allow_ocbm, build_req, available_req, expected): """ Test that the `get_reusable_module` tries to reuse the latest module in case when multiple modules can be reused allow_only_compatible_base_modules is True. """ cfg.return_value = allow_ocbm - # Set "fedora" virtual stream to platform:f28. - platform_f28 = db_session.query(models.ModuleBuild).filter_by(name="platform").one() - mmd = platform_f28.mmd() - xmd = mmd.get_xmd() - xmd["mbs"]["virtual_streams"] = ["fedora"] - mmd.set_xmd(xmd) - platform_f28.modulemd = mmd_to_str(mmd) - platform_f28.update_virtual_streams(db_session, ["fedora"]) - # Create platform:f29 with "fedora" virtual stream. - mmd = load_mmd(read_staged_data("platform")) - mmd = mmd.copy("platform", "f29") - xmd = mmd.get_xmd() - xmd["mbs"]["virtual_streams"] = ["fedora"] - mmd.set_xmd(xmd) - platform_f29 = import_mmd(db_session, mmd)[0] + # Create all the platform streams we need + needed_platform_streams = set([build_req]) + needed_platform_streams.update(available_req) + platform_modules = {} + for stream in needed_platform_streams: + skip_virtual = False + if stream.startswith('+'): + skip_virtual = True + stream = stream[1:] - # Create another copy of `testmodule:master` which should be reused, because its - # stream version will be higher than the previous one. Also set its buildrequires - # to platform:f29. - latest_module = db_session.query(models.ModuleBuild).filter_by( - name="testmodule", state=models.BUILD_STATES["ready"]).one() - # This is used to clone the ModuleBuild SQLAlchemy object without recreating it from - # scratch. - db_session.expunge(latest_module) - make_transient(latest_module) + # Create platform:f29.0.0 with "f29" virtual stream. + mmd = load_mmd(read_staged_data("platform")) + mmd = mmd.copy("platform", stream) + xmd = mmd.get_xmd() + xmd["mbs"]["virtual_streams"] = [] if skip_virtual else ["f29"] + mmd.set_xmd(xmd) + platform_modules[stream] = import_mmd(db_session, mmd)[0] - # Change the platform:f28 buildrequirement to platform:f29 and recompute the build_context. - mmd = latest_module.mmd() - xmd = mmd.get_xmd() - xmd["mbs"]["buildrequires"]["platform"]["stream"] = "f29" - mmd.set_xmd(xmd) - latest_module.modulemd = mmd_to_str(mmd) - contexts = models.ModuleBuild.contexts_from_mmd( - latest_module.modulemd - ) - latest_module.build_context = contexts.build_context - latest_module.context = contexts.context - latest_module.buildrequires = [platform_f29] + # Modify a module to buildrequire a particular platform stream + def modify_buildrequires(module, platform_module): + mmd = module.mmd() + deps = mmd.get_dependencies()[0] + deps.clear_buildtime_dependencies() + deps.add_buildtime_stream('platform', platform_module.stream) + xmd = mmd.get_xmd() + xmd["mbs"]["buildrequires"]["platform"]["stream"] = platform_module.stream + mmd.set_xmd(xmd) + module.modulemd = mmd_to_str(mmd) + contexts = models.ModuleBuild.contexts_from_mmd( + module.modulemd + ) + module.build_context = contexts.build_context + module.context = contexts.context + module.buildrequires = [platform_module] - # Set the `id` to None, so new one is generated by SQLAlchemy. - latest_module.id = None - db_session.add(latest_module) - db_session.commit() + stream_to_testmodule_id = {} + + first = True + for stream in available_req: + if stream.startswith('+'): + stream = stream[1:] + + if first: + # Reuse the testmodule:master already in the database + module = db_session.query(models.ModuleBuild).filter_by( + name="testmodule", state=models.BUILD_STATES["ready"]).one() + + modify_buildrequires(module, platform_modules[stream]) + time_completed = module.time_completed + + first = False + else: + # Create another copy of `testmodule:master`, and modify it + module = db_session.query(models.ModuleBuild).filter_by( + name="testmodule", state=models.BUILD_STATES["ready"]).first() + + # This is used to clone the ModuleBuild SQLAlchemy object without recreating it from + # scratch. + db_session.expunge(module) + make_transient(module) + + modify_buildrequires(module, platform_modules[stream]) + + # Add modules with ascending time_completed + time_completed += timedelta(days=1) + module.time_completed = time_completed + + # Set the `id` to None, so new one is generated by SQLAlchemy. + module.id = None + db_session.add(module) + + db_session.commit() + stream_to_testmodule_id[stream] = module.id module = db_session.query(models.ModuleBuild)\ .filter_by(name="testmodule")\ .filter_by(state=models.BUILD_STATES["build"])\ .one() + modify_buildrequires(module, platform_modules[build_req]) db_session.commit() - reusable_module = get_reusable_module(module) - - if allow_ocbm: - assert reusable_module.id == latest_module.id + if expected is TestUtilsModuleReuse._EXCEPTION: + with pytest.raises(StreamNotXyz): + get_reusable_module(module) else: - # There are two testmodules in ready state, the first one with - # lower id is what we want. - first_module = db_session.query(models.ModuleBuild).filter_by( - name="testmodule", state=models.BUILD_STATES["ready"] - ).order_by(models.ModuleBuild.id).first() - - assert reusable_module.id == first_module.id + reusable_module = get_reusable_module(module) + if expected: + assert reusable_module.id == stream_to_testmodule_id[expected] + else: + assert reusable_module is None @pytest.mark.parametrize("allow_ocbm", (True, False)) @mock.patch(