summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJames E. Blair <jeblair@redhat.com>2019-03-06 14:39:51 -0800
committerJames E. Blair <jeblair@redhat.com>2019-03-06 15:27:26 -0800
commitbd7a5d47bf4b33f05aabd93bbf108cc24e627177 (patch)
treedd2e4ee41e3f955d53eac4d522c56d21d09b1eec
parent967828b1f05a2676b1c06714add052108a9f3102 (diff)
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
Notes
Notes (review): Code-Review+2: Jeremy Stanley <fungi@yuggoth.org> Code-Review+2: Clark Boylan <cboylan@sapwetik.org> Workflow+1: Clark Boylan <cboylan@sapwetik.org> Verified+2: Zuul Submitted-by: Zuul Submitted-at: Thu, 07 Mar 2019 00:49:02 +0000 Reviewed-on: https://review.openstack.org/641508 Project: openstack-infra/zuul Branch: refs/heads/master
-rw-r--r--tests/fixtures/layouts/provides-requires.yaml5
-rw-r--r--tests/unit/test_v3.py113
-rw-r--r--zuul/model.py8
3 files changed, 116 insertions, 10 deletions
diff --git a/tests/fixtures/layouts/provides-requires.yaml b/tests/fixtures/layouts/provides-requires.yaml
index c71c49c..b60dd16 100644
--- a/tests/fixtures/layouts/provides-requires.yaml
+++ b/tests/fixtures/layouts/provides-requires.yaml
@@ -57,12 +57,16 @@
57 name: library-user 57 name: library-user
58 requires: libraries 58 requires: libraries
59 59
60- job:
61 name: hold
62
60- project: 63- project:
61 name: org/project1 64 name: org/project1
62 check: 65 check:
63 jobs: 66 jobs:
64 - image-builder 67 - image-builder
65 - library-builder 68 - library-builder
69 - hold
66 gate: 70 gate:
67 queue: integrated 71 queue: integrated
68 jobs: 72 jobs:
@@ -74,6 +78,7 @@
74 jobs: 78 jobs:
75 - image-user 79 - image-user
76 - library-user 80 - library-user
81 - hold
77 gate: 82 gate:
78 queue: integrated 83 queue: integrated
79 jobs: 84 jobs:
diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py
index 0f697e3..5c21ce1 100644
--- a/tests/unit/test_v3.py
+++ b/tests/unit/test_v3.py
@@ -4977,16 +4977,41 @@ class TestProvidesRequires(ZuulDBTestCase):
4977 self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1)) 4977 self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
4978 self.waitUntilSettled() 4978 self.waitUntilSettled()
4979 4979
4980 self.assertEqual(len(self.builds), 2) 4980 self.assertEqual(len(self.builds), 3)
4981 4981
4982 B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B') 4982 B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B')
4983 B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % ( 4983 B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
4984 B.subject, A.data['id']) 4984 B.subject, A.data['id'])
4985 self.executor_server.returnData(
4986 'image-builder', B,
4987 {'zuul':
4988 {'artifacts': [
4989 {'name': 'image2', 'url': 'http://example.com/image2'},
4990 ]}}
4991 )
4992 self.executor_server.returnData(
4993 'library-builder', B,
4994 {'zuul':
4995 {'artifacts': [
4996 {'name': 'library2', 'url': 'http://example.com/library2'},
4997 ]}}
4998 )
4985 self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) 4999 self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
4986 self.waitUntilSettled() 5000 self.waitUntilSettled()
4987 5001
4988 self.assertEqual(len(self.builds), 2) 5002 self.assertEqual(len(self.builds), 6)
4989 5003
5004 C = self.fake_gerrit.addFakeChange('org/project2', 'master', 'C')
5005 C.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
5006 C.subject, B.data['id'])
5007 self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
5008 self.waitUntilSettled()
5009
5010 self.assertEqual(len(self.builds), 7)
5011
5012 self.executor_server.release('image-*')
5013 self.executor_server.release('library-*')
5014 self.waitUntilSettled()
4990 self.executor_server.hold_jobs_in_build = False 5015 self.executor_server.hold_jobs_in_build = False
4991 self.executor_server.release() 5016 self.executor_server.release()
4992 self.waitUntilSettled() 5017 self.waitUntilSettled()
@@ -4994,8 +5019,14 @@ class TestProvidesRequires(ZuulDBTestCase):
4994 self.assertHistory([ 5019 self.assertHistory([
4995 dict(name='image-builder', result='SUCCESS', changes='1,1'), 5020 dict(name='image-builder', result='SUCCESS', changes='1,1'),
4996 dict(name='library-builder', result='SUCCESS', changes='1,1'), 5021 dict(name='library-builder', result='SUCCESS', changes='1,1'),
4997 dict(name='image-user', result='SUCCESS', changes='1,1 2,1'), 5022 dict(name='hold', result='SUCCESS', changes='1,1'),
4998 dict(name='library-user', result='SUCCESS', changes='1,1 2,1'), 5023 dict(name='image-builder', result='SUCCESS', changes='1,1 2,1'),
5024 dict(name='library-builder', result='SUCCESS', changes='1,1 2,1'),
5025 dict(name='hold', result='SUCCESS', changes='1,1 2,1'),
5026 dict(name='image-user', result='SUCCESS', changes='1,1 2,1 3,1'),
5027 dict(name='library-user', result='SUCCESS',
5028 changes='1,1 2,1 3,1'),
5029 dict(name='hold', result='SUCCESS', changes='1,1 2,1 3,1'),
4999 ], ordered=False) 5030 ], ordered=False)
5000 image_user = self.getJobFromHistory('image-user') 5031 image_user = self.getJobFromHistory('image-user')
5001 self.assertEqual( 5032 self.assertEqual(
@@ -5007,6 +5038,13 @@ class TestProvidesRequires(ZuulDBTestCase):
5007 'job': 'image-builder', 5038 'job': 'image-builder',
5008 'url': 'http://example.com/image', 5039 'url': 'http://example.com/image',
5009 'name': 'image', 5040 'name': 'image',
5041 }, {
5042 'project': 'org/project1',
5043 'change': '2',
5044 'patchset': '1',
5045 'job': 'image-builder',
5046 'url': 'http://example.com/image2',
5047 'name': 'image2',
5010 }]) 5048 }])
5011 library_user = self.getJobFromHistory('library-user') 5049 library_user = self.getJobFromHistory('library-user')
5012 self.assertEqual( 5050 self.assertEqual(
@@ -5018,6 +5056,13 @@ class TestProvidesRequires(ZuulDBTestCase):
5018 'job': 'library-builder', 5056 'job': 'library-builder',
5019 'url': 'http://example.com/library', 5057 'url': 'http://example.com/library',
5020 'name': 'library', 5058 'name': 'library',
5059 }, {
5060 'project': 'org/project1',
5061 'change': '2',
5062 'patchset': '1',
5063 'job': 'library-builder',
5064 'url': 'http://example.com/library2',
5065 'name': 'library2',
5021 }]) 5066 }])
5022 5067
5023 @simple_layout('layouts/provides-requires.yaml') 5068 @simple_layout('layouts/provides-requires.yaml')
@@ -5042,19 +5087,54 @@ class TestProvidesRequires(ZuulDBTestCase):
5042 self.assertHistory([ 5087 self.assertHistory([
5043 dict(name='image-builder', result='SUCCESS', changes='1,1'), 5088 dict(name='image-builder', result='SUCCESS', changes='1,1'),
5044 dict(name='library-builder', result='SUCCESS', changes='1,1'), 5089 dict(name='library-builder', result='SUCCESS', changes='1,1'),
5090 dict(name='hold', result='SUCCESS', changes='1,1'),
5045 ], ordered=False) 5091 ], ordered=False)
5046 5092
5047 B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B') 5093 B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B')
5048 B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % ( 5094 B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
5049 B.subject, A.data['id']) 5095 B.subject, A.data['id'])
5096 self.executor_server.returnData(
5097 'image-builder', B,
5098 {'zuul':
5099 {'artifacts': [
5100 {'name': 'image2', 'url': 'http://example.com/image2'},
5101 ]}}
5102 )
5103 self.executor_server.returnData(
5104 'library-builder', B,
5105 {'zuul':
5106 {'artifacts': [
5107 {'name': 'library2', 'url': 'http://example.com/library2'},
5108 ]}}
5109 )
5050 self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1)) 5110 self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
5051 self.waitUntilSettled() 5111 self.waitUntilSettled()
5112 self.assertHistory([
5113 dict(name='image-builder', result='SUCCESS', changes='1,1'),
5114 dict(name='library-builder', result='SUCCESS', changes='1,1'),
5115 dict(name='hold', result='SUCCESS', changes='1,1'),
5116 dict(name='image-builder', result='SUCCESS', changes='1,1 2,1'),
5117 dict(name='library-builder', result='SUCCESS', changes='1,1 2,1'),
5118 dict(name='hold', result='SUCCESS', changes='1,1 2,1'),
5119 ], ordered=False)
5120
5121 C = self.fake_gerrit.addFakeChange('org/project2', 'master', 'C')
5122 C.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
5123 C.subject, B.data['id'])
5124 self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
5125 self.waitUntilSettled()
5052 5126
5053 self.assertHistory([ 5127 self.assertHistory([
5054 dict(name='image-builder', result='SUCCESS', changes='1,1'), 5128 dict(name='image-builder', result='SUCCESS', changes='1,1'),
5055 dict(name='library-builder', result='SUCCESS', changes='1,1'), 5129 dict(name='library-builder', result='SUCCESS', changes='1,1'),
5056 dict(name='image-user', result='SUCCESS', changes='1,1 2,1'), 5130 dict(name='hold', result='SUCCESS', changes='1,1'),
5057 dict(name='library-user', result='SUCCESS', changes='1,1 2,1'), 5131 dict(name='image-builder', result='SUCCESS', changes='1,1 2,1'),
5132 dict(name='library-builder', result='SUCCESS', changes='1,1 2,1'),
5133 dict(name='hold', result='SUCCESS', changes='1,1 2,1'),
5134 dict(name='image-user', result='SUCCESS', changes='1,1 2,1 3,1'),
5135 dict(name='library-user', result='SUCCESS',
5136 changes='1,1 2,1 3,1'),
5137 dict(name='hold', result='SUCCESS', changes='1,1 2,1 3,1'),
5058 ], ordered=False) 5138 ], ordered=False)
5059 image_user = self.getJobFromHistory('image-user') 5139 image_user = self.getJobFromHistory('image-user')
5060 self.assertEqual( 5140 self.assertEqual(
@@ -5066,6 +5146,13 @@ class TestProvidesRequires(ZuulDBTestCase):
5066 'job': 'image-builder', 5146 'job': 'image-builder',
5067 'url': 'http://example.com/image', 5147 'url': 'http://example.com/image',
5068 'name': 'image', 5148 'name': 'image',
5149 }, {
5150 'project': 'org/project1',
5151 'change': '2',
5152 'patchset': '1',
5153 'job': 'image-builder',
5154 'url': 'http://example.com/image2',
5155 'name': 'image2',
5069 }]) 5156 }])
5070 library_user = self.getJobFromHistory('library-user') 5157 library_user = self.getJobFromHistory('library-user')
5071 self.assertEqual( 5158 self.assertEqual(
@@ -5077,6 +5164,13 @@ class TestProvidesRequires(ZuulDBTestCase):
5077 'job': 'library-builder', 5164 'job': 'library-builder',
5078 'url': 'http://example.com/library', 5165 'url': 'http://example.com/library',
5079 'name': 'library', 5166 'name': 'library',
5167 }, {
5168 'project': 'org/project1',
5169 'change': '2',
5170 'patchset': '1',
5171 'job': 'library-builder',
5172 'url': 'http://example.com/library2',
5173 'name': 'library2',
5080 }]) 5174 }])
5081 5175
5082 @simple_layout('layouts/provides-requires.yaml') 5176 @simple_layout('layouts/provides-requires.yaml')
@@ -5090,6 +5184,7 @@ class TestProvidesRequires(ZuulDBTestCase):
5090 self.assertHistory([ 5184 self.assertHistory([
5091 dict(name='image-builder', result='FAILURE', changes='1,1'), 5185 dict(name='image-builder', result='FAILURE', changes='1,1'),
5092 dict(name='library-builder', result='FAILURE', changes='1,1'), 5186 dict(name='library-builder', result='FAILURE', changes='1,1'),
5187 dict(name='hold', result='SUCCESS', changes='1,1'),
5093 ], ordered=False) 5188 ], ordered=False)
5094 5189
5095 B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B') 5190 B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
@@ -5101,6 +5196,8 @@ class TestProvidesRequires(ZuulDBTestCase):
5101 self.assertHistory([ 5196 self.assertHistory([
5102 dict(name='image-builder', result='FAILURE', changes='1,1'), 5197 dict(name='image-builder', result='FAILURE', changes='1,1'),
5103 dict(name='library-builder', result='FAILURE', changes='1,1'), 5198 dict(name='library-builder', result='FAILURE', changes='1,1'),
5199 dict(name='hold', result='SUCCESS', changes='1,1'),
5200 dict(name='hold', result='SUCCESS', changes='1,1 2,1'),
5104 ], ordered=False) 5201 ], ordered=False)
5105 self.assertIn('image-user : SKIPPED', B.messages[0]) 5202 self.assertIn('image-user : SKIPPED', B.messages[0])
5106 self.assertIn('not met by build', B.messages[0]) 5203 self.assertIn('not met by build', B.messages[0])
diff --git a/zuul/model.py b/zuul/model.py
index 642f71b..8426826 100644
--- a/zuul/model.py
+++ b/zuul/model.py
@@ -2257,7 +2257,7 @@ class QueueItem(object):
2257 data.append(artifact) 2257 data.append(artifact)
2258 return data 2258 return data
2259 2259
2260 def providesRequirements(self, requirements, data): 2260 def providesRequirements(self, requirements, data, recurse=True):
2261 # Mutates data and returns true/false if requirements 2261 # Mutates data and returns true/false if requirements
2262 # satisfied. 2262 # satisfied.
2263 if not requirements: 2263 if not requirements:
@@ -2271,7 +2271,8 @@ class QueueItem(object):
2271 found = True 2271 found = True
2272 break 2272 break
2273 if found: 2273 if found:
2274 if not item.providesRequirements(requirements, data): 2274 if not item.providesRequirements(requirements, data,
2275 recurse=False):
2275 return False 2276 return False
2276 else: 2277 else:
2277 # Look for this item in the SQL DB. 2278 # Look for this item in the SQL DB.
@@ -2299,6 +2300,8 @@ class QueueItem(object):
2299 data += artifacts 2300 data += artifacts
2300 if not self.item_ahead: 2301 if not self.item_ahead:
2301 return True 2302 return True
2303 if not recurse:
2304 return True
2302 return self.item_ahead.providesRequirements(requirements, data) 2305 return self.item_ahead.providesRequirements(requirements, data)
2303 2306
2304 def jobRequirementsReady(self, job): 2307 def jobRequirementsReady(self, job):
@@ -2307,6 +2310,7 @@ class QueueItem(object):
2307 try: 2310 try:
2308 data = [] 2311 data = []
2309 ret = self.item_ahead.providesRequirements(job.requires, data) 2312 ret = self.item_ahead.providesRequirements(job.requires, data)
2313 data.reverse()
2310 job.updateArtifactData(data) 2314 job.updateArtifactData(data)
2311 except RequirementsError as e: 2315 except RequirementsError as e:
2312 self.warning(str(e)) 2316 self.warning(str(e))