Merge lp:~allenap/launchpad/handle-status-ro-crash-bug-905853 into lp:launchpad
- handle-status-ro-crash-bug-905853
- Merge into devel
Proposed by
Gavin Panella
Status: | Merged |
---|---|
Approved by: | Julian Edwards |
Approved revision: | no longer in the source branch. |
Merged at revision: | 14641 |
Proposed branch: | lp:~allenap/launchpad/handle-status-ro-crash-bug-905853 |
Merge into: | lp:launchpad |
Diff against target: |
1657 lines (+555/-227) 16 files modified
buildout-templates/bin/retest.in (+11/-2) lib/lp/archiveuploader/tests/test_uploadprocessor.py (+4/-2) lib/lp/buildmaster/interfaces/builder.py (+3/-1) lib/lp/buildmaster/manager.py (+74/-46) lib/lp/buildmaster/model/builder.py (+28/-15) lib/lp/buildmaster/model/buildfarmjobbehavior.py (+63/-32) lib/lp/buildmaster/model/packagebuild.py (+94/-54) lib/lp/buildmaster/testing.py (+59/-0) lib/lp/buildmaster/tests/test_builder.py (+25/-2) lib/lp/buildmaster/tests/test_manager.py (+120/-29) lib/lp/buildmaster/tests/test_packagebuild.py (+18/-16) lib/lp/code/model/tests/test_sourcepackagerecipebuild.py (+6/-8) lib/lp/services/database/tests/test_transaction_policy.py (+14/-4) lib/lp/services/database/transaction_policy.py (+16/-4) lib/lp/soyuz/tests/test_binarypackagebuild.py (+7/-4) lib/lp/translations/model/translationtemplatesbuildbehavior.py (+13/-8) |
To merge this branch: | bzr merge lp:~allenap/launchpad/handle-status-ro-crash-bug-905853 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Graham Binns (community) | code | Approve | |
Julian Edwards (community) | Approve | ||
Review via email: mp+86298@code.launchpad.net |
Commit message
[r=gmb,
Description of the change
This fixes the problem described in the bug, but it also fixes the tests to (a) run correctly (i.e. with AsynchronousDef
To post a comment you must log in.
Revision history for this message
Graham Binns (gmb) wrote : | # |
Nice branch; I've no complaints.
review:
Approve
(code)
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'buildout-templates/bin/retest.in' |
2 | --- buildout-templates/bin/retest.in 2011-12-22 14:35:39 +0000 |
3 | +++ buildout-templates/bin/retest.in 2012-01-03 12:26:30 +0000 |
4 | @@ -28,7 +28,7 @@ |
5 | import os |
6 | import re |
7 | import sys |
8 | -from itertools import takewhile |
9 | +from itertools import takewhile, imap |
10 | |
11 | ${python-relative-path-setup} |
12 | |
13 | @@ -38,6 +38,14 @@ |
14 | # Regular expression to match numbered stories. |
15 | STORY_RE = re.compile("(.*)/\d{2}-.*") |
16 | |
17 | +# Regular expression to remove terminal color escapes. |
18 | +COLOR_RE = re.compile("\x1b[[][0-9;]+m") |
19 | + |
20 | + |
21 | +def decolorize(text): |
22 | + """Remove all ANSI terminal color escapes from `text`.""" |
23 | + return COLOR_RE.sub("", text) |
24 | + |
25 | |
26 | def get_test_name(test): |
27 | """Get the test name of a failed test. |
28 | @@ -91,7 +99,8 @@ |
29 | |
30 | |
31 | if __name__ == '__main__': |
32 | - tests = extract_tests(fileinput.input()) |
33 | + lines = imap(decolorize, fileinput.input()) |
34 | + tests = extract_tests(lines) |
35 | if len(tests) >= 1: |
36 | run_tests(tests) |
37 | else: |
38 | |
39 | === modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py' |
40 | --- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2011-12-30 06:14:56 +0000 |
41 | +++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2012-01-03 12:26:30 +0000 |
42 | @@ -629,7 +629,8 @@ |
43 | from_addr, to_addrs, raw_msg = stub.test_emails.pop() |
44 | foo_bar = "Foo Bar <foo.bar@canonical.com>" |
45 | daniel = "Daniel Silverstone <daniel.silverstone@canonical.com>" |
46 | - self.assertEqual([e.strip() for e in to_addrs], [foo_bar, daniel]) |
47 | + self.assertContentEqual( |
48 | + [foo_bar, daniel], [e.strip() for e in to_addrs]) |
49 | self.assertTrue( |
50 | "NEW" in raw_msg, "Expected email containing 'NEW', got:\n%s" |
51 | % raw_msg) |
52 | @@ -663,7 +664,8 @@ |
53 | from_addr, to_addrs, raw_msg = stub.test_emails.pop() |
54 | daniel = "Daniel Silverstone <daniel.silverstone@canonical.com>" |
55 | foo_bar = "Foo Bar <foo.bar@canonical.com>" |
56 | - self.assertEqual([e.strip() for e in to_addrs], [foo_bar, daniel]) |
57 | + self.assertContentEqual( |
58 | + [foo_bar, daniel], [e.strip() for e in to_addrs]) |
59 | self.assertTrue("Waiting for approval" in raw_msg, |
60 | "Expected an 'upload awaits approval' email.\n" |
61 | "Got:\n%s" % raw_msg) |
62 | |
63 | === modified file 'lib/lp/buildmaster/interfaces/builder.py' |
64 | --- lib/lp/buildmaster/interfaces/builder.py 2012-01-01 02:58:52 +0000 |
65 | +++ lib/lp/buildmaster/interfaces/builder.py 2012-01-03 12:26:30 +0000 |
66 | @@ -1,4 +1,4 @@ |
67 | -# Copyright 2009 Canonical Ltd. This software is licensed under the |
68 | +# Copyright 2009-2011 Canonical Ltd. This software is licensed under the |
69 | # GNU Affero General Public License version 3 (see the file LICENSE). |
70 | |
71 | # pylint: disable-msg=E0211,E0213 |
72 | @@ -195,6 +195,8 @@ |
73 | |
74 | def setSlaveForTesting(proxy): |
75 | """Sets the RPC proxy through which to operate the build slave.""" |
76 | + # XXX JeroenVermeulen 2011-11-09, bug=888010: Don't use this. |
77 | + # It's a trap. See bug for details. |
78 | |
79 | def verifySlaveBuildCookie(slave_build_id): |
80 | """Verify that a slave's build cookie is consistent. |
81 | |
82 | === modified file 'lib/lp/buildmaster/manager.py' |
83 | --- lib/lp/buildmaster/manager.py 2012-01-01 02:58:52 +0000 |
84 | +++ lib/lp/buildmaster/manager.py 2012-01-03 12:26:30 +0000 |
85 | @@ -1,4 +1,4 @@ |
86 | -# Copyright 2009 Canonical Ltd. This software is licensed under the |
87 | +# Copyright 2009-2011 Canonical Ltd. This software is licensed under the |
88 | # GNU Affero General Public License version 3 (see the file LICENSE). |
89 | |
90 | """Soyuz buildd slave manager logic.""" |
91 | @@ -34,6 +34,7 @@ |
92 | BuildBehaviorMismatch, |
93 | ) |
94 | from lp.buildmaster.model.builder import Builder |
95 | +from lp.services.database.transaction_policy import DatabaseTransactionPolicy |
96 | |
97 | |
98 | BUILDD_MANAGER_LOG_NAME = "slave-scanner" |
99 | @@ -111,13 +112,17 @@ |
100 | # algorithm for polling. |
101 | SCAN_INTERVAL = 15 |
102 | |
103 | - def __init__(self, builder_name, logger): |
104 | + def __init__(self, builder_name, logger, clock=None): |
105 | self.builder_name = builder_name |
106 | self.logger = logger |
107 | + if clock is None: |
108 | + clock = reactor |
109 | + self._clock = clock |
110 | |
111 | def startCycle(self): |
112 | """Scan the builder and dispatch to it or deal with failures.""" |
113 | self.loop = LoopingCall(self.singleCycle) |
114 | + self.loop.clock = self._clock |
115 | self.stopping_deferred = self.loop.start(self.SCAN_INTERVAL) |
116 | return self.stopping_deferred |
117 | |
118 | @@ -138,51 +143,58 @@ |
119 | 1. Print the error in the log |
120 | 2. Increment and assess failure counts on the builder and job. |
121 | """ |
122 | - # Make sure that pending database updates are removed as it |
123 | - # could leave the database in an inconsistent state (e.g. The |
124 | - # job says it's running but the buildqueue has no builder set). |
125 | + # Since this is a failure path, we could be in a broken |
126 | + # transaction. Get us a fresh one. |
127 | transaction.abort() |
128 | |
129 | # If we don't recognise the exception include a stack trace with |
130 | # the error. |
131 | error_message = failure.getErrorMessage() |
132 | - if failure.check( |
133 | + familiar_error = failure.check( |
134 | BuildSlaveFailure, CannotBuild, BuildBehaviorMismatch, |
135 | - CannotResumeHost, BuildDaemonError, CannotFetchFile): |
136 | - self.logger.info("Scanning %s failed with: %s" % ( |
137 | - self.builder_name, error_message)) |
138 | + CannotResumeHost, BuildDaemonError, CannotFetchFile) |
139 | + if familiar_error: |
140 | + self.logger.info( |
141 | + "Scanning %s failed with: %s", |
142 | + self.builder_name, error_message) |
143 | else: |
144 | - self.logger.info("Scanning %s failed with: %s\n%s" % ( |
145 | + self.logger.info( |
146 | + "Scanning %s failed with: %s\n%s", |
147 | self.builder_name, failure.getErrorMessage(), |
148 | - failure.getTraceback())) |
149 | + failure.getTraceback()) |
150 | |
151 | # Decide if we need to terminate the job or fail the |
152 | # builder. |
153 | try: |
154 | builder = get_builder(self.builder_name) |
155 | - builder.gotFailure() |
156 | - if builder.currentjob is not None: |
157 | - build_farm_job = builder.getCurrentBuildFarmJob() |
158 | - build_farm_job.gotFailure() |
159 | - self.logger.info( |
160 | - "builder %s failure count: %s, " |
161 | - "job '%s' failure count: %s" % ( |
162 | + transaction.commit() |
163 | + |
164 | + with DatabaseTransactionPolicy(read_only=False): |
165 | + builder.gotFailure() |
166 | + |
167 | + if builder.currentjob is None: |
168 | + self.logger.info( |
169 | + "Builder %s failed a probe, count: %s", |
170 | + self.builder_name, builder.failure_count) |
171 | + else: |
172 | + build_farm_job = builder.getCurrentBuildFarmJob() |
173 | + build_farm_job.gotFailure() |
174 | + self.logger.info( |
175 | + "builder %s failure count: %s, " |
176 | + "job '%s' failure count: %s", |
177 | self.builder_name, |
178 | builder.failure_count, |
179 | build_farm_job.title, |
180 | - build_farm_job.failure_count)) |
181 | - else: |
182 | - self.logger.info( |
183 | - "Builder %s failed a probe, count: %s" % ( |
184 | - self.builder_name, builder.failure_count)) |
185 | - assessFailureCounts(builder, failure.getErrorMessage()) |
186 | - transaction.commit() |
187 | + build_farm_job.failure_count) |
188 | + |
189 | + assessFailureCounts(builder, failure.getErrorMessage()) |
190 | + transaction.commit() |
191 | except: |
192 | # Catastrophic code failure! Not much we can do. |
193 | + transaction.abort() |
194 | self.logger.error( |
195 | "Miserable failure when trying to examine failure counts:\n", |
196 | exc_info=True) |
197 | - transaction.abort() |
198 | |
199 | def checkCancellation(self, builder): |
200 | """See if there is a pending cancellation request. |
201 | @@ -236,14 +248,9 @@ |
202 | """ |
203 | # We need to re-fetch the builder object on each cycle as the |
204 | # Storm store is invalidated over transaction boundaries. |
205 | - |
206 | self.builder = get_builder(self.builder_name) |
207 | |
208 | def status_updated(ignored): |
209 | - # Commit the changes done while possibly rescuing jobs, to |
210 | - # avoid holding table locks. |
211 | - transaction.commit() |
212 | - |
213 | # See if we think there's an active build on the builder. |
214 | buildqueue = self.builder.getBuildQueue() |
215 | |
216 | @@ -253,14 +260,10 @@ |
217 | return self.builder.updateBuild(buildqueue) |
218 | |
219 | def build_updated(ignored): |
220 | - # Commit changes done while updating the build, to avoid |
221 | - # holding table locks. |
222 | - transaction.commit() |
223 | - |
224 | # If the builder is in manual mode, don't dispatch anything. |
225 | if self.builder.manual: |
226 | self.logger.debug( |
227 | - '%s is in manual mode, not dispatching.' % |
228 | + '%s is in manual mode, not dispatching.', |
229 | self.builder.name) |
230 | return |
231 | |
232 | @@ -278,22 +281,33 @@ |
233 | job = self.builder.currentjob |
234 | if job is not None and not self.builder.builderok: |
235 | self.logger.info( |
236 | - "%s was made unavailable, resetting attached " |
237 | - "job" % self.builder.name) |
238 | - job.reset() |
239 | + "%s was made unavailable; resetting attached job.", |
240 | + self.builder.name) |
241 | transaction.commit() |
242 | + with DatabaseTransactionPolicy(read_only=False): |
243 | + job.reset() |
244 | + transaction.commit() |
245 | return |
246 | |
247 | # See if there is a job we can dispatch to the builder slave. |
248 | |
249 | + # XXX JeroenVermeulen 2011-10-11, bug=872112: The job's |
250 | + # failure count will be reset once the job has started |
251 | + # successfully. Because of intervening commits, you may see |
252 | + # a build with a nonzero failure count that's actually going |
253 | + # to succeed later (and have a failure count of zero). Or |
254 | + # it may fail yet end up with a lower failure count than you |
255 | + # saw earlier. |
256 | d = self.builder.findAndStartJob() |
257 | |
258 | def job_started(candidate): |
259 | if self.builder.currentjob is not None: |
260 | # After a successful dispatch we can reset the |
261 | # failure_count. |
262 | - self.builder.resetFailureCount() |
263 | transaction.commit() |
264 | + with DatabaseTransactionPolicy(read_only=False): |
265 | + self.builder.resetFailureCount() |
266 | + transaction.commit() |
267 | return self.builder.slave |
268 | else: |
269 | return None |
270 | @@ -372,6 +386,7 @@ |
271 | self.logger = self._setupLogger() |
272 | self.new_builders_scanner = NewBuildersScanner( |
273 | manager=self, clock=clock) |
274 | + self.transaction_policy = DatabaseTransactionPolicy(read_only=True) |
275 | |
276 | def _setupLogger(self): |
277 | """Set up a 'slave-scanner' logger that redirects to twisted. |
278 | @@ -390,16 +405,28 @@ |
279 | logger.setLevel(level) |
280 | return logger |
281 | |
282 | + def enterReadOnlyDatabasePolicy(self): |
283 | + """Set the database transaction policy to read-only. |
284 | + |
285 | + Any previously pending changes are committed first. |
286 | + """ |
287 | + transaction.commit() |
288 | + self.transaction_policy.__enter__() |
289 | + |
290 | + def exitReadOnlyDatabasePolicy(self, *args): |
291 | + """Reset database transaction policy to the default read-write.""" |
292 | + self.transaction_policy.__exit__(None, None, None) |
293 | + |
294 | def startService(self): |
295 | """Service entry point, called when the application starts.""" |
296 | + # Avoiding circular imports. |
297 | + from lp.buildmaster.interfaces.builder import IBuilderSet |
298 | + |
299 | + self.enterReadOnlyDatabasePolicy() |
300 | |
301 | # Get a list of builders and set up scanners on each one. |
302 | - |
303 | - # Avoiding circular imports. |
304 | - from lp.buildmaster.interfaces.builder import IBuilderSet |
305 | - builder_set = getUtility(IBuilderSet) |
306 | - builders = [builder.name for builder in builder_set] |
307 | - self.addScanForBuilders(builders) |
308 | + self.addScanForBuilders( |
309 | + [builder.name for builder in getUtility(IBuilderSet)]) |
310 | self.new_builders_scanner.scheduleScan() |
311 | |
312 | # Events will now fire in the SlaveScanner objects to scan each |
313 | @@ -420,6 +447,7 @@ |
314 | # stopped, so we can wait on them all at once here before |
315 | # exiting. |
316 | d = defer.DeferredList(deferreds, consumeErrors=True) |
317 | + d.addCallback(self.exitReadOnlyDatabasePolicy) |
318 | return d |
319 | |
320 | def addScanForBuilders(self, builders): |
321 | |
322 | === modified file 'lib/lp/buildmaster/model/builder.py' |
323 | --- lib/lp/buildmaster/model/builder.py 2012-01-02 11:21:14 +0000 |
324 | +++ lib/lp/buildmaster/model/builder.py 2012-01-03 12:26:30 +0000 |
325 | @@ -1,4 +1,4 @@ |
326 | -# Copyright 2009,2011 Canonical Ltd. This software is licensed under the |
327 | +# Copyright 2009-2011 Canonical Ltd. This software is licensed under the |
328 | # GNU Affero General Public License version 3 (see the file LICENSE). |
329 | |
330 | # pylint: disable-msg=E0611,W0212 |
331 | @@ -66,6 +66,7 @@ |
332 | SQLBase, |
333 | sqlvalues, |
334 | ) |
335 | +from lp.services.database.transaction_policy import DatabaseTransactionPolicy |
336 | from lp.services.helpers import filenameToContentType |
337 | from lp.services.job.interfaces.job import JobStatus |
338 | from lp.services.job.model.job import Job |
339 | @@ -545,6 +546,8 @@ |
340 | |
341 | def setSlaveForTesting(self, proxy): |
342 | """See IBuilder.""" |
343 | + # XXX JeroenVermeulen 2011-11-09, bug=888010: Don't use this. |
344 | + # It's a trap. See bug for details. |
345 | self._testing_slave = proxy |
346 | del get_property_cache(self).slave |
347 | |
348 | @@ -673,10 +676,13 @@ |
349 | bytes_written = out_file.tell() |
350 | out_file.seek(0) |
351 | |
352 | - library_file = getUtility(ILibraryFileAliasSet).create( |
353 | - filename, bytes_written, out_file, |
354 | - contentType=filenameToContentType(filename), |
355 | - restricted=private) |
356 | + transaction.commit() |
357 | + with DatabaseTransactionPolicy(read_only=False): |
358 | + library_file = getUtility(ILibraryFileAliasSet).create( |
359 | + filename, bytes_written, out_file, |
360 | + contentType=filenameToContentType(filename), |
361 | + restricted=private) |
362 | + transaction.commit() |
363 | finally: |
364 | # Remove the temporary file. getFile() closes the file |
365 | # object. |
366 | @@ -714,7 +720,7 @@ |
367 | def acquireBuildCandidate(self): |
368 | """Acquire a build candidate in an atomic fashion. |
369 | |
370 | - When retrieiving a candidate we need to mark it as building |
371 | + When retrieving a candidate we need to mark it as building |
372 | immediately so that it is not dispatched by another builder in the |
373 | build manager. |
374 | |
375 | @@ -724,12 +730,15 @@ |
376 | can be in this code at the same time. |
377 | |
378 | If there's ever more than one build manager running at once, then |
379 | - this code will need some sort of mutex. |
380 | + this code will need some sort of mutex, or run in a single |
381 | + transaction. |
382 | """ |
383 | candidate = self._findBuildCandidate() |
384 | if candidate is not None: |
385 | - candidate.markAsBuilding(self) |
386 | transaction.commit() |
387 | + with DatabaseTransactionPolicy(read_only=False): |
388 | + candidate.markAsBuilding(self) |
389 | + transaction.commit() |
390 | return candidate |
391 | |
392 | def _findBuildCandidate(self): |
393 | @@ -792,13 +801,17 @@ |
394 | store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) |
395 | candidate_jobs = store.execute(query).get_all() |
396 | |
397 | - for (candidate_id,) in candidate_jobs: |
398 | - candidate = getUtility(IBuildQueueSet).get(candidate_id) |
399 | - job_class = job_classes[candidate.job_type] |
400 | - candidate_approved = job_class.postprocessCandidate( |
401 | - candidate, logger) |
402 | - if candidate_approved: |
403 | - return candidate |
404 | + transaction.commit() |
405 | + with DatabaseTransactionPolicy(read_only=False): |
406 | + for (candidate_id,) in candidate_jobs: |
407 | + candidate = getUtility(IBuildQueueSet).get(candidate_id) |
408 | + job_class = job_classes[candidate.job_type] |
409 | + candidate_approved = job_class.postprocessCandidate( |
410 | + candidate, logger) |
411 | + if candidate_approved: |
412 | + transaction.commit() |
413 | + return candidate |
414 | + transaction.commit() |
415 | |
416 | return None |
417 | |
418 | |
419 | === modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py' |
420 | --- lib/lp/buildmaster/model/buildfarmjobbehavior.py 2011-12-30 02:24:09 +0000 |
421 | +++ lib/lp/buildmaster/model/buildfarmjobbehavior.py 2012-01-03 12:26:30 +0000 |
422 | @@ -1,4 +1,4 @@ |
423 | -# Copyright 2009-2010 Canonical Ltd. This software is licensed under the |
424 | +# Copyright 2009-2011 Canonical Ltd. This software is licensed under the |
425 | # GNU Affero General Public License version 3 (see the file LICENSE). |
426 | |
427 | # pylint: disable-msg=E0211,E0213 |
428 | @@ -16,6 +16,7 @@ |
429 | import socket |
430 | import xmlrpclib |
431 | |
432 | +import transaction |
433 | from twisted.internet import defer |
434 | from zope.component import getUtility |
435 | from zope.interface import implements |
436 | @@ -30,6 +31,7 @@ |
437 | IBuildFarmJobBehavior, |
438 | ) |
439 | from lp.services import encoding |
440 | +from lp.services.database.transaction_policy import DatabaseTransactionPolicy |
441 | from lp.services.job.interfaces.job import JobStatus |
442 | from lp.services.librarian.interfaces.client import ILibrarianClient |
443 | |
444 | @@ -69,6 +71,25 @@ |
445 | if slave_build_cookie != expected_cookie: |
446 | raise CorruptBuildCookie("Invalid slave build cookie.") |
447 | |
448 | + def _getBuilderStatusHandler(self, status_text, logger): |
449 | + """Look up the handler method for a given builder status. |
450 | + |
451 | + If status is not a known one, logs an error and returns None. |
452 | + """ |
453 | + builder_status_handlers = { |
454 | + 'BuilderStatus.IDLE': self.updateBuild_IDLE, |
455 | + 'BuilderStatus.BUILDING': self.updateBuild_BUILDING, |
456 | + 'BuilderStatus.ABORTING': self.updateBuild_ABORTING, |
457 | + 'BuilderStatus.ABORTED': self.updateBuild_ABORTED, |
458 | + 'BuilderStatus.WAITING': self.updateBuild_WAITING, |
459 | + } |
460 | + handler = builder_status_handlers.get(status_text) |
461 | + if handler is None: |
462 | + logger.critical( |
463 | + "Builder on %s returned unknown status %s; failing it.", |
464 | + self._builder.url, status_text) |
465 | + return handler |
466 | + |
467 | def updateBuild(self, queueItem): |
468 | """See `IBuildFarmJobBehavior`.""" |
469 | logger = logging.getLogger('slave-scanner') |
470 | @@ -76,6 +97,7 @@ |
471 | d = self._builder.slaveStatus() |
472 | |
473 | def got_failure(failure): |
474 | + transaction.abort() |
475 | failure.trap(xmlrpclib.Fault, socket.error) |
476 | info = failure.value |
477 | info = ("Could not contact the builder %s, caught a (%s)" |
478 | @@ -83,27 +105,22 @@ |
479 | raise BuildSlaveFailure(info) |
480 | |
481 | def got_status(slave_status): |
482 | - builder_status_handlers = { |
483 | - 'BuilderStatus.IDLE': self.updateBuild_IDLE, |
484 | - 'BuilderStatus.BUILDING': self.updateBuild_BUILDING, |
485 | - 'BuilderStatus.ABORTING': self.updateBuild_ABORTING, |
486 | - 'BuilderStatus.ABORTED': self.updateBuild_ABORTED, |
487 | - 'BuilderStatus.WAITING': self.updateBuild_WAITING, |
488 | - } |
489 | - |
490 | builder_status = slave_status['builder_status'] |
491 | - if builder_status not in builder_status_handlers: |
492 | - logger.critical( |
493 | - "Builder on %s returned unknown status %s, failing it" |
494 | - % (self._builder.url, builder_status)) |
495 | - self._builder.failBuilder( |
496 | + status_handler = self._getBuilderStatusHandler( |
497 | + builder_status, logger) |
498 | + if status_handler is None: |
499 | + error = ( |
500 | "Unknown status code (%s) returned from status() probe." |
501 | % builder_status) |
502 | - # XXX: This will leave the build and job in a bad state, but |
503 | - # should never be possible, since our builder statuses are |
504 | - # known. |
505 | - queueItem._builder = None |
506 | - queueItem.setDateStarted(None) |
507 | + transaction.commit() |
508 | + with DatabaseTransactionPolicy(read_only=False): |
509 | + self._builder.failBuilder(error) |
510 | + # XXX: This will leave the build and job in a bad |
511 | + # state, but should never be possible since our |
512 | + # builder statuses are known. |
513 | + queueItem._builder = None |
514 | + queueItem.setDateStarted(None) |
515 | + transaction.commit() |
516 | return |
517 | |
518 | # Since logtail is a xmlrpclib.Binary container and it is |
519 | @@ -113,9 +130,8 @@ |
520 | # will simply remove the proxy. |
521 | logtail = removeSecurityProxy(slave_status.get('logtail')) |
522 | |
523 | - method = builder_status_handlers[builder_status] |
524 | return defer.maybeDeferred( |
525 | - method, queueItem, slave_status, logtail, logger) |
526 | + status_handler, queueItem, slave_status, logtail, logger) |
527 | |
528 | d.addErrback(got_failure) |
529 | d.addCallback(got_status) |
530 | @@ -127,22 +143,32 @@ |
531 | Log this and reset the record. |
532 | """ |
533 | logger.warn( |
534 | - "Builder %s forgot about buildqueue %d -- resetting buildqueue " |
535 | - "record" % (queueItem.builder.url, queueItem.id)) |
536 | - queueItem.reset() |
537 | + "Builder %s forgot about buildqueue %d -- " |
538 | + "resetting buildqueue record.", |
539 | + queueItem.builder.url, queueItem.id) |
540 | + transaction.commit() |
541 | + with DatabaseTransactionPolicy(read_only=False): |
542 | + queueItem.reset() |
543 | + transaction.commit() |
544 | |
545 | def updateBuild_BUILDING(self, queueItem, slave_status, logtail, logger): |
546 | """Build still building, collect the logtail""" |
547 | - if queueItem.job.status != JobStatus.RUNNING: |
548 | - queueItem.job.start() |
549 | - queueItem.logtail = encoding.guess(str(logtail)) |
550 | + transaction.commit() |
551 | + with DatabaseTransactionPolicy(read_only=False): |
552 | + if queueItem.job.status != JobStatus.RUNNING: |
553 | + queueItem.job.start() |
554 | + queueItem.logtail = encoding.guess(str(logtail)) |
555 | + transaction.commit() |
556 | |
557 | def updateBuild_ABORTING(self, queueItem, slave_status, logtail, logger): |
558 | """Build was ABORTED. |
559 | |
560 | Master-side should wait until the slave finish the process correctly. |
561 | """ |
562 | - queueItem.logtail = "Waiting for slave process to be terminated" |
563 | + transaction.commit() |
564 | + with DatabaseTransactionPolicy(read_only=False): |
565 | + queueItem.logtail = "Waiting for slave process to be terminated" |
566 | + transaction.commit() |
567 | |
568 | def updateBuild_ABORTED(self, queueItem, slave_status, logtail, logger): |
569 | """ABORTING process has successfully terminated. |
570 | @@ -150,11 +176,16 @@ |
571 | Clean the builder for another jobs. |
572 | """ |
573 | d = queueItem.builder.cleanSlave() |
574 | + |
575 | def got_cleaned(ignored): |
576 | - queueItem.builder = None |
577 | - if queueItem.job.status != JobStatus.FAILED: |
578 | - queueItem.job.fail() |
579 | - queueItem.specific_job.jobAborted() |
580 | + transaction.commit() |
581 | + with DatabaseTransactionPolicy(read_only=False): |
582 | + queueItem.builder = None |
583 | + if queueItem.job.status != JobStatus.FAILED: |
584 | + queueItem.job.fail() |
585 | + queueItem.specific_job.jobAborted() |
586 | + transaction.commit() |
587 | + |
588 | return d.addCallback(got_cleaned) |
589 | |
590 | def extractBuildStatus(self, slave_status): |
591 | |
592 | === modified file 'lib/lp/buildmaster/model/packagebuild.py' |
593 | --- lib/lp/buildmaster/model/packagebuild.py 2011-12-30 06:14:56 +0000 |
594 | +++ lib/lp/buildmaster/model/packagebuild.py 2012-01-03 12:26:30 +0000 |
595 | @@ -1,4 +1,4 @@ |
596 | -# Copyright 2010 Canonical Ltd. This software is licensed under the |
597 | +# Copyright 2010-2011 Canonical Ltd. This software is licensed under the |
598 | # GNU Affero General Public License version 3 (see the file LICENSE). |
599 | |
600 | __metaclass__ = type |
601 | @@ -24,6 +24,7 @@ |
602 | Storm, |
603 | Unicode, |
604 | ) |
605 | +import transaction |
606 | from zope.component import getUtility |
607 | from zope.interface import ( |
608 | classProvides, |
609 | @@ -50,6 +51,7 @@ |
610 | from lp.services.config import config |
611 | from lp.services.database.enumcol import DBEnum |
612 | from lp.services.database.lpstorm import IMasterStore |
613 | +from lp.services.database.transaction_policy import DatabaseTransactionPolicy |
614 | from lp.services.helpers import filenameToContentType |
615 | from lp.services.librarian.browser import ProxiedLibraryFileAlias |
616 | from lp.services.librarian.interfaces import ILibraryFileAliasSet |
617 | @@ -177,19 +179,24 @@ |
618 | def storeBuildInfo(build, librarian, slave_status): |
619 | """See `IPackageBuild`.""" |
620 | def got_log(lfa_id): |
621 | + dependencies = slave_status.get('dependencies') |
622 | + if dependencies is not None: |
623 | + dependencies = unicode(dependencies) |
624 | + |
625 | # log, builder and date_finished are read-only, so we must |
626 | # currently remove the security proxy to set them. |
627 | naked_build = removeSecurityProxy(build) |
628 | - naked_build.log = lfa_id |
629 | - naked_build.builder = build.buildqueue_record.builder |
630 | - # XXX cprov 20060615 bug=120584: Currently buildduration includes |
631 | - # the scanner latency, it should really be asking the slave for |
632 | - # the duration spent building locally. |
633 | - naked_build.date_finished = datetime.datetime.now(pytz.UTC) |
634 | - if slave_status.get('dependencies') is not None: |
635 | - build.dependencies = unicode(slave_status.get('dependencies')) |
636 | - else: |
637 | - build.dependencies = None |
638 | + |
639 | + transaction.commit() |
640 | + with DatabaseTransactionPolicy(read_only=False): |
641 | + naked_build.log = lfa_id |
642 | + naked_build.builder = build.buildqueue_record.builder |
643 | + # XXX cprov 20060615 bug=120584: Currently buildduration |
644 | + # includes the scanner latency. It should really be asking |
645 | + # the slave for the duration spent building locally. |
646 | + naked_build.date_finished = datetime.datetime.now(pytz.UTC) |
647 | + build.dependencies = dependencies |
648 | + transaction.commit() |
649 | |
650 | d = build.getLogFromSlave(build) |
651 | return d.addCallback(got_log) |
652 | @@ -290,22 +297,41 @@ |
653 | |
654 | def handleStatus(self, status, librarian, slave_status): |
655 | """See `IPackageBuild`.""" |
656 | + # Avoid circular imports. |
657 | from lp.buildmaster.manager import BUILDD_MANAGER_LOG_NAME |
658 | + |
659 | logger = logging.getLogger(BUILDD_MANAGER_LOG_NAME) |
660 | send_notification = status in self.ALLOWED_STATUS_NOTIFICATIONS |
661 | method = getattr(self, '_handleStatus_' + status, None) |
662 | if method is None: |
663 | - logger.critical("Unknown BuildStatus '%s' for builder '%s'" |
664 | - % (status, self.buildqueue_record.builder.url)) |
665 | - return |
666 | + logger.critical( |
667 | + "Unknown BuildStatus '%s' for builder '%s'", |
668 | + status, self.buildqueue_record.builder.url) |
669 | + return None |
670 | + |
671 | d = method(librarian, slave_status, logger, send_notification) |
672 | return d |
673 | |
674 | + def _destroy_buildqueue_record(self, unused_arg): |
675 | + """Destroy this build's `BuildQueue` record.""" |
676 | + transaction.commit() |
677 | + with DatabaseTransactionPolicy(read_only=False): |
678 | + self.buildqueue_record.destroySelf() |
679 | + transaction.commit() |
680 | + |
681 | def _release_builder_and_remove_queue_item(self): |
682 | # Release the builder for another job. |
683 | d = self.buildqueue_record.builder.cleanSlave() |
684 | # Remove BuildQueue record. |
685 | - return d.addCallback(lambda x: self.buildqueue_record.destroySelf()) |
686 | + return d.addCallback(self._destroy_buildqueue_record) |
687 | + |
688 | + def _notify_if_appropriate(self, appropriate=True, extra_info=None): |
689 | + """If `appropriate`, call `self.notify` in a write transaction.""" |
690 | + if appropriate: |
691 | + transaction.commit() |
692 | + with DatabaseTransactionPolicy(read_only=False): |
693 | + self.notify(extra_info=extra_info) |
694 | + transaction.commit() |
695 | |
696 | def _handleStatus_OK(self, librarian, slave_status, logger, |
697 | send_notification): |
698 | @@ -321,16 +347,19 @@ |
699 | self.buildqueue_record.specific_job.build.title, |
700 | self.buildqueue_record.builder.name)) |
701 | |
702 | - # If this is a binary package build, discard it if its source is |
703 | - # no longer published. |
704 | + # If this is a binary package build for a source that is no |
705 | + # longer published, discard it. |
706 | if self.build_farm_job_type == BuildFarmJobType.PACKAGEBUILD: |
707 | build = self.buildqueue_record.specific_job.build |
708 | if not build.current_source_publication: |
709 | - build.status = BuildStatus.SUPERSEDED |
710 | + transaction.commit() |
711 | + with DatabaseTransactionPolicy(read_only=False): |
712 | + build.status = BuildStatus.SUPERSEDED |
713 | + transaction.commit() |
714 | return self._release_builder_and_remove_queue_item() |
715 | |
716 | - # Explode before collect a binary that is denied in this |
717 | - # distroseries/pocket |
718 | + # Explode rather than collect a binary that is denied in this |
719 | + # distroseries/pocket. |
720 | if not self.archive.allowUpdatesToReleasePocket(): |
721 | assert self.distro_series.canUploadToPocket(self.pocket), ( |
722 | "%s (%s) can not be built for pocket %s: illegal status" |
723 | @@ -375,18 +404,26 @@ |
724 | # files from the slave. |
725 | if successful_copy_from_slave: |
726 | logger.info( |
727 | - "Gathered %s %d completely. Moving %s to uploader queue." |
728 | - % (self.__class__.__name__, self.id, upload_leaf)) |
729 | + "Gathered %s %d completely. " |
730 | + "Moving %s to uploader queue.", |
731 | + self.__class__.__name__, self.id, upload_leaf) |
732 | target_dir = os.path.join(root, "incoming") |
733 | - self.status = BuildStatus.UPLOADING |
734 | + resulting_status = BuildStatus.UPLOADING |
735 | else: |
736 | logger.warning( |
737 | - "Copy from slave for build %s was unsuccessful.", self.id) |
738 | - self.status = BuildStatus.FAILEDTOUPLOAD |
739 | - if send_notification: |
740 | - self.notify( |
741 | - extra_info='Copy from slave was unsuccessful.') |
742 | + "Copy from slave for build %s was unsuccessful.", |
743 | + self.id) |
744 | target_dir = os.path.join(root, "failed") |
745 | + resulting_status = BuildStatus.FAILEDTOUPLOAD |
746 | + |
747 | + transaction.commit() |
748 | + with DatabaseTransactionPolicy(read_only=False): |
749 | + self.status = resulting_status |
750 | + transaction.commit() |
751 | + |
752 | + if not successful_copy_from_slave: |
753 | + self._notify_if_appropriate( |
754 | + send_notification, "Copy from slave was unsuccessful.") |
755 | |
756 | if not os.path.exists(target_dir): |
757 | os.mkdir(target_dir) |
758 | @@ -394,10 +431,6 @@ |
759 | # Release the builder for another job. |
760 | d = self._release_builder_and_remove_queue_item() |
761 | |
762 | - # Commit so there are no race conditions with archiveuploader |
763 | - # about self.status. |
764 | - Store.of(self).commit() |
765 | - |
766 | # Move the directory used to grab the binaries into |
767 | # the incoming directory so the upload processor never |
768 | # sees half-finished uploads. |
769 | @@ -421,14 +454,15 @@ |
770 | set the job status as FAILEDTOBUILD, store available info and |
771 | remove Buildqueue entry. |
772 | """ |
773 | - self.status = BuildStatus.FAILEDTOBUILD |
774 | + transaction.commit() |
775 | + with DatabaseTransactionPolicy(read_only=False): |
776 | + self.status = BuildStatus.FAILEDTOBUILD |
777 | + transaction.commit() |
778 | |
779 | def build_info_stored(ignored): |
780 | - if send_notification: |
781 | - self.notify() |
782 | + self._notify_if_appropriate(send_notification) |
783 | d = self.buildqueue_record.builder.cleanSlave() |
784 | - return d.addCallback( |
785 | - lambda x: self.buildqueue_record.destroySelf()) |
786 | + return d.addCallback(self._destroy_buildqueue_record) |
787 | |
788 | d = self.storeBuildInfo(self, librarian, slave_status) |
789 | return d.addCallback(build_info_stored) |
790 | @@ -441,16 +475,16 @@ |
791 | MANUALDEPWAIT, store available information, remove BuildQueue |
792 | entry and release builder slave for another job. |
793 | """ |
794 | - self.status = BuildStatus.MANUALDEPWAIT |
795 | + with DatabaseTransactionPolicy(read_only=False): |
796 | + self.status = BuildStatus.MANUALDEPWAIT |
797 | + transaction.commit() |
798 | |
799 | def build_info_stored(ignored): |
800 | logger.critical("***** %s is MANUALDEPWAIT *****" |
801 | % self.buildqueue_record.builder.name) |
802 | - if send_notification: |
803 | - self.notify() |
804 | + self._notify_if_appropriate(send_notification) |
805 | d = self.buildqueue_record.builder.cleanSlave() |
806 | - return d.addCallback( |
807 | - lambda x: self.buildqueue_record.destroySelf()) |
808 | + return d.addCallback(self._destroy_buildqueue_record) |
809 | |
810 | d = self.storeBuildInfo(self, librarian, slave_status) |
811 | return d.addCallback(build_info_stored) |
812 | @@ -463,20 +497,29 @@ |
813 | job as CHROOTFAIL, store available information, remove BuildQueue |
814 | and release the builder. |
815 | """ |
816 | - self.status = BuildStatus.CHROOTWAIT |
817 | + with DatabaseTransactionPolicy(read_only=False): |
818 | + self.status = BuildStatus.CHROOTWAIT |
819 | + transaction.commit() |
820 | |
821 | def build_info_stored(ignored): |
822 | - logger.critical("***** %s is CHROOTWAIT *****" % |
823 | - self.buildqueue_record.builder.name) |
824 | - if send_notification: |
825 | - self.notify() |
826 | + logger.critical( |
827 | + "***** %s is CHROOTWAIT *****", |
828 | + self.buildqueue_record.builder.name) |
829 | + |
830 | + self._notify_if_appropriate(send_notification) |
831 | d = self.buildqueue_record.builder.cleanSlave() |
832 | - return d.addCallback( |
833 | - lambda x: self.buildqueue_record.destroySelf()) |
834 | + return d.addCallback(self._destroy_buildqueue_record) |
835 | |
836 | d = self.storeBuildInfo(self, librarian, slave_status) |
837 | return d.addCallback(build_info_stored) |
838 | |
839 | + def _reset_buildqueue_record(self, ignored_arg=None): |
840 | + """Reset the `BuildQueue` record, in a write transaction.""" |
841 | + transaction.commit() |
842 | + with DatabaseTransactionPolicy(read_only=False): |
843 | + self.buildqueue_record.reset() |
844 | + transaction.commit() |
845 | + |
846 | def _handleStatus_BUILDERFAIL(self, librarian, slave_status, logger, |
847 | send_notification): |
848 | """Handle builder failures. |
849 | @@ -490,11 +533,8 @@ |
850 | self.buildqueue_record.builder.failBuilder( |
851 | "Builder returned BUILDERFAIL when asked for its status") |
852 | |
853 | - def build_info_stored(ignored): |
854 | - # simply reset job |
855 | - self.buildqueue_record.reset() |
856 | d = self.storeBuildInfo(self, librarian, slave_status) |
857 | - return d.addCallback(build_info_stored) |
858 | + return d.addCallback(self._reset_buildqueue_record) |
859 | |
860 | def _handleStatus_GIVENBACK(self, librarian, slave_status, logger, |
861 | send_notification): |
862 | @@ -514,7 +554,7 @@ |
863 | # the next Paris Summit, infinity has some ideas about how |
864 | # to use this content. For now we just ensure it's stored. |
865 | d = self.buildqueue_record.builder.cleanSlave() |
866 | - self.buildqueue_record.reset() |
867 | + self._reset_buildqueue_record() |
868 | return d |
869 | |
870 | d = self.storeBuildInfo(self, librarian, slave_status) |
871 | |
872 | === added file 'lib/lp/buildmaster/testing.py' |
873 | --- lib/lp/buildmaster/testing.py 1970-01-01 00:00:00 +0000 |
874 | +++ lib/lp/buildmaster/testing.py 2012-01-03 12:26:30 +0000 |
875 | @@ -0,0 +1,59 @@ |
876 | +# Copyright 2011 Canonical Ltd. This software is licensed under the |
877 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
878 | + |
879 | +"""Testing helpers for buildmaster code.""" |
880 | + |
881 | +__metaclass__ = type |
882 | +__all__ = [ |
883 | + "BuilddManagerTestFixture", |
884 | + ] |
885 | + |
886 | +from contextlib import contextmanager |
887 | + |
888 | +import fixtures |
889 | +import transaction |
890 | + |
891 | +from lp.services.database.transaction_policy import DatabaseTransactionPolicy |
892 | + |
893 | + |
894 | +class BuilddManagerTestFixture(fixtures.Fixture): |
895 | + """Helps provide an environment more like `BuilddManager` provides. |
896 | + |
897 | + This mimics the default transaction access policy of `BuilddManager`, |
898 | + though it can be configured with a different policy by passing in `store` |
899 | + and/or `read_only`. See `BuilddManager.enterReadOnlyDatabasePolicy`. |
900 | + |
901 | + Because this will shift into a read-only database transaction access mode, |
902 | + individual tests that need to do more setup can use the `extraSetUp()` |
903 | + context manager to temporarily shift back to a read-write mode. |
904 | + """ |
905 | + |
906 | + def __init__(self, store=None, read_only=True): |
907 | + super(BuilddManagerTestFixture, self).__init__() |
908 | + self.policy = DatabaseTransactionPolicy( |
909 | + store=store, read_only=read_only) |
910 | + |
911 | + def setUp(self): |
912 | + # Commit everything done so far then shift into a read-only |
913 | + # transaction access mode by default. |
914 | + super(BuilddManagerTestFixture, self).setUp() |
915 | + transaction.commit() |
916 | + self.policy.__enter__() |
917 | + self.addCleanup(self.policy.__exit__, None, None, None) |
918 | + |
919 | + @staticmethod |
920 | + @contextmanager |
921 | + def extraSetUp(): |
922 | + """Temporarily enter a read-write transaction to do extra setup. |
923 | + |
924 | + For example: |
925 | + |
926 | + with self.extraSetUp(): |
927 | + removeSecurityProxy(self.build).date_finished = None |
928 | + |
929 | + On exit it will commit the changes and restore the previous |
930 | + transaction access mode. |
931 | + """ |
932 | + with DatabaseTransactionPolicy(read_only=False): |
933 | + yield |
934 | + transaction.commit() |
935 | |
936 | === modified file 'lib/lp/buildmaster/tests/test_builder.py' |
937 | --- lib/lp/buildmaster/tests/test_builder.py 2011-12-30 06:14:56 +0000 |
938 | +++ lib/lp/buildmaster/tests/test_builder.py 2012-01-03 12:26:30 +0000 |
939 | @@ -1,4 +1,4 @@ |
940 | -# Copyright 2009-2010 Canonical Ltd. This software is licensed under the |
941 | +# Copyright 2009-2011 Canonical Ltd. This software is licensed under the |
942 | # GNU Affero General Public License version 3 (see the file LICENSE). |
943 | |
944 | """Test Builder features.""" |
945 | @@ -15,6 +15,7 @@ |
946 | AsynchronousDeferredRunTestForBrokenTwisted, |
947 | SynchronousDeferredRunTest, |
948 | ) |
949 | +import transaction |
950 | from twisted.internet.defer import ( |
951 | CancelledError, |
952 | DeferredList, |
953 | @@ -60,8 +61,10 @@ |
954 | TrivialBehavior, |
955 | WaitingSlave, |
956 | ) |
957 | +from lp.registry.interfaces.pocket import PackagePublishingPocket |
958 | from lp.services.config import config |
959 | from lp.services.database.sqlbase import flush_database_updates |
960 | +from lp.services.database.transaction_policy import DatabaseTransactionPolicy |
961 | from lp.services.job.interfaces.job import JobStatus |
962 | from lp.services.log.logger import BufferLogger |
963 | from lp.services.webapp.interfaces import ( |
964 | @@ -152,7 +155,7 @@ |
965 | d = lostbuilding_builder.updateStatus(BufferLogger()) |
966 | def check_slave_status(failure): |
967 | self.assertIn('abort', slave.call_log) |
968 | - # 'Fault' comes from the LostBuildingBrokenSlave, this is |
969 | + # 'Fault' comes from the LostBuildingBrokenSlave. This is |
970 | # just testing that the value is passed through. |
971 | self.assertIsInstance(failure.value, xmlrpclib.Fault) |
972 | return d.addBoth(check_slave_status) |
973 | @@ -531,6 +534,26 @@ |
974 | # And the old_candidate is superseded: |
975 | self.assertEqual(BuildStatus.SUPERSEDED, build.status) |
976 | |
977 | + def test_findBuildCandidate_postprocesses_in_read_write_policy(self): |
978 | + # _findBuildCandidate invokes BuildFarmJob.postprocessCandidate, |
979 | + # which may modify the database. This happens in a read-write |
980 | + # transaction even if _findBuildCandidate itself runs in a |
981 | + # read-only transaction policy. |
982 | + |
983 | + # PackageBuildJob.postprocessCandidate will attempt to delete |
984 | + # security builds. |
985 | + pub = self.publisher.getPubSource( |
986 | + sourcename="gedit", status=PackagePublishingStatus.PUBLISHED, |
987 | + archive=self.factory.makeArchive(), |
988 | + pocket=PackagePublishingPocket.SECURITY) |
989 | + pub.createMissingBuilds() |
990 | + transaction.commit() |
991 | + with DatabaseTransactionPolicy(read_only=True): |
992 | + removeSecurityProxy(self.frog_builder)._findBuildCandidate() |
993 | + # The test is that this passes without a "transaction is |
994 | + # read-only" error. |
995 | + transaction.commit() |
996 | + |
997 | def test_acquireBuildCandidate_marks_building(self): |
998 | # acquireBuildCandidate() should call _findBuildCandidate and |
999 | # mark the build as building. |
1000 | |
1001 | === modified file 'lib/lp/buildmaster/tests/test_manager.py' |
1002 | --- lib/lp/buildmaster/tests/test_manager.py 2012-01-01 02:58:52 +0000 |
1003 | +++ lib/lp/buildmaster/tests/test_manager.py 2012-01-03 12:26:30 +0000 |
1004 | @@ -1,8 +1,9 @@ |
1005 | -# Copyright 2009-2010 Canonical Ltd. This software is licensed under the |
1006 | +# Copyright 2009-2011 Canonical Ltd. This software is licensed under the |
1007 | # GNU Affero General Public License version 3 (see the file LICENSE). |
1008 | |
1009 | """Tests for the renovated slave scanner aka BuilddManager.""" |
1010 | |
1011 | +from collections import namedtuple |
1012 | import os |
1013 | import signal |
1014 | import time |
1015 | @@ -34,15 +35,18 @@ |
1016 | SlaveScanner, |
1017 | ) |
1018 | from lp.buildmaster.model.builder import Builder |
1019 | +from lp.buildmaster.model.packagebuild import PackageBuild |
1020 | from lp.buildmaster.tests.harness import BuilddManagerTestSetup |
1021 | from lp.buildmaster.tests.mock_slaves import ( |
1022 | BrokenSlave, |
1023 | BuildingSlave, |
1024 | make_publisher, |
1025 | OkSlave, |
1026 | + WaitingSlave, |
1027 | ) |
1028 | from lp.registry.interfaces.distribution import IDistributionSet |
1029 | from lp.services.config import config |
1030 | +from lp.services.database.transaction_policy import DatabaseTransactionPolicy |
1031 | from lp.services.log.logger import BufferLogger |
1032 | from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet |
1033 | from lp.testing import ( |
1034 | @@ -58,7 +62,10 @@ |
1035 | LaunchpadZopelessLayer, |
1036 | ZopelessDatabaseLayer, |
1037 | ) |
1038 | -from lp.testing.sampledata import BOB_THE_BUILDER_NAME |
1039 | +from lp.testing.sampledata import ( |
1040 | + BOB_THE_BUILDER_NAME, |
1041 | + FROG_THE_BUILDER_NAME, |
1042 | + ) |
1043 | |
1044 | |
1045 | class TestSlaveScannerScan(TestCase): |
1046 | @@ -76,6 +83,8 @@ |
1047 | 'bob' builder. |
1048 | """ |
1049 | super(TestSlaveScannerScan, self).setUp() |
1050 | + self.read_only = DatabaseTransactionPolicy(read_only=True) |
1051 | + |
1052 | # Creating the required chroots needed for dispatching. |
1053 | test_publisher = make_publisher() |
1054 | ubuntu = getUtility(IDistributionSet).getByName('ubuntu') |
1055 | @@ -83,6 +92,15 @@ |
1056 | test_publisher.setUpDefaultDistroSeries(hoary) |
1057 | test_publisher.addFakeChroots() |
1058 | |
1059 | + def _enterReadOnly(self): |
1060 | + """Go into read-only transaction policy.""" |
1061 | + self.read_only.__enter__() |
1062 | + self.addCleanup(self._exitReadOnly) |
1063 | + |
1064 | + def _exitReadOnly(self): |
1065 | + """Leave read-only transaction policy.""" |
1066 | + self.read_only.__exit__(None, None, None) |
1067 | + |
1068 | def _resetBuilder(self, builder): |
1069 | """Reset the given builder and its job.""" |
1070 | |
1071 | @@ -93,6 +111,23 @@ |
1072 | |
1073 | transaction.commit() |
1074 | |
1075 | + def getFreshBuilder(self, slave=None, name=BOB_THE_BUILDER_NAME, |
1076 | + failure_count=0): |
1077 | + """Return a builder. |
1078 | + |
1079 | + The builder is taken from sample data, but reset to a usable state. |
1080 | + Be careful: this is not a proper factory method. Identical calls |
1081 | + return (and reset) the same builder. Don't rely on that though; |
1082 | + maybe someday we'll have a proper factory here. |
1083 | + """ |
1084 | + if slave is None: |
1085 | + slave = OkSlave() |
1086 | + builder = getUtility(IBuilderSet)[name] |
1087 | + self._resetBuilder(builder) |
1088 | + builder.setSlaveForTesting(slave) |
1089 | + builder.failure_count = failure_count |
1090 | + return builder |
1091 | + |
1092 | def assertBuildingJob(self, job, builder, logtail=None): |
1093 | """Assert the given job is building on the given builder.""" |
1094 | from lp.services.job.interfaces.job import JobStatus |
1095 | @@ -107,14 +142,14 @@ |
1096 | self.assertEqual(build.status, BuildStatus.BUILDING) |
1097 | self.assertEqual(job.logtail, logtail) |
1098 | |
1099 | - def _getScanner(self, builder_name=None): |
1100 | + def _getScanner(self, builder_name=None, clock=None): |
1101 | """Instantiate a SlaveScanner object. |
1102 | |
1103 | Replace its default logging handler by a testing version. |
1104 | """ |
1105 | if builder_name is None: |
1106 | builder_name = BOB_THE_BUILDER_NAME |
1107 | - scanner = SlaveScanner(builder_name, BufferLogger()) |
1108 | + scanner = SlaveScanner(builder_name, BufferLogger(), clock=clock) |
1109 | scanner.logger.name = 'slave-scanner' |
1110 | |
1111 | return scanner |
1112 | @@ -130,17 +165,15 @@ |
1113 | def testScanDispatchForResetBuilder(self): |
1114 | # A job gets dispatched to the sampledata builder after it's reset. |
1115 | |
1116 | - # Reset sampledata builder. |
1117 | - builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME] |
1118 | - self._resetBuilder(builder) |
1119 | - builder.setSlaveForTesting(OkSlave()) |
1120 | - # Set this to 1 here so that _checkDispatch can make sure it's |
1121 | - # reset to 0 after a successful dispatch. |
1122 | - builder.failure_count = 1 |
1123 | + # Obtain a builder. Initialize failure count to 1 so that |
1124 | + # _checkDispatch can make sure that a successful dispatch resets |
1125 | + # the count to 0. |
1126 | + builder = self.getFreshBuilder(failure_count=1) |
1127 | |
1128 | # Run 'scan' and check its result. |
1129 | self.layer.txn.commit() |
1130 | self.layer.switchDbUser(config.builddmaster.dbuser) |
1131 | + self._enterReadOnly() |
1132 | scanner = self._getScanner() |
1133 | d = defer.maybeDeferred(scanner.scan) |
1134 | d.addCallback(self._checkDispatch, builder) |
1135 | @@ -153,20 +186,18 @@ |
1136 | to the asynchonous dispatcher and the builder remained active |
1137 | and IDLE. |
1138 | """ |
1139 | - self.assertTrue(slave is None, "Unexpected slave.") |
1140 | + self.assertIs(None, slave, "Unexpected slave.") |
1141 | |
1142 | builder = getUtility(IBuilderSet).get(builder.id) |
1143 | self.assertTrue(builder.builderok) |
1144 | - self.assertTrue(builder.currentjob is None) |
1145 | + self.assertIs(None, builder.currentjob) |
1146 | |
1147 | def testNoDispatchForMissingChroots(self): |
1148 | # When a required chroot is not present the `scan` method |
1149 | # should not return any `RecordingSlaves` to be processed |
1150 | # and the builder used should remain active and IDLE. |
1151 | |
1152 | - # Reset sampledata builder. |
1153 | - builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME] |
1154 | - self._resetBuilder(builder) |
1155 | + builder = self.getFreshBuilder() |
1156 | |
1157 | # Remove hoary/i386 chroot. |
1158 | login('foo.bar@canonical.com') |
1159 | @@ -179,6 +210,7 @@ |
1160 | |
1161 | # Run 'scan' and check its result. |
1162 | self.layer.switchDbUser(config.builddmaster.dbuser) |
1163 | + self._enterReadOnly() |
1164 | scanner = self._getScanner() |
1165 | d = defer.maybeDeferred(scanner.singleCycle) |
1166 | d.addCallback(self._checkNoDispatch, builder) |
1167 | @@ -220,6 +252,7 @@ |
1168 | |
1169 | # Run 'scan' and check its result. |
1170 | self.layer.switchDbUser(config.builddmaster.dbuser) |
1171 | + self._enterReadOnly() |
1172 | scanner = self._getScanner() |
1173 | d = defer.maybeDeferred(scanner.scan) |
1174 | d.addCallback(self._checkJobRescued, builder, job) |
1175 | @@ -255,25 +288,27 @@ |
1176 | |
1177 | # Run 'scan' and check its result. |
1178 | self.layer.switchDbUser(config.builddmaster.dbuser) |
1179 | + self._enterReadOnly() |
1180 | scanner = self._getScanner() |
1181 | d = defer.maybeDeferred(scanner.scan) |
1182 | d.addCallback(self._checkJobUpdated, builder, job) |
1183 | return d |
1184 | |
1185 | def test_scan_with_nothing_to_dispatch(self): |
1186 | - factory = LaunchpadObjectFactory() |
1187 | - builder = factory.makeBuilder() |
1188 | + builder = self.factory.makeBuilder() |
1189 | builder.setSlaveForTesting(OkSlave()) |
1190 | + transaction.commit() |
1191 | + self._enterReadOnly() |
1192 | scanner = self._getScanner(builder_name=builder.name) |
1193 | d = scanner.scan() |
1194 | return d.addCallback(self._checkNoDispatch, builder) |
1195 | |
1196 | def test_scan_with_manual_builder(self): |
1197 | # Reset sampledata builder. |
1198 | - builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME] |
1199 | - self._resetBuilder(builder) |
1200 | - builder.setSlaveForTesting(OkSlave()) |
1201 | + builder = self.getFreshBuilder() |
1202 | builder.manual = True |
1203 | + transaction.commit() |
1204 | + self._enterReadOnly() |
1205 | scanner = self._getScanner() |
1206 | d = scanner.scan() |
1207 | d.addCallback(self._checkNoDispatch, builder) |
1208 | @@ -281,10 +316,10 @@ |
1209 | |
1210 | def test_scan_with_not_ok_builder(self): |
1211 | # Reset sampledata builder. |
1212 | - builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME] |
1213 | - self._resetBuilder(builder) |
1214 | - builder.setSlaveForTesting(OkSlave()) |
1215 | + builder = self.getFreshBuilder() |
1216 | builder.builderok = False |
1217 | + transaction.commit() |
1218 | + self._enterReadOnly() |
1219 | scanner = self._getScanner() |
1220 | d = scanner.scan() |
1221 | # Because the builder is not ok, we can't use _checkNoDispatch. |
1222 | @@ -293,25 +328,27 @@ |
1223 | return d |
1224 | |
1225 | def test_scan_of_broken_slave(self): |
1226 | - builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME] |
1227 | - self._resetBuilder(builder) |
1228 | - builder.setSlaveForTesting(BrokenSlave()) |
1229 | - builder.failure_count = 0 |
1230 | + builder = self.getFreshBuilder(slave=BrokenSlave()) |
1231 | + transaction.commit() |
1232 | + self._enterReadOnly() |
1233 | scanner = self._getScanner(builder_name=builder.name) |
1234 | d = scanner.scan() |
1235 | return assert_fails_with(d, xmlrpclib.Fault) |
1236 | |
1237 | def _assertFailureCounting(self, builder_count, job_count, |
1238 | expected_builder_count, expected_job_count): |
1239 | + # Avoid circular imports. |
1240 | + from lp.buildmaster import manager as manager_module |
1241 | + |
1242 | # If scan() fails with an exception, failure_counts should be |
1243 | # incremented. What we do with the results of the failure |
1244 | # counts is tested below separately, this test just makes sure that |
1245 | # scan() is setting the counts. |
1246 | def failing_scan(): |
1247 | return defer.fail(Exception("fake exception")) |
1248 | + |
1249 | scanner = self._getScanner() |
1250 | scanner.scan = failing_scan |
1251 | - from lp.buildmaster import manager as manager_module |
1252 | self.patch(manager_module, 'assessFailureCounts', FakeMethod()) |
1253 | builder = getUtility(IBuilderSet)[scanner.builder_name] |
1254 | |
1255 | @@ -459,6 +496,60 @@ |
1256 | d.addCallback(check_cancelled, builder, buildqueue) |
1257 | return d |
1258 | |
1259 | + def makeFakeFailure(self): |
1260 | + """Produce a fake failure for use with SlaveScanner._scanFailed.""" |
1261 | + FakeFailure = namedtuple('FakeFailure', ['getErrorMessage', 'check']) |
1262 | + return FakeFailure( |
1263 | + FakeMethod(self.factory.getUniqueString()), |
1264 | + FakeMethod(True)) |
1265 | + |
1266 | + def test_interleaved_success_and_failure_do_not_interfere(self): |
1267 | + # It's possible for one builder to fail while another continues |
1268 | + # to function properly. When that happens, the failed builder |
1269 | + # may cause database changes to be rolled back. But that does |
1270 | + # not affect the functioning builder. |
1271 | + clock = task.Clock() |
1272 | + |
1273 | + broken_builder = self.getFreshBuilder( |
1274 | + slave=BrokenSlave(), name=BOB_THE_BUILDER_NAME) |
1275 | + broken_scanner = self._getScanner(builder_name=broken_builder.name) |
1276 | + good_builder = self.getFreshBuilder( |
1277 | + slave=WaitingSlave(), name=FROG_THE_BUILDER_NAME) |
1278 | + good_build = self.factory.makeBinaryPackageBuild( |
1279 | + distroarchseries=self.factory.makeDistroArchSeries()) |
1280 | + |
1281 | + # The good build is being handled by the good builder. |
1282 | + buildqueue = good_build.queueBuild() |
1283 | + buildqueue.builder = good_builder |
1284 | + |
1285 | + removeSecurityProxy(good_build.build_farm_job).date_started = UTC_NOW |
1286 | + |
1287 | + # The good builder requests information from a successful build, |
1288 | + # and up receiving it, updates the build's metadata. |
1289 | + # Our dependencies string goes into the build, and its |
1290 | + # date_finished will be set. |
1291 | + dependencies = self.factory.getUniqueString() |
1292 | + PackageBuild.storeBuildInfo( |
1293 | + good_build, None, {'dependencies': dependencies}) |
1294 | + clock.advance(1) |
1295 | + |
1296 | + # The broken scanner experiences a failure before the good |
1297 | + # scanner is receiving its data. This aborts the ongoing |
1298 | + # transaction. |
1299 | + # As a somewhat weird example, if the builder changed its own |
1300 | + # title, that change will be rolled back. |
1301 | + original_broken_builder_title = broken_builder.title |
1302 | + broken_builder.title = self.factory.getUniqueString() |
1303 | + broken_scanner._scanFailed(self.makeFakeFailure()) |
1304 | + |
1305 | + # The work done by the good scanner is retained. The |
1306 | + # storeBuildInfo code committed it. |
1307 | + self.assertEqual(dependencies, good_build.dependencies) |
1308 | + self.assertIsNot(None, good_build.date_finished) |
1309 | + |
1310 | + # The work done by the broken scanner is rolled back. |
1311 | + self.assertEqual(original_broken_builder_title, broken_builder.title) |
1312 | + |
1313 | |
1314 | class TestCancellationChecking(TestCaseWithFactory): |
1315 | """Unit tests for the checkCancellation method.""" |
1316 | |
1317 | === modified file 'lib/lp/buildmaster/tests/test_packagebuild.py' |
1318 | --- lib/lp/buildmaster/tests/test_packagebuild.py 2011-12-30 06:14:56 +0000 |
1319 | +++ lib/lp/buildmaster/tests/test_packagebuild.py 2012-01-03 12:26:30 +0000 |
1320 | @@ -1,4 +1,4 @@ |
1321 | -# Copyright 2010 Canonical Ltd. This software is licensed under the |
1322 | +# Copyright 2010-2011 Canonical Ltd. This software is licensed under the |
1323 | # GNU Affero General Public License version 3 (see the file LICENSE). |
1324 | |
1325 | """Tests for `IPackageBuild`.""" |
1326 | @@ -12,6 +12,7 @@ |
1327 | import tempfile |
1328 | |
1329 | from storm.store import Store |
1330 | +from testtools.deferredruntest import AsynchronousDeferredRunTest |
1331 | from zope.component import getUtility |
1332 | from zope.security.interfaces import Unauthorized |
1333 | from zope.security.proxy import removeSecurityProxy |
1334 | @@ -26,8 +27,10 @@ |
1335 | IPackageBuildSet, |
1336 | IPackageBuildSource, |
1337 | ) |
1338 | +from lp.buildmaster.model.builder import BuilderSlave |
1339 | from lp.buildmaster.model.buildfarmjob import BuildFarmJob |
1340 | from lp.buildmaster.model.packagebuild import PackageBuild |
1341 | +from lp.buildmaster.testing import BuilddManagerTestFixture |
1342 | from lp.buildmaster.tests.mock_slaves import WaitingSlave |
1343 | from lp.registry.interfaces.pocket import PackagePublishingPocket |
1344 | from lp.services.config import config |
1345 | @@ -281,12 +284,10 @@ |
1346 | |
1347 | |
1348 | class TestHandleStatusMixin: |
1349 | - """Tests for `IPackageBuild`s handleStatus method. |
1350 | - |
1351 | - This should be run with a Trial TestCase. |
1352 | - """ |
1353 | + """Tests for `IPackageBuild`s handleStatus method.""" |
1354 | |
1355 | layer = LaunchpadZopelessLayer |
1356 | + run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20) |
1357 | |
1358 | def makeBuild(self): |
1359 | """Allow classes to override the build with which the test runs.""" |
1360 | @@ -303,7 +304,7 @@ |
1361 | self.build.buildqueue_record.setDateStarted(UTC_NOW) |
1362 | self.slave = WaitingSlave('BuildStatus.OK') |
1363 | self.slave.valid_file_hashes.append('test_file_hash') |
1364 | - builder.setSlaveForTesting(self.slave) |
1365 | + self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(self.slave)) |
1366 | |
1367 | # We overwrite the buildmaster root to use a temp directory. |
1368 | tempdir = tempfile.mkdtemp() |
1369 | @@ -321,6 +322,8 @@ |
1370 | removeSecurityProxy(self.build).verifySuccessfulUpload = FakeMethod( |
1371 | result=True) |
1372 | |
1373 | + self.useFixture(BuilddManagerTestFixture()) |
1374 | + |
1375 | def assertResultCount(self, count, result): |
1376 | self.assertEquals( |
1377 | 1, len(os.listdir(os.path.join(self.upload_root, result)))) |
1378 | @@ -344,7 +347,7 @@ |
1379 | def got_status(ignored): |
1380 | self.assertEqual(BuildStatus.FAILEDTOUPLOAD, self.build.status) |
1381 | self.assertResultCount(0, "failed") |
1382 | - self.assertIdentical(None, self.build.buildqueue_record) |
1383 | + self.assertIs(None, self.build.buildqueue_record) |
1384 | |
1385 | d = self.build.handleStatus('OK', None, { |
1386 | 'filemap': {'/tmp/myfile.py': 'test_file_hash'}, |
1387 | @@ -365,7 +368,8 @@ |
1388 | |
1389 | def test_handleStatus_OK_sets_build_log(self): |
1390 | # The build log is set during handleStatus. |
1391 | - removeSecurityProxy(self.build).log = None |
1392 | + with BuilddManagerTestFixture.extraSetUp(): |
1393 | + removeSecurityProxy(self.build).log = None |
1394 | self.assertEqual(None, self.build.log) |
1395 | d = self.build.handleStatus('OK', None, { |
1396 | 'filemap': {'myfile.py': 'test_file_hash'}, |
1397 | @@ -386,14 +390,10 @@ |
1398 | |
1399 | def got_status(ignored): |
1400 | if expected_notification: |
1401 | - self.failIf( |
1402 | - len(pop_notifications()) == 0, |
1403 | - "No notifications received") |
1404 | + self.assertNotEqual( |
1405 | + 0, len(pop_notifications()), "No notifications received.") |
1406 | else: |
1407 | - self.failIf( |
1408 | - len(pop_notifications()) > 0, |
1409 | - "Notifications received") |
1410 | - |
1411 | + self.assertContentEqual([], pop_notifications()) |
1412 | d = self.build.handleStatus(status, None, {}) |
1413 | return d.addCallback(got_status) |
1414 | |
1415 | @@ -408,7 +408,9 @@ |
1416 | |
1417 | def test_date_finished_set(self): |
1418 | # The date finished is updated during handleStatus_OK. |
1419 | - removeSecurityProxy(self.build).date_finished = None |
1420 | + with BuilddManagerTestFixture.extraSetUp(): |
1421 | + removeSecurityProxy(self.build).date_finished = None |
1422 | + |
1423 | self.assertEqual(None, self.build.date_finished) |
1424 | d = self.build.handleStatus('OK', None, { |
1425 | 'filemap': {'myfile.py': 'test_file_hash'}, |
1426 | |
1427 | === modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py' |
1428 | --- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py 2012-01-01 02:58:52 +0000 |
1429 | +++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py 2012-01-03 12:26:30 +0000 |
1430 | @@ -13,14 +13,15 @@ |
1431 | |
1432 | from pytz import utc |
1433 | from storm.locals import Store |
1434 | +from testtools.deferredruntest import AsynchronousDeferredRunTest |
1435 | import transaction |
1436 | -from twisted.trial.unittest import TestCase as TrialTestCase |
1437 | from zope.component import getUtility |
1438 | from zope.security.proxy import removeSecurityProxy |
1439 | |
1440 | from lp.app.errors import NotFoundError |
1441 | from lp.buildmaster.enums import BuildStatus |
1442 | from lp.buildmaster.interfaces.buildqueue import IBuildQueue |
1443 | +from lp.buildmaster.model.builder import BuilderSlave |
1444 | from lp.buildmaster.model.buildfarmjob import BuildFarmJob |
1445 | from lp.buildmaster.model.packagebuild import PackageBuild |
1446 | from lp.buildmaster.tests.mock_slaves import WaitingSlave |
1447 | @@ -588,14 +589,11 @@ |
1448 | self.assertEquals(0, len(notifications)) |
1449 | |
1450 | |
1451 | -class TestBuildNotifications(TrialTestCase): |
1452 | +class TestBuildNotifications(TestCaseWithFactory): |
1453 | |
1454 | layer = LaunchpadZopelessLayer |
1455 | |
1456 | - def setUp(self): |
1457 | - super(TestBuildNotifications, self).setUp() |
1458 | - from lp.testing.factory import LaunchpadObjectFactory |
1459 | - self.factory = LaunchpadObjectFactory() |
1460 | + run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20) |
1461 | |
1462 | def prepare_build(self, fake_successful_upload=False): |
1463 | queue_record = self.factory.makeSourcePackageRecipeBuildJob() |
1464 | @@ -608,7 +606,7 @@ |
1465 | result=True) |
1466 | queue_record.builder = self.factory.makeBuilder() |
1467 | slave = WaitingSlave('BuildStatus.OK') |
1468 | - queue_record.builder.setSlaveForTesting(slave) |
1469 | + self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave)) |
1470 | return build |
1471 | |
1472 | def assertDeferredNotifyCount(self, status, build, expected_count): |
1473 | @@ -666,5 +664,5 @@ |
1474 | |
1475 | |
1476 | class TestHandleStatusForSPRBuild( |
1477 | - MakeSPRecipeBuildMixin, TestHandleStatusMixin, TrialTestCase): |
1478 | + MakeSPRecipeBuildMixin, TestHandleStatusMixin, TestCaseWithFactory): |
1479 | """IPackageBuild.handleStatus works with SPRecipe builds.""" |
1480 | |
1481 | === modified file 'lib/lp/services/database/tests/test_transaction_policy.py' |
1482 | --- lib/lp/services/database/tests/test_transaction_policy.py 2012-01-01 02:58:52 +0000 |
1483 | +++ lib/lp/services/database/tests/test_transaction_policy.py 2012-01-03 12:26:30 +0000 |
1484 | @@ -76,16 +76,26 @@ |
1485 | |
1486 | self.assertRaises(InternalError, make_forbidden_update) |
1487 | |
1488 | - def test_will_not_start_in_ongoing_transaction(self): |
1489 | - # You cannot enter a DatabaseTransactionPolicy while already in |
1490 | - # a transaction. |
1491 | + def test_will_not_go_read_only_when_read_write_transaction_ongoing(self): |
1492 | + # You cannot enter a read-only DatabaseTransactionPolicy while in an |
1493 | + # active read-write transaction. |
1494 | def enter_policy(): |
1495 | - with DatabaseTransactionPolicy(): |
1496 | + with DatabaseTransactionPolicy(read_only=True): |
1497 | pass |
1498 | |
1499 | self.writeToDatabase() |
1500 | self.assertRaises(TransactionInProgress, enter_policy) |
1501 | |
1502 | + def test_will_go_read_write_when_read_write_transaction_ongoing(self): |
1503 | + # You can enter a read-write DatabaseTransactionPolicy while in an |
1504 | + # active read-write transaction. |
1505 | + def enter_policy(): |
1506 | + with DatabaseTransactionPolicy(read_only=False): |
1507 | + pass |
1508 | + |
1509 | + self.writeToDatabase() |
1510 | + enter_policy() # No exception. |
1511 | + |
1512 | def test_successful_exit_requires_commit_or_abort(self): |
1513 | # If a read-write policy exits normally (which would probably |
1514 | # indicate successful completion of its code), it requires that |
1515 | |
1516 | === modified file 'lib/lp/services/database/transaction_policy.py' |
1517 | --- lib/lp/services/database/transaction_policy.py 2011-12-30 06:14:56 +0000 |
1518 | +++ lib/lp/services/database/transaction_policy.py 2012-01-03 12:26:30 +0000 |
1519 | @@ -92,9 +92,18 @@ |
1520 | |
1521 | :raise TransactionInProgress: if a transaction was already ongoing. |
1522 | """ |
1523 | - self._checkNoTransaction( |
1524 | - "Entered DatabaseTransactionPolicy while in a transaction.") |
1525 | + # We must check the transaction status before checking the current |
1526 | + # policy because getting the policy causes a status change. |
1527 | + in_transaction = self._isInTransaction() |
1528 | self.previous_policy = self._getCurrentPolicy() |
1529 | + # If the current transaction is read-write and we're moving to |
1530 | + # read-only then we should check for a transaction. If the current |
1531 | + # policy is read-only then we don't care if a transaction is in |
1532 | + # progress. |
1533 | + if in_transaction and self.read_only and not self.previous_policy: |
1534 | + raise TransactionInProgress( |
1535 | + "Attempting to enter a read-only transaction while holding " |
1536 | + "open a read-write transaction.") |
1537 | self._setPolicy(self.read_only) |
1538 | # Commit should include the policy itself. If this breaks |
1539 | # because the transaction was already in a failed state before |
1540 | @@ -133,8 +142,11 @@ |
1541 | def _isInTransaction(self): |
1542 | """Is our store currently in a transaction?""" |
1543 | pg_connection = self.store._connection._raw_connection |
1544 | - status = pg_connection.get_transaction_status() |
1545 | - return status != TRANSACTION_STATUS_IDLE |
1546 | + if pg_connection is None: |
1547 | + return False |
1548 | + else: |
1549 | + status = pg_connection.get_transaction_status() |
1550 | + return status != TRANSACTION_STATUS_IDLE |
1551 | |
1552 | def _checkNoTransaction(self, error_msg): |
1553 | """Verify that no transaction is ongoing. |
1554 | |
1555 | === modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py' |
1556 | --- lib/lp/soyuz/tests/test_binarypackagebuild.py 2012-01-01 02:58:52 +0000 |
1557 | +++ lib/lp/soyuz/tests/test_binarypackagebuild.py 2012-01-03 12:26:30 +0000 |
1558 | @@ -1,4 +1,4 @@ |
1559 | -# Copyright 2009-2010 Canonical Ltd. This software is licensed under the |
1560 | +# Copyright 2009-2011 Canonical Ltd. This software is licensed under the |
1561 | # GNU Affero General Public License version 3 (see the file LICENSE). |
1562 | |
1563 | """Test Build features.""" |
1564 | @@ -10,7 +10,6 @@ |
1565 | |
1566 | import pytz |
1567 | from storm.store import Store |
1568 | -from twisted.trial.unittest import TestCase as TrialTestCase |
1569 | from zope.component import getUtility |
1570 | from zope.security.proxy import removeSecurityProxy |
1571 | |
1572 | @@ -18,6 +17,7 @@ |
1573 | from lp.buildmaster.interfaces.builder import IBuilderSet |
1574 | from lp.buildmaster.interfaces.buildqueue import IBuildQueue |
1575 | from lp.buildmaster.interfaces.packagebuild import IPackageBuild |
1576 | +from lp.buildmaster.model.builder import BuilderSlave |
1577 | from lp.buildmaster.model.buildqueue import BuildQueue |
1578 | from lp.buildmaster.tests.mock_slaves import WaitingSlave |
1579 | from lp.buildmaster.tests.test_packagebuild import ( |
1580 | @@ -48,6 +48,7 @@ |
1581 | logout, |
1582 | TestCaseWithFactory, |
1583 | ) |
1584 | +from lp.testing.fakemethod import FakeMethod |
1585 | from lp.testing.layers import ( |
1586 | DatabaseFunctionalLayer, |
1587 | LaunchpadZopelessLayer, |
1588 | @@ -522,7 +523,9 @@ |
1589 | self.build = gedit_src_hist.createMissingBuilds()[0] |
1590 | |
1591 | self.builder = self.factory.makeBuilder() |
1592 | - self.builder.setSlaveForTesting(WaitingSlave('BuildStatus.OK')) |
1593 | + self.patch( |
1594 | + BuilderSlave, 'makeBuilderSlave', |
1595 | + FakeMethod(WaitingSlave('BuildStatus.OK'))) |
1596 | self.build.buildqueue_record.markAsBuilding(self.builder) |
1597 | |
1598 | def testDependencies(self): |
1599 | @@ -568,7 +571,7 @@ |
1600 | |
1601 | |
1602 | class TestHandleStatusForBinaryPackageBuild( |
1603 | - MakeBinaryPackageBuildMixin, TestHandleStatusMixin, TrialTestCase): |
1604 | + MakeBinaryPackageBuildMixin, TestHandleStatusMixin, TestCaseWithFactory): |
1605 | """IPackageBuild.handleStatus works with binary builds.""" |
1606 | |
1607 | |
1608 | |
1609 | === modified file 'lib/lp/translations/model/translationtemplatesbuildbehavior.py' |
1610 | --- lib/lp/translations/model/translationtemplatesbuildbehavior.py 2012-01-01 02:58:52 +0000 |
1611 | +++ lib/lp/translations/model/translationtemplatesbuildbehavior.py 2012-01-03 12:26:30 +0000 |
1612 | @@ -1,4 +1,4 @@ |
1613 | -# Copyright 2010 Canonical Ltd. This software is licensed under the |
1614 | +# Copyright 2010-2011 Canonical Ltd. This software is licensed under the |
1615 | # GNU Affero General Public License version 3 (see the file LICENSE). |
1616 | |
1617 | """An `IBuildFarmJobBehavior` for `TranslationTemplatesBuildJob`. |
1618 | @@ -16,6 +16,7 @@ |
1619 | import tempfile |
1620 | |
1621 | import pytz |
1622 | +import transaction |
1623 | from twisted.internet import defer |
1624 | from zope.component import getUtility |
1625 | from zope.interface import implements |
1626 | @@ -28,6 +29,7 @@ |
1627 | ) |
1628 | from lp.buildmaster.model.buildfarmjobbehavior import BuildFarmJobBehaviorBase |
1629 | from lp.registry.interfaces.productseries import IProductSeriesSet |
1630 | +from lp.services.database.transaction_policy import DatabaseTransactionPolicy |
1631 | from lp.translations.interfaces.translationimportqueue import ( |
1632 | ITranslationImportQueue, |
1633 | ) |
1634 | @@ -132,13 +134,16 @@ |
1635 | def storeBuildInfo(build, queue_item, build_status): |
1636 | """See `IPackageBuild`.""" |
1637 | def got_log(lfa_id): |
1638 | - build.build.log = lfa_id |
1639 | - build.build.builder = queue_item.builder |
1640 | - build.build.date_started = queue_item.date_started |
1641 | - # XXX cprov 20060615 bug=120584: Currently buildduration includes |
1642 | - # the scanner latency, it should really be asking the slave for |
1643 | - # the duration spent building locally. |
1644 | - build.build.date_finished = datetime.datetime.now(pytz.UTC) |
1645 | + transaction.commit() |
1646 | + with DatabaseTransactionPolicy(read_only=False): |
1647 | + build.build.log = lfa_id |
1648 | + build.build.builder = queue_item.builder |
1649 | + build.build.date_started = queue_item.date_started |
1650 | + # XXX cprov 20060615 bug=120584: Currently buildduration |
1651 | + # includes the scanner latency. It should really be |
1652 | + # asking the slave for the duration spent building locally. |
1653 | + build.build.date_finished = datetime.datetime.now(pytz.UTC) |
1654 | + transaction.commit() |
1655 | |
1656 | d = build.getLogFromSlave(build, queue_item) |
1657 | return d.addCallback(got_log) |
<bigjools> allenap: in the tests in your branch, it's probably worth refactoring the bit that sets properties on objects in a r/w transaction us_OK_sets_ build_log?
<allenap> bigjools: Erm, which bit?
<allenap> bigjools: Like in test_handleStat
<bigjools> allenap: line 72/83 of the diff
<bigjools> allenap: I suspect we'll need to do that a lot more in the future
<allenap> bigjools: I don't know what a better way would be. I could instead enter read-only mode in each test individually (via a fixture) I guess.
<bigjools> allenap: I was thinking just a test helper
<bigjools> like setattr
<bigjools> but does the whole transactionny thing
<allenap> bigjools: With the removeSecurityProxy thing too I assume.
<bigjools> allenap: no, the caller can do that
<allenap> bigjools: Okay, I think I have a cool way to do that.