zuul
/
zuul
Code Issues Proposed changes

Fix NNFI bug with two failing changes at head 

If A is the head in A <- B <- C, and B failed, then C would be
correctly reparented to A.  Then if A failed, B and C would be
restarted, but C would not be reparented back to B.  This is
because the check around moving a change short-circuited if
there was no change ahead (which is the case if C is behind A
and A reports).

The solution to this is to still perform the move check even if
there is no change currently ahead (so that if there is a NNFI
change ahead, the current change will be moved behind it).  This
effectively means we should remove the "not item ahead" part of
the conditional around the move.

This part of the conditional serves two additional purposes --
to make sure that we don't dereference an attribute on item_ahead
if it is None, and also to ensure that the NNFI algorithm is not
applied to independent queues.

So the fix moves that part of the conditional out so that we can
safely reference the needed attributes if there is a change ahead,
and also makes explicit that we ignore the situation if we are
working on an independent change queue.

This also adds a test that failed (at the indicated position) with
the previous code.

Change-Id: I4cf5e868af7cddb7e95ef378abb966613ac9701c
changes/98/47898/1
James E. Blair 2013-09-23 11:15:47 -07:00
parent 0045126c38
commit fef7163d52
2 changed files with 110 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

105
tests/test_scheduler.py
View File

@ -1501,6 +1501,111 @@ class TestScheduler(testtools.TestCase):
self.assertEqual(B.reported, 2)
self.assertEqual(C.reported, 2)
def test_two_failed_changes_at_head(self):
"Test that changes are reparented correctly if 2 fail at head"
self.worker.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/project', 'master', 'C')
A.addApproval('CRVW', 2)
B.addApproval('CRVW', 2)
C.addApproval('CRVW', 2)
self.worker.addFailTest('project-test1', A)
self.worker.addFailTest('project-test1', B)
self.fake_gerrit.addEvent(A.addApproval('APRV', 1))
self.fake_gerrit.addEvent(B.addApproval('APRV', 1))
self.fake_gerrit.addEvent(C.addApproval('APRV', 1))
self.waitUntilSettled()
self.worker.release('.*-merge')
self.waitUntilSettled()
self.worker.release('.*-merge')
self.waitUntilSettled()
self.worker.release('.*-merge')
self.waitUntilSettled()
self.assertEqual(len(self.builds), 6)
self.assertEqual(self.builds[0].name, 'project-test1')
self.assertEqual(self.builds[1].name, 'project-test2')
self.assertEqual(self.builds[2].name, 'project-test1')
self.assertEqual(self.builds[3].name, 'project-test2')
self.assertEqual(self.builds[4].name, 'project-test1')
self.assertEqual(self.builds[5].name, 'project-test2')
self.assertTrue(self.job_has_changes(self.builds[0], A))
self.assertTrue(self.job_has_changes(self.builds[2], A))
self.assertTrue(self.job_has_changes(self.builds[2], B))
self.assertTrue(self.job_has_changes(self.builds[4], A))
self.assertTrue(self.job_has_changes(self.builds[4], B))
self.assertTrue(self.job_has_changes(self.builds[4], C))
# Fail change B first
self.release(self.builds[2])
self.waitUntilSettled()
# restart of C after B failure
self.worker.release('.*-merge')
self.waitUntilSettled()
self.assertEqual(len(self.builds), 5)
self.assertEqual(self.builds[0].name, 'project-test1')
self.assertEqual(self.builds[1].name, 'project-test2')
self.assertEqual(self.builds[2].name, 'project-test2')
self.assertEqual(self.builds[3].name, 'project-test1')
self.assertEqual(self.builds[4].name, 'project-test2')
self.assertTrue(self.job_has_changes(self.builds[1], A))
self.assertTrue(self.job_has_changes(self.builds[2], A))
self.assertTrue(self.job_has_changes(self.builds[2], B))
self.assertTrue(self.job_has_changes(self.builds[4], A))
self.assertFalse(self.job_has_changes(self.builds[4], B))
self.assertTrue(self.job_has_changes(self.builds[4], C))
# Finish running all passing jobs for change A
self.release(self.builds[1])
self.waitUntilSettled()
# Fail and report change A
self.release(self.builds[0])
self.waitUntilSettled()
# restart of B,C after A failure
self.worker.release('.*-merge')
self.waitUntilSettled()
self.worker.release('.*-merge')
self.waitUntilSettled()
self.assertEqual(len(self.builds), 4)
self.assertEqual(self.builds[0].name, 'project-test1') # B
self.assertEqual(self.builds[1].name, 'project-test2') # B
self.assertEqual(self.builds[2].name, 'project-test1') # C
self.assertEqual(self.builds[3].name, 'project-test2') # C
self.assertFalse(self.job_has_changes(self.builds[1], A))
self.assertTrue(self.job_has_changes(self.builds[1], B))
self.assertFalse(self.job_has_changes(self.builds[1], C))
self.assertFalse(self.job_has_changes(self.builds[2], A))
# After A failed and B and C restarted, B should be back in
# C's tests because it has not failed yet.
self.assertTrue(self.job_has_changes(self.builds[2], B))
self.assertTrue(self.job_has_changes(self.builds[2], C))
self.worker.hold_jobs_in_build = False
self.worker.release()
self.waitUntilSettled()
self.assertEqual(len(self.builds), 0)
self.assertEqual(len(self.history), 21)
self.assertEqual(A.data['status'], 'NEW')
self.assertEqual(B.data['status'], 'NEW')
self.assertEqual(C.data['status'], 'MERGED')
self.assertEqual(A.reported, 2)
self.assertEqual(B.reported, 2)
self.assertEqual(C.reported, 2)
def test_patch_order(self):
"Test that dependent patches are tested in the right order"
A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')

7
zuul/scheduler.py
View File

@ -982,8 +982,11 @@ class BasePipelineManager(object):
failing_reasons.append('a needed change is failing')
self.cancelJobs(item, prime=False)
else:
if (item_ahead and item_ahead != nnfi and
not item_ahead.change.is_merged):
item_ahead_merged = False
if ((item_ahead and item_ahead.change.is_merged) or
not change_queue.dependent):
item_ahead_merged = True
if (item_ahead != nnfi and not item_ahead_merged):
# Our current base is different than what we expected,
# and it's not because our current base merged. Something
# ahead must have failed.