zuul
/
zuul
Code Issues Proposed changes

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
This commit is contained in:
James E. Blair 2022-03-05 12:31:43 -08:00
parent 81d84e7c15
commit 249ccc403b
8 changed files with 152 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

70
tests/fixtures/layouts/submitted-together-per-branch.yaml vendored Normal file
View File

@ -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

31
tests/unit/test_circular_dependencies.py
View File

@ -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"

4
tests/unit/test_cross_crd.py
View File

@ -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)

8
tests/unit/test_gerrit_crd.py
View File

@ -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)

8
tests/unit/test_gerrit_legacy_crd.py
View File

@ -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)

47
zuul/manager/__init__.py
View File

@ -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",

23
zuul/manager/dependent.py
View File

@ -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")

3
zuul/manager/independent.py
View File

@ -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 []