Merge lp:~cjwatson/launchpad-buildd/fix-abort into lp:launchpad-buildd
- fix-abort
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | William Grant |
Approved revision: | 64 |
Merged at revision: | 68 |
Proposed branch: | lp:~cjwatson/launchpad-buildd/fix-abort |
Merge into: | lp:launchpad-buildd |
Diff against target: |
1160 lines (+562/-180) 14 files modified
Makefile (+1/-0) debian/changelog (+7/-0) debian/upgrade-config (+19/-1) lpbuildd/binarypackage.py (+9/-6) lpbuildd/debian.py (+34/-20) lpbuildd/slave.py (+123/-48) lpbuildd/sourcepackagerecipe.py (+6/-2) lpbuildd/tests/buildd-slave-test.conf (+1/-2) lpbuildd/tests/fakeslave.py (+104/-0) lpbuildd/tests/harness.py (+1/-0) lpbuildd/tests/test_binarypackage.py (+206/-0) lpbuildd/tests/test_translationtemplatesbuildmanager.py (+44/-96) lpbuildd/translationtemplates.py (+6/-4) template-buildd-slave.conf (+1/-1) |
To merge this branch: | bzr merge lp:~cjwatson/launchpad-buildd/fix-abort |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
William Grant | code | Approve | |
Adam Conrad (community) | Approve | ||
Review via email: mp+177003@code.launchpad.net |
Commit message
Make abort work properly, calling scan-for-processes to kill all processes in the chroot.
Description of the change
Make the abort operation work, so that we have a hope of being able to cancel builds.
This turned out to be gratuitously complicated, involving moving scan-for-processes configuration up to the slave, rearranging how reaping is handled in the state machine so that it can reasonably easily happen at multiple stages, and introducing a new BuildStatus.ABORTED state (which should eventually supersede BuilderStatus.
- 60. By Colin Watson
-
Handle calls to abort while already ABORTING.
- 61. By Colin Watson
-
Avoid awkward Python conditional expression syntax.
- 62. By Colin Watson
-
Move processscanpath to the end of the allmanagers section on upgrade, not the start.
- 63. By Colin Watson
-
Merge trunk.
- 64. By Colin Watson
-
Merge trunk.
William Grant (wgrant) : | # |
Preview Diff
1 | === modified file 'Makefile' |
2 | --- Makefile 2011-11-21 01:47:22 +0000 |
3 | +++ Makefile 2013-07-30 11:30:43 +0000 |
4 | @@ -27,6 +27,7 @@ |
5 | # a Launchpad checkout. |
6 | check: |
7 | PYTHONPATH=$(PWD):$(PYTHONPATH) $(PYTHON) -m testtools.run -v \ |
8 | + lpbuildd.tests.test_binarypackage \ |
9 | lpbuildd.tests.test_buildd_slave \ |
10 | lpbuildd.tests.test_check_implicit_pointer_functions \ |
11 | lpbuildd.tests.test_harness \ |
12 | |
13 | === modified file 'debian/changelog' |
14 | --- debian/changelog 2013-07-29 23:48:58 +0000 |
15 | +++ debian/changelog 2013-07-30 11:30:43 +0000 |
16 | @@ -1,5 +1,6 @@ |
17 | launchpad-buildd (115) UNRELEASED; urgency=low |
18 | |
19 | + [ Adam Conrad ] |
20 | * Short the readlink call in scan-for-process with a true to avoid |
21 | prematurely exiting the process scan when tripping over zombies. |
22 | * Write to temporary cache files and then rename after validation |
23 | @@ -12,6 +13,12 @@ |
24 | * Strip trailing whitespace in buildd-genconfig because I'm anal. |
25 | * Mangle recipe versions to match backports policy (LP: #1095103) |
26 | |
27 | + [ Colin Watson ] |
28 | + * Move scan-for-processes up to the top-level slave code so that it is |
29 | + available for more general use. |
30 | + * Make abort work properly, calling scan-for-processes to kill all |
31 | + processes in the chroot. |
32 | + |
33 | -- Adam Conrad <adconrad@ubuntu.com> Mon, 29 Apr 2013 15:47:20 -0600 |
34 | |
35 | launchpad-buildd (114) hardy; urgency=low |
36 | |
37 | === modified file 'debian/upgrade-config' |
38 | --- debian/upgrade-config 2011-12-05 02:39:05 +0000 |
39 | +++ debian/upgrade-config 2013-07-30 11:30:43 +0000 |
40 | @@ -7,7 +7,6 @@ |
41 | |
42 | import re |
43 | import sys |
44 | -import os |
45 | import subprocess |
46 | |
47 | (old_version, conf_file) = sys.argv[1:] |
48 | @@ -114,6 +113,23 @@ |
49 | in_file.close() |
50 | out_file.close() |
51 | |
52 | +def upgrade_to_115(): |
53 | + print "Upgrading %s to version 115" % conf_file |
54 | + subprocess.call(["mv", conf_file, conf_file+"-prev115~"]) |
55 | + in_allmanagers = False |
56 | + in_file = open(conf_file+"-prev115~", "r") |
57 | + out_file = open(conf_file, "w") |
58 | + for line in in_file: |
59 | + if line.startswith("[allmanagers]"): |
60 | + in_allmanagers = True |
61 | + elif in_allmanagers and (line.startswith("[") or not line.strip()): |
62 | + out_file.write("processscanpath = /usr/share/launchpad-buildd/slavebin/scan-for-processes\n") |
63 | + in_allmanagers = False |
64 | + if not line.startswith("processscanpath = "): |
65 | + out_file.write(line) |
66 | + in_file.close() |
67 | + out_file.close() |
68 | + |
69 | if __name__ == "__main__": |
70 | old_version = re.sub(r'[~-].*', '', old_version) |
71 | if int(old_version) < 12: |
72 | @@ -132,4 +148,6 @@ |
73 | upgrade_to_63() |
74 | if int(old_version) < 110: |
75 | upgrade_to_110() |
76 | + if int(old_version) < 115: |
77 | + upgrade_to_115() |
78 | |
79 | |
80 | === modified file 'lpbuildd/binarypackage.py' |
81 | --- lpbuildd/binarypackage.py 2011-11-10 04:55:02 +0000 |
82 | +++ lpbuildd/binarypackage.py 2013-07-30 11:30:43 +0000 |
83 | @@ -39,8 +39,8 @@ |
84 | |
85 | initial_build_state = BinaryPackageBuildState.SBUILD |
86 | |
87 | - def __init__(self, slave, buildid): |
88 | - DebianBuildManager.__init__(self, slave, buildid) |
89 | + def __init__(self, slave, buildid, **kwargs): |
90 | + DebianBuildManager.__init__(self, slave, buildid, **kwargs) |
91 | self._sbuildpath = slave._config.get("binarypackagemanager", "sbuildpath") |
92 | self._sbuildargs = slave._config.get("binarypackagemanager", |
93 | "sbuildargs").split(" ") |
94 | @@ -124,10 +124,13 @@ |
95 | print("Returning build status: BUILDERFAIL") |
96 | self._slave.builderFail() |
97 | self.alreadyfailed = True |
98 | - self._state = DebianBuildState.REAP |
99 | - self.doReapProcesses() |
100 | + self.doReapProcesses(self._state) |
101 | else: |
102 | print("Returning build status: OK") |
103 | self.gatherResults() |
104 | - self._state = DebianBuildState.REAP |
105 | - self.doReapProcesses() |
106 | + self.doReapProcesses(self._state) |
107 | + |
108 | + def iterateReap_SBUILD(self, success): |
109 | + """Finished reaping after sbuild run.""" |
110 | + self._state = DebianBuildState.UMOUNT |
111 | + self.doUnmounting() |
112 | |
113 | === modified file 'lpbuildd/debian.py' |
114 | --- lpbuildd/debian.py 2011-11-18 04:17:27 +0000 |
115 | +++ lpbuildd/debian.py 2013-07-30 11:30:43 +0000 |
116 | @@ -22,7 +22,6 @@ |
117 | MOUNT = "MOUNT" |
118 | SOURCES = "SOURCES" |
119 | UPDATE = "UPDATE" |
120 | - REAP = "REAP" |
121 | UMOUNT = "UMOUNT" |
122 | CLEANUP = "CLEANUP" |
123 | |
124 | @@ -30,10 +29,9 @@ |
125 | class DebianBuildManager(BuildManager): |
126 | """Base behaviour for Debian chrooted builds.""" |
127 | |
128 | - def __init__(self, slave, buildid): |
129 | - BuildManager.__init__(self, slave, buildid) |
130 | + def __init__(self, slave, buildid, **kwargs): |
131 | + BuildManager.__init__(self, slave, buildid, **kwargs) |
132 | self._updatepath = slave._config.get("debianmanager", "updatepath") |
133 | - self._scanpath = slave._config.get("debianmanager", "processscanpath") |
134 | self._sourcespath = slave._config.get("debianmanager", "sourcespath") |
135 | self._cachepath = slave._config.get("slave","filecache") |
136 | self._state = DebianBuildState.INIT |
137 | @@ -74,10 +72,6 @@ |
138 | """ |
139 | raise NotImplementedError() |
140 | |
141 | - def doReapProcesses(self): |
142 | - """Reap any processes left lying around in the chroot.""" |
143 | - self.runSubProcess( self._scanpath, [self._scanpath, self._buildid] ) |
144 | - |
145 | @staticmethod |
146 | def _parseChangesFile(linesIter): |
147 | """A generator that iterates over files listed in a changes file. |
148 | @@ -98,7 +92,7 @@ |
149 | |
150 | def getChangesFilename(self): |
151 | changes = self._dscfile[:-4] + "_" + self.arch_tag + ".changes" |
152 | - return get_build_path(self._buildid, changes) |
153 | + return get_build_path(self.home, self._buildid, changes) |
154 | |
155 | def gatherResults(self): |
156 | """Gather the results of the build and add them to the file cache. |
157 | @@ -113,7 +107,8 @@ |
158 | seenfiles = False |
159 | |
160 | for fn in self._parseChangesFile(chfile): |
161 | - self._slave.addWaitingFile(get_build_path(self._buildid, fn)) |
162 | + self._slave.addWaitingFile( |
163 | + get_build_path(self.home, self._buildid, fn)) |
164 | |
165 | chfile.close() |
166 | |
167 | @@ -128,6 +123,15 @@ |
168 | raise ValueError, "Unknown internal state " + self._state |
169 | func(success) |
170 | |
171 | + def iterateReap(self, state, success): |
172 | + print ("Iterating with success flag %s against stage %s after reaping " |
173 | + "processes" |
174 | + % (success, state)) |
175 | + func = getattr(self, "iterateReap_" + state, None) |
176 | + if func is None: |
177 | + raise ValueError, "Unknown internal post-reap state " + state |
178 | + func(success) |
179 | + |
180 | def iterate_INIT(self, success): |
181 | """Just finished initializing the build.""" |
182 | if success != 0: |
183 | @@ -183,26 +187,29 @@ |
184 | if not self.alreadyfailed: |
185 | self._slave.chrootFail() |
186 | self.alreadyfailed = True |
187 | - self._state = DebianBuildState.REAP |
188 | - self.doReapProcesses() |
189 | + self.doReapProcesses(self._state) |
190 | else: |
191 | self._state = DebianBuildState.UPDATE |
192 | self.doUpdateChroot() |
193 | |
194 | + def iterateReap_SOURCES(self, success): |
195 | + """Just finished reaping after failure to overwrite sources.list.""" |
196 | + self._state = DebianBuildState.UMOUNT |
197 | + self.doUnmounting() |
198 | + |
199 | def iterate_UPDATE(self, success): |
200 | """Just finished updating the chroot.""" |
201 | if success != 0: |
202 | if not self.alreadyfailed: |
203 | self._slave.chrootFail() |
204 | self.alreadyfailed = True |
205 | - self._state = DebianBuildState.REAP |
206 | - self.doReapProcesses() |
207 | + self.doReapProcesses(self._state) |
208 | else: |
209 | self._state = self.initial_build_state |
210 | self.doRunBuild() |
211 | |
212 | - def iterate_REAP(self, success): |
213 | - """Finished reaping processes; ignore error returns.""" |
214 | + def iterateReap_UPDATE(self, success): |
215 | + """Just finished reaping after failure to update the chroot.""" |
216 | self._state = DebianBuildState.UMOUNT |
217 | self.doUnmounting() |
218 | |
219 | @@ -227,13 +234,20 @@ |
220 | self._slave.buildOK() |
221 | self._slave.buildComplete() |
222 | |
223 | - |
224 | -def get_build_path(build_id, *extra): |
225 | + def abortReap(self): |
226 | + """Abort by killing all processes in the chroot, as hard as we can. |
227 | + |
228 | + Overridden here to handle state management. |
229 | + """ |
230 | + self.doReapProcesses(self._state, notify=False) |
231 | + |
232 | + |
233 | +def get_build_path(home, build_id, *extra): |
234 | """Generate a path within the build directory. |
235 | |
236 | + :param home: the user's home directory. |
237 | :param build_id: the build id to use. |
238 | :param extra: the extra path segments within the build directory. |
239 | :return: the generated path. |
240 | """ |
241 | - return os.path.join( |
242 | - os.environ["HOME"], "build-" + build_id, *extra) |
243 | + return os.path.join(home, "build-" + build_id, *extra) |
244 | |
245 | === modified file 'lpbuildd/slave.py' |
246 | --- lpbuildd/slave.py 2013-07-30 11:27:43 +0000 |
247 | +++ lpbuildd/slave.py 2013-07-30 11:30:43 +0000 |
248 | @@ -8,6 +8,7 @@ |
249 | |
250 | __metaclass__ = type |
251 | |
252 | +from functools import partial |
253 | import hashlib |
254 | import os |
255 | import re |
256 | @@ -15,7 +16,7 @@ |
257 | import xmlrpclib |
258 | |
259 | from twisted.internet import protocol |
260 | -from twisted.internet import reactor |
261 | +from twisted.internet import reactor as default_reactor |
262 | from twisted.internet import process |
263 | from twisted.web import xmlrpc |
264 | |
265 | @@ -52,7 +53,8 @@ |
266 | def __init__(self, slave, callback): |
267 | self.slave = slave |
268 | self.notify = callback |
269 | - self.killCall = None |
270 | + self.builderFailCall = None |
271 | + self.ignore = False |
272 | |
273 | def outReceived(self, data): |
274 | """Pass on stdout data to the log.""" |
275 | @@ -67,30 +69,27 @@ |
276 | def processEnded(self, statusobject): |
277 | """This method is called when a child process got terminated. |
278 | |
279 | - Three actions are required at this point: identify if we are within an |
280 | - "aborting" process, eliminate pending calls to "kill" and invoke the |
281 | - programmed notification callback. We only really care about invoking |
282 | - the notification callback last thing in this method. The order |
283 | - of the rest of the method is not critical. |
284 | + Two actions are required at this point: eliminate pending calls to |
285 | + "builderFail", and invoke the programmed notification callback. The |
286 | + notification callback must be invoked last. |
287 | """ |
288 | - # finishing the ABORTING workflow |
289 | - if self.slave.builderstatus == BuilderStatus.ABORTING: |
290 | - self.slave.builderstatus = BuilderStatus.ABORTED |
291 | + if self.ignore: |
292 | + # The build manager no longer cares about this process. |
293 | + return |
294 | |
295 | - # check if there is a pending request for kill the process, |
296 | - # in afirmative case simply cancel this request since it |
297 | - # already died. |
298 | - if self.killCall and self.killCall.active(): |
299 | - self.killCall.cancel() |
300 | + # Since the process terminated, we don't need to fail the builder. |
301 | + if self.builderFailCall and self.builderFailCall.active(): |
302 | + self.builderFailCall.cancel() |
303 | |
304 | # notify the slave, it'll perform the required actions |
305 | - self.notify(statusobject.value.exitCode) |
306 | + if self.notify is not None: |
307 | + self.notify(statusobject.value.exitCode) |
308 | |
309 | |
310 | class BuildManager(object): |
311 | """Build Daemon slave build manager abstract parent""" |
312 | |
313 | - def __init__(self, slave, buildid): |
314 | + def __init__(self, slave, buildid, reactor=None): |
315 | """Create a BuildManager. |
316 | |
317 | :param slave: A `BuildDSlave`. |
318 | @@ -99,20 +98,29 @@ |
319 | object.__init__(self) |
320 | self._buildid = buildid |
321 | self._slave = slave |
322 | + if reactor is None: |
323 | + reactor = default_reactor |
324 | + self._reactor = reactor |
325 | self._preppath = slave._config.get("allmanagers", "preppath") |
326 | self._unpackpath = slave._config.get("allmanagers", "unpackpath") |
327 | self._cleanpath = slave._config.get("allmanagers", "cleanpath") |
328 | self._mountpath = slave._config.get("allmanagers", "mountpath") |
329 | self._umountpath = slave._config.get("allmanagers", "umountpath") |
330 | + self._scanpath = slave._config.get("allmanagers", "processscanpath") |
331 | + self._subprocess = None |
332 | + self._reaped_states = set() |
333 | self.is_archive_private = False |
334 | self.home = os.environ['HOME'] |
335 | + self.abort_timeout = 120 |
336 | |
337 | - def runSubProcess(self, command, args): |
338 | + def runSubProcess(self, command, args, iterate=None): |
339 | """Run a sub process capturing the results in the log.""" |
340 | - self._subprocess = RunCapture(self._slave, self.iterate) |
341 | + if iterate is None: |
342 | + iterate = self.iterate |
343 | + self._subprocess = RunCapture(self._slave, iterate) |
344 | self._slave.log("RUN: %s %r\n" % (command, args)) |
345 | childfds = {0: devnull.fileno(), 1: "r", 2: "r"} |
346 | - reactor.spawnProcess( |
347 | + self._reactor.spawnProcess( |
348 | self._subprocess, command, args, env=os.environ, |
349 | path=self.home, childFDs=childfds) |
350 | |
351 | @@ -122,6 +130,25 @@ |
352 | self._unpackpath, |
353 | ["unpack-chroot", self._buildid, self._chroottarfile]) |
354 | |
355 | + def doReapProcesses(self, state, notify=True): |
356 | + """Reap any processes left lying around in the chroot.""" |
357 | + if state is not None and state in self._reaped_states: |
358 | + # We've already reaped this state. To avoid a loop, proceed |
359 | + # immediately to the next iterator. |
360 | + self._slave.log("Already reaped from state %s" % state) |
361 | + if notify: |
362 | + self.iterateReap(state, 0) |
363 | + else: |
364 | + if state is not None: |
365 | + self._reaped_states.add(state) |
366 | + if notify: |
367 | + iterate = partial(self.iterateReap, state) |
368 | + else: |
369 | + iterate = lambda success: None |
370 | + self.runSubProcess( |
371 | + self._scanpath, [self._scanpath, self._buildid], |
372 | + iterate=iterate) |
373 | + |
374 | def doCleanup(self): |
375 | """Remove the build tree etc.""" |
376 | self.runSubProcess(self._cleanpath, ["remove-build", self._buildid]) |
377 | @@ -175,37 +202,66 @@ |
378 | raise NotImplementedError("BuildManager should be subclassed to be " |
379 | "used") |
380 | |
381 | + def iterateReap(self, state, success): |
382 | + """Perform an iteration of the slave following subprocess reaping. |
383 | + |
384 | + Subprocess reaping is special, typically occurring at several |
385 | + positions in a build manager's state machine. We therefore keep |
386 | + track of the state being reaped so that we can select the |
387 | + appropriate next state. |
388 | + """ |
389 | + raise NotImplementedError("BuildManager should be subclassed to be " |
390 | + "used") |
391 | + |
392 | + def abortReap(self): |
393 | + """Abort by killing all processes in the chroot, as hard as we can. |
394 | + |
395 | + We expect this to result in the main build process exiting non-zero |
396 | + and giving us some useful logs. |
397 | + |
398 | + This may be overridden in subclasses so that they can perform their |
399 | + own state machine management. |
400 | + """ |
401 | + self.doReapProcesses(None, notify=False) |
402 | + |
403 | def abort(self): |
404 | """Abort the build by killing the subprocess.""" |
405 | if self.alreadyfailed or self._subprocess is None: |
406 | return |
407 | else: |
408 | self.alreadyfailed = True |
409 | - # Either SIGKILL and SIGTERM presents the same behavior, the |
410 | - # process is just killed some time after the signal was sent |
411 | - # 10 s ~ 40 s, and returns None as exit_code, instead of the normal |
412 | - # integer. See further info on DebianBuildermanager.iterate in |
413 | - # debian.py |
414 | - # XXX cprov 2005-09-02: |
415 | - # we may want to follow the canonical.tachandler kill process |
416 | - # style, which sends SIGTERM to the process wait a given timeout |
417 | - # and if was not killed sends a SIGKILL. IMO it only would be worth |
418 | - # if we found different behaviour than the previous described. |
419 | - self._subprocess.transport.signalProcess('TERM') |
420 | - # alternatively to simply send SIGTERM, we can pend a request to |
421 | - # send SIGKILL to the process if nothing happened in 10 seconds |
422 | - # see base class process |
423 | - self._subprocess.killCall = reactor.callLater(10, self.kill) |
424 | - |
425 | - def kill(self): |
426 | - """Send SIGKILL to child process |
427 | - |
428 | - Mask exception generated when the child process has already exited. |
429 | - """ |
430 | + primary_subprocess = self._subprocess |
431 | + self.abortReap() |
432 | + # In extreme cases the build may be hung too badly for |
433 | + # scan-for-processes to manage to kill it (blocked on I/O, |
434 | + # forkbombing test suite, etc.). In this case, fail the builder and |
435 | + # let an admin sort it out. |
436 | + self._subprocess.builderFailCall = self._reactor.callLater( |
437 | + self.abort_timeout, self.builderFail, |
438 | + "Failed to kill all processes.", primary_subprocess) |
439 | + |
440 | + def builderFail(self, reason, primary_subprocess): |
441 | + """Mark the builder as failed.""" |
442 | + self._slave.log("ABORTING: %s\n" % reason) |
443 | + self._subprocess.builderFailCall = None |
444 | + self._slave.builderFail() |
445 | + self.alreadyfailed = True |
446 | + # If we failed to kill all processes in the chroot, then the primary |
447 | + # subprocess (i.e. the one running immediately before |
448 | + # doReapProcesses was called) may not have exited. Kill it so that |
449 | + # we can proceed. |
450 | try: |
451 | - self._subprocess.transport.signalProcess('KILL') |
452 | + primary_subprocess.transport.signalProcess('KILL') |
453 | except process.ProcessExitedAlready: |
454 | self._slave.log("ABORTING: Process Exited Already\n") |
455 | + primary_subprocess.transport.loseConnection() |
456 | + # Leave the reaper running, but disconnect it from our state |
457 | + # machine. Perhaps an admin can make something of it, and in any |
458 | + # case scan-for-processes elevates itself to root so it's awkward to |
459 | + # kill it. |
460 | + self._subprocess.ignore = True |
461 | + self._subprocess.transport.loseConnection() |
462 | + |
463 | |
464 | class BuilderStatus: |
465 | """Status values for the builder.""" |
466 | @@ -229,6 +285,7 @@ |
467 | PACKAGEFAIL = "BuildStatus.PACKAGEFAIL" |
468 | CHROOTFAIL = "BuildStatus.CHROOTFAIL" |
469 | BUILDERFAIL = "BuildStatus.BUILDERFAIL" |
470 | + ABORTED = "BuildStatus.ABORTED" |
471 | |
472 | |
473 | class BuildDSlave(object): |
474 | @@ -355,6 +412,11 @@ |
475 | # XXX: dsilvers: 2005-01-21: Current abort mechanism doesn't wait |
476 | # for abort to complete. This is potentially an issue in a heavy |
477 | # load situation. |
478 | + if self.builderstatus == BuilderStatus.ABORTING: |
479 | + # This might happen if the master side restarts in the middle of |
480 | + # an abort cycle. |
481 | + self.log("Slave already ABORTING when asked to abort") |
482 | + return |
483 | if self.builderstatus != BuilderStatus.BUILDING: |
484 | # XXX: Should raise a known Fault so that the client can make |
485 | # useful decisions about the error! |
486 | @@ -458,8 +520,10 @@ |
487 | |
488 | def builderFail(self): |
489 | """Cease building because the builder has a problem.""" |
490 | - if self.builderstatus != BuilderStatus.BUILDING: |
491 | - raise ValueError("Slave is not BUILDING when set to BUILDERFAIL") |
492 | + if self.builderstatus not in (BuilderStatus.BUILDING, |
493 | + BuilderStatus.ABORTING): |
494 | + raise ValueError("Slave is not BUILDING|ABORTING when set to " |
495 | + "BUILDERFAIL") |
496 | self.buildstatus = BuildStatus.BUILDERFAIL |
497 | |
498 | def chrootFail(self): |
499 | @@ -497,14 +561,25 @@ |
500 | raise ValueError("Slave is not BUILDING when set to GIVENBACK") |
501 | self.buildstatus = BuildStatus.GIVENBACK |
502 | |
503 | + def buildAborted(self): |
504 | + """Mark a build as aborted.""" |
505 | + if self.builderstatus != BuilderStatus.ABORTING: |
506 | + raise ValueError("Slave is not ABORTING when set to ABORTED") |
507 | + if self.buildstatus != BuildStatus.BUILDERFAIL: |
508 | + self.buildstatus = BuildStatus.ABORTED |
509 | + |
510 | def buildComplete(self): |
511 | """Mark the build as complete and waiting interaction from the build |
512 | daemon master. |
513 | """ |
514 | - if self.builderstatus != BuilderStatus.BUILDING: |
515 | - raise ValueError("Slave is not BUILDING when told build is " |
516 | - "complete") |
517 | - self.builderstatus = BuilderStatus.WAITING |
518 | + if self.builderstatus == BuilderStatus.BUILDING: |
519 | + self.builderstatus = BuilderStatus.WAITING |
520 | + elif self.builderstatus == BuilderStatus.ABORTING: |
521 | + self.buildAborted() |
522 | + self.builderstatus = BuilderStatus.WAITING |
523 | + else: |
524 | + raise ValueError("Slave is not BUILDING|ABORTING when told build " |
525 | + "is complete") |
526 | |
527 | def sanitizeBuildlog(self, log_path): |
528 | """Removes passwords from buildlog URLs. |
529 | |
530 | === modified file 'lpbuildd/sourcepackagerecipe.py' |
531 | --- lpbuildd/sourcepackagerecipe.py 2011-11-10 04:55:02 +0000 |
532 | +++ lpbuildd/sourcepackagerecipe.py 2013-07-30 11:30:43 +0000 |
533 | @@ -124,8 +124,12 @@ |
534 | self._slave.builderFail() |
535 | print("Returning build status: Builder failed.") |
536 | self.alreadyfailed = True |
537 | - self._state = DebianBuildState.REAP |
538 | - self.doReapProcesses() |
539 | + self.doReapProcesses(self._state) |
540 | + |
541 | + def iterateReap_BUILD_RECIPE(self, retcode): |
542 | + """Finished reaping after recipe building.""" |
543 | + self._state = DebianBuildState.UMOUNT |
544 | + self.doUnmounting() |
545 | |
546 | def getChangesFilename(self): |
547 | """Return the path to the changes file.""" |
548 | |
549 | === modified file 'lpbuildd/tests/buildd-slave-test.conf' |
550 | --- lpbuildd/tests/buildd-slave-test.conf 2011-12-08 22:13:03 +0000 |
551 | +++ lpbuildd/tests/buildd-slave-test.conf 2013-07-30 11:30:43 +0000 |
552 | @@ -12,17 +12,16 @@ |
553 | mountpath = /var/tmp/buildd/slavebin/mount-chroot |
554 | umountpath = /var/tmp/buildd/slavebin/umount-chroot |
555 | preppath = /var/tmp/buildd/slavebin/slave-prep |
556 | +processscanpath = /var/tmp/buildd/slavebin/scan-for-processes |
557 | |
558 | [debianmanager] |
559 | sbuildpath = /var/tmp/buildd/slavebin/sbuild-package |
560 | sbuildargs = -dautobuild --nolog --batch |
561 | updatepath = /var/tmp/buildd/slavebin/update-debian-chroot |
562 | -processscanpath = /var/tmp/buildd/slavebin/scan-for-processes |
563 | sourcespath = /usr/share/launchpad-buildd/slavebin/override-sources-list |
564 | |
565 | [binarypackagemanager] |
566 | sbuildpath = /var/tmp/buildd/slavebin/sbuild-package |
567 | sbuildargs = -dautobuild --nolog --batch |
568 | updatepath = /var/tmp/buildd/slavebin/update-debian-chroot |
569 | -processscanpath = /var/tmp/buildd/slavebin/scan-for-processes |
570 | sourcespath = /usr/share/launchpad-buildd/slavebin/override-sources-list |
571 | |
572 | === added file 'lpbuildd/tests/fakeslave.py' |
573 | --- lpbuildd/tests/fakeslave.py 1970-01-01 00:00:00 +0000 |
574 | +++ lpbuildd/tests/fakeslave.py 2013-07-30 11:30:43 +0000 |
575 | @@ -0,0 +1,104 @@ |
576 | +# Copyright 2013 Canonical Ltd. This software is licensed under the |
577 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
578 | + |
579 | +__metaclass__ = type |
580 | +__all__ = [ |
581 | + 'FakeMethod', |
582 | + 'FakeSlave', |
583 | + ] |
584 | + |
585 | +import os |
586 | + |
587 | + |
588 | +class FakeMethod: |
589 | + """Catch any function or method call, and record the fact. |
590 | + |
591 | + Use this for easy stubbing. The call operator can return a fixed |
592 | + value, or raise a fixed exception object. |
593 | + |
594 | + This is useful when unit-testing code that does things you don't |
595 | + want to integration-test, e.g. because it wants to talk to remote |
596 | + systems. |
597 | + """ |
598 | + |
599 | + def __init__(self, result=None, failure=None): |
600 | + """Set up a fake function or method. |
601 | + |
602 | + :param result: Value to return. |
603 | + :param failure: Exception to raise. |
604 | + """ |
605 | + self.result = result |
606 | + self.failure = failure |
607 | + |
608 | + # A log of arguments for each call to this method. |
609 | + self.calls = [] |
610 | + |
611 | + def __call__(self, *args, **kwargs): |
612 | + """Catch an invocation to the method. |
613 | + |
614 | + Increment `call_count`, and adds the arguments to `calls`. |
615 | + |
616 | + Accepts any and all parameters. Raises the failure passed to |
617 | + the constructor, if any; otherwise, returns the result value |
618 | + passed to the constructor. |
619 | + """ |
620 | + self.calls.append((args, kwargs)) |
621 | + |
622 | + if self.failure is None: |
623 | + return self.result |
624 | + else: |
625 | + # pylint thinks this raises None, which is clearly not |
626 | + # possible. That's why this test disables pylint message |
627 | + # E0702. |
628 | + raise self.failure |
629 | + |
630 | + @property |
631 | + def call_count(self): |
632 | + return len(self.calls) |
633 | + |
634 | + def extract_args(self): |
635 | + """Return just the calls' positional-arguments tuples.""" |
636 | + return [args for args, kwargs in self.calls] |
637 | + |
638 | + def extract_kwargs(self): |
639 | + """Return just the calls' keyword-arguments dicts.""" |
640 | + return [kwargs for args, kwargs in self.calls] |
641 | + |
642 | + |
643 | +class FakeConfig: |
644 | + def get(self, section, key): |
645 | + return key |
646 | + |
647 | + |
648 | +class FakeSlave: |
649 | + def __init__(self, tempdir): |
650 | + self._cachepath = tempdir |
651 | + self._config = FakeConfig() |
652 | + self._was_called = set() |
653 | + self.waitingfiles = {} |
654 | + |
655 | + def cachePath(self, file): |
656 | + return os.path.join(self._cachepath, file) |
657 | + |
658 | + def anyMethod(self, *args, **kwargs): |
659 | + pass |
660 | + |
661 | + fake_methods = [ |
662 | + 'emptyLog', 'log', 'chrootFail', 'buildFail', 'builderFail', |
663 | + ] |
664 | + def __getattr__(self, name): |
665 | + """Remember which fake methods were called.""" |
666 | + if name not in self.fake_methods: |
667 | + raise AttributeError( |
668 | + "'%s' object has no attribute '%s'" % (self.__class__, name)) |
669 | + self._was_called.add(name) |
670 | + return self.anyMethod |
671 | + |
672 | + def wasCalled(self, name): |
673 | + return name in self._was_called |
674 | + |
675 | + def getArch(self): |
676 | + return 'i386' |
677 | + |
678 | + storeFile = FakeMethod() |
679 | + addWaitingFile = FakeMethod() |
680 | |
681 | === modified file 'lpbuildd/tests/harness.py' |
682 | --- lpbuildd/tests/harness.py 2012-06-05 17:11:03 +0000 |
683 | +++ lpbuildd/tests/harness.py 2013-07-30 11:30:43 +0000 |
684 | @@ -4,6 +4,7 @@ |
685 | __metaclass__ = type |
686 | __all__ = [ |
687 | 'BuilddTestCase', |
688 | + 'FakeSlave', |
689 | ] |
690 | |
691 | import os |
692 | |
693 | === added file 'lpbuildd/tests/test_binarypackage.py' |
694 | --- lpbuildd/tests/test_binarypackage.py 1970-01-01 00:00:00 +0000 |
695 | +++ lpbuildd/tests/test_binarypackage.py 2013-07-30 11:30:43 +0000 |
696 | @@ -0,0 +1,206 @@ |
697 | +# Copyright 2013 Canonical Ltd. This software is licensed under the |
698 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
699 | + |
700 | +__metaclass__ = type |
701 | + |
702 | +import tempfile |
703 | + |
704 | +import os |
705 | +import shutil |
706 | +from testtools import TestCase |
707 | + |
708 | +from twisted.internet.task import Clock |
709 | + |
710 | +from lpbuildd.binarypackage import ( |
711 | + BinaryPackageBuildManager, |
712 | + BinaryPackageBuildState, |
713 | + ) |
714 | +from lpbuildd.tests.fakeslave import ( |
715 | + FakeMethod, |
716 | + FakeSlave, |
717 | + ) |
718 | + |
719 | + |
720 | +class MockTransport: |
721 | + loseConnection = FakeMethod() |
722 | + signalProcess = FakeMethod() |
723 | + |
724 | + |
725 | +class MockSubprocess: |
726 | + def __init__(self, path): |
727 | + self.path = path |
728 | + self.transport = MockTransport() |
729 | + |
730 | + |
731 | +class MockBuildManager(BinaryPackageBuildManager): |
732 | + def __init__(self, *args, **kwargs): |
733 | + super(MockBuildManager, self).__init__(*args, **kwargs) |
734 | + self.commands = [] |
735 | + self.iterators = [] |
736 | + |
737 | + def runSubProcess(self, path, command, iterate=None): |
738 | + self.commands.append([path]+command) |
739 | + if iterate is None: |
740 | + iterate = self.iterate |
741 | + self.iterators.append(iterate) |
742 | + self._subprocess = MockSubprocess(path) |
743 | + return 0 |
744 | + |
745 | + |
746 | +class TestBinaryPackageBuildManagerIteration(TestCase): |
747 | + """Run BinaryPackageBuildManager through its iteration steps.""" |
748 | + def setUp(self): |
749 | + super(TestBinaryPackageBuildManagerIteration, self).setUp() |
750 | + self.working_dir = tempfile.mkdtemp() |
751 | + self.addCleanup(lambda: shutil.rmtree(self.working_dir)) |
752 | + slave_dir = os.path.join(self.working_dir, 'slave') |
753 | + home_dir = os.path.join(self.working_dir, 'home') |
754 | + for dir in (slave_dir, home_dir): |
755 | + os.mkdir(dir) |
756 | + self.slave = FakeSlave(slave_dir) |
757 | + self.buildid = '123' |
758 | + self.clock = Clock() |
759 | + self.buildmanager = MockBuildManager( |
760 | + self.slave, self.buildid, reactor=self.clock) |
761 | + self.buildmanager.home = home_dir |
762 | + self.buildmanager._cachepath = self.slave._cachepath |
763 | + self.chrootdir = os.path.join( |
764 | + home_dir, 'build-%s' % self.buildid, 'chroot-autobuild') |
765 | + |
766 | + def getState(self): |
767 | + """Retrieve build manager's state.""" |
768 | + return self.buildmanager._state |
769 | + |
770 | + def startBuild(self): |
771 | + # The build manager's iterate() kicks off the consecutive states |
772 | + # after INIT. |
773 | + self.buildmanager.initiate( |
774 | + {'foo_1.dsc': ''}, 'chroot.tar.gz', |
775 | + {'suite': 'warty', 'ogrecomponent': 'main'}) |
776 | + |
777 | + # Skip DebianBuildManager states to the state directly before |
778 | + # SBUILD. |
779 | + self.buildmanager._state = BinaryPackageBuildState.UPDATE |
780 | + |
781 | + # SBUILD: Build the package. |
782 | + self.buildmanager.iterate(0) |
783 | + self.assertEqual(BinaryPackageBuildState.SBUILD, self.getState()) |
784 | + expected_command = [ |
785 | + 'sbuildpath', 'sbuild-package', self.buildid, 'i386', 'warty', |
786 | + 'sbuildargs', '--dist=warty', '--architecture=i386', '--comp=main', |
787 | + 'foo_1.dsc', |
788 | + ] |
789 | + self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
790 | + self.assertEqual( |
791 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
792 | + self.assertFalse(self.slave.wasCalled('chrootFail')) |
793 | + |
794 | + def test_iterate(self): |
795 | + # The build manager iterates a normal build from start to finish. |
796 | + self.startBuild() |
797 | + |
798 | + log_path = os.path.join(self.buildmanager._cachepath, 'buildlog') |
799 | + log = open(log_path, 'w') |
800 | + log.write("I am a build log.") |
801 | + log.close() |
802 | + |
803 | + changes_path = os.path.join( |
804 | + self.buildmanager.home, 'build-%s' % self.buildid, |
805 | + 'foo_1_i386.changes') |
806 | + changes = open(changes_path, 'w') |
807 | + changes.write("I am a changes file.") |
808 | + changes.close() |
809 | + |
810 | + # After building the package, reap processes. |
811 | + self.buildmanager.iterate(0) |
812 | + expected_command = [ |
813 | + 'processscanpath', 'processscanpath', self.buildid, |
814 | + ] |
815 | + self.assertEqual(BinaryPackageBuildState.SBUILD, self.getState()) |
816 | + self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
817 | + self.assertNotEqual( |
818 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
819 | + self.assertFalse(self.slave.wasCalled('buildFail')) |
820 | + self.assertEqual([], self.slave.addWaitingFile.calls) |
821 | + |
822 | + # Control returns to the DebianBuildManager in the UMOUNT state. |
823 | + self.buildmanager.iterateReap(self.getState(), 0) |
824 | + expected_command = [ |
825 | + 'umountpath', 'umount-chroot', self.buildid |
826 | + ] |
827 | + self.assertEqual(BinaryPackageBuildState.UMOUNT, self.getState()) |
828 | + self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
829 | + self.assertEqual( |
830 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
831 | + self.assertFalse(self.slave.wasCalled('buildFail')) |
832 | + |
833 | + def test_abort_sbuild(self): |
834 | + # Aborting sbuild kills processes in the chroot. |
835 | + self.startBuild() |
836 | + |
837 | + # Send an abort command. The build manager reaps processes. |
838 | + self.buildmanager.abort() |
839 | + expected_command = [ |
840 | + 'processscanpath', 'processscanpath', self.buildid |
841 | + ] |
842 | + self.assertEqual(BinaryPackageBuildState.SBUILD, self.getState()) |
843 | + self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
844 | + self.assertNotEqual( |
845 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
846 | + self.assertFalse(self.slave.wasCalled('buildFail')) |
847 | + |
848 | + # If reaping completes successfully, the build manager returns |
849 | + # control to the DebianBuildManager in the UMOUNT state. |
850 | + self.buildmanager.iterateReap(self.getState(), 0) |
851 | + expected_command = [ |
852 | + 'umountpath', 'umount-chroot', self.buildid |
853 | + ] |
854 | + self.assertEqual(BinaryPackageBuildState.UMOUNT, self.getState()) |
855 | + self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
856 | + self.assertEqual( |
857 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
858 | + self.assertFalse(self.slave.wasCalled('buildFail')) |
859 | + |
860 | + def test_abort_sbuild_fail(self): |
861 | + # If killing processes in the chroot hangs, the build manager does |
862 | + # its best to clean up and fails the builder. |
863 | + self.startBuild() |
864 | + sbuild_subprocess = self.buildmanager._subprocess |
865 | + |
866 | + # Send an abort command. The build manager reaps processes. |
867 | + self.buildmanager.abort() |
868 | + expected_command = [ |
869 | + 'processscanpath', 'processscanpath', self.buildid |
870 | + ] |
871 | + self.assertEqual(BinaryPackageBuildState.SBUILD, self.getState()) |
872 | + self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
873 | + self.assertNotEqual( |
874 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
875 | + self.assertFalse(self.slave.wasCalled('builderFail')) |
876 | + reap_subprocess = self.buildmanager._subprocess |
877 | + |
878 | + # If reaping fails, the builder is failed, sbuild is killed, and the |
879 | + # reaper is disconnected. |
880 | + self.clock.advance(120) |
881 | + self.assertTrue(self.slave.wasCalled('builderFail')) |
882 | + self.assertEqual( |
883 | + [(('KILL',), {})], sbuild_subprocess.transport.signalProcess.calls) |
884 | + self.assertNotEqual( |
885 | + [], sbuild_subprocess.transport.loseConnection.calls) |
886 | + self.assertNotEqual([], reap_subprocess.transport.loseConnection.calls) |
887 | + |
888 | + log_path = os.path.join(self.buildmanager._cachepath, 'buildlog') |
889 | + log = open(log_path, 'w') |
890 | + log.write("I am a build log.") |
891 | + log.close() |
892 | + |
893 | + # When sbuild exits, it does not reap processes again, but proceeds |
894 | + # directly to UMOUNT. |
895 | + self.buildmanager.iterate(128 + 9) # SIGKILL |
896 | + expected_command = [ |
897 | + 'umountpath', 'umount-chroot', self.buildid |
898 | + ] |
899 | + self.assertEqual(BinaryPackageBuildState.UMOUNT, self.getState()) |
900 | + self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
901 | + self.assertEqual( |
902 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
903 | |
904 | === modified file 'lpbuildd/tests/test_translationtemplatesbuildmanager.py' |
905 | --- lpbuildd/tests/test_translationtemplatesbuildmanager.py 2011-11-21 05:41:27 +0000 |
906 | +++ lpbuildd/tests/test_translationtemplatesbuildmanager.py 2013-07-30 11:30:43 +0000 |
907 | @@ -9,109 +9,24 @@ |
908 | |
909 | from testtools import TestCase |
910 | |
911 | +from lpbuildd.tests.fakeslave import FakeSlave |
912 | from lpbuildd.translationtemplates import ( |
913 | TranslationTemplatesBuildManager, |
914 | TranslationTemplatesBuildState, |
915 | ) |
916 | |
917 | |
918 | -class FakeMethod: |
919 | - """Catch any function or method call, and record the fact. |
920 | - |
921 | - Use this for easy stubbing. The call operator can return a fixed |
922 | - value, or raise a fixed exception object. |
923 | - |
924 | - This is useful when unit-testing code that does things you don't |
925 | - want to integration-test, e.g. because it wants to talk to remote |
926 | - systems. |
927 | - """ |
928 | - |
929 | - def __init__(self, result=None, failure=None): |
930 | - """Set up a fake function or method. |
931 | - |
932 | - :param result: Value to return. |
933 | - :param failure: Exception to raise. |
934 | - """ |
935 | - self.result = result |
936 | - self.failure = failure |
937 | - |
938 | - # A log of arguments for each call to this method. |
939 | - self.calls = [] |
940 | - |
941 | - def __call__(self, *args, **kwargs): |
942 | - """Catch an invocation to the method. |
943 | - |
944 | - Increment `call_count`, and adds the arguments to `calls`. |
945 | - |
946 | - Accepts any and all parameters. Raises the failure passed to |
947 | - the constructor, if any; otherwise, returns the result value |
948 | - passed to the constructor. |
949 | - """ |
950 | - self.calls.append((args, kwargs)) |
951 | - |
952 | - if self.failure is None: |
953 | - return self.result |
954 | - else: |
955 | - # pylint thinks this raises None, which is clearly not |
956 | - # possible. That's why this test disables pylint message |
957 | - # E0702. |
958 | - raise self.failure |
959 | - |
960 | - @property |
961 | - def call_count(self): |
962 | - return len(self.calls) |
963 | - |
964 | - def extract_args(self): |
965 | - """Return just the calls' positional-arguments tuples.""" |
966 | - return [args for args, kwargs in self.calls] |
967 | - |
968 | - def extract_kwargs(self): |
969 | - """Return just the calls' keyword-arguments dicts.""" |
970 | - return [kwargs for args, kwargs in self.calls] |
971 | - |
972 | - |
973 | -class FakeConfig: |
974 | - def get(self, section, key): |
975 | - return key |
976 | - |
977 | - |
978 | -class FakeSlave: |
979 | - def __init__(self, tempdir): |
980 | - self._cachepath = tempdir |
981 | - self._config = FakeConfig() |
982 | - self._was_called = set() |
983 | - |
984 | - def cachePath(self, file): |
985 | - return os.path.join(self._cachepath, file) |
986 | - |
987 | - def anyMethod(self, *args, **kwargs): |
988 | - pass |
989 | - |
990 | - fake_methods = ['emptyLog', 'chrootFail', 'buildFail', 'builderFail',] |
991 | - def __getattr__(self, name): |
992 | - """Remember which fake methods were called.""" |
993 | - if name not in self.fake_methods: |
994 | - raise AttributeError( |
995 | - "'%s' object has no attribute '%s'" % (self.__class__, name)) |
996 | - self._was_called.add(name) |
997 | - return self.anyMethod |
998 | - |
999 | - def wasCalled(self, name): |
1000 | - return name in self._was_called |
1001 | - |
1002 | - def getArch(self): |
1003 | - return 'i386' |
1004 | - |
1005 | - addWaitingFile = FakeMethod() |
1006 | - |
1007 | - |
1008 | class MockBuildManager(TranslationTemplatesBuildManager): |
1009 | def __init__(self, *args, **kwargs): |
1010 | super(MockBuildManager, self).__init__(*args, **kwargs) |
1011 | self.commands = [] |
1012 | + self.iterators = [] |
1013 | |
1014 | - def runSubProcess(self, path, command): |
1015 | + def runSubProcess(self, path, command, iterate=None): |
1016 | self.commands.append([path]+command) |
1017 | + if iterate is None: |
1018 | + iterate = self.iterate |
1019 | + self.iterators.append(iterate) |
1020 | return 0 |
1021 | |
1022 | |
1023 | @@ -143,7 +58,7 @@ |
1024 | # after INIT. |
1025 | self.buildmanager.initiate({}, 'chroot.tar.gz', {'branch_url': url}) |
1026 | |
1027 | - # Skip states that are done in DebianBuldManager to the state |
1028 | + # Skip states that are done in DebianBuildManager to the state |
1029 | # directly before INSTALL. |
1030 | self.buildmanager._state = TranslationTemplatesBuildState.UPDATE |
1031 | |
1032 | @@ -158,6 +73,8 @@ |
1033 | 'apt-get', |
1034 | ] |
1035 | self.assertEqual(expected_command, self.buildmanager.commands[-1][:5]) |
1036 | + self.assertEqual( |
1037 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
1038 | |
1039 | # GENERATE: Run the slave's payload, the script that generates |
1040 | # templates. |
1041 | @@ -168,6 +85,8 @@ |
1042 | 'generatepath', 'generatepath', self.buildid, url, 'resultarchive' |
1043 | ] |
1044 | self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
1045 | + self.assertEqual( |
1046 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
1047 | self.assertFalse(self.slave.wasCalled('chrootFail')) |
1048 | |
1049 | outfile_path = os.path.join( |
1050 | @@ -179,18 +98,32 @@ |
1051 | outfile.write("I am a template tarball. Seriously.") |
1052 | outfile.close() |
1053 | |
1054 | - # The control returns to the DebianBuildManager in the REAP state. |
1055 | + # After generating templates, reap processes. |
1056 | self.buildmanager.iterate(0) |
1057 | expected_command = [ |
1058 | 'processscanpath', 'processscanpath', self.buildid |
1059 | ] |
1060 | self.assertEqual( |
1061 | - TranslationTemplatesBuildState.REAP, self.getState()) |
1062 | + TranslationTemplatesBuildState.GENERATE, self.getState()) |
1063 | self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
1064 | + self.assertNotEqual( |
1065 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
1066 | self.assertFalse(self.slave.wasCalled('buildFail')) |
1067 | self.assertEqual( |
1068 | [((outfile_path,), {})], self.slave.addWaitingFile.calls) |
1069 | |
1070 | + # The control returns to the DebianBuildManager in the UMOUNT state. |
1071 | + self.buildmanager.iterateReap(self.getState(), 0) |
1072 | + expected_command = [ |
1073 | + 'umountpath', 'umount-chroot', self.buildid |
1074 | + ] |
1075 | + self.assertEqual( |
1076 | + TranslationTemplatesBuildState.UMOUNT, self.getState()) |
1077 | + self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
1078 | + self.assertEqual( |
1079 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
1080 | + self.assertFalse(self.slave.wasCalled('buildFail')) |
1081 | + |
1082 | def test_iterate_fail_INSTALL(self): |
1083 | # See that a failing INSTALL is handled properly. |
1084 | url = 'lp:~my/branch' |
1085 | @@ -209,6 +142,8 @@ |
1086 | 'umountpath', 'umount-chroot', self.buildid |
1087 | ] |
1088 | self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
1089 | + self.assertEqual( |
1090 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
1091 | self.assertTrue(self.slave.wasCalled('chrootFail')) |
1092 | |
1093 | def test_iterate_fail_GENERATE(self): |
1094 | @@ -221,12 +156,25 @@ |
1095 | # Skip states to the INSTALL state. |
1096 | self.buildmanager._state = TranslationTemplatesBuildState.GENERATE |
1097 | |
1098 | - # The buildmanager fails and iterates to the REAP state. |
1099 | + # The buildmanager fails and reaps processes. |
1100 | self.buildmanager.iterate(-1) |
1101 | expected_command = [ |
1102 | 'processscanpath', 'processscanpath', self.buildid |
1103 | ] |
1104 | self.assertEqual( |
1105 | - TranslationTemplatesBuildState.REAP, self.getState()) |
1106 | + TranslationTemplatesBuildState.GENERATE, self.getState()) |
1107 | self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
1108 | + self.assertNotEqual( |
1109 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
1110 | self.assertTrue(self.slave.wasCalled('buildFail')) |
1111 | + |
1112 | + # The buildmanager iterates to the UMOUNT state. |
1113 | + self.buildmanager.iterateReap(self.getState(), 0) |
1114 | + self.assertEqual( |
1115 | + TranslationTemplatesBuildState.UMOUNT, self.getState()) |
1116 | + expected_command = [ |
1117 | + 'umountpath', 'umount-chroot', self.buildid |
1118 | + ] |
1119 | + self.assertEqual(expected_command, self.buildmanager.commands[-1]) |
1120 | + self.assertEqual( |
1121 | + self.buildmanager.iterate, self.buildmanager.iterators[-1]) |
1122 | |
1123 | === modified file 'lpbuildd/translationtemplates.py' |
1124 | --- lpbuildd/translationtemplates.py 2011-11-10 04:55:02 +0000 |
1125 | +++ lpbuildd/translationtemplates.py 2013-07-30 11:30:43 +0000 |
1126 | @@ -88,12 +88,14 @@ |
1127 | if success == 0: |
1128 | # It worked! Now let's bring in the harvest. |
1129 | self.gatherResults() |
1130 | - self._state = TranslationTemplatesBuildState.REAP |
1131 | - self.doReapProcesses() |
1132 | + self.doReapProcesses(self._state) |
1133 | else: |
1134 | if not self.alreadyfailed: |
1135 | self._slave.buildFail() |
1136 | self.alreadyfailed = True |
1137 | - self._state = TranslationTemplatesBuildState.REAP |
1138 | - self.doReapProcesses() |
1139 | + self.doReapProcesses(self._state) |
1140 | |
1141 | + def iterateReap_GENERATE(self, success): |
1142 | + """Finished reaping after template generation.""" |
1143 | + self._state = TranslationTemplatesBuildState.UMOUNT |
1144 | + self.doUnmounting() |
1145 | |
1146 | === modified file 'template-buildd-slave.conf' |
1147 | --- template-buildd-slave.conf 2011-12-05 02:39:05 +0000 |
1148 | +++ template-buildd-slave.conf 2013-07-30 11:30:43 +0000 |
1149 | @@ -15,10 +15,10 @@ |
1150 | cleanpath = /usr/share/launchpad-buildd/slavebin/remove-build |
1151 | mountpath = /usr/share/launchpad-buildd/slavebin/mount-chroot |
1152 | umountpath = /usr/share/launchpad-buildd/slavebin/umount-chroot |
1153 | +processscanpath = /usr/share/launchpad-buildd/slavebin/scan-for-processes |
1154 | |
1155 | [debianmanager] |
1156 | updatepath = /usr/share/launchpad-buildd/slavebin/update-debian-chroot |
1157 | -processscanpath = /usr/share/launchpad-buildd/slavebin/scan-for-processes |
1158 | sourcespath = /usr/share/launchpad-buildd/slavebin/override-sources-list |
1159 | |
1160 | [binarypackagemanager] |
Looks good to me after several readings, and having seen it in action last week. Given that it was a longer change than expected, I'd like to see a review from William too (especially as he's better versed in nit-picking Python/LP bits), but it all logically makes sense to me.