#1048 Check transitive stream collision
Closed 6 months ago by cqi. Opened 6 months ago by cqi.

@@ -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(

@@ -62,3 +62,7 @@ 

           'message': message})

      response.status_code = status

      return response

+ 

+ 

+ class TransitiveStreamCollision(ValueError):

+     pass

@@ -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

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

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)

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

6 months ago

rebased onto f215cf04d4bd946ee868064b823ccb0519eab7b4

6 months 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.

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.

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

6 months 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

6 months ago

rebased onto bbe9be02c31cc80bc235c6af78d3320c34b7c7f8

6 months ago

rebased onto 5e7fd10

6 months 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.

@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?

Fully agree with @mprahl , do not add such useless stuff here, but instead make some better error message in resolver when conflicts happen.

Pull-Request has been closed by cqi

6 months ago