From cc7464b668904ec5bb740c32335c97108e247bbf Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Jun 28 2019 21:32:26 +0000 Subject: Do not mark the event failed in-case of a service failure Currently, when Freshmaker fails because of some DNS error while handling a build, it marks the entire event as failed. That is, all successfull builds are also marked as failed. This change changes this behavior by only marking the current build in consideration as failed and not the entire event. --- 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 4bcaa43..e46f9d7 100644 --- a/freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py +++ b/freshmaker/handlers/koji/rebuild_images_on_parent_image_build.py @@ -29,8 +29,8 @@ from freshmaker.errata import Errata from freshmaker.events import ( BrewContainerTaskStateChangeEvent, ErrataAdvisoryRPMsSignedEvent) from freshmaker.models import ArtifactBuild, EVENT_TYPES -from freshmaker.handlers import ( - ContainerBuildHandler, fail_event_on_handler_exception) +from freshmaker.handlers import (ContainerBuildHandler, + fail_artifact_build_on_handler_exception) from freshmaker.kojiservice import koji_service from freshmaker.types import ArtifactType, ArtifactBuildState, EventState from freshmaker.utils import get_rebuilt_nvr @@ -44,7 +44,6 @@ class RebuildImagesOnParentImageBuild(ContainerBuildHandler): def can_handle(self, event): return isinstance(event, BrewContainerTaskStateChangeEvent) - @fail_event_on_handler_exception def handle(self, event): """ When build container task state changed in brew, update build state in @@ -66,69 +65,77 @@ class RebuildImagesOnParentImageBuild(ContainerBuildHandler): if found_build.event.state not in [EventState.INITIALIZED.value, EventState.BUILDING.value]: return - # update build state in db - if event.new_state == 'CLOSED': - # if build is triggered by an advisory, verify the container - # contains latest RPMs from the advisory - if found_build.event.event_type_id == EVENT_TYPES[ErrataAdvisoryRPMsSignedEvent]: - errata_id = found_build.event.search_key - # build_id is actually task id in build system, find out the actual build first - with koji_service( - conf.koji_profile, log, login=False, - dry_run=self.dry_run) as session: - container_build_id = session.get_container_build_id_from_task(build_id) - - ret, msg = self._verify_advisory_rpms_in_container_build(errata_id, container_build_id) - if ret: - found_build.transition(ArtifactBuildState.DONE.value, "Built successfully.") - else: - found_build.transition(ArtifactBuildState.FAILED.value, msg) - - # for other builds, mark them as DONE - else: + self.update_db_build_state(build_id, found_build, event) + self.rebuild_dependent_containers(found_build) + + @fail_artifact_build_on_handler_exception() + def update_db_build_state(self, build_id, found_build, event): + """ Update build state in db. """ + if event.new_state == 'CLOSED': + # if build is triggered by an advisory, verify the container + # contains latest RPMs from the advisory + if found_build.event.event_type_id == EVENT_TYPES[ErrataAdvisoryRPMsSignedEvent]: + errata_id = found_build.event.search_key + # build_id is actually task id in build system, find out the actual build first + with koji_service( + conf.koji_profile, log, login=False, + dry_run=self.dry_run) as session: + container_build_id = session.get_container_build_id_from_task(build_id) + + ret, msg = self._verify_advisory_rpms_in_container_build(errata_id, container_build_id) + if ret: found_build.transition(ArtifactBuildState.DONE.value, "Built successfully.") - if event.new_state == 'FAILED': - args = json.loads(found_build.build_args) - if "retry_count" not in args: - args["retry_count"] = 0 - 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))) - self.start_to_build_images([found_build]) else: - found_build.transition( - ArtifactBuildState.FAILED.value, - "Failed to build in Koji.") - db.session.commit() - - if found_build.state == ArtifactBuildState.DONE.value: - # check db to see whether there is any planned image build - # depends on this build - planned_builds = db.session.query(ArtifactBuild).filter_by( - type=ArtifactType.IMAGE.value, - state=ArtifactBuildState.PLANNED.value, - dep_on=found_build - ).all() - - log.info("Found following PLANNED builds to rebuild that " - "depends on %r", found_build) - for build in planned_builds: - log.info(" %r", build) - - self.start_to_build_images(planned_builds) - - # Finally, we check if all builds scheduled by event - # found_build.event (ErrataAdvisoryRPMsSignedEvent) have been - # switched to FAILED or COMPLETE. If yes, mark the event COMPLETE. - self._mark_event_complete_when_all_builds_done(found_build.event) + found_build.transition(ArtifactBuildState.FAILED.value, msg) + + # for other builds, mark them as DONE + else: + found_build.transition(ArtifactBuildState.DONE.value, "Built successfully.") + if event.new_state == 'FAILED': + args = json.loads(found_build.build_args) + if "retry_count" not in args: + args["retry_count"] = 0 + 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))) + self.start_to_build_images([found_build]) + else: + found_build.transition( + ArtifactBuildState.FAILED.value, + "Failed to build in Koji.") + db.session.commit() + + @fail_artifact_build_on_handler_exception() + def rebuild_dependent_containers(self, found_build): + """ Rebuild containers depend on the success build as necessary. """ + if found_build.state == ArtifactBuildState.DONE.value: + # check db to see whether there is any planned image build + # depends on this build + planned_builds = db.session.query(ArtifactBuild).filter_by( + type=ArtifactType.IMAGE.value, + state=ArtifactBuildState.PLANNED.value, + dep_on=found_build + ).all() + + log.info("Found following PLANNED builds to rebuild that " + "depends on %r", found_build) + for build in planned_builds: + log.info(" %r", build) + + self.start_to_build_images(planned_builds) + + # Finally, we check if all builds scheduled by event + # found_build.event (ErrataAdvisoryRPMsSignedEvent) have been + # switched to FAILED or COMPLETE. If yes, mark the event COMPLETE. + self._mark_event_complete_when_all_builds_done(found_build.event) def _mark_event_complete_when_all_builds_done(self, db_event): """Mark ErrataAdvisoryRPMsSignedEvent COMPLETE 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 e78869e..a0ed381 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 @@ -269,6 +269,41 @@ class TestRebuildImagesOnParentImageBuild(helpers.ModelsTestCase): self.assertEqual(build.state, ArtifactBuildState.FAILED.value) six.assertRegex(self, build.state_reason, r"The following RPMs in container build.*") + @mock.patch('freshmaker.handlers.ContainerBuildHandler.build_image_artifact_build') + @mock.patch('freshmaker.handlers.ContainerBuildHandler.get_repo_urls') + @mock.patch('freshmaker.handlers.koji.rebuild_images_on_parent_image_build.' + 'RebuildImagesOnParentImageBuild.update_db_build_state', + side_effect=RuntimeError('something went wrong!')) + def test_no_event_state_change_if_service_fails( + self, update_db, get_repo_urls, build_image_artifact_build): + build_image_artifact_build.return_value = 67890 + + self.db_advisory_rpm_signed_event = models.Event.create( + db.session, 'msg-id-123', '12345', + events.ErrataAdvisoryStateChangedEvent, + state=EventState.BUILDING.value) + + self.image_a_build = models.ArtifactBuild.create( + db.session, self.db_advisory_rpm_signed_event, + 'image-a-0.1-1', ArtifactType.IMAGE, + build_id=12345, + state=ArtifactBuildState.PLANNED.value) + + db.session.commit() + + state_changed_event = events.BrewContainerTaskStateChangeEvent( + 'msg-id-890', 'image-a', 'branch', 'target', 12345, + 'BUILD', 'FAILED') + + handler = RebuildImagesOnParentImageBuild() + with self.assertRaises(RuntimeError): + handler.handle(state_changed_event) + + # As self.image_b_build starts to be rebuilt, not all images are + # rebuilt yet. + self.assertEqual(EventState.BUILDING.value, + self.db_advisory_rpm_signed_event.state) + if __name__ == '__main__': unittest.main()