#1168 Allow submitting the same NSV multiple times for scratch module builds.
Merged 3 months ago by mprahl. Opened 4 months ago by merlinm.

@@ -354,6 +354,16 @@

              name=name, stream=stream, version=str(version), context=context, **kwargs).first()

  

      @staticmethod

+     def get_scratch_builds_from_nsvc(session, name, stream, version, context, **kwargs):

+         """

+         Returns all scratch builds defined by NSVC. This is done by using the supplied `context`

+         as a match prefix. Optional kwargs are passed to SQLAlchemy filter_by method.

+         """

+         return session.query(ModuleBuild).filter_by(

+             name=name, stream=stream, version=str(version), scratch=True, **kwargs)\

+             .filter(ModuleBuild.context.like(context + '%')).all()

+ 

+     @staticmethod

      def get_last_builds_in_stream_version_lte(session, name, stream_version):

          """

          Returns the latest builds in "ready" state for given name:stream limited by

@@ -507,7 +517,7 @@

      @property

      def siblings(self):

          query = self.query.filter_by(

-             name=self.name, stream=self.stream, version=self.version).options(

+             name=self.name, stream=self.stream, version=self.version, scratch=self.scratch).options(

              load_only('id')).filter(ModuleBuild.id != self.id)

          return [build.id for build in query.all()]

  

@@ -831,10 +841,10 @@

          return rv

  

      def __repr__(self):

-         return (("<ModuleBuild %s, id=%d, stream=%s, version=%s, state %r,"

-                  " batch %r, state_reason %r>")

-                 % (self.name, self.id, self.stream, self.version, INVERSE_BUILD_STATES[self.state],

-                    self.batch, self.state_reason))

+         return (("<ModuleBuild %s, id=%d, stream=%s, version=%s, scratch=%r,"

+                  " state %r, batch %r, state_reason %r>")

+                 % (self.name, self.id, self.stream, self.version, self.scratch,

+                    INVERSE_BUILD_STATES[self.state], self.batch, self.state_reason))

  

  

  class ModuleBuildTrace(MBSBase):

@@ -602,6 +602,10 @@

      """

      import koji  # Placed here to avoid py2/py3 conflicts...

  

+     log.debug('Submitted %s module build for %s:%s:%s',

+               ("scratch" if params.get('scratch', False) else "normal"),

+               mmd.get_name(), mmd.get_stream(), mmd.get_version())

+ 

      if mmd.get_name() in conf.base_module_names:

          raise ValidationError(

              'You cannot build a module named "{}" since it is a base module'.format(mmd.get_name()))

@@ -639,7 +643,7 @@

          log.debug('Checking whether module build already exists: %s.', nsvc)

          module = models.ModuleBuild.get_build_from_nsvc(

              db.session, mmd.get_name(), mmd.get_stream(), version_str, mmd.get_context())

-         if module:

+         if module and not params.get('scratch', False):

              if module.state != models.BUILD_STATES['failed']:

                  log.info("Skipping rebuild of %s, only rebuild of modules in failed state "

                           "is allowed.", nsvc)

@@ -669,6 +673,18 @@

              module.transition(conf, transition_to, "Resubmitted by %s" % username)

              log.info("Resumed existing module build in previous state %s" % module.state)

          else:

+             # make NSVC unique for every scratch build

+             context_suffix = ''

+             if params.get('scratch', False):

+                 log.debug('Checking for existing scratch module builds by NSVC')

+                 scrmods = models.ModuleBuild.get_scratch_builds_from_nsvc(

+                     db.session, mmd.get_name(), mmd.get_stream(), version_str, mmd.get_context())

+                 scrmod_contexts = [scrmod.context for scrmod in scrmods]

+                 log.debug('Found %d previous scratch module build context(s): %s',

+                           len(scrmods), ",".join(scrmod_contexts))

+                 # append incrementing counter to context

+                 context_suffix = '_' + str(len(scrmods) + 1)

+                 mmd.set_context(mmd.get_context() + context_suffix)

              log.debug('Creating new module build')

              module = models.ModuleBuild.create(

                  db.session,

@@ -685,6 +701,7 @@

              )

              (module.ref_build_context, module.build_context, module.runtime_context,

               module.context) = module.contexts_from_mmd(module.modulemd)

+             module.context += context_suffix

  

          all_modules_skipped = False

          db.session.add(module)

@@ -1200,6 +1200,125 @@

  

      @patch('module_build_service.auth.get_user', return_value=user)

      @patch('module_build_service.scm.SCM')

+     @patch('module_build_service.config.Config.modules_allow_scratch',

+            new_callable=PropertyMock, return_value=True)

+     def test_submit_scratch_vs_normal(

+             self, mocked_allow_scratch, mocked_scm, mocked_get_user, conf_system, dbg, hmsc):

+         """

+         Tests that submitting a scratch build with the same NSV as a previously

+         completed normal build succeeds and both have expected contexts

+         """

+         FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml',

+                 '620ec77321b2ea7b0d67d82992dda3e1d67055b4')

+         # Post so a module is in the init phase

+         post_url = '/module-build-service/1/module-builds/'

+         post_data = {

+             'branch': 'master',

+             'scmurl': 'https://src.stg.fedoraproject.org/modules/'

+                       'testmodule.git?#620ec77321b2ea7b0d67d82992dda3e1d67055b4',

+             'scratch': False,

+         }

+         rv = self.client.post(post_url, data=json.dumps(post_data))

+         assert rv.status_code == 201

+         data = json.loads(rv.data)

+         module_build_id = data['id']

+         module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one()

+         # make sure normal build has expected context without a suffix

+         assert module_build.context == '9c690d0e'

+         # Run the backend

+         stop = module_build_service.scheduler.make_simple_stop_condition(db.session)

+         module_build_service.scheduler.main([], stop)

+         # Post again as a scratch build and make sure it succeeds

+         post_data['scratch'] = True

+         rv2 = self.client.post(post_url, data=json.dumps(post_data))

+         assert rv2.status_code == 201

+         data = json.loads(rv2.data)

+         module_build_id = data['id']

+         module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one()

+         # make sure scratch build has expected context with unique suffix

+         assert module_build.context == '9c690d0e_1'

+ 

+     @patch('module_build_service.auth.get_user', return_value=user)

+     @patch('module_build_service.scm.SCM')

+     @patch('module_build_service.config.Config.modules_allow_scratch',

+            new_callable=PropertyMock, return_value=True)

+     def test_submit_normal_vs_scratch(

+             self, mocked_allow_scratch, mocked_scm, mocked_get_user, conf_system, dbg, hmsc):

+         """

+         Tests that submitting a normal build with the same NSV as a previously

+         completed scratch build succeeds and both have expected contexts

+         """

+         FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml',

+                 '620ec77321b2ea7b0d67d82992dda3e1d67055b4')

+         # Post so a scratch module build is in the init phase

+         post_url = '/module-build-service/1/module-builds/'

+         post_data = {

+             'branch': 'master',

+             'scmurl': 'https://src.stg.fedoraproject.org/modules/'

+                       'testmodule.git?#620ec77321b2ea7b0d67d82992dda3e1d67055b4',

+             'scratch': True,

+         }

+         rv = self.client.post(post_url, data=json.dumps(post_data))

+         assert rv.status_code == 201

+         data = json.loads(rv.data)

+         module_build_id = data['id']

+         module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one()

+         # make sure scratch build has expected context with unique suffix

+         assert module_build.context == '9c690d0e_1'

+         # Run the backend

+         stop = module_build_service.scheduler.make_simple_stop_condition(db.session)

+         module_build_service.scheduler.main([], stop)

+         # Post again as a non-scratch build and make sure it succeeds

+         post_data['scratch'] = False

+         rv2 = self.client.post(post_url, data=json.dumps(post_data))

+         assert rv2.status_code == 201

+         data = json.loads(rv2.data)

+         module_build_id = data['id']

+         module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one()

+         # make sure normal build has expected context without suffix

+         assert module_build.context == '9c690d0e'

+ 

+     @patch('module_build_service.auth.get_user', return_value=user)

+     @patch('module_build_service.scm.SCM')

+     @patch('module_build_service.config.Config.modules_allow_scratch',

+            new_callable=PropertyMock, return_value=True)

+     def test_submit_scratch_vs_scratch(

+             self, mocked_allow_scratch, mocked_scm, mocked_get_user, conf_system, dbg, hmsc):

+         """

+         Tests that submitting a scratch build with the same NSV as a previously

+         completed scratch build succeeds and both have expected contexts

+         """

+         FakeSCM(mocked_scm, 'testmodule', 'testmodule.yaml',

+                 '620ec77321b2ea7b0d67d82992dda3e1d67055b4')

+         # Post so a scratch module build is in the init phase

+         post_url = '/module-build-service/1/module-builds/'

+         post_data = {

+             'branch': 'master',

+             'scmurl': 'https://src.stg.fedoraproject.org/modules/'

+                       'testmodule.git?#620ec77321b2ea7b0d67d82992dda3e1d67055b4',

+             'scratch': True,

+         }

+         rv = self.client.post(post_url, data=json.dumps(post_data))

+         assert rv.status_code == 201

+         data = json.loads(rv.data)

+         module_build_id = data['id']

+         module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one()

+         # make sure first scratch build has expected context with unique suffix

+         assert module_build.context == '9c690d0e_1'

+         # Run the backend

+         stop = module_build_service.scheduler.make_simple_stop_condition(db.session)

+         module_build_service.scheduler.main([], stop)

+         # Post scratch build again and make sure it succeeds

+         rv2 = self.client.post(post_url, data=json.dumps(post_data))

+         assert rv2.status_code == 201

+         data = json.loads(rv2.data)

+         module_build_id = data['id']

+         module_build = models.ModuleBuild.query.filter_by(id=module_build_id).one()

+         # make sure second scratch build has expected context with unique suffix

+         assert module_build.context == '9c690d0e_2'

+ 

+     @patch('module_build_service.auth.get_user', return_value=user)

+     @patch('module_build_service.scm.SCM')

      def test_submit_build_repo_regen_not_started_batch(self, mocked_scm, mocked_get_user,

                                                         conf_system, dbg, hmsc):

          """

Does it allow removing already built non-scratch build and submitting is as scratch-build? If so, it is wrong. Finished non-scratch build should always stay in db unchanged without any way to resubmit them.

Maybe your code would end up with two builds with the same NSVC, one with scratch=False and one with scratch=True in this case. I personally think this might be confusing, but will wait for others to give their opinions :).

The logic I have attempted to implement is...

IF a failed build (scratch or not) is resubmitted:
    try building it again
ELSE IF a successful build is resubmitted AND the old build was non-scratch AND the resubmit request is non-scratch
    skip the rebuild
ELSE:
    submit a new build (either an old build was scratch, the new request is for scratch, or both)

After looking at this again, I'm not convinced it's the correct logic. At this point I'm not even sure what the correct logic should be. I'm starting to think this issue may need to be investigated and implemented by an experienced MBS subject matter expert. :worried:

@merlinm, we have discussed this with @mprahl on Friday and agreed that it makes sense to allow single NSVC to be built at least twice - once as scratch-build and once as normal build.

The question is if we also want to support single NSVC to be built multiple times as scratch-build without overriding it in MBS DB. Current MBS code relies on fact that NSVC is unique and more changes would need to be done to do it differently.

The elegant solution for that would be appending ".N" to context in this case for scratch-builds. What do you think @mprahl?

Overriding the existing "done" scratch-build NSVC can be problematic, because other services might have some tests running on scratch-build and if we change it in MBS DB in meantime, it could lead to issues.

Whatever we do, we need to ensure that the MBS DB queries for name/stream/version/context without the "ready" state also contains scratch==False, otherwise in case when we have both scratch and non-scratch build for the same NSVC, the MBS could choose the wrong module. This is for example case of ModuleBuild.get_build_from_nsvc and other methods like that. This is an issue even now and is not related to this PR.

The elegant solution for that would be appending ".N" to context in this case for scratch-builds.

Why not always append ".<timestamp>" to scratch builds? This avoids conflict of NSVC with non-scratch builds and it becomes very simple to allow multiple scratch builds.

@merlinm, we have discussed this with @mprahl on Friday and agreed that it makes sense to allow single NSVC to be built at least twice - once as scratch-build and once as normal build.
The question is if we also want to support single NSVC to be built multiple times as scratch-build without overriding it in MBS DB. Current MBS code relies on fact that NSVC is unique and more changes would need to be done to do it differently.
The elegant solution for that would be appending ".N" to context in this case for scratch-builds. What do you think @mprahl?
Overriding the existing "done" scratch-build NSVC can be problematic, because other services might have some tests running on scratch-build and if we change it in MBS DB in meantime, it could lead to issues.
Whatever we do, we need to ensure that the MBS DB queries for name/stream/version/context without the "ready" state also contains scratch==False, otherwise in case when we have both scratch and non-scratch build for the same NSVC, the MBS could choose the wrong module. This is for example case of ModuleBuild.get_build_from_nsvc and other methods like that. This is an issue even now and is not related to this PR.

I like the idea, assuming that "N" is a variable that is incremented based on the number of modules with that NSVC. This has the added benefit of not allowing the resuming of scratch builds that have failed.

@merlinm how's this going? Are you blocked on anything?

@mprahl I get the part about ensuring MBS DB queries for NSVC that don't include state=ready need to include scratch=False, and I've been trying to identify those queries that need fixing.

However, I'm having a difficult time wrapping my head around how to implement allowing scratch builds multiple times for the same NSVC. The idea of appending ".N" to context sounds good until you append it. Upon modification of context, it becomes a unique value and queries for the original NSVC no longer match it to figure out what the next ".N" should be. Or maybe I'm thinking about this all wrong.

Perhaps for scratch builds version should always be based on the current timestamp so i it is (almost) always unique?

@mprahl I get the part about ensuring MBS DB queries for NSVC that don't include state=ready need to include scratch=False, and I've been trying to identify those queries that need fixing.
However, I'm having a difficult time wrapping my head around how to implement allowing scratch builds multiple times for the same NSVC. The idea of appending ".N" to context sounds good until you append it. Upon modification of context, it becomes a unique value and queries for the original NSVC no longer match it to figure out what the next ".N" should be. Or maybe I'm thinking about this all wrong.
Perhaps for scratch builds version should always be based on the current timestamp so i it is (almost) always unique?

If you modify the context by appending the .N, then you don't need to modify the queries that use NSVC since the NSVC will be unique for every module build. As for finding the number to increment, you could query for the name, stream, version, and then the context with a wildcard. For example:

import sqlalchemy
from module_build_service import models

count = int(db.session.query(sqlalchemy.func.count(models.ModuleBuild.id)).filter_by(
    name='testmodule', stream='private-mprahl', version='820190326122133')\
    .filter(models.ModuleBuild.context.like('9edba%')).scalar())

rebased onto d825d688f49ea619ea5decc1e1aa32ff11693092

4 months ago

rebased onto 360ebac08e652eeaeb0f6d1cf643d6aedbe7b5b5

4 months ago

Reworked and rebased to latest master.

@mprahl @jkaluza Is this along the lines of what you had in mind for appending .N to context?

If so, I still need to re-evaluate which queries need to include scratch=False.

@mprahl @jkaluza When using the approach of appending .N to context for scratch builds... does it make sense to prohibit submitting a scratch module build if a normal build with the same NSV already exists? (If so, my current test_submit_scratch_vs_normal test in test_build.py is invalid.)

rebased onto 7ef1ec75a5b1c165cc3507749adb0ee2398ac388

4 months ago

@mprahl @jkaluza Assuming the answer to my previous question is "no", and there's no reason a normal module build can't be rebuilt again as scratch with the same NSV... I just pushed an improved version that appends .N to context. But I still need to sift through which queries need to include scratch=False.

Feedback would be appreciated. Thanks.

rebased onto 86293bbc1c02c62d401453e2aeb13da5acc2d3db

4 months ago

@mprahl @jkaluza I got to wondering... Since context ends up getting embedded in various NVR strings, should the . character be avoided in the uniqueness suffix--instead using _N or +N?

@merlinm I'd be fine with that. Also, do we even need to modify the NSVC queries since now every NSVC will be unique?

As for your question about not allowing a scratch build of an NSVC that was already built, I'm fine with either approach. We can always change it later.

@mprahl Thanks. I'll update the code to use _N for safety's sake.

No, NSVC queries shouldn't need modification now. But any NSV queries may--to make sure scratch modules aren't accidentally picked up as dependencies.

1 new commit added

  • Update scratch context uniqueness suffix character from '.' to '_'.
4 months ago

@mprahl @jkaluza After examining the NSV queries, the only one I think may need tweaking is in models.ModuleBuild.siblings(). Adding scratch=self.scratch is a start, but I think it needs something more. Any thoughts or suggestions?

rebased onto d8df76383556dd48cc3a4f18b1fee08734c0b441

3 months ago

Could you add .all() at the end of this so that the query is executed and a list is returned?

It'd be nice to validate the contexts for both scratch builds here

This docstring needs updating

After adding .all() to the query in get_scratch_builds_from_nsvc, you'll need to use len(scrmods) instead of scrmods.count().

@mprahl @jkaluza After examining the NSV queries, the only one I think may need tweaking is in models.ModuleBuild.siblings(). Adding scratch=self.scratch is a start, but I think it needs something more. Any thoughts or suggestions?

Good catch, I think what you suggested would be enough.

@merlinm this looks great. I left a few comments, but once they are addressed, I can merge this.

Sorry, I'm not seeing the problem with the docstring.

1 new commit added

  • Fixes and test improvements based on review feedback.
3 months ago

@mprahl Thank you for the feedback. I addressed all your comments. I'm not sure what you wanted updated with the test_submit_scratch_vs_scratch() docstring, but I revised the docstrings for all three new tests to be a little clearer. Let me know if you'd like any other changes, and/or if you'd like me to squash my commits.

Also, thank you for already rebasing my PR. Unfortunately, it looks like CI failed--and has been failing for all fm-orchestrator runs for the past few days with some TestMockModuleBuilder failures.

:thumbsup:

I'll work on fixing the tests in a separate PR.

rebased onto 663b7fc

3 months ago

Pull-Request has been merged by mprahl

3 months ago