diff --git a/doc/source/config/pipeline.rst b/doc/source/config/pipeline.rst index cb7ea3c088..d877014dea 100644 --- a/doc/source/config/pipeline.rst +++ b/doc/source/config/pipeline.rst @@ -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 diff --git a/releasenotes/notes/supercedent-circular-deps-6eab21a4ba2f6ff8.yaml b/releasenotes/notes/supercedent-circular-deps-6eab21a4ba2f6ff8.yaml new file mode 100644 index 0000000000..7519a32321 --- /dev/null +++ b/releasenotes/notes/supercedent-circular-deps-6eab21a4ba2f6ff8.yaml @@ -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. diff --git a/tests/fixtures/layouts/supercedent-circular-gerrit.yaml b/tests/fixtures/layouts/supercedent-circular-gerrit.yaml new file mode 100644 index 0000000000..29da29d2e6 --- /dev/null +++ b/tests/fixtures/layouts/supercedent-circular-gerrit.yaml @@ -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 diff --git a/tests/fixtures/layouts/supercedent-circular-github.yaml b/tests/fixtures/layouts/supercedent-circular-github.yaml new file mode 100644 index 0000000000..95116ffc64 --- /dev/null +++ b/tests/fixtures/layouts/supercedent-circular-github.yaml @@ -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 diff --git a/tests/unit/test_supercedent.py b/tests/unit/test_supercedent.py index 7505c0779d..931c494ce0 100644 --- a/tests/unit/test_supercedent.py +++ b/tests/unit/test_supercedent.py @@ -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() diff --git a/zuul/manager/supercedent.py b/zuul/manager/supercedent.py index c6409f0628..327f8daa0d 100644 --- a/zuul/manager/supercedent.py +++ b/zuul/manager/supercedent.py @@ -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(