#2263 improve race condition for getNextRelease / images
Merged 2 years ago by tkopecek. Opened 2 years ago by tkopecek.
tkopecek/koji issue2138  into  master

file modified
+8 -22
@@ -2441,10 +2441,6 @@ 

                                                  dict(name=name, version=version, release=release,

                                                       epoch=0))

  

-     def getRelease(self, name, ver):

-         """return the next available release number for an N-V"""

-         return self.session.getNextRelease(dict(name=name, version=ver))

- 

  

  class BuildBaseImageTask(BuildImageTask):

      Methods = ['image']
@@ -2476,15 +2472,14 @@ 

          bld_info = None

          try:

              release = opts.get('release')

-             if not release:

-                 release = self.getRelease(name, version)

              if '-' in version:

                  raise koji.ApplianceError('The Version may not have a hyphen')

-             if '-' in release:

+             if release and '-' in release:

                  raise koji.ApplianceError('The Release may not have a hyphen')

              if not opts.get('scratch'):

                  bld_info = self.initImageBuild(name, version, release,

                                                 target_info, opts)

+                 release = bld_info['release']

  

              subtasks = {}

              self.logger.debug("Spawning jobs for image arches: %r" % (arches))
@@ -2616,11 +2611,10 @@ 

          bld_info = None

          try:

              release = opts.get('release')

-             if not release:

-                 release = self.getRelease(name, version)

              if not opts.get('scratch'):

                  bld_info = self.initImageBuild(name, version, release,

                                                 target_info, opts)

+                 release = bld_info['release']

              create_task_id = self.session.host.subtask(method='createAppliance',

                                                         arglist=[name, version, release, arch,

                                                                  target_info, build_tag,
@@ -2704,11 +2698,10 @@ 

          bld_info = None

          try:

              release = opts.get('release')

-             if not release:

-                 release = self.getRelease(name, version)

              if not opts.get('scratch'):

                  bld_info = self.initImageBuild(name, version, release,

                                                 target_info, opts)

+                 release = bld_info['release']

              create_task_id = self.session.host.subtask(method='createLiveCD',

                                                         arglist=[name, version, release, arch,

                                                                  target_info, build_tag,
@@ -2797,16 +2790,16 @@ 

          bld_info = None

          try:

              release = opts.get('release')

-             if not release:

-                 release = self.getRelease(name, version)

              if not opts.get('scratch'):

                  bld_info = self.initImageBuild(name, version, release,

                                                 target_info, opts)

+                 release = bld_info['release']

              subtasks = {}

              canfail = []

              for arch in arches:

                  subtasks[arch] = self.subtask('createLiveMedia',

-                                               [name, version, release, arch, target_info,

+                                               [name, version, release,

+                                                arch, target_info,

                                                 build_tag, repo_info, ksfile, opts],

                                                label='livemedia %s' % arch, arch=arch)

                  if arch in opts.get('optional_arches', []):
@@ -4469,11 +4462,6 @@ 

          return self.session.host.initImageBuild(self.id,

                                                  dict(name=name, version=version, release=release,

                                                       epoch=0))

- 

-     def getRelease(self, name, ver):

-         """return the next available release number for an N-V"""

-         return self.session.getNextRelease(dict(name=name, version=ver))

- 

      # END inefficient base image task method copies

  

      def fetchHubOrSCM(self, filepath, fileurl, build_tag):
@@ -4652,11 +4640,9 @@ 

          release = opts['release']

  

          # TODO: Another mostly copy-paste

-         if not release:

-             release = self.getRelease(name, version)

          if '-' in version:

              raise koji.ApplianceError('The Version may not have a hyphen')

-         if '-' in release:

+         if release and '-' in release:

              raise koji.ApplianceError('The Release may not have a hyphen')

  

          indirection_template = self.fetchHubOrSCM(opts.get('indirection_template'),

file modified
+2
@@ -13972,6 +13972,8 @@ 

          data['owner'] = task.getOwner()

          data['state'] = koji.BUILD_STATES['BUILDING']

          data['completion_time'] = None

+         if data.get('release') is None:

+             data['release'] = get_next_release(build_info)

          build_id = new_build(data)

          data['id'] = build_id

          new_image_build(data)

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-ready
- Pull-request tagged with: no_qe

2 years ago

I'm a little worried about the scratch case here. If the release is not specified, we're going to pass None through to the subtasks. I don't think it will cause a failure, but it's going to be visible and possibly confusing.

Three possibilities come to mind to set a release value for scratch builds:

  1. use a fixed release value. e.g. 1.scratch, since these are just scratch builds
  2. incorporate a timestamp. e.g. "%i-scratch" % time.time()
  3. just use getNextRelease() as before for the scratch case. The race is not an issue for scratch builds
  4. same as previous, but append .scratch for emphasis

Option 1 is the easiest, but I'd be a little worried that even though Koji won't mind these scratch builds having the same names, some other external process could be confused.

Option 2 seems safe, but is different than what we were doing before. The scratch nvrs would appear out of order compared to normal image builds.

Option 3 is probably the safest, as it preserves the behavior we had for scratch builds before.

Option 4 seems like a nice variation on the above, but is a behavior change.

On the whole, I would lean towards option 3.

I would also stay with 3 for now.

Commit fc4cb3d fixes this pull-request

Pull-Request has been merged by tkopecek

2 years ago