PR#1048: Check transitive stream collision - fm-orchestrator

fm-orchestrator

#1048 Check transitive stream collision

Closed 5 years ago by cqi. Opened 5 years ago by cqi.

cqi/fm-orchestrator resolve-transitive-deps-collision into master

Check transitive stream collision

Chenxiong Qi • 5 years ago

5e7fd10

module_build_service/__init__.py

file modified

+7 -1

		`@@ -54,7 +54,7 @@`

		`from module_build_service.errors import (`
		`ValidationError, Unauthorized, UnprocessableEntity, Conflict, NotFound,`
		`- Forbidden, json_error)`
		`+ Forbidden, json_error, TransitiveStreamCollision)`
		`from module_build_service.config import init_config`
		`from module_build_service.proxy import ReverseProxy`

		`@@ -153,6 +153,12 @@`
		`return json_error(404, 'Not Found', str(e))`


		`+ @app.errorhandler(TransitiveStreamCollision)`
		`+ def transitivestreamcollision_error(e):`
		`+ """Flask error handler for TransitiveStreamCollision exceptions"""`
		`+ return json_error(400, 'Bad Request', str(e))`
		`+`
		`+`
		`init_logging(conf)`
		`log = getLogger(__name__)`
		`build_logs = ModuleBuildLogs(`

module_build_service/errors.py

file modified

		`@@ -62,3 +62,7 @@`
		`'message': message})`
		`response.status_code = status`
		`return response`
		`+`
		`+`
		`+ class TransitiveStreamCollision(ValueError):`
		`+ pass`

module_build_service/utils/submit.py

file modified

+32 -2

		`@@ -41,7 +41,8 @@`
		`from module_build_service.errors import (`
		`ValidationError, UnprocessableEntity, Forbidden, Conflict)`
		`from module_build_service import glib`
		`- from .mse import generate_expanded_mmds`
		`+ from module_build_service.utils import mse`
		`+ from module_build_service.errors import TransitiveStreamCollision`


		`def record_filtered_rpms(mmd):`
		`@@ -441,7 +442,7 @@`
		`default_streams = {}`

		`validate_mmd(mmd)`
		`- mmds = generate_expanded_mmds(db.session, mmd, raise_if_stream_ambigous, default_streams)`
		`+ mmds = mse.generate_expanded_mmds(db.session, mmd, raise_if_stream_ambigous, default_streams)`
		`modules = []`

		`for mmd in mmds:`
		`@@ -483,6 +484,12 @@`
		`module.transition(conf, transition_to, "Resubmitted by %s" % username)`
		`log.info("Resumed existing module build in previous state %s" % module.state)`
		`else:`
		`+ required_mmds = mse.get_mmds_required_by_module_recursively(`
		`+ mmd,`
		`+ default_streams=default_streams,`
		`+ raise_if_stream_ambigous=raise_if_stream_ambigous)`
		`+ check_transitive_stream_collision(required_mmds)`
		`+`
		`log.debug('Creating new module build')`
		`module = models.ModuleBuild.create(`
		`db.session,`
		`@@ -700,3 +707,26 @@`
		`'Parsed metadata results for "{0}" don\'t match the directory name'`
		`.format(found_build[3]))`
		`log.info("Loaded local module build %r", module)`
		`+`
		`+`
		`+ def check_transitive_stream_collision(mmds):`
		`+ """Check if there is transitive stream collision modules through requires path`
		`+`
		`+ Think about a case: modulea buildrequires foo:1 and bar:1, then foo:1 requires baz:1`
mprahl commented 5 years ago `modulea` => module "a"`
		`+ and bar:1 requires baz:2. As a result, the collision happens on baz:1 and baz:2.`
		`+`
		`+ :param mmds: list of module metadata, each of which is a result of module`
		`+ stream expansion.`
		`+ :type mmds: list[Modulemd.Module]`
		`+ :raises TransitiveStreamCollision: if collision is detected.`
		`+ """`
		`+ name_streams = {}`
		`+ for mmd in mmds:`
		`+ name = mmd.get_name()`
		`+ stream = mmd.get_stream()`
		`+ if name in name_streams and name_streams[name] != stream:`
		`+ raise TransitiveStreamCollision(`
		`+ 'Transitive stream collision happens on {}:{} and {}:{}'`
		`+ .format(name, name_streams[name], name, stream))`
		`+ else:`
		`+ name_streams[name] = stream`

tests/test_utils/test_utils.py

file modified

+73 -16

		`@@ -18,27 +18,26 @@`
		`# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE`
		`# SOFTWARE.`

		`+ import koji`
		`+ import mock`
		`+ import module_build_service.scheduler.handlers.components`
		`+ import module_build_service.scm`
		`+ import module_build_service.utils`
		`+ import pytest`
		`import tempfile`
		`- from os import path, mkdir`
		`- from shutil import copyfile, rmtree`
		`+`
		`from datetime import datetime`
		`- from werkzeug.datastructures import FileStorage`
		`from mock import patch`
		`- import module_build_service.utils`
		`- import module_build_service.scm`
		`- from module_build_service import models, conf`
		`- from module_build_service.errors import ProgrammingError, ValidationError, UnprocessableEntity`
		`- from tests import (`
		`- reuse_component_init_data, db, reuse_shared_userspace_init_data, clean_database, init_data,`
		`- scheduler_init_data)`
		`- import mock`
		`- import koji`
		`- import pytest`
		`- import module_build_service.scheduler.handlers.components`
		`from module_build_service.builder.base import GenericBuilder`
		`from module_build_service.builder.KojiModuleBuilder import KojiModuleBuilder`
		`- from module_build_service import glib, Modulemd`
		`- from tests import app`
		`+ from module_build_service.errors import ProgrammingError, ValidationError, UnprocessableEntity`
		`+ from module_build_service import glib, Modulemd, utils, models, conf`
		`+ from os import path, mkdir`
		`+ from shutil import copyfile, rmtree`
		`+ from werkzeug.datastructures import FileStorage`
		`+ from tests import (`
		`+ app, db, reuse_component_init_data, reuse_shared_userspace_init_data,`
		`+ clean_database, init_data, scheduler_init_data, make_module)`

		`BASE_DIR = path.abspath(path.dirname(__file__))`

		`@@ -1081,3 +1080,61 @@`
		`assert len(local_modules) == 1`
		`assert local_modules[0].koji_tag.endswith(`
		`"/module-platform-f28-3/results")`
		`+`
		`+`
		`+ class TestCheckTransitiveStreamCollision:`
		`+ """Test submit.check_transitive_stream_collision"""`
		`+`
		`+ def setup_method(self, test_method):`
		`+ clean_database()`
		`+`
		`+ def teardown_method(self, test_method):`
		`+ clean_database()`
		`+`
		`+ @staticmethod`
		`+ def _generate_collision_modules():`
		`+ """Generate test modules`
		`+`
		`+ gtk:1 requires bar:1 which requires platform:f29`
		`+ foo:1 requires bar:2 which requires platform:f29`
		`+ """`
		`+ make_module('gtk:1:1:c2', {'bar': ['1']}, {})`
		`+ make_module('foo:1:1:c2', {'bar': ['2']}, {})`
		`+ make_module('bar:1:0:c2', {'platform': ['f29']}, {})`
		`+ make_module('bar:2:0:c2', {'platform': ['f29']}, {})`
		`+ make_module('platform:f29:0:c11', {}, {})`
		`+`
		`+ @staticmethod`
		`+ def _generate_modules_without_colliison():`
		`+ """Generate test modules`
		`+`
		`+ gtk:1 requires bar:1 which requires platform:f29`
		`+ foo:1 requires baz:2 which requires platform:f29`
		`+ """`
		`+ make_module('gtk:1:1:c2', {'bar': ['1']}, {})`
		`+ make_module('foo:1:1:c2', {'baz': ['2']}, {})`
		`+ make_module('bar:1:0:c2', {'platform': ['f29']}, {})`
		`+ make_module('baz:2:0:c2', {'platform': ['f29']}, {})`
		`+ make_module('platform:f29:0:c11', {}, {})`
		`+`
		`+ @staticmethod`
		`+ def _get_mmds_required_by_module_recursively(module_build):`
		`+ mmd = module_build.mmd()`
		`+ utils.mse.expand_mse_streams(db.session, mmd)`
		`+ return utils.mse.get_mmds_required_by_module_recursively(mmd)`
		`+`
		`+ def test_raise_error_when_there_is_collision(self):`
		`+ # app:1 buildrequires gtk:1 and foo:1`
		`+ module_build = make_module("app:1:0:c1", {}, {'gtk': '1', 'foo': '1'})`
		`+ self._generate_collision_modules()`
		`+ modules = self._get_mmds_required_by_module_recursively(module_build)`
		`+ with pytest.raises(utils.submit.TransitiveStreamCollision) as excinfo:`
		`+ utils.submit.check_transitive_stream_collision(modules)`
		`+ assert 'bar:1 and bar:2' in str(excinfo.value)`
		`+`
		`+ def test_no_error_raised_when_no_collision(self):`
		`+ # app:1 buildrequires gtk:1 and foo:1`
		`+ module_build = make_module("app:1:0:c1", {}, {'gtk': '1', 'foo': '1'})`
		`+ self._generate_modules_without_colliison()`
		`+ modules = self._get_mmds_required_by_module_recursively(module_build)`
		`+ utils.submit.check_transitive_stream_collision(modules)`

cqi commented 5 years ago

A transitive stream collision could happen in such a case, for example,
modulea buildrequires foo:1 and bar:1, then foo:1 requires baz:1 and
bar:1 requires baz:2, which will cause the collision on modules baz:1
and baz:2.

This patch checks the collision when submit a module build and each
expanded (module stream expansion) module will be check. If a module has
collision, error will be raised immediately.

Resolves: FACTORY-2612

Signed-off-by: Chenxiong Qi cqi@redhat.com

rebased onto 1a3569f258ccd338e3c3edeb228a415de1a4ead2

5 years ago

rebased onto f215cf04d4bd946ee868064b823ccb0519eab7b4

5 years ago

jkaluza commented on line 24 of module_build_service/utils/submit.py 5 years ago

Hm, why not to do that in generate_expanded_mmds(). The issue is that get_mmds_required_by_module_recursively() during local builds here will need to download all the required modulemd data from MBS REST API again, which will slow down the local module builds.

You should ideally reuse the already downloaded data from previous call of get_mmds_required_by_module_recursively() in generate_expanded_mmds().

I know you probably need to call that method for each expanded MMD, so you probably need a way to get the required mmds from the result of get_mmds_required_by_module_recursively() in generate_expanded_mmds() without fetching it again from MBS.

mprahl commented on line 42 of module_build_service/utils/submit.py 5 years ago

buildrequiers => buildrequires

mprahl commented on line 55 of module_build_service/utils/submit.py 5 years ago

This is called on the API, so you want to add a Flask error handler on this exception type in mdoule_build_service/__init__.py so that JSON is returned instead of a 500 error.

cqi commented on line 24 of module_build_service/utils/submit.py 5 years ago

I considered to call check_transitive_stream_collision only for new module build after stream expansion. If call that method in generate_expanded_mmds, every built module will be checked again and again, and that would also make generate_expanded_mmds bigger with more things to do.

A module's buildrequires and recursive requires of each buildrequire module are already built in MBS. Can we cache these built modules' modulemd for some time to reduce duplicate MBS API calls?

rebased onto 61b682360776056043bb3ab02aba27910309e1cc

5 years ago

cqi commented 5 years ago

buildrequiers => buildrequires

Done.

This is called on the API, so you want to add a Flask error handler on this exception type in mdoule_build_service/__init__.py so that JSON is returned instead of a 500 error.

Done

rebased onto 45c5b4613382938834fd8ac26375dfbc48dfb95d

5 years ago

rebased onto bbe9be02c31cc80bc235c6af78d3320c34b7c7f8

5 years ago

rebased onto 5e7fd10

5 years ago

mprahl commented on line 42 of module_build_service/utils/submit.py 5 years ago

modulea => module "a"`

mprahl commented 5 years ago

I think this approach works, but I'm not sure it's necessary or the correct approach. In IRC @jkaluza and I were talking and noticed this line in the libsolv code in mmd_resolver.py:

solvable.add_deparray(solv.SOLVABLE_CONFLICTS, pool.Dep("module(%s)" % n))

That line seems to imply that the buildroot cannot contain more than one module of the same name.

mprahl commented 5 years ago

@cqi, @jkaluza added some tests to show that the current libsolv code doesn't allow these types of stream collisions:
https://pagure.io/fm-orchestrator/pull-request/1057

Could you please close this PR and open a new PR that just makes sure that the user gets a friendly error message in the API return value when this type of stream collision occurs?