diff --git a/tests/fixtures/layouts/crd-github.yaml b/tests/fixtures/layouts/crd-github.yaml index bc938dec37..7fa3b72d19 100644 --- a/tests/fixtures/layouts/crd-github.yaml +++ b/tests/fixtures/layouts/crd-github.yaml @@ -19,6 +19,8 @@ github: - event: pull_request action: edited + - event: pull_request + action: reopened start: github: {} success: diff --git a/tests/unit/test_github_crd.py b/tests/unit/test_github_crd.py index d35cb73b75..e5dd2f7ae4 100644 --- a/tests/unit/test_github_crd.py +++ b/tests/unit/test_github_crd.py @@ -215,3 +215,45 @@ class TestGithubCrossRepoDeps(ZuulTestCase): # The job should be aborted since B was updated while enqueued. self.assertHistory([dict(name='project3-test', result='ABORTED')]) + + @simple_layout('layouts/crd-github.yaml', driver='github') + def test_crd_dependent_close_reopen(self): + """ + Test that closing and reopening PR correctly re-enqueues the + dependent change in the correct order. + """ + self.executor_server.hold_jobs_in_build = True + + A = self.fake_github.openFakePullRequest('org/project3', 'master', 'A') + B = self.fake_github.openFakePullRequest( + 'org/project4', 'master', 'B', + body=f"Depends-On: https://github.com/org/project3/pull/{A.number}" + ) + + self.fake_github.emitEvent(B.getPullRequestEditedEvent()) + self.waitUntilSettled() + + # Close and reopen PR A + self.fake_github.emitEvent(A.getPullRequestClosedEvent()) + self.fake_github.emitEvent(A.getPullRequestReopenedEvent()) + self.waitUntilSettled() + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + # The changes for the job from project4 should include the project3 + # PR content + self.assertHistory([ + dict(name='project3-test', result='ABORTED', + changes=f"{A.number},{A.head_sha}"), + dict(name='project4-test', result='ABORTED', + changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha}"), + dict(name='project3-test', result='SUCCESS', + changes=f"{A.number},{A.head_sha}"), + dict(name='project4-test', result='SUCCESS', + changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha}"), + ], ordered=False) + + self.assertTrue(A.is_merged) + self.assertTrue(B.is_merged) diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index cd98b6f5a1..a62e95a50a 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -312,10 +312,15 @@ class PipelineManager(metaclass=ABCMeta): return True return False - def isChangeAlreadyInQueue(self, change, change_queue): + def isChangeAlreadyInQueue(self, change, change_queue, item=None): # Checks any item in the specified change queue - for item in change_queue.queue: - for c in item.changes: + # If item is supplied, only consider items ahead of the + # supplied item (ie, is the change already in the queue ahead + # of this item?). + for queue_item in change_queue.queue: + if queue_item is item: + break + for c in queue_item.changes: if change.equals(c): return True return False @@ -419,13 +424,18 @@ class PipelineManager(metaclass=ABCMeta): return True def getMissingNeededChanges(self, changes, change_queue, event, - dependency_graph=None): + dependency_graph=None, item=None): """Check that all needed changes are ahead in the queue. Return a list of any that are missing. If it is not possible to correct the missing changes, "abort" will be true. + If item is supplied, only consider items ahead of the supplied + item when determining whether needed changes are already ahead + in the queue. + :returns: (abort, needed_changes) + """ return False, [] @@ -472,23 +482,71 @@ class PipelineManager(metaclass=ABCMeta): def removeAbandonedChange(self, change, event): log = get_annotated_logger(self.log, event) log.debug("Change %s abandoned, removing", change) - for item in self.pipeline.getAllItems(): - if not item.live: + for queue in self.pipeline.queues: + # Below we need to remove dependent changes of abandoned + # changes we remove here, but only if both are live. + # Therefore, track the changes we remove independently for + # each shared queue (since different queues may have + # different live/non-live changes). + changes_removed = set() + for item in queue.queue: + if not item.live: + continue + self._removeAbandonedChangeExamineItem( + change, item, changes_removed) + # Abbreviated version of dependency check; we're not going + # to check everything (processOneItem will take care of + # that), but we will remove any changes that depend on any + # changes we just removed; we can do that without + # recreating the full dependency graphs. + if not changes_removed: continue - for item_change in item.changes: - if item_change.equals(change): - if len(item.changes) > 1: - # We need to cancel all jobs here as setting - # dequeued needing change will skip all jobs. - self.cancelJobs(item) - msg = ("Dependency cycle change " - f"{change.url} abandoned.") + for item in queue.queue: + if not item.live: + continue + self._removeAbandonedChangeDependentsExamineItem( + log, item, changes_removed) + + def _removeAbandonedChangeExamineItem(self, change, item, changes_removed): + # The inner loop from the first part of removeAbandonedChange. + for item_change in item.changes: + if item_change.equals(change): + if len(item.changes) > 1: + # We need to cancel all jobs here as setting + # dequeued needing change will skip all jobs. + self.cancelJobs(item) + msg = ("Dependency cycle change " + f"{change.url} abandoned.") + item.setDequeuedNeedingChange(msg) + try: + self.reportItem(item) + except exceptions.MergeFailure: + pass + self.removeItem(item) + changes_removed.update(item.changes) + return + + def _removeAbandonedChangeDependentsExamineItem( + self, log, item, changes_removed): + # The inner loop from the second part of removeAbandonedChange. + for item_change in item.changes: + for needed_change in self.resolveChangeReferences( + item_change.getNeedsChanges( + self.useDependenciesByTopic(item_change.project))): + for removed_change in changes_removed: + if needed_change.equals(removed_change): + log.debug("Change %s depends on abandoned change %s, " + "removing", item_change, removed_change) + msg = f'Change {removed_change} is needed.' item.setDequeuedNeedingChange(msg) - try: - self.reportItem(item) - except exceptions.MergeFailure: - pass - self.removeItem(item) + if item.live: + try: + self.reportItem(item) + except exceptions.MergeFailure: + pass + self.dequeueItem(item) + changes_removed.update(item.changes) + return def reEnqueueChanges(self, item, changes): for change in changes: @@ -1659,7 +1717,8 @@ class PipelineManager(metaclass=ABCMeta): abort, needs_changes = self.getMissingNeededChanges( item.changes, change_queue, item.event, - dependency_graph=dependency_graph) + dependency_graph=dependency_graph, + item=item) if not (meets_reqs and not needs_changes): # It's not okay to enqueue this change, we should remove it. log.info("Dequeuing %s because " diff --git a/zuul/manager/dependent.py b/zuul/manager/dependent.py index 87d1a5136d..427424cb02 100644 --- a/zuul/manager/dependent.py +++ b/zuul/manager/dependent.py @@ -183,7 +183,8 @@ class DependentPipelineManager(SharedQueuePipelineManager): return True def getMissingNeededChanges(self, changes, change_queue, event, - dependency_graph=None, warnings=None): + dependency_graph=None, warnings=None, + item=None): log = get_annotated_logger(self.log, event) changes_needed = [] abort = False @@ -231,7 +232,7 @@ class DependentPipelineManager(SharedQueuePipelineManager): log.debug(" Needed change is in cycle") continue if self.isChangeAlreadyInQueue( - needed_change, change_queue): + needed_change, change_queue, item): log.debug(" Needed change is already " "ahead in the queue") continue diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py index ad1d0bfcb2..a662111a65 100644 --- a/zuul/manager/independent.py +++ b/zuul/manager/independent.py @@ -80,7 +80,7 @@ class IndependentPipelineManager(PipelineManager): return True def getMissingNeededChanges(self, changes, change_queue, event, - dependency_graph=None): + dependency_graph=None, item=None): log = get_annotated_logger(self.log, event) if self.pipeline.ignore_dependencies: