← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~wgrant/launchpad/dispatchBuildToSlave-log-betterer into lp:launchpad

 

William Grant has proposed merging lp:~wgrant/launchpad/dispatchBuildToSlave-log-betterer into lp:launchpad with lp:~wgrant/launchpad/dispatchBuildToSlave-common as a prerequisite.

Commit message:
Rework buildd-manager job dispatch logging.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/dispatchBuildToSlave-log-betterer/+merge/224435

This branch redesigns buildd-manager's logging during job dispatches.

Previously, each BuildFarmJobBehaviour implementation provided logStartBuild, which described the build in variously useless and inconsistent ways. dispatchBuildToSlave then hideously logged things *after* the dispatch completed, with lots of asterisks.

logStartBuild is dead. dispatchBuildToSlave now logs once at the very start, once when all the files are sent, and once when the build is actually started. We also now sanitise URLs containing credentials when we log build parameters.

Each BuildFarmJobBehaviour implementation's dispatchBuildToSlave test has also been replaced with a composeBuildRequest test, simplifying things a lot.
-- 
https://code.launchpad.net/~wgrant/launchpad/dispatchBuildToSlave-log-betterer/+merge/224435
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/dispatchBuildToSlave-log-betterer into lp:launchpad.
=== modified file 'lib/lp/buildmaster/doc/buildfarmjobbehaviour.txt'
--- lib/lp/buildmaster/doc/buildfarmjobbehaviour.txt	2014-01-30 15:04:06 +0000
+++ lib/lp/buildmaster/doc/buildfarmjobbehaviour.txt	2014-06-25 12:30:38 +0000
@@ -31,7 +31,7 @@
     ...     """A custom build behaviour for building blah."""
     ...     implements(IBuildFarmJobBehaviour)
     ...
-    ...     def dispatchBuildToSlave(self, build_queue_item_id, logger):
+    ...     def dispatchBuildToSlave(self, logger):
     ...         print "Did something special to dispatch MySpecialBuild."
 
 For this documentation, we'll also need a dummy new build farm job.

=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py	2014-06-25 12:30:38 +0000
+++ lib/lp/buildmaster/interactor.py	2014-06-25 12:30:38 +0000
@@ -352,7 +352,6 @@
             value is None, or a Failure that contains an exception
             explaining what went wrong.
         """
-        behaviour.logStartBuild(logger)
         behaviour.verifyBuildRequest(logger)
 
         # Set the build behaviour depending on the provided build queue item.
@@ -367,7 +366,7 @@
         builder.setCleanStatus(BuilderCleanStatus.DIRTY)
         transaction.commit()
 
-        yield behaviour.dispatchBuildToSlave(build_queue_item.id, logger)
+        yield behaviour.dispatchBuildToSlave(logger)
 
     @classmethod
     @defer.inlineCallbacks

=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py'
--- lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py	2014-06-25 12:30:38 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjobbehaviour.py	2014-06-25 12:30:38 +0000
@@ -17,14 +17,6 @@
     def setBuilder(builder, slave):
         """Sets the associated builder and slave for this instance."""
 
-    def logStartBuild(logger):
-        """Log the start of a specific build queue item.
-
-        The form of the log message will vary depending on the type of build.
-        :param build_queue_item: A BuildQueueItem to build.
-        :param logger: A logger to be used to log diagnostic information.
-        """
-
     def composeBuildRequest(logger):
         """Compose parameters for a slave build request.
 
@@ -34,10 +26,9 @@
             {filename: `sendFileToSlave` arguments}, {extra build arguments})
         """
 
-    def dispatchBuildToSlave(build_queue_item_id, logger):
+    def dispatchBuildToSlave(logger):
         """Dispatch a specific build to the slave.
 
-        :param build_queue_item_id: An identifier for the build queue item.
         :param logger: A logger to be used to log diagnostic information.
         """
 

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehaviour.py'
--- lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2014-06-25 12:30:38 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2014-06-25 12:30:38 +0000
@@ -13,6 +13,7 @@
 import gzip
 import logging
 import os
+import re
 import tempfile
 
 import transaction
@@ -33,6 +34,18 @@
 SLAVE_LOG_FILENAME = 'buildlog'
 
 
+def sanitise_arguments(s):
+    """Sanitise a string of arguments for logging.
+
+    Some jobs are started with arguments that probably shouldn't be
+    logged in their entirety (usernames and passwords for P3As, for
+    example. This function removes them.
+    """
+    # Remove credentials from URLs.
+    password_re = re.compile('://([^:]+:[^@]+@)(\S+)')
+    return password_re.sub(r'://<redacted>@\2', s)
+
+
 class BuildFarmJobBehaviourBase:
     """Ensures that all behaviours inherit the same initialization.
 
@@ -58,8 +71,13 @@
         return '%s-%s' % (self.build.job_type.name, self.build.id)
 
     @defer.inlineCallbacks
-    def dispatchBuildToSlave(self, build_queue_id, logger):
+    def dispatchBuildToSlave(self, logger):
         """See `IBuildFarmJobBehaviour`."""
+        cookie = self.getBuildCookie()
+        logger.info(
+            "Preparing job %s (%s) on %s."
+            % (cookie, self.build.title, self._builder.url))
+
         builder_type, das, files, args = self.composeBuildRequest(logger)
 
         # First cache the chroot and any other files that the job needs.
@@ -67,6 +85,7 @@
         if chroot is None:
             raise CannotBuild(
                 "Unable to find a chroot for %s" % das.displayname)
+
         filename_to_sha1 = {}
         dl = []
         dl.append(self._slave.sendFileToSlave(
@@ -76,33 +95,20 @@
             dl.append(self._slave.sendFileToSlave(logger=logger, **params))
         yield defer.gatherResults(dl)
 
-        # Generate a string which can be used to cross-check when
-        # obtaining results so we know we are referring to the right
-        # database object in subsequent runs.
-        buildid = "%s-%s" % (self.build.id, build_queue_id)
-        cookie = self.getBuildCookie()
-        chroot_sha1 = chroot.content.sha1
-        logger.debug(
-            "Initiating build %s on %s" % (buildid, self._builder.url))
+        combined_args = {
+            'builder_type': builder_type, 'chroot_sha1': chroot.content.sha1,
+            'filemap': filename_to_sha1, 'args': args}
+        logger.info(
+            "Dispatching job %s (%s) to %s:\n%s"
+            % (cookie, self.build.title, self._builder.url,
+               sanitise_arguments(repr(combined_args))))
 
         (status, info) = yield self._slave.build(
-            cookie, builder_type, chroot_sha1, filename_to_sha1, args)
+            cookie, builder_type, chroot.content.sha1, filename_to_sha1, args)
 
-        message = """%s (%s):
-        ***** RESULT *****
-        %s
-        %s
-        %s: %s
-        ******************
-        """ % (
-            self._builder.name,
-            self._builder.url,
-            filename_to_sha1,
-            args,
-            status,
-            info,
-            )
-        logger.info(message)
+        logger.info(
+            "Job %s (%s) started on %s: %s %s"
+            % (cookie, self.build.title, self._builder.url, status, info))
 
     def getUploadDirLeaf(self, build_cookie, now=None):
         """See `IPackageBuild`."""

=== modified file 'lib/lp/buildmaster/tests/mock_slaves.py'
--- lib/lp/buildmaster/tests/mock_slaves.py	2014-06-25 12:30:38 +0000
+++ lib/lp/buildmaster/tests/mock_slaves.py	2014-06-25 12:30:38 +0000
@@ -104,8 +104,7 @@
     def build(self, buildid, buildtype, chroot, filemap, args):
         self.call_log.append(
             ('build', buildid, buildtype, chroot, filemap.keys(), args))
-        info = 'OkSlave BUILDING'
-        return defer.succeed(('BuildStatus.Building', info))
+        return defer.succeed(('BuildStatus.BUILDING', buildid))
 
     def echo(self, *args):
         self.call_log.append(('echo',) + args)

=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py'
--- lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py	2014-06-20 10:19:30 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjobbehaviour.py	2014-06-25 12:30:38 +0000
@@ -6,16 +6,21 @@
 __metaclass__ = type
 
 from datetime import datetime
+import hashlib
 import os
 import shutil
 import tempfile
 
+from testtools.deferredruntest import AsynchronousDeferredRunTest
 from twisted.internet import defer
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.archiveuploader.uploadprocessor import parse_build_upload_leaf_name
-from lp.buildmaster.enums import BuildStatus
+from lp.buildmaster.enums import (
+    BuildFarmJobType,
+    BuildStatus,
+    )
 from lp.buildmaster.interactor import BuilderInteractor
 from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
     IBuildFarmJobBehaviour,
@@ -23,11 +28,19 @@
 from lp.buildmaster.model.buildfarmjobbehaviour import (
     BuildFarmJobBehaviourBase,
     )
-from lp.buildmaster.tests.mock_slaves import WaitingSlave
+from lp.buildmaster.tests.mock_slaves import (
+    MockBuilder,
+    OkSlave,
+    WaitingSlave,
+    )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.config import config
+from lp.services.log.logger import BufferLogger
 from lp.soyuz.interfaces.processor import IProcessorSet
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    TestCase,
+    TestCaseWithFactory,
+    )
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import (
@@ -39,7 +52,30 @@
 
 class FakeBuildFarmJob:
     """Dummy BuildFarmJob."""
-    pass
+
+    id = 1
+    job_type = BuildFarmJobType.PACKAGEBUILD
+    title = 'some job for something'
+
+
+class FakeLibraryFileContent:
+
+    def __init__(self, filename):
+        self.sha1 = hashlib.sha1(filename).hexdigest()
+
+
+class FakeLibraryFileAlias:
+
+    def __init__(self, filename):
+        self.filename = filename
+        self.content = FakeLibraryFileContent(filename)
+        self.http_url = 'http://librarian.dev/%s' % filename
+
+
+class FakeDistroArchSeries:
+
+    def getChroot(self):
+        return FakeLibraryFileAlias('chroot-fooix-bar-y86.tar.bz2')
 
 
 class TestBuildFarmJobBehaviourBase(TestCaseWithFactory):
@@ -89,6 +125,57 @@
             upload_leaf)
 
 
+class TestDispatchBuildToSlave(TestCase):
+
+    run_tests_with = AsynchronousDeferredRunTest
+
+    @defer.inlineCallbacks
+    def test_dispatchBuildToSlave(self):
+        files = {
+            'foo.dsc': {'url': 'http://host/foo.dsc', 'sha1': '0'},
+            'bar.tar': {
+                'url': 'http://host/bar.tar', 'sha1': '0',
+                'username': 'admin', 'password': 'sekrit'}}
+
+        behaviour = BuildFarmJobBehaviourBase(FakeBuildFarmJob())
+        builder = MockBuilder()
+        slave = OkSlave()
+        logger = BufferLogger()
+        behaviour.composeBuildRequest = FakeMethod(
+            ('foobuild', FakeDistroArchSeries(), files,
+             {'some': 'arg', 'archives': ['http://admin:sekrit@blah/']}))
+        behaviour.setBuilder(builder, slave)
+        yield behaviour.dispatchBuildToSlave(logger)
+
+        # The slave's been asked to cache the chroot and both source
+        # files, and then to start the build.
+        expected_calls = [
+            ('ensurepresent',
+             'http://librarian.dev/chroot-fooix-bar-y86.tar.bz2', '', ''),
+            ('ensurepresent', 'http://host/bar.tar', 'admin', 'sekrit'),
+            ('ensurepresent', 'http://host/foo.dsc', '', ''),
+            ('build', 'PACKAGEBUILD-1', 'foobuild',
+             hashlib.sha1('chroot-fooix-bar-y86.tar.bz2').hexdigest(),
+             ['foo.dsc', 'bar.tar'],
+             {'archives': ['http://admin:sekrit@blah/'], 'some': 'arg'})]
+        self.assertEqual(expected_calls, slave.call_log)
+
+        # And details have been logged, including the build arguments
+        # with credentials redacted.
+        self.assertStartsWith(
+            logger.getLogBuffer(),
+            "INFO Preparing job PACKAGEBUILD-1 (some job for something) on "
+            "http://fake:0000.\n";
+            "INFO Dispatching job PACKAGEBUILD-1 (some job for something) to "
+            "http://fake:0000:\n{";)
+        self.assertIn('http://<redacted>@blah/', logger.getLogBuffer())
+        self.assertNotIn('sekrit', logger.getLogBuffer())
+        self.assertEndsWith(
+            logger.getLogBuffer(),
+            "INFO Job PACKAGEBUILD-1 (some job for something) started on "
+            "http://fake:0000: BuildStatus.BUILDING PACKAGEBUILD-1\n")
+
+
 class TestGetUploadMethodsMixin:
     """Tests for `IPackageBuild` that need objects from the rest of LP."""
 

=== modified file 'lib/lp/code/model/recipebuilder.py'
--- lib/lp/code/model/recipebuilder.py	2014-06-25 12:30:38 +0000
+++ lib/lp/code/model/recipebuilder.py	2014-06-25 12:30:38 +0000
@@ -44,19 +44,6 @@
     ALLOWED_STATUS_NOTIFICATIONS = [
         'OK', 'PACKAGEFAIL', 'DEPFAIL', 'CHROOTFAIL']
 
-    @property
-    def display_name(self):
-        ret = "%s, %s, %s" % (
-            self.build.distroseries.displayname, self.build.recipe.name,
-            self.build.recipe.owner.name)
-        if self._builder is not None:
-            ret += " (on %s)" % self._builder.url
-        return ret
-
-    def logStartBuild(self, logger):
-        """See `IBuildFarmJobBehaviour`."""
-        logger.info("startBuild(%s)", self.display_name)
-
     def _extraBuildArgs(self, distroarchseries, logger=None):
         """
         Return the extra arguments required by the slave for the given build.

=== modified file 'lib/lp/code/model/tests/test_recipebuilder.py'
--- lib/lp/code/model/tests/test_recipebuilder.py	2014-06-25 12:30:38 +0000
+++ lib/lp/code/model/tests/test_recipebuilder.py	2014-06-25 12:30:38 +0000
@@ -7,23 +7,14 @@
 
 import shutil
 import tempfile
-from textwrap import dedent
 
-from testtools import run_test_with
-from testtools.deferredruntest import (
-    assert_fails_with,
-    AsynchronousDeferredRunTest,
-    )
-from testtools.matchers import StartsWith
 import transaction
-from twisted.internet import defer
 from twisted.trial.unittest import TestCase as TrialTestCase
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interactor import BuilderInteractor
-from lp.buildmaster.interfaces.builder import CannotBuild
 from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
     IBuildFarmJobBehaviour,
     )
@@ -102,20 +93,6 @@
         job = IBuildFarmJobBehaviour(build)
         self.assertProvides(job, IBuildFarmJobBehaviour)
 
-    def test_display_name(self):
-        # display_name contains a sane description of the job
-        job = self.makeJob()
-        self.assertEquals(job.display_name,
-            "Mydistro, recept, joe")
-
-    def test_logStartBuild(self):
-        # logStartBuild will properly report the package that's being built
-        job = self.makeJob()
-        logger = BufferLogger()
-        job.logStartBuild(logger)
-        self.assertEquals(logger.getLogBuffer(),
-            "INFO startBuild(Mydistro, recept, joe)\n")
-
     def test_verifyBuildRequest_valid(self):
         # VerifyBuildRequest won't raise any exceptions when called with a
         # valid builder set.
@@ -289,48 +266,18 @@
         self.assertEquals(
             job.build, SourcePackageRecipeBuild.getByID(job.build.id))
 
-    @run_test_with(AsynchronousDeferredRunTest)
-    def test_dispatchBuildToSlave(self):
-        # Ensure dispatchBuildToSlave will make the right calls to the slave
+    def test_composeBuildRequest(self):
+        # Ensure composeBuildRequest is correct.
         job = self.makeJob()
         test_publisher = SoyuzTestPublisher()
         test_publisher.addFakeChroots(job.build.distroseries)
-        slave = OkSlave()
-        builder = MockBuilder("bob-de-bouwer")
-        builder.processor = getUtility(IProcessorSet).getByName('386')
-        job.setBuilder(builder, slave)
-        logger = BufferLogger()
-        d = defer.maybeDeferred(job.dispatchBuildToSlave, "someid", logger)
-
-        def check_dispatch(ignored):
-            self.assertThat(
-                logger.getLogBuffer(),
-                StartsWith(dedent("""\
-                  DEBUG Initiating build 1-someid on http://fake:0000
-                  """)))
-            self.assertEquals(["ensurepresent", "build"],
-                              [call[0] for call in slave.call_log])
-            build_args = slave.call_log[1][1:]
-            self.assertEquals(build_args[0], job.getBuildCookie())
-            self.assertEquals(build_args[1], "sourcepackagerecipe")
-            self.assertEquals(build_args[3], [])
-            distroarchseries = job.build.distroseries.architectures[0]
-            self.assertEqual(
-                build_args[4], job._extraBuildArgs(distroarchseries))
-        return d.addCallback(check_dispatch)
-
-    @run_test_with(AsynchronousDeferredRunTest)
-    def test_dispatchBuildToSlave_nochroot(self):
-        # dispatchBuildToSlave will fail when there is not chroot tarball
-        # available for the distroseries to build for.
-        job = self.makeJob()
-        #test_publisher = SoyuzTestPublisher()
-        builder = MockBuilder("bob-de-bouwer")
-        builder.processor = getUtility(IProcessorSet).getByName('386')
-        job.setBuilder(builder, OkSlave())
-        logger = BufferLogger()
-        d = defer.maybeDeferred(job.dispatchBuildToSlave, "someid", logger)
-        return assert_fails_with(d, CannotBuild)
+        das = job.build.distroseries.nominatedarchindep
+        builder = MockBuilder("bob-de-bouwer")
+        builder.processor = das.processor
+        job.setBuilder(builder, None)
+        self.assertEqual(
+            ('sourcepackagerecipe', das, {}, job._extraBuildArgs(das)),
+            job.composeBuildRequest(None))
 
 
 class TestBuildNotifications(TrialTestCase):

=== modified file 'lib/lp/soyuz/model/binarypackagebuildbehaviour.py'
--- lib/lp/soyuz/model/binarypackagebuildbehaviour.py	2014-06-25 12:30:38 +0000
+++ lib/lp/soyuz/model/binarypackagebuildbehaviour.py	2014-06-25 12:30:38 +0000
@@ -33,12 +33,6 @@
 
     implements(IBuildFarmJobBehaviour)
 
-    def logStartBuild(self, logger):
-        """See `IBuildFarmJobBehaviour`."""
-        spr = self.build.source_package_release
-        logger.info("startBuild(%s, %s, %s, %s)", self._builder.url,
-                    spr.name, spr.version, self.build.pocket.title)
-
     def getLogFileName(self):
         """See `IBuildPackageJob`."""
         sourcename = self.build.source_package_release.name

=== modified file 'lib/lp/soyuz/model/livefsbuildbehaviour.py'
--- lib/lp/soyuz/model/livefsbuildbehaviour.py	2014-06-25 12:30:38 +0000
+++ lib/lp/soyuz/model/livefsbuildbehaviour.py	2014-06-25 12:30:38 +0000
@@ -37,17 +37,6 @@
     adapts(ILiveFSBuild)
     implements(IBuildFarmJobBehaviour)
 
-    @property
-    def displayname(self):
-        ret = self.build.title
-        if self._builder is not None:
-            ret += " (on %s)" % self._builder.url
-        return ret
-
-    def logStartBuild(self, logger):
-        """See `IBuildFarmJobBehaviour`."""
-        logger.info("startBuild(%s)", self.displayname)
-
     def getLogFileName(self):
         das = self.build.distro_arch_series
         archname = das.architecturetag

=== modified file 'lib/lp/soyuz/tests/test_livefsbuildbehaviour.py'
--- lib/lp/soyuz/tests/test_livefsbuildbehaviour.py	2014-06-25 12:30:38 +0000
+++ lib/lp/soyuz/tests/test_livefsbuildbehaviour.py	2014-06-25 12:30:38 +0000
@@ -6,17 +6,10 @@
 __metaclass__ = type
 
 from datetime import datetime
-from textwrap import dedent
 
 import fixtures
 import pytz
-from testtools import run_test_with
-from testtools.deferredruntest import (
-    assert_fails_with,
-    AsynchronousDeferredRunTest,
-    )
 import transaction
-from twisted.internet import defer
 from twisted.trial.unittest import TestCase as TrialTestCase
 from zope.component import getUtility
 from zope.security.proxy import (
@@ -89,22 +82,6 @@
         job = IBuildFarmJobBehaviour(build)
         self.assertProvides(job, IBuildFarmJobBehaviour)
 
-    def test_displayname(self):
-        # displayname contains a reasonable description of the job.
-        job = self.makeJob()
-        self.assertEqual(
-            "i386 build of test-livefs livefs in distro unstable",
-            job.displayname)
-
-    def test_logStartBuild(self):
-        # logStartBuild will properly report the image that's being built.
-        job = self.makeJob()
-        logger = BufferLogger()
-        job.logStartBuild(logger)
-        self.assertEqual(
-            "INFO startBuild(i386 build of test-livefs livefs in distro "
-            "unstable)\n", logger.getLogBuffer())
-
     def test_verifyBuildRequest_valid(self):
         # verifyBuildRequest doesn't raise any exceptions when called with a
         # valid builder set.
@@ -227,42 +204,14 @@
         self.assertEqual(["--option=value"], args["lb_args"])
         self.assertIsNot(Proxy, type(args["lb_args"]))
 
-    @run_test_with(AsynchronousDeferredRunTest)
-    @defer.inlineCallbacks
-    def test_dispatchBuildToSlave(self):
-        # dispatchBuildToSlave makes the proper calls to the slave.
+    def test_composeBuildRequest(self):
         job = self.makeJob()
-        lfa = self.factory.makeLibraryFileAlias()
-        transaction.commit()
+        lfa = self.factory.makeLibraryFileAlias(db_only=True)
         job.build.distro_arch_series.addOrUpdateChroot(lfa)
-        slave = OkSlave()
-        builder = MockBuilder("bob")
-        builder.processor = getUtility(IProcessorSet).getByName("386")
-        job.setBuilder(builder, slave)
-        logger = BufferLogger()
-        yield job.dispatchBuildToSlave("someid", logger)
-        self.assertStartsWith(
-            logger.getLogBuffer(),
-            dedent("""\
-                DEBUG Initiating build 1-someid on http://fake:0000
-                """))
         self.assertEqual(
-            ["ensurepresent", "build"], [call[0] for call in slave.call_log])
-        build_args = slave.call_log[1][1:]
-        self.assertEqual(job.getBuildCookie(), build_args[0])
-        self.assertEqual("livefs", build_args[1])
-        self.assertEqual([], build_args[3])
-        self.assertEqual(job._extraBuildArgs(), build_args[4])
-
-    @run_test_with(AsynchronousDeferredRunTest)
-    def test_dispatchBuildToSlave_no_chroot(self):
-        # dispatchBuildToSlave fails when the DAS has no chroot.
-        job = self.makeJob()
-        builder = MockBuilder()
-        builder.processor = getUtility(IProcessorSet).getByName("386")
-        job.setBuilder(builder, OkSlave())
-        d = job.dispatchBuildToSlave("someid", BufferLogger())
-        return assert_fails_with(d, CannotBuild)
+            ('livefs', job.build.distro_arch_series, {},
+             job._extraBuildArgs()),
+            job.composeBuildRequest(None))
 
 
 class MakeLiveFSBuildMixin:

=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehaviour.py'
--- lib/lp/translations/model/translationtemplatesbuildbehaviour.py	2014-06-25 12:30:38 +0000
+++ lib/lp/translations/model/translationtemplatesbuildbehaviour.py	2014-06-25 12:30:38 +0000
@@ -40,9 +40,6 @@
     """Dispatches `TranslationTemplateBuildJob`s to slaves."""
     implements(IBuildFarmJobBehaviour)
 
-    # Identify the type of job to the slave.
-    build_type = 'translation-templates'
-
     # Filename for the tarball of templates that the slave builds.
     templates_tarball_path = 'translation-templates.tar.gz'
 
@@ -65,13 +62,6 @@
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
         return ubuntu.currentseries.nominatedarchindep
 
-    def logStartBuild(self, logger):
-        """See `IBuildFarmJobBehaviour`."""
-        logger.info(
-            "Starting templates build %s for %s." % (
-            self.getBuildCookie(),
-            self.build.branch.bzr_identity))
-
     def _readTarball(self, buildqueue, filemap, logger):
         """Read tarball with generated translation templates from slave."""
         if filemap is None:

=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py	2014-06-25 12:30:38 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildbehaviour.py	2014-06-25 12:30:38 +0000
@@ -9,14 +9,12 @@
 
 import pytz
 from testtools.deferredruntest import AsynchronousDeferredRunTest
-from testtools.testcase import ExpectedException
 from twisted.internet import defer
 from zope.component import getUtility
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interactor import BuilderInteractor
-from lp.buildmaster.interfaces.builder import CannotBuild
 from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
     IBuildFarmJobBehaviour,
     )
@@ -25,7 +23,6 @@
     WaitingSlave,
     )
 from lp.services.config import config
-from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.librarian.utils import copy_and_close
 from lp.testing import TestCaseWithFactory
 from lp.testing.dbuser import switch_dbuser
@@ -100,36 +97,14 @@
         b2 = self.makeBehaviour()
         self.assertNotEqual(b1.getLogFileName(), b2.getLogFileName())
 
-    @defer.inlineCallbacks
-    def test_dispatchBuildToSlave_no_chroot_fails(self):
-        # dispatchBuildToSlave will fail if the chroot does not exist.
-        behaviour = self.makeBehaviour(use_fake_chroot=False)
-        switch_dbuser(config.builddmaster.dbuser)
-        with ExpectedException(CannotBuild):
-            yield behaviour.dispatchBuildToSlave(None, logging)
-
-    def test_dispatchBuildToSlave(self):
-        # dispatchBuildToSlave ultimately causes the slave's build
-        # method to be invoked.  The slave receives the URL of the
-        # branch it should build from.
+    def test_composeBuildRequest(self):
         behaviour = self.makeBehaviour()
         switch_dbuser(config.builddmaster.dbuser)
-        d = behaviour.dispatchBuildToSlave(FakeBuildQueue(behaviour), logging)
-
-        def got_dispatch(ignored):
-            # call_log lives on the mock WaitingSlave and tells us what
-            # calls to the slave that the behaviour class made.
-            call_log = behaviour._slave.call_log
-            build_params = call_log[-1]
-            self.assertEqual('build', build_params[0])
-            build_type = build_params[2]
-            self.assertEqual('translation-templates', build_type)
-            branch_url = build_params[-1]['branch_url']
-            # The slave receives the public http URL for the branch.
-            self.assertEqual(
-                branch_url,
-                behaviour.build.branch.composePublicURL())
-        return d.addCallback(got_dispatch)
+        self.assertEqual(
+            ('translation-templates', behaviour._getDistroArchSeries(), {},
+             {'arch_tag': behaviour._getDistroArchSeries().architecturetag,
+              'branch_url': behaviour.build.branch.composePublicURL()}),
+             behaviour.composeBuildRequest(None))
 
     def test_getDistroArchSeries(self):
         # _getDistroArchSeries produces the nominated arch-indep
@@ -167,13 +142,7 @@
         queue_item = FakeBuildQueue(behaviour)
         slave = behaviour._slave
 
-        d = behaviour.dispatchBuildToSlave(queue_item, logging)
-
-        def got_dispatch(ignored):
-            self.assertEqual(0, queue_item.destroySelf.call_count)
-            self.assertEqual(0, behaviour._uploadTarball.call_count)
-
-            return slave.status()
+        d = slave.status()
 
         def got_status(status):
             return (
@@ -189,7 +158,6 @@
             self.assertEqual(1, queue_item.destroySelf.call_count)
             self.assertEqual(1, behaviour._uploadTarball.call_count)
 
-        d.addCallback(got_dispatch)
         d.addCallback(got_status)
         d.addCallback(build_updated)
         return d
@@ -200,11 +168,8 @@
         behaviour._uploadTarball = FakeMethod()
         queue_item = FakeBuildQueue(behaviour)
         slave = behaviour._slave
-        d = behaviour.dispatchBuildToSlave(queue_item, logging)
 
-        def got_dispatch(ignored):
-            # Now that we've dispatched, get the status.
-            return slave.status()
+        d = slave.status()
 
         def got_status(status):
             del status['filemap']
@@ -220,7 +185,6 @@
             self.assertEqual(1, queue_item.destroySelf.call_count)
             self.assertEqual(0, behaviour._uploadTarball.call_count)
 
-        d.addCallback(got_dispatch)
         d.addCallback(got_status)
         d.addCallback(build_updated)
         return d
@@ -232,10 +196,8 @@
         behaviour._uploadTarball = FakeMethod()
         queue_item = FakeBuildQueue(behaviour)
         slave = behaviour._slave
-        d = behaviour.dispatchBuildToSlave(queue_item, logging)
 
-        def got_dispatch(ignored):
-            return slave.status()
+        d = slave.status()
 
         def got_status(status):
             del status['filemap']
@@ -249,7 +211,6 @@
             self.assertEqual(1, queue_item.destroySelf.call_count)
             self.assertEqual(0, behaviour._uploadTarball.call_count)
 
-        d.addCallback(got_dispatch)
         d.addCallback(got_status)
         d.addCallback(build_updated)
         return d
@@ -262,8 +223,6 @@
         queue_item = FakeBuildQueue(behaviour)
         slave = behaviour._slave
 
-        d = behaviour.dispatchBuildToSlave(queue_item, logging)
-
         def fake_getFile(sum, file):
             dummy_tar = os.path.join(
                 os.path.dirname(__file__), 'dummy_templates.tar.gz')
@@ -271,9 +230,8 @@
             copy_and_close(tar_file, file)
             return defer.succeed(None)
 
-        def got_dispatch(ignored):
-            slave.getFile = fake_getFile
-            return slave.status()
+        slave.getFile = fake_getFile
+        d = slave.status()
 
         def got_status(status):
             return behaviour.handleStatus(
@@ -294,7 +252,6 @@
             list2 = sorted([entry.path for entry in entries])
             self.assertEqual(list1, list2)
 
-        d.addCallback(got_dispatch)
         d.addCallback(got_status)
         d.addCallback(build_updated)
         return d


Follow ups