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

[Jonathan Lange]

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

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

[Julian Edwards (julian-edwards)]

review: Approve

=== modified file 'lib/canonical/buildd/slave.py'
--- lib/canonical/buildd/slave.py	2010-05-20 11:19:56 +0000
+++ lib/canonical/buildd/slave.py	2010-09-20 13:43:43 +0000
@@ -356,6 +356,8 @@
         # for abort to complete. This is potentially an issue in a heavy
         # load situation.
         if self.builderstatus != BuilderStatus.BUILDING:
+            # XXX: Should raise a known Fault so that the client can make
+            # useful decisions about the error!
             raise ValueError("Slave is not BUILDING when asked to abort")
         self.manager.abort()
         self.builderstatus = BuilderStatus.ABORTING

=== modified file 'lib/canonical/buildd/tests/buildd-slave-test.conf'
--- lib/canonical/buildd/tests/buildd-slave-test.conf	2010-06-12 03:48:34 +0000
+++ lib/canonical/buildd/tests/buildd-slave-test.conf	2010-09-20 13:43:43 +0000
@@ -18,3 +18,10 @@
 updatepath = /var/tmp/buildd/slavebin/update-debian-chroot
 processscanpath = /var/tmp/buildd/slavebin/scan-for-processes
 sourcespath = /usr/share/launchpad-buildd/slavebin/override-sources-list
+
+[binarypackagemanager]
+sbuildpath = /var/tmp/buildd/slavebin/sbuild-package
+sbuildargs = -dautobuild --nolog --batch
+updatepath = /var/tmp/buildd/slavebin/update-debian-chroot
+processscanpath = /var/tmp/buildd/slavebin/scan-for-processes
+sourcespath = /usr/share/launchpad-buildd/slavebin/override-sources-list

=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2010-09-02 11:35:36 +0000
+++ lib/lp/buildmaster/manager.py	2010-09-20 13:43:43 +0000
@@ -106,6 +106,7 @@
 
     def build(self, *args):
         """Perform the build."""
+        # XXX: This method does not appear to be used.
         self.calls.append(('build', args))
         result = buildd_success_result_map.get('build')
         return [result, args[0]]
@@ -275,6 +276,7 @@
     def scheduleNextScanCycle(self):
         """Schedule another scan of the builder some time in the future."""
         self._deferred_list = []
+        # XXX: Change this to use LoopingCall.
         reactor.callLater(self.SCAN_INTERVAL, self.startCycle)
 
     def startCycle(self):
@@ -286,6 +288,7 @@
             if slave is None:
                 self.scheduleNextScanCycle()
             else:
+                # XXX: Ought to return Deferred.
                 self.resumeAndDispatch(slave)
         except:
             error = Failure()
@@ -362,9 +365,16 @@
             return None
 
         # See if there is a job we can dispatch to the builder slave.
+
+        # XXX: Rather than use the slave actually associated with the builder
+        # (which, incidentally, shouldn't be a property anyway), we make a new
+        # RecordingSlave so we can get access to its asynchronous
+        # "resumeSlave" method. Blech.
         slave = RecordingSlave(
             self.builder.name, self.builder.url, self.builder.vm_host)
-        candidate = self.builder.findAndStartJob(buildd_slave=slave)
+        # XXX: Passing buildd_slave=slave overwrites the 'slave' property of
+        # self.builder. Not sure why this is needed yet.
+        self.builder.findAndStartJob(buildd_slave=slave)
         if self.builder.currentjob is not None:
             # After a successful dispatch we can reset the
             # failure_count.
@@ -376,9 +386,13 @@
 
     def resumeAndDispatch(self, slave):
         """Chain the resume and dispatching Deferreds."""
+        # XXX: resumeAndDispatch makes Deferreds without returning them.
         if slave.resume_requested:
             # The slave needs to be reset before we can dispatch to
             # it (e.g. a virtual slave)
+
+            # XXX: Two problems here. The first is that 'resumeSlave' only
+            # exists on RecordingSlave (BuilderSlave calls it 'resume').
             d = slave.resumeSlave()
             d.addBoth(self.checkResume, slave)
         else:

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2010-09-03 15:02:39 +0000
+++ lib/lp/buildmaster/model/builder.py	2010-09-20 13:43:43 +0000
@@ -112,17 +112,52 @@
         return TimeoutHTTP(host)
 
 
-class BuilderSlave(xmlrpclib.ServerProxy):
+class BuilderSlave(object):
     """Add in a few useful methods for the XMLRPC slave."""
 
+    # XXX: This (BuilderSlave) should use composition, rather than
+    # inheritance.
+
+    # XXX: Have a documented interface for the XML-RPC server:
+    #  - what methods
+    #  - what return values expected
+    #  - what faults
+    #  (see XMLRPCBuildDSlave in lib/canonical/buildd/slave.py).
+
+    # XXX: Arguably, this interface should be asynchronous
+    # (i.e. Deferred-returning). This would mean that Builder (see below)
+    # would have to expect Deferreds.
+
+    # XXX: Once we have a client object with a defined, tested interface, we
+    # should make a test double that doesn't do any XML-RPC and can be used to
+    # make testing easier & tests faster.
+
     def __init__(self, urlbase, vm_host):
         """Initialise a Server with specific parameter to our buildfarm."""
         self.vm_host = vm_host
         self.urlbase = urlbase
         rpc_url = urlappend(urlbase, "rpc")
-        xmlrpclib.Server.__init__(self, rpc_url,
-                                  transport=TimeoutTransport(),
-                                  allow_none=True)
+        self._server = xmlrpclib.Server(
+            rpc_url, transport=TimeoutTransport(), allow_none=True)
+
+    def abort(self):
+        return self._server.abort()
+
+    def echo(self, *args):
+        """Echo the arguments back."""
+        return self._server.echo(*args)
+
+    def info(self):
+        """Return the protocol version and the builder methods supported."""
+        return self._server.info()
+
+    def status(self):
+        """Return the status of the build daemon."""
+        return self._server.status()
+
+    def ensurepresent(self, sha1sum, url, username, password):
+        """Attempt to ensure the given file is present."""
+        return self._server.ensurepresent(sha1sum, url, username, password)
 
     def getFile(self, sha_sum):
         """Construct a file-like object to return the named file."""
@@ -138,6 +173,10 @@
 
         :return: a (stdout, stderr, subprocess exitcode) triple
         """
+        # XXX: This executes the vm_resume_command
+        # synchronously. RecordingSlave does so asynchronously. Since we
+        # always want to do this asynchronously, there's no need for the
+        # duplication.
         resume_command = config.builddmaster.vm_resume_command % {
             'vm_host': self.vm_host}
         resume_argv = resume_command.split()
@@ -176,12 +215,9 @@
         :param args: A dictionary of extra arguments. The contents depend on
             the build job type.
         """
-        # Can't upcall to xmlrpclib.ServerProxy, since it doesn't actually
-        # have a 'build' method.
-        build_method = xmlrpclib.ServerProxy.__getattr__(self, 'build')
         try:
-            return build_method(
-                self, buildid, builder_type, chroot_sha1, filemap, args)
+            return self._server.build(
+                buildid, builder_type, chroot_sha1, filemap, args)
         except xmlrpclib.Fault, info:
             raise BuildSlaveFailure(info)
 

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2010-08-30 15:00:23 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2010-09-20 13:43:43 +0000
@@ -4,11 +4,18 @@
 """Test Builder features."""
 
 import errno
+import os
 import socket
+import xmlrpclib
+
+from testtools.content import Content
+from testtools.content_type import UTF8_TEXT
 
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from canonical.buildd.slave import BuilderStatus
+from canonical.buildd.tests.harness import BuilddSlaveTestSetup
 from canonical.database.sqlbase import flush_database_updates
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR,
@@ -25,6 +32,7 @@
     IBuildFarmJobBehavior,
     )
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
+from lp.buildmaster.model.builder import BuilderSlave
 from lp.buildmaster.model.buildfarmjobbehavior import IdleBuildBehavior
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.soyuz.enums import (
@@ -40,7 +48,10 @@
     MockBuilder,
     )
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    TestCase,
+    TestCaseWithFactory,
+    )
 from lp.testing.fakemethod import FakeMethod
 
 
@@ -454,3 +465,148 @@
 
         self.assertIsInstance(
             self.builder.current_build_behavior, BinaryPackageBuildBehavior)
+
+
+class TestSlave(TestCase):
+    """
+    Integration tests for BuilderSlave that verify how it works against a
+    real slave server.
+    """
+
+    # XXX: JonathanLange 2010-09-20 bug=643521: There are also tests for
+    # BuilderSlave in buildd-slave.txt and in other places. The tests here
+    # ought to become the canonical tests for BuilderSlave vs running buildd
+    # XML-RPC server interaction.
+
+    # The URL for the XML-RPC service set up by `BuilddSlaveTestSetup`.
+    TEST_URL = 'http://localhost:8221/rpc/'
+
+    def getServerSlave(self):
+        """Set up a test build slave server.
+
+        :return: A `BuilddSlaveTestSetup` object.
+        """
+        tachandler = BuilddSlaveTestSetup()
+        tachandler.setUp()
+        def addLogFile(exc_info):
+            self.addDetail(
+                'xmlrpc-log-file',
+                Content(UTF8_TEXT, lambda: open(tachandler.logfile, 'r').read()))
+        self.addOnException(addLogFile)
+        self.addCleanup(tachandler.tearDown)
+        return tachandler
+
+    def getClientSlave(self):
+        """Return a `BuilderSlave` for use in testing.
+
+        Points to a fixed URL that is also used by `BuilddSlaveTestSetup`.
+        """
+        return BuilderSlave(self.TEST_URL, 'vmhost')
+
+    def makeCacheFile(self, tachandler, filename):
+        """Make a cache file available on the remote slave.
+
+        :param tachandler: The TacTestSetup object used to start the remote
+            slave.
+        :param filename: The name of the file to create in the file cache
+            area.
+        """
+        path = os.path.join(tachandler.root, 'filecache', filename)
+        fd = open(path, 'w')
+        fd.write('something')
+        fd.close()
+        self.addCleanup(os.unlink, path)
+
+    def triggerGoodBuild(self, slave, build_id=None):
+        """Trigger a good build on 'slave'.
+
+        :param slave: A `BuilderSlave` instance to trigger the build on.
+        :param build_id: The build identifier. If not specified, defaults to
+            an arbitrary string.
+        :type build_id: str
+        :return: The build id returned by the slave.
+        """
+        if build_id is None:
+            build_id = self.getUniqueString()
+        tachandler = self.getServerSlave()
+        chroot_file = 'fake-chroot'
+        dsc_file = 'thing'
+        self.makeCacheFile(tachandler, chroot_file)
+        self.makeCacheFile(tachandler, dsc_file)
+        return slave.build(
+            build_id, 'debian', chroot_file, {'.dsc': dsc_file},
+            {'ogrecomponent': 'main'})
+
+    def test_abort(self):
+        slave = self.getClientSlave()
+        # We need to be in a BUILDING state before we can abort.
+        self.triggerGoodBuild(slave)
+        result = slave.abort()
+        self.assertEqual(result, BuilderStatus.ABORTING)
+
+    def test_build(self):
+        # Calling 'build' with an expected builder type, a good build id,
+        # valid chroot & filemaps works and returns a BuilderStatus of
+        # BUILDING.
+        build_id = 'some-id'
+        slave = self.getClientSlave()
+        result = self.triggerGoodBuild(slave, build_id)
+        self.assertEqual([BuilderStatus.BUILDING, build_id], result)
+
+    def test_echo(self):
+        # Calling 'echo' contacts the server which returns the arguments we
+        # gave it.
+        self.getServerSlave()
+        slave = self.getClientSlave()
+        result = slave.echo('foo', 'bar', 42)
+        self.assertEqual(['foo', 'bar', 42], result)
+
+    def test_info(self):
+        # Calling 'info' gets some information about the slave.
+        self.getServerSlave()
+        slave = self.getClientSlave()
+        result = slave.info()
+        # We're testing the hard-coded values, since the version is hard-coded
+        # into the remote slave, the supported build managers are hard-coded
+        # into the tac file for the remote slave and config is returned from
+        # the configuration file.
+        self.assertEqual(
+            ['1.0',
+             'i386',
+             ['sourcepackagerecipe',
+              'translation-templates', 'binarypackage', 'debian']],
+            result)
+
+    def test_initial_status(self):
+        # Calling 'status' returns the current status of the slave. The
+        # initial status is IDLE.
+        self.getServerSlave()
+        slave = self.getClientSlave()
+        status = slave.status()
+        self.assertEqual([BuilderStatus.IDLE, ''], status)
+
+    def test_status_after_build(self):
+        # Calling 'status' returns the current status of the slave. After a
+        # build has been triggered, the status is BUILDING.
+        slave = self.getClientSlave()
+        build_id = 'status-build-id'
+        self.triggerGoodBuild(slave, build_id)
+        status = slave.status()
+        self.assertEqual([BuilderStatus.BUILDING, build_id], status[:2])
+        [log_file] = status[2:]
+        self.assertIsInstance(log_file, xmlrpclib.Binary)
+
+    def test_ensurepresent_not_there(self):
+        # ensurepresent checks to see if a file is there.
+        self.getServerSlave()
+        slave = self.getClientSlave()
+        result = slave.ensurepresent('blahblah', None, None, None)
+        self.assertEqual([False, 'No URL'], result)
+
+    def test_ensurepresent_actually_there(self):
+        # ensurepresent checks to see if a file is there.
+        tachandler = self.getServerSlave()
+        slave = self.getClientSlave()
+        self.makeCacheFile(tachandler, 'blahblah')
+        result = slave.ensurepresent('blahblah', None, None, None)
+        self.assertEqual([True, 'No URL'], result)

=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2010-08-30 15:00:23 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2010-09-20 13:43:43 +0000
@@ -6,7 +6,6 @@
 import os
 import signal
 import time
-import unittest
 
 import transaction
 

=== modified file 'lib/lp/soyuz/tests/soyuzbuilddhelpers.py'
--- lib/lp/soyuz/tests/soyuzbuilddhelpers.py	2010-08-20 20:31:18 +0000
+++ lib/lp/soyuz/tests/soyuzbuilddhelpers.py	2010-09-20 13:43:43 +0000
@@ -61,10 +61,14 @@
             slave_build_id)
 
     def cleanSlave(self):
+        # XXX: This should not print anything. The print is only here to make
+        # doc/builder.txt a meaningful test.
         print 'Cleaning slave'
         return self.slave.clean()
 
     def requestAbort(self):
+        # XXX: This should not print anything. The print is only here to make
+        # doc/builder.txt a meaningful test.
         print 'Aborting slave'
         return self.slave.abort()