zuul/zuul
Code Issues Proposed changes

Ignore circular dependencies in supercedent pipelines

Browse Source 
There are two issues with supercedent pipelines related to circular deps:

1) When operating in a post-merge configuration on changes (not refs), the
   pipeline manager would throw an exception starting with 10.0.0 because
   any time it operates on change objects, it attempts to collect the
   dependency cycle before enqueing a change, and starting with 10.0.0,
   the supercedent manager raises an exception in that case.
2) When operating in a pre-merge configuration on changes, the behavior
   regarding circular dependencies was undefined before 10.0.0.  It is
   likely that they were ignored because the manager creates a dynamic
   queue based on the project-ref, but it wasn't explicitly documented
   or tested.

To correct both of these:

Override the cycleForChange method in the supercedent manager so that it
always returns an empty cycle.

Document the expected behavior.

Add tests that cover the cases described above.

Change-Id: Icf30d488334d40a929f31c2f390e18ae599a3c42
This commit is contained in:
James E. Blair
2024-03-04 10:44:13 -08:00
parent d1a5291882
commit 79a9f86c8d
6 changed files with 298 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
doc/source/config/pipeline.rst
View File

@@ -151,12 +151,18 @@ success, the pipeline reports back to Gerrit with ``Verified`` vote of
these cases, build resources can be conserved by avoiding
building intermediate versions.
.. note:: Since this pipeline filters intermediate buildsets
using it in combination with file filters on jobs
is dangerous. In this case jobs of in between
buildsets can be unexpectedly skipped entirely. If
file filters are needed the ``independent`` or
``serial`` pipeline managers should be used.
.. note:: Since this pipeline manager filters intermediate
buildsets using it in combination with file filters
on jobs is dangerous. In this case jobs of in
between buildsets can be unexpectedly skipped
entirely. If file filters are needed the
``independent`` or ``serial`` pipeline managers
should be used.
.. note:: Since this pipeline manager creates a virtual queue
for each project-ref, it ignores manually
configured shared queues as well as circular
dependencies.
.. attr:: post-review
:default: false

8
releasenotes/notes/supercedent-circular-deps-6eab21a4ba2f6ff8.yaml Normal file
View File

@@ -0,0 +1,8 @@
---
fixes:
- |
In 10.0.0, supercedent pipelines could raise exceptions when
enqueing changes that were part of a dependency cycle (regardless
of whether the changes merged or not). This has been corrected,
and the documentation has been updated to clarify that circular
dependencies are ignored in supercedent pipelines.

40
tests/fixtures/layouts/supercedent-circular-gerrit.yaml vendored Normal file
View File

@@ -0,0 +1,40 @@
- pipeline:
name: post
manager: supercedent
trigger:
gerrit:
- event: patchset-created
- job:
name: base
parent: null
run: playbooks/base.yaml
- job:
name: post-job
- job:
name: post1-job
- job:
name: post2-job
- queue:
name: shared
allow-circular-dependencies: true
- project:
name: org/project1
queue: shared
post:
jobs:
- post-job
- post1-job
- project:
name: org/project2
queue: shared
post:
jobs:
- post-job
- post2-job

49
tests/fixtures/layouts/supercedent-circular-github.yaml vendored Normal file
View File

@@ -0,0 +1,49 @@
- pipeline:
name: post
manager: supercedent
require:
github:
merged: true
trigger:
github:
- event: pull_request
action:
- closed
success:
github: {}
failure:
github: {}
- job:
name: base
parent: null
run: playbooks/base.yaml
- job:
name: post-job
- job:
name: post1-job
- job:
name: post2-job
- queue:
name: shared
allow-circular-dependencies: true
- project:
name: org/project1
queue: shared
post:
jobs:
- post-job
- post1-job
- project:
name: org/project2
queue: shared
post:
jobs:
- post-job
- post2-job

184
tests/unit/test_supercedent.py
View File

@@ -114,3 +114,187 @@ class TestSupercedent(ZuulTestCase):
dict(name='promote-job', result='SUCCESS', changes='1,1'),
dict(name='promote-job', result='SUCCESS', changes='3,1'),
], ordered=False)
class TestSupercedentCircularDependencies(ZuulTestCase):
config_file = "zuul-gerrit-github.conf"
tenant_config_file = "config/circular-dependencies/main.yaml"
@simple_layout('layouts/supercedent-circular-gerrit.yaml')
def test_supercedent_gerrit_circular_deps(self):
# Unlike other supercedent tests, this one operates on
# pre-merge changes instead of post-merge refs so that we can
# better exercise the circular dependency machinery.
self.executor_server.hold_jobs_in_build = True
A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A')
B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
# A <-> B (via commit-depends)
A.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
A.subject, B.data["url"]
)
B.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
B.subject, A.data["url"]
)
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
# We should never run jobs for more than one change
# per-project at a time
self.assertEqual(len(self.builds), 4)
# This pair of changes should be superceded by the next
C = self.fake_gerrit.addFakeChange('org/project1', 'master', 'C')
D = self.fake_gerrit.addFakeChange('org/project2', 'master', 'D')
# C <-> D (via commit-depends)
C.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
C.subject, D.data["url"]
)
D.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
D.subject, C.data["url"]
)
self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.fake_gerrit.addEvent(D.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
# We should never run jobs for more than one change
# per-project at a time
self.assertEqual(len(self.builds), 4)
# This change should supercede C
E = self.fake_gerrit.addFakeChange('org/project1', 'master', 'E')
self.fake_gerrit.addEvent(E.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
# We should never run jobs for more than one change
# per-project at a time
self.assertEqual(len(self.builds), 4)
self.executor_server.hold_jobs_in_build = True
self.orderedRelease()
self.assertHistory([
dict(name='post-job', result='SUCCESS', changes="1,1"),
dict(name='post1-job', result='SUCCESS', changes="1,1"),
dict(name='post-job', result='SUCCESS', changes="2,1"),
dict(name='post2-job', result='SUCCESS', changes="2,1"),
dict(name='post-job', result='SUCCESS', changes="5,1"),
dict(name='post1-job', result='SUCCESS', changes="5,1"),
dict(name='post-job', result='SUCCESS', changes="4,1"),
dict(name='post2-job', result='SUCCESS', changes="4,1"),
], ordered=False)
@simple_layout('layouts/supercedent-circular-github.yaml', driver='github')
def test_supercedent_github_circular_deps_merged(self):
# We leave testing pre-merge changes to the gerrit test above.
# In this test, we're testing post-merge change objects (not
# refs) via github since there is a reasonable post-merge
# pipeline configuration that triggers on those.
self.executor_server.hold_jobs_in_build = True
A = self.fake_github.openFakePullRequest("org/project1", "master", "A")
B = self.fake_github.openFakePullRequest("org/project2", "master", "B")
# A <-> B (via PR-depends)
A.body = "{}\n\nDepends-On: {}\n".format(
A.subject, B.url,
)
B.body = "{}\n\nDepends-On: {}\n".format(
B.subject, A.url
)
A.setMerged('merged')
self.fake_github.emitEvent(A.getPullRequestClosedEvent())
self.waitUntilSettled()
B.setMerged('merged')
self.fake_github.emitEvent(B.getPullRequestClosedEvent())
self.waitUntilSettled()
# We should never run jobs for more than one change
# per-project at a time
self.assertEqual(len(self.builds), 4)
# This pair of changes should be superceded by the next
C = self.fake_github.openFakePullRequest("org/project1", "master", "C")
D = self.fake_github.openFakePullRequest("org/project2", "master", "D")
# C <-> D (via PR-depends)
C.body = "{}\n\nDepends-On: {}\n".format(
C.subject, D.url,
)
D.body = "{}\n\nDepends-On: {}\n".format(
D.subject, C.url
)
C.setMerged('merged')
self.fake_github.emitEvent(C.getPullRequestClosedEvent())
self.waitUntilSettled()
D.setMerged('merged')
self.fake_github.emitEvent(D.getPullRequestClosedEvent())
self.waitUntilSettled()
# We should never run jobs for more than one change
# per-project at a time
self.assertEqual(len(self.builds), 4)
# This change should supercede C
E = self.fake_github.openFakePullRequest("org/project1", "master", "E")
E.setMerged('merged')
self.fake_github.emitEvent(E.getPullRequestClosedEvent())
self.waitUntilSettled()
# We should never run jobs for more than one change
# per-project at a time
self.assertEqual(len(self.builds), 4)
self.executor_server.hold_jobs_in_build = True
self.orderedRelease()
self.assertHistory([
dict(name='post-job', result='SUCCESS',
changes=f"{A.number},{A.head_sha}"),
dict(name='post1-job', result='SUCCESS',
changes=f"{A.number},{A.head_sha}"),
dict(name='post-job', result='SUCCESS',
changes=f"{B.number},{B.head_sha}"),
dict(name='post2-job', result='SUCCESS',
changes=f"{B.number},{B.head_sha}"),
dict(name='post-job', result='SUCCESS',
changes=f"{E.number},{E.head_sha}"),
dict(name='post1-job', result='SUCCESS',
changes=f"{E.number},{E.head_sha}"),
dict(name='post-job', result='SUCCESS',
changes=f"{D.number},{D.head_sha}"),
dict(name='post2-job', result='SUCCESS',
changes=f"{D.number},{D.head_sha}"),
], ordered=False)
@simple_layout('layouts/supercedent-circular-github.yaml', driver='github')
def test_supercedent_github_circular_deps_closed(self):
# We leave testing pre-merge changes to the gerrit test above.
# In this test, we're testing post-merge change objects (not
# refs) via github since there is a reasonable post-merge
# pipeline configuration that triggers on those. This test
# exercises the code path where the change is closed but not
# merged (we should run no jobs, but we should also not fail
# pipeline processing while dealing with circular deps).
self.executor_server.hold_jobs_in_build = True
A = self.fake_github.openFakePullRequest("org/project1", "master", "A")
B = self.fake_github.openFakePullRequest("org/project2", "master", "B")
# A <-> B (via PR-depends)
A.body = "{}\n\nDepends-On: {}\n".format(
A.subject, B.url,
)
B.body = "{}\n\nDepends-On: {}\n".format(
B.subject, A.url
)
with self.assertNoLogs('zuul.Scheduler-0', level='ERROR'):
self.fake_github.emitEvent(A.getPullRequestClosedEvent())
self.waitUntilSettled()
self.fake_github.emitEvent(B.getPullRequestClosedEvent())
self.waitUntilSettled()

10
zuul/manager/supercedent.py
View File

@@ -69,11 +69,11 @@ class SupercedentPipelineManager(PipelineManager):
self.removeItem(item)
def cycleForChange(self, *args, **kw):
ret = super().cycleForChange(*args, **kw)
if len(ret) > 1:
raise Exception("Dependency cycles not supported "
"in supercedent pipelines")
return ret
# Supercedent pipelines ignore circular dependencies and
# individually enqueue each change that matches the trigger.
# This is because they ignore shared queues and instead create
# a virtual queue for each project-ref.
return []
def addChange(self, *args, **kw):
ret = super(SupercedentPipelineManager, self).addChange(