Merge lp:~twom/launchpad/move-snap-building-metadata-to-snapbuild into lp:launchpad

Proposed by Tom Wardill on 2018-12-06
Status: Needs review
Proposed branch: lp:~twom/launchpad/move-snap-building-metadata-to-snapbuild
Merge into: lp:launchpad
Prerequisite: lp:~twom/launchpad/snapbuild-json-data-column
Diff against target: 272 lines (+90/-20)
6 files modified
lib/lp/snappy/interfaces/snapbuild.py (+7/-1)
lib/lp/snappy/interfaces/snapbuildjob.py (+7/-0)
lib/lp/snappy/model/snapbuild.py (+8/-3)
lib/lp/snappy/model/snapbuildjob.py (+31/-14)
lib/lp/snappy/tests/test_snapbuildjob.py (+36/-1)
lib/lp/testing/factory.py (+1/-1)
To merge this branch: bzr merge lp:~twom/launchpad/move-snap-building-metadata-to-snapbuild
Reviewer Review Type Date Requested Status
Colin Watson 2018-12-06 Resubmit on 2018-12-11
Review via email: mp+360192@code.launchpad.net

Commit message

Move some of SnapBuildJob.metadata up to SnapBuilt to enable better retryable state.

Description of the change

Storing the metadata (store_url and status_url) on the SnapBuildJob meant that on each retry it needed to be set. This meant that it was possible to get to an unretryable state if the upload succeeded, but the release failed.

Moved the appropriate metadata up to the SnapBuild level, keeps better track of the build progress.

To post a comment you must log in.
Colin Watson (cjwatson) wrote :

I think we need a specific integration test for the main workflow that's at issue here: that is, if a SnapBuildJob successfully pushes but then fails to release, then a retry attempt doesn't retry the push but does retry the release.

review: Needs Fixing
18835. By Tom Wardill on 2018-12-10

Rename metadata field in snapbuild

18836. By Tom Wardill on 2018-12-10

Tidy method signature for SnapBuildSet#

18837. By Tom Wardill on 2018-12-10

Merge snapbuildjob and snapbuild upload metadata

18838. By Tom Wardill on 2018-12-10

Wrong arguments for Attribute

18839. By Tom Wardill on 2018-12-10

More fixes for Attribute

18840. By Tom Wardill on 2018-12-10

add integration test for retry behaviour

Colin Watson (cjwatson) :
review: Approve
Colin Watson (cjwatson) wrote :

Oh, you're going to need to rebase and resubmit this MP to get rid of the stray prerequisite on the old DB branch. You can just base the new MP directly on devel without a prerequisite - the DB patch doesn't have to be part of the history of the code branch before landing.

review: Resubmit

Unmerged revisions

18840. By Tom Wardill on 2018-12-10

add integration test for retry behaviour

18839. By Tom Wardill on 2018-12-10

More fixes for Attribute

18838. By Tom Wardill on 2018-12-10

Wrong arguments for Attribute

18837. By Tom Wardill on 2018-12-10

Merge snapbuildjob and snapbuild upload metadata

18836. By Tom Wardill on 2018-12-10

Tidy method signature for SnapBuildSet#

18835. By Tom Wardill on 2018-12-10

Rename metadata field in snapbuild

18834. By Tom Wardill on 2018-12-06

Fix tests for new metadata location

18833. By Tom Wardill on 2018-12-05

Add SnapBuild.metadata support to factory and SnapBuildJob

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/snappy/interfaces/snapbuild.py'
2--- lib/lp/snappy/interfaces/snapbuild.py 2018-10-09 10:35:44 +0000
3+++ lib/lp/snappy/interfaces/snapbuild.py 2018-12-10 17:26:07 +0000
4@@ -34,7 +34,10 @@
5 Reference,
6 )
7 from zope.component.interfaces import IObjectEvent
8-from zope.interface import Interface
9+from zope.interface import (
10+ Attribute,
11+ Interface
12+ )
13 from zope.schema import (
14 Bool,
15 Choice,
16@@ -242,6 +245,9 @@
17 value_type=Dict(key_type=TextLine()),
18 required=False, readonly=True))
19
20+ store_upload_metadata = Attribute(
21+ _("A dict of data about store upload progress."))
22+
23 def getFiles():
24 """Retrieve the build's `ISnapFile` records.
25
26
27=== modified file 'lib/lp/snappy/interfaces/snapbuildjob.py'
28--- lib/lp/snappy/interfaces/snapbuildjob.py 2017-04-18 17:54:03 +0000
29+++ lib/lp/snappy/interfaces/snapbuildjob.py 2018-12-10 17:26:07 +0000
30@@ -54,6 +54,9 @@
31 class ISnapStoreUploadJob(IRunnableJob):
32 """A Job that uploads a snap build to the store."""
33
34+ store_metadata = Attribute(
35+ _("Combined metadata for this job and matching snapbuild"))
36+
37 error_message = TextLine(
38 title=_("Error message"), required=False, readonly=True)
39
40@@ -68,6 +71,10 @@
41 title=_("The revision assigned to this build by the store"),
42 required=False, readonly=True)
43
44+ status_url = TextLine(
45+ title=_("The URL on the store to get the status of this build"),
46+ required=False, readonly=True)
47+
48
49 class ISnapStoreUploadJobSource(IJobSource):
50
51
52=== modified file 'lib/lp/snappy/model/snapbuild.py'
53--- lib/lp/snappy/model/snapbuild.py 2018-10-09 10:35:44 +0000
54+++ lib/lp/snappy/model/snapbuild.py 2018-12-10 17:26:07 +0000
55@@ -175,9 +175,11 @@
56
57 failure_count = Int(name='failure_count', allow_none=False)
58
59+ store_upload_metadata = JSON('store_upload_json_data', allow_none=False)
60+
61 def __init__(self, build_farm_job, requester, snap, archive,
62 distro_arch_series, pocket, channels, processor, virtualized,
63- date_created, build_request=None):
64+ date_created, store_upload_metadata, build_request=None):
65 """Construct a `SnapBuild`."""
66 super(SnapBuild, self).__init__()
67 self.build_farm_job = build_farm_job
68@@ -190,6 +192,7 @@
69 self.processor = processor
70 self.virtualized = virtualized
71 self.date_created = date_created
72+ self.store_upload_metadata = store_upload_metadata
73 if build_request is not None:
74 self.build_request_id = build_request.id
75 self.status = BuildStatus.NEEDSBUILD
76@@ -507,7 +510,8 @@
77 class SnapBuildSet(SpecificBuildFarmJobSourceMixin):
78
79 def new(self, requester, snap, archive, distro_arch_series, pocket,
80- channels=None, date_created=DEFAULT, build_request=None):
81+ channels=None, date_created=DEFAULT,
82+ store_upload_metadata=None, build_request=None, ):
83 """See `ISnapBuildSet`."""
84 store = IMasterStore(SnapBuild)
85 build_farm_job = getUtility(IBuildFarmJobSource).new(
86@@ -518,7 +522,8 @@
87 pocket, channels, distro_arch_series.processor,
88 not distro_arch_series.processor.supports_nonvirtualized
89 or snap.require_virtualized or archive.require_virtualized,
90- date_created, build_request=build_request)
91+ date_created, store_upload_metadata=store_upload_metadata or {},
92+ build_request=build_request)
93 store.add(snapbuild)
94 return snapbuild
95
96
97=== modified file 'lib/lp/snappy/model/snapbuildjob.py'
98--- lib/lp/snappy/model/snapbuildjob.py 2018-09-18 14:13:16 +0000
99+++ lib/lp/snappy/model/snapbuildjob.py 2018-12-10 17:26:07 +0000
100@@ -212,54 +212,71 @@
101 return job
102
103 @property
104+ def store_metadata(self):
105+ """See `ISnapStoreUploadJob`."""
106+ intermediate = {}
107+ intermediate.update(self.metadata)
108+ intermediate.update(self.snapbuild.store_upload_metadata)
109+ return intermediate
110+
111+ @property
112 def error_message(self):
113 """See `ISnapStoreUploadJob`."""
114- return self.metadata.get("error_message")
115+ return self.store_metadata.get("error_message")
116
117 @error_message.setter
118 def error_message(self, message):
119 """See `ISnapStoreUploadJob`."""
120- self.metadata["error_message"] = message
121+ self.snapbuild.store_upload_metadata["error_message"] = message
122
123 @property
124 def error_detail(self):
125 """See `ISnapStoreUploadJob`."""
126- return self.metadata.get("error_detail")
127+ return self.store_metadata.get("error_detail")
128
129 @error_detail.setter
130 def error_detail(self, detail):
131 """See `ISnapStoreUploadJob`."""
132- self.metadata["error_detail"] = detail
133+ self.snapbuild.store_upload_metadata["error_detail"] = detail
134
135 @property
136 def error_messages(self):
137 """See `ISnapStoreUploadJob`."""
138- return self.metadata.get("error_messages")
139+ return self.store_metadata.get("error_messages")
140
141 @error_messages.setter
142 def error_messages(self, messages):
143 """See `ISnapStoreUploadJob`."""
144- self.metadata["error_messages"] = messages
145+ self.snapbuild.store_upload_metadata["error_messages"] = messages
146
147 @property
148 def store_url(self):
149 """See `ISnapStoreUploadJob`."""
150- return self.metadata.get("store_url")
151+ return self.store_metadata.get("store_url")
152
153 @store_url.setter
154 def store_url(self, url):
155 """See `ISnapStoreUploadJob`."""
156- self.metadata["store_url"] = url
157+ self.snapbuild.store_upload_metadata["store_url"] = url
158
159 @property
160 def store_revision(self):
161 """See `ISnapStoreUploadJob`."""
162- return self.metadata.get("store_revision")
163+ return self.store_metadata.get("store_revision")
164
165 @store_revision.setter
166 def store_revision(self, revision):
167 """See `ISnapStoreUploadJob`."""
168- self.metadata["store_revision"] = revision
169+ self.snapbuild.store_upload_metadata["store_revision"] = revision
170+
171+ @property
172+ def status_url(self):
173+ """See `ISnapStoreUploadJob`."""
174+ return self.store_metadata.get('status_url')
175+
176+ @status_url.setter
177+ def status_url(self, url):
178+ self.snapbuild.store_upload_metadata["status_url"] = url
179
180 # Ideally we'd just override Job._set_status or similar, but
181 # lazr.delegates makes that difficult, so we use this to override all
182@@ -297,7 +314,7 @@
183 @property
184 def retry_delay(self):
185 """See `BaseRunnableJob`."""
186- if "status_url" in self.metadata and self.store_url is None:
187+ if "status_url" in self.store_metadata and self.store_url is None:
188 # At the moment we have to poll the status endpoint to find out
189 # if the store has finished scanning. Try to deal with easy
190 # cases quickly without hammering our job runners or the store
191@@ -313,13 +330,13 @@
192 """See `IRunnableJob`."""
193 client = getUtility(ISnapStoreClient)
194 try:
195- if "status_url" not in self.metadata:
196- self.metadata["status_url"] = client.upload(self.snapbuild)
197+ if "status_url" not in self.store_metadata:
198+ self.status_url = client.upload(self.snapbuild)
199 # We made progress, so reset attempt_count.
200 self.attempt_count = 1
201 if self.store_url is None:
202 self.store_url, self.store_revision = (
203- client.checkStatus(self.metadata["status_url"]))
204+ client.checkStatus(self.store_metadata["status_url"]))
205 # We made progress, so reset attempt_count.
206 self.attempt_count = 1
207 if self.snapbuild.snap.store_channels:
208
209=== modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
210--- lib/lp/snappy/tests/test_snapbuildjob.py 2018-08-03 13:53:20 +0000
211+++ lib/lp/snappy/tests/test_snapbuildjob.py 2018-12-10 17:26:07 +0000
212@@ -628,7 +628,7 @@
213 for expected_delay in (15, 15, 30, 30, 60):
214 with dbuser(config.ISnapStoreUploadJobSource.dbuser):
215 JobRunner([job]).runAll()
216- self.assertIn("status_url", job.metadata)
217+ self.assertIn("status_url", job.snapbuild.store_upload_metadata)
218 self.assertIsNone(job.store_url)
219 self.assertEqual(
220 timedelta(seconds=expected_delay), job.retry_delay)
221@@ -641,3 +641,38 @@
222 self.assertIsNone(job.error_message)
223 self.assertEqual([], pop_notifications())
224 self.assertEqual(JobStatus.COMPLETED, job.job.status)
225+
226+ def test_retry_after_upload_does_not_upload(self):
227+ # If the job has uploaded, but failed to release, it should
228+ # not attempt to upload again on the next run.
229+ self.useFixture(FakeLogger())
230+ snapbuild = self.makeSnapBuild(store_channels=["stable", "edge"])
231+ self.assertContentEqual([], snapbuild.store_upload_jobs)
232+ job = SnapStoreUploadJob.create(snapbuild)
233+ client = FakeSnapStoreClient()
234+ client.upload.result = self.status_url
235+ client.checkStatus.result = (self.store_url, 1)
236+ client.release.failure = UploadFailedResponse(
237+ "Proxy error", can_retry=True)
238+ self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
239+ with dbuser(config.ISnapStoreUploadJobSource.dbuser):
240+ JobRunner([job]).runAll()
241+
242+ previous_upload = client.upload.calls
243+ previous_checkStatus = client.checkStatus.calls
244+ len_previous_release = len(client.release.calls)
245+
246+ # Check we uploaded as expected
247+ self.assertEqual(self.store_url, job.store_url)
248+ self.assertEqual(1, job.store_revision)
249+ self.assertEqual(timedelta(seconds=60), job.retry_delay)
250+ self.assertEqual(1, len(client.upload.calls))
251+
252+ # Run the job again
253+ with dbuser(config.ISnapStoreUploadJobSource.dbuser):
254+ JobRunner([job]).runAll()
255+
256+ # We should not have called `upload`, but moved straight to `release`
257+ self.assertEqual(previous_upload, client.upload.calls)
258+ self.assertEqual(previous_checkStatus, client.checkStatus.calls)
259+ self.assertEqual(len_previous_release + 1, len(client.release.calls))
260
261=== modified file 'lib/lp/testing/factory.py'
262--- lib/lp/testing/factory.py 2018-10-15 14:44:25 +0000
263+++ lib/lp/testing/factory.py 2018-12-10 17:26:07 +0000
264@@ -4770,7 +4770,7 @@
265 archive=None, distroarchseries=None, pocket=None,
266 channels=None, date_created=DEFAULT,
267 status=BuildStatus.NEEDSBUILD, builder=None,
268- duration=None, **kwargs):
269+ duration=None, metadata=None, **kwargs):
270 """Make a new SnapBuild."""
271 if requester is None:
272 requester = self.makePerson()