From 8d8beb5b0955c58fe4755719e54ec7b397751ce4 Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Jan 21 2019 11:40:03 +0000 Subject: [PATCH 1/2] Do not wait for result of Pulp compose - we can add multiple compose to single Event for long time. They are stored in ArtifactBuild.composes and Pulp compose is also already added there by its only caller - the koji.RebuildImagesOnRPMAdvisoryChange method. --- diff --git a/freshmaker/odcsclient.py b/freshmaker/odcsclient.py index 7486afa..b9c9ee2 100644 --- a/freshmaker/odcsclient.py +++ b/freshmaker/odcsclient.py @@ -298,32 +298,6 @@ class FreshmakerODCSClient(object): with krb_context(): new_compose = odcs.new_compose( ' '.join(content_sets), 'pulp') - - # Pulp composes in ODCS takes just few seconds, because ODCS - # only generates the .repo file after single query to Pulp. - # TODO: Freshmaker is currently not designed to handle - # multiple ODCS composes per rebuild Event and since these - # composes are done in no-time normally, it is OK here to - # block. It would still be nice to redesign that part of - # Freshmaker to do things "right". - # This is tracked here: https://pagure.io/freshmaker/issue/114 - @retry(timeout=60 * 30, interval=2) - def wait_for_compose(compose_id): - ret = odcs.get_compose(compose_id) - if ret["state_name"] == "done": - return True - elif ret["state_name"] == "failed": - return False - self.handler.log_info( - "Waiting for Pulp compose to finish: %r", ret) - raise Exception("ODCS compose not finished.") - - done = wait_for_compose(new_compose["id"]) - if not done: - build.transition( - ArtifactBuildState.FAILED.value, "Cannot generate " - "ODCS PULP compose %s for content_sets %r" - % (str(new_compose["id"]), content_sets)) else: new_compose = self._fake_odcs_new_compose( content_sets, 'pulp') From 3352be1f1ef76957b62a6ccf44ae6be3fff6a856 Mon Sep 17 00:00:00 2001 From: Jan Kaluza Date: Jan 21 2019 11:54:42 +0000 Subject: [PATCH 2/2] Fix RebuildImagesOnODCSComposeDone to check all the images and not just those in the first batch. The RebuildImagesOnODCSComposeDone currently checks only the images in the first building batch (images with `dep_on == None`). This is wrong, because there might be a situation when even image with some parent image (dep_on set to another image) waits for a ODCS compose. The chance for this to happen are small, because usually the ODCS compose needed by child images are done by the time the matching parent image is built, but with the change of the code to not wait for result of Pulp compose, we might never know for sure. --- diff --git a/freshmaker/handlers/koji/rebuild_images_on_odcs_compose_done.py b/freshmaker/handlers/koji/rebuild_images_on_odcs_compose_done.py index af8fffe..12b98ea 100644 --- a/freshmaker/handlers/koji/rebuild_images_on_odcs_compose_done.py +++ b/freshmaker/handlers/koji/rebuild_images_on_odcs_compose_done.py @@ -48,13 +48,22 @@ class RebuildImagesOnODCSComposeDone(ContainerBuildHandler): self.force_dry_run() query = db.session.query(ArtifactBuild).join('composes') - first_batch_builds = query.filter( - ArtifactBuild.dep_on == None, # noqa + # Get all the builds waiting for this compose in PLANNED state ... + builds_ready_to_rebuild = query.filter( ArtifactBuild.state == ArtifactBuildState.PLANNED.value, Compose.odcs_compose_id == event.compose['id']) - if self.dry_run: - builds_ready_to_rebuild = first_batch_builds - else: + # ... and depending on DONE parent image or parent image which is + # not planned to be built in this Event (dep_on == None). + builds_ready_to_rebuild = [ + b for b in builds_ready_to_rebuild if + b.dep_on is None or b.dep_on.state == ArtifactBuildState.DONE.value + ] + + if not self.dry_run: + # In non-dry-run mode, check that all the composes are ready. + # In dry-run mode, the composes are fake, so they are always ready. builds_ready_to_rebuild = six.moves.filter( - lambda build: build.composes_ready, first_batch_builds) + lambda build: build.composes_ready, builds_ready_to_rebuild) + + # Start the rebuild. self.start_to_build_images(builds_ready_to_rebuild) diff --git a/freshmaker/odcsclient.py b/freshmaker/odcsclient.py index b9c9ee2..8d6380c 100644 --- a/freshmaker/odcsclient.py +++ b/freshmaker/odcsclient.py @@ -44,7 +44,7 @@ from freshmaker.errata import Errata from freshmaker.kojiservice import koji_service from freshmaker.consumer import work_queue_put from freshmaker.types import ArtifactBuildState -from freshmaker.utils import krb_context, retry +from freshmaker.utils import krb_context from freshmaker.events import ODCSComposeStateChangeEvent diff --git a/tests/handlers/koji/test_rebuild_images_on_odcs_compose_done.py b/tests/handlers/koji/test_rebuild_images_on_odcs_compose_done.py index 8904efc..2c41f48 100644 --- a/tests/handlers/koji/test_rebuild_images_on_odcs_compose_done.py +++ b/tests/handlers/koji/test_rebuild_images_on_odcs_compose_done.py @@ -119,3 +119,21 @@ class TestRebuildImagesOnODCSComposeDone(helpers.ModelsTestCase): args, kwargs = start_to_build_images.call_args passed_builds = sorted(args[0], key=lambda build: build.id) self.assertEqual([self.build_1, self.build_3], passed_builds) + + @patch('freshmaker.models.ArtifactBuild.composes_ready', + new_callable=PropertyMock) + @patch('freshmaker.handlers.ContainerBuildHandler.start_to_build_images') + def test_start_to_build_parent_image_done(self, start_to_build_images, composes_ready): + composes_ready.return_value = True + self.build_1.state = ArtifactBuildState.DONE.value + + event = ODCSComposeStateChangeEvent( + 'msg-id', {'id': self.compose_1.id, 'state': 'done'} + ) + + handler = RebuildImagesOnODCSComposeDone() + handler.handle(event) + + args, kwargs = start_to_build_images.call_args + passed_builds = sorted(args[0], key=lambda build: build.id) + self.assertEqual([self.build_3, self.build_2], passed_builds)