PR#498: Generate rebuilt_nvr just before submitting an image build - freshmaker

freshmaker

#498 Generate rebuilt_nvr just before submitting an image build

Merged 4 years ago by cqi. Opened 4 years ago by cqi.

cqi/freshmaker generate-rebuilt_nvr-release-when-build into master

Generate rebuilt_nvr just before submitting an image build

Chenxiong Qi • 4 years ago

97d57a9

freshmaker/handlers/__init__.py

file modified

+23 -15

		`@@ -493,6 +493,12 @@`
		`"Container image does not have 'build_args' filled in.")`
		`return`

		`+ if not build.original_nvr:`
		`+ build.transition(`
		`+ ArtifactBuildState.FAILED.value,`
		`+ "Container image does not have original_nvr set.")`
		`+ return`
		`+`
		`args = json.loads(build.build_args)`
		`scm_url = "%s/%s#%s" % (conf.git_base_url, args["repository"],`
		`args["commit"])`
		`@@ -513,18 +519,6 @@`
		`# of the build tag associated with the target.`
		`arches = args.get("arches")`

		`- if not build.rebuilt_nvr and build.original_nvr:`
		`- build.rebuilt_nvr = get_rebuilt_nvr(`
		`- build.type, build.original_nvr)`
		`-`
		`- if not build.rebuilt_nvr:`
		`- build.transition(`
		`- ArtifactBuildState.FAILED.value,`
		`- "Container image does not have rebuilt_nvr set.")`
		`- return`
		`-`
		`- release = parse_NVR(build.rebuilt_nvr)["release"]`
		`-`
		`# Get the list of ODCS compose IDs which should be used to build`
		`# the image.`
		`compose_ids = []`
		`@@ -546,10 +540,24 @@`
		`# OSBS can renew a compose if it needs to, so we can just pass`
		`# it along without further verification for other states.`

		`+ rebuilt_nvr = get_rebuilt_nvr(build.type, build.original_nvr)`
		`+ if build.rebuilt_nvr is not None:`
		`+ self.log_debug(`
		`+ "Artifact build %s has rebuilt_nvr %s already. "`
		`+ "It will be replaced with a new one %s to be rebuilt.",`
		`+ build, build.rebuilt_nvr, rebuilt_nvr)`
		`+`
		`+ build.rebuilt_nvr = rebuilt_nvr`
		`+ db.session.commit()`
		`+`
		`return self.build_container(`
		`- scm_url, branch, target, repo_urls=repo_urls,`
		`- isolated=True, release=release, koji_parent_build=parent,`
		`- arch_override=arches, compose_ids=compose_ids)`
		`+ scm_url, branch, target,`
		`+ repo_urls=repo_urls,`
		`+ isolated=True,`
		`+ release=parse_NVR(build.rebuilt_nvr)["release"],`
		`+ koji_parent_build=parent,`
		`+ arch_override=arches,`
		`+ compose_ids=compose_ids)`

		`def odcs_get_compose(self, compose_id):`
		`"""`

freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py

file modified

-6

		`@@ -34,7 +34,6 @@`
		`fail_event_on_handler_exception)`
		`from freshmaker.kojiservice import koji_service`
		`from freshmaker.types import ArtifactType, ArtifactBuildState, EventState`
		`- from freshmaker.utils import get_rebuilt_nvr`


		`class RebuildImagesOnParentImageBuild(ContainerBuildHandler):`
		`@@ -100,11 +99,6 @@`
		`args["retry_count"] += 1`
		`found_build.build_args = json.dumps(args)`
		`if args["retry_count"] < 3:`
		`- # Change the rebuilt_nvr, because Koji/OSBS might be in weird`
gnaponie commented 4 years ago Why are you removing this? Is it because it gets done in the `__init__.py`? Edited 4 years ago by gnaponie
cqi commented 4 years ago Why are you removing this? Is it because it gets done in the init.py? Yes.
		`- # state in which the build for old NVR already exists and rebuild`
		`- # would fail because of NVR conflict.`
		`- found_build.rebuilt_nvr = get_rebuilt_nvr(`
		`- found_build.type, found_build.original_nvr)`
		`found_build.transition(`
		`ArtifactBuildState.PLANNED.value,`
		`"Retrying failed build %s" % (str(found_build.build_id)))`

freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py

file modified

-3

		`@@ -36,7 +36,6 @@`
		`from freshmaker.types import (`
		`ArtifactType, ArtifactBuildState, EventState, RebuildReason)`
		`from freshmaker.models import Event, Compose`
		`- from freshmaker.utils import get_rebuilt_nvr`


		`class RebuildImagesOnRPMAdvisoryChange(ContainerBuildHandler):`
		`@@ -263,7 +262,6 @@`
		`state_reason = ""`
		`state = ArtifactBuildState.PLANNED.value`

		`- rebuilt_nvr = get_rebuilt_nvr(ArtifactType.IMAGE.value, nvr)`
		`image_name = koji.parse_NVR(image["brew"]["build"])["name"]`

		`# Only released images are considered as directly affected for`
		`@@ -280,7 +278,6 @@`
		`dep_on=dep_on,`
		`state=ArtifactBuildState.PLANNED.value,`
		`original_nvr=nvr,`
		`- rebuilt_nvr=rebuilt_nvr,`
		`rebuild_reason=rebuild_reason)`

		`# Set context to particular build so logging shows this build`

tests/handlers/koji/test_rebuild_images_on_parent_image_build.py

file modified

+2 -6

		`@@ -90,23 +90,21 @@`
		`self.assertEqual(build_1.build_id, 2)`
		`self.assertEqual(build_2.build_id, 3)`

		`- @mock.patch('freshmaker.handlers.koji.rebuild_images_on_parent_image_build.get_rebuilt_nvr')`
		`@mock.patch('freshmaker.handlers.ContainerBuildHandler.build_image_artifact_build')`
		`@mock.patch('freshmaker.handlers.ContainerBuildHandler.get_repo_urls')`
		`def test_not_build_containers_when_dependency_container_build_task_failed(`
		`- self, repo_urls, build_image, rebuilt_nvr):`
		`+ self, repo_urls, build_image):`
		`"""`
		`Tests when dependency container build task failed in brew, only update build state in db.`
		`"""`
		`build_image.side_effect = [1, 2, 3, 4]`
		`repo_urls.return_value = ["url"]`
		`- rebuilt_nvr.side_effect = ["foo-1-1.2", "foo-1-1.3"]`
		`e1 = models.Event.create(db.session, "test_msg_id", "RHSA-2018-001", events.TestingEvent)`
		`event = self.get_event_from_msg(get_fedmsg('brew_container_task_failed'))`

		`base_build = models.ArtifactBuild.create(`
		`db.session, e1, 'test-product-docker', ArtifactType.IMAGE, event.task_id,`
		`- original_nvr='foo-1-1', rebuilt_nvr='foo-1-1.1')`
		`+ original_nvr='foo-1-1')`
		`base_build.build_args = json.dumps({})`

		`models.ArtifactBuild.create(db.session, e1, 'docker-up', ArtifactType.IMAGE, 0,`
		`@@ -114,12 +112,10 @@`
		`self.handler.handle(event)`
		`self.assertEqual(base_build.state, ArtifactBuildState.BUILD.value)`
		`self.assertEqual(base_build.build_id, 1)`
		`- self.assertEqual(base_build.rebuilt_nvr, "foo-1-1.2")`
		`event.task_id = 1`
		`self.handler.handle(event)`
		`self.assertEqual(base_build.state, ArtifactBuildState.BUILD.value)`
		`self.assertEqual(base_build.build_id, 2)`
		`- self.assertEqual(base_build.rebuilt_nvr, "foo-1-1.3")`
		`event.task_id = 2`
		`self.handler.handle(event)`
		`self.assertEqual(base_build.state, ArtifactBuildState.FAILED.value)`

tests/test_handler.py

file modified

+17 -10

		`@@ -124,13 +124,14 @@`

		`self.build_1 = ArtifactBuild.create(`
		`db.session, self.event, 'build-1', ArtifactType.IMAGE,`
		`- state=ArtifactBuildState.PLANNED)`
		`- self.build_1.rebuilt_nvr = "foo-1-2"`
		`+ state=ArtifactBuildState.PLANNED,`
		`+ original_nvr="foo-1-2")`
		`self.build_1.build_args = json.dumps(build_args)`
		`+`
		`self.build_2 = ArtifactBuild.create(`
		`db.session, self.event, 'build-2', ArtifactType.IMAGE,`
		`- state=ArtifactBuildState.PLANNED)`
		`- self.build_2.rebuilt_nvr = "foo-2-2"`
		`+ state=ArtifactBuildState.PLANNED,`
		`+ original_nvr="foo-2-2")`
		`self.build_2.build_args = json.dumps(build_args)`

		`db.session.commit()`
		`@@ -187,19 +188,23 @@`
		`repos = handler.get_repo_urls(build_3)`
		`self.assertEqual(repos, ["http://localhost/test.repo"])`

		`+ @patch("time.time")`
		`@patch("freshmaker.handlers.ContainerBuildHandler.build_container")`
		`def test_build_image_artifact_build_only_odcs_composes(`
		`- self, build_container):`
		`+ self, build_container, time):`
		`+ time.return_value = 1234567.1234`
		`handler = MyHandler()`
		`handler.build_image_artifact_build(self.build_1)`
		`build_container.assert_called_once_with(`
		`'git://pkgs.fedoraproject.org/repo#hash', 'branch', 'target',`
		`arch_override='x86_64', compose_ids=[5, 6, 7, 8], isolated=True,`
		`- koji_parent_build=None, release='2', repo_urls=None)`
		`+ koji_parent_build=None, release='2.1234567', repo_urls=None)`

		`+ @patch("time.time")`
		`@patch("freshmaker.handlers.ContainerBuildHandler.build_container")`
		`def test_build_image_artifact_build_renewed_odcs_composes(`
		`- self, build_container):`
		`+ self, build_container, time):`
		`+ time.return_value = 1234567.1234`
		`build_args = json.loads(self.build_1.build_args)`
		`build_args["renewed_odcs_compose_ids"] = [7300, 7301]`
		`self.build_1.build_args = json.dumps(build_args)`
		`@@ -210,11 +215,13 @@`
		`build_container.assert_called_once_with(`
		`'git://pkgs.fedoraproject.org/repo#hash', 'branch', 'target',`
		`arch_override='x86_64', compose_ids=[5, 6, 7, 8, 7300, 7301],`
		`- isolated=True, koji_parent_build=None, release='2', repo_urls=None)`
		`+ isolated=True, koji_parent_build=None, release='2.1234567', repo_urls=None)`

		`+ @patch("time.time")`
		`@patch("freshmaker.handlers.ContainerBuildHandler.build_container")`
		`def test_build_image_artifact_build_repo_urls(`
		`- self, build_container):`
		`+ self, build_container, time):`
		`+ time.return_value = 1234567.1234`
		`handler = MyHandler()`
		`handler.build_image_artifact_build(self.build_1, ["http://localhost/x.repo"])`

		`@@ -222,7 +229,7 @@`
		`build_container.assert_called_once_with(`
		`'git://pkgs.fedoraproject.org/repo#hash', 'branch', 'target',`
		`arch_override='x86_64', compose_ids=[5, 6, 7, 8], isolated=True,`
		`- koji_parent_build=None, release='2', repo_urls=repo_urls)`
		`+ koji_parent_build=None, release='2.1234567', repo_urls=repo_urls)`

		`@patch("freshmaker.handlers.ContainerBuildHandler.build_container")`
		`def test_build_image_artifact_build_repo_urls_compose_not_ready(`

cqi commented 4 years ago

This change ensures the timestamp inside the release part of a
rebuilt_nvr just reflects when the image build is submitted.

Resolves: FACTORY-5949

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

rebased onto 281cb66c5a99e6b1ed55725f2f2cf471d192cb2b

4 years ago

rebased onto daa83deab2831798dcc9146a0ebcbef5b49d5064

4 years ago

cqi commented 4 years ago

Ping for review

gnaponie commented on line 13 of freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py 4 years ago

Why are you removing this? Is it because it gets done in the __init__.py?

Edited 4 years ago by gnaponie

lucarval commented on line 14 of freshmaker/handlers/__init__.py 4 years ago

What about the few lines above?

516         if not build.rebuilt_nvr and build.original_nvr:                                            
517             build.rebuilt_nvr = get_rebuilt_nvr(                                                    
518                 build.type, build.original_nvr)                                                     
519                                                                                                     
520         if not build.rebuilt_nvr:                                                                   
521             build.transition(                                                                       
522                 ArtifactBuildState.FAILED.value,                                                    
523                 "Container image does not have rebuilt_nvr set.")                                   
524             return   
525                                                                                                     
526         release = parse_NVR(build.rebuilt_nvr)["release"]

Should we just move this whole chunk down to just before self.build_container is called?

rebased onto f9291c345869a9be8c300a8197c895d2656246f6

4 years ago

rebased onto 1a0340b8688a7e3ca9f297b0a2b67a72da1cde3b

4 years ago

cqi commented on line 13 of freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py 4 years ago

Why are you removing this? Is it because it gets done in the init.py?

Yes.

cqi commented 4 years ago

@lucarval Updated. PTAL.

gnaponie commented 4 years ago

Looks good to me +1
but wait for @lucarval final review before merging.

lucarval commented on line 5 of freshmaker/handlers/__init__.py 4 years ago

Nice catch!

lucarval commented on line 39 of freshmaker/handlers/__init__.py 4 years ago

For debugging purposes, it would be nice if both of these log messages were combined.

lucarval commented on line 43 of freshmaker/handlers/__init__.py 4 years ago

Committing the DB changes makes sense, but I wonder why it wasn't done before. Any ideas?

lucarval commented 4 years ago

Left some optional comments, but I don't see any functional issues. :thumbsup:

cqi commented on line 43 of freshmaker/handlers/__init__.py 4 years ago

My understand is, this function is called by start_to_build_images where db.session.commit is called, and if ODCSComposeNotReady is raised, the event handler ensures db.session.commit is called eventually