diff --git a/doc/source/reference/queue_def.rst b/doc/source/reference/queue_def.rst index 78441de25e..758a0fa18d 100644 --- a/doc/source/reference/queue_def.rst +++ b/doc/source/reference/queue_def.rst @@ -42,3 +42,26 @@ Here is an example ``queue`` configuration. means that all projects that should be gated must have aligned branch names when using per branch queues. Otherwise changes that belong together end up in different queues. + + .. attr:: allow-circular-dependencies + :default: false + + Define if Zuul is allowed to process circular dependencies between + changes for this queue. All projects that are part of a dependency cycle + must share the same change queue. + + In case Zuul detects a dependency cycle it will make sure that every + change also includes all other changes that are part of the cycle. + However each change will still be a normal item in the queue with its own + jobs. + + Reporting of success will be postponed until all items in the cycle + succeeded. In case of a failure in any of those items the whole cycle + will be dequeued. + + An error message will be posted to all items of the cycle in case some + items fail to report (e.g. merge failure when some items were already + merged). In this case the target branch(es) might be in a broken state. + + In general, circular dependencies are considered to be an antipattern but + can't be avoided in certain cases. diff --git a/tests/fixtures/config/circular-dependencies/git/common-config/playbooks/run.yaml b/tests/fixtures/config/circular-dependencies/git/common-config/playbooks/run.yaml new file mode 100644 index 0000000000..ed97d539c0 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/common-config/playbooks/run.yaml @@ -0,0 +1 @@ +--- diff --git a/tests/fixtures/config/circular-dependencies/git/common-config/zuul.yaml b/tests/fixtures/config/circular-dependencies/git/common-config/zuul.yaml new file mode 100644 index 0000000000..e8b166ae01 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/common-config/zuul.yaml @@ -0,0 +1,187 @@ +- queue: + name: integrated + allow-circular-dependencies: true + +- pipeline: + name: check + manager: independent + trigger: + gerrit: + - event: patchset-created + github: + - event: pull_request + action: + - opened + - changed + - reopened + - edited + success: + gerrit: + Verified: 1 + github: + status: success + failure: + gerrit: + Verified: -1 + github: + status: failure + +- pipeline: + name: check-unused + manager: independent + trigger: + gerrit: + - event: patchset-created + success: + gerrit: + Verified: 1 + github: + status: success + failure: + gerrit: + Verified: -1 + github: + status: failure + +- pipeline: + name: gate + manager: dependent + success-message: Build succeeded (gate). + require: + gerrit: + approval: + - Approved: 1 + github: + label: approved + trigger: + gerrit: + - event: comment-added + approval: + - Approved: 1 + github: + - event: pull_request + action: edited + - event: pull_request + action: labeled + label: approved + success: + gerrit: + Verified: 2 + submit: true + github: + merge: true + failure: + gerrit: + Verified: -2 + github: {} + start: + gerrit: + Verified: 0 + github: {} + precedence: high + +- pipeline: + name: gate-unused + manager: dependent + success-message: Build succeeded (gate). + trigger: + gerrit: + - event: comment-added + approval: + - Approved: 1 + success: + gerrit: + Verified: 2 + submit: true + github: + merge: true + failure: + gerrit: + Verified: -2 + github: {} + start: + gerrit: + Verified: 0 + github: {} + precedence: high + +- job: + name: base + parent: null + run: playbooks/run.yaml + required-projects: + - common-config + - org/project + - org/project1 + - org/project2 + +- job: + name: common-config-job + +- job: + name: project-job + +- job: + name: project1-job + +- job: + name: project2-job + +- job: + name: project3-job + +- project: + name: common-config + queue: integrated + check: + jobs: + - common-config-job + gate: + jobs: + - common-config-job + +- project: + name: ^.*/project + queue: integrated + check: + jobs: + - project-job + gate: + jobs: + - project-job + +- project: + name: ^.*/project1 + queue: integrated + check: + jobs: + - project1-job + - project-vars-job + gate: + jobs: + - project1-job + - project-vars-job + +- project-template: + name: project2-template + queue: integrated + check: + jobs: + - project2-job + gate: + jobs: + - project2-job + +- project: + name: ^.*/project2 + templates: + - project2-template + +- project: + name: ^.*/project3 + check: + jobs: + - project3-job + gate: + jobs: + - project3-job diff --git a/tests/fixtures/config/circular-dependencies/git/gh_project/README b/tests/fixtures/config/circular-dependencies/git/gh_project/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/gh_project/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/circular-dependencies/git/gh_project1/README b/tests/fixtures/config/circular-dependencies/git/gh_project1/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/gh_project1/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/circular-dependencies/git/gh_project2/README b/tests/fixtures/config/circular-dependencies/git/gh_project2/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/gh_project2/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/circular-dependencies/git/gh_project3/README b/tests/fixtures/config/circular-dependencies/git/gh_project3/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/gh_project3/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/circular-dependencies/git/org_project/README b/tests/fixtures/config/circular-dependencies/git/org_project/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/org_project/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/circular-dependencies/git/org_project1/README b/tests/fixtures/config/circular-dependencies/git/org_project1/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/org_project1/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/circular-dependencies/git/org_project2/README b/tests/fixtures/config/circular-dependencies/git/org_project2/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/org_project2/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/circular-dependencies/git/org_project2/zuul.yaml b/tests/fixtures/config/circular-dependencies/git/org_project2/zuul.yaml new file mode 100644 index 0000000000..22d126c29d --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/org_project2/zuul.yaml @@ -0,0 +1,4 @@ +- job: + name: project-vars-job + vars: + test_var: fail diff --git a/tests/fixtures/config/circular-dependencies/git/org_project3/README b/tests/fixtures/config/circular-dependencies/git/org_project3/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/org_project3/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/circular-dependencies/git/org_project4/README b/tests/fixtures/config/circular-dependencies/git/org_project4/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/git/org_project4/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/circular-dependencies/main.yaml b/tests/fixtures/config/circular-dependencies/main.yaml new file mode 100644 index 0000000000..8083a582b3 --- /dev/null +++ b/tests/fixtures/config/circular-dependencies/main.yaml @@ -0,0 +1,25 @@ +- tenant: + name: tenant-one + exclude-unprotected-branches: true + source: + gerrit: + config-projects: + - common-config + untrusted-projects: + - org/project + - org/project1 + - org/project2 + - org/project3 + github: + untrusted-projects: + - gh/project + - gh/project1 + - gh/project2 + - gh/project3 + +- tenant: + name: tenant-two + source: + gerrit: + untrusted-projects: + - org/project4 diff --git a/tests/unit/test_circular_dependencies.py b/tests/unit/test_circular_dependencies.py new file mode 100644 index 0000000000..a49d8895b2 --- /dev/null +++ b/tests/unit/test_circular_dependencies.py @@ -0,0 +1,1416 @@ +# Copyright 2019 BMW Group +# +# This module is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This software is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this software. If not, see . + +import textwrap + +import zuul.rpcclient + +from tests.base import ZuulTestCase + + +class TestGerritCircularDependencies(ZuulTestCase): + config_file = "zuul-gerrit-github.conf" + tenant_config_file = "config/circular-dependencies/main.yaml" + + def _test_simple_cycle(self, project1, project2): + A = self.fake_gerrit.addFakeChange(project1, "master", "A") + B = self.fake_gerrit.addFakeChange(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.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(B.patchsets[-1]["approvals"]), 1) + self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "1") + + # We're about to add approvals to changes without adding the + # triggering events to Zuul, so that we can be sure that it is + # enqueing the changes based on dependencies, not because of + # triggering events. Since it will have the changes cached + # already (without approvals), we need to clear the cache + # first. + for connection in self.scheds.first.connections.connections.values(): + connection.maintainCache([]) + + 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, 3) + self.assertEqual(B.reported, 3) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + + def _test_transitive_cycle(self, project1, project2, project3): + A = self.fake_gerrit.addFakeChange(project1, "master", "A") + B = self.fake_gerrit.addFakeChange(project2, "master", "B") + C = self.fake_gerrit.addFakeChange(project3, "master", "C") + + # A -> B -> C -> A (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, C.data["url"] + ) + C.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + C.subject, A.data["url"] + ) + + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(B.patchsets[-1]["approvals"]), 1) + self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(C.patchsets[-1]["approvals"]), 1) + self.assertEqual(C.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(C.patchsets[-1]["approvals"][0]["value"], "1") + + # We're about to add approvals to changes without adding the + # triggering events to Zuul, so that we can be sure that it is + # enqueing the changes based on dependencies, not because of + # triggering events. Since it will have the changes cached + # already (without approvals), we need to clear the cache + # first. + for connection in self.scheds.first.connections.connections.values(): + connection.maintainCache([]) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + C.addApproval("Code-Review", 2) + C.addApproval("Approved", 1) + A.addApproval("Approved", 1) + self.fake_gerrit.addEvent(B.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 3) + self.assertEqual(B.reported, 3) + self.assertEqual(C.reported, 3) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + self.assertEqual(C.data["status"], "MERGED") + + def test_single_project_cycle(self): + self._test_simple_cycle("org/project", "org/project") + + def test_crd_cycle(self): + self._test_simple_cycle("org/project1", "org/project2") + + def test_single_project_transitive_cycle(self): + self._test_transitive_cycle( + "org/project1", "org/project1", "org/project1" + ) + + def test_crd_transitive_cycle(self): + self._test_transitive_cycle( + "org/project", "org/project1", "org/project2" + ) + + def test_forbidden_cycle(self): + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project3", "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.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "-1") + + self.assertEqual(len(B.patchsets[-1]["approvals"]), 1) + self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "-1") + + # We're about to add approvals to changes without adding the + # triggering events to Zuul, so that we can be sure that it is + # enqueing the changes based on dependencies, not because of + # triggering events. Since it will have the changes cached + # already (without approvals), we need to clear the cache + # first. + for connection in self.scheds.first.connections.connections.values(): + connection.maintainCache([]) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 1) + self.assertEqual(B.reported, 1) + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "NEW") + + def test_git_dependency_with_cycle(self): + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project", "master", "B") + C = self.fake_gerrit.addFakeChange("org/project1", "master", "C") + + # A -> B (git) -> C -> A + A.setDependsOn(B, 1) + B.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + B.subject, C.data["url"] + ) + C.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + C.subject, A.data["url"] + ) + + self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(B.patchsets[-1]["approvals"]), 1) + self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(C.patchsets[-1]["approvals"]), 1) + self.assertEqual(C.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(C.patchsets[-1]["approvals"][0]["value"], "1") + + # We're about to add approvals to changes without adding the + # triggering events to Zuul, so that we can be sure that it is + # enqueing the changes based on dependencies, not because of + # triggering events. Since it will have the changes cached + # already (without approvals), we need to clear the cache + # first. + for connection in self.scheds.first.connections.connections.values(): + connection.maintainCache([]) + + self.executor_server.hold_jobs_in_build = True + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + C.addApproval("Code-Review", 2) + A.addApproval("Approved", 1) + B.addApproval("Approved", 1) + self.fake_gerrit.addEvent(C.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(A.reported, 3) + self.assertEqual(B.reported, 3) + self.assertEqual(C.reported, 3) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + self.assertEqual(C.data["status"], "MERGED") + + def test_dependency_on_cycle(self): + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B") + C = self.fake_gerrit.addFakeChange("org/project2", "master", "C") + + # A -> B -> C -> 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, C.data["url"] + ) + C.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + C.subject, B.data["url"] + ) + + self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(B.patchsets[-1]["approvals"]), 1) + self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(C.patchsets[-1]["approvals"]), 1) + self.assertEqual(C.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(C.patchsets[-1]["approvals"][0]["value"], "1") + + # We're about to add approvals to changes without adding the + # triggering events to Zuul, so that we can be sure that it is + # enqueing the changes based on dependencies, not because of + # triggering events. Since it will have the changes cached + # already (without approvals), we need to clear the cache + # first. + for connection in self.scheds.first.connections.connections.values(): + connection.maintainCache([]) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + C.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + C.addApproval("Approved", 1) + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 3) + self.assertEqual(B.reported, 3) + self.assertEqual(C.reported, 3) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + self.assertEqual(C.data["status"], "MERGED") + + def test_dependent_change_on_cycle(self): + self.executor_server.hold_jobs_in_build = True + + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project", "master", "B") + C = self.fake_gerrit.addFakeChange("org/project2", "master", "C") + + A.setDependsOn(B, 1) + B.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + B.subject, C.data["url"] + ) + C.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + C.subject, B.data["url"] + ) + + A.addApproval("Code-Review", 2) + A.addApproval("Approved", 1) + B.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + C.addApproval("Code-Review", 2) + self.fake_gerrit.addEvent(C.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(len(self.builds), 2) + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(A.reported, 0) + self.assertEqual(B.reported, 2) + self.assertEqual(C.reported, 2) + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "MERGED") + self.assertEqual(C.data["status"], "MERGED") + + def test_cycle_dependency_on_cycle(self): + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B") + C = self.fake_gerrit.addFakeChange("org/project1", "master", "C") + D = self.fake_gerrit.addFakeChange("org/project2", "master", "D") + + # A -> B -> A + C + # C -> D -> C + A.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + A.subject, B.data["url"] + ) + B.data[ + "commitMessage" + ] = "{}\n\nDepends-On: {}\nDepends-On: {}\n".format( + B.subject, A.data["url"], C.data["url"] + ) + 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(D.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(B.patchsets[-1]["approvals"]), 1) + self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(C.patchsets[-1]["approvals"]), 1) + self.assertEqual(C.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(C.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(D.patchsets[-1]["approvals"]), 1) + self.assertEqual(D.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(D.patchsets[-1]["approvals"][0]["value"], "1") + + # We're about to add approvals to changes without adding the + # triggering events to Zuul, so that we can be sure that it is + # enqueing the changes based on dependencies, not because of + # triggering events. Since it will have the changes cached + # already (without approvals), we need to clear the cache + # first. + for connection in self.scheds.first.connections.connections.values(): + connection.maintainCache([]) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + C.addApproval("Code-Review", 2) + D.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + C.addApproval("Approved", 1) + D.addApproval("Approved", 1) + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 3) + self.assertEqual(B.reported, 3) + self.assertEqual(C.reported, 3) + self.assertEqual(D.reported, 3) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + self.assertEqual(C.data["status"], "MERGED") + self.assertEqual(D.data["status"], "MERGED") + + def test_cycle_failure(self): + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project1", "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.executor_server.failJob("project-job", A) + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "-1") + + # We're about to add approvals to changes without adding the + # triggering events to Zuul, so that we can be sure that it is + # enqueing the changes based on dependencies, not because of + # triggering events. Since it will have the changes cached + # already (without approvals), we need to clear the cache + # first. + for connection in self.scheds.first.connections.connections.values(): + connection.maintainCache([]) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + + self.executor_server.failJob("project-job", A) + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 3) + self.assertEqual(B.reported, 3) + self.assertIn("bundle", A.messages[-1]) + self.assertIn("bundle", B.messages[-1]) + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "NEW") + + def test_dependency_on_cycle_failure(self): + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B") + C = self.fake_gerrit.addFakeChange("org/project2", "master", "C") + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + C.addApproval("Code-Review", 2) + C.addApproval("Approved", 1) + + # A -> B -> C -> 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, C.data["url"] + ) + C.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + C.subject, B.data["url"] + ) + + self.executor_server.failJob("project2-job", C) + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertIn("depends on a change that failed to merge", + A.messages[-1]) + self.assertIn("bundle that failed.", B.messages[-1]) + self.assertIn("bundle that failed.", C.messages[-1]) + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "NEW") + self.assertEqual(C.data["status"], "NEW") + + def test_cycle_dependency_on_change(self): + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B") + C = self.fake_gerrit.addFakeChange("org/project2", "master", "C") + + # A -> B -> A + C (git) + 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"] + ) + B.setDependsOn(C, 1) + + self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(B.patchsets[-1]["approvals"]), 1) + self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(C.patchsets[-1]["approvals"]), 1) + self.assertEqual(C.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(C.patchsets[-1]["approvals"][0]["value"], "1") + + # We're about to add approvals to changes without adding the + # triggering events to Zuul, so that we can be sure that it is + # enqueing the changes based on dependencies, not because of + # triggering events. Since it will have the changes cached + # already (without approvals), we need to clear the cache + # first. + for connection in self.scheds.first.connections.connections.values(): + connection.maintainCache([]) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + C.addApproval("Code-Review", 2) + A.addApproval("Approved", 1) + B.addApproval("Approved", 1) + self.fake_gerrit.addEvent(C.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 3) + self.assertEqual(B.reported, 3) + self.assertEqual(C.reported, 3) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + self.assertEqual(C.data["status"], "MERGED") + + def test_failing_cycle_dependency_on_change(self): + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B") + C = self.fake_gerrit.addFakeChange("org/project2", "master", "C") + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + C.addApproval("Code-Review", 2) + C.addApproval("Approved", 1) + + # A -> B -> A + C (via commit-depends) + A.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + A.subject, B.data["url"] + ) + B.data[ + "commitMessage" + ] = "{}\n\nDepends-On: {}\nDepends-On: {}\n".format( + B.subject, A.data["url"], C.data["url"] + ) + + self.executor_server.failJob("project-job", A) + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 2) + self.assertEqual(B.reported, 2) + self.assertEqual(C.reported, 2) + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "NEW") + self.assertEqual(C.data["status"], "MERGED") + + def test_reopen_cycle(self): + self.executor_server.hold_jobs_in_build = True + A = self.fake_gerrit.addFakeChange("org/project", "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"] + ) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + A.addApproval("Approved", 1) + + self.fake_gerrit.addEvent(B.addApproval("Approved", 1)) + self.waitUntilSettled() + + tenant = self.scheds.first.sched.abide.tenants.get('tenant-one') + items_before = tenant.layout.pipelines['gate'].getAllItems() + + # Trigger a re-enqueue of change B + self.fake_gerrit.addEvent(B.getChangeAbandonedEvent()) + self.fake_gerrit.addEvent(B.getChangeRestoredEvent()) + self.fake_gerrit.addEvent(B.addApproval("Approved", 1)) + self.waitUntilSettled() + + tenant = self.scheds.first.sched.abide.tenants.get('tenant-one') + items_after = tenant.layout.pipelines['gate'].getAllItems() + + # Make sure the complete cycle was re-enqueued + for before, after in zip(items_before, items_after): + self.assertNotEqual(before, after) + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(A.reported, 3) + self.assertEqual(B.reported, 3) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + + def test_cycle_larger_pipeline_window(self): + tenant = self.scheds.first.sched.abide.tenants.get("tenant-one") + + # Make the gate window smaller than the length of the cycle + for queue in tenant.layout.pipelines["gate"].queues: + if any("org/project" in p.name for p in queue.projects): + queue.window = 1 + + self._test_simple_cycle("org/project", "org/project") + + def test_cycle_reporting_failure(self): + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B") + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + + B.fail_merge = True + + # 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.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 2) + self.assertEqual(B.reported, 3) + self.assertEqual(A.patchsets[-1]["approvals"][-1]["value"], "-2") + self.assertEqual(B.patchsets[-1]["approvals"][-1]["value"], "-2") + self.assertIn("bundle", A.messages[-1]) + self.assertIn("bundle", B.messages[-1]) + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "NEW") + + def test_cycle_reporting_partial_failure(self): + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B") + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + + A.fail_merge = True + + # 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.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 3) + self.assertEqual(B.reported, 3) + self.assertIn("bundle", A.messages[-1]) + self.assertIn("bundle", B.messages[-1]) + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "MERGED") + + def test_gate_reset_with_cycle(self): + self.executor_server.hold_jobs_in_build = True + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B") + C = self.fake_gerrit.addFakeChange("org/project1", "master", "C") + + # A <-> B (via depends-on) + 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"] + ) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + C.addApproval("Code-Review", 2) + C.addApproval("Approved", 1) + B.addApproval("Approved", 1) + self.fake_gerrit.addEvent(C.addApproval("Approved", 1)) + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.executor_server.failJob("project1-job", C) + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(len(self.builds), 3) + for build in self.builds: + self.assertTrue(build.hasChanges(A, B)) + self.assertFalse(build.hasChanges(C)) + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(A.reported, 2) + self.assertEqual(B.reported, 2) + self.assertEqual(C.reported, 2) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + self.assertEqual(C.data["status"], "NEW") + + def test_independent_bundle_items(self): + self.executor_server.hold_jobs_in_build = True + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project", "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.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + tenant = self.scheds.first.sched.abide.tenants.get("tenant-one") + for queue in tenant.layout.pipelines["check"].queues: + for item in queue.queue: + self.assertIn(item, item.bundle.items) + self.assertEqual(len(item.bundle.items), 2) + + for build in self.builds: + self.assertTrue(build.hasChanges(A, B)) + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1") + + self.assertEqual(len(B.patchsets[-1]["approvals"]), 1) + self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "1") + + def test_gate_correct_commits(self): + self.executor_server.hold_jobs_in_build = True + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B") + C = self.fake_gerrit.addFakeChange("org/project1", "master", "C") + D = self.fake_gerrit.addFakeChange("org/project", "master", "D") + + # A <-> B (via depends-on) + 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"] + ) + D.setDependsOn(A, 1) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + C.addApproval("Code-Review", 2) + D.addApproval("Code-Review", 2) + C.addApproval("Approved", 1) + B.addApproval("Approved", 1) + self.fake_gerrit.addEvent(C.addApproval("Approved", 1)) + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.fake_gerrit.addEvent(D.addApproval("Approved", 1)) + self.waitUntilSettled() + + for build in self.builds: + if build.change in ("1 1", "2 1"): + self.assertTrue(build.hasChanges(C, B, A)) + self.assertFalse(build.hasChanges(D)) + elif build.change == "3 1": + self.assertTrue(build.hasChanges(C)) + self.assertFalse(build.hasChanges(A)) + self.assertFalse(build.hasChanges(B)) + self.assertFalse(build.hasChanges(D)) + else: + self.assertTrue(build.hasChanges(C, B, A, D)) + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(A.reported, 2) + self.assertEqual(B.reported, 2) + self.assertEqual(C.reported, 2) + self.assertEqual(D.reported, 2) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + self.assertEqual(C.data["status"], "MERGED") + self.assertEqual(D.data["status"], "MERGED") + + def test_cycle_git_dependency(self): + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project", "master", "B") + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + + # A -> B (via commit-depends) + A.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + A.subject, B.data["url"] + ) + # B -> A (via parent-child dependency) + B.setDependsOn(A, 1) + + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 2) + self.assertEqual(B.reported, 2) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + + def test_cycle_git_dependency_failure(self): + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project", "master", "B") + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + + # A -> B (via commit-depends) + A.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + A.subject, B.data["url"] + ) + # B -> A (via parent-child dependency) + B.setDependsOn(A, 1) + + self.executor_server.failJob("project-job", A) + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 2) + self.assertEqual(B.reported, 2) + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "NEW") + + def test_independent_reporting(self): + self.executor_server.hold_jobs_in_build = True + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project", "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.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.fake_gerrit.addEvent(B.getChangeAbandonedEvent()) + self.waitUntilSettled() + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1") + + def test_cycle_merge_conflict(self): + self.gearman_server.hold_merge_jobs_in_queue = True + A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A') + B = self.fake_gerrit.addFakeChange('org/project', '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"] + ) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + # We only want to have a merge failure for the first item in the queue + tenant = self.scheds.first.sched.abide.tenants.get('tenant-one') + items = tenant.layout.pipelines['gate'].getAllItems() + items[0].current_build_set.unable_to_merge = True + + self.waitUntilSettled() + + self.gearman_server.hold_merge_jobs_in_queue = False + self.gearman_server.release() + self.waitUntilSettled() + + self.assertEqual(A.reported, 0) + self.assertEqual(B.reported, 1) + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "NEW") + + def test_circular_config_change(self): + define_job = textwrap.dedent( + """ + - job: + name: new-job + """) + use_job = textwrap.dedent( + """ + - project: + check: + jobs: + - new-job + gate: + queue: integrated + jobs: + - new-job + """) + A = self.fake_gerrit.addFakeChange("org/project", "master", "A", + files={"zuul.yaml": define_job}) + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B", + files={"zuul.yaml": use_job}) + + # 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.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1") + + self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(B.patchsets[-1]["approvals"]), 1) + self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "1") + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + self.fake_gerrit.addEvent(B.addApproval("Approved", 1)) + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 3) + self.assertEqual(B.reported, 3) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + + def test_circular_config_change_job_vars(self): + org_project_files = { + "zuul.yaml": textwrap.dedent( + """ + - job: + name: project-vars-job + vars: + test_var: pass + + - project: + check: + jobs: + - project-vars-job + gate: + queue: integrated + jobs: + - project-vars-job + """) + } + A = self.fake_gerrit.addFakeChange("org/project2", "master", "A", + files=org_project_files) + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B") + C = self.fake_gerrit.addFakeChange("org/project1", "master", "C") + + # C <-> A <-> B (via commit-depends) + A.data["commitMessage"] = ( + "{}\n\nDepends-On: {}\nDepends-On: {}\n".format( + A.subject, B.data["url"], C.data["url"] + ) + ) + B.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + B.subject, A.data["url"] + ) + C.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + C.subject, A.data["url"] + ) + + self.executor_server.hold_jobs_in_build = True + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + vars_builds = [b for b in self.builds if b.name == "project-vars-job"] + self.assertEqual(len(vars_builds), 1) + self.assertEqual(vars_builds[0].parameters["vars"]["test_var"], "pass") + + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "1") + + self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + vars_builds = [b for b in self.builds if b.name == "project-vars-job"] + self.assertEqual(len(vars_builds), 1) + self.assertEqual(vars_builds[0].parameters["vars"]["test_var"], "pass") + + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(len(B.patchsets[-1]["approvals"]), 1) + self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "1") + + self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + vars_builds = [b for b in self.builds if b.name == "project-vars-job"] + self.assertEqual(len(vars_builds), 1) + self.assertEqual(vars_builds[0].parameters["vars"]["test_var"], "pass") + + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(len(C.patchsets[-1]["approvals"]), 1) + self.assertEqual(C.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(C.patchsets[-1]["approvals"][0]["value"], "1") + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + C.addApproval("Code-Review", 2) + + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.fake_gerrit.addEvent(B.addApproval("Approved", 1)) + self.fake_gerrit.addEvent(C.addApproval("Approved", 1)) + self.waitUntilSettled() + + vars_builds = [b for b in self.builds if b.name == "project-vars-job"] + self.assertEqual(len(vars_builds), 3) + for build in vars_builds: + self.assertEqual(build.parameters["vars"]["test_var"], "pass") + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(A.reported, 3) + self.assertEqual(B.reported, 3) + self.assertEqual(C.reported, 3) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + self.assertEqual(C.data["status"], "MERGED") + + def test_cross_tenant_cycle(self): + org_project_files = { + "zuul.yaml": textwrap.dedent( + """ + - job: + name: project-vars-job + vars: + test_var: pass + + - project: + check: + jobs: + - project-vars-job + gate: + queue: integrated + jobs: + - project-vars-job + """) + } + # Change zuul config so the bundle is considered updating config + A = self.fake_gerrit.addFakeChange("org/project2", "master", "A", + files=org_project_files) + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B") + C = self.fake_gerrit.addFakeChange("org/project1", "master", "C") + D = self.fake_gerrit.addFakeChange("org/project4", "master", "D",) + + # C <-> A <-> B (via commit-depends) + A.data["commitMessage"] = ( + "{}\n\nDepends-On: {}\nDepends-On: {}\n".format( + A.subject, B.data["url"], C.data["url"] + ) + ) + # A <-> B (via commit-depends) + B.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + B.subject, A.data["url"] + ) + # A <-> C <-> D (via commit-depends) + C.data["commitMessage"] = ( + "{}\n\nDepends-On: {}\nDepends-On: {}\n".format( + C.subject, A.data["url"], D.data["url"] + ) + ) + # D <-> C (via commit-depends) + D.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format( + D.subject, C.data["url"] + ) + + self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) + self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "-1") + + self.assertEqual(len(B.patchsets[-1]["approvals"]), 1) + self.assertEqual(B.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(B.patchsets[-1]["approvals"][0]["value"], "-1") + + self.assertEqual(len(C.patchsets[-1]["approvals"]), 1) + self.assertEqual(C.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(C.patchsets[-1]["approvals"][0]["value"], "-1") + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + C.addApproval("Code-Review", 2) + D.addApproval("Code-Review", 2) + + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.fake_gerrit.addEvent(B.addApproval("Approved", 1)) + self.fake_gerrit.addEvent(C.addApproval("Approved", 1)) + self.fake_gerrit.addEvent(D.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "NEW") + self.assertEqual(C.data["status"], "NEW") + + D.setMerged() + for connection in self.scheds.first.connections.connections.values(): + connection.maintainCache([]) + + # Pretend D was merged so we can gate the cycle + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 3) + self.assertEqual(B.reported, 3) + self.assertEqual(C.reported, 3) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + self.assertEqual(C.data["status"], "MERGED") + + def test_cycle_unknown_repo(self): + self.init_repo("org/unknown", tag='init') + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/unknown", "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.assertEqual(len(A.patchsets[-1]["approvals"]), 1) + self.assertEqual(A.patchsets[-1]["approvals"][0]["type"], "Verified") + self.assertEqual(A.patchsets[-1]["approvals"][0]["value"], "-1") + + for connection in self.scheds.first.connections.connections.values(): + connection.maintainCache([]) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 1) + self.assertEqual(A.data["status"], "NEW") + self.assertEqual(B.data["status"], "NEW") + + for connection in self.scheds.first.connections.connections.values(): + connection.maintainCache([]) + + B.setMerged() + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + self.assertEqual(A.reported, 3) + self.assertEqual(A.data["status"], "MERGED") + + def test_promote_cycle(self): + self.executor_server.hold_jobs_in_build = True + A = self.fake_gerrit.addFakeChange("org/project", "master", "A") + B = self.fake_gerrit.addFakeChange("org/project1", "master", "B") + C = self.fake_gerrit.addFakeChange("org/project2", "master", "C") + + # A <-> B + 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"] + ) + + A.addApproval("Code-Review", 2) + B.addApproval("Code-Review", 2) + C.addApproval("Code-Review", 2) + B.addApproval("Approved", 1) + self.fake_gerrit.addEvent(C.addApproval("Approved", 1)) + self.fake_gerrit.addEvent(A.addApproval("Approved", 1)) + self.waitUntilSettled() + + client = zuul.rpcclient.RPCClient("127.0.0.1", + self.gearman_server.port) + self.addCleanup(client.shutdown) + client.promote( + tenant="tenant-one", + pipeline="gate", + change_ids=["2,1"] + ) + self.waitUntilSettled() + + self.assertEqual(len(self.builds), 4) + self.assertTrue(self.builds[0].hasChanges(A)) + self.assertTrue(self.builds[0].hasChanges(B)) + self.assertFalse(self.builds[0].hasChanges(C)) + + self.assertTrue(self.builds[1].hasChanges(A)) + self.assertTrue(self.builds[1].hasChanges(B)) + self.assertFalse(self.builds[0].hasChanges(C)) + + self.assertTrue(self.builds[3].hasChanges(B)) + self.assertTrue(self.builds[3].hasChanges(C)) + self.assertTrue(self.builds[3].hasChanges(A)) + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual(A.reported, 2) + self.assertEqual(B.reported, 2) + self.assertEqual(C.reported, 2) + self.assertEqual(A.data["status"], "MERGED") + self.assertEqual(B.data["status"], "MERGED") + self.assertEqual(C.data["status"], "MERGED") + + +class TestGithubCircularDependencies(ZuulTestCase): + config_file = "zuul-gerrit-github.conf" + tenant_config_file = "config/circular-dependencies/main.yaml" + + def test_cycle_not_ready(self): + A = self.fake_github.openFakePullRequest("gh/project", "master", "A") + B = self.fake_github.openFakePullRequest("gh/project1", "master", "B") + C = self.fake_github.openFakePullRequest("gh/project1", "master", "C") + A.addReview('derp', 'APPROVED') + B.addReview('derp', 'APPROVED') + B.addLabel("approved") + C.addReview('derp', 'APPROVED') + + # A -> B + C (via PR depends) + # B -> A + # C -> A + A.body = "{}\n\nDepends-On: {}\nDepends-On: {}\n".format( + A.subject, B.url, C.url + ) + B.body = "{}\n\nDepends-On: {}\n".format( + B.subject, A.url + ) + C.body = "{}\n\nDepends-On: {}\n".format( + C.subject, A.url + ) + + self.fake_github.emitEvent(A.addLabel("approved")) + self.waitUntilSettled() + + self.assertEqual(len(A.comments), 0) + self.assertEqual(len(B.comments), 0) + self.assertEqual(len(C.comments), 0) + self.assertFalse(A.is_merged) + self.assertFalse(B.is_merged) + self.assertFalse(C.is_merged) + + def test_complex_cycle_not_ready(self): + A = self.fake_github.openFakePullRequest("gh/project", "master", "A") + B = self.fake_github.openFakePullRequest("gh/project1", "master", "B") + C = self.fake_github.openFakePullRequest("gh/project1", "master", "C") + X = self.fake_github.openFakePullRequest("gh/project1", "master", "C") + Y = self.fake_github.openFakePullRequest("gh/project1", "master", "C") + A.addReview('derp', 'APPROVED') + A.addLabel("approved") + B.addReview('derp', 'APPROVED') + B.addLabel("approved") + C.addReview('derp', 'APPROVED') + Y.addReview('derp', 'APPROVED') + Y.addLabel("approved") + X.addReview('derp', 'APPROVED') + + # A -> B + C (via PR depends) + # B -> A + # C -> A + # X -> A + Y + # Y -> X + A.body = "{}\n\nDepends-On: {}\nDepends-On: {}\n".format( + A.subject, B.url, C.url + ) + B.body = "{}\n\nDepends-On: {}\n".format( + B.subject, A.url + ) + C.body = "{}\n\nDepends-On: {}\n".format( + C.subject, A.url + ) + X.body = "{}\n\nDepends-On: {}\nDepends-On: {}\n".format( + X.subject, Y.url, A.url + ) + Y.body = "{}\n\nDepends-On: {}\n".format( + Y.subject, X.url + ) + + self.fake_github.emitEvent(X.addLabel("approved")) + self.waitUntilSettled() + + self.assertEqual(len(A.comments), 0) + self.assertEqual(len(B.comments), 0) + self.assertEqual(len(C.comments), 0) + self.assertEqual(len(X.comments), 0) + self.assertEqual(len(Y.comments), 0) + self.assertFalse(A.is_merged) + self.assertFalse(B.is_merged) + self.assertFalse(C.is_merged) + self.assertFalse(X.is_merged) + self.assertFalse(Y.is_merged) + + def test_filter_unprotected_branches(self): + """ + Tests that repo state filtering due to excluding unprotected branches + doesn't break builds if the items are targeted against different + branches. + """ + github = self.fake_github.getGithubClient() + self.create_branch('gh/project', 'stable/foo') + github.repo_from_project('gh/project')._set_branch_protection( + 'master', True) + github.repo_from_project('gh/project')._set_branch_protection( + 'stable/foo', True) + + self.create_branch('gh/project1', 'stable/bar') + github.repo_from_project('gh/project1')._set_branch_protection( + 'master', True) + github.repo_from_project('gh/project1')._set_branch_protection( + 'stable/bar', True) + + # Reconfigure to pick up branch protection settings + self.scheds.execute(lambda app: app.sched.reconfigure(app.config)) + self.waitUntilSettled() + + A = self.fake_github.openFakePullRequest( + "gh/project", "stable/foo", "A") + B = self.fake_github.openFakePullRequest( + "gh/project1", "stable/bar", "B") + A.addReview('derp', 'APPROVED') + B.addReview('derp', 'APPROVED') + B.addLabel("approved") + + # A <-> B + A.body = "{}\n\nDepends-On: {}\n".format( + A.subject, B.url + ) + B.body = "{}\n\nDepends-On: {}\n".format( + B.subject, A.url + ) + + self.fake_github.emitEvent(A.addLabel("approved")) + self.waitUntilSettled() + + self.assertEqual(len(A.comments), 2) + self.assertEqual(len(B.comments), 2) + self.assertTrue(A.is_merged) + self.assertTrue(B.is_merged) diff --git a/zuul/configloader.py b/zuul/configloader.py index c7e6f357d8..5ce090000a 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -1413,6 +1413,7 @@ class QueueParser: def getSchema(self): queue = {vs.Required('name'): str, 'per-branch': bool, + 'allow-circular-dependencies': bool, '_source_context': model.SourceContext, '_start_mark': ZuulMark, } @@ -1420,7 +1421,11 @@ class QueueParser: def fromYaml(self, conf): self.schema(conf) - queue = model.Queue(conf['name'], conf.get('per-branch', False)) + queue = model.Queue( + conf['name'], + conf.get('per-branch', False), + conf.get('allow-circular-dependencies', False), + ) queue.source_context = conf.get('_source_context') queue.start_mark = conf.get('_start_mark') queue.freeze() @@ -1524,6 +1529,7 @@ class TenantParser(object): 'exclude-unprotected-branches': bool, 'extra-config-paths': to_list(str), 'load-branch': str, + 'allow-circular-dependencies': bool, }} project = vs.Any(str, project_dict) @@ -1564,6 +1570,7 @@ class TenantParser(object): 'allowed-reporters': to_list(str), 'allowed-labels': to_list(str), 'disallowed-labels': to_list(str), + 'allow-circular-dependencies': bool, 'default-parent': str, 'default-ansible-version': vs.Any(str, float), 'admin-rules': to_list(str), diff --git a/zuul/lib/tarjan.py b/zuul/lib/tarjan.py new file mode 100644 index 0000000000..1ef479a356 --- /dev/null +++ b/zuul/lib/tarjan.py @@ -0,0 +1,60 @@ +# Algorithm from http://www.logarithmic.net/pfh/blog/01208083168 +# License: public domain +# Authors: Dr. Paul Harrison / Dries Verdegem + + +def strongly_connected_components(graph): + """ + Tarjan's Algorithm (named for its discoverer, Robert Tarjan) is a graph + theory algorithm for finding the strongly connected components of a graph. + + Based on: + http://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_algorithm + """ + + index_counter = [0] + stack = [] + lowlinks = {} + index = {} + result = [] + + def strongconnect(node): + # set the depth index for this node to the smallest unused index + index[node] = index_counter[0] + lowlinks[node] = index_counter[0] + index_counter[0] += 1 + stack.append(node) + + # Consider successors of `node` + try: + successors = graph[node] + except KeyError: + successors = [] + for successor in successors: + if successor not in lowlinks: + # Successor has not yet been visited; recurse on it + strongconnect(successor) + lowlinks[node] = min(lowlinks[node], lowlinks[successor]) + elif successor in stack: + # the successor is in the stack and hence in the current + # strongly connected component (SCC) + lowlinks[node] = min(lowlinks[node], index[successor]) + + # If 'node' is a root node, pop the stack and generate an SCC + if lowlinks[node] == index[node]: + connected_component = [] + + while True: + successor = stack.pop() + connected_component.append(successor) + if successor == node: + break + component = tuple(connected_component) + # storing the result + result.append(component) + + for node in graph: + if node not in lowlinks: + strongconnect(node) + + return result diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index 00e3f48a24..2d9abde279 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -9,6 +9,7 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. +import collections import logging import textwrap import urllib @@ -18,6 +19,7 @@ from zuul import exceptions from zuul import model from zuul.lib.dependson import find_dependency_headers from zuul.lib.logutil import get_annotated_logger +from zuul.lib.tarjan import strongly_connected_components class DynamicChangeQueueContextManager(object): @@ -212,21 +214,28 @@ class PipelineManager(metaclass=ABCMeta): return True def enqueueChangesAhead(self, change, event, quiet, ignore_requirements, - change_queue, history=None): + change_queue, history=None, dependency_graph=None): return True def enqueueChangesBehind(self, change, event, quiet, ignore_requirements, - change_queue): + change_queue, history=None, + dependency_graph=None): return True - def checkForChangesNeededBy(self, change, change_queue, event): + def checkForChangesNeededBy(self, change, change_queue, event, + dependency_graph=None): return True def getFailingDependentItems(self, item): return None - def getItemForChange(self, change): - for item in self.pipeline.getAllItems(): + def getItemForChange(self, change, change_queue=None): + if change_queue is not None: + items = change_queue.queue + else: + items = self.pipeline.getAllItems() + + for item in items: if item.change.equals(change): return item return None @@ -318,10 +327,18 @@ class PipelineManager(metaclass=ABCMeta): def addChange(self, change, event, quiet=False, enqueue_time=None, ignore_requirements=False, live=True, - change_queue=None, history=None): + change_queue=None, history=None, dependency_graph=None): log = get_annotated_logger(self.log, event) log.debug("Considering adding change %s" % change) + history = history if history is not None else [] + log.debug("History: %s", history) + + # Ensure the dependency graph is created when the first change is + # processed to allow cycle detection with the Tarjan algorithm + dependency_graph = dependency_graph or collections.OrderedDict() + log.debug("Dependency graph: %s", dependency_graph) + # If we are adding a live change, check if it's a live item # anywhere in the pipeline. Otherwise, we will perform the # duplicate check below on the specific change_queue. @@ -354,30 +371,47 @@ class PipelineManager(metaclass=ABCMeta): (change, change.project)) return False - if history and change in history: - log.debug("Dependency cycle detected for " - "change %s in project %s" % ( - change, change.project)) - item = model.QueueItem(self, change, event) - item.warning("Dependency cycle detected") - actions = self.pipeline.failure_actions - item.setReportedResult('FAILURE') - self.sendReport(actions, item) - return False - if not self.enqueueChangesAhead(change, event, quiet, ignore_requirements, - change_queue, history=history): + change_queue, history=history, + dependency_graph=dependency_graph): + self.dequeueIncompleteCycle(change, dependency_graph, event, + change_queue) log.debug("Failed to enqueue changes ahead of %s" % change) return False + log.debug("History after enqueuing changes ahead: %s", history) + if self.isChangeAlreadyInQueue(change, change_queue): log.debug("Change %s is already in queue, ignoring" % change) return True + cycle = None + if hasattr(change, "needs_changes"): + cycle = self.cycleForChange(change, dependency_graph, event) + 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 = model.QueueItem(self, 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) + + return False + log.info("Adding change %s to queue %s in %s" % (change, change_queue, self.pipeline)) item = change_queue.enqueueChange(change, event) + self.updateBundle(item, change_queue, cycle) + if enqueue_time: item.enqueue_time = enqueue_time item.live = live @@ -386,8 +420,18 @@ class PipelineManager(metaclass=ABCMeta): if item.live and not item.reported_enqueue: self.reportEnqueue(item) item.reported_enqueue = True - self.enqueueChangesBehind(change, event, quiet, - ignore_requirements, change_queue) + + # Items in a dependency cycle are expected to be enqueued after + # each other. To prevent non-cycle items from being enqueued + # between items of the same cycle, we skip that step when a cycle + # was found. + if not cycle: + self.enqueueChangesBehind(change, event, quiet, + ignore_requirements, change_queue, + history, dependency_graph) + else: + self.log.debug("Skip enqueueing changes behind because of " + "dependency cycle") zuul_driver = self.sched.connections.drivers['zuul'] tenant = self.pipeline.tenant zuul_driver.onChangeEnqueued( @@ -395,6 +439,86 @@ class PipelineManager(metaclass=ABCMeta): self.dequeueSupercededItems(item) return 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", + dependency_graph) + sccs = [s for s in strongly_connected_components(dependency_graph) + if len(s) > 1] + log.debug("Strongly connected components (cyles): %s", sccs) + for scc in sccs: + if change in scc: + log.debug("Dependency cycle detected for " + "change %s in project %s", + change, change.project) + # Change can not be part of multiple cycles, so we can return + return scc + + def canProcessCycle(self, project): + layout = self.pipeline.tenant.layout + pipeline_queue_name = None + project_queue_name = None + for project_config in layout.getAllProjectConfigs( + project.canonical_name + ): + if not project_queue_name: + project_queue_name = project_config.queue_name + + project_pipeline_config = project_config.pipelines.get( + self.pipeline.name) + + if project_pipeline_config is None: + continue + + # TODO(simonw): Remove pipeline_queue_name after deprecation + if not pipeline_queue_name: + pipeline_queue_name = project_pipeline_config.queue_name + + # Note: we currently support queue name per pipeline and per + # project while project has precedence. + queue_name = project_queue_name or pipeline_queue_name + if queue_name is None: + return False + + queue_config = layout.queues.get(queue_name) + return ( + queue_config is not None and + queue_config.allow_circular_dependencies + ) + + def updateBundle(self, item, change_queue, cycle): + if not cycle: + return + + log = get_annotated_logger(self.log, item.event) + item.bundle = model.Bundle() + + # Try to find already enqueued items of this cycle, so we use + # the same bundle + for needed_change in (c for c in cycle if c is not item.change): + needed_item = self.getItemForChange(needed_change, change_queue) + if not needed_item: + continue + # Use a common bundle for the cycle + item.bundle = needed_item.bundle + break + + log.info("Adding cycle item %s to bundle %s", item, item.bundle) + item.bundle.add_item(item) + + def dequeueIncompleteCycle(self, change, dependency_graph, event, + change_queue): + log = get_annotated_logger(self.log, event) + cycle = self.cycleForChange(change, dependency_graph, event) or [] + enqueued_cycle_items = [i for i in (self.getItemForChange(c, + change_queue) + for c in cycle) if i is not None] + if enqueued_cycle_items: + log.info("Dequeuing incomplete cycle items: %s", + enqueued_cycle_items) + for cycle_item in enqueued_cycle_items: + self.dequeueItem(cycle_item) + def dequeueItem(self, item): log = get_annotated_logger(self.log, item.event) log.debug("Removing change %s from queue", item.change) @@ -416,6 +540,16 @@ class PipelineManager(metaclass=ABCMeta): self.dequeueItem(item) self.reportStats(item) + if item.bundle is None: + return + + log.debug("Dequeueing items in bundle %s", item.bundle) + bundle_iter = (i for i in item.bundle.items if i is not item) + for bundle_item in bundle_iter: + self.cancelJobs(bundle_item) + self.dequeueItem(bundle_item) + self.reportStats(bundle_item) + def dequeueSupercededItems(self, item): for other_name in self.pipeline.supercedes: other_pipeline = self.pipeline.tenant.layout.pipelines.get( @@ -525,7 +659,11 @@ class PipelineManager(metaclass=ABCMeta): old_build_set = item.current_build_set jobs_to_cancel = item.getJobs() - if prime and item.current_build_set.ref: + # Don't reset builds for a failing bundle when it has already started + # reporting, to keep available build results. Those items will be + # reported immediately afterwards during queue processing. + if (prime and item.current_build_set.ref and not + item.didBundleStartReporting()): item.resetAllBuilds() for job in jobs_to_cancel: @@ -677,13 +815,6 @@ class PipelineManager(metaclass=ABCMeta): item.setConfigError("Unknown configuration error") return None - def _queueUpdatesConfig(self, item): - while item: - if item.change.updatesConfig(item.pipeline.tenant): - return True - item = item.item_ahead - return False - def getLayout(self, item): if item.item_ahead: fallback_layout = item.item_ahead.layout @@ -692,8 +823,21 @@ class PipelineManager(metaclass=ABCMeta): return None else: fallback_layout = item.pipeline.tenant.layout - if not item.change.updatesConfig(item.pipeline.tenant): - # Current change does not update layout, use its parent. + + # If the current change does not update the layout, use its parent. + # If the bundle doesn't update the config or the bundle updates the + # config but the current change's project is not part of the tenant + # (e.g. when dealing w/ cross-tenant cycles), use the parent layout. + if not ( + item.change.updatesConfig(item.pipeline.tenant) or + ( + item.bundle + and item.bundle.updatesConfig(item.pipeline.tenant) + and item.pipeline.tenant.getProject( + item.change.project.canonical_name + )[1] is not None + ) + ): return fallback_layout # Else this item updates the config, # ask the merger for the result. @@ -717,10 +861,12 @@ class PipelineManager(metaclass=ABCMeta): # change that is tested. tenant = item.pipeline.tenant items = list(item.items_ahead) + [item] - projects = [ + if item.bundle: + items.extend(item.bundle.items) + projects = { item.change.project for item in items if tenant.getProject(item.change.project.canonical_name)[1] - ] + } if all(tenant.getExcludeUnprotectedBranches(project) for project in projects): branches = set() @@ -792,8 +938,15 @@ class PipelineManager(metaclass=ABCMeta): ready = False # If this change alters config or is live, schedule merge and # build a layout. + # If we are dealing w/ a bundle and the bundle updates config we also + # have to merge since a config change in any of the bundle's items + # applies to all items. This is, unless the current item is not part + # of this tenant (e.g. cross-tenant cycle). if build_set.merge_state == build_set.NEW: - if item.live or item.change.updatesConfig(tenant): + if item.live or item.change.updatesConfig(tenant) or ( + item.bundle and + item.bundle.updatesConfig(tenant) and tpc is not None + ): ready = self.scheduleMerge( item, files=(['zuul.yaml', '.zuul.yaml'] + @@ -864,7 +1017,10 @@ class PipelineManager(metaclass=ABCMeta): "it can no longer merge" % item.change) self.cancelJobs(item) self.dequeueItem(item) - item.setDequeuedNeedingChange() + if item.isBundleFailing(): + item.setDequeuedBundleFailing() + else: + item.setDequeuedNeedingChange() if item.live: try: self.reportItem(item) @@ -911,6 +1067,12 @@ class PipelineManager(metaclass=ABCMeta): failing_reasons.append("it has an invalid configuration") if ready and self.provisionNodes(item): changed = True + if ready and item.bundle and item.didBundleFinish(): + # Since the bundle finished we need to check if any item + # can report. If that's the case we need to process the + # queue again. + changed = changed or any( + i.item_ahead is None for i in item.bundle.items) if ready and self.executeJobs(item): changed = True @@ -920,7 +1082,15 @@ class PipelineManager(metaclass=ABCMeta): failing_reasons.append("is a non-live item with no items behind") self.dequeueItem(item) changed = dequeued = True - if ((not item_ahead) and item.areAllJobsComplete() and item.live): + + can_report = not item_ahead and item.areAllJobsComplete() and item.live + if can_report and item.bundle: + can_report = can_report and ( + item.isBundleFailing() or item.didBundleFinish() + ) + item.bundle.started_reporting = can_report + + if can_report: try: self.reportItem(item) except exceptions.MergeFailure: @@ -930,6 +1100,9 @@ class PipelineManager(metaclass=ABCMeta): "item ahead, %s, failed to merge" % (item_behind.change, item)) self.cancelJobs(item_behind) + if item.bundle and not item.isBundleFailing(): + item.bundle.failed_reporting = True + self.reportProcessedBundleItems(item) self.dequeueItem(item) changed = dequeued = True elif not failing_reasons and item.live: @@ -946,6 +1119,19 @@ class PipelineManager(metaclass=ABCMeta): node_request, priority) return (changed, nnfi) + def reportProcessedBundleItems(self, item): + """Report failure to already reported bundle items. + + In case we encounter e.g. a merge failure when we already successfully + reported some items, we need to go back and report again. + """ + reported_items = [i for i in item.bundle.items if i.reported] + + actions = self.pipeline.failure_actions + for ri in reported_items: + ri.setReportedResult('FAILURE') + self.sendReport(actions, ri) + def processQueue(self): # Do whatever needs to be done for each change in the queue self.log.debug("Starting queue processor: %s" % self.pipeline.name) @@ -1117,7 +1303,7 @@ class PipelineManager(metaclass=ABCMeta): # _reportItem() returns True if it failed to report. item.reported = not self._reportItem(item) if self.changes_merge: - succeeded = item.didAllJobsSucceed() + succeeded = item.didAllJobsSucceed() and not item.isBundleFailing() merged = item.reported source = item.change.project.source if merged: @@ -1181,9 +1367,11 @@ class PipelineManager(metaclass=ABCMeta): actions = self.pipeline.merge_failure_actions item.setReportedResult('CONFIG_ERROR') elif item.didMergerFail(): + log.debug("Merger failure") actions = self.pipeline.merge_failure_actions item.setReportedResult('MERGER_FAILURE') elif item.wasDequeuedNeedingChange(): + log.debug("Dequeued needing change") actions = self.pipeline.failure_actions item.setReportedResult('FAILURE') elif not item.getJobs(): @@ -1191,7 +1379,13 @@ class PipelineManager(metaclass=ABCMeta): log.debug("No jobs for change %s", item.change) actions = self.pipeline.no_jobs_actions item.setReportedResult('NO_JOBS') - elif item.didAllJobsSucceed(): + elif item.isBundleFailing(): + log.debug("Bundle is failing") + actions = self.pipeline.failure_actions + item.setReportedResult("FAILURE") + if not item.didAllJobsSucceed(): + self.pipeline._consecutive_failures += 1 + elif item.didAllJobsSucceed() and not item.isBundleFailing(): log.debug("success %s", self.pipeline.success_actions) actions = self.pipeline.success_actions item.setReportedResult('SUCCESS') diff --git a/zuul/manager/dependent.py b/zuul/manager/dependent.py index 5d9450fc4b..ea0b4ab3a0 100644 --- a/zuul/manager/dependent.py +++ b/zuul/manager/dependent.py @@ -55,8 +55,10 @@ class DependentPipelineManager(SharedQueuePipelineManager): return True def enqueueChangesBehind(self, change, event, quiet, ignore_requirements, - change_queue): + change_queue, history=None, + dependency_graph=None): log = get_annotated_logger(self.log, event) + history = history if history is not None else [] log.debug("Checking for changes needing %s:" % change) if not hasattr(change, 'needed_by_changes'): @@ -84,7 +86,17 @@ class DependentPipelineManager(SharedQueuePipelineManager): log.debug(" Following changes: %s", needed_by_changes) to_enqueue = [] + change_dependencies = dependency_graph.get(change, []) for other_change in needed_by_changes: + if other_change in change_dependencies: + # Only consider the change if it is not part of a cycle, as + # cycle changes will otherwise be partially enqueued without + # any error handling + self.log.debug( + " Skipping change %s due to dependency cycle" + ) + continue + with self.getChangeQueue(other_change, event) as other_change_queue: if other_change_queue != change_queue: @@ -106,32 +118,41 @@ class DependentPipelineManager(SharedQueuePipelineManager): for other_change in to_enqueue: self.addChange(other_change, event, quiet=quiet, ignore_requirements=ignore_requirements, - change_queue=change_queue) + change_queue=change_queue, history=history, + dependency_graph=dependency_graph) def enqueueChangesAhead(self, change, event, quiet, ignore_requirements, - change_queue, history=None): + change_queue, history=None, dependency_graph=None): log = get_annotated_logger(self.log, event) + history = history if history is not None else [] if hasattr(change, 'number'): - history = history or [] - history = history + [change] + history.append(change) else: # Don't enqueue dependencies ahead of a non-change ref. return True - ret = self.checkForChangesNeededBy(change, change_queue, event) + ret = self.checkForChangesNeededBy(change, change_queue, event, + dependency_graph=dependency_graph) if ret in [True, False]: return ret log.debug(" Changes %s must be merged ahead of %s", ret, change) for needed_change in ret: - r = self.addChange(needed_change, event, quiet=quiet, - ignore_requirements=ignore_requirements, - change_queue=change_queue, history=history) - if not r: - return False + # If the change is already in the history, but the change also has + # a git level dependency, we need to enqueue it before the current + # change. + if (needed_change not in history or + needed_change in change.git_needs_changes): + r = self.addChange(needed_change, event, quiet=quiet, + ignore_requirements=ignore_requirements, + change_queue=change_queue, history=history, + dependency_graph=dependency_graph) + if not r: + return False return True - def checkForChangesNeededBy(self, change, change_queue, event): + def checkForChangesNeededBy(self, change, change_queue, event, + dependency_graph=None): log = get_annotated_logger(self.log, event) # Return true if okay to proceed enqueing this change, @@ -155,6 +176,13 @@ class DependentPipelineManager(SharedQueuePipelineManager): if needed_change.is_merged: log.debug(" Needed change is merged") continue + + if dependency_graph is not None: + log.debug(" Adding change %s to dependency graph for " + "change %s", needed_change, change) + node = dependency_graph.setdefault(change, []) + node.append(needed_change) + with self.getChangeQueue(needed_change, event) as needed_change_queue: if needed_change_queue != change_queue: @@ -197,6 +225,9 @@ class DependentPipelineManager(SharedQueuePipelineManager): continue if needed_item.current_build_set.failing_reasons: failing_items.add(needed_item) + if item.isBundleFailing(): + failing_items.update(item.bundle.items) + failing_items.remove(item) if failing_items: return failing_items return None diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py index b57f7a0780..7686d5a7d4 100644 --- a/zuul/manager/independent.py +++ b/zuul/manager/independent.py @@ -37,17 +37,18 @@ class IndependentPipelineManager(PipelineManager): return DynamicChangeQueueContextManager(change_queue) def enqueueChangesAhead(self, change, event, quiet, ignore_requirements, - change_queue, history=None): + change_queue, history=None, dependency_graph=None): log = get_annotated_logger(self.log, event) + history = history if history is not None else [] if hasattr(change, 'number'): - history = history or [] - history = history + [change] + history.append(change) else: # Don't enqueue dependencies ahead of a non-change ref. return True - ret = self.checkForChangesNeededBy(change, change_queue, event) + ret = self.checkForChangesNeededBy(change, change_queue, event, + dependency_graph=dependency_graph) if ret in [True, False]: return ret log.debug(" Changes %s must be merged ahead of %s" % (ret, change)) @@ -57,15 +58,17 @@ class IndependentPipelineManager(PipelineManager): # have jobs run. Also, pipeline requirements are always # ignored (which is safe because the changes are not # live). - r = self.addChange(needed_change, event, quiet=True, - ignore_requirements=True, - live=False, change_queue=change_queue, - history=history) - if not r: - return False + if needed_change not in history: + r = self.addChange(needed_change, event, quiet=True, + ignore_requirements=True, live=False, + change_queue=change_queue, history=history, + dependency_graph=dependency_graph) + if not r: + return False return True - def checkForChangesNeededBy(self, change, change_queue, event): + def checkForChangesNeededBy(self, change, change_queue, event, + dependency_graph=None): log = get_annotated_logger(self.log, event) if self.pipeline.ignore_dependencies: @@ -89,6 +92,13 @@ class IndependentPipelineManager(PipelineManager): if needed_change.is_merged: log.debug(" Needed change is merged") continue + + if dependency_graph is not None: + log.debug(" Adding change %s to dependency graph for " + "change %s", needed_change, change) + node = dependency_graph.setdefault(change, []) + node.append(needed_change) + if self.isChangeAlreadyInQueue(needed_change, change_queue): log.debug(" Needed change is already ahead in the queue") continue diff --git a/zuul/model.py b/zuul/model.py index d0b741f80d..3114cebc3f 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -499,10 +499,15 @@ class ChangeQueue(object): return True def isActionable(self, item): - if self.window: - return item in self.queue[:self.window] - else: + if not self.window: return True + # Ignore done items waiting for bundle dependencies to finish + num_waiting_items = len([ + i for i in self.queue + if i.bundle and i.areAllJobsComplete() + ]) + window = self.window + num_waiting_items + return item in self.queue[:window] def increaseWindowSize(self): if self.window: @@ -2083,12 +2088,15 @@ class BuildSet(object): if not self.uuid: self.uuid = uuid4().hex if self.dependent_changes is None: - items = [self.item] - next_item = self.item.item_ahead - while next_item: - items.append(next_item) - next_item = next_item.item_ahead + items = [] + if self.item.bundle: + items.extend(reversed(self.item.bundle.items)) + else: + items.append(self.item) + + items.extend(i for i in self.item.items_ahead if i not in items) items.reverse() + self.dependent_changes = [i.change.toDict() for i in items] self.merger_items = [i.makeMergerItem() for i in items] @@ -2222,6 +2230,11 @@ class QueueItem(object): # by reporters throughout the lifecycle self.dynamic_state = defaultdict(dict) + # A bundle holds other queue items that have to be successful + # for the current queue item to succeed + self.bundle = None + self.dequeued_bundle_failing = False + def annotateLogger(self, logger): """Return an annotated logger with the trigger event""" return get_annotated_logger(logger, self.event) @@ -2394,6 +2407,31 @@ class QueueItem(object): return True return False + def isBundleFailing(self): + if self.bundle: + # We are only checking other items that share the same change + # queue, since we don't need to wait for changes in other change + # queues. + return self.bundle.failed_reporting or any( + i.hasAnyJobFailed() or i.didMergerFail() + for i in self.bundle.items + if i.live and i.queue == self.queue) + return False + + def didBundleFinish(self): + if self.bundle: + # We are only checking other items that share the same change + # queue, since we don't need to wait for changes in other change + # queues. + return all(i.areAllJobsComplete() for i in self.bundle.items if + i.live and i.queue == self.queue) + return True + + def didBundleStartReporting(self): + if self.bundle: + return self.bundle.started_reporting + return False + def didMergerFail(self): return self.current_build_set.unable_to_merge @@ -2408,6 +2446,21 @@ class QueueItem(object): includes_untrusted = False tenant = self.pipeline.tenant item = self + + if item.bundle: + # Check all items in the bundle for config updates + for bundle_item in item.bundle.items: + if bundle_item.change.updatesConfig(tenant): + trusted, project = tenant.getProject( + bundle_item.change.project.canonical_name) + if trusted: + includes_trusted = True + else: + includes_untrusted = True + if includes_trusted and includes_untrusted: + # We're done early + return includes_trusted, includes_untrusted + while item: if item.change.updatesConfig(tenant): (trusted, project) = tenant.getProject( @@ -2781,6 +2834,10 @@ class QueueItem(object): self.dequeued_needing_change = True self._setAllJobsSkipped() + def setDequeuedBundleFailing(self): + self.dequeued_bundle_failing = True + self._setMissingJobsSkipped() + def setUnableToMerge(self): self.current_build_set.unable_to_merge = True self._setAllJobsSkipped() @@ -2799,6 +2856,15 @@ class QueueItem(object): fakebuild.result = 'SKIPPED' self.addBuild(fakebuild) + def _setMissingJobsSkipped(self): + for job in self.getJobs(): + if job.name in self.current_build_set.builds: + # We already have a build for this job + continue + fakebuild = Build(job, None) + fakebuild.result = 'SKIPPED' + self.addBuild(fakebuild) + def getNodePriority(self): return self.pipeline.manager.getNodePriority(self) @@ -3135,6 +3201,25 @@ class QueueItem(object): return False +class Bundle: + """Identifies a collection of changes that must be treated as one unit.""" + + def __init__(self): + self.items = [] + self.started_reporting = False + self.failed_reporting = False + + def __repr__(self): + return '