From 249ccc403b96842ea0c2e8f187c7fce6ef9be786 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Sat, 5 Mar 2022 12:31:43 -0800 Subject: [PATCH] Report per-branch cyclic-dependency conflicts If a cycle of dependencies is attempted to be enqueued into a pipeline and at least one of the participating projects has a per-branch change queue and the changes in the cycle are in different branches, it can be confusing for users why the changes were not enqueued. This is even more likely to happen with implicit cyclic dependencies such as those from Gerrit's submitted-together feature (but can happen with any driver). To aid users in this situation, report this situation back to the code review system. Change-Id: I26174849deab627b2cf91d75029c5a2674cc37d6 --- .../submitted-together-per-branch.yaml | 70 +++++++++++++++++++ tests/unit/test_circular_dependencies.py | 31 ++++++-- tests/unit/test_cross_crd.py | 4 +- tests/unit/test_gerrit_crd.py | 8 +-- tests/unit/test_gerrit_legacy_crd.py | 8 +-- zuul/manager/__init__.py | 47 ++++++++----- zuul/manager/dependent.py | 23 +++--- zuul/manager/independent.py | 3 +- 8 files changed, 152 insertions(+), 42 deletions(-) create mode 100644 tests/fixtures/layouts/submitted-together-per-branch.yaml diff --git a/tests/fixtures/layouts/submitted-together-per-branch.yaml b/tests/fixtures/layouts/submitted-together-per-branch.yaml new file mode 100644 index 0000000000..73898ac994 --- /dev/null +++ b/tests/fixtures/layouts/submitted-together-per-branch.yaml @@ -0,0 +1,70 @@ +- queue: + name: integrated + allow-circular-dependencies: true + per-branch: true + +- pipeline: + name: check + manager: independent + trigger: + gerrit: + - event: patchset-created + success: + gerrit: + Verified: 1 + failure: + gerrit: + Verified: -1 + +- pipeline: + name: gate + manager: dependent + success-message: Build succeeded (gate). + require: + gerrit: + approval: + - Approved: 1 + trigger: + gerrit: + - event: comment-added + approval: + - Approved: 1 + success: + gerrit: + Verified: 2 + submit: true + failure: + gerrit: + Verified: -2 + start: + gerrit: + Verified: 0 + precedence: high + +- job: + name: base + parent: null + run: playbooks/run.yaml + +- job: + name: test-job + +- project: + name: org/project1 + queue: integrated + check: + jobs: + - test-job + gate: + jobs: + - test-job + +- project: + name: org/project2 + queue: integrated + check: + jobs: + - test-job + gate: + jobs: + - test-job diff --git a/tests/unit/test_circular_dependencies.py b/tests/unit/test_circular_dependencies.py index 713408ee98..f26aaeb96f 100644 --- a/tests/unit/test_circular_dependencies.py +++ b/tests/unit/test_circular_dependencies.py @@ -17,7 +17,7 @@ import textwrap from zuul.model import PromoteEvent -from tests.base import ZuulTestCase +from tests.base import ZuulTestCase, simple_layout class TestGerritCircularDependencies(ZuulTestCase): @@ -180,7 +180,7 @@ class TestGerritCircularDependencies(ZuulTestCase): self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) self.waitUntilSettled() - self.assertEqual(A.reported, 1) + self.assertEqual(A.reported, 2) self.assertEqual(B.reported, 1) self.assertEqual(A.data["status"], "NEW") self.assertEqual(B.data["status"], "NEW") @@ -1266,7 +1266,7 @@ class TestGerritCircularDependencies(ZuulTestCase): self.assertEqual(A.reported, 3) self.assertEqual(B.reported, 3) - self.assertEqual(C.reported, 3) + self.assertEqual(C.reported, 6) self.assertEqual(A.data["status"], "MERGED") self.assertEqual(B.data["status"], "MERGED") self.assertEqual(C.data["status"], "MERGED") @@ -1300,7 +1300,7 @@ class TestGerritCircularDependencies(ZuulTestCase): self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) self.waitUntilSettled() - self.assertEqual(A.reported, 1) + self.assertEqual(A.reported, 2) self.assertEqual(A.data["status"], "NEW") self.assertEqual(B.data["status"], "NEW") @@ -1311,7 +1311,7 @@ class TestGerritCircularDependencies(ZuulTestCase): self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) self.waitUntilSettled() - self.assertEqual(A.reported, 3) + self.assertEqual(A.reported, 4) self.assertEqual(A.data["status"], "MERGED") def test_promote_cycle(self): @@ -1449,6 +1449,27 @@ class TestGerritCircularDependencies(ZuulTestCase): self.assertEqual(A.data["status"], "MERGED") self.assertEqual(B.data["status"], "MERGED") + @simple_layout('layouts/submitted-together-per-branch.yaml') + def test_submitted_together_per_branch(self): + self.fake_gerrit._fake_submit_whole_topic = True + self.create_branch('org/project2', 'stable/foo') + A = self.fake_gerrit.addFakeChange('org/project1', "master", "A", + topic='test-topic') + B = self.fake_gerrit.addFakeChange('org/project2', "stable/foo", "B", + topic='test-topic') + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + A.addApproval("Approved", 1) + self.fake_gerrit.addEvent(B.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 0) + self.assertEqual(B.reported, 1) + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "NEW") + self.assertIn("does not share a change queue", B.messages[-1]) + class TestGithubCircularDependencies(ZuulTestCase): config_file = "zuul-gerrit-github.conf" diff --git a/tests/unit/test_cross_crd.py b/tests/unit/test_cross_crd.py index 442277f51d..42d86275ba 100644 --- a/tests/unit/test_cross_crd.py +++ b/tests/unit/test_cross_crd.py @@ -669,7 +669,7 @@ class TestGithubToGerritCRD(ZuulTestCase): # should not be processed in dependent pipeline self.assertFalse(A.is_merged) self.assertEqual(B.data['status'], 'NEW') - self.assertEqual(len(A.comments), 0) + self.assertEqual(len(A.comments), 1) self.assertEqual(B.reported, 0) self.assertEqual(len(self.history), 0) @@ -688,7 +688,7 @@ class TestGithubToGerritCRD(ZuulTestCase): self.waitUntilSettled() self.assertTrue(A.is_merged) - self.assertEqual(len(A.comments), 2) + self.assertEqual(len(A.comments), 3) self.assertEqual(B.data['status'], 'MERGED') self.assertEqual(B.reported, 0) diff --git a/tests/unit/test_gerrit_crd.py b/tests/unit/test_gerrit_crd.py index d925666845..0b0ca86feb 100644 --- a/tests/unit/test_gerrit_crd.py +++ b/tests/unit/test_gerrit_crd.py @@ -253,7 +253,7 @@ class TestGerritCRD(ZuulTestCase): self.assertEqual(A.data['status'], 'NEW') self.assertEqual(B.data['status'], 'NEW') - self.assertEqual(A.reported, 0) + self.assertEqual(A.reported, 1) self.assertEqual(B.reported, 0) self.assertEqual(len(self.history), 0) @@ -270,7 +270,7 @@ class TestGerritCRD(ZuulTestCase): self.waitUntilSettled() self.assertEqual(A.data['status'], 'MERGED') - self.assertEqual(A.reported, 2) + self.assertEqual(A.reported, 3) def _test_crd_gate_reverse(self, url_fmt): "Test reverse cross-repo dependencies" @@ -369,7 +369,7 @@ class TestGerritCRD(ZuulTestCase): # should not be processed in dependent pipeline self.assertEqual(A.data['status'], 'NEW') self.assertEqual(B.data['status'], 'NEW') - self.assertEqual(A.reported, 0) + self.assertEqual(A.reported, 1) self.assertEqual(B.reported, 0) self.assertEqual(len(self.history), 0) @@ -388,7 +388,7 @@ class TestGerritCRD(ZuulTestCase): self.waitUntilSettled() self.assertEqual(A.data['status'], 'MERGED') - self.assertEqual(A.reported, 2) + self.assertEqual(A.reported, 3) self.assertEqual(B.data['status'], 'MERGED') self.assertEqual(B.reported, 0) diff --git a/tests/unit/test_gerrit_legacy_crd.py b/tests/unit/test_gerrit_legacy_crd.py index 5c3e868f93..60616b6cff 100644 --- a/tests/unit/test_gerrit_legacy_crd.py +++ b/tests/unit/test_gerrit_legacy_crd.py @@ -194,7 +194,7 @@ class TestGerritLegacyCRD(ZuulTestCase): self.assertEqual(A.data['status'], 'NEW') self.assertEqual(B.data['status'], 'NEW') - self.assertEqual(A.reported, 0) + self.assertEqual(A.reported, 1) self.assertEqual(B.reported, 0) self.assertEqual(len(self.history), 0) @@ -211,7 +211,7 @@ class TestGerritLegacyCRD(ZuulTestCase): self.waitUntilSettled() self.assertEqual(A.data['status'], 'MERGED') - self.assertEqual(A.reported, 2) + self.assertEqual(A.reported, 3) def test_crd_gate_reverse(self): "Test reverse cross-repo dependencies" @@ -298,7 +298,7 @@ class TestGerritLegacyCRD(ZuulTestCase): # should not be processed in dependent pipeline self.assertEqual(A.data['status'], 'NEW') self.assertEqual(B.data['status'], 'NEW') - self.assertEqual(A.reported, 0) + self.assertEqual(A.reported, 1) self.assertEqual(B.reported, 0) self.assertEqual(len(self.history), 0) @@ -317,7 +317,7 @@ class TestGerritLegacyCRD(ZuulTestCase): self.waitUntilSettled() self.assertEqual(A.data['status'], 'MERGED') - self.assertEqual(A.reported, 2) + self.assertEqual(A.reported, 3) self.assertEqual(B.data['status'], 'MERGED') self.assertEqual(B.reported, 0) diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index 0c8f2ddc37..63614b4c45 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -353,7 +353,8 @@ class PipelineManager(metaclass=ABCMeta): return True def enqueueChangesAhead(self, change, event, quiet, ignore_requirements, - change_queue, history=None, dependency_graph=None): + change_queue, history=None, dependency_graph=None, + warnings=None): return True def enqueueChangesBehind(self, change, event, quiet, ignore_requirements, @@ -525,13 +526,18 @@ class PipelineManager(metaclass=ABCMeta): (change, change.project)) return False + warnings = [] if not self.enqueueChangesAhead(change, event, quiet, ignore_requirements, change_queue, history=history, - dependency_graph=dependency_graph): + dependency_graph=dependency_graph, + warnings=warnings): self.dequeueIncompleteCycle(change, dependency_graph, event, change_queue) log.debug("Failed to enqueue changes ahead of %s" % change) + if warnings: + self._reportNonEqueuedItem(change_queue, change, + event, warnings) return False log.debug("History after enqueuing changes ahead: %s", history) @@ -546,22 +552,9 @@ class PipelineManager(metaclass=ABCMeta): if cycle and not self.canProcessCycle(change.project): log.info("Dequeing change %s since at least one project " "does not allow circular dependencies", change) - actions = self.pipeline.failure_actions - ci = change_queue.enqueueChange(cycle[-1], event) - ci.warning("Dependency cycle detected") - ci.setReportedResult('FAILURE') - - # Only report the cycle if the project is in the current - # pipeline. Otherwise the change could be spammed by - # reports from unrelated pipelines. - if self.pipeline.tenant.layout.getProjectPipelineConfig( - ci - ): - self.sendReport(actions, ci) - self.dequeueItem(ci) - self.sql.reportBuildsetEnd(ci.current_build_set, - 'failure', final=True) - + warnings = ["Dependency cycle detected"] + self._reportNonEqueuedItem(change_queue, + cycle[-1], event, warnings) return False log.info("Adding change %s to queue %s in %s" % @@ -600,6 +593,24 @@ class PipelineManager(metaclass=ABCMeta): self.dequeueSupercededItems(item) return True + def _reportNonEqueuedItem(self, change_queue, change, event, warnings): + # Enqueue an item which otherwise can not be enqueued in order + # to report a message to the user. + actions = self.pipeline.failure_actions + ci = change_queue.enqueueChange(change, event) + for w in warnings: + ci.warning(w) + ci.setReportedResult('FAILURE') + + # Only report the item if the project is in the current + # pipeline. Otherwise the change could be spammed by + # reports from unrelated pipelines. + if self.pipeline.tenant.layout.getProjectPipelineConfig(ci): + self.sendReport(actions, ci) + self.dequeueItem(ci) + self.sql.reportBuildsetEnd(ci.current_build_set, + 'failure', final=True) + def cycleForChange(self, change, dependency_graph, event): log = get_annotated_logger(self.log, event) log.debug("Running Tarjan's algorithm on current dependencies: %s", diff --git a/zuul/manager/dependent.py b/zuul/manager/dependent.py index 30b7c97bcb..d4d4f05dd0 100644 --- a/zuul/manager/dependent.py +++ b/zuul/manager/dependent.py @@ -138,7 +138,8 @@ class DependentPipelineManager(SharedQueuePipelineManager): dependency_graph=dependency_graph) def enqueueChangesAhead(self, change, event, quiet, ignore_requirements, - change_queue, history=None, dependency_graph=None): + change_queue, history=None, dependency_graph=None, + warnings=None): log = get_annotated_logger(self.log, event) history = history if history is not None else [] @@ -149,7 +150,8 @@ class DependentPipelineManager(SharedQueuePipelineManager): return True ret = self.checkForChangesNeededBy(change, change_queue, event, - dependency_graph=dependency_graph) + dependency_graph=dependency_graph, + warnings=warnings) if ret in [True, False]: return ret log.debug(" Changes %s must be merged ahead of %s", ret, change) @@ -168,7 +170,7 @@ class DependentPipelineManager(SharedQueuePipelineManager): return True def checkForChangesNeededBy(self, change, change_queue, event, - dependency_graph=None): + dependency_graph=None, warnings=None): log = get_annotated_logger(self.log, event) # Return true if okay to proceed enqueing this change, @@ -200,11 +202,16 @@ class DependentPipelineManager(SharedQueuePipelineManager): with self.getChangeQueue(needed_change, event) as needed_change_queue: if needed_change_queue != change_queue: - log.debug(" Change %s in project %s does not " - "share a change queue with %s " - "in project %s", - needed_change, needed_change.project, - change, change.project) + msg = ("Change %s in project %s does not " + "share a change queue with %s " + "in project %s" % + (needed_change.number, + needed_change.project, + change.number, + change.project)) + log.debug(" " + msg) + if warnings is not None: + warnings.append(msg) return False if not needed_change.is_current_patchset: log.debug(" Needed change is not the current patchset") diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py index 4405c44817..7cd14ffd56 100644 --- a/zuul/manager/independent.py +++ b/zuul/manager/independent.py @@ -38,7 +38,8 @@ class IndependentPipelineManager(PipelineManager): return DynamicChangeQueueContextManager(change_queue) def enqueueChangesAhead(self, change, event, quiet, ignore_requirements, - change_queue, history=None, dependency_graph=None): + change_queue, history=None, dependency_graph=None, + warnings=None): log = get_annotated_logger(self.log, event) history = history if history is not None else []