From a86134f5287a65b0eede090dc8564f210c5a13f8 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Tue, 16 Jan 2018 13:32:14 -0800 Subject: Fix dependency cycle false positive This corrects a false-positive in the dependency cycle detection, but only for the new URL-style depends-on headers. It does not do so for the legacy gerrit headers. We used a single history list to store all the changes we enqueued ahead of a given change, but this meant that if there was more than one path to a change, we would see it in the history on the second traversal. Instead, when traversing the tree, use copies of the history list at each stage so that it can be rewound when going back up the tree. The second path to a change will not trip the cycle detection, and will proceed on to the point where it notices the change is already in the queue and return harmlessly. Also, check whether the exact change is in the history, not just the number, since numbers are no longer unique with multiple sources. Also, fix a bug in the test_crd_cycle test which was causing the test to always pass since the changes were never enqueued due to missing approval requirements. Change-Id: I3241f90a1d7469d433cfa176e719322203d4d089 Story: 2001427 Task: 6133 --- tests/unit/test_gerrit_crd.py | 59 ++++++++++++++++++++++++++++++++++++ tests/unit/test_gerrit_legacy_crd.py | 1 + zuul/manager/dependent.py | 7 +++-- zuul/manager/independent.py | 4 +-- 4 files changed, 67 insertions(+), 4 deletions(-) diff --git a/tests/unit/test_gerrit_crd.py b/tests/unit/test_gerrit_crd.py index 732bc3d60..a8924b902 100644 --- a/tests/unit/test_gerrit_crd.py +++ b/tests/unit/test_gerrit_crd.py @@ -90,6 +90,40 @@ class TestGerritCRD(ZuulTestCase): 'project-merge', 'org/project1').changes self.assertEqual(changes, '2,1 1,1') + def test_crd_gate_triangle(self): + A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A') + B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B') + C = self.fake_gerrit.addFakeChange('org/project2', 'master', 'C') + A.addApproval('Code-Review', 2) + B.addApproval('Code-Review', 2) + C.addApproval('Code-Review', 2) + A.addApproval('Approved', 1) + B.addApproval('Approved', 1) + + # C-->B + # \ / + # v + # A + + # C Depends-On: A + C.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % ( + C.subject, A.data['url']) + # B Depends-On: A + B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % ( + B.subject, A.data['url']) + # C git-depends on B + C.setDependsOn(B, 1) + self.fake_gerrit.addEvent(C.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'], 'MERGED') + self.assertEqual(B.data['status'], 'MERGED') + self.assertEqual(C.data['status'], 'MERGED') + self.assertEqual(self.history[-1].changes, '1,1 2,1 3,1') + def test_crd_branch(self): "Test cross-repo dependencies in multiple branches" @@ -257,6 +291,7 @@ class TestGerritCRD(ZuulTestCase): B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B') A.addApproval('Code-Review', 2) B.addApproval('Code-Review', 2) + B.addApproval('Approved', 1) # A -> B -> A (via commit-depends) @@ -522,6 +557,30 @@ class TestGerritCRD(ZuulTestCase): for job in self.history: self.assertEqual(len(job.changes.split()), 1) + def test_crd_check_triangle(self): + A = self.fake_gerrit.addFakeChange('org/project1', 'master', 'A') + B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B') + C = self.fake_gerrit.addFakeChange('org/project2', 'master', 'C') + + # C-->B + # \ / + # v + # A + + # C Depends-On: A + C.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % ( + C.subject, A.data['url']) + # B Depends-On: A + B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % ( + B.subject, A.data['url']) + # C git-depends on B + C.setDependsOn(B, 1) + self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1)) + self.waitUntilSettled() + + self.assertEqual(C.reported, 1) + self.assertEqual(self.history[0].changes, '1,1 2,1 3,1') + @simple_layout('layouts/three-projects.yaml') def test_crd_check_transitive(self): "Test transitive cross-repo dependencies" diff --git a/tests/unit/test_gerrit_legacy_crd.py b/tests/unit/test_gerrit_legacy_crd.py index c711e4d95..90c93ecae 100644 --- a/tests/unit/test_gerrit_legacy_crd.py +++ b/tests/unit/test_gerrit_legacy_crd.py @@ -260,6 +260,7 @@ class TestGerritLegacyCRD(ZuulTestCase): B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B') A.addApproval('Code-Review', 2) B.addApproval('Code-Review', 2) + B.addApproval('Approved', 1) # A -> B -> A (via commit-depends) diff --git a/zuul/manager/dependent.py b/zuul/manager/dependent.py index 20b376d6a..5ad761196 100644 --- a/zuul/manager/dependent.py +++ b/zuul/manager/dependent.py @@ -141,13 +141,16 @@ class DependentPipelineManager(PipelineManager): def enqueueChangesAhead(self, change, quiet, ignore_requirements, change_queue, history=None): - if history and change.number in history: + if history and change in history: # detected dependency cycle self.log.warn("Dependency cycle detected") return False if hasattr(change, 'number'): history = history or [] - history.append(change.number) + history = history + [change] + else: + # Don't enqueue dependencies ahead of a non-change ref. + return True ret = self.checkForChangesNeededBy(change, change_queue) if ret in [True, False]: diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py index 0c2baf010..9da40d582 100644 --- a/zuul/manager/independent.py +++ b/zuul/manager/independent.py @@ -34,13 +34,13 @@ class IndependentPipelineManager(PipelineManager): def enqueueChangesAhead(self, change, quiet, ignore_requirements, change_queue, history=None): - if history and change.number in history: + if history and change in history: # detected dependency cycle self.log.warn("Dependency cycle detected") return False if hasattr(change, 'number'): history = history or [] - history.append(change.number) + history = history + [change] else: # Don't enqueue dependencies ahead of a non-change ref. return True -- cgit v1.2.1