Fix duplicate and reversed artifacts

The "current" provides/requires test (which is supposed to test
live changes) was inadvertently following the SQL code path in
some cases.  The test has been updated to include a third job
which continues running until the end of the test to ensure that
the changes remain in the pipeline the entire time.

It, and the SQL version, have been extended with one more change
to illustrate a bug where artifact data were being included twice.
This was because the data collection method has two recursion points:
a) if it is called on a non-live change and it finds a live version
   of the same change, it recursively calls itself on the non-live
   version of the same change.  Essentially, it "switches" to that
   version of the change and continues.
b) at the end of normal processing of a change, it recurses "up"
   the queue to the next change ahead.

The (a) case did not return in some cases, so in those cases, it
would "switch" to a change, add its data, recurse up the queue,
then, after unrolling back to the point of the switch, would then
recurse up the queue again in case (b).  The solution is to avoid
recursing up the queue in the (a) case and allow it only in the (b)
case -- that is, we only walk up the queue of dependent changes for
the original change.

Finally, the artifacts were being returned in reverse order -- it
is more useful for them to be returned such that the latest version
of a given artifact is returned last so that competing versions
are overwritten in the correct order.

Change-Id: I41ac336c1bc776609645e3662003f2df076dd1d5
This commit is contained in:
James E. Blair 2019-03-06 14:39:51 -08:00
parent 967828b1f0
commit bd7a5d47bf
3 changed files with 116 additions and 10 deletions

View File

@ -57,12 +57,16 @@
name: library-user
requires: libraries
- job:
name: hold
- project:
name: org/project1
check:
jobs:
- image-builder
- library-builder
- hold
gate:
queue: integrated
jobs:
@ -74,6 +78,7 @@
jobs:
- image-user
- library-user
- hold
gate:
queue: integrated
jobs:

View File

@ -4977,16 +4977,41 @@ class TestProvidesRequires(ZuulDBTestCase):
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertEqual(len(self.builds), 2)
self.assertEqual(len(self.builds), 3)
B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B')
B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
B.subject, A.data['id'])
self.executor_server.returnData(
'image-builder', B,
{'zuul':
{'artifacts': [
{'name': 'image2', 'url': 'http://example.com/image2'},
]}}
)
self.executor_server.returnData(
'library-builder', B,
{'zuul':
{'artifacts': [
{'name': 'library2', 'url': 'http://example.com/library2'},
]}}
)
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertEqual(len(self.builds), 2)
self.assertEqual(len(self.builds), 6)
C = self.fake_gerrit.addFakeChange('org/project2', 'master', 'C')
C.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
C.subject, B.data['id'])
self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertEqual(len(self.builds), 7)
self.executor_server.release('image-*')
self.executor_server.release('library-*')
self.waitUntilSettled()
self.executor_server.hold_jobs_in_build = False
self.executor_server.release()
self.waitUntilSettled()
@ -4994,8 +5019,14 @@ class TestProvidesRequires(ZuulDBTestCase):
self.assertHistory([
dict(name='image-builder', result='SUCCESS', changes='1,1'),
dict(name='library-builder', result='SUCCESS', changes='1,1'),
dict(name='image-user', result='SUCCESS', changes='1,1 2,1'),
dict(name='library-user', result='SUCCESS', changes='1,1 2,1'),
dict(name='hold', result='SUCCESS', changes='1,1'),
dict(name='image-builder', result='SUCCESS', changes='1,1 2,1'),
dict(name='library-builder', result='SUCCESS', changes='1,1 2,1'),
dict(name='hold', result='SUCCESS', changes='1,1 2,1'),
dict(name='image-user', result='SUCCESS', changes='1,1 2,1 3,1'),
dict(name='library-user', result='SUCCESS',
changes='1,1 2,1 3,1'),
dict(name='hold', result='SUCCESS', changes='1,1 2,1 3,1'),
], ordered=False)
image_user = self.getJobFromHistory('image-user')
self.assertEqual(
@ -5007,6 +5038,13 @@ class TestProvidesRequires(ZuulDBTestCase):
'job': 'image-builder',
'url': 'http://example.com/image',
'name': 'image',
}, {
'project': 'org/project1',
'change': '2',
'patchset': '1',
'job': 'image-builder',
'url': 'http://example.com/image2',
'name': 'image2',
}])
library_user = self.getJobFromHistory('library-user')
self.assertEqual(
@ -5018,6 +5056,13 @@ class TestProvidesRequires(ZuulDBTestCase):
'job': 'library-builder',
'url': 'http://example.com/library',
'name': 'library',
}, {
'project': 'org/project1',
'change': '2',
'patchset': '1',
'job': 'library-builder',
'url': 'http://example.com/library2',
'name': 'library2',
}])
@simple_layout('layouts/provides-requires.yaml')
@ -5042,19 +5087,54 @@ class TestProvidesRequires(ZuulDBTestCase):
self.assertHistory([
dict(name='image-builder', result='SUCCESS', changes='1,1'),
dict(name='library-builder', result='SUCCESS', changes='1,1'),
dict(name='hold', result='SUCCESS', changes='1,1'),
], ordered=False)
B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B')
B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
B.subject, A.data['id'])
self.executor_server.returnData(
'image-builder', B,
{'zuul':
{'artifacts': [
{'name': 'image2', 'url': 'http://example.com/image2'},
]}}
)
self.executor_server.returnData(
'library-builder', B,
{'zuul':
{'artifacts': [
{'name': 'library2', 'url': 'http://example.com/library2'},
]}}
)
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertHistory([
dict(name='image-builder', result='SUCCESS', changes='1,1'),
dict(name='library-builder', result='SUCCESS', changes='1,1'),
dict(name='hold', result='SUCCESS', changes='1,1'),
dict(name='image-builder', result='SUCCESS', changes='1,1 2,1'),
dict(name='library-builder', result='SUCCESS', changes='1,1 2,1'),
dict(name='hold', result='SUCCESS', changes='1,1 2,1'),
], ordered=False)
C = self.fake_gerrit.addFakeChange('org/project2', 'master', 'C')
C.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
C.subject, B.data['id'])
self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertHistory([
dict(name='image-builder', result='SUCCESS', changes='1,1'),
dict(name='library-builder', result='SUCCESS', changes='1,1'),
dict(name='image-user', result='SUCCESS', changes='1,1 2,1'),
dict(name='library-user', result='SUCCESS', changes='1,1 2,1'),
dict(name='hold', result='SUCCESS', changes='1,1'),
dict(name='image-builder', result='SUCCESS', changes='1,1 2,1'),
dict(name='library-builder', result='SUCCESS', changes='1,1 2,1'),
dict(name='hold', result='SUCCESS', changes='1,1 2,1'),
dict(name='image-user', result='SUCCESS', changes='1,1 2,1 3,1'),
dict(name='library-user', result='SUCCESS',
changes='1,1 2,1 3,1'),
dict(name='hold', result='SUCCESS', changes='1,1 2,1 3,1'),
], ordered=False)
image_user = self.getJobFromHistory('image-user')
self.assertEqual(
@ -5066,6 +5146,13 @@ class TestProvidesRequires(ZuulDBTestCase):
'job': 'image-builder',
'url': 'http://example.com/image',
'name': 'image',
}, {
'project': 'org/project1',
'change': '2',
'patchset': '1',
'job': 'image-builder',
'url': 'http://example.com/image2',
'name': 'image2',
}])
library_user = self.getJobFromHistory('library-user')
self.assertEqual(
@ -5077,6 +5164,13 @@ class TestProvidesRequires(ZuulDBTestCase):
'job': 'library-builder',
'url': 'http://example.com/library',
'name': 'library',
}, {
'project': 'org/project1',
'change': '2',
'patchset': '1',
'job': 'library-builder',
'url': 'http://example.com/library2',
'name': 'library2',
}])
@simple_layout('layouts/provides-requires.yaml')
@ -5090,6 +5184,7 @@ class TestProvidesRequires(ZuulDBTestCase):
self.assertHistory([
dict(name='image-builder', result='FAILURE', changes='1,1'),
dict(name='library-builder', result='FAILURE', changes='1,1'),
dict(name='hold', result='SUCCESS', changes='1,1'),
], ordered=False)
B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
@ -5101,6 +5196,8 @@ class TestProvidesRequires(ZuulDBTestCase):
self.assertHistory([
dict(name='image-builder', result='FAILURE', changes='1,1'),
dict(name='library-builder', result='FAILURE', changes='1,1'),
dict(name='hold', result='SUCCESS', changes='1,1'),
dict(name='hold', result='SUCCESS', changes='1,1 2,1'),
], ordered=False)
self.assertIn('image-user : SKIPPED', B.messages[0])
self.assertIn('not met by build', B.messages[0])

View File

@ -2257,7 +2257,7 @@ class QueueItem(object):
data.append(artifact)
return data
def providesRequirements(self, requirements, data):
def providesRequirements(self, requirements, data, recurse=True):
# Mutates data and returns true/false if requirements
# satisfied.
if not requirements:
@ -2271,7 +2271,8 @@ class QueueItem(object):
found = True
break
if found:
if not item.providesRequirements(requirements, data):
if not item.providesRequirements(requirements, data,
recurse=False):
return False
else:
# Look for this item in the SQL DB.
@ -2299,6 +2300,8 @@ class QueueItem(object):
data += artifacts
if not self.item_ahead:
return True
if not recurse:
return True
return self.item_ahead.providesRequirements(requirements, data)
def jobRequirementsReady(self, job):
@ -2307,6 +2310,7 @@ class QueueItem(object):
try:
data = []
ret = self.item_ahead.providesRequirements(job.requires, data)
data.reverse()
job.updateArtifactData(data)
except RequirementsError as e:
self.warning(str(e))