Clean up safety check
The safety check was originally written to detect when a dependncy cycle changed without the pipeline manager noticing. Since the dependency cycle refactor, items can have multiple changes and previous processes that were designed around updates to items causing cascading updates to items behind them (but in the same bundle) no longer make as much sense. However, the "safety check" now seems to make more sense as the primary method for determining that a dependency cycle has changed. It fits in well with other checks in pipeline processing now that it examines the situation for a single item. Resolve the temporary safety check by keeping it. It is cleaned up a bit and moved earlier in the pipeline processing. We can also clean up the slightly awkward silent dequeue/re-enqueue method and incorporate it into the safety check. Now the process is: If a dpendency cycle changes, dequeue the item without reporting and then re-enqueue all of the items changes. This means that if a dependency cycle (no matter how large) is split in half, we will keep both halves (now separate) in the pipeline. This behavior is likely to be the most intuitive to users. In general, there are two ways to update a dependency cycle: with a new patchset that changes the graph (typically only gerrit) or with a PR message or topic change (gerrit and others). To achieve some consistency between these methods, we reuse the same re-enqueue method in both cases (but in the case of a patchset superceding a change, we don't re-enqueue the old change; but we do expect the patchset-created event to enqueue the new version). The timing is still a little different, but the results are the same. Change-Id: Ifa42b081cbd103ef04d8814c27ab5c51aa5e8335
This commit is contained in:
James E. Blair
parent
1cc276687a
commit
ca83980bb7
@ -16,7 +16,6 @@
|
||||
|
||||
import re
|
||||
import textwrap
|
||||
from unittest import skip
|
||||
|
||||
from zuul.model import PromoteEvent
|
||||
|
||||
@ -4113,7 +4112,6 @@ class TestGithubCircularDependencies(ZuulTestCase):
|
||||
self.assertHistory([
|
||||
dict(name="project-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha}"),
|
||||
# TODO: changed for safety check
|
||||
dict(name="project-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha}"),
|
||||
dict(name="project-job", result="SUCCESS",
|
||||
@ -4165,16 +4163,12 @@ class TestGithubAppCircularDependencies(ZuulGithubAppTestCase):
|
||||
self.fake_github.emitEvent(A.getPullRequestEditedEvent(A.subject))
|
||||
self.waitUntilSettled()
|
||||
|
||||
# Validate that the Github check-run is still in progress
|
||||
# and wasn't cancelled.
|
||||
|
||||
# TODO: we actually do cancel the check-run now due to the
|
||||
# safety check in processOneItem. In the future we should be
|
||||
# able to avoid that.
|
||||
# Validate that the Github check-run was cancelled and a new
|
||||
# one restarted.
|
||||
check_runs = self.fake_github.getCommitChecks("gh/project", A.head_sha)
|
||||
self.assertEqual(len(check_runs), 1)
|
||||
check_run = check_runs[0]
|
||||
# self.assertEqual(check_run["status"], "in_progress")
|
||||
self.assertEqual(len(check_runs), 2)
|
||||
self.assertEqual(check_runs[0]["status"], "in_progress")
|
||||
self.assertEqual(check_runs[1]["status"], "completed")
|
||||
|
||||
self.executor_server.hold_jobs_in_build = False
|
||||
self.executor_server.release()
|
||||
@ -4183,7 +4177,6 @@ class TestGithubAppCircularDependencies(ZuulGithubAppTestCase):
|
||||
self.assertHistory([
|
||||
dict(name="project-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha}"),
|
||||
# TODO: changed for safety check
|
||||
dict(name="project-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha}"),
|
||||
dict(name="project-job", result="SUCCESS",
|
||||
@ -4193,8 +4186,7 @@ class TestGithubAppCircularDependencies(ZuulGithubAppTestCase):
|
||||
changes=f"{B.number},{B.head_sha} {A.number},{A.head_sha}",
|
||||
ref="refs/pull/2/head"),
|
||||
], ordered=False)
|
||||
# TODO: We shouldn't need this in the future, but for now,
|
||||
# verify that since we are issuing two check runs, they both
|
||||
# Verify that since we are issuing two check runs, they both
|
||||
# complete.
|
||||
check_runs = self.fake_github.getCommitChecks("gh/project", A.head_sha)
|
||||
self.assertEqual(len(check_runs), 2)
|
||||
@ -4212,182 +4204,8 @@ class TestGithubAppCircularDependencies(ZuulGithubAppTestCase):
|
||||
self.assertEqual(check_run["status"], "completed")
|
||||
self.assertNotEqual(check_runs[0]["id"], check_runs[1]["id"])
|
||||
|
||||
@skip("Disabled due to safety check")
|
||||
@simple_layout('layouts/dependency_removal_gate.yaml', driver='github')
|
||||
def test_dependency_removal_gate_orig(self):
|
||||
# Dependency cycles can be updated without uploading new
|
||||
# patchsets. This test exercises such changes.
|
||||
|
||||
self.executor_server.hold_jobs_in_build = True
|
||||
tenant = self.scheds.first.sched.abide.tenants.get("tenant-one")
|
||||
pipeline = tenant.layout.pipelines["gate"]
|
||||
|
||||
A = self.fake_github.openFakePullRequest("gh/project1", "master", "A")
|
||||
B = self.fake_github.openFakePullRequest("gh/project2", "master", "B")
|
||||
C = self.fake_github.openFakePullRequest("gh/project3", "master", "C")
|
||||
D = self.fake_github.openFakePullRequest("gh/project", "master", "D")
|
||||
E = self.fake_github.openFakePullRequest("gh/project", "master", "E")
|
||||
|
||||
# Because we call setConfiguration immediately upon removing a
|
||||
# change, in order to fully exercise setConfiguration on a
|
||||
# queue that has been reloaded from ZK with potentially stale
|
||||
# bundle data, we need to remove a change from the bundle
|
||||
# twice. ABC is the main bundle we're interested in. E is a
|
||||
# sacrificial change to force a bundle update when it's
|
||||
# removed.
|
||||
|
||||
# E->C, A->E, B->A, C->B
|
||||
E.body = "{}\n\nDepends-On: {}\n".format(
|
||||
E.subject, C.url
|
||||
)
|
||||
A.body = "{}\n\nDepends-On: {}\n".format(
|
||||
A.subject, E.url
|
||||
)
|
||||
B.body = "{}\n\nDepends-On: {}\n".format(
|
||||
B.subject, A.url
|
||||
)
|
||||
C.body = "{}\n\nDepends-On: {}\n".format(
|
||||
C.subject, B.url
|
||||
)
|
||||
|
||||
A.addLabel("approved")
|
||||
B.addLabel("approved")
|
||||
D.addLabel("approved")
|
||||
E.addLabel("approved")
|
||||
|
||||
self.fake_github.emitEvent(C.addLabel("approved"))
|
||||
self.waitUntilSettled()
|
||||
expected_cycle = {A.number, B.number, C.number, E.number}
|
||||
self.assertEqual(len(list(pipeline.getAllItems())), 4)
|
||||
for item in pipeline.getAllItems():
|
||||
cycle = {i.change.number for i in item.bundle.items}
|
||||
self.assertEqual(expected_cycle, cycle)
|
||||
|
||||
# Now we remove the dependency on E. The change E itself
|
||||
# won't be removed yet.
|
||||
|
||||
# A->C, B->A, C->B
|
||||
A.body = "{}\n\nDepends-On: {}\n".format(
|
||||
A.subject, C.url
|
||||
)
|
||||
self.fake_github.emitEvent(A.getPullRequestEditedEvent(A.body))
|
||||
self.waitUntilSettled()
|
||||
self.assertEqual(len(list(pipeline.getAllItems())), 4)
|
||||
self.assertIsNone(pipeline.getAllItems()[0].bundle)
|
||||
# E at the head is alone
|
||||
# The next 3 are a cycle
|
||||
expected_cycle = {A.number, B.number, C.number}
|
||||
for item in pipeline.getAllItems()[1:]:
|
||||
cycle = {i.change.number for i in item.bundle.items}
|
||||
self.assertEqual(expected_cycle, cycle)
|
||||
|
||||
# Now remove E from the queue by forcing a dependency on D,
|
||||
# which is not enqueued.
|
||||
E.body = "{}\n\nDepends-On: {}\n".format(
|
||||
E.subject, D.url
|
||||
)
|
||||
self.fake_github.emitEvent(E.getPullRequestEditedEvent(E.body))
|
||||
self.waitUntilSettled()
|
||||
self.assertEqual(len(list(pipeline.getAllItems())), 3)
|
||||
expected_cycle = {A.number, B.number, C.number}
|
||||
for item in pipeline.getAllItems():
|
||||
cycle = {i.change.number for i in item.bundle.items}
|
||||
self.assertEqual(expected_cycle, cycle)
|
||||
|
||||
# Now remove all dependencies for the three remaining changes.
|
||||
A.body = A.subject
|
||||
B.body = B.subject
|
||||
C.body = C.subject
|
||||
|
||||
self.fake_github.emitEvent(A.getPullRequestEditedEvent(A.body))
|
||||
self.waitUntilSettled()
|
||||
self.fake_github.emitEvent(B.getPullRequestEditedEvent(B.body))
|
||||
self.waitUntilSettled()
|
||||
self.fake_github.emitEvent(C.getPullRequestEditedEvent(C.body))
|
||||
self.waitUntilSettled()
|
||||
self.assertEqual(len(list(pipeline.getAllItems())), 3)
|
||||
for item in pipeline.getAllItems():
|
||||
self.assertIsNone(item.bundle)
|
||||
|
||||
# Remove the first change from the queue by forcing a
|
||||
# dependency on D.
|
||||
A.body = "{}\n\nDepends-On: {}\n".format(
|
||||
A.subject, D.url
|
||||
)
|
||||
self.fake_github.emitEvent(A.getPullRequestEditedEvent(A.body))
|
||||
self.waitUntilSettled()
|
||||
self.assertEqual(len(list(pipeline.getAllItems())), 2)
|
||||
for item in pipeline.getAllItems():
|
||||
self.assertIsNone(item.bundle)
|
||||
|
||||
# B and C are still in the queue. Verify that we can put them
|
||||
# back into a bundle.
|
||||
C.body = "{}\n\nDepends-On: {}\n".format(
|
||||
C.subject, B.url
|
||||
)
|
||||
self.fake_github.emitEvent(C.getPullRequestEditedEvent(C.body))
|
||||
self.waitUntilSettled()
|
||||
B.body = "{}\n\nDepends-On: {}\n".format(
|
||||
B.subject, C.url
|
||||
)
|
||||
self.fake_github.emitEvent(B.getPullRequestEditedEvent(B.body))
|
||||
self.waitUntilSettled()
|
||||
self.assertEqual(len(list(pipeline.getAllItems())), 2)
|
||||
expected_cycle = {B.number, C.number}
|
||||
for item in pipeline.getAllItems():
|
||||
cycle = {i.change.number for i in item.bundle.items}
|
||||
self.assertEqual(expected_cycle, cycle)
|
||||
|
||||
# All done.
|
||||
self.executor_server.hold_jobs_in_build = False
|
||||
self.executor_server.release()
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertHistory([
|
||||
dict(name="project-job", result="ABORTED",
|
||||
changes=f"{E.number},{E.head_sha} {A.number},{A.head_sha} "
|
||||
f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
dict(name="common-job", result="ABORTED",
|
||||
changes=f"{E.number},{E.head_sha} {A.number},{A.head_sha} "
|
||||
f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
dict(name="project1-job", result="ABORTED",
|
||||
changes=f"{E.number},{E.head_sha} {A.number},{A.head_sha} "
|
||||
f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
dict(name="project2-job", result="ABORTED",
|
||||
changes=f"{E.number},{E.head_sha} {A.number},{A.head_sha} "
|
||||
f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
dict(name="project3-job", result="ABORTED",
|
||||
changes=f"{E.number},{E.head_sha} {A.number},{A.head_sha} "
|
||||
f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
|
||||
dict(name="common-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha} "
|
||||
f"{C.number},{C.head_sha}"),
|
||||
dict(name="project1-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha} "
|
||||
f"{C.number},{C.head_sha}"),
|
||||
dict(name="project2-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha} "
|
||||
f"{C.number},{C.head_sha}"),
|
||||
dict(name="project3-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha} "
|
||||
f"{C.number},{C.head_sha}"),
|
||||
|
||||
dict(name="common-job", result="SUCCESS",
|
||||
changes=f"{B.number},{B.head_sha}"),
|
||||
dict(name="project2-job", result="SUCCESS",
|
||||
changes=f"{B.number},{B.head_sha}"),
|
||||
|
||||
dict(name="common-job", result="SUCCESS",
|
||||
changes=f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
dict(name="project3-job", result="SUCCESS",
|
||||
changes=f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
], ordered=False)
|
||||
|
||||
@simple_layout('layouts/dependency_removal_gate.yaml', driver='github')
|
||||
def test_dependency_removal_gate(self):
|
||||
# TODO: After removal of the safety check, remove this test
|
||||
# and use the _orig version.
|
||||
# Dependency cycles can be updated without uploading new
|
||||
# patchsets. This test exercises such changes.
|
||||
|
||||
@ -4504,73 +4322,8 @@ class TestGithubAppCircularDependencies(ZuulGithubAppTestCase):
|
||||
for build in self.history[-3:]:
|
||||
self.assertEqual(build.result, 'SUCCESS')
|
||||
|
||||
@skip("Disabled due to safety check")
|
||||
@simple_layout('layouts/dependency_removal_gate.yaml', driver='github')
|
||||
def test_dependency_addition_gate_orig(self):
|
||||
# Dependency cycles can be updated without uploading new
|
||||
# patchsets. This test exercises such changes.
|
||||
|
||||
self.executor_server.hold_jobs_in_build = True
|
||||
tenant = self.scheds.first.sched.abide.tenants.get("tenant-one")
|
||||
pipeline = tenant.layout.pipelines["gate"]
|
||||
|
||||
A = self.fake_github.openFakePullRequest("gh/project1", "master", "A")
|
||||
B = self.fake_github.openFakePullRequest("gh/project2", "master", "B")
|
||||
|
||||
B.addLabel("approved")
|
||||
|
||||
self.fake_github.emitEvent(A.addLabel("approved"))
|
||||
self.waitUntilSettled()
|
||||
|
||||
expected_cycle = {A.number}
|
||||
self.assertEqual(len(list(pipeline.getAllItems())), 1)
|
||||
for item in pipeline.getAllItems():
|
||||
self.assertIsNone(item.bundle)
|
||||
|
||||
B.body = "{}\n\nDepends-On: {}\n".format(
|
||||
B.subject, A.url
|
||||
)
|
||||
self.fake_github.emitEvent(B.getPullRequestEditedEvent(B.body))
|
||||
self.waitUntilSettled()
|
||||
|
||||
A.body = "{}\n\nDepends-On: {}\n".format(
|
||||
A.subject, B.url
|
||||
)
|
||||
self.fake_github.emitEvent(A.getPullRequestEditedEvent(A.body))
|
||||
self.waitUntilSettled()
|
||||
self.assertEqual(len(list(pipeline.getAllItems())), 2)
|
||||
expected_cycle = {A.number, B.number}
|
||||
for item in pipeline.getAllItems():
|
||||
cycle = {i.change.number for i in item.bundle.items}
|
||||
self.assertEqual(expected_cycle, cycle)
|
||||
|
||||
# All done.
|
||||
self.executor_server.hold_jobs_in_build = False
|
||||
self.executor_server.release()
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertHistory([
|
||||
# Change 1 first enqueue
|
||||
dict(name="project1-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha}"),
|
||||
dict(name="common-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha}"),
|
||||
|
||||
# Change 1 bundle update
|
||||
dict(name="project1-job", result="SUCCESS",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha}"),
|
||||
dict(name="common-job", result="SUCCESS",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha}"),
|
||||
|
||||
# Change 2 bundle update
|
||||
dict(name="project2-job", result="SUCCESS",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha}"),
|
||||
], ordered=False)
|
||||
|
||||
@simple_layout('layouts/dependency_removal_gate.yaml', driver='github')
|
||||
def test_dependency_addition_gate(self):
|
||||
# TODO: After removal of the safety check, remove this test
|
||||
# and use the _orig version.
|
||||
# Dependency cycles can be updated without uploading new
|
||||
# patchsets. This test exercises such changes.
|
||||
|
||||
@ -4627,167 +4380,8 @@ class TestGithubAppCircularDependencies(ZuulGithubAppTestCase):
|
||||
expected_cycle = {c.number for c in bundles[x]}
|
||||
self.assertEqual(expected_cycle, cycle)
|
||||
|
||||
@skip("Disabled due to safety check")
|
||||
@simple_layout('layouts/dependency_removal_check.yaml', driver='github')
|
||||
def test_dependency_removal_check_orig(self):
|
||||
# Dependency cycles can be updated without uploading new
|
||||
# patchsets. This test exercises such changes.
|
||||
|
||||
self.executor_server.hold_jobs_in_build = True
|
||||
tenant = self.scheds.first.sched.abide.tenants.get("tenant-one")
|
||||
pipeline = tenant.layout.pipelines["check"]
|
||||
|
||||
A = self.fake_github.openFakePullRequest("gh/project1", "master", "A")
|
||||
B = self.fake_github.openFakePullRequest("gh/project2", "master", "B")
|
||||
C = self.fake_github.openFakePullRequest("gh/project3", "master", "C")
|
||||
D = self.fake_github.openFakePullRequest("gh/project", "master", "D")
|
||||
E = self.fake_github.openFakePullRequest("gh/project", "master", "E")
|
||||
|
||||
# Because we call setConfiguration immediately upon removing a
|
||||
# change, in order to fully exercise setConfiguration on a
|
||||
# queue that has been reloaded from ZK with potentially stale
|
||||
# bundle data, we need to remove a change from the bundle
|
||||
# twice. ABC is the main bundle we're interested in. E is a
|
||||
# sacrificial change to force a bundle update when it's
|
||||
# removed.
|
||||
|
||||
# E->C, A->E, B->A, C->B
|
||||
E.body = "{}\n\nDepends-On: {}\n".format(
|
||||
E.subject, C.url
|
||||
)
|
||||
A.body = "{}\n\nDepends-On: {}\n".format(
|
||||
A.subject, E.url
|
||||
)
|
||||
B.body = "{}\n\nDepends-On: {}\n".format(
|
||||
B.subject, A.url
|
||||
)
|
||||
C.body = "{}\n\nDepends-On: {}\n".format(
|
||||
C.subject, B.url
|
||||
)
|
||||
|
||||
self.fake_github.emitEvent(C.getPullRequestOpenedEvent())
|
||||
self.waitUntilSettled()
|
||||
abce = [A, B, C, E]
|
||||
self.assertEqual(len(pipeline.queues), 1)
|
||||
self.assertQueueCycles(pipeline, 0, [abce, abce, abce, abce])
|
||||
|
||||
# Now we remove the dependency on E.
|
||||
|
||||
# A->C, B->A, C->B
|
||||
A.body = "{}\n\nDepends-On: {}\n".format(
|
||||
A.subject, C.url
|
||||
)
|
||||
self.fake_github.emitEvent(A.getPullRequestEditedEvent(A.body))
|
||||
self.waitUntilSettled()
|
||||
|
||||
abc = [A, B, C]
|
||||
# A,B,C<live>
|
||||
self.assertQueueCycles(pipeline, 0, [abc, abc, abc])
|
||||
# B,C,A<live>
|
||||
self.assertQueueCycles(pipeline, 1, [abc, abc, abc])
|
||||
|
||||
# Now remove all dependencies for the three remaining changes.
|
||||
A.body = A.subject
|
||||
B.body = B.subject
|
||||
C.body = C.subject
|
||||
|
||||
self.fake_github.emitEvent(A.getPullRequestEditedEvent(A.body))
|
||||
self.waitUntilSettled()
|
||||
self.fake_github.emitEvent(B.getPullRequestEditedEvent(B.body))
|
||||
self.waitUntilSettled()
|
||||
self.fake_github.emitEvent(C.getPullRequestEditedEvent(C.body))
|
||||
self.waitUntilSettled()
|
||||
|
||||
# A, B, C individually (not in that order)
|
||||
self.assertEqual(len(pipeline.queues), 3)
|
||||
self.assertQueueCycles(pipeline, 0, [None])
|
||||
self.assertQueueCycles(pipeline, 1, [None])
|
||||
self.assertQueueCycles(pipeline, 2, [None])
|
||||
|
||||
# Remove the first change from the queue by forcing a
|
||||
# dependency on D.
|
||||
A.body = "{}\n\nDepends-On: {}\n".format(
|
||||
A.subject, D.url
|
||||
)
|
||||
self.fake_github.emitEvent(A.getPullRequestEditedEvent(A.body))
|
||||
self.waitUntilSettled()
|
||||
self.assertEqual(len(pipeline.queues), 2)
|
||||
self.assertQueueCycles(pipeline, 0, [None])
|
||||
self.assertQueueCycles(pipeline, 1, [None])
|
||||
|
||||
# Verify that we can put B and C into a bundle.
|
||||
C.body = "{}\n\nDepends-On: {}\n".format(
|
||||
C.subject, B.url
|
||||
)
|
||||
self.fake_github.emitEvent(C.getPullRequestEditedEvent(C.body))
|
||||
self.waitUntilSettled()
|
||||
B.body = "{}\n\nDepends-On: {}\n".format(
|
||||
B.subject, C.url
|
||||
)
|
||||
self.fake_github.emitEvent(B.getPullRequestEditedEvent(B.body))
|
||||
self.waitUntilSettled()
|
||||
self.assertEqual(len(pipeline.queues), 2)
|
||||
bc = [B, C]
|
||||
self.assertQueueCycles(pipeline, 0, [bc, bc])
|
||||
self.assertQueueCycles(pipeline, 1, [bc, bc])
|
||||
|
||||
# All done.
|
||||
self.executor_server.hold_jobs_in_build = False
|
||||
self.executor_server.release()
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertHistory([
|
||||
dict(name="common-job", result="ABORTED",
|
||||
changes=f"{E.number},{E.head_sha} {A.number},{A.head_sha} "
|
||||
f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
dict(name="project3-job", result="ABORTED",
|
||||
changes=f"{E.number},{E.head_sha} {A.number},{A.head_sha} "
|
||||
f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
|
||||
# Relaunched
|
||||
dict(name="common-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha} "
|
||||
f"{C.number},{C.head_sha}"),
|
||||
dict(name="project3-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha} "
|
||||
f"{C.number},{C.head_sha}"),
|
||||
|
||||
dict(name="project1-job", result="ABORTED",
|
||||
changes=f"{B.number},{B.head_sha} {C.number},{C.head_sha} "
|
||||
f"{A.number},{A.head_sha}"),
|
||||
# common-job deduplicated from stanza above
|
||||
|
||||
dict(name="common-job", result="ABORTED",
|
||||
changes=f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
dict(name="project3-job", result="ABORTED",
|
||||
changes=f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
|
||||
dict(name="common-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha}"),
|
||||
dict(name="project1-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha}"),
|
||||
dict(name="common-job", result="ABORTED",
|
||||
changes=f"{B.number},{B.head_sha}"),
|
||||
dict(name="project2-job", result="ABORTED",
|
||||
changes=f"{B.number},{B.head_sha}"),
|
||||
dict(name="common-job", result="ABORTED",
|
||||
changes=f"{C.number},{C.head_sha}"),
|
||||
dict(name="project3-job", result="ABORTED",
|
||||
changes=f"{C.number},{C.head_sha}"),
|
||||
|
||||
dict(name="common-job", result="SUCCESS",
|
||||
changes=f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
dict(name="project3-job", result="SUCCESS",
|
||||
changes=f"{B.number},{B.head_sha} {C.number},{C.head_sha}"),
|
||||
# common-job deduplicated
|
||||
dict(name="project2-job", result="SUCCESS",
|
||||
changes=f"{C.number},{C.head_sha} {B.number},{B.head_sha}"),
|
||||
], ordered=False)
|
||||
|
||||
@simple_layout('layouts/dependency_removal_check.yaml', driver='github')
|
||||
def test_dependency_removal_check(self):
|
||||
# TODO: After removal of the safety check, remove this test
|
||||
# and use the _orig version.
|
||||
# Dependency cycles can be updated without uploading new
|
||||
# patchsets. This test exercises such changes.
|
||||
|
||||
@ -4854,12 +4448,10 @@ class TestGithubAppCircularDependencies(ZuulGithubAppTestCase):
|
||||
self.waitUntilSettled()
|
||||
|
||||
# A, B, C individually
|
||||
# ABC<nonlive> E<live>, A<live>, B<live>, C<live>
|
||||
self.assertEqual(len(pipeline.queues), 4)
|
||||
self.assertQueueCycles(pipeline, 0, [abc, [E]])
|
||||
self.assertQueueCycles(pipeline, 1, [[A]])
|
||||
self.assertQueueCycles(pipeline, 2, [[B]])
|
||||
self.assertQueueCycles(pipeline, 3, [[C]])
|
||||
self.assertEqual(len(pipeline.queues), 3)
|
||||
self.assertQueueCycles(pipeline, 0, [[A], [B]])
|
||||
self.assertQueueCycles(pipeline, 1, [[A], [B], [C]])
|
||||
self.assertQueueCycles(pipeline, 2, [[A]])
|
||||
|
||||
# Verify that we can put B and C into a bundle.
|
||||
C.body = "{}\n\nDepends-On: {}\n".format(
|
||||
@ -4872,93 +4464,24 @@ class TestGithubAppCircularDependencies(ZuulGithubAppTestCase):
|
||||
)
|
||||
self.fake_github.emitEvent(B.getPullRequestEditedEvent(B.body))
|
||||
self.waitUntilSettled()
|
||||
self.assertEqual(len(pipeline.queues), 3)
|
||||
self.assertEqual(len(pipeline.queues), 2)
|
||||
bc = [B, C]
|
||||
self.assertQueueCycles(pipeline, 0, [abc, [E]])
|
||||
self.assertQueueCycles(pipeline, 1, [[A]])
|
||||
self.assertQueueCycles(pipeline, 2, [bc])
|
||||
self.assertQueueCycles(pipeline, 0, [[A]])
|
||||
self.assertQueueCycles(pipeline, 1, [bc])
|
||||
|
||||
# All done.
|
||||
self.executor_server.hold_jobs_in_build = False
|
||||
self.executor_server.release()
|
||||
self.waitUntilSettled()
|
||||
|
||||
for build in self.history[:-7]:
|
||||
for build in self.history[:-5]:
|
||||
self.assertEqual(build.result, 'ABORTED')
|
||||
# Changes A, B, C in the end
|
||||
for build in self.history[-7:]:
|
||||
for build in self.history[-5:]:
|
||||
self.assertEqual(build.result, 'SUCCESS')
|
||||
|
||||
@skip("Disabled due to safety check")
|
||||
@simple_layout('layouts/dependency_removal_check.yaml', driver='github')
|
||||
def test_dependency_addition_check_orig(self):
|
||||
# Dependency cycles can be updated without uploading new
|
||||
# patchsets. This test exercises such changes.
|
||||
|
||||
self.executor_server.hold_jobs_in_build = True
|
||||
tenant = self.scheds.first.sched.abide.tenants.get("tenant-one")
|
||||
pipeline = tenant.layout.pipelines["check"]
|
||||
|
||||
A = self.fake_github.openFakePullRequest("gh/project1", "master", "A")
|
||||
B = self.fake_github.openFakePullRequest("gh/project2", "master", "B")
|
||||
|
||||
self.fake_github.emitEvent(A.getPullRequestOpenedEvent())
|
||||
self.waitUntilSettled()
|
||||
|
||||
expected_cycle = {A.number}
|
||||
self.assertEqual(len(list(pipeline.getAllItems())), 1)
|
||||
for item in pipeline.getAllItems():
|
||||
self.assertIsNone(item.bundle)
|
||||
|
||||
B.body = "{}\n\nDepends-On: {}\n".format(
|
||||
B.subject, A.url
|
||||
)
|
||||
self.fake_github.emitEvent(B.getPullRequestEditedEvent(B.body))
|
||||
self.waitUntilSettled()
|
||||
|
||||
A.body = "{}\n\nDepends-On: {}\n".format(
|
||||
A.subject, B.url
|
||||
)
|
||||
self.fake_github.emitEvent(A.getPullRequestEditedEvent(A.body))
|
||||
self.waitUntilSettled()
|
||||
self.assertEqual(len(list(pipeline.getAllItems())), 4)
|
||||
self.assertEqual(len(pipeline.queues), 2)
|
||||
ab = [A, B]
|
||||
self.assertQueueCycles(pipeline, 0, [ab, ab])
|
||||
self.assertQueueCycles(pipeline, 1, [ab, ab])
|
||||
|
||||
expected_cycle = {A.number, B.number}
|
||||
for item in pipeline.getAllItems():
|
||||
cycle = {i.change.number for i in item.bundle.items}
|
||||
self.assertEqual(expected_cycle, cycle)
|
||||
|
||||
# All done.
|
||||
self.executor_server.hold_jobs_in_build = False
|
||||
self.executor_server.release()
|
||||
self.waitUntilSettled()
|
||||
|
||||
self.assertHistory([
|
||||
# Change 1 first enqueue
|
||||
dict(name="project1-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha}"),
|
||||
dict(name="common-job", result="ABORTED",
|
||||
changes=f"{A.number},{A.head_sha}"),
|
||||
|
||||
# Change 1 bundle update
|
||||
dict(name="project1-job", result="SUCCESS",
|
||||
changes=f"{B.number},{B.head_sha} {A.number},{A.head_sha}"),
|
||||
dict(name="common-job", result="SUCCESS",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha}"),
|
||||
|
||||
# Change 2 bundle update
|
||||
dict(name="project2-job", result="SUCCESS",
|
||||
changes=f"{A.number},{A.head_sha} {B.number},{B.head_sha}"),
|
||||
], ordered=False)
|
||||
|
||||
@simple_layout('layouts/dependency_removal_check.yaml', driver='github')
|
||||
def test_dependency_addition_check(self):
|
||||
# TODO: After removal of the safety check, remove this test
|
||||
# and use the _orig version.
|
||||
# Dependency cycles can be updated without uploading new
|
||||
# patchsets. This test exercises such changes.
|
||||
|
||||
|
||||
@ -28,7 +28,7 @@ from zuul.lib.tarjan import strongly_connected_components
|
||||
import zuul.lib.tracing as tracing
|
||||
from zuul.model import (
|
||||
Change, PipelineState, PipelineChangeList, QueueItem,
|
||||
filter_severity
|
||||
filter_severity, EnqueueEvent
|
||||
)
|
||||
from zuul.zk.change_cache import ChangeKey
|
||||
from zuul.zk.exceptions import LockException
|
||||
@ -445,23 +445,29 @@ class PipelineManager(metaclass=ABCMeta):
|
||||
return None
|
||||
|
||||
def findOldVersionOfChangeAlreadyInQueue(self, change):
|
||||
# Return the item and the old version of the change
|
||||
for item in self.pipeline.getAllItems():
|
||||
if not item.live:
|
||||
continue
|
||||
for item_change in item.changes:
|
||||
if change.isUpdateOf(item_change):
|
||||
return item
|
||||
return None
|
||||
return item, item_change
|
||||
return None, None
|
||||
|
||||
def removeOldVersionsOfChange(self, change, event):
|
||||
if not self.pipeline.dequeue_on_new_patchset:
|
||||
return
|
||||
old_item = self.findOldVersionOfChangeAlreadyInQueue(change)
|
||||
old_item, old_change = self.findOldVersionOfChangeAlreadyInQueue(
|
||||
change)
|
||||
if old_item:
|
||||
log = get_annotated_logger(self.log, event)
|
||||
log.debug("Change %s is a new version, removing %s",
|
||||
change, old_item)
|
||||
self.removeItem(old_item)
|
||||
if old_item.live:
|
||||
changes = old_item.changes[:]
|
||||
changes.remove(old_change)
|
||||
self.reEnqueueChanges(old_item, changes)
|
||||
|
||||
def removeAbandonedChange(self, change, event):
|
||||
log = get_annotated_logger(self.log, event)
|
||||
@ -481,44 +487,16 @@ class PipelineManager(metaclass=ABCMeta):
|
||||
pass
|
||||
self.removeItem(item)
|
||||
|
||||
def reEnqueueIfDepsPresent(self, item, needs_changes, log,
|
||||
skip_presence_check=True):
|
||||
# This item is about to be dequeued because it's missing
|
||||
# changes. Try to re-enqueue it before dequeing.
|
||||
# Return whether the dequeue should be quiet.
|
||||
if not item.live:
|
||||
return False
|
||||
if not all(self.isChangeAlreadyInPipeline(c) for c in needs_changes):
|
||||
return False
|
||||
# We enqueue only the first change in the item, presuming
|
||||
# that the remaining changes will be pulled in as
|
||||
# appropriate. Because we skip the presence check, we
|
||||
# can't enqueue all of the items changes directly since we
|
||||
# would end up with new items for every change. We only
|
||||
# want one new item.
|
||||
change = item.changes[0]
|
||||
# Check if there is another live item with the change already.
|
||||
for other_item in self.pipeline.getAllItems():
|
||||
if other_item is item:
|
||||
continue
|
||||
if not other_item.live:
|
||||
continue
|
||||
for item_change in other_item.changes:
|
||||
if item_change.equals(change):
|
||||
return True
|
||||
# Try enqueue, if that succeeds, keep this dequeue quiet
|
||||
try:
|
||||
log.info("Attempting re-enqueue of %s", item)
|
||||
return self.addChange(
|
||||
change, item.event,
|
||||
enqueue_time=item.enqueue_time,
|
||||
quiet=True,
|
||||
skip_presence_check=skip_presence_check)
|
||||
except Exception:
|
||||
log.exception("Unable to re-enqueue %s "
|
||||
"which is missing dependencies",
|
||||
item)
|
||||
return False
|
||||
def reEnqueueChanges(self, item, changes):
|
||||
for change in changes:
|
||||
event = EnqueueEvent(self.pipeline.tenant.name,
|
||||
self.pipeline.name,
|
||||
change.project.canonical_hostname,
|
||||
change.project.name,
|
||||
change=change._id())
|
||||
event.zuul_event_id = item.event.zuul_event_id
|
||||
self.sched.pipeline_management_events[
|
||||
self.pipeline.tenant.name][self.pipeline.name].put(event)
|
||||
|
||||
@abstractmethod
|
||||
def getChangeQueue(self, change, event, existing=None):
|
||||
@ -1644,6 +1622,20 @@ class PipelineManager(metaclass=ABCMeta):
|
||||
dependency_graph = collections.OrderedDict()
|
||||
self.getDependencyGraph(item.changes[0], dependency_graph, item.event,
|
||||
quiet=True)
|
||||
|
||||
# Verify that the cycle dependency graph is correct
|
||||
cycle = self.cycleForChange(
|
||||
item.changes[0], dependency_graph, item.event, debug=False)
|
||||
cycle = cycle or [item.changes[0]]
|
||||
item_cycle = set(item.changes)
|
||||
if set(cycle) != item_cycle:
|
||||
log.info("Item cycle has changed: %s, now: %s, was: %s", item,
|
||||
set(cycle), item_cycle)
|
||||
self.removeItem(item)
|
||||
if item.live:
|
||||
self.reEnqueueChanges(item, item.changes)
|
||||
return (True, nnfi)
|
||||
|
||||
abort, needs_changes = self.getMissingNeededChanges(
|
||||
item.changes, change_queue, item.event,
|
||||
dependency_graph=dependency_graph)
|
||||
@ -1652,7 +1644,6 @@ class PipelineManager(metaclass=ABCMeta):
|
||||
log.info("Dequeuing %s because "
|
||||
"it can no longer merge" % item)
|
||||
self.cancelJobs(item)
|
||||
quiet_dequeue = False
|
||||
if not meets_reqs:
|
||||
item.setDequeuedMissingRequirements()
|
||||
else:
|
||||
@ -1662,14 +1653,7 @@ class PipelineManager(metaclass=ABCMeta):
|
||||
else:
|
||||
msg = f'Change {clist} is needed.'
|
||||
item.setDequeuedNeedingChange(msg)
|
||||
# If all the dependencies are already in the pipeline
|
||||
# (but not ahead of this change), then we probably
|
||||
# just added updated versions of them, possibly
|
||||
# updating a cycle. In that case, attempt to
|
||||
# re-enqueue this change with the updated deps.
|
||||
quiet_dequeue = self.reEnqueueIfDepsPresent(
|
||||
item, needs_changes, log)
|
||||
if item.live and not quiet_dequeue:
|
||||
if item.live:
|
||||
try:
|
||||
self.reportItem(item)
|
||||
except exceptions.MergeFailure:
|
||||
@ -1677,20 +1661,6 @@ class PipelineManager(metaclass=ABCMeta):
|
||||
self.dequeueItem(item)
|
||||
return (True, nnfi)
|
||||
|
||||
# Safety check: verify that the cycle dependency graph is correct
|
||||
cycle = self.cycleForChange(
|
||||
item.changes[0], dependency_graph, item.event, debug=False)
|
||||
cycle = cycle or [item.changes[0]]
|
||||
item_cycle = set(item.changes)
|
||||
if (item.live and set(cycle) != item_cycle):
|
||||
log.info("Item cycle has changed: %s, now: %s, was: %s", item,
|
||||
set(cycle), item_cycle)
|
||||
self.removeItem(item)
|
||||
if item.live:
|
||||
self.reEnqueueIfDepsPresent(item, needs_changes, log,
|
||||
skip_presence_check=False)
|
||||
return (True, nnfi)
|
||||
|
||||
actionable = change_queue.isActionable(item)
|
||||
item.updateAttributes(self.current_context, active=actionable)
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user