Launchpad itself

Merge lp:~jml/launchpad/buildd-deferred into lp:launchpad

  1. buildd-deferred
  2. Merge into devel
Proposed by Jonathan Lange
Status: Merged
Approved by: Julian Edwards
Approved revision: no longer in the source branch.
Merged at revision: 11581
Proposed branch: lp:~jml/launchpad/buildd-deferred
Merge into: lp:launchpad
Diff against target: 396 lines (+230/-12)
7 files modified
lib/canonical/buildd/slave.py (+2/-0)
lib/canonical/buildd/tests/buildd-slave-test.conf (+7/-0)
lib/lp/buildmaster/manager.py (+15/-1)
lib/lp/buildmaster/model/builder.py (+45/-9)
lib/lp/buildmaster/tests/test_builder.py (+157/-1)
lib/lp/buildmaster/tests/test_manager.py (+0/-1)
lib/lp/soyuz/tests/soyuzbuilddhelpers.py (+4/-0)
To merge this branch: bzr merge lp:~jml/launchpad/buildd-deferred
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Review via email: mp+36010@code.launchpad.net

Commit message

Change BuilderSlave to use xmlrpclib.ServerProxy rather than inherit from it.

Description of the change

This branch changes the BuilderSlave class in Soyuz to use composition, rather than inheritance, to do the XML-RPC interactions.

To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/buildd/slave.py'
2--- lib/canonical/buildd/slave.py 2010-05-20 11:19:56 +0000
3+++ lib/canonical/buildd/slave.py 2010-09-20 13:43:43 +0000
4@@ -356,6 +356,8 @@
5 # for abort to complete. This is potentially an issue in a heavy
6 # load situation.
7 if self.builderstatus != BuilderStatus.BUILDING:
8+ # XXX: Should raise a known Fault so that the client can make
9+ # useful decisions about the error!
10 raise ValueError("Slave is not BUILDING when asked to abort")
11 self.manager.abort()
12 self.builderstatus = BuilderStatus.ABORTING
13
14=== modified file 'lib/canonical/buildd/tests/buildd-slave-test.conf'
15--- lib/canonical/buildd/tests/buildd-slave-test.conf 2010-06-12 03:48:34 +0000
16+++ lib/canonical/buildd/tests/buildd-slave-test.conf 2010-09-20 13:43:43 +0000
17@@ -18,3 +18,10 @@
18 updatepath = /var/tmp/buildd/slavebin/update-debian-chroot
19 processscanpath = /var/tmp/buildd/slavebin/scan-for-processes
20 sourcespath = /usr/share/launchpad-buildd/slavebin/override-sources-list
21+
22+[binarypackagemanager]
23+sbuildpath = /var/tmp/buildd/slavebin/sbuild-package
24+sbuildargs = -dautobuild --nolog --batch
25+updatepath = /var/tmp/buildd/slavebin/update-debian-chroot
26+processscanpath = /var/tmp/buildd/slavebin/scan-for-processes
27+sourcespath = /usr/share/launchpad-buildd/slavebin/override-sources-list
28
29=== modified file 'lib/lp/buildmaster/manager.py'
30--- lib/lp/buildmaster/manager.py 2010-09-02 11:35:36 +0000
31+++ lib/lp/buildmaster/manager.py 2010-09-20 13:43:43 +0000
32@@ -106,6 +106,7 @@
33
34 def build(self, *args):
35 """Perform the build."""
36+ # XXX: This method does not appear to be used.
37 self.calls.append(('build', args))
38 result = buildd_success_result_map.get('build')
39 return [result, args[0]]
40@@ -275,6 +276,7 @@
41 def scheduleNextScanCycle(self):
42 """Schedule another scan of the builder some time in the future."""
43 self._deferred_list = []
44+ # XXX: Change this to use LoopingCall.
45 reactor.callLater(self.SCAN_INTERVAL, self.startCycle)
46
47 def startCycle(self):
48@@ -286,6 +288,7 @@
49 if slave is None:
50 self.scheduleNextScanCycle()
51 else:
52+ # XXX: Ought to return Deferred.
53 self.resumeAndDispatch(slave)
54 except:
55 error = Failure()
56@@ -362,9 +365,16 @@
57 return None
58
59 # See if there is a job we can dispatch to the builder slave.
60+
61+ # XXX: Rather than use the slave actually associated with the builder
62+ # (which, incidentally, shouldn't be a property anyway), we make a new
63+ # RecordingSlave so we can get access to its asynchronous
64+ # "resumeSlave" method. Blech.
65 slave = RecordingSlave(
66 self.builder.name, self.builder.url, self.builder.vm_host)
67- candidate = self.builder.findAndStartJob(buildd_slave=slave)
68+ # XXX: Passing buildd_slave=slave overwrites the 'slave' property of
69+ # self.builder. Not sure why this is needed yet.
70+ self.builder.findAndStartJob(buildd_slave=slave)
71 if self.builder.currentjob is not None:
72 # After a successful dispatch we can reset the
73 # failure_count.
74@@ -376,9 +386,13 @@
75
76 def resumeAndDispatch(self, slave):
77 """Chain the resume and dispatching Deferreds."""
78+ # XXX: resumeAndDispatch makes Deferreds without returning them.
79 if slave.resume_requested:
80 # The slave needs to be reset before we can dispatch to
81 # it (e.g. a virtual slave)
82+
83+ # XXX: Two problems here. The first is that 'resumeSlave' only
84+ # exists on RecordingSlave (BuilderSlave calls it 'resume').
85 d = slave.resumeSlave()
86 d.addBoth(self.checkResume, slave)
87 else:
88
89=== modified file 'lib/lp/buildmaster/model/builder.py'
90--- lib/lp/buildmaster/model/builder.py 2010-09-03 15:02:39 +0000
91+++ lib/lp/buildmaster/model/builder.py 2010-09-20 13:43:43 +0000
92@@ -112,17 +112,52 @@
93 return TimeoutHTTP(host)
94
95
96-class BuilderSlave(xmlrpclib.ServerProxy):
97+class BuilderSlave(object):
98 """Add in a few useful methods for the XMLRPC slave."""
99
100+ # XXX: This (BuilderSlave) should use composition, rather than
101+ # inheritance.
102+
103+ # XXX: Have a documented interface for the XML-RPC server:
104+ # - what methods
105+ # - what return values expected
106+ # - what faults
107+ # (see XMLRPCBuildDSlave in lib/canonical/buildd/slave.py).
108+
109+ # XXX: Arguably, this interface should be asynchronous
110+ # (i.e. Deferred-returning). This would mean that Builder (see below)
111+ # would have to expect Deferreds.
112+
113+ # XXX: Once we have a client object with a defined, tested interface, we
114+ # should make a test double that doesn't do any XML-RPC and can be used to
115+ # make testing easier & tests faster.
116+
117 def __init__(self, urlbase, vm_host):
118 """Initialise a Server with specific parameter to our buildfarm."""
119 self.vm_host = vm_host
120 self.urlbase = urlbase
121 rpc_url = urlappend(urlbase, "rpc")
122- xmlrpclib.Server.__init__(self, rpc_url,
123- transport=TimeoutTransport(),
124- allow_none=True)
125+ self._server = xmlrpclib.Server(
126+ rpc_url, transport=TimeoutTransport(), allow_none=True)
127+
128+ def abort(self):
129+ return self._server.abort()
130+
131+ def echo(self, *args):
132+ """Echo the arguments back."""
133+ return self._server.echo(*args)
134+
135+ def info(self):
136+ """Return the protocol version and the builder methods supported."""
137+ return self._server.info()
138+
139+ def status(self):
140+ """Return the status of the build daemon."""
141+ return self._server.status()
142+
143+ def ensurepresent(self, sha1sum, url, username, password):
144+ """Attempt to ensure the given file is present."""
145+ return self._server.ensurepresent(sha1sum, url, username, password)
146
147 def getFile(self, sha_sum):
148 """Construct a file-like object to return the named file."""
149@@ -138,6 +173,10 @@
150
151 :return: a (stdout, stderr, subprocess exitcode) triple
152 """
153+ # XXX: This executes the vm_resume_command
154+ # synchronously. RecordingSlave does so asynchronously. Since we
155+ # always want to do this asynchronously, there's no need for the
156+ # duplication.
157 resume_command = config.builddmaster.vm_resume_command % {
158 'vm_host': self.vm_host}
159 resume_argv = resume_command.split()
160@@ -176,12 +215,9 @@
161 :param args: A dictionary of extra arguments. The contents depend on
162 the build job type.
163 """
164- # Can't upcall to xmlrpclib.ServerProxy, since it doesn't actually
165- # have a 'build' method.
166- build_method = xmlrpclib.ServerProxy.__getattr__(self, 'build')
167 try:
168- return build_method(
169- self, buildid, builder_type, chroot_sha1, filemap, args)
170+ return self._server.build(
171+ buildid, builder_type, chroot_sha1, filemap, args)
172 except xmlrpclib.Fault, info:
173 raise BuildSlaveFailure(info)
174
175
176=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
177--- lib/lp/buildmaster/tests/test_builder.py 2010-08-30 15:00:23 +0000
178+++ lib/lp/buildmaster/tests/test_builder.py 2010-09-20 13:43:43 +0000
179@@ -4,11 +4,18 @@
180 """Test Builder features."""
181
182 import errno
183+import os
184 import socket
185+import xmlrpclib
186+
187+from testtools.content import Content
188+from testtools.content_type import UTF8_TEXT
189
190 from zope.component import getUtility
191 from zope.security.proxy import removeSecurityProxy
192
193+from canonical.buildd.slave import BuilderStatus
194+from canonical.buildd.tests.harness import BuilddSlaveTestSetup
195 from canonical.database.sqlbase import flush_database_updates
196 from canonical.launchpad.webapp.interfaces import (
197 DEFAULT_FLAVOR,
198@@ -25,6 +32,7 @@
199 IBuildFarmJobBehavior,
200 )
201 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
202+from lp.buildmaster.model.builder import BuilderSlave
203 from lp.buildmaster.model.buildfarmjobbehavior import IdleBuildBehavior
204 from lp.buildmaster.model.buildqueue import BuildQueue
205 from lp.soyuz.enums import (
206@@ -40,7 +48,10 @@
207 MockBuilder,
208 )
209 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
210-from lp.testing import TestCaseWithFactory
211+from lp.testing import (
212+ TestCase,
213+ TestCaseWithFactory,
214+ )
215 from lp.testing.fakemethod import FakeMethod
216
217
218@@ -454,3 +465,148 @@
219
220 self.assertIsInstance(
221 self.builder.current_build_behavior, BinaryPackageBuildBehavior)
222+
223+
224+class TestSlave(TestCase):
225+ """
226+ Integration tests for BuilderSlave that verify how it works against a
227+ real slave server.
228+ """
229+
230+ # XXX: JonathanLange 2010-09-20 bug=643521: There are also tests for
231+ # BuilderSlave in buildd-slave.txt and in other places. The tests here
232+ # ought to become the canonical tests for BuilderSlave vs running buildd
233+ # XML-RPC server interaction.
234+
235+ # The URL for the XML-RPC service set up by `BuilddSlaveTestSetup`.
236+ TEST_URL = 'http://localhost:8221/rpc/'
237+
238+ def getServerSlave(self):
239+ """Set up a test build slave server.
240+
241+ :return: A `BuilddSlaveTestSetup` object.
242+ """
243+ tachandler = BuilddSlaveTestSetup()
244+ tachandler.setUp()
245+ def addLogFile(exc_info):
246+ self.addDetail(
247+ 'xmlrpc-log-file',
248+ Content(UTF8_TEXT, lambda: open(tachandler.logfile, 'r').read()))
249+ self.addOnException(addLogFile)
250+ self.addCleanup(tachandler.tearDown)
251+ return tachandler
252+
253+ def getClientSlave(self):
254+ """Return a `BuilderSlave` for use in testing.
255+
256+ Points to a fixed URL that is also used by `BuilddSlaveTestSetup`.
257+ """
258+ return BuilderSlave(self.TEST_URL, 'vmhost')
259+
260+ def makeCacheFile(self, tachandler, filename):
261+ """Make a cache file available on the remote slave.
262+
263+ :param tachandler: The TacTestSetup object used to start the remote
264+ slave.
265+ :param filename: The name of the file to create in the file cache
266+ area.
267+ """
268+ path = os.path.join(tachandler.root, 'filecache', filename)
269+ fd = open(path, 'w')
270+ fd.write('something')
271+ fd.close()
272+ self.addCleanup(os.unlink, path)
273+
274+ def triggerGoodBuild(self, slave, build_id=None):
275+ """Trigger a good build on 'slave'.
276+
277+ :param slave: A `BuilderSlave` instance to trigger the build on.
278+ :param build_id: The build identifier. If not specified, defaults to
279+ an arbitrary string.
280+ :type build_id: str
281+ :return: The build id returned by the slave.
282+ """
283+ if build_id is None:
284+ build_id = self.getUniqueString()
285+ tachandler = self.getServerSlave()
286+ chroot_file = 'fake-chroot'
287+ dsc_file = 'thing'
288+ self.makeCacheFile(tachandler, chroot_file)
289+ self.makeCacheFile(tachandler, dsc_file)
290+ return slave.build(
291+ build_id, 'debian', chroot_file, {'.dsc': dsc_file},
292+ {'ogrecomponent': 'main'})
293+
294+ def test_abort(self):
295+ slave = self.getClientSlave()
296+ # We need to be in a BUILDING state before we can abort.
297+ self.triggerGoodBuild(slave)
298+ result = slave.abort()
299+ self.assertEqual(result, BuilderStatus.ABORTING)
300+
301+ def test_build(self):
302+ # Calling 'build' with an expected builder type, a good build id,
303+ # valid chroot & filemaps works and returns a BuilderStatus of
304+ # BUILDING.
305+ build_id = 'some-id'
306+ slave = self.getClientSlave()
307+ result = self.triggerGoodBuild(slave, build_id)
308+ self.assertEqual([BuilderStatus.BUILDING, build_id], result)
309+
310+ def test_echo(self):
311+ # Calling 'echo' contacts the server which returns the arguments we
312+ # gave it.
313+ self.getServerSlave()
314+ slave = self.getClientSlave()
315+ result = slave.echo('foo', 'bar', 42)
316+ self.assertEqual(['foo', 'bar', 42], result)
317+
318+ def test_info(self):
319+ # Calling 'info' gets some information about the slave.
320+ self.getServerSlave()
321+ slave = self.getClientSlave()
322+ result = slave.info()
323+ # We're testing the hard-coded values, since the version is hard-coded
324+ # into the remote slave, the supported build managers are hard-coded
325+ # into the tac file for the remote slave and config is returned from
326+ # the configuration file.
327+ self.assertEqual(
328+ ['1.0',
329+ 'i386',
330+ ['sourcepackagerecipe',
331+ 'translation-templates', 'binarypackage', 'debian']],
332+ result)
333+
334+ def test_initial_status(self):
335+ # Calling 'status' returns the current status of the slave. The
336+ # initial status is IDLE.
337+ self.getServerSlave()
338+ slave = self.getClientSlave()
339+ status = slave.status()
340+ self.assertEqual([BuilderStatus.IDLE, ''], status)
341+
342+ def test_status_after_build(self):
343+ # Calling 'status' returns the current status of the slave. After a
344+ # build has been triggered, the status is BUILDING.
345+ slave = self.getClientSlave()
346+ build_id = 'status-build-id'
347+ self.triggerGoodBuild(slave, build_id)
348+ status = slave.status()
349+ self.assertEqual([BuilderStatus.BUILDING, build_id], status[:2])
350+ [log_file] = status[2:]
351+ self.assertIsInstance(log_file, xmlrpclib.Binary)
352+
353+ def test_ensurepresent_not_there(self):
354+ # ensurepresent checks to see if a file is there.
355+ self.getServerSlave()
356+ slave = self.getClientSlave()
357+ result = slave.ensurepresent('blahblah', None, None, None)
358+ self.assertEqual([False, 'No URL'], result)
359+
360+ def test_ensurepresent_actually_there(self):
361+ # ensurepresent checks to see if a file is there.
362+ tachandler = self.getServerSlave()
363+ slave = self.getClientSlave()
364+ self.makeCacheFile(tachandler, 'blahblah')
365+ result = slave.ensurepresent('blahblah', None, None, None)
366+ self.assertEqual([True, 'No URL'], result)
367
368=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
369--- lib/lp/buildmaster/tests/test_manager.py 2010-08-30 15:00:23 +0000
370+++ lib/lp/buildmaster/tests/test_manager.py 2010-09-20 13:43:43 +0000
371@@ -6,7 +6,6 @@
372 import os
373 import signal
374 import time
375-import unittest
376
377 import transaction
378
379
380=== modified file 'lib/lp/soyuz/tests/soyuzbuilddhelpers.py'
381--- lib/lp/soyuz/tests/soyuzbuilddhelpers.py 2010-08-20 20:31:18 +0000
382+++ lib/lp/soyuz/tests/soyuzbuilddhelpers.py 2010-09-20 13:43:43 +0000
383@@ -61,10 +61,14 @@
384 slave_build_id)
385
386 def cleanSlave(self):
387+ # XXX: This should not print anything. The print is only here to make
388+ # doc/builder.txt a meaningful test.
389 print 'Cleaning slave'
390 return self.slave.clean()
391
392 def requestAbort(self):
393+ # XXX: This should not print anything. The print is only here to make
394+ # doc/builder.txt a meaningful test.
395 print 'Aborting slave'
396 return self.slave.abort()
397