From 79a9f86c8dc4ef97aa736f44e7d067b714b03275 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Mon, 4 Mar 2024 10:44:13 -0800 Subject: [PATCH] Ignore circular dependencies in supercedent pipelines 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 --- doc/source/config/pipeline.rst | 18 +- ...cedent-circular-deps-6eab21a4ba2f6ff8.yaml | 8 + .../layouts/supercedent-circular-gerrit.yaml | 40 ++++ .../layouts/supercedent-circular-github.yaml | 49 +++++ tests/unit/test_supercedent.py | 184 ++++++++++++++++++ zuul/manager/supercedent.py | 10 +- 6 files changed, 298 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/supercedent-circular-deps-6eab21a4ba2f6ff8.yaml create mode 100644 tests/fixtures/layouts/supercedent-circular-gerrit.yaml create mode 100644 tests/fixtures/layouts/supercedent-circular-github.yaml 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(