From 97d57a9db975b58b43113e15d29e35de6c1a3f0b Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Mar 24 2020 02:17:15 +0000 Subject: Generate rebuilt_nvr just before submitting an image build This change ensures the timestamp inside the release part of a rebuilt_nvr just reflects time when the image build is submitted. Resolves: FACTORY-5949 Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/handlers/__init__.py b/freshmaker/handlers/__init__.py index a7de567..852f547 100644 --- a/freshmaker/handlers/__init__.py +++ b/freshmaker/handlers/__init__.py @@ -493,6 +493,12 @@ class ContainerBuildHandler(BaseHandler): "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 @@ class ContainerBuildHandler(BaseHandler): # 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 @@ class ContainerBuildHandler(BaseHandler): # 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): """ diff --git a/freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py b/freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py index d3f067f..4acfaa1 100644 --- a/freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py +++ b/freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py @@ -34,7 +34,6 @@ from freshmaker.handlers import (ContainerBuildHandler, 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 @@ class RebuildImagesOnParentImageBuild(ContainerBuildHandler): 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 - # 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))) diff --git a/freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py b/freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py index e545586..e49976b 100644 --- a/freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py +++ b/freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py @@ -36,7 +36,6 @@ from freshmaker.errata import Errata 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 @@ class RebuildImagesOnRPMAdvisoryChange(ContainerBuildHandler): 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 @@ class RebuildImagesOnRPMAdvisoryChange(ContainerBuildHandler): 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 diff --git a/tests/handlers/koji/test_rebuild_images_on_parent_image_build.py b/tests/handlers/koji/test_rebuild_images_on_parent_image_build.py index 4f97f12..9c5e146 100644 --- a/tests/handlers/koji/test_rebuild_images_on_parent_image_build.py +++ b/tests/handlers/koji/test_rebuild_images_on_parent_image_build.py @@ -90,23 +90,21 @@ class TestRebuildImagesOnParentImageBuild(helpers.ModelsTestCase): 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 @@ class TestRebuildImagesOnParentImageBuild(helpers.ModelsTestCase): 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) diff --git a/tests/test_handler.py b/tests/test_handler.py index 6970d4f..f1bc6f4 100644 --- a/tests/test_handler.py +++ b/tests/test_handler.py @@ -124,13 +124,14 @@ class TestGetRepoURLs(helpers.ModelsTestCase): 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 @@ class TestGetRepoURLs(helpers.ModelsTestCase): 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 @@ class TestGetRepoURLs(helpers.ModelsTestCase): 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 @@ class TestGetRepoURLs(helpers.ModelsTestCase): 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(