PR#2263: improve race condition for getNextRelease / images - koji

koji

#2263 improve race condition for getNextRelease / images

Merged 3 years ago by tkopecek. Opened 3 years ago by tkopecek.

tkopecek/koji issue2138 into master

improve race condition for getNextRelease / images

Tomas Kopecek • 3 years ago

1595d7d

builder/kojid

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'),`

hub/kojihub.py

file modified

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

tkopecek commented 3 years ago

Fixes: https://pagure.io/koji/issue/2138

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

3 years ago

julian8628 commented 3 years ago

:thumbsup:

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

3 years ago

mikem commented 3 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:

use a fixed release value. e.g. 1.scratch, since these are just scratch builds
incorporate a timestamp. e.g. "%i-scratch" % time.time()
just use getNextRelease() as before for the scratch case. The race is not an issue for scratch builds
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.

tkopecek commented 3 years ago

I would also stay with 3 for now.

tkopecek commented 3 years ago

Commit fc4cb3d fixes this pull-request

Pull-Request has been merged by tkopecek

3 years ago