Merge lp:~julian-edwards/launchpad/builderslave-resume into lp:launchpad

[Julian Edwards]

This is the integration branch for the "fully asynchronous build manager" changes.

[Jonathan Lange (jml)]

Looks good. Thanks.

I think that as long as we have resumeSlave, we should keep the tests in 'lib/lp/buildmaster/tests/test_manager.py'. Please revert your changes to that file & land this branch.

[Jonathan Lange (jml)]

review: Approve

[Jonathan Lange (jml)]

Branch has been pushed to since review. Definitely not approved.

review: Abstain

[Julian Edwards (julian-edwards)]

This is now the integration branch. WIP.

[Jonathan Lange (jml)]

Some quick comments, almost all of which are shallow. This doesn't count as a proper code review. Importantly, I haven't rigorously checked that the deleted tests have been properly replaced, and I haven't checked a lot of the Deferred-transformation work for correctness.

Will respond soon with a review of the new manager & test_manager.

> === modified file 'lib/lp/buildmaster/interfaces/builder.py'
> --- lib/lp/buildmaster/interfaces/builder.py 2010-09-23 18:17:21 +0000
> +++ lib/lp/buildmaster/interfaces/builder.py 2010-10-08 17:29:52 +0000
> @@ -154,11 +154,6 @@ class IBuilder(IHasOwner):
>

Could you please go through this interface and:

  * Make sure that all methods that return Deferreds are documented as doing
    so.

  * Possibly, group all of the methods that return Deferreds. I think it will
    make the interface clearer.

> === modified file 'lib/lp/buildmaster/model/builder.py'
> --- lib/lp/buildmaster/model/builder.py 2010-09-24 13:39:27 +0000
> +++ lib/lp/buildmaster/model/builder.py 2010-10-18 10:09:54 +0000
...
> @@ -125,24 +112,17 @@ class BuilderSlave(object):
> # many false positives in your test run and will most likely break
> # production.
>

> # 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).
>

I've filed bug https://bugs.edge.launchpad.net/soyuz/+bug/662599 for this. I
don't think having the XXX here helps.

> # 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.
>

This XXX can be safely deleted, I think.

> def getFile(self, sha_sum):
> """Construct a file-like object to return the named file."""
> + # XXX: Change this to do non-blocking IO.

Please file a bug.

...
> + # Twisted API requires string but the configuration provides unicode.
> + resume_argv = [str(term) for term in resume_command.split()]

It's more explicit to do .encode('utf-8'), rather than str().

> def updateStatus(self, logger=None):
> """See `IBuilder`."""
> - updateBuilderStatus(self, logger)
> + # updateBuilderStatus returns a Deferred if the builder timed
> + # out, otherwise it returns a thing that we can wrap in a
> + # defer.succeed. maybeDeferred() handles this for us.
> + return defer.maybeDeferred(updateBuilderStatus, self, logger)
>

This comment seems bogus. As far as I can tell, updateBuilderStatus always
returns a Deferred.

> - if builder_should_be_failed:
> + d = self.resumeSlaveHost()
> + return d
> + else:
> + # XXX: This should really let the failure bubble up to the
> + # scan() method that does the failure counting.
> # Mark builder as 'failed'.

Are you going to fix this in this branch or in another? If so, when?

> logger.warn(
> - "Disabling builder: %s -- %s" % (self.url, error_message),
> - ...

[Jonathan Lange (jml)]

Hey Julian,

The new manager is far, far more readable than before. The hard work has paid off.

I mention a lot of small things in the comments below. I'd really appreciate it if you could address them all, since now is the best opportunity we'll have in a while to make this code comprehensible to others.

> # Copyright 2009 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
> """Soyuz buildd slave manager logic."""
>
> __metaclass__ = type
>
> __all__ = [
> 'BuilddManager',
> 'BUILDD_MANAGER_LOG_NAME',
> 'buildd_success_result_map',
> ]
>
> import logging
>
> import transaction
> from twisted.application import service
> from twisted.internet import (
> defer,
> reactor,
> )
> from twisted.internet.task import LoopingCall
> from twisted.python import log
> from zope.component import getUtility
>
> from lp.buildmaster.enums import BuildStatus
> from lp.buildmaster.interfaces.buildfarmjobbehavior import (
> BuildBehaviorMismatch,
> )
> from lp.buildmaster.interfaces.builder import (
> BuildDaemonError,
> BuildSlaveFailure,
> CannotBuild,
> CannotResumeHost,
> )
>
>
> BUILDD_MANAGER_LOG_NAME = "slave-scanner"
>
>
> buildd_success_result_map = {
> 'ensurepresent': True,
> 'build': 'BuilderStatus.BUILDING',
> }
>

You can delete this now. Yay. (Don't forget the __all__ too).

> class SlaveScanner:
> """A manager for a single builder."""
>
> SCAN_INTERVAL = 5
>

Can you please add a comment explaining what this means, what unit it's in,
and hinting at why 5 is a good number for it?

> def startCycle(self):
> """Scan the builder and dispatch to it or deal with failures."""
> self.loop = LoopingCall(self._startCycle)
> self.stopping_deferred = self.loop.start(self.SCAN_INTERVAL)
> return self.stopping_deferred
>
> def _startCycle(self):
> # Same as _startCycle but the next cycle is not scheduled. This
> # is so tests can initiate a single scan.

This comment is obsolete. Also, there's probably a better name than
"_startCycle", since this is pretty much doing the scan. Perhaps 'oneCycle'.

> def _scanFailed(self, failure):
> # Trap known exceptions and print a message without a
> # stack trace in that case, or if we don't know about it,
> # include the trace.
>

This comment is also obsolete. Although the traceback/no-traceback logic is
still here, it's hardly the point of the method.

> # Paranoia.
> transaction.abort()
>

Can you please explain in the comment exactly what you are being paranoid
about?

> error_message = failure.getErrorMessage()
> if failure.check(
> BuildSlaveFailure, CannotBuild, BuildBehaviorMismatch,
> CannotResumeHost, BuildDaemonError):
> self.logger.info("Scanning failed with: %s" % error_message)
> else:
> self.logger.info("Scanning failed with: %s\n%s" %
> (failure.getErrorMessage(), failure.getTraceback()))
>
> builder = get_builder(self.builder_name)
>

Shouldn...

review: Needs Fixing

[Julian Edwards (julian-edwards)]

On Monday 18 October 2010 11:59:57 Jonathan Lange wrote:
> Some quick comments, almost all of which are shallow. This doesn't count as
> a proper code review. Importantly, I haven't rigorously checked that the
> deleted tests have been properly replaced, and I haven't checked a lot of
> the Deferred-transformation work for correctness.
>
> Will respond soon with a review of the new manager & test_manager.

Cheers, I replied inline.

>
> > === modified file 'lib/lp/buildmaster/interfaces/builder.py'
> > --- lib/lp/buildmaster/interfaces/builder.py 2010-09-23 18:17:21 +0000
> > +++ lib/lp/buildmaster/interfaces/builder.py 2010-10-08 17:29:52 +0000
>
> > @@ -154,11 +154,6 @@ class IBuilder(IHasOwner):
> Could you please go through this interface and:
>
> * Make sure that all methods that return Deferreds are documented as
> doing so.
>
> * Possibly, group all of the methods that return Deferreds. I think it
> will make the interface clearer.

I've done both of these are done as you suggest.

> > === modified file 'lib/lp/buildmaster/model/builder.py'
> > --- lib/lp/buildmaster/model/builder.py 2010-09-24 13:39:27 +0000
> > +++ lib/lp/buildmaster/model/builder.py 2010-10-18 10:09:54 +0000
>
> ...
>
> > @@ -125,24 +112,17 @@ class BuilderSlave(object):
> > # many false positives in your test run and will most likely break
> > # production.
> >
> >
> > # 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).
>
> I've filed bug https://bugs.edge.launchpad.net/soyuz/+bug/662599 for this.
> I don't think having the XXX here helps.

Right, I've deleted it.

>
> > # 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.
>
> This XXX can be safely deleted, I think.

Done.

>
> > def getFile(self, sha_sum):
> > """Construct a file-like object to return the named file."""
> >
> > + # XXX: Change this to do non-blocking IO.
>
> Please file a bug.

https://bugs.edge.launchpad.net/soyuz/+bug/662631

>
> ...
>
> > + # Twisted API requires string but the configuration provides
> > unicode. + resume_argv = [str(term) for term in
> > resume_command.split()]
>
> It's more explicit to do .encode('utf-8'), rather than str().

Not my code, but I've done that.

>
> > def updateStatus(self, logger=None):
> > """See `IBuilder`."""
> >
> > - updateBuilderStatus(self, logger)
> > + # updateBuilderStatus returns a Deferred if the builder timed
> > + # out, otherwise it returns a thing that we can wrap in a
> > + # defer.succeed. maybeDeferred() handles this for us.
> > + return defer.maybeDeferred(updateBuilderStatus, self, logger)
>
> This comment seems bogus. As far as I can tell, updateBuilderStatus always
> returns a Deferred.

It does, so I've removed the comment and the maybeDeferred().

>
> > - if builder_shoul...

[Julian Edwards (julian-edwards)]

On Monday 18 October 2010 13:21:45 you wrote:
> Review: Needs Fixing
> Hey Julian,
>
> The new manager is far, far more readable than before. The hard work has
> paid off.

\o/

> I mention a lot of small things in the comments below. I'd really
> appreciate it if you could address them all, since now is the best
> opportunity we'll have in a while to make this code comprehensible to
> others.

I shall endeavour to do so.

> > # Copyright 2009 Canonical Ltd. This software is licensed under the
> > # GNU Affero General Public License version 3 (see the file LICENSE).
> >
> > """Soyuz buildd slave manager logic."""
> >
> > __metaclass__ = type
> >
> > __all__ = [
> >
> > 'BuilddManager',
> > 'BUILDD_MANAGER_LOG_NAME',
> > 'buildd_success_result_map',
> > ]
> >
> > import logging
> >
> > import transaction
> > from twisted.application import service
> > from twisted.internet import (
> >
> > defer,
> > reactor,
> > )
> >
> > from twisted.internet.task import LoopingCall
> > from twisted.python import log
> > from zope.component import getUtility
> >
> > from lp.buildmaster.enums import BuildStatus
> > from lp.buildmaster.interfaces.buildfarmjobbehavior import (
> >
> > BuildBehaviorMismatch,
> > )
> >
> > from lp.buildmaster.interfaces.builder import (
> >
> > BuildDaemonError,
> > BuildSlaveFailure,
> > CannotBuild,
> > CannotResumeHost,
> > )
> >
> > BUILDD_MANAGER_LOG_NAME = "slave-scanner"
> >
> >
> > buildd_success_result_map = {
> >
> > 'ensurepresent': True,
> > 'build': 'BuilderStatus.BUILDING',
> > }
>
> You can delete this now. Yay. (Don't forget the __all__ too).

Right!

>
> > class SlaveScanner:
> > """A manager for a single builder."""
> >
> > SCAN_INTERVAL = 5
>
> Can you please add a comment explaining what this means, what unit it's in,
> and hinting at why 5 is a good number for it?

Done.

>
> > def startCycle(self):
> > """Scan the builder and dispatch to it or deal with failures."""
> > self.loop = LoopingCall(self._startCycle)
> > self.stopping_deferred = self.loop.start(self.SCAN_INTERVAL)
> > return self.stopping_deferred
> >
> > def _startCycle(self):
> > # Same as _startCycle but the next cycle is not scheduled. This
> > # is so tests can initiate a single scan.
>
> This comment is obsolete. Also, there's probably a better name than
> "_startCycle", since this is pretty much doing the scan. Perhaps
> 'oneCycle'.

I named it singleCycle()

>
> > def _scanFailed(self, failure):
> > # Trap known exceptions and print a message without a
> > # stack trace in that case, or if we don't know about it,
> > # include the trace.
>
> This comment is also obsolete. Although the traceback/no-traceback logic
> is still here, it's hardly the point of the method.

Spruced up as discussed in IRC.

>
> > # Paranoia.
> > transaction.abort()
>
> Can you please explain in the comment exactly what you are being paranoid
> about?

Yup, done.

>
> > error_message = failure.getErrorMessage()
> > if failure.check(
...

[Jonathan Lange (jml)]

On Mon, Oct 18, 2010 at 4:49 PM, Julian Edwards
<email address hidden> wrote:
> On Monday 18 October 2010 13:21:45 you wrote:
>> Review: Needs Fixing
>> Hey Julian,
>>
>> The new manager is far, far more readable than before.  The hard work has
>> paid off.
>
> \o/
>
>> I mention a lot of small things in the comments below.  I'd really
>> appreciate it if you could address them all, since now is the best
>> opportunity we'll have in a while to make this code comprehensible to
>> others.
>
> I shall endeavour to do so.
>
...
>>
>> > class SlaveScanner:
>> >     """A manager for a single builder."""
>> >
>> >     SCAN_INTERVAL = 5
>>
>> Can you please add a comment explaining what this means, what unit it's in,
>> and hinting at why 5 is a good number for it?
>
> Done.
>

...
>>
>> >         """
>> >         # We need to re-fetch the builder object on each cycle as the
>> >         # Storm store is invalidated over transaction boundaries.
>>
>> This method is complicated enough that I think it would benefit from a
>> prose summary of what it does.
>>
>> For example:
>>
>>   If the builder is OK [XXX - I don't know what this actually means - jml],
>>   then update its status [XXX - this seems redundant, didn't we just check
>>   that it was OK? - jml].  If we think there's an active build on the
>>   builder, then check to see if it's done (builder.updateBuild).  If it
>>   is done, then check that it's available and not in manual mode, then
>>   dispatch the job to the build.
>>
>> Well, that's my best guess.  I think it's only worth describing the happy
>> case, since the unusual cases will be easy enough to follow in the code.
>
> I've written something - let me know if you think it's easy enough to follow.
> It does require some knowledge of how the farm works and I don't think code
> comments are the best place for explaining that.
>

It's good. Much more helpful, thanks.

>> > class BuilddManager(service.Service):
>> >     """Main Buildd Manager service class."""
>> >
>> >     def __init__(self, clock=None):
>> >         self.builder_slaves = []
>> >         self.logger = self._setupLogger()
>>
>> Given that _setupLogger changes global state, it's better to put it in
>> startService.
>
> Nocando (which is a bit near Katmandu) - the NewBuildersScanner needs it
> there.  (More below on this section of code)
>

>>
>> >         self.new_builders_scanner = NewBuildersScanner(
>> >
>> >             manager=self, clock=clock)
>> >
>> >     def _setupLogger(self):
>> >         """Setup a 'slave-scanner' logger that redirects to twisted.
>>
>> FWIW, "Setup" is a noun. "Set up" is a verb.
>
> Setup is not a word, it's a typo.
>

>> > class TestSlaveScannerScan(TrialTestCase):
>> >     """Tests `SlaveScanner.scan` method.
>> >
>> >     This method uses the old framework for scanning and dispatching
>> >     builds. """
>> >     layer = LaunchpadZopelessLayer
>> >
>> >     def setUp(self):
>> >         """Setup TwistedLayer, TrialTestCase and BuilddSlaveTest.
>> >
>> >         Also adjust the sampledata in a way a build can be dispatched to
>> >         'bob' builder.
>> >         """
>> >         from lp.soyuz.tests.test_publishing import...

[Jonathan Lange (jml)]

Add the XXXs as recommended in the last review comment and please land!

review: Approve

[Jonathan Lange (jml)]

Oh, forgot about the two WIP things mentioned in your reply.

review: Needs Fixing

[Jonathan Lange (jml)]

The new code looks good. You should add an explanation about why you don't disable builders as soon as they've got a failure, i.e. why the threshold exists at all.

Also, in my previous review just prior to commit r11699, I suggested adding several XXXs. Could you please do that.

Do both of these things, and then land.

review: Approve

[Julian Edwards (julian-edwards)]

On Tuesday 19 October 2010 16:39:43 you wrote:
> Review: Approve
> The new code looks good. You should add an explanation about why you don't
> disable builders as soon as they've got a failure, i.e. why the threshold
> exists at all.

Roger.

> Also, in my previous review just prior to commit r11699, I suggested adding
> several XXXs. Could you please do that.

Grar, I forgot to finish that, thanks for reminding me.

> Do both of these things, and then land.

I'm not going to land it right away. I want to soak test it on dogfood first,
borrowing some builders from production. Once I'm happy with it on there,
it's time to let loose the hounds.

Thanks for all your help on this branch! You'll notice the large list of bugs
I linked to it just now. We've fixed all of those in this branch, and
probably more.

Cheers.

=== modified file 'lib/lp/buildmaster/doc/builder.txt'
--- lib/lp/buildmaster/doc/builder.txt	2010-09-23 12:35:21 +0000
+++ lib/lp/buildmaster/doc/builder.txt	2010-10-25 19:14:01 +0000
@@ -19,9 +19,6 @@
 As expected, it implements IBuilder.
 
     >>> from canonical.launchpad.webapp.testing import verifyObject
-    >>> from lp.buildmaster.interfaces.builder import IBuilder
-    >>> verifyObject(IBuilder, builder)
-    True
 
     >>> print builder.name
     bob
@@ -86,7 +83,7 @@
 The 'new' method will create a new builder in the database.
 
     >>> bnew = builderset.new(1, 'http://dummy.com:8221/', 'dummy',
-    ...	                   'Dummy Title', 'eh ?', 1)
+    ...                    'Dummy Title', 'eh ?', 1)
     >>> bnew.name
     u'dummy'
 
@@ -170,7 +167,7 @@
     >>> recipe_bq.processor = i386_family.processors[0]
     >>> recipe_bq.virtualized = True
     >>> transaction.commit()
-    
+
     >>> queue_sizes = builderset.getBuildQueueSizes()
     >>> print queue_sizes['virt']['386']
     (1L, datetime.timedelta(0, 64))
@@ -188,116 +185,3 @@
 
     >>> print queue_sizes['virt']['386']
     (2L, datetime.timedelta(0, 128))
-
-
-Resuming buildd slaves
-======================
-
-Virtual slaves are resumed using a command specified in the
-configuration profile. Production configuration uses a SSH trigger
-account accessed via a private key available in the builddmaster
-machine (which used ftpmaster configuration profile) as in:
-
-{{{
-ssh ~/.ssh/ppa-reset-key ppa@%(vm_host)s
-}}}
-
-The test configuration uses a fake command that can be performed in
-development machine and allow us to tests the important features used
-in production, as 'vm_host' variable replacement.
-
-    >>> from canonical.config import config
-    >>> config.builddmaster.vm_resume_command
-    'echo %(vm_host)s'
-
-Before performing the command, it checks if the builder is indeed
-virtual and raises CannotResumeHost if it isn't.
-
-    >>> bob = getUtility(IBuilderSet)['bob']
-    >>> bob.resumeSlaveHost()
-    Traceback (most recent call last):
-    ...
-    CannotResumeHost: Builder is not virtualized.
-
-For testing purposes resumeSlaveHost returns the stdout and stderr
-buffer resulted from the command.
-
-    >>> frog = getUtility(IBuilderSet)['frog']
-    >>> out, err = frog.resumeSlaveHost()
-    >>> print out.strip()
-    localhost-host.ppa
-
-If the specified command fails, resumeSlaveHost also raises
-CannotResumeHost exception with the results stdout and stderr.
-
-    # The command must have a vm_host dict key and when executed,
-    # have a returncode that is not 0.
-    >>> vm_resume_command = """
-    ...     [builddmaster]
-    ...     vm_resume_command: test "%(vm_host)s = 'false'"
-    ...     """
-    >>> config.push('vm_resume_command', vm_resume_command)
-    >>> frog.resumeSlaveHost()
-    Traceback (most recent call last):
-    ...
-    CannotResumeHost: Resuming failed:
-    OUT:
-    <BLANKLINE>
-    ERR:
-    <BLANKLINE>
-
-Restore default value for resume command.
-
-    >>> config_data = config.pop('vm_resume_command')
-
-
-Rescuing lost slaves
-====================
-
-Builder.rescueIfLost() checks the build ID reported in the slave status
-against the database. If it isn't building what we think it should be,
-the current build will be aborted and the slave cleaned in preparation
-for a new task. The decision about the slave's correctness is left up
-to IBuildFarmJobBehavior.verifySlaveBuildCookie -- for these examples we
-will use a special behavior that just checks if the cookie reads 'good'.
-
-    >>> import logging
-    >>> from lp.buildmaster.interfaces.builder import CorruptBuildCookie
-    >>> from lp.buildmaster.tests.mock_slaves import (
-    ...    BuildingSlave, MockBuilder, OkSlave, WaitingSlave)
-
-    >>> class TestBuildBehavior:
-    ...     def verifySlaveBuildCookie(self, cookie):
-    ...         if cookie != 'good':
-    ...             raise CorruptBuildCookie('Bad value')
-
-    >>> def rescue_slave_if_lost(slave):
-    ...     builder = MockBuilder('mock', slave, TestBuildBehavior())
-    ...     builder.rescueIfLost(logging.getLogger())
-
-An idle slave is not rescued.
-
-    >>> rescue_slave_if_lost(OkSlave())
-
-Slaves building or having built the correct build are not rescued
-either.
-
-    >>> rescue_slave_if_lost(BuildingSlave(build_id='good'))
-    >>> rescue_slave_if_lost(WaitingSlave(build_id='good'))
-
-But if a slave is building the wrong ID, it is declared lost and
-an abort is attempted. MockSlave prints out a message when it is aborted
-or cleaned.
-
-    >>> rescue_slave_if_lost(BuildingSlave(build_id='bad'))
-    Aborting slave
-    INFO:root:Builder 'mock' rescued from 'bad': 'Bad value'
-
-Slaves having completed an incorrect build are also declared lost,
-but there's no need to abort a completed build. Such builders are
-instead simply cleaned, ready for the next build.
-
-    >>> rescue_slave_if_lost(WaitingSlave(build_id='bad'))
-    Cleaning slave
-    INFO:root:Builder 'mock' rescued from 'bad': 'Bad value'
-
 
=== modified file 'lib/lp/buildmaster/interfaces/builder.py'
--- lib/lp/buildmaster/interfaces/builder.py	2010-09-23 18:17:21 +0000
+++ lib/lp/buildmaster/interfaces/builder.py	2010-10-25 19:14:01 +0000
@@ -154,11 +154,6 @@
 
     currentjob = Attribute("BuildQueue instance for job being processed.")
 
-    is_available = Bool(
-        title=_("Whether or not a builder is available for building "
-                "new jobs. "),
-        required=False)
-
     failure_count = Int(
         title=_('Failure Count'), required=False, default=0,
         description=_("Number of consecutive failures for this builder."))
@@ -173,32 +168,74 @@
     def resetFailureCount():
         """Set the failure_count back to zero."""
 
-    def checkSlaveAlive():
-        """Check that the buildd slave is alive.
-
-        This pings the slave over the network via the echo method and looks
-        for the sent message as the reply.
-
-        :raises BuildDaemonError: When the slave is down.
+    def failBuilder(reason):
+        """Mark builder as failed for a given reason."""
+
+    def setSlaveForTesting(proxy):
+        """Sets the RPC proxy through which to operate the build slave."""
+
+    def verifySlaveBuildCookie(slave_build_id):
+        """Verify that a slave's build cookie is consistent.
+
+        This should delegate to the current `IBuildFarmJobBehavior`.
+        """
+
+    def transferSlaveFileToLibrarian(file_sha1, filename, private):
+        """Transfer a file from the slave to the librarian.
+
+        :param file_sha1: The file's sha1, which is how the file is addressed
+            in the slave XMLRPC protocol. Specially, the file_sha1 'buildlog'
+            will cause the build log to be retrieved and gzipped.
+        :param filename: The name of the file to be given to the librarian file
+            alias.
+        :param private: True if the build is for a private archive.
+        :return: A librarian file alias.
+        """
+
+    def getBuildQueue():
+        """Return a `BuildQueue` if there's an active job on this builder.
+
+        :return: A BuildQueue, or None.
+        """
+
+    def getCurrentBuildFarmJob():
+        """Return a `BuildFarmJob` for this builder."""
+
+    # All methods below here return Deferred.
+
+    def isAvailable():
+        """Whether or not a builder is available for building new jobs.
+
+        :return: A Deferred that fires with True or False, depending on
+            whether the builder is available or not.
         """
 
     def rescueIfLost(logger=None):
         """Reset the slave if its job information doesn't match the DB.
 
-        If the builder is BUILDING or WAITING but has a build ID string
-        that doesn't match what is stored in the DB, we have to dismiss
-        its current actions and clean the slave for another job, assuming
-        the XMLRPC is working properly at this point.
+        This checks the build ID reported in the slave status against the
+        database. If it isn't building what we think it should be, the current
+        build will be aborted and the slave cleaned in preparation for a new
+        task. The decision about the slave's correctness is left up to
+        `IBuildFarmJobBehavior.verifySlaveBuildCookie`.
+
+        :return: A Deferred that fires when the dialog with the slave is
+            finished.  It does not have a return value.
         """
 
     def updateStatus(logger=None):
-        """Update the builder's status by probing it."""
+        """Update the builder's status by probing it.
+        
+        :return: A Deferred that fires when the dialog with the slave is
+            finished.  It does not have a return value.
+        """
 
     def cleanSlave():
-        """Clean any temporary files from the slave."""
-
-    def failBuilder(reason):
-        """Mark builder as failed for a given reason."""
+        """Clean any temporary files from the slave.
+        
+        :return: A Deferred that fires when the dialog with the slave is
+            finished.  It does not have a return value.
+        """
 
     def requestAbort():
         """Ask that a build be aborted.
@@ -206,6 +243,9 @@
         This takes place asynchronously: Actually killing everything running
         can take some time so the slave status should be queried again to
         detect when the abort has taken effect. (Look for status ABORTED).
+
+        :return: A Deferred that fires when the dialog with the slave is
+            finished.  It does not have a return value.
         """
 
     def resumeSlaveHost():
@@ -217,37 +257,35 @@
         :raises: CannotResumeHost: if builder is not virtual or if the
             configuration command has failed.
 
-        :return: command stdout and stderr buffers as a tuple.
+        :return: A Deferred that fires when the resume operation finishes,
+            whose value is a (stdout, stderr) tuple for success, or a Failure
+            whose value is a CannotResumeHost exception.
         """
 
-    def setSlaveForTesting(proxy):
-        """Sets the RPC proxy through which to operate the build slave."""
-
     def slaveStatus():
         """Get the slave status for this builder.
 
-        :return: a dict containing at least builder_status, but potentially
-            other values included by the current build behavior.
+        :return: A Deferred which fires when the slave dialog is complete.
+            Its value is a dict containing at least builder_status, but
+            potentially other values included by the current build
+            behavior.
         """
 
     def slaveStatusSentence():
         """Get the slave status sentence for this builder.
 
-        :return: A tuple with the first element containing the slave status,
-            build_id-queue-id and then optionally more elements depending on
-            the status.
-        """
-
-    def verifySlaveBuildCookie(slave_build_id):
-        """Verify that a slave's build cookie is consistent.
-
-        This should delegate to the current `IBuildFarmJobBehavior`.
+        :return: A Deferred which fires when the slave dialog is complete.
+            Its value is a  tuple with the first element containing the
+            slave status, build_id-queue-id and then optionally more
+            elements depending on the status.
         """
 
     def updateBuild(queueItem):
         """Verify the current build job status.
 
         Perform the required actions for each state.
+
+        :return: A Deferred that fires when the slave dialog is finished.
         """
 
     def startBuild(build_queue_item, logger):
@@ -255,21 +293,10 @@
 
         :param build_queue_item: A BuildQueueItem to build.
         :param logger: A logger to be used to log diagnostic information.
-        :raises BuildSlaveFailure: When the build slave fails.
-        :raises CannotBuild: When a build cannot be started for some reason
-            other than the build slave failing.
-        """
-
-    def transferSlaveFileToLibrarian(file_sha1, filename, private):
-        """Transfer a file from the slave to the librarian.
-
-        :param file_sha1: The file's sha1, which is how the file is addressed
-            in the slave XMLRPC protocol. Specially, the file_sha1 'buildlog'
-            will cause the build log to be retrieved and gzipped.
-        :param filename: The name of the file to be given to the librarian file
-            alias.
-        :param private: True if the build is for a private archive.
-        :return: A librarian file alias.
+
+        :return: A Deferred that fires after the dispatch has completed whose
+            value is None, or a Failure that contains an exception
+            explaining what went wrong.
         """
 
     def handleTimeout(logger, error_message):
@@ -284,6 +311,8 @@
 
         :param logger: The logger object to be used for logging.
         :param error_message: The error message to be used for logging.
+        :return: A Deferred that fires after the virtual slave was resumed
+            or immediately if it's a non-virtual slave.
         """
 
     def findAndStartJob(buildd_slave=None):
@@ -291,17 +320,9 @@
 
         :param buildd_slave: An optional buildd slave that this builder should
             talk to.
-        :return: the `IBuildQueue` instance found or None if no job was found.
-        """
-
-    def getBuildQueue():
-        """Return a `BuildQueue` if there's an active job on this builder.
-
-        :return: A BuildQueue, or None.
-        """
-
-    def getCurrentBuildFarmJob():
-        """Return a `BuildFarmJob` for this builder."""
+        :return: A Deferred whose value is the `IBuildQueue` instance
+            found or None if no job was found.
+        """
 
 
 class IBuilderSet(Interface):
 
=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2010-09-24 15:40:49 +0000
+++ lib/lp/buildmaster/manager.py	2010-10-25 19:14:01 +0000
@@ -10,13 +10,10 @@
     'BuilddManager',
     'BUILDD_MANAGER_LOG_NAME',
     'FailDispatchResult',
-    'RecordingSlave',
     'ResetDispatchResult',
-    'buildd_success_result_map',
     ]
 
 import logging
-import os
 
 import transaction
 from twisted.application import service
@@ -24,129 +21,27 @@
     defer,
     reactor,
     )
-from twisted.protocols.policies import TimeoutMixin
+from twisted.internet.task import LoopingCall
 from twisted.python import log
-from twisted.python.failure import Failure
-from twisted.web import xmlrpc
 from zope.component import getUtility
 
-from canonical.config import config
-from canonical.launchpad.webapp import urlappend
-from lp.services.database import write_transaction
 from lp.buildmaster.enums import BuildStatus
-from lp.services.twistedsupport.processmonitor import ProcessWithTimeout
+from lp.buildmaster.interfaces.buildfarmjobbehavior import (
+    BuildBehaviorMismatch,
+    )
+from lp.buildmaster.model.builder import Builder
+from lp.buildmaster.interfaces.builder import (
+    BuildDaemonError,
+    BuildSlaveFailure,
+    CannotBuild,
+    CannotFetchFile,
+    CannotResumeHost,
+    )
 
 
 BUILDD_MANAGER_LOG_NAME = "slave-scanner"
 
 
-buildd_success_result_map = {
-    'ensurepresent': True,
-    'build': 'BuilderStatus.BUILDING',
-    }
-
-
-class QueryWithTimeoutProtocol(xmlrpc.QueryProtocol, TimeoutMixin):
-    """XMLRPC query protocol with a configurable timeout.
-
-    XMLRPC queries using this protocol will be unconditionally closed
-    when the timeout is elapsed. The timeout is fetched from the context
-    Launchpad configuration file (`config.builddmaster.socket_timeout`).
-    """
-    def connectionMade(self):
-        xmlrpc.QueryProtocol.connectionMade(self)
-        self.setTimeout(config.builddmaster.socket_timeout)
-
-
-class QueryFactoryWithTimeout(xmlrpc._QueryFactory):
-    """XMLRPC client factory with timeout support."""
-    # Make this factory quiet.
-    noisy = False
-    # Use the protocol with timeout support.
-    protocol = QueryWithTimeoutProtocol
-
-
-class RecordingSlave:
-    """An RPC proxy for buildd slaves that records instructions to the latter.
-
-    The idea here is to merely record the instructions that the slave-scanner
-    issues to the buildd slaves and "replay" them a bit later in asynchronous
-    and parallel fashion.
-
-    By dealing with a number of buildd slaves in parallel we remove *the*
-    major slave-scanner throughput issue while avoiding large-scale changes to
-    its code base.
-    """
-
-    def __init__(self, name, url, vm_host):
-        self.name = name
-        self.url = url
-        self.vm_host = vm_host
-
-        self.resume_requested = False
-        self.calls = []
-
-    def __repr__(self):
-        return '<%s:%s>' % (self.name, self.url)
-
-    def cacheFile(self, logger, libraryfilealias):
-        """Cache the file on the server."""
-        self.ensurepresent(
-            libraryfilealias.content.sha1, libraryfilealias.http_url, '', '')
-
-    def sendFileToSlave(self, *args):
-        """Helper to send a file to this builder."""
-        return self.ensurepresent(*args)
-
-    def ensurepresent(self, *args):
-        """Download files needed for the build."""
-        self.calls.append(('ensurepresent', args))
-        result = buildd_success_result_map.get('ensurepresent')
-        return [result, 'Download']
-
-    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]]
-
-    def resume(self):
-        """Record the request to resume the builder..
-
-        Always succeed.
-
-        :return: a (stdout, stderr, subprocess exitcode) triple
-        """
-        self.resume_requested = True
-        return ['', '', 0]
-
-    def resumeSlave(self, clock=None):
-        """Resume the builder in a asynchronous fashion.
-
-        Used the configuration command-line in the same way
-        `BuilddSlave.resume` does.
-
-        Also use the builddmaster configuration 'socket_timeout' as
-        the process timeout.
-
-        :param clock: An optional twisted.internet.task.Clock to override
-                      the default clock.  For use in tests.
-
-        :return: a Deferred
-        """
-        resume_command = config.builddmaster.vm_resume_command % {
-            'vm_host': self.vm_host}
-        # Twisted API require string and the configuration provides unicode.
-        resume_argv = [str(term) for term in resume_command.split()]
-
-        d = defer.Deferred()
-        p = ProcessWithTimeout(
-            d, config.builddmaster.socket_timeout, clock=clock)
-        p.spawnProcess(resume_argv[0], tuple(resume_argv))
-        return d
-
-
 def get_builder(name):
     """Helper to return the builder given the slave for this request."""
     # Avoiding circular imports.
@@ -159,9 +54,12 @@
     # builder.currentjob hides a complicated query, don't run it twice.
     # See bug 623281.
     current_job = builder.currentjob
-    build_job = current_job.specific_job.build
+    if current_job is None:
+        job_failure_count = 0
+    else:
+        job_failure_count = current_job.specific_job.build.failure_count
 
-    if builder.failure_count == build_job.failure_count:
+    if builder.failure_count == job_failure_count and current_job is not None:
         # If the failure count for the builder is the same as the
         # failure count for the job being built, then we cannot
         # tell whether the job or the builder is at fault. The  best
@@ -170,17 +68,28 @@
         current_job.reset()
         return
 
-    if builder.failure_count > build_job.failure_count:
+    if builder.failure_count > job_failure_count:
         # The builder has failed more than the jobs it's been
-        # running, so let's disable it and re-schedule the build.
-        builder.failBuilder(fail_notes)
-        current_job.reset()
+        # running.
+
+        # Re-schedule the build if there is one.
+        if current_job is not None:
+            current_job.reset()
+
+        # We are a little more tolerant with failing builders than
+        # failing jobs because sometimes they get unresponsive due to
+        # human error, flaky networks etc.  We expect the builder to get
+        # better, whereas jobs are very unlikely to get better.
+        if builder.failure_count >= Builder.FAILURE_THRESHOLD:
+            # It's also gone over the threshold so let's disable it.
+            builder.failBuilder(fail_notes)
     else:
         # The job is the culprit!  Override its status to 'failed'
         # to make sure it won't get automatically dispatched again,
         # and remove the buildqueue request.  The failure should
         # have already caused any relevant slave data to be stored
         # on the build record so don't worry about that here.
+        build_job = current_job.specific_job.build
         build_job.status = BuildStatus.FAILEDTOBUILD
         builder.currentjob.destroySelf()
 
@@ -190,133 +99,108 @@
         # next buildd scan.
 
 
-class BaseDispatchResult:
-    """Base class for *DispatchResult variations.
-
-    It will be extended to represent dispatching results and allow
-    homogeneous processing.
-    """
-
-    def __init__(self, slave, info=None):
-        self.slave = slave
-        self.info = info
-
-    def _cleanJob(self, job):
-        """Clean up in case of builder reset or dispatch failure."""
-        if job is not None:
-            job.reset()
-
-    def assessFailureCounts(self):
-        """View builder/job failure_count and work out which needs to die.
-
-        :return: True if we disabled something, False if we did not.
-        """
-        builder = get_builder(self.slave.name)
-        assessFailureCounts(builder, self.info)
-
-    def ___call__(self):
-        raise NotImplementedError(
-            "Call sites must define an evaluation method.")
-
-
-class FailDispatchResult(BaseDispatchResult):
-    """Represents a communication failure while dispatching a build job..
-
-    When evaluated this object mark the corresponding `IBuilder` as
-    'NOK' with the given text as 'failnotes'. It also cleans up the running
-    job (`IBuildQueue`).
-    """
-
-    def __repr__(self):
-        return  '%r failure (%s)' % (self.slave, self.info)
-
-    @write_transaction
-    def __call__(self):
-        self.assessFailureCounts()
-
-
-class ResetDispatchResult(BaseDispatchResult):
-    """Represents a failure to reset a builder.
-
-    When evaluated this object simply cleans up the running job
-    (`IBuildQueue`) and marks the builder down.
-    """
-
-    def __repr__(self):
-        return  '%r reset failure' % self.slave
-
-    @write_transaction
-    def __call__(self):
-        builder = get_builder(self.slave.name)
-        # Builders that fail to reset should be disabled as per bug
-        # 563353.
-        # XXX Julian bug=586362
-        # This is disabled until this code is not also used for dispatch
-        # failures where we *don't* want to disable the builder.
-        # builder.failBuilder(self.info)
-        self._cleanJob(builder.currentjob)
-
-
 class SlaveScanner:
     """A manager for a single builder."""
 
+    # The interval between each poll cycle, in seconds.  We'd ideally
+    # like this to be lower but 5 seems a reasonable compromise between
+    # responsivity and load on the database server, since in each cycle
+    # we can run quite a few queries.
     SCAN_INTERVAL = 5
 
-    # These are for the benefit of tests; see `TestingSlaveScanner`.
-    # It pokes fake versions in here so that it can verify methods were
-    # called.  The tests should really be using FakeMethod() though.
-    reset_result = ResetDispatchResult
-    fail_result = FailDispatchResult
-
     def __init__(self, builder_name, logger):
         self.builder_name = builder_name
         self.logger = logger
-        self._deferred_list = []
-
-    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):
         """Scan the builder and dispatch to it or deal with failures."""
+        self.loop = LoopingCall(self.singleCycle)
+        self.stopping_deferred = self.loop.start(self.SCAN_INTERVAL)
+        return self.stopping_deferred
+
+    def stopCycle(self):
+        """Terminate the LoopingCall."""
+        self.loop.stop()
+
+    def singleCycle(self):
         self.logger.debug("Scanning builder: %s" % self.builder_name)
-
+        d = self.scan()
+
+        d.addErrback(self._scanFailed)
+        return d
+
+    def _scanFailed(self, failure):
+        """Deal with failures encountered during the scan cycle.
+
+        1. Print the error in the log
+        2. Increment and assess failure counts on the builder and job.
+        """
+        # Make sure that pending database updates are removed as it
+        # could leave the database in an inconsistent state (e.g. The
+        # job says it's running but the buildqueue has no builder set).
+        transaction.abort()
+
+        # If we don't recognise the exception include a stack trace with
+        # the error.
+        error_message = failure.getErrorMessage()
+        if failure.check(
+            BuildSlaveFailure, CannotBuild, BuildBehaviorMismatch,
+            CannotResumeHost, BuildDaemonError, CannotFetchFile):
+            self.logger.info("Scanning failed with: %s" % error_message)
+        else:
+            self.logger.info("Scanning failed with: %s\n%s" %
+                (failure.getErrorMessage(), failure.getTraceback()))
+
+        # Decide if we need to terminate the job or fail the
+        # builder.
         try:
-            slave = self.scan()
-            if slave is None:
-                self.scheduleNextScanCycle()
+            builder = get_builder(self.builder_name)
+            builder.gotFailure()
+            if builder.currentjob is not None:
+                build_farm_job = builder.getCurrentBuildFarmJob()
+                build_farm_job.gotFailure()
+                self.logger.info(
+                    "builder %s failure count: %s, "
+                    "job '%s' failure count: %s" % (
+                        self.builder_name,
+                        builder.failure_count,
+                        build_farm_job.title, 
+                        build_farm_job.failure_count))
             else:
-                # XXX: Ought to return Deferred.
-                self.resumeAndDispatch(slave)
+                self.logger.info(
+                    "Builder %s failed a probe, count: %s" % (
+                        self.builder_name, builder.failure_count))
+            assessFailureCounts(builder, failure.getErrorMessage())
+            transaction.commit()
         except:
-            error = Failure()
-            self.logger.info("Scanning failed with: %s\n%s" %
-                (error.getErrorMessage(), error.getTraceback()))
-
-            builder = get_builder(self.builder_name)
-
-            # Decide if we need to terminate the job or fail the
-            # builder.
-            self._incrementFailureCounts(builder)
-            self.logger.info(
-                "builder failure count: %s, job failure count: %s" % (
-                    builder.failure_count,
-                    builder.getCurrentBuildFarmJob().failure_count))
-            assessFailureCounts(builder, error.getErrorMessage())
-            transaction.commit()
-
-            self.scheduleNextScanCycle()
-
-    @write_transaction
+            # Catastrophic code failure! Not much we can do.
+            self.logger.error(
+                "Miserable failure when trying to examine failure counts:\n",
+                exc_info=True)
+            transaction.abort()
+
     def scan(self):
         """Probe the builder and update/dispatch/collect as appropriate.
 
-        The whole method is wrapped in a transaction, but we do partial
-        commits to avoid holding locks on tables.
-
-        :return: A `RecordingSlave` if we dispatched a job to it, or None.
+        There are several steps to scanning:
+
+        1. If the builder is marked as "ok" then probe it to see what state
+            it's in.  This is where lost jobs are rescued if we think the
+            builder is doing something that it later tells us it's not,
+            and also where the multi-phase abort procedure happens.
+            See IBuilder.rescueIfLost, which is called by
+            IBuilder.updateStatus().
+        2. If the builder is still happy, we ask it if it has an active build
+            and then either update the build in Launchpad or collect the
+            completed build. (builder.updateBuild)
+        3. If the builder is not happy or it was marked as unavailable
+            mid-build, we need to reset the job that we thought it had, so
+            that the job is dispatched elsewhere.
+        4. If the builder is idle and we have another build ready, dispatch
+            it.
+
+        :return: A Deferred that fires when the scan is complete, whose
+            value is A `BuilderSlave` if we dispatched a job to it, or None.
         """
         # We need to re-fetch the builder object on each cycle as the
         # Storm store is invalidated over transaction boundaries.
@@ -324,240 +208,72 @@
         self.builder = get_builder(self.builder_name)
 
         if self.builder.builderok:
-            self.builder.updateStatus(self.logger)
-            transaction.commit()
-
-        # See if we think there's an active build on the builder.
-        buildqueue = self.builder.getBuildQueue()
-
-        # XXX Julian 2010-07-29 bug=611258
-        # We're not using the RecordingSlave until dispatching, which
-        # means that this part blocks until we've received a response
-        # from the builder.  updateBuild() needs to be made
-        # asyncronous.
-
-        # Scan the slave and get the logtail, or collect the build if
-        # it's ready.  Yes, "updateBuild" is a bad name.
-        if buildqueue is not None:
-            self.builder.updateBuild(buildqueue)
-            transaction.commit()
-
-        # If the builder is in manual mode, don't dispatch anything.
-        if self.builder.manual:
-            self.logger.debug(
-                '%s is in manual mode, not dispatching.' % self.builder.name)
-            return None
-
-        # If the builder is marked unavailable, don't dispatch anything.
-        # Additionaly, because builders can be removed from the pool at
-        # any time, we need to see if we think there was a build running
-        # on it before it was marked unavailable. In this case we reset
-        # the build thusly forcing it to get re-dispatched to another
-        # builder.
-        if not self.builder.is_available:
-            job = self.builder.currentjob
-            if job is not None and not self.builder.builderok:
-                self.logger.info(
-                    "%s was made unavailable, resetting attached "
-                    "job" % self.builder.name)
-                job.reset()
-                transaction.commit()
-            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)
-        # 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.
-            self.builder.resetFailureCount()
-            transaction.commit()
-            return slave
-
-        return None
-
-    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)
+            d = self.builder.updateStatus(self.logger)
         else:
-            # No resume required, build dispatching can commence.
             d = defer.succeed(None)
 
-        # Dispatch the build to the slave asynchronously.
-        d.addCallback(self.initiateDispatch, slave)
-        # Store this deferred so we can wait for it along with all
-        # the others that will be generated by RecordingSlave during
-        # the dispatch process, and chain a callback after they've
-        # all fired.
-        self._deferred_list.append(d)
-
-    def initiateDispatch(self, resume_result, slave):
-        """Start dispatching a build to a slave.
-
-        If the previous task in chain (slave resuming) has failed it will
-        receive a `ResetBuilderRequest` instance as 'resume_result' and
-        will immediately return that so the subsequent callback can collect
-        it.
-
-        If the slave resuming succeeded, it starts the XMLRPC dialogue.  The
-        dialogue may consist of many calls to the slave before the build
-        starts.  Each call is done via a Deferred event, where slave calls
-        are sent in callSlave(), and checked in checkDispatch() which will
-        keep firing events via callSlave() until all the events are done or
-        an error occurs.
-        """
-        if resume_result is not None:
-            self.slaveConversationEnded()
-            return resume_result
-
-        self.logger.info('Dispatching: %s' % slave)
-        self.callSlave(slave)
-
-    def _getProxyForSlave(self, slave):
-        """Return a twisted.web.xmlrpc.Proxy for the buildd slave.
-
-        Uses a protocol with timeout support, See QueryFactoryWithTimeout.
-        """
-        proxy = xmlrpc.Proxy(str(urlappend(slave.url, 'rpc')))
-        proxy.queryFactory = QueryFactoryWithTimeout
-        return proxy
-
-    def callSlave(self, slave):
-        """Dispatch the next XMLRPC for the given slave."""
-        if len(slave.calls) == 0:
-            # That's the end of the dialogue with the slave.
-            self.slaveConversationEnded()
-            return
-
-        # Get an XMLRPC proxy for the buildd slave.
-        proxy = self._getProxyForSlave(slave)
-        method, args = slave.calls.pop(0)
-        d = proxy.callRemote(method, *args)
-        d.addBoth(self.checkDispatch, method, slave)
-        self._deferred_list.append(d)
-        self.logger.debug('%s -> %s(%s)' % (slave, method, args))
-
-    def slaveConversationEnded(self):
-        """After all the Deferreds are set up, chain a callback on them."""
-        dl = defer.DeferredList(self._deferred_list, consumeErrors=True)
-        dl.addBoth(self.evaluateDispatchResult)
-        return dl
-
-    def evaluateDispatchResult(self, deferred_list_results):
-        """Process the DispatchResult for this dispatch chain.
-
-        After waiting for the Deferred chain to finish, we'll have a
-        DispatchResult to evaluate, which deals with the result of
-        dispatching.
-        """
-        # The `deferred_list_results` is what we get when waiting on a
-        # DeferredList.  It's a list of tuples of (status, result) where
-        # result is what the last callback in that chain returned.
-
-        # If the result is an instance of BaseDispatchResult we need to
-        # evaluate it, as there's further action required at the end of
-        # the dispatch chain.  None, resulting from successful chains,
-        # are discarded.
-
-        dispatch_results = [
-            result for status, result in deferred_list_results
-            if isinstance(result, BaseDispatchResult)]
-
-        for result in dispatch_results:
-            self.logger.info("%r" % result)
-            result()
-
-        # At this point, we're done dispatching, so we can schedule the
-        # next scan cycle.
-        self.scheduleNextScanCycle()
-
-        # For the test suite so that it can chain callback results.
-        return deferred_list_results
-
-    def checkResume(self, response, slave):
-        """Check the result of resuming a slave.
-
-        If there's a problem resuming, we return a ResetDispatchResult which
-        will get evaluated at the end of the scan, or None if the resume
-        was OK.
-
-        :param response: the tuple that's constructed in
-            ProcessWithTimeout.processEnded(), or a Failure that
-            contains the tuple.
-        :param slave: the slave object we're talking to
-        """
-        if isinstance(response, Failure):
-            out, err, code = response.value
-        else:
-            out, err, code = response
-            if code == os.EX_OK:
-                return None
-
-        error_text = '%s\n%s' % (out, err)
-        self.logger.error('%s resume failure: %s' % (slave, error_text))
-        return self.reset_result(slave, error_text)
-
-    def _incrementFailureCounts(self, builder):
-        builder.gotFailure()
-        builder.getCurrentBuildFarmJob().gotFailure()
-
-    def checkDispatch(self, response, method, slave):
-        """Verify the results of a slave xmlrpc call.
-
-        If it failed and it compromises the slave then return a corresponding
-        `FailDispatchResult`, if it was a communication failure, simply
-        reset the slave by returning a `ResetDispatchResult`.
-        """
-        from lp.buildmaster.interfaces.builder import IBuilderSet
-        builder = getUtility(IBuilderSet)[slave.name]
-
-        # XXX these DispatchResult classes are badly named and do the
-        # same thing.  We need to fix that.
-        self.logger.debug(
-            '%s response for "%s": %s' % (slave, method, response))
-
-        if isinstance(response, Failure):
-            self.logger.warn(
-                '%s communication failed (%s)' %
-                (slave, response.getErrorMessage()))
-            self.slaveConversationEnded()
-            self._incrementFailureCounts(builder)
-            return self.fail_result(slave)
-
-        if isinstance(response, list) and len(response) == 2:
-            if method in buildd_success_result_map:
-                expected_status = buildd_success_result_map.get(method)
-                status, info = response
-                if status == expected_status:
-                    self.callSlave(slave)
+        def status_updated(ignored):
+            # Commit the changes done while possibly rescuing jobs, to
+            # avoid holding table locks.
+            transaction.commit()
+
+            # See if we think there's an active build on the builder.
+            buildqueue = self.builder.getBuildQueue()
+
+            # Scan the slave and get the logtail, or collect the build if
+            # it's ready.  Yes, "updateBuild" is a bad name.
+            if buildqueue is not None:
+                return self.builder.updateBuild(buildqueue)
+
+        def build_updated(ignored):
+            # Commit changes done while updating the build, to avoid
+            # holding table locks.
+            transaction.commit()
+
+            # If the builder is in manual mode, don't dispatch anything.
+            if self.builder.manual:
+                self.logger.debug(
+                    '%s is in manual mode, not dispatching.' %
+                    self.builder.name)
+                return
+
+            # If the builder is marked unavailable, don't dispatch anything.
+            # Additionaly, because builders can be removed from the pool at
+            # any time, we need to see if we think there was a build running
+            # on it before it was marked unavailable. In this case we reset
+            # the build thusly forcing it to get re-dispatched to another
+            # builder.
+
+            return self.builder.isAvailable().addCallback(got_available)
+
+        def got_available(available):
+            if not available:
+                job = self.builder.currentjob
+                if job is not None and not self.builder.builderok:
+                    self.logger.info(
+                        "%s was made unavailable, resetting attached "
+                        "job" % self.builder.name)
+                    job.reset()
+                    transaction.commit()
+                return
+
+            # See if there is a job we can dispatch to the builder slave.
+
+            d = self.builder.findAndStartJob()
+            def job_started(candidate):
+                if self.builder.currentjob is not None:
+                    # After a successful dispatch we can reset the
+                    # failure_count.
+                    self.builder.resetFailureCount()
+                    transaction.commit()
+                    return self.builder.slave
+                else:
                     return None
-            else:
-                info = 'Unknown slave method: %s' % method
-        else:
-            info = 'Unexpected response: %s' % repr(response)
-
-        self.logger.error(
-            '%s failed to dispatch (%s)' % (slave, info))
-
-        self.slaveConversationEnded()
-        self._incrementFailureCounts(builder)
-        return self.fail_result(slave, info)
+            return d.addCallback(job_started)
+
+        d.addCallback(status_updated)
+        d.addCallback(build_updated)
+        return d
 
 
 class NewBuildersScanner:
@@ -578,15 +294,21 @@
         self.current_builders = [
             builder.name for builder in getUtility(IBuilderSet)]
 
+    def stop(self):
+        """Terminate the LoopingCall."""
+        self.loop.stop()
+
     def scheduleScan(self):
         """Schedule a callback SCAN_INTERVAL seconds later."""
-        return self._clock.callLater(self.SCAN_INTERVAL, self.scan)
+        self.loop = LoopingCall(self.scan)
+        self.loop.clock = self._clock
+        self.stopping_deferred = self.loop.start(self.SCAN_INTERVAL)
+        return self.stopping_deferred
 
     def scan(self):
         """If a new builder appears, create a SlaveScanner for it."""
         new_builders = self.checkForNewBuilders()
         self.manager.addScanForBuilders(new_builders)
-        self.scheduleScan()
 
     def checkForNewBuilders(self):
         """See if any new builders were added."""
@@ -609,10 +331,7 @@
             manager=self, clock=clock)
 
     def _setupLogger(self):
-        """Setup a 'slave-scanner' logger that redirects to twisted.
-
-        It is going to be used locally and within the thread running
-        the scan() method.
+        """Set up a 'slave-scanner' logger that redirects to twisted.
 
         Make it less verbose to avoid messing too much with the old code.
         """
@@ -643,12 +362,29 @@
         # Events will now fire in the SlaveScanner objects to scan each
         # builder.
 
+    def stopService(self):
+        """Callback for when we need to shut down."""
+        # XXX: lacks unit tests
+        # All the SlaveScanner objects need to be halted gracefully.
+        deferreds = [slave.stopping_deferred for slave in self.builder_slaves]
+        deferreds.append(self.new_builders_scanner.stopping_deferred)
+
+        self.new_builders_scanner.stop()
+        for slave in self.builder_slaves:
+            slave.stopCycle()
+
+        # The 'stopping_deferred's are called back when the loops are
+        # stopped, so we can wait on them all at once here before
+        # exiting.
+        d = defer.DeferredList(deferreds, consumeErrors=True)
+        return d
+
     def addScanForBuilders(self, builders):
         """Set up scanner objects for the builders specified."""
         for builder in builders:
             slave_scanner = SlaveScanner(builder, self.logger)
             self.builder_slaves.append(slave_scanner)
-            slave_scanner.scheduleNextScanCycle()
+            slave_scanner.startCycle()
 
         # Return the slave list for the benefit of tests.
         return self.builder_slaves
 
=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2010-09-24 13:39:27 +0000
+++ lib/lp/buildmaster/model/builder.py	2010-10-25 19:14:01 +0000
@@ -13,12 +13,11 @@
     ]
 
 import gzip
-import httplib
 import logging
 import os
 import socket
-import subprocess
 import tempfile
+import transaction
 import urllib2
 import xmlrpclib
 
@@ -34,6 +33,13 @@
     Count,
     Sum,
     )
+
+from twisted.internet import (
+    defer,
+    reactor as default_reactor,
+    )
+from twisted.web import xmlrpc
+
 from zope.component import getUtility
 from zope.interface import implements
 
@@ -58,7 +64,6 @@
 from lp.buildmaster.interfaces.builder import (
     BuildDaemonError,
     BuildSlaveFailure,
-    CannotBuild,
     CannotFetchFile,
     CannotResumeHost,
     CorruptBuildCookie,
@@ -66,9 +71,6 @@
     IBuilderSet,
     )
 from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJobSet
-from lp.buildmaster.interfaces.buildfarmjobbehavior import (
-    BuildBehaviorMismatch,
-    )
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
 from lp.buildmaster.model.buildfarmjobbehavior import IdleBuildBehavior
 from lp.buildmaster.model.buildqueue import (
@@ -78,9 +80,9 @@
 from lp.registry.interfaces.person import validate_public_person
 from lp.services.job.interfaces.job import JobStatus
 from lp.services.job.model.job import Job
-from lp.services.osutils import until_no_eintr
 from lp.services.propertycache import cachedproperty
-from lp.services.twistedsupport.xmlrpc import BlockingProxy
+from lp.services.twistedsupport.processmonitor import ProcessWithTimeout
+from lp.services.twistedsupport import cancel_on_timeout
 # XXX Michael Nelson 2010-01-13 bug=491330
 # These dependencies on soyuz will be removed when getBuildRecords()
 # is moved.
@@ -92,25 +94,9 @@
 from lp.soyuz.model.processor import Processor
 
 
-class TimeoutHTTPConnection(httplib.HTTPConnection):
-
-    def connect(self):
-        """Override the standard connect() methods to set a timeout"""
-        ret = httplib.HTTPConnection.connect(self)
-        self.sock.settimeout(config.builddmaster.socket_timeout)
-        return ret
-
-
-class TimeoutHTTP(httplib.HTTP):
-    _connection_class = TimeoutHTTPConnection
-
-
-class TimeoutTransport(xmlrpclib.Transport):
-    """XMLRPC Transport to setup a socket with defined timeout"""
-
-    def make_connection(self, host):
-        host, extra_headers, x509 = self.get_host_info(host)
-        return TimeoutHTTP(host)
+class QuietQueryFactory(xmlrpc._QueryFactory):
+    """XMLRPC client factory that doesn't splatter the log with junk."""
+    noisy = False
 
 
 class BuilderSlave(object):
@@ -125,24 +111,7 @@
     # many false positives in your test run and will most likely break
     # production.
 
-    # 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, proxy, builder_url, vm_host):
+    def __init__(self, proxy, builder_url, vm_host, reactor=None):
         """Initialize a BuilderSlave.
 
         :param proxy: An XML-RPC proxy, implementing 'callRemote'. It must
@@ -155,63 +124,87 @@
         self._file_cache_url = urlappend(builder_url, 'filecache')
         self._server = proxy
 
+        if reactor is None:
+            self.reactor = default_reactor
+        else:
+            self.reactor = reactor
+
     @classmethod
-    def makeBlockingSlave(cls, builder_url, vm_host):
-        rpc_url = urlappend(builder_url, 'rpc')
-        server_proxy = xmlrpclib.ServerProxy(
-            rpc_url, transport=TimeoutTransport(), allow_none=True)
-        return cls(BlockingProxy(server_proxy), builder_url, vm_host)
+    def makeBuilderSlave(cls, builder_url, vm_host, reactor=None, proxy=None):
+        """Create and return a `BuilderSlave`.
+
+        :param builder_url: The URL of the slave buildd machine,
+            e.g. http://localhost:8221
+        :param vm_host: If the slave is virtual, specify its host machine here.
+        :param reactor: Used by tests to override the Twisted reactor.
+        :param proxy: Used By tests to override the xmlrpc.Proxy.
+        """
+        rpc_url = urlappend(builder_url.encode('utf-8'), 'rpc')
+        if proxy is None:
+            server_proxy = xmlrpc.Proxy(rpc_url, allowNone=True)
+            server_proxy.queryFactory = QuietQueryFactory
+        else:
+            server_proxy = proxy
+        return cls(server_proxy, builder_url, vm_host, reactor)
+
+    def _with_timeout(self, d):
+        TIMEOUT = config.builddmaster.socket_timeout
+        return cancel_on_timeout(d, TIMEOUT, self.reactor)
 
     def abort(self):
         """Abort the current build."""
-        return self._server.callRemote('abort')
+        return self._with_timeout(self._server.callRemote('abort'))
 
     def clean(self):
         """Clean up the waiting files and reset the slave's internal state."""
-        return self._server.callRemote('clean')
+        return self._with_timeout(self._server.callRemote('clean'))
 
     def echo(self, *args):
         """Echo the arguments back."""
-        return self._server.callRemote('echo', *args)
+        return self._with_timeout(self._server.callRemote('echo', *args))
 
     def info(self):
         """Return the protocol version and the builder methods supported."""
-        return self._server.callRemote('info')
+        return self._with_timeout(self._server.callRemote('info'))
 
     def status(self):
         """Return the status of the build daemon."""
-        return self._server.callRemote('status')
+        return self._with_timeout(self._server.callRemote('status'))
 
     def ensurepresent(self, sha1sum, url, username, password):
+        # XXX: Nothing external calls this. Make it private.
         """Attempt to ensure the given file is present."""
-        return self._server.callRemote(
-            'ensurepresent', sha1sum, url, username, password)
+        return self._with_timeout(self._server.callRemote(
+            'ensurepresent', sha1sum, url, username, password))
 
     def getFile(self, sha_sum):
         """Construct a file-like object to return the named file."""
+        # XXX 2010-10-18 bug=662631
+        # Change this to do non-blocking IO.
         file_url = urlappend(self._file_cache_url, sha_sum)
         return urllib2.urlopen(file_url)
 
-    def resume(self):
-        """Resume a virtual builder.
-
-        It uses the configuration command-line (replacing 'vm_host') and
-        return its output.
-
-        :return: a (stdout, stderr, subprocess exitcode) triple
+    def resume(self, clock=None):
+        """Resume the builder in an asynchronous fashion.
+
+        We use the builddmaster configuration 'socket_timeout' as
+        the process timeout.
+
+        :param clock: An optional twisted.internet.task.Clock to override
+                      the default clock.  For use in tests.
+
+        :return: a Deferred that returns 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()
-        resume_process = subprocess.Popen(
-            resume_argv, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        stdout, stderr = resume_process.communicate()
-
-        return (stdout, stderr, resume_process.returncode)
+        # Twisted API requires string but the configuration provides unicode.
+        resume_argv = [term.encode('utf-8') for term in resume_command.split()]
+        d = defer.Deferred()
+        p = ProcessWithTimeout(
+            d, config.builddmaster.socket_timeout, clock=clock)
+        p.spawnProcess(resume_argv[0], tuple(resume_argv))
+        return d
 
     def cacheFile(self, logger, libraryfilealias):
         """Make sure that the file at 'libraryfilealias' is on the slave.
@@ -224,13 +217,15 @@
             "Asking builder on %s to ensure it has file %s (%s, %s)" % (
                 self._file_cache_url, libraryfilealias.filename, url,
                 libraryfilealias.content.sha1))
-        self.sendFileToSlave(libraryfilealias.content.sha1, url)
+        return self.sendFileToSlave(libraryfilealias.content.sha1, url)
 
     def sendFileToSlave(self, sha1, url, username="", password=""):
         """Helper to send the file at 'url' with 'sha1' to this builder."""
-        present, info = self.ensurepresent(sha1, url, username, password)
-        if not present:
-            raise CannotFetchFile(url, info)
+        d = self.ensurepresent(sha1, url, username, password)
+        def check_present((present, info)):
+            if not present:
+                raise CannotFetchFile(url, info)
+        return d.addCallback(check_present)
 
     def build(self, buildid, builder_type, chroot_sha1, filemap, args):
         """Build a thing on this build slave.
@@ -243,19 +238,18 @@
         :param args: A dictionary of extra arguments. The contents depend on
             the build job type.
         """
-        try:
-            return self._server.callRemote(
-                'build', buildid, builder_type, chroot_sha1, filemap, args)
-        except xmlrpclib.Fault, info:
-            raise BuildSlaveFailure(info)
+        d = self._with_timeout(self._server.callRemote(
+            'build', buildid, builder_type, chroot_sha1, filemap, args))
+        def got_fault(failure):
+            failure.trap(xmlrpclib.Fault)
+            raise BuildSlaveFailure(failure.value)
+        return d.addErrback(got_fault)
 
 
 # This is a separate function since MockBuilder needs to use it too.
 # Do not use it -- (Mock)Builder.rescueIfLost should be used instead.
 def rescueBuilderIfLost(builder, logger=None):
     """See `IBuilder`."""
-    status_sentence = builder.slaveStatusSentence()
-
     # 'ident_position' dict relates the position of the job identifier
     # token in the sentence received from status(), according the
     # two status we care about. See see lib/canonical/buildd/slave.py
@@ -265,61 +259,58 @@
         'BuilderStatus.WAITING': 2
         }
 
-    # Isolate the BuilderStatus string, always the first token in
-    # see lib/canonical/buildd/slave.py and
-    # IBuilder.slaveStatusSentence().
-    status = status_sentence[0]
-
-    # If the cookie test below fails, it will request an abort of the
-    # builder.  This will leave the builder in the aborted state and
-    # with no assigned job, and we should now "clean" the slave which
-    # will reset its state back to IDLE, ready to accept new builds.
-    # This situation is usually caused by a temporary loss of
-    # communications with the slave and the build manager had to reset
-    # the job.
-    if status == 'BuilderStatus.ABORTED' and builder.currentjob is None:
-        builder.cleanSlave()
-        if logger is not None:
-            logger.info(
-                "Builder '%s' cleaned up from ABORTED" % builder.name)
-        return
-
-    # If slave is not building nor waiting, it's not in need of rescuing.
-    if status not in ident_position.keys():
-        return
-
-    slave_build_id = status_sentence[ident_position[status]]
-
-    try:
-        builder.verifySlaveBuildCookie(slave_build_id)
-    except CorruptBuildCookie, reason:
-        if status == 'BuilderStatus.WAITING':
-            builder.cleanSlave()
+    d = builder.slaveStatusSentence()
+
+    def got_status(status_sentence):
+        """After we get the status, clean if we have to.
+
+        Always return status_sentence.
+        """
+        # Isolate the BuilderStatus string, always the first token in
+        # see lib/canonical/buildd/slave.py and
+        # IBuilder.slaveStatusSentence().
+        status = status_sentence[0]
+
+        # If the cookie test below fails, it will request an abort of the
+        # builder.  This will leave the builder in the aborted state and
+        # with no assigned job, and we should now "clean" the slave which
+        # will reset its state back to IDLE, ready to accept new builds.
+        # This situation is usually caused by a temporary loss of
+        # communications with the slave and the build manager had to reset
+        # the job.
+        if status == 'BuilderStatus.ABORTED' and builder.currentjob is None:
+            if logger is not None:
+                logger.info(
+                    "Builder '%s' being cleaned up from ABORTED" %
+                    (builder.name,))
+            d = builder.cleanSlave()
+            return d.addCallback(lambda ignored: status_sentence)
         else:
-            builder.requestAbort()
-        if logger:
-            logger.info(
-                "Builder '%s' rescued from '%s': '%s'" %
-                (builder.name, slave_build_id, reason))
-
-
-def _update_builder_status(builder, logger=None):
-    """Really update the builder status."""
-    try:
-        builder.checkSlaveAlive()
-        builder.rescueIfLost(logger)
-    # Catch only known exceptions.
-    # XXX cprov 2007-06-15 bug=120571: ValueError & TypeError catching is
-    # disturbing in this context. We should spend sometime sanitizing the
-    # exceptions raised in the Builder API since we already started the
-    # main refactoring of this area.
-    except (ValueError, TypeError, xmlrpclib.Fault,
-            BuildDaemonError), reason:
-        builder.failBuilder(str(reason))
-        if logger:
-            logger.warn(
-                "%s (%s) marked as failed due to: %s",
-                builder.name, builder.url, builder.failnotes, exc_info=True)
+            return status_sentence
+
+    def rescue_slave(status_sentence):
+        # If slave is not building nor waiting, it's not in need of rescuing.
+        status = status_sentence[0]
+        if status not in ident_position.keys():
+            return
+        slave_build_id = status_sentence[ident_position[status]]
+        try:
+            builder.verifySlaveBuildCookie(slave_build_id)
+        except CorruptBuildCookie, reason:
+            if status == 'BuilderStatus.WAITING':
+                d = builder.cleanSlave()
+            else:
+                d = builder.requestAbort()
+            def log_rescue(ignored):
+                if logger:
+                    logger.info(
+                        "Builder '%s' rescued from '%s': '%s'" %
+                        (builder.name, slave_build_id, reason))
+            return d.addCallback(log_rescue)
+
+    d.addCallback(got_status)
+    d.addCallback(rescue_slave)
+    return d
 
 
 def updateBuilderStatus(builder, logger=None):
@@ -327,16 +318,7 @@
     if logger:
         logger.debug('Checking %s' % builder.name)
 
-    MAX_EINTR_RETRIES = 42 # pulling a number out of my a$$ here
-    try:
-        return until_no_eintr(
-            MAX_EINTR_RETRIES, _update_builder_status, builder, logger=logger)
-    except socket.error, reason:
-        # In Python 2.6 we can use IOError instead.  It also has
-        # reason.errno but we might be using 2.5 here so use the
-        # index hack.
-        error_message = str(reason)
-        builder.handleTimeout(logger, error_message)
+    return builder.rescueIfLost(logger)
 
 
 class Builder(SQLBase):
@@ -364,6 +346,10 @@
     active = BoolCol(dbName='active', notNull=True, default=True)
     failure_count = IntCol(dbName='failure_count', default=0, notNull=True)
 
+    # The number of times a builder can consecutively fail before we
+    # give up and mark it builderok=False.
+    FAILURE_THRESHOLD = 5
+
     def _getCurrentBuildBehavior(self):
         """Return the current build behavior."""
         if not safe_hasattr(self, '_current_build_behavior'):
@@ -409,18 +395,13 @@
         """See `IBuilder`."""
         self.failure_count = 0
 
-    def checkSlaveAlive(self):
-        """See IBuilder."""
-        if self.slave.echo("Test")[0] != "Test":
-            raise BuildDaemonError("Failed to echo OK")
-
     def rescueIfLost(self, logger=None):
         """See `IBuilder`."""
-        rescueBuilderIfLost(self, logger)
+        return rescueBuilderIfLost(self, logger)
 
     def updateStatus(self, logger=None):
         """See `IBuilder`."""
-        updateBuilderStatus(self, logger)
+        return updateBuilderStatus(self, logger)
 
     def cleanSlave(self):
         """See IBuilder."""
@@ -440,20 +421,23 @@
     def resumeSlaveHost(self):
         """See IBuilder."""
         if not self.virtualized:
-            raise CannotResumeHost('Builder is not virtualized.')
+            return defer.fail(CannotResumeHost('Builder is not virtualized.'))
 
         if not self.vm_host:
-            raise CannotResumeHost('Undefined vm_host.')
+            return defer.fail(CannotResumeHost('Undefined vm_host.'))
 
         logger = self._getSlaveScannerLogger()
         logger.debug("Resuming %s (%s)" % (self.name, self.url))
 
-        stdout, stderr, returncode = self.slave.resume()
-        if returncode != 0:
+        d = self.slave.resume()
+        def got_resume_ok((stdout, stderr, returncode)):
+            return stdout, stderr
+        def got_resume_bad(failure):
+            stdout, stderr, code = failure.value
             raise CannotResumeHost(
                 "Resuming failed:\nOUT:\n%s\nERR:\n%s\n" % (stdout, stderr))
 
-        return stdout, stderr
+        return d.addCallback(got_resume_ok).addErrback(got_resume_bad)
 
     @cachedproperty
     def slave(self):
@@ -462,7 +446,7 @@
         # the slave object, which is usually an XMLRPC client, with a
         # stub object that removes the need to actually create a buildd
         # slave in various states - which can be hard to create.
-        return BuilderSlave.makeBlockingSlave(self.url, self.vm_host)
+        return BuilderSlave.makeBuilderSlave(self.url, self.vm_host)
 
     def setSlaveForTesting(self, proxy):
         """See IBuilder."""
@@ -483,18 +467,23 @@
 
         # If we are building a virtual build, resume the virtual machine.
         if self.virtualized:
-            self.resumeSlaveHost()
+            d = self.resumeSlaveHost()
+        else:
+            d = defer.succeed(None)
 
-        # Do it.
-        build_queue_item.markAsBuilding(self)
-        try:
-            self.current_build_behavior.dispatchBuildToSlave(
+        def resume_done(ignored):
+            return self.current_build_behavior.dispatchBuildToSlave(
                 build_queue_item.id, logger)
-        except BuildSlaveFailure, e:
-            logger.debug("Disabling builder: %s" % self.url, exc_info=1)
+
+        def eb_slave_failure(failure):
+            failure.trap(BuildSlaveFailure)
+            e = failure.value
             self.failBuilder(
                 "Exception (%s) when setting up to new job" % (e,))
-        except CannotFetchFile, e:
+
+        def eb_cannot_fetch_file(failure):
+            failure.trap(CannotFetchFile)
+            e = failure.value
             message = """Slave '%s' (%s) was unable to fetch file.
             ****** URL ********
             %s
@@ -503,10 +492,19 @@
             *******************
             """ % (self.name, self.url, e.file_url, e.error_information)
             raise BuildDaemonError(message)
-        except socket.error, e:
+
+        def eb_socket_error(failure):
+            failure.trap(socket.error)
+            e = failure.value
             error_message = "Exception (%s) when setting up new job" % (e,)
-            self.handleTimeout(logger, error_message)
-            raise BuildSlaveFailure
+            d = self.handleTimeout(logger, error_message)
+            return d.addBoth(lambda ignored: failure)
+
+        d.addCallback(resume_done)
+        d.addErrback(eb_slave_failure)
+        d.addErrback(eb_cannot_fetch_file)
+        d.addErrback(eb_socket_error)
+        return d
 
     def failBuilder(self, reason):
         """See IBuilder"""
@@ -534,22 +532,24 @@
 
     def slaveStatus(self):
         """See IBuilder."""
-        builder_version, builder_arch, mechanisms = self.slave.info()
-        status_sentence = self.slave.status()
-
-        status = {'builder_status': status_sentence[0]}
-
-        # Extract detailed status and log information if present.
-        # Although build_id is also easily extractable here, there is no
-        # valid reason for anything to use it, so we exclude it.
-        if status['builder_status'] == 'BuilderStatus.WAITING':
-            status['build_status'] = status_sentence[1]
-        else:
-            if status['builder_status'] == 'BuilderStatus.BUILDING':
-                status['logtail'] = status_sentence[2]
-
-        self.current_build_behavior.updateSlaveStatus(status_sentence, status)
-        return status
+        d = self.slave.status()
+        def got_status(status_sentence):
+            status = {'builder_status': status_sentence[0]}
+
+            # Extract detailed status and log information if present.
+            # Although build_id is also easily extractable here, there is no
+            # valid reason for anything to use it, so we exclude it.
+            if status['builder_status'] == 'BuilderStatus.WAITING':
+                status['build_status'] = status_sentence[1]
+            else:
+                if status['builder_status'] == 'BuilderStatus.BUILDING':
+                    status['logtail'] = status_sentence[2]
+
+            self.current_build_behavior.updateSlaveStatus(
+                status_sentence, status)
+            return status
+
+        return d.addCallback(got_status)
 
     def slaveStatusSentence(self):
         """See IBuilder."""
@@ -562,13 +562,15 @@
 
     def updateBuild(self, queueItem):
         """See `IBuilder`."""
-        self.current_build_behavior.updateBuild(queueItem)
+        return self.current_build_behavior.updateBuild(queueItem)
 
     def transferSlaveFileToLibrarian(self, file_sha1, filename, private):
         """See IBuilder."""
         out_file_fd, out_file_name = tempfile.mkstemp(suffix=".buildlog")
         out_file = os.fdopen(out_file_fd, "r+")
         try:
+            # XXX 2010-10-18 bug=662631
+            # Change this to do non-blocking IO.
             slave_file = self.slave.getFile(file_sha1)
             copy_and_close(slave_file, out_file)
             # If the requested file is the 'buildlog' compress it using gzip
@@ -599,18 +601,17 @@
 
         return library_file.id
 
-    @property
-    def is_available(self):
+    def isAvailable(self):
         """See `IBuilder`."""
         if not self.builderok:
-            return False
-        try:
-            slavestatus = self.slaveStatusSentence()
-        except (xmlrpclib.Fault, socket.error):
-            return False
-        if slavestatus[0] != BuilderStatus.IDLE:
-            return False
-        return True
+            return defer.succeed(False)
+        d = self.slaveStatusSentence()
+        def catch_fault(failure):
+            failure.trap(xmlrpclib.Fault, socket.error)
+            return False
+        def check_available(status):
+            return status[0] == BuilderStatus.IDLE
+        return d.addCallbacks(check_available, catch_fault)
 
     def _getSlaveScannerLogger(self):
         """Return the logger instance from buildd-slave-scanner.py."""
@@ -621,6 +622,27 @@
         logger = logging.getLogger('slave-scanner')
         return logger
 
+    def acquireBuildCandidate(self):
+        """Acquire a build candidate in an atomic fashion.
+
+        When retrieiving a candidate we need to mark it as building
+        immediately so that it is not dispatched by another builder in the
+        build manager.
+
+        We can consider this to be atomic because although the build manager
+        is a Twisted app and gives the appearance of doing lots of things at
+        once, it's still single-threaded so no more than one builder scan
+        can be in this code at the same time.
+
+        If there's ever more than one build manager running at once, then
+        this code will need some sort of mutex.
+        """
+        candidate = self._findBuildCandidate()
+        if candidate is not None:
+            candidate.markAsBuilding(self)
+            transaction.commit()
+        return candidate
+
     def _findBuildCandidate(self):
         """Find a candidate job for dispatch to an idle buildd slave.
 
@@ -700,52 +722,46 @@
         :param candidate: The job to dispatch.
         """
         logger = self._getSlaveScannerLogger()
-        try:
-            self.startBuild(candidate, logger)
-        except (BuildSlaveFailure, CannotBuild, BuildBehaviorMismatch), err:
-            logger.warn('Could not build: %s' % err)
+        # Using maybeDeferred ensures that any exceptions are also
+        # wrapped up and caught later.
+        d = defer.maybeDeferred(self.startBuild, candidate, logger)
+        return d
 
     def handleTimeout(self, logger, error_message):
         """See IBuilder."""
-        builder_should_be_failed = True
-
         if self.virtualized:
             # Virtualized/PPA builder: attempt a reset.
             logger.warn(
                 "Resetting builder: %s -- %s" % (self.url, error_message),
                 exc_info=True)
-            try:
-                self.resumeSlaveHost()
-            except CannotResumeHost, err:
-                # Failed to reset builder.
-                logger.warn(
-                    "Failed to reset builder: %s -- %s" %
-                    (self.url, str(err)), exc_info=True)
-            else:
-                # Builder was reset, do *not* mark it as failed.
-                builder_should_be_failed = False
-
-        if builder_should_be_failed:
+            d = self.resumeSlaveHost()
+            return d
+        else:
+            # XXX: This should really let the failure bubble up to the
+            # scan() method that does the failure counting.
             # Mark builder as 'failed'.
             logger.warn(
-                "Disabling builder: %s -- %s" % (self.url, error_message),
-                exc_info=True)
+                "Disabling builder: %s -- %s" % (self.url, error_message))
             self.failBuilder(error_message)
+            return defer.succeed(None)
 
     def findAndStartJob(self, buildd_slave=None):
         """See IBuilder."""
+        # XXX This method should be removed in favour of two separately
+        # called methods that find and dispatch the job.  It will
+        # require a lot of test fixing.
         logger = self._getSlaveScannerLogger()
-        candidate = self._findBuildCandidate()
+        candidate = self.acquireBuildCandidate()
 
         if candidate is None:
             logger.debug("No build candidates available for builder.")
-            return None
+            return defer.succeed(None)
 
         if buildd_slave is not None:
             self.setSlaveForTesting(buildd_slave)
 
-        self._dispatchBuildCandidate(candidate)
-        return candidate
+        d = self._dispatchBuildCandidate(candidate)
+        return d.addCallback(lambda ignored: candidate)
 
     def getBuildQueue(self):
         """See `IBuilder`."""
 
=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py	2010-08-20 20:31:18 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py	2010-10-25 19:14:01 +0000
@@ -16,13 +16,18 @@
 import socket
 import xmlrpclib
 
+from twisted.internet import defer
+
 from zope.component import getUtility
 from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
 
 from canonical import encoding
 from canonical.librarian.interfaces import ILibrarianClient
-from lp.buildmaster.interfaces.builder import CorruptBuildCookie
+from lp.buildmaster.interfaces.builder import (
+    BuildSlaveFailure,
+    CorruptBuildCookie,
+    )
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     BuildBehaviorMismatch,
     IBuildFarmJobBehavior,
@@ -69,54 +74,53 @@
         """See `IBuildFarmJobBehavior`."""
         logger = logging.getLogger('slave-scanner')
 
-        try:
-            slave_status = self._builder.slaveStatus()
-        except (xmlrpclib.Fault, socket.error), info:
-            # XXX cprov 2005-06-29:
-            # Hmm, a problem with the xmlrpc interface,
-            # disable the builder ?? or simple notice the failure
-            # with a timestamp.
+        d = self._builder.slaveStatus()
+
+        def got_failure(failure):
+            failure.trap(xmlrpclib.Fault, socket.error)
+            info = failure.value
             info = ("Could not contact the builder %s, caught a (%s)"
                     % (queueItem.builder.url, info))
-            logger.debug(info, exc_info=True)
-            # keep the job for scan
-            return
-
-        builder_status_handlers = {
-            'BuilderStatus.IDLE': self.updateBuild_IDLE,
-            'BuilderStatus.BUILDING': self.updateBuild_BUILDING,
-            'BuilderStatus.ABORTING': self.updateBuild_ABORTING,
-            'BuilderStatus.ABORTED': self.updateBuild_ABORTED,
-            'BuilderStatus.WAITING': self.updateBuild_WAITING,
-            }
-
-        builder_status = slave_status['builder_status']
-        if builder_status not in builder_status_handlers:
-            logger.critical(
-                "Builder on %s returned unknown status %s, failing it"
-                % (self._builder.url, builder_status))
-            self._builder.failBuilder(
-                "Unknown status code (%s) returned from status() probe."
-                % builder_status)
-            # XXX: This will leave the build and job in a bad state, but
-            # should never be possible, since our builder statuses are
-            # known.
-            queueItem._builder = None
-            queueItem.setDateStarted(None)
-            return
-
-        # Since logtail is a xmlrpclib.Binary container and it is returned
-        # from the IBuilder content class, it arrives protected by a Zope
-        # Security Proxy, which is not declared, thus empty. Before passing
-        # it to the status handlers we will simply remove the proxy.
-        logtail = removeSecurityProxy(slave_status.get('logtail'))
-
-        method = builder_status_handlers[builder_status]
-        try:
-            method(queueItem, slave_status, logtail, logger)
-        except TypeError, e:
-            logger.critical("Received wrong number of args in response.")
-            logger.exception(e)
+            raise BuildSlaveFailure(info)
+
+        def got_status(slave_status):
+            builder_status_handlers = {
+                'BuilderStatus.IDLE': self.updateBuild_IDLE,
+                'BuilderStatus.BUILDING': self.updateBuild_BUILDING,
+                'BuilderStatus.ABORTING': self.updateBuild_ABORTING,
+                'BuilderStatus.ABORTED': self.updateBuild_ABORTED,
+                'BuilderStatus.WAITING': self.updateBuild_WAITING,
+                }
+
+            builder_status = slave_status['builder_status']
+            if builder_status not in builder_status_handlers:
+                logger.critical(
+                    "Builder on %s returned unknown status %s, failing it"
+                    % (self._builder.url, builder_status))
+                self._builder.failBuilder(
+                    "Unknown status code (%s) returned from status() probe."
+                    % builder_status)
+                # XXX: This will leave the build and job in a bad state, but
+                # should never be possible, since our builder statuses are
+                # known.
+                queueItem._builder = None
+                queueItem.setDateStarted(None)
+                return
+
+            # Since logtail is a xmlrpclib.Binary container and it is
+            # returned from the IBuilder content class, it arrives
+            # protected by a Zope Security Proxy, which is not declared,
+            # thus empty. Before passing it to the status handlers we
+            # will simply remove the proxy.
+            logtail = removeSecurityProxy(slave_status.get('logtail'))
+
+            method = builder_status_handlers[builder_status]
+            return defer.maybeDeferred(
+                method, queueItem, slave_status, logtail, logger)
+
+        d.addErrback(got_failure)
+        d.addCallback(got_status)
+        return d
 
     def updateBuild_IDLE(self, queueItem, slave_status, logtail, logger):
         """Somehow the builder forgot about the build job.
@@ -146,11 +150,13 @@
 
         Clean the builder for another jobs.
         """
-        queueItem.builder.cleanSlave()
-        queueItem.builder = None
-        if queueItem.job.status != JobStatus.FAILED:
-            queueItem.job.fail()
-        queueItem.specific_job.jobAborted()
+        d = queueItem.builder.cleanSlave()
+        def got_cleaned(ignored):
+            queueItem.builder = None
+            if queueItem.job.status != JobStatus.FAILED:
+                queueItem.job.fail()
+            queueItem.specific_job.jobAborted()
+        return d.addCallback(got_cleaned)
 
     def extractBuildStatus(self, slave_status):
         """Read build status name.
@@ -185,6 +191,8 @@
         # XXX: dsilvers 2005-03-02: Confirm the builder has the right build?
 
         build = queueItem.specific_job.build
+        # XXX 2010-10-18 bug=662631
+        # Change this to do non-blocking IO.
         build.handleStatus(build_status, librarian, slave_status)
 
 
 
=== modified file 'lib/lp/buildmaster/model/packagebuild.py'
--- lib/lp/buildmaster/model/packagebuild.py	2010-10-02 11:41:43 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2010-10-25 19:14:01 +0000
@@ -165,6 +165,8 @@
     def getLogFromSlave(package_build):
         """See `IPackageBuild`."""
         builder = package_build.buildqueue_record.builder
+        # XXX 2010-10-18 bug=662631
+        # Change this to do non-blocking IO.
         return builder.transferSlaveFileToLibrarian(
             SLAVE_LOG_FILENAME,
             package_build.buildqueue_record.getLogFileName(),
@@ -180,6 +182,8 @@
         # log, builder and date_finished are read-only, so we must
         # currently remove the security proxy to set them.
         naked_build = removeSecurityProxy(build)
+        # XXX 2010-10-18 bug=662631
+        # Change this to do non-blocking IO.
         naked_build.log = build.getLogFromSlave(build)
         naked_build.builder = build.buildqueue_record.builder
         # XXX cprov 20060615 bug=120584: Currently buildduration includes
@@ -276,6 +280,8 @@
             logger.critical("Unknown BuildStatus '%s' for builder '%s'"
                             % (status, self.buildqueue_record.builder.url))
             return
+        # XXX 2010-10-18 bug=662631
+        # Change this to do non-blocking IO.
         method(librarian, slave_status, logger)
 
     def _handleStatus_OK(self, librarian, slave_status, logger):
 
=== modified file 'lib/lp/buildmaster/tests/mock_slaves.py'
--- lib/lp/buildmaster/tests/mock_slaves.py	2010-09-23 12:35:21 +0000
+++ lib/lp/buildmaster/tests/mock_slaves.py	2010-10-25 19:14:01 +0000
@@ -6,21 +6,40 @@
 __metaclass__ = type
 
 __all__ = [
+    'AbortedSlave',
+    'AbortingSlave',
+    'BrokenSlave',
+    'BuildingSlave',
+    'CorruptBehavior',
+    'DeadProxy',
+    'LostBuildingBrokenSlave',
     'MockBuilder',
-    'LostBuildingBrokenSlave',
-    'BrokenSlave',
     'OkSlave',
-    'BuildingSlave',
-    'AbortedSlave',
+    'SlaveTestHelpers',
+    'TrivialBehavior',
     'WaitingSlave',
-    'AbortingSlave',
     ]
 
+import fixtures
+import os
+
 from StringIO import StringIO
 import xmlrpclib
 
-from lp.buildmaster.interfaces.builder import CannotFetchFile
+from testtools.content import Content
+from testtools.content_type import UTF8_TEXT
+
+from twisted.internet import defer
+from twisted.web import xmlrpc
+
+from canonical.buildd.tests.harness import BuilddSlaveTestSetup
+
+from lp.buildmaster.interfaces.builder import (
+    CannotFetchFile,
+    CorruptBuildCookie,
+    )
 from lp.buildmaster.model.builder import (
+    BuilderSlave,
     rescueBuilderIfLost,
     updateBuilderStatus,
     )
@@ -59,15 +78,9 @@
             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()
 
     def resumeSlave(self, logger):
@@ -77,10 +90,10 @@
         pass
 
     def rescueIfLost(self, logger=None):
-        rescueBuilderIfLost(self, logger)
+        return rescueBuilderIfLost(self, logger)
 
     def updateStatus(self, logger=None):
-        updateBuilderStatus(self, logger)
+        return defer.maybeDeferred(updateBuilderStatus, self, logger)
 
 
 # XXX: It would be *really* nice to run some set of tests against the real
@@ -95,36 +108,44 @@
         self.arch_tag = arch_tag
 
     def status(self):
-        return ('BuilderStatus.IDLE', '')
+        return defer.succeed(('BuilderStatus.IDLE', ''))
 
     def ensurepresent(self, sha1, url, user=None, password=None):
         self.call_log.append(('ensurepresent', url, user, password))
-        return True, None
+        return defer.succeed((True, None))
 
     def build(self, buildid, buildtype, chroot, filemap, args):
         self.call_log.append(
             ('build', buildid, buildtype, chroot, filemap.keys(), args))
         info = 'OkSlave BUILDING'
-        return ('BuildStatus.Building', info)
+        return defer.succeed(('BuildStatus.Building', info))
 
     def echo(self, *args):
         self.call_log.append(('echo',) + args)
-        return args
+        return defer.succeed(args)
 
     def clean(self):
         self.call_log.append('clean')
+        return defer.succeed(None)
 
     def abort(self):
         self.call_log.append('abort')
+        return defer.succeed(None)
 
     def info(self):
         self.call_log.append('info')
-        return ('1.0', self.arch_tag, 'debian')
+        return defer.succeed(('1.0', self.arch_tag, 'debian'))
+
+    def resume(self):
+        self.call_log.append('resume')
+        return defer.succeed(("", "", 0))
 
     def sendFileToSlave(self, sha1, url, username="", password=""):
-        present, info = self.ensurepresent(sha1, url, username, password)
-        if not present:
-            raise CannotFetchFile(url, info)
+        d = self.ensurepresent(sha1, url, username, password)
+        def check_present((present, info)):
+            if not present:
+                raise CannotFetchFile(url, info)
+        return d.addCallback(check_present)
 
     def cacheFile(self, logger, libraryfilealias):
         return self.sendFileToSlave(
@@ -141,9 +162,11 @@
     def status(self):
         self.call_log.append('status')
         buildlog = xmlrpclib.Binary("This is a build log")
-        return ('BuilderStatus.BUILDING', self.build_id, buildlog)
+        return defer.succeed(
+            ('BuilderStatus.BUILDING', self.build_id, buildlog))
 
     def getFile(self, sum):
+        # XXX: This needs to be updated to return a Deferred.
         self.call_log.append('getFile')
         if sum == "buildlog":
             s = StringIO("This is a build log")
@@ -155,11 +178,15 @@
     """A mock slave that looks like it's currently waiting."""
 
     def __init__(self, state='BuildStatus.OK', dependencies=None,
-                 build_id='1-1'):
+                 build_id='1-1', filemap=None):
         super(WaitingSlave, self).__init__()
         self.state = state
         self.dependencies = dependencies
         self.build_id = build_id
+        if filemap is None:
+            self.filemap = {}
+        else:
+            self.filemap = filemap
 
         # By default, the slave only has a buildlog, but callsites
         # can update this list as needed.
@@ -167,10 +194,12 @@
 
     def status(self):
         self.call_log.append('status')
-        return ('BuilderStatus.WAITING', self.state, self.build_id, {},
-                self.dependencies)
+        return defer.succeed((
+            'BuilderStatus.WAITING', self.state, self.build_id, self.filemap,
+            self.dependencies))
 
     def getFile(self, hash):
+        # XXX: This needs to be updated to return a Deferred.
         self.call_log.append('getFile')
         if hash in self.valid_file_hashes:
             content = "This is a %s" % hash
@@ -184,15 +213,19 @@
 
     def status(self):
         self.call_log.append('status')
-        return ('BuilderStatus.ABORTING', '1-1')
+        return defer.succeed(('BuilderStatus.ABORTING', '1-1'))
 
 
 class AbortedSlave(OkSlave):
     """A mock slave that looks like it's aborted."""
 
+    def clean(self):
+        self.call_log.append('status')
+        return defer.succeed(None)
+
     def status(self):
-        self.call_log.append('status')
-        return ('BuilderStatus.ABORTED', '1-1')
+        self.call_log.append('clean')
+        return defer.succeed(('BuilderStatus.ABORTED', '1-1'))
 
 
 class LostBuildingBrokenSlave:
@@ -206,16 +239,108 @@
 
     def status(self):
         self.call_log.append('status')
-        return ('BuilderStatus.BUILDING', '1000-10000')
+        return defer.succeed(('BuilderStatus.BUILDING', '1000-10000'))
 
     def abort(self):
         self.call_log.append('abort')
-        raise xmlrpclib.Fault(8002, "Could not abort")
+        return defer.fail(xmlrpclib.Fault(8002, "Could not abort"))
 
 
 class BrokenSlave:
     """A mock slave that reports that it is broken."""
 
+    def __init__(self):
+        self.call_log = []
+
     def status(self):
         self.call_log.append('status')
-        raise xmlrpclib.Fault(8001, "Broken slave")
+        return defer.fail(xmlrpclib.Fault(8001, "Broken slave"))
+
+
+class CorruptBehavior:
+
+    def verifySlaveBuildCookie(self, cookie):
+        raise CorruptBuildCookie("Bad value: %r" % (cookie,))
+
+
+class TrivialBehavior:
+
+    def verifySlaveBuildCookie(self, cookie):
+        pass
+
+
+class DeadProxy(xmlrpc.Proxy):
+    """An xmlrpc.Proxy that doesn't actually send any messages.
+
+    Used when you want to test timeouts, for example.
+    """
+
+    def callRemote(self, *args, **kwargs):
+        return defer.Deferred()
+
+
+class SlaveTestHelpers(fixtures.Fixture):
+
+    # The URL for the XML-RPC service set up by `BuilddSlaveTestSetup`.
+    BASE_URL = 'http://localhost:8221'
+    TEST_URL = '%s/rpc/' % (BASE_URL,)
+
+    def getServerSlave(self):
+        """Set up a test build slave server.
+
+        :return: A `BuilddSlaveTestSetup` object.
+        """
+        tachandler = BuilddSlaveTestSetup()
+        tachandler.setUp()
+        # Basically impossible to do this w/ TrialTestCase. But it would be
+        # really nice to keep it.
+        #
+        # 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, reactor=None, proxy=None):
+        """Return a `BuilderSlave` for use in testing.
+
+        Points to a fixed URL that is also used by `BuilddSlaveTestSetup`.
+        """
+        return BuilderSlave.makeBuilderSlave(
+            self.TEST_URL, 'vmhost', reactor, proxy)
+
+    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 = 'random-build-id'
+        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'})
 
=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2010-10-06 09:06:30 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2010-10-25 19:14:01 +0000
@@ -3,20 +3,24 @@
 
 """Test Builder features."""
 
-import errno
 import os
-import socket
+import signal
 import xmlrpclib
 
-from testtools.content import Content
-from testtools.content_type import UTF8_TEXT
+from twisted.web.client import getPage
+
+from twisted.internet.defer import CancelledError
+from twisted.internet.task import Clock
+from twisted.python.failure import Failure
+from twisted.trial.unittest import TestCase as TrialTestCase
 
 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.config import config
 from canonical.database.sqlbase import flush_database_updates
+from canonical.launchpad.scripts import QuietFakeLogger
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR,
     IStoreSelector,
@@ -24,21 +28,38 @@
     )
 from canonical.testing.layers import (
     DatabaseFunctionalLayer,
-    LaunchpadZopelessLayer
+    LaunchpadZopelessLayer,
+    TwistedLaunchpadZopelessLayer,
+    TwistedLayer,
     )
 from lp.buildmaster.enums import BuildStatus
-from lp.buildmaster.interfaces.builder import IBuilder, IBuilderSet
+from lp.buildmaster.interfaces.builder import (
+    CannotFetchFile,
+    IBuilder,
+    IBuilderSet,
+    )
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     IBuildFarmJobBehavior,
     )
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
-from lp.buildmaster.model.builder import BuilderSlave
+from lp.buildmaster.interfaces.builder import CannotResumeHost
 from lp.buildmaster.model.buildfarmjobbehavior import IdleBuildBehavior
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.buildmaster.tests.mock_slaves import (
     AbortedSlave,
+    AbortingSlave,
+    BrokenSlave,
+    BuildingSlave,
+    CorruptBehavior,
+    DeadProxy,
+    LostBuildingBrokenSlave,
     MockBuilder,
+    OkSlave,
+    SlaveTestHelpers,
+    TrivialBehavior,
+    WaitingSlave,
     )
+from lp.services.job.interfaces.job import JobStatus
 from lp.soyuz.enums import (
     ArchivePurpose,
     PackagePublishingStatus,
@@ -49,9 +70,12 @@
     )
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import (
-    TestCase,
+    ANONYMOUS,
+    login_as,
+    logout,
     TestCaseWithFactory,
     )
+from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.fakemethod import FakeMethod
 
 
@@ -92,42 +116,121 @@
         bq = builder.getBuildQueue()
         self.assertIs(None, bq)
 
-    def test_updateBuilderStatus_catches_repeated_EINTR(self):
-        # A single EINTR return from a socket operation should cause the
-        # operation to be retried, not fail/reset the builder.
-        builder = removeSecurityProxy(self.factory.makeBuilder())
-        builder.handleTimeout = FakeMethod()
-        builder.rescueIfLost = FakeMethod()
-
-        def _fake_checkSlaveAlive():
-            # Raise an EINTR error for all invocations.
-            raise socket.error(errno.EINTR, "fake eintr")
-
-        builder.checkSlaveAlive = _fake_checkSlaveAlive
-        builder.updateStatus()
-
-        # builder.updateStatus should eventually have called
-        # handleTimeout()
-        self.assertEqual(1, builder.handleTimeout.call_count)
-
-    def test_updateBuilderStatus_catches_single_EINTR(self):
-        builder = removeSecurityProxy(self.factory.makeBuilder())
-        builder.handleTimeout = FakeMethod()
-        builder.rescueIfLost = FakeMethod()
-        self.eintr_returned = False
-
-        def _fake_checkSlaveAlive():
-            # raise an EINTR error for the first invocation only.
-            if not self.eintr_returned:
-                self.eintr_returned = True
-                raise socket.error(errno.EINTR, "fake eintr")
-
-        builder.checkSlaveAlive = _fake_checkSlaveAlive
-        builder.updateStatus()
-
-        # builder.updateStatus should never call handleTimeout() for a
-        # single EINTR.
-        self.assertEqual(0, builder.handleTimeout.call_count)
+
+class TestBuilderWithTrial(TrialTestCase):
+
+    layer = TwistedLaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestBuilderWithTrial, self)
+        self.slave_helper = SlaveTestHelpers()
+        self.slave_helper.setUp()
+        self.addCleanup(self.slave_helper.cleanUp)
+        self.factory = LaunchpadObjectFactory()
+        login_as(ANONYMOUS)
+        self.addCleanup(logout)
+
+    def test_updateStatus_aborts_lost_and_broken_slave(self):
+        # A slave that's 'lost' should be aborted; when the slave is
+        # broken then abort() should also throw a fault.
+        slave = LostBuildingBrokenSlave()
+        lostbuilding_builder = MockBuilder(
+            'Lost Building Broken Slave', slave, behavior=CorruptBehavior())
+        d = lostbuilding_builder.updateStatus(QuietFakeLogger())
+        def check_slave_status(failure):
+            self.assertIn('abort', slave.call_log)
+            # 'Fault' comes from the LostBuildingBrokenSlave, this is
+            # just testing that the value is passed through.
+            self.assertIsInstance(failure.value, xmlrpclib.Fault)
+        return d.addBoth(check_slave_status)
+
+    def test_resumeSlaveHost_nonvirtual(self):
+        builder = self.factory.makeBuilder(virtualized=False)
+        d = builder.resumeSlaveHost()
+        return self.assertFailure(d, CannotResumeHost)
+
+    def test_resumeSlaveHost_no_vmhost(self):
+        builder = self.factory.makeBuilder(virtualized=True, vm_host=None)
+        d = builder.resumeSlaveHost()
+        return self.assertFailure(d, CannotResumeHost)
+
+    def test_resumeSlaveHost_success(self):
+        reset_config = """
+            [builddmaster]
+            vm_resume_command: /bin/echo -n parp"""
+        config.push('reset', reset_config)
+        self.addCleanup(config.pop, 'reset')
+
+        builder = self.factory.makeBuilder(virtualized=True, vm_host="pop")
+        d = builder.resumeSlaveHost()
+        def got_resume(output):
+            self.assertEqual(('parp', ''), output)
+        return d.addCallback(got_resume)
+
+    def test_resumeSlaveHost_command_failed(self):
+        reset_fail_config = """
+            [builddmaster]
+            vm_resume_command: /bin/false"""
+        config.push('reset fail', reset_fail_config)
+        self.addCleanup(config.pop, 'reset fail')
+        builder = self.factory.makeBuilder(virtualized=True, vm_host="pop")
+        d = builder.resumeSlaveHost()
+        return self.assertFailure(d, CannotResumeHost)
+
+    def test_handleTimeout_resume_failure(self):
+        reset_fail_config = """
+            [builddmaster]
+            vm_resume_command: /bin/false"""
+        config.push('reset fail', reset_fail_config)
+        self.addCleanup(config.pop, 'reset fail')
+        builder = self.factory.makeBuilder(virtualized=True, vm_host="pop")
+        builder.builderok = True
+        d = builder.handleTimeout(QuietFakeLogger(), 'blah')
+        return self.assertFailure(d, CannotResumeHost)
+
+    def _setupRecipeBuildAndBuilder(self):
+        # Helper function to make a builder capable of building a
+        # recipe, returning both.
+        processor = self.factory.makeProcessor(name="i386")
+        builder = self.factory.makeBuilder(
+            processor=processor, virtualized=True, vm_host="bladh")
+        builder.setSlaveForTesting(OkSlave())
+        distroseries = self.factory.makeDistroSeries()
+        das = self.factory.makeDistroArchSeries(
+            distroseries=distroseries, architecturetag="i386",
+            processorfamily=processor.family)
+        chroot = self.factory.makeLibraryFileAlias()
+        das.addOrUpdateChroot(chroot)
+        distroseries.nominatedarchindep = das
+        build = self.factory.makeSourcePackageRecipeBuild(
+            distroseries=distroseries)
+        return builder, build
+
+    def test_findAndStartJob_returns_candidate(self):
+        # findAndStartJob finds the next queued job using _findBuildCandidate.
+        # We don't care about the type of build at all.
+        builder, build = self._setupRecipeBuildAndBuilder()
+        candidate = build.queueBuild()
+        # _findBuildCandidate is tested elsewhere, we just make sure that
+        # findAndStartJob delegates to it.
+        removeSecurityProxy(builder)._findBuildCandidate = FakeMethod(
+            result=candidate)
+        d = builder.findAndStartJob()
+        return d.addCallback(self.assertEqual, candidate)
+
+    def test_findAndStartJob_starts_job(self):
+        # findAndStartJob finds the next queued job using _findBuildCandidate
+        # and then starts it.
+        # We don't care about the type of build at all.
+        builder, build = self._setupRecipeBuildAndBuilder()
+        candidate = build.queueBuild()
+        removeSecurityProxy(builder)._findBuildCandidate = FakeMethod(
+            result=candidate)
+        d = builder.findAndStartJob()
+        def check_build_started(candidate):
+            self.assertEqual(candidate.builder, builder)
+            self.assertEqual(BuildStatus.BUILDING, build.status)
+        return d.addCallback(check_build_started)
 
     def test_slave(self):
         # Builder.slave is a BuilderSlave that points at the actual Builder.
@@ -136,25 +239,147 @@
         builder = removeSecurityProxy(self.factory.makeBuilder())
         self.assertEqual(builder.url, builder.slave.url)
 
-
-class Test_rescueBuilderIfLost(TestCaseWithFactory):
-    """Tests for lp.buildmaster.model.builder.rescueBuilderIfLost."""
-
-    layer = LaunchpadZopelessLayer
-
     def test_recovery_of_aborted_slave(self):
         # If a slave is in the ABORTED state, rescueBuilderIfLost should
         # clean it if we don't think it's currently building anything.
         # See bug 463046.
         aborted_slave = AbortedSlave()
-        # The slave's clean() method is normally an XMLRPC call, so we
-        # can just stub it out and check that it got called.
-        aborted_slave.clean = FakeMethod()
         builder = MockBuilder("mock_builder", aborted_slave)
         builder.currentjob = None
-        builder.rescueIfLost()
-
-        self.assertEqual(1, aborted_slave.clean.call_count)
+        d = builder.rescueIfLost()
+        def check_slave_calls(ignored):
+            self.assertIn('clean', aborted_slave.call_log)
+        return d.addCallback(check_slave_calls)
+
+    def test_recover_ok_slave(self):
+        # An idle slave is not rescued.
+        slave = OkSlave()
+        builder = MockBuilder("mock_builder", slave, TrivialBehavior())
+        d = builder.rescueIfLost()
+        def check_slave_calls(ignored):
+            self.assertNotIn('abort', slave.call_log)
+            self.assertNotIn('clean', slave.call_log)
+        return d.addCallback(check_slave_calls)
+
+    def test_recover_waiting_slave_with_good_id(self):
+        # rescueIfLost does not attempt to abort or clean a builder that is
+        # WAITING.
+        waiting_slave = WaitingSlave()
+        builder = MockBuilder("mock_builder", waiting_slave, TrivialBehavior())
+        d = builder.rescueIfLost()
+        def check_slave_calls(ignored):
+            self.assertNotIn('abort', waiting_slave.call_log)
+            self.assertNotIn('clean', waiting_slave.call_log)
+        return d.addCallback(check_slave_calls)
+
+    def test_recover_waiting_slave_with_bad_id(self):
+        # If a slave is WAITING with a build for us to get, and the build
+        # cookie cannot be verified, which means we don't recognize the build,
+        # then rescueBuilderIfLost should attempt to abort it, so that the
+        # builder is reset for a new build, and the corrupt build is
+        # discarded.
+        waiting_slave = WaitingSlave()
+        builder = MockBuilder("mock_builder", waiting_slave, CorruptBehavior())
+        d = builder.rescueIfLost()
+        def check_slave_calls(ignored):
+            self.assertNotIn('abort', waiting_slave.call_log)
+            self.assertIn('clean', waiting_slave.call_log)
+        return d.addCallback(check_slave_calls)
+
+    def test_recover_building_slave_with_good_id(self):
+        # rescueIfLost does not attempt to abort or clean a builder that is
+        # BUILDING.
+        building_slave = BuildingSlave()
+        builder = MockBuilder("mock_builder", building_slave, TrivialBehavior())
+        d = builder.rescueIfLost()
+        def check_slave_calls(ignored):
+            self.assertNotIn('abort', building_slave.call_log)
+            self.assertNotIn('clean', building_slave.call_log)
+        return d.addCallback(check_slave_calls)
+
+    def test_recover_building_slave_with_bad_id(self):
+        # If a slave is BUILDING with a build id we don't recognize, then we
+        # abort the build, thus stopping it in its tracks.
+        building_slave = BuildingSlave()
+        builder = MockBuilder("mock_builder", building_slave, CorruptBehavior())
+        d = builder.rescueIfLost()
+        def check_slave_calls(ignored):
+            self.assertIn('abort', building_slave.call_log)
+            self.assertNotIn('clean', building_slave.call_log)
+        return d.addCallback(check_slave_calls)
+
+
+class TestBuilderSlaveStatus(TestBuilderWithTrial):
+
+    # Verify what IBuilder.slaveStatus returns with slaves in different
+    # states.
+
+    def assertStatus(self, slave, builder_status=None,
+                     build_status=None, logtail=False, filemap=None,
+                     dependencies=None):
+        builder = self.factory.makeBuilder()
+        builder.setSlaveForTesting(slave)
+        d = builder.slaveStatus()
+
+        def got_status(status_dict):
+            expected = {}
+            if builder_status is not None:
+                expected["builder_status"] = builder_status
+            if build_status is not None:
+                expected["build_status"] = build_status
+            if dependencies is not None:
+                expected["dependencies"] = dependencies
+
+            # We don't care so much about the content of the logtail,
+            # just that it's there.
+            if logtail:
+                tail = status_dict.pop("logtail")
+                self.assertIsInstance(tail, xmlrpclib.Binary)
+
+            self.assertEqual(expected, status_dict)
+
+        return d.addCallback(got_status)
+
+    def test_slaveStatus_idle_slave(self):
+        self.assertStatus(
+            OkSlave(), builder_status='BuilderStatus.IDLE')
+
+    def test_slaveStatus_building_slave(self):
+        self.assertStatus(
+            BuildingSlave(), builder_status='BuilderStatus.BUILDING',
+            logtail=True)
+
+    def test_slaveStatus_waiting_slave(self):
+        self.assertStatus(
+            WaitingSlave(), builder_status='BuilderStatus.WAITING',
+            build_status='BuildStatus.OK', filemap={})
+
+    def test_slaveStatus_aborting_slave(self):
+        self.assertStatus(
+            AbortingSlave(), builder_status='BuilderStatus.ABORTING')
+
+    def test_slaveStatus_aborted_slave(self):
+        self.assertStatus(
+            AbortedSlave(), builder_status='BuilderStatus.ABORTED')
+
+    def test_isAvailable_with_not_builderok(self):
+        # isAvailable() is a wrapper around slaveStatusSentence()
+        builder = self.factory.makeBuilder()
+        builder.builderok = False
+        d = builder.isAvailable()
+        return d.addCallback(self.assertFalse)
+
+    def test_isAvailable_with_slave_fault(self):
+        builder = self.factory.makeBuilder()
+        builder.setSlaveForTesting(BrokenSlave())
+        d = builder.isAvailable()
+        return d.addCallback(self.assertFalse)
+
+    def test_isAvailable_with_slave_idle(self):
+        builder = self.factory.makeBuilder()
+        builder.setSlaveForTesting(OkSlave())
+        d = builder.isAvailable()
+        return d.addCallback(self.assertTrue)
 
 
 class TestFindBuildCandidateBase(TestCaseWithFactory):
@@ -188,6 +413,49 @@
             builder.manual = False
 
 
+class TestFindBuildCandidateGeneralCases(TestFindBuildCandidateBase):
+    # Test usage of findBuildCandidate not specific to any archive type.
+
+    def test_findBuildCandidate_supersedes_builds(self):
+        # IBuilder._findBuildCandidate identifies if there are builds
+        # for superseded source package releases in the queue and marks
+        # the corresponding build record as SUPERSEDED.
+        archive = self.factory.makeArchive()
+        self.publisher.getPubSource(
+            sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
+            archive=archive).createMissingBuilds()
+        old_candidate = removeSecurityProxy(
+            self.frog_builder)._findBuildCandidate()
+
+        # The candidate starts off as NEEDSBUILD:
+        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(
+            old_candidate)
+        self.assertEqual(BuildStatus.NEEDSBUILD, build.status)
+
+        # Now supersede the source package:
+        publication = build.current_source_publication
+        publication.status = PackagePublishingStatus.SUPERSEDED
+
+        # The candidate returned is now a different one:
+        new_candidate = removeSecurityProxy(
+            self.frog_builder)._findBuildCandidate()
+        self.assertNotEqual(new_candidate, old_candidate)
+
+        # And the old_candidate is superseded:
+        self.assertEqual(BuildStatus.SUPERSEDED, build.status)
+
+    def test_acquireBuildCandidate_marks_building(self):
+        # acquireBuildCandidate() should call _findBuildCandidate and
+        # mark the build as building.
+        archive = self.factory.makeArchive()
+        self.publisher.getPubSource(
+            sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
+            archive=archive).createMissingBuilds()
+        candidate = removeSecurityProxy(
+            self.frog_builder).acquireBuildCandidate()
+        self.assertEqual(JobStatus.RUNNING, candidate.job.status)
+
+
 class TestFindBuildCandidatePPAWithSingleBuilder(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
@@ -320,6 +588,16 @@
         build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(next_job)
         self.failUnlessEqual('joesppa', build.archive.name)
 
+    def test_findBuildCandidate_with_disabled_archive(self):
+        # Disabled archives should not be considered for dispatching
+        # builds.
+        disabled_job = removeSecurityProxy(self.builder4)._findBuildCandidate()
+        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(
+            disabled_job)
+        build.archive.disable()
+        next_job = removeSecurityProxy(self.builder4)._findBuildCandidate()
+        self.assertNotEqual(disabled_job, next_job)
+
 
 class TestFindBuildCandidatePrivatePPA(TestFindBuildCandidatePPABase):
 
@@ -332,6 +610,14 @@
         build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(next_job)
         self.failUnlessEqual('joesppa', build.archive.name)
 
+        # If the source for the build is still pending, it won't be
+        # dispatched because the builder has to fetch the source files
+        # from the (password protected) repo area, not the librarian.
+        pub = build.current_source_publication
+        pub.status = PackagePublishingStatus.PENDING
+        candidate = removeSecurityProxy(self.builder4)._findBuildCandidate()
+        self.assertNotEqual(next_job.id, candidate.id)
+
 
 class TestFindBuildCandidateDistroArchive(TestFindBuildCandidateBase):
 
@@ -474,97 +760,48 @@
             self.builder.current_build_behavior, BinaryPackageBuildBehavior)
 
 
-class TestSlave(TestCase):
+class TestSlave(TrialTestCase):
     """
     Integration tests for BuilderSlave that verify how it works against a
     real slave server.
     """
 
+    layer = TwistedLayer
+
+    def setUp(self):
+        super(TestSlave, self).setUp()
+        self.slave_helper = SlaveTestHelpers()
+        self.slave_helper.setUp()
+        self.addCleanup(self.slave_helper.cleanUp)
+
     # 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()
-        self.addCleanup(tachandler.tearDown)
-        def addLogFile(exc_info):
-            self.addDetail(
-                'xmlrpc-log-file',
-                Content(UTF8_TEXT, lambda: open(tachandler.logfile, 'r').read()))
-        self.addOnException(addLogFile)
-        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.makeBlockingSlave(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'})
-
     # XXX 2010-10-06 Julian bug=655559
     # This is failing on buildbot but not locally; it's trying to abort
     # before the build has started.
     def disabled_test_abort(self):
-        slave = self.getClientSlave()
+        slave = self.slave_helper.getClientSlave()
         # We need to be in a BUILDING state before we can abort.
-        self.triggerGoodBuild(slave)
-        result = slave.abort()
-        self.assertEqual(result, BuilderStatus.ABORTING)
+        d = self.slave_helper.triggerGoodBuild(slave)
+        d.addCallback(lambda ignored: slave.abort())
+        d.addCallback(self.assertEqual, BuilderStatus.ABORTING)
+        return d
 
     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)
+        slave = self.slave_helper.getClientSlave()
+        d = self.slave_helper.triggerGoodBuild(slave, build_id)
+        return d.addCallback(
+            self.assertEqual, [BuilderStatus.BUILDING, build_id])
 
     def test_clean(self):
-        slave = self.getClientSlave()
+        slave = self.slave_helper.getClientSlave()
         # XXX: JonathanLange 2010-09-21: Calling clean() on the slave requires
         # it to be in either the WAITING or ABORTED states, and both of these
         # states are very difficult to achieve in a test environment. For the
@@ -574,57 +811,248 @@
     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)
+        self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+        d = slave.echo('foo', 'bar', 42)
+        return d.addCallback(self.assertEqual, ['foo', 'bar', 42])
 
     def test_info(self):
         # Calling 'info' gets some information about the slave.
-        self.getServerSlave()
-        slave = self.getClientSlave()
-        result = slave.info()
+        self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+        d = 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(
+        return d.addCallback(
+            self.assertEqual,
             ['1.0',
              'i386',
              ['sourcepackagerecipe',
-              'translation-templates', 'binarypackage', 'debian']],
-            result)
+              'translation-templates', 'binarypackage', 'debian']])
 
     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)
+        self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+        d = slave.status()
+        return d.addCallback(self.assertEqual, [BuilderStatus.IDLE, ''])
 
     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()
+        slave = self.slave_helper.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)
+        d = self.slave_helper.triggerGoodBuild(slave, build_id)
+        d.addCallback(lambda ignored: slave.status())
+        def check_status(status):
+            self.assertEqual([BuilderStatus.BUILDING, build_id], status[:2])
+            [log_file] = status[2:]
+            self.assertIsInstance(log_file, xmlrpclib.Binary)
+        return d.addCallback(check_status)
 
     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)
+        self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+        d = slave.ensurepresent('blahblah', None, None, None)
+        d.addCallback(self.assertEqual, [False, 'No URL'])
+        return d
 
     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)
+        tachandler = self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+        self.slave_helper.makeCacheFile(tachandler, 'blahblah')
+        d = slave.ensurepresent('blahblah', None, None, None)
+        d.addCallback(self.assertEqual, [True, 'No URL'])
+        return d
+
+    def test_sendFileToSlave_not_there(self):
+        self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+        d = slave.sendFileToSlave('blahblah', None, None, None)
+        return self.assertFailure(d, CannotFetchFile)
+
+    def test_sendFileToSlave_actually_there(self):
+        tachandler = self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+        self.slave_helper.makeCacheFile(tachandler, 'blahblah')
+        d = slave.sendFileToSlave('blahblah', None, None, None)
+        def check_present(ignored):
+            d = slave.ensurepresent('blahblah', None, None, None)
+            return d.addCallback(self.assertEqual, [True, 'No URL'])
+        d.addCallback(check_present)
+        return d
+
+    def test_resumeHost_success(self):
+        # On a successful resume resume() fires the returned deferred
+        # callback with 'None'.
+        self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+
+        # The configuration testing command-line.
+        self.assertEqual(
+            'echo %(vm_host)s', config.builddmaster.vm_resume_command)
+
+        # On success the response is None.
+        def check_resume_success(response):
+            out, err, code = response
+            self.assertEqual(os.EX_OK, code)
+            # XXX: JonathanLange 2010-09-23: We should instead pass the
+            # expected vm_host into the client slave. Not doing this now,
+            # since the SlaveHelper is being moved around.
+            self.assertEqual("%s\n" % slave._vm_host, out)
+        d = slave.resume()
+        d.addBoth(check_resume_success)
+        return d
+
+    def test_resumeHost_failure(self):
+        # On a failed resume, 'resumeHost' fires the returned deferred
+        # errorback with the `ProcessTerminated` failure.
+        self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+
+        # Override the configuration command-line with one that will fail.
+        failed_config = """
+        [builddmaster]
+        vm_resume_command: test "%(vm_host)s = 'no-sir'"
+        """
+        config.push('failed_resume_command', failed_config)
+        self.addCleanup(config.pop, 'failed_resume_command')
+
+        # On failures, the response is a twisted `Failure` object containing
+        # a tuple.
+        def check_resume_failure(failure):
+            out, err, code = failure.value
+            # The process will exit with a return code of "1".
+            self.assertEqual(code, 1)
+        d = slave.resume()
+        d.addBoth(check_resume_failure)
+        return d
+
+    def test_resumeHost_timeout(self):
+        # On a resume timeouts, 'resumeHost' fires the returned deferred
+        # errorback with the `TimeoutError` failure.
+        self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+
+        # Override the configuration command-line with one that will timeout.
+        timeout_config = """
+        [builddmaster]
+        vm_resume_command: sleep 5
+        socket_timeout: 1
+        """
+        config.push('timeout_resume_command', timeout_config)
+        self.addCleanup(config.pop, 'timeout_resume_command')
+
+        # On timeouts, the response is a twisted `Failure` object containing
+        # a `TimeoutError` error.
+        def check_resume_timeout(failure):
+            self.assertIsInstance(failure, Failure)
+            out, err, code = failure.value
+            self.assertEqual(code, signal.SIGKILL)
+        clock = Clock()
+        d = slave.resume(clock=clock)
+        # Move the clock beyond the socket_timeout but earlier than the
+        # sleep 5.  This stops the test having to wait for the timeout.
+        # Fast tests FTW!
+        clock.advance(2)
+        d.addBoth(check_resume_timeout)
+        return d
+
+
+class TestSlaveTimeouts(TrialTestCase):
+    # Testing that the methods that call callRemote() all time out
+    # as required.
+
+    layer = TwistedLayer
+
+    def setUp(self):
+        super(TestSlaveTimeouts, self).setUp()
+        self.slave_helper = SlaveTestHelpers()
+        self.slave_helper.setUp()
+        self.addCleanup(self.slave_helper.cleanUp)
+        self.clock = Clock()
+        self.proxy = DeadProxy("url")
+        self.slave = self.slave_helper.getClientSlave(
+            reactor=self.clock, proxy=self.proxy)
+
+    def assertCancelled(self, d):
+        self.clock.advance(config.builddmaster.socket_timeout + 1)
+        return self.assertFailure(d, CancelledError)
+
+    def test_timeout_abort(self):
+        return self.assertCancelled(self.slave.abort())
+
+    def test_timeout_clean(self):
+        return self.assertCancelled(self.slave.clean())
+
+    def test_timeout_echo(self):
+        return self.assertCancelled(self.slave.echo())
+
+    def test_timeout_info(self):
+        return self.assertCancelled(self.slave.info())
+
+    def test_timeout_status(self):
+        return self.assertCancelled(self.slave.status())
+
+    def test_timeout_ensurepresent(self):
+        return self.assertCancelled(
+            self.slave.ensurepresent(None, None, None, None))
+
+    def test_timeout_build(self):
+        return self.assertCancelled(
+            self.slave.build(None, None, None, None, None))
+
+
+class TestSlaveWithLibrarian(TrialTestCase):
+    """Tests that need more of Launchpad to run."""
+
+    layer = TwistedLaunchpadZopelessLayer
+
+    def setUp(self):
+        super(TestSlaveWithLibrarian, self)
+        self.slave_helper = SlaveTestHelpers()
+        self.slave_helper.setUp()
+        self.addCleanup(self.slave_helper.cleanUp)
+        self.factory = LaunchpadObjectFactory()
+        login_as(ANONYMOUS)
+        self.addCleanup(logout)
+
+    def test_ensurepresent_librarian(self):
+        # ensurepresent, when given an http URL for a file will download the
+        # file from that URL and report that the file is present, and it was
+        # downloaded.
+
+        # Use the Librarian because it's a "convenient" web server.
+        lf = self.factory.makeLibraryFileAlias(
+            'HelloWorld.txt', content="Hello World")
+        self.layer.txn.commit()
+        self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+        d = slave.ensurepresent(
+            lf.content.sha1, lf.http_url, "", "")
+        d.addCallback(self.assertEqual, [True, 'Download'])
+        return d
+
+    def test_retrieve_files_from_filecache(self):
+        # Files that are present on the slave can be downloaded with a
+        # filename made from the sha1 of the content underneath the
+        # 'filecache' directory.
+        content = "Hello World"
+        lf = self.factory.makeLibraryFileAlias(
+            'HelloWorld.txt', content=content)
+        self.layer.txn.commit()
+        expected_url = '%s/filecache/%s' % (
+            self.slave_helper.BASE_URL, lf.content.sha1)
+        self.slave_helper.getServerSlave()
+        slave = self.slave_helper.getClientSlave()
+        d = slave.ensurepresent(
+            lf.content.sha1, lf.http_url, "", "")
+        def check_file(ignored):
+            d = getPage(expected_url.encode('utf8'))
+            return d.addCallback(self.assertEqual, content)
+        return d.addCallback(check_file)
 
=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2010-09-28 11:05:14 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2010-10-25 19:14:01 +0000
@@ -6,6 +6,7 @@
 import os
 import signal
 import time
+import xmlrpclib
 
 import transaction
 
@@ -14,9 +15,7 @@
     reactor,
     task,
     )
-from twisted.internet.error import ConnectionClosed
 from twisted.internet.task import (
-    Clock,
     deferLater,
     )
 from twisted.python.failure import Failure
@@ -30,577 +29,45 @@
     ANONYMOUS,
     login,
     )
-from canonical.launchpad.scripts.logger import BufferLogger
+from canonical.launchpad.scripts.logger import (
+    QuietFakeLogger,
+    )
 from canonical.testing.layers import (
     LaunchpadScriptLayer,
-    LaunchpadZopelessLayer,
+    TwistedLaunchpadZopelessLayer,
     TwistedLayer,
+    ZopelessDatabaseLayer,
     )
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
 from lp.buildmaster.manager import (
-    BaseDispatchResult,
-    buildd_success_result_map,
+    assessFailureCounts,
     BuilddManager,
-    FailDispatchResult,
     NewBuildersScanner,
-    RecordingSlave,
-    ResetDispatchResult,
     SlaveScanner,
     )
+from lp.buildmaster.model.builder import Builder
 from lp.buildmaster.tests.harness import BuilddManagerTestSetup
-from lp.buildmaster.tests.mock_slaves import BuildingSlave
+from lp.buildmaster.tests.mock_slaves import (
+    BrokenSlave,
+    BuildingSlave,
+    OkSlave,
+    )
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
-from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
-from lp.testing import TestCase as LaunchpadTestCase
+from lp.testing import TestCaseWithFactory
 from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.fakemethod import FakeMethod
 from lp.testing.sampledata import BOB_THE_BUILDER_NAME
 
 
-class TestRecordingSlaves(TrialTestCase):
-    """Tests for the recording slave class."""
-    layer = TwistedLayer
-
-    def setUp(self):
-        """Setup a fresh `RecordingSlave` for tests."""
-        TrialTestCase.setUp(self)
-        self.slave = RecordingSlave(
-            'foo', 'http://foo:8221/rpc', 'foo.host')
-
-    def test_representation(self):
-        """`RecordingSlave` has a custom representation.
-
-        It encloses builder name and xmlrpc url for debug purposes.
-        """
-        self.assertEqual('<foo:http://foo:8221/rpc>', repr(self.slave))
-
-    def assert_ensurepresent(self, func):
-        """Helper function to test results from calling ensurepresent."""
-        self.assertEqual(
-            [True, 'Download'],
-            func('boing', 'bar', 'baz'))
-        self.assertEqual(
-            [('ensurepresent', ('boing', 'bar', 'baz'))],
-            self.slave.calls)
-
-    def test_ensurepresent(self):
-        """`RecordingSlave.ensurepresent` always succeeds.
-
-        It returns the expected succeed code and records the interaction
-        information for later use.
-        """
-        self.assert_ensurepresent(self.slave.ensurepresent)
-
-    def test_sendFileToSlave(self):
-        """RecordingSlave.sendFileToSlave always succeeeds.
-
-        It calls ensurepresent() and hence returns the same results.
-        """
-        self.assert_ensurepresent(self.slave.sendFileToSlave)
-
-    def test_build(self):
-        """`RecordingSlave.build` always succeeds.
-
-        It returns the expected succeed code and records the interaction
-        information for later use.
-        """
-        self.assertEqual(
-            ['BuilderStatus.BUILDING', 'boing'],
-            self.slave.build('boing', 'bar', 'baz'))
-        self.assertEqual(
-            [('build', ('boing', 'bar', 'baz'))],
-            self.slave.calls)
-
-    def test_resume(self):
-        """`RecordingSlave.resume` always returns successs."""
-        # Resume isn't requested in a just-instantiated RecordingSlave.
-        self.assertFalse(self.slave.resume_requested)
-
-        # When resume is called, it returns the success list and mark
-        # the slave for resuming.
-        self.assertEqual(['', '', os.EX_OK], self.slave.resume())
-        self.assertTrue(self.slave.resume_requested)
-
-    def test_resumeHost_success(self):
-        # On a successful resume resumeHost() fires the returned deferred
-        # callback with 'None'.
-
-        # The configuration testing command-line.
-        self.assertEqual(
-            'echo %(vm_host)s', config.builddmaster.vm_resume_command)
-
-        # On success the response is None.
-        def check_resume_success(response):
-            out, err, code = response
-            self.assertEqual(os.EX_OK, code)
-            self.assertEqual("%s\n" % self.slave.vm_host, out)
-        d = self.slave.resumeSlave()
-        d.addBoth(check_resume_success)
-        return d
-
-    def test_resumeHost_failure(self):
-        # On a failed resume, 'resumeHost' fires the returned deferred
-        # errorback with the `ProcessTerminated` failure.
-
-        # Override the configuration command-line with one that will fail.
-        failed_config = """
-        [builddmaster]
-        vm_resume_command: test "%(vm_host)s = 'no-sir'"
-        """
-        config.push('failed_resume_command', failed_config)
-        self.addCleanup(config.pop, 'failed_resume_command')
-
-        # On failures, the response is a twisted `Failure` object containing
-        # a tuple.
-        def check_resume_failure(failure):
-            out, err, code = failure.value
-            # The process will exit with a return code of "1".
-            self.assertEqual(code, 1)
-        d = self.slave.resumeSlave()
-        d.addBoth(check_resume_failure)
-        return d
-
-    def test_resumeHost_timeout(self):
-        # On a resume timeouts, 'resumeHost' fires the returned deferred
-        # errorback with the `TimeoutError` failure.
-
-        # Override the configuration command-line with one that will timeout.
-        timeout_config = """
-        [builddmaster]
-        vm_resume_command: sleep 5
-        socket_timeout: 1
-        """
-        config.push('timeout_resume_command', timeout_config)
-        self.addCleanup(config.pop, 'timeout_resume_command')
-
-        # On timeouts, the response is a twisted `Failure` object containing
-        # a `TimeoutError` error.
-        def check_resume_timeout(failure):
-            self.assertIsInstance(failure, Failure)
-            out, err, code = failure.value
-            self.assertEqual(code, signal.SIGKILL)
-        clock = Clock()
-        d = self.slave.resumeSlave(clock=clock)
-        # Move the clock beyond the socket_timeout but earlier than the
-        # sleep 5.  This stops the test having to wait for the timeout.
-        # Fast tests FTW!
-        clock.advance(2)
-        d.addBoth(check_resume_timeout)
-        return d
-
-
-class TestingXMLRPCProxy:
-    """This class mimics a twisted XMLRPC Proxy class."""
-
-    def __init__(self, failure_info=None):
-        self.calls = []
-        self.failure_info = failure_info
-        self.works = failure_info is None
-
-    def callRemote(self, *args):
-        self.calls.append(args)
-        if self.works:
-            result = buildd_success_result_map.get(args[0])
-        else:
-            result = 'boing'
-        return defer.succeed([result, self.failure_info])
-
-
-class TestingResetDispatchResult(ResetDispatchResult):
-    """Override the evaluation method to simply annotate the call."""
-
-    def __init__(self, slave, info=None):
-        ResetDispatchResult.__init__(self, slave, info)
-        self.processed = False
-
-    def __call__(self):
-        self.processed = True
-
-
-class TestingFailDispatchResult(FailDispatchResult):
-    """Override the evaluation method to simply annotate the call."""
-
-    def __init__(self, slave, info=None):
-        FailDispatchResult.__init__(self, slave, info)
-        self.processed = False
-
-    def __call__(self):
-        self.processed = True
-
-
-class TestingSlaveScanner(SlaveScanner):
-    """Override the dispatch result factories """
-
-    reset_result = TestingResetDispatchResult
-    fail_result = TestingFailDispatchResult
-
-
-class TestSlaveScanner(TrialTestCase):
-    """Tests for the actual build slave manager."""
-    layer = LaunchpadZopelessLayer
-
-    def setUp(self):
-        TrialTestCase.setUp(self)
-        self.manager = TestingSlaveScanner(
-            BOB_THE_BUILDER_NAME, BufferLogger())
-
-        self.fake_builder_url = 'http://bob.buildd:8221/'
-        self.fake_builder_host = 'bob.host'
-
-        # We will use an instrumented SlaveScanner instance for tests in
-        # this context.
-
-        # Stop cyclic execution and record the end of the cycle.
-        self.stopped = False
-
-        def testNextCycle():
-            self.stopped = True
-
-        self.manager.scheduleNextScanCycle = testNextCycle
-
-        # Return the testing Proxy version.
-        self.test_proxy = TestingXMLRPCProxy()
-
-        def testGetProxyForSlave(slave):
-            return self.test_proxy
-        self.manager._getProxyForSlave = testGetProxyForSlave
-
-        # Deactivate the 'scan' method.
-        def testScan():
-            pass
-        self.manager.scan = testScan
-
-        # Stop automatic collection of dispatching results.
-        def testslaveConversationEnded():
-            pass
-        self._realslaveConversationEnded = self.manager.slaveConversationEnded
-        self.manager.slaveConversationEnded = testslaveConversationEnded
-
-    def assertIsDispatchReset(self, result):
-        self.assertTrue(
-            isinstance(result, TestingResetDispatchResult),
-            'Dispatch failure did not result in a ResetBuildResult object')
-
-    def assertIsDispatchFail(self, result):
-        self.assertTrue(
-            isinstance(result, TestingFailDispatchResult),
-            'Dispatch failure did not result in a FailBuildResult object')
-
-    def test_checkResume(self):
-        """`SlaveScanner.checkResume` is chained after resume requests.
-
-        If the resume request succeed it returns None, otherwise it returns
-        a `ResetBuildResult` (the one in the test context) that will be
-        collect and evaluated later.
-
-        See `RecordingSlave.resumeHost` for more information about the resume
-        result contents.
-        """
-        slave = RecordingSlave('foo', 'http://foo.buildd:8221/', 'foo.host')
-
-        successful_response = ['', '', os.EX_OK]
-        result = self.manager.checkResume(successful_response, slave)
-        self.assertEqual(
-            None, result, 'Successful resume checks should return None')
-
-        failed_response = ['stdout', 'stderr', 1]
-        result = self.manager.checkResume(failed_response, slave)
-        self.assertIsDispatchReset(result)
-        self.assertEqual(
-            '<foo:http://foo.buildd:8221/> reset failure', repr(result))
-        self.assertEqual(
-            result.info, "stdout\nstderr")
-
-    def test_fail_to_resume_slave_resets_slave(self):
-        # If an attempt to resume and dispatch a slave fails, we reset the
-        # slave by calling self.reset_result(slave)().
-
-        reset_result_calls = []
-
-        class LoggingResetResult(BaseDispatchResult):
-            """A DispatchResult that logs calls to itself.
-
-            This *must* subclass BaseDispatchResult, otherwise finishCycle()
-            won't treat it like a dispatch result.
-            """
-
-            def __init__(self, slave, info=None):
-                self.slave = slave
-
-            def __call__(self):
-                reset_result_calls.append(self.slave)
-
-        # Make a failing slave that is requesting a resume.
-        slave = RecordingSlave('foo', 'http://foo.buildd:8221/', 'foo.host')
-        slave.resume_requested = True
-        slave.resumeSlave = lambda: deferLater(
-            reactor, 0, defer.fail, Failure(('out', 'err', 1)))
-
-        # Make the manager log the reset result calls.
-        self.manager.reset_result = LoggingResetResult
-
-        # We only care about this one slave. Reset the list of manager
-        # deferreds in case setUp did something unexpected.
-        self.manager._deferred_list = []
-
-        # Here, we're patching the slaveConversationEnded method so we can
-        # get an extra callback at the end of it, so we can
-        # verify that the reset_result was really called.
-        def _slaveConversationEnded():
-            d = self._realslaveConversationEnded()
-            return d.addCallback(
-                lambda ignored: self.assertEqual([slave], reset_result_calls))
-        self.manager.slaveConversationEnded = _slaveConversationEnded
-
-        self.manager.resumeAndDispatch(slave)
-
-    def test_failed_to_resume_slave_ready_for_reset(self):
-        # When a slave fails to resume, the manager has a Deferred in its
-        # Deferred list that is ready to fire with a ResetDispatchResult.
-
-        # Make a failing slave that is requesting a resume.
-        slave = RecordingSlave('foo', 'http://foo.buildd:8221/', 'foo.host')
-        slave.resume_requested = True
-        slave.resumeSlave = lambda: defer.fail(Failure(('out', 'err', 1)))
-
-        # We only care about this one slave. Reset the list of manager
-        # deferreds in case setUp did something unexpected.
-        self.manager._deferred_list = []
-        # Restore the slaveConversationEnded method. It's very relevant to
-        # this test.
-        self.manager.slaveConversationEnded = self._realslaveConversationEnded
-        self.manager.resumeAndDispatch(slave)
-        [d] = self.manager._deferred_list
-
-        # The Deferred for our failing slave should be ready to fire
-        # successfully with a ResetDispatchResult.
-        def check_result(result):
-            self.assertIsInstance(result, ResetDispatchResult)
-            self.assertEqual(slave, result.slave)
-            self.assertFalse(result.processed)
-        return d.addCallback(check_result)
-
-    def _setUpSlaveAndBuilder(self, builder_failure_count=None,
-                              job_failure_count=None):
-        # Helper function to set up a builder and its recording slave.
-        if builder_failure_count is None:
-            builder_failure_count = 0
-        if job_failure_count is None:
-            job_failure_count = 0
-        slave = RecordingSlave(
-            BOB_THE_BUILDER_NAME, self.fake_builder_url,
-            self.fake_builder_host)
-        bob_builder = getUtility(IBuilderSet)[slave.name]
-        bob_builder.failure_count = builder_failure_count
-        bob_builder.getCurrentBuildFarmJob().failure_count = job_failure_count
-        return slave, bob_builder
-
-    def test_checkDispatch_success(self):
-        # SlaveScanner.checkDispatch returns None for a successful
-        # dispatch.
-
-        """
-        If the dispatch request fails or a unknown method is given, it
-        returns a `FailDispatchResult` (in the test context) that will
-        be evaluated later.
-
-        Builders will be marked as failed if the following responses
-        categories are received.
-
-         * Legitimate slave failures: when the response is a list with 2
-           elements but the first element ('status') does not correspond to
-           the expected 'success' result. See `buildd_success_result_map`.
-
-         * Unexpected (code) failures: when the given 'method' is unknown
-           or the response isn't a 2-element list or Failure instance.
-
-        Communication failures (a twisted `Failure` instance) will simply
-        cause the builder to be reset, a `ResetDispatchResult` object is
-        returned. In other words, network failures are ignored in this
-        stage, broken builders will be identified and marked as so
-        during 'scan()' stage.
-
-        On success dispatching it returns None.
-        """
-        slave, bob_builder = self._setUpSlaveAndBuilder(
-            builder_failure_count=0, job_failure_count=0)
-
-        # Successful legitimate response, None is returned.
-        successful_response = [
-            buildd_success_result_map.get('ensurepresent'), 'cool builder']
-        result = self.manager.checkDispatch(
-            successful_response, 'ensurepresent', slave)
-        self.assertEqual(
-            None, result, 'Successful dispatch checks should return None')
-
-    def test_checkDispatch_first_fail(self):
-        # Failed legitimate response, results in FailDispatchResult and
-        # failure_count on the job and the builder are both incremented.
-        slave, bob_builder = self._setUpSlaveAndBuilder(
-            builder_failure_count=0, job_failure_count=0)
-
-        failed_response = [False, 'uncool builder']
-        result = self.manager.checkDispatch(
-            failed_response, 'ensurepresent', slave)
-        self.assertIsDispatchFail(result)
-        self.assertEqual(
-            repr(result),
-            '<bob:%s> failure (uncool builder)' % self.fake_builder_url)
-        self.assertEqual(1, bob_builder.failure_count)
-        self.assertEqual(
-            1, bob_builder.getCurrentBuildFarmJob().failure_count)
-
-    def test_checkDispatch_second_reset_fail_by_builder(self):
-        # Twisted Failure response, results in a `FailDispatchResult`.
-        slave, bob_builder = self._setUpSlaveAndBuilder(
-            builder_failure_count=1, job_failure_count=0)
-
-        twisted_failure = Failure(ConnectionClosed('Boom!'))
-        result = self.manager.checkDispatch(
-            twisted_failure, 'ensurepresent', slave)
-        self.assertIsDispatchFail(result)
-        self.assertEqual(
-            '<bob:%s> failure (None)' % self.fake_builder_url, repr(result))
-        self.assertEqual(2, bob_builder.failure_count)
-        self.assertEqual(
-            1, bob_builder.getCurrentBuildFarmJob().failure_count)
-
-    def test_checkDispatch_second_comms_fail_by_builder(self):
-        # Unexpected response, results in a `FailDispatchResult`.
-        slave, bob_builder = self._setUpSlaveAndBuilder(
-            builder_failure_count=1, job_failure_count=0)
-
-        unexpected_response = [1, 2, 3]
-        result = self.manager.checkDispatch(
-            unexpected_response, 'build', slave)
-        self.assertIsDispatchFail(result)
-        self.assertEqual(
-            '<bob:%s> failure '
-            '(Unexpected response: [1, 2, 3])' % self.fake_builder_url,
-            repr(result))
-        self.assertEqual(2, bob_builder.failure_count)
-        self.assertEqual(
-            1, bob_builder.getCurrentBuildFarmJob().failure_count)
-
-    def test_checkDispatch_second_comms_fail_by_job(self):
-        # Unknown method was given, results in a `FailDispatchResult`.
-        # This could be caused by a faulty job which would fail the job.
-        slave, bob_builder = self._setUpSlaveAndBuilder(
-            builder_failure_count=0, job_failure_count=1)
-
-        successful_response = [
-            buildd_success_result_map.get('ensurepresent'), 'cool builder']
-        result = self.manager.checkDispatch(
-            successful_response, 'unknown-method', slave)
-        self.assertIsDispatchFail(result)
-        self.assertEqual(
-            '<bob:%s> failure '
-            '(Unknown slave method: unknown-method)' % self.fake_builder_url,
-            repr(result))
-        self.assertEqual(1, bob_builder.failure_count)
-        self.assertEqual(
-            2, bob_builder.getCurrentBuildFarmJob().failure_count)
-
-    def test_initiateDispatch(self):
-        """Check `dispatchBuild` in various scenarios.
-
-        When there are no recording slaves (i.e. no build got dispatched
-        in scan()) it simply finishes the cycle.
-
-        When there is a recording slave with pending slave calls, they are
-        performed and if they all succeed the cycle is finished with no
-        errors.
-
-        On slave call failure the chain is stopped immediately and an
-        FailDispatchResult is collected while finishing the cycle.
-        """
-        def check_no_events(results):
-            errors = [
-                r for s, r in results if isinstance(r, BaseDispatchResult)]
-            self.assertEqual(0, len(errors))
-
-        def check_events(results):
-            [error] = [r for s, r in results if r is not None]
-            self.assertEqual(
-                '<bob:%s> failure (very broken slave)'
-                    % self.fake_builder_url,
-                repr(error))
-            self.assertTrue(error.processed)
-
-        def _wait_on_deferreds_then_check_no_events():
-            dl = self._realslaveConversationEnded()
-            dl.addCallback(check_no_events)
-
-        def _wait_on_deferreds_then_check_events():
-            dl = self._realslaveConversationEnded()
-            dl.addCallback(check_events)
-
-        # A functional slave charged with some interactions.
-        slave = RecordingSlave(
-            BOB_THE_BUILDER_NAME, self.fake_builder_url,
-            self.fake_builder_host)
-        slave.ensurepresent('arg1', 'arg2', 'arg3')
-        slave.build('arg1', 'arg2', 'arg3')
-
-        # If the previous step (resuming) has failed nothing gets dispatched.
-        reset_result = ResetDispatchResult(slave)
-        result = self.manager.initiateDispatch(reset_result, slave)
-        self.assertTrue(result is reset_result)
-        self.assertFalse(slave.resume_requested)
-        self.assertEqual(0, len(self.manager._deferred_list))
-
-        # Operation with the default (funcional slave), no resets or
-        # failures results are triggered.
-        slave.resume()
-        result = self.manager.initiateDispatch(None, slave)
-        self.assertEqual(None, result)
-        self.assertTrue(slave.resume_requested)
-        self.assertEqual(
-            [('ensurepresent', 'arg1', 'arg2', 'arg3'),
-             ('build', 'arg1', 'arg2', 'arg3')],
-            self.test_proxy.calls)
-        self.assertEqual(2, len(self.manager._deferred_list))
-
-        # Monkey patch the slaveConversationEnded method so we can chain a
-        # callback to check the end of the result chain.
-        self.manager.slaveConversationEnded = \
-            _wait_on_deferreds_then_check_no_events
-        events = self.manager.slaveConversationEnded()
-
-        # Create a broken slave and insert interaction that will
-        # cause the builder to be marked as fail.
-        self.test_proxy = TestingXMLRPCProxy('very broken slave')
-        slave = RecordingSlave(
-            BOB_THE_BUILDER_NAME, self.fake_builder_url,
-            self.fake_builder_host)
-        slave.ensurepresent('arg1', 'arg2', 'arg3')
-        slave.build('arg1', 'arg2', 'arg3')
-
-        result = self.manager.initiateDispatch(None, slave)
-        self.assertEqual(None, result)
-        self.assertEqual(3, len(self.manager._deferred_list))
-        self.assertEqual(
-            [('ensurepresent', 'arg1', 'arg2', 'arg3')],
-            self.test_proxy.calls)
-
-        # Monkey patch the slaveConversationEnded method so we can chain a
-        # callback to check the end of the result chain.
-        self.manager.slaveConversationEnded = \
-            _wait_on_deferreds_then_check_events
-        events = self.manager.slaveConversationEnded()
-
-        return events
-
-
 class TestSlaveScannerScan(TrialTestCase):
     """Tests `SlaveScanner.scan` method.
 
     This method uses the old framework for scanning and dispatching builds.
     """
-    layer = LaunchpadZopelessLayer
+    layer = TwistedLaunchpadZopelessLayer
 
     def setUp(self):
         """Setup TwistedLayer, TrialTestCase and BuilddSlaveTest.
@@ -608,19 +75,18 @@
         Also adjust the sampledata in a way a build can be dispatched to
         'bob' builder.
         """
+        from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
         TwistedLayer.testSetUp()
         TrialTestCase.setUp(self)
         self.slave = BuilddSlaveTestSetup()
         self.slave.setUp()
 
         # Creating the required chroots needed for dispatching.
-        login('[e-mail]')
         test_publisher = SoyuzTestPublisher()
         ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
         hoary = ubuntu.getSeries('hoary')
         test_publisher.setUpDefaultDistroSeries(hoary)
         test_publisher.addFakeChroots()
-        login(ANONYMOUS)
 
     def tearDown(self):
         self.slave.tearDown()
@@ -628,8 +94,7 @@
         TwistedLayer.testTearDown()
 
     def _resetBuilder(self, builder):
-        """Reset the given builder and it's job."""
-        login('[e-mail]')
+        """Reset the given builder and its job."""
 
         builder.builderok = True
         job = builder.currentjob
@@ -637,7 +102,6 @@
             job.reset()
 
         transaction.commit()
-        login(ANONYMOUS)
 
     def assertBuildingJob(self, job, builder, logtail=None):
         """Assert the given job is building on the given builder."""
@@ -653,55 +117,25 @@
         self.assertEqual(build.status, BuildStatus.BUILDING)
         self.assertEqual(job.logtail, logtail)
 
-    def _getManager(self):
+    def _getScanner(self, builder_name=None):
         """Instantiate a SlaveScanner object.
 
         Replace its default logging handler by a testing version.
         """
-        manager = SlaveScanner(BOB_THE_BUILDER_NAME, BufferLogger())
-        manager.logger.name = 'slave-scanner'
+        if builder_name is None:
+            builder_name = BOB_THE_BUILDER_NAME
+        scanner = SlaveScanner(builder_name, QuietFakeLogger())
+        scanner.logger.name = 'slave-scanner'
 
-        return manager
+        return scanner
 
     def _checkDispatch(self, slave, builder):
-        """`SlaveScanner.scan` returns a `RecordingSlave`.
-
-        The single slave returned should match the given builder and
-        contain interactions that should be performed asynchronously for
-        properly dispatching the sampledata job.
-        """
-        self.assertFalse(
-            slave is None, "Unexpected recording_slaves.")
-
-        self.assertEqual(slave.name, builder.name)
-        self.assertEqual(slave.url, builder.url)
-        self.assertEqual(slave.vm_host, builder.vm_host)
+        # SlaveScanner.scan returns a slave when a dispatch was
+        # successful.  We also check that the builder has a job on it.
+
+        self.assertTrue(slave is not None, "Expected a slave.")
         self.assertEqual(0, builder.failure_count)
-
-        self.assertEqual(
-            [('ensurepresent',
-              ('0feca720e2c29dafb2c900713ba560e03b758711',
-               'http://localhost:58000/93/fake_chroot.tar.gz',
-               '', '')),
-             ('ensurepresent',
-              ('4e3961baf4f56fdbc95d0dd47f3c5bc275da8a33',
-               'http://localhost:58000/43/alsa-utils_1.0.9a-4ubuntu1.dsc',
-               '', '')),
-             ('build',
-              ('6358a89e2215e19b02bf91e2e4d009640fae5cf8',
-               'binarypackage', '0feca720e2c29dafb2c900713ba560e03b758711',
-               {'alsa-utils_1.0.9a-4ubuntu1.dsc':
-                '4e3961baf4f56fdbc95d0dd47f3c5bc275da8a33'},
-               {'arch_indep': True,
-                'arch_tag': 'i386',
-                'archive_private': False,
-                'archive_purpose': 'PRIMARY',
-                'archives':
-                ['deb http://ftpmaster.internal/ubuntu hoary main'],
-                'build_debug_symbols': False,
-                'ogrecomponent': 'main',
-                'suite': u'hoary'}))],
-            slave.calls, "Job was not properly dispatched.")
+        self.assertTrue(builder.currentjob is not None)
 
     def testScanDispatchForResetBuilder(self):
         # A job gets dispatched to the sampledata builder after it's reset.
@@ -709,26 +143,27 @@
         # Reset sampledata builder.
         builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
         self._resetBuilder(builder)
+        builder.setSlaveForTesting(OkSlave())
         # Set this to 1 here so that _checkDispatch can make sure it's
         # reset to 0 after a successful dispatch.
         builder.failure_count = 1
 
         # Run 'scan' and check its result.
-        LaunchpadZopelessLayer.switchDbUser(config.builddmaster.dbuser)
-        manager = self._getManager()
-        d = defer.maybeDeferred(manager.scan)
+        self.layer.txn.commit()
+        self.layer.switchDbUser(config.builddmaster.dbuser)
+        scanner = self._getScanner()
+        d = defer.maybeDeferred(scanner.scan)
         d.addCallback(self._checkDispatch, builder)
         return d
 
-    def _checkNoDispatch(self, recording_slave, builder):
+    def _checkNoDispatch(self, slave, builder):
         """Assert that no dispatch has occurred.
 
-        'recording_slave' is None, so no interations would be passed
+        'slave' is None, so no interations would be passed
         to the asynchonous dispatcher and the builder remained active
         and IDLE.
         """
-        self.assertTrue(
-            recording_slave is None, "Unexpected recording_slave.")
+        self.assertTrue(slave is None, "Unexpected slave.")
 
         builder = getUtility(IBuilderSet).get(builder.id)
         self.assertTrue(builder.builderok)
@@ -753,9 +188,9 @@
         login(ANONYMOUS)
 
         # Run 'scan' and check its result.
-        LaunchpadZopelessLayer.switchDbUser(config.builddmaster.dbuser)
-        manager = self._getManager()
-        d = defer.maybeDeferred(manager.scan)
+        self.layer.switchDbUser(config.builddmaster.dbuser)
+        scanner = self._getScanner()
+        d = defer.maybeDeferred(scanner.singleCycle)
         d.addCallback(self._checkNoDispatch, builder)
         return d
 
@@ -793,9 +228,9 @@
         login(ANONYMOUS)
 
         # Run 'scan' and check its result.
-        LaunchpadZopelessLayer.switchDbUser(config.builddmaster.dbuser)
-        manager = self._getManager()
-        d = defer.maybeDeferred(manager.scan)
+        self.layer.switchDbUser(config.builddmaster.dbuser)
+        scanner = self._getScanner()
+        d = defer.maybeDeferred(scanner.scan)
         d.addCallback(self._checkJobRescued, builder, job)
         return d
 
@@ -814,8 +249,6 @@
         self.assertBuildingJob(job, builder, logtail='This is a build log')
 
     def testScanUpdatesBuildingJobs(self):
-        # The job assigned to a broken builder is rescued.
-
         # Enable sampledata builder attached to an appropriate testing
         # slave. It will respond as if it was building the sampledata job.
         builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
@@ -830,188 +263,174 @@
         self.assertBuildingJob(job, builder)
 
         # Run 'scan' and check its result.
-        LaunchpadZopelessLayer.switchDbUser(config.builddmaster.dbuser)
-        manager = self._getManager()
-        d = defer.maybeDeferred(manager.scan)
+        self.layer.switchDbUser(config.builddmaster.dbuser)
+        scanner = self._getScanner()
+        d = defer.maybeDeferred(scanner.scan)
         d.addCallback(self._checkJobUpdated, builder, job)
         return d
 
-    def test_scan_assesses_failure_exceptions(self):
+    def test_scan_with_nothing_to_dispatch(self):
+        factory = LaunchpadObjectFactory()
+        builder = factory.makeBuilder()
+        builder.setSlaveForTesting(OkSlave())
+        scanner = self._getScanner(builder_name=builder.name)
+        d = scanner.scan()
+        return d.addCallback(self._checkNoDispatch, builder)
+
+    def test_scan_with_manual_builder(self):
+        # Reset sampledata builder.
+        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
+        self._resetBuilder(builder)
+        builder.setSlaveForTesting(OkSlave())
+        builder.manual = True
+        scanner = self._getScanner()
+        d = scanner.scan()
+        d.addCallback(self._checkNoDispatch, builder)
+        return d
+
+    def test_scan_with_not_ok_builder(self):
+        # Reset sampledata builder.
+        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
+        self._resetBuilder(builder)
+        builder.setSlaveForTesting(OkSlave())
+        builder.builderok = False
+        scanner = self._getScanner()
+        d = scanner.scan()
+        # Because the builder is not ok, we can't use _checkNoDispatch.
+        d.addCallback(
+            lambda ignored: self.assertIdentical(None, builder.currentjob))
+        return d
+
+    def test_scan_of_broken_slave(self):
+        builder = getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME]
+        self._resetBuilder(builder)
+        builder.setSlaveForTesting(BrokenSlave())
+        builder.failure_count = 0
+        scanner = self._getScanner(builder_name=builder.name)
+        d = scanner.scan()
+        return self.assertFailure(d, xmlrpclib.Fault)
+
+    def _assertFailureCounting(self, builder_count, job_count,
+                               expected_builder_count, expected_job_count):
         # If scan() fails with an exception, failure_counts should be
-        # incremented and tested.
+        # incremented.  What we do with the results of the failure
+        # counts is tested below separately, this test just makes sure that
+        # scan() is setting the counts.
         def failing_scan():
-            raise Exception("fake exception")
-        manager = self._getManager()
-        manager.scan = failing_scan
-        manager.scheduleNextScanCycle = FakeMethod()
+            return defer.fail(Exception("fake exception"))
+        scanner = self._getScanner()
+        scanner.scan = failing_scan
         from lp.buildmaster import manager as manager_module
         self.patch(manager_module, 'assessFailureCounts', FakeMethod())
-        builder = getUtility(IBuilderSet)[manager.builder_name]
-
-        # Failure counts start at zero.
-        self.assertEqual(0, builder.failure_count)
-        self.assertEqual(
-            0, builder.currentjob.specific_job.build.failure_count)
-
-        # startCycle() calls scan() which is our fake one that throws an
+        builder = getUtility(IBuilderSet)[scanner.builder_name]
+
+        builder.failure_count = builder_count
+        builder.currentjob.specific_job.build.failure_count = job_count
+        # The _scanFailed() calls abort, so make sure our existing
+        # failure counts are persisted.
+        self.layer.txn.commit()
+
+        # singleCycle() calls scan() which is our fake one that throws an
         # exception.
-        manager.startCycle()
+        d = scanner.singleCycle()
 
         # Failure counts should be updated, and the assessment method
-        # should have been called.
-        self.assertEqual(1, builder.failure_count)
-        self.assertEqual(
-            1, builder.currentjob.specific_job.build.failure_count)
-
-        self.assertEqual(
-            1, manager_module.assessFailureCounts.call_count)
-
-
-class TestDispatchResult(LaunchpadTestCase):
-    """Tests `BaseDispatchResult` variations.
-
-    Variations of `BaseDispatchResult` when evaluated update the database
-    information according to their purpose.
-    """
-
-    layer = LaunchpadZopelessLayer
-
-    def _getBuilder(self, name):
-        """Return a fixed `IBuilder` instance from the sampledata.
-
-        Ensure it's active (builderok=True) and it has a in-progress job.
-        """
-        login('[e-mail]')
-
-        builder = getUtility(IBuilderSet)[name]
-        builder.builderok = True
-
-        job = builder.currentjob
-        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(job)
-        self.assertEqual(
-            'i386 build of mozilla-firefox 0.9 in ubuntu hoary RELEASE',
-            build.title)
-
-        self.assertEqual('BUILDING', build.status.name)
-        self.assertNotEqual(None, job.builder)
-        self.assertNotEqual(None, job.date_started)
-        self.assertNotEqual(None, job.logtail)
-
-        transaction.commit()
-
-        return builder, job.id
-
-    def assertBuildqueueIsClean(self, buildqueue):
-        # Check that the buildqueue is reset.
-        self.assertEqual(None, buildqueue.builder)
-        self.assertEqual(None, buildqueue.date_started)
-        self.assertEqual(None, buildqueue.logtail)
-
-    def assertBuilderIsClean(self, builder):
-        # Check that the builder is ready for a new build.
-        self.assertTrue(builder.builderok)
-        self.assertIs(None, builder.failnotes)
-        self.assertIs(None, builder.currentjob)
-
-    def testResetDispatchResult(self):
-        # Test that `ResetDispatchResult` resets the builder and job.
-        builder, job_id = self._getBuilder(BOB_THE_BUILDER_NAME)
-        buildqueue_id = builder.currentjob.id
-        builder.builderok = True
-        builder.failure_count = 1
-
-        # Setup a interaction to satisfy 'write_transaction' decorator.
-        login(ANONYMOUS)
-        slave = RecordingSlave(builder.name, builder.url, builder.vm_host)
-        result = ResetDispatchResult(slave)
-        result()
-
-        buildqueue = getUtility(IBuildQueueSet).get(buildqueue_id)
-        self.assertBuildqueueIsClean(buildqueue)
-
-        # XXX Julian
-        # Disabled test until bug 586362 is fixed.
-        #self.assertFalse(builder.builderok)
-        self.assertBuilderIsClean(builder)
-
-    def testFailDispatchResult(self):
-        # Test that `FailDispatchResult` calls assessFailureCounts() so
-        # that we know the builders and jobs are failed as necessary
-        # when a FailDispatchResult is called at the end of the dispatch
-        # chain.
-        builder, job_id = self._getBuilder(BOB_THE_BUILDER_NAME)
-
-        # Setup a interaction to satisfy 'write_transaction' decorator.
-        login(ANONYMOUS)
-        slave = RecordingSlave(builder.name, builder.url, builder.vm_host)
-        result = FailDispatchResult(slave, 'does not work!')
-        result.assessFailureCounts = FakeMethod()
-        self.assertEqual(0, result.assessFailureCounts.call_count)
-        result()
-        self.assertEqual(1, result.assessFailureCounts.call_count)
-
-    def _setup_failing_dispatch_result(self):
-        # assessFailureCounts should fail jobs or builders depending on
-        # whether it sees the failure_counts on each increasing.
-        builder, job_id = self._getBuilder(BOB_THE_BUILDER_NAME)
-        slave = RecordingSlave(builder.name, builder.url, builder.vm_host)
-        result = FailDispatchResult(slave, 'does not work!')
-        return builder, result
-
-    def test_assessFailureCounts_equal_failures(self):
-        # Basic case where the failure counts are equal and the job is
-        # reset to try again & the builder is not failed.
-        builder, result = self._setup_failing_dispatch_result()
-        buildqueue = builder.currentjob
-        build = buildqueue.specific_job.build
-        builder.failure_count = 2
-        build.failure_count = 2
-        result.assessFailureCounts()
-
-        self.assertBuilderIsClean(builder)
-        self.assertEqual('NEEDSBUILD', build.status.name)
-        self.assertBuildqueueIsClean(buildqueue)
-
-    def test_assessFailureCounts_job_failed(self):
-        # Case where the job has failed more than the builder.
-        builder, result = self._setup_failing_dispatch_result()
-        buildqueue = builder.currentjob
-        build = buildqueue.specific_job.build
-        build.failure_count = 2
-        builder.failure_count = 1
-        result.assessFailureCounts()
-
-        self.assertBuilderIsClean(builder)
-        self.assertEqual('FAILEDTOBUILD', build.status.name)
-        # The buildqueue should have been removed entirely.
-        self.assertEqual(
-            None, getUtility(IBuildQueueSet).getByBuilder(builder),
-            "Buildqueue was not removed when it should be.")
-
-    def test_assessFailureCounts_builder_failed(self):
-        # Case where the builder has failed more than the job.
-        builder, result = self._setup_failing_dispatch_result()
-        buildqueue = builder.currentjob
-        build = buildqueue.specific_job.build
-        build.failure_count = 2
-        builder.failure_count = 3
-        result.assessFailureCounts()
-
-        self.assertFalse(builder.builderok)
-        self.assertEqual('does not work!', builder.failnotes)
-        self.assertTrue(builder.currentjob is None)
-        self.assertEqual('NEEDSBUILD', build.status.name)
-        self.assertBuildqueueIsClean(buildqueue)
+        # should have been called.  The actual behaviour is tested below
+        # in TestFailureAssessments.
+        def got_scan(ignored):
+            self.assertEqual(expected_builder_count, builder.failure_count)
+            self.assertEqual(
+                expected_job_count,
+                builder.currentjob.specific_job.build.failure_count)
+            self.assertEqual(
+                1, manager_module.assessFailureCounts.call_count)
+
+        return d.addCallback(got_scan)
+
+    def test_scan_first_fail(self):
+        # The first failure of a job should result in the failure_count
+        # on the job and the builder both being incremented.
+        self._assertFailureCounting(
+            builder_count=0, job_count=0, expected_builder_count=1,
+            expected_job_count=1)
+
+    def test_scan_second_builder_fail(self):
+        # The first failure of a job should result in the failure_count
+        # on the job and the builder both being incremented.
+        self._assertFailureCounting(
+            builder_count=1, job_count=0, expected_builder_count=2,
+            expected_job_count=1)
+
+    def test_scan_second_job_fail(self):
+        # The first failure of a job should result in the failure_count
+        # on the job and the builder both being incremented.
+        self._assertFailureCounting(
+            builder_count=0, job_count=1, expected_builder_count=1,
+            expected_job_count=2)
+
+    def test_scanFailed_handles_lack_of_a_job_on_the_builder(self):
+        def failing_scan():
+            return defer.fail(Exception("fake exception"))
+        scanner = self._getScanner()
+        scanner.scan = failing_scan
+        builder = getUtility(IBuilderSet)[scanner.builder_name]
+        builder.failure_count = Builder.FAILURE_THRESHOLD
+        builder.currentjob.reset()
+        self.layer.txn.commit()
+
+        d = scanner.singleCycle()
+
+        def scan_finished(ignored):
+            self.assertFalse(builder.builderok)
+
+        return d.addCallback(scan_finished)
+
+    def test_fail_to_resume_slave_resets_job(self):
+        # If an attempt to resume and dispatch a slave fails, it should
+        # reset the job via job.reset()
+
+        # Make a slave with a failing resume() method.
+        slave = OkSlave()
+        slave.resume = lambda: deferLater(
+            reactor, 0, defer.fail, Failure(('out', 'err', 1)))
+
+        # Reset sampledata builder.
+        builder = removeSecurityProxy(
+            getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME])
+        self._resetBuilder(builder)
+        self.assertEqual(0, builder.failure_count)
+        builder.setSlaveForTesting(slave)
+        builder.vm_host = "fake_vm_host"
+
+        scanner = self._getScanner()
+
+        # Get the next job that will be dispatched.
+        job = removeSecurityProxy(builder._findBuildCandidate())
+        job.virtualized = True
+        builder.virtualized = True
+        d = scanner.singleCycle()
+
+        def check(ignored):
+            # The failure_count will have been incremented on the
+            # builder, we can check that to see that a dispatch attempt
+            # did indeed occur.
+            self.assertEqual(1, builder.failure_count)
+            # There should also be no builder set on the job.
+            self.assertTrue(job.builder is None)
+            build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(job)
+            self.assertEqual(build.status, BuildStatus.NEEDSBUILD)
+
+        return d.addCallback(check)
 
 
 class TestBuilddManager(TrialTestCase):
 
-    layer = LaunchpadZopelessLayer
+    layer = TwistedLaunchpadZopelessLayer
 
     def _stub_out_scheduleNextScanCycle(self):
         # stub out the code that adds a callLater, so that later tests
         # don't get surprises.
-        self.patch(SlaveScanner, 'scheduleNextScanCycle', FakeMethod())
+        self.patch(SlaveScanner, 'startCycle', FakeMethod())
 
     def test_addScanForBuilders(self):
         # Test that addScanForBuilders generates NewBuildersScanner objects.
@@ -1040,10 +459,62 @@
         self.assertNotEqual(0, manager.new_builders_scanner.scan.call_count)
 
 
+class TestFailureAssessments(TestCaseWithFactory):
+
+    layer = ZopelessDatabaseLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        self.builder = self.factory.makeBuilder()
+        self.build = self.factory.makeSourcePackageRecipeBuild()
+        self.buildqueue = self.build.queueBuild()
+        self.buildqueue.markAsBuilding(self.builder)
+
+    def test_equal_failures_reset_job(self):
+        self.builder.gotFailure()
+        self.builder.getCurrentBuildFarmJob().gotFailure()
+
+        assessFailureCounts(self.builder, "failnotes")
+        self.assertIs(None, self.builder.currentjob)
+        self.assertEqual(self.build.status, BuildStatus.NEEDSBUILD)
+
+    def test_job_failing_more_than_builder_fails_job(self):
+        self.builder.getCurrentBuildFarmJob().gotFailure()
+
+        assessFailureCounts(self.builder, "failnotes")
+        self.assertIs(None, self.builder.currentjob)
+        self.assertEqual(self.build.status, BuildStatus.FAILEDTOBUILD)
+
+    def test_builder_failing_more_than_job_but_under_fail_threshold(self):
+        self.builder.failure_count = Builder.FAILURE_THRESHOLD - 1
+
+        assessFailureCounts(self.builder, "failnotes")
+        self.assertIs(None, self.builder.currentjob)
+        self.assertEqual(self.build.status, BuildStatus.NEEDSBUILD)
+        self.assertTrue(self.builder.builderok)
+
+    def test_builder_failing_more_than_job_but_over_fail_threshold(self):
+        self.builder.failure_count = Builder.FAILURE_THRESHOLD
+
+        assessFailureCounts(self.builder, "failnotes")
+        self.assertIs(None, self.builder.currentjob)
+        self.assertEqual(self.build.status, BuildStatus.NEEDSBUILD)
+        self.assertFalse(self.builder.builderok)
+        self.assertEqual("failnotes", self.builder.failnotes)
+
+    def test_builder_failing_with_no_attached_job(self):
+        self.buildqueue.reset()
+        self.builder.failure_count = Builder.FAILURE_THRESHOLD
+
+        assessFailureCounts(self.builder, "failnotes")
+        self.assertFalse(self.builder.builderok)
+        self.assertEqual("failnotes", self.builder.failnotes)
+
+
 class TestNewBuilders(TrialTestCase):
     """Test detecting of new builders."""
 
-    layer = LaunchpadZopelessLayer
+    layer = TwistedLaunchpadZopelessLayer
 
     def _getScanner(self, manager=None, clock=None):
         return NewBuildersScanner(manager=manager, clock=clock)
@@ -1084,11 +555,8 @@
             new_builders, builder_scanner.checkForNewBuilders())
 
     def test_scan(self):
-        # See if scan detects new builders and schedules the next scan.
+        # See if scan detects new builders.
 
-        # stub out the addScanForBuilders and scheduleScan methods since
-        # they use callLater; we only want to assert that they get
-        # called.
         def fake_checkForNewBuilders():
             return "new_builders"
 
@@ -1104,9 +572,6 @@
         builder_scanner.scan()
         advance = NewBuildersScanner.SCAN_INTERVAL + 1
         clock.advance(advance)
-        self.assertNotEqual(
-            0, builder_scanner.scheduleScan.call_count,
-            "scheduleScan did not get called")
 
 
 def is_file_growing(filepath, poll_interval=1, poll_repeat=10):
@@ -1147,7 +612,7 @@
     return False
 
 
-class TestBuilddManagerScript(LaunchpadTestCase):
+class TestBuilddManagerScript(TestCaseWithFactory):
 
     layer = LaunchpadScriptLayer
 
@@ -1156,6 +621,7 @@
         fixture = BuilddManagerTestSetup()
         fixture.setUp()
         fixture.tearDown()
+        self.layer.force_dirty_database()
 
     # XXX Julian 2010-08-06 bug=614275
     # These next 2 tests are in the wrong place, they should be near the
 
=== modified file 'lib/lp/buildmaster/tests/test_packagebuild.py'
--- lib/lp/buildmaster/tests/test_packagebuild.py	2010-10-02 11:41:43 +0000
+++ lib/lp/buildmaster/tests/test_packagebuild.py	2010-10-25 19:14:01 +0000
@@ -99,6 +99,8 @@
         self.assertRaises(
             NotImplementedError, self.package_build.verifySuccessfulUpload)
         self.assertRaises(NotImplementedError, self.package_build.notify)
+        # XXX 2010-10-18 bug=662631
+        # Change this to do non-blocking IO.
         self.assertRaises(
             NotImplementedError, self.package_build.handleStatus,
             None, None, None)
@@ -311,6 +313,8 @@
         # A filemap with plain filenames should not cause a problem.
         # The call to handleStatus will attempt to get the file from
         # the slave resulting in a URL error in this test case.
+        # XXX 2010-10-18 bug=662631
+        # Change this to do non-blocking IO.
         self.build.handleStatus('OK', None, {
                 'filemap': {'myfile.py': 'test_file_hash'},
                 })
@@ -321,6 +325,8 @@
     def test_handleStatus_OK_absolute_filepath(self):
         # A filemap that tries to write to files outside of
         # the upload directory will result in a failed upload.
+        # XXX 2010-10-18 bug=662631
+        # Change this to do non-blocking IO.
         self.build.handleStatus('OK', None, {
             'filemap': {'/tmp/myfile.py': 'test_file_hash'},
             })
@@ -331,6 +337,8 @@
     def test_handleStatus_OK_relative_filepath(self):
         # A filemap that tries to write to files outside of
         # the upload directory will result in a failed upload.
+        # XXX 2010-10-18 bug=662631
+        # Change this to do non-blocking IO.
         self.build.handleStatus('OK', None, {
             'filemap': {'../myfile.py': 'test_file_hash'},
             })
@@ -341,6 +349,8 @@
         # The build log is set during handleStatus.
         removeSecurityProxy(self.build).log = None
         self.assertEqual(None, self.build.log)
+        # XXX 2010-10-18 bug=662631
+        # Change this to do non-blocking IO.
         self.build.handleStatus('OK', None, {
                 'filemap': {'myfile.py': 'test_file_hash'},
                 })
@@ -350,6 +360,8 @@
         # The date finished is updated during handleStatus_OK.
         removeSecurityProxy(self.build).date_finished = None
         self.assertEqual(None, self.build.date_finished)
+        # XXX 2010-10-18 bug=662631
+        # Change this to do non-blocking IO.
         self.build.handleStatus('OK', None, {
                 'filemap': {'myfile.py': 'test_file_hash'},
                 })
 
=== modified file 'lib/lp/code/model/recipebuilder.py'
--- lib/lp/code/model/recipebuilder.py	2010-08-20 20:31:18 +0000
+++ lib/lp/code/model/recipebuilder.py	2010-10-25 19:14:01 +0000
@@ -117,38 +117,42 @@
             raise CannotBuild("Unable to find distroarchseries for %s in %s" %
                 (self._builder.processor.name,
                 self.build.distroseries.displayname))
-
+        args = self._extraBuildArgs(distroarchseries, logger)
         chroot = distroarchseries.getChroot()
         if chroot is None:
             raise CannotBuild("Unable to find a chroot for %s" %
                               distroarchseries.displayname)
-        self._builder.slave.cacheFile(logger, chroot)
-
-        # 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.buildfarmjob.generateSlaveBuildCookie()
-        chroot_sha1 = chroot.content.sha1
-        logger.debug(
-            "Initiating build %s on %s" % (buildid, self._builder.url))
-
-        args = self._extraBuildArgs(distroarchseries, logger)
-        status, info = self._builder.slave.build(
-            cookie, "sourcepackagerecipe", chroot_sha1, {}, args)
-        message = """%s (%s):
-        ***** RESULT *****
-        %s
-        %s: %s
-        ******************
-        """ % (
-            self._builder.name,
-            self._builder.url,
-            args,
-            status,
-            info,
-            )
-        logger.info(message)
+        d = self._builder.slave.cacheFile(logger, chroot)
+
+        def got_cache_file(ignored):
+            # 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.buildfarmjob.generateSlaveBuildCookie()
+            chroot_sha1 = chroot.content.sha1
+            logger.debug(
+                "Initiating build %s on %s" % (buildid, self._builder.url))
+
+            return self._builder.slave.build(
+                cookie, "sourcepackagerecipe", chroot_sha1, {}, args)
+
+        def log_build_result((status, info)):
+            message = """%s (%s):
+            ***** RESULT *****
+            %s
+            %s: %s
+            ******************
+            """ % (
+                self._builder.name,
+                self._builder.url,
+                args,
+                status,
+                info,
+                )
+            logger.info(message)
+
+        return d.addCallback(got_cache_file).addCallback(log_build_result)
 
     def verifyBuildRequest(self, logger):
         """Assert some pre-build checks.
 
=== modified file 'lib/lp/soyuz/browser/tests/test_builder_views.py'
--- lib/lp/soyuz/browser/tests/test_builder_views.py	2010-10-04 19:50:45 +0000
+++ lib/lp/soyuz/browser/tests/test_builder_views.py	2010-10-25 19:14:01 +0000
@@ -34,7 +34,7 @@
         return view
 
     def test_posting_form_doesnt_call_slave_xmlrpc(self):
-        # Posting the +edit for should not call is_available, which
+        # Posting the +edit for should not call isAvailable, which
         # would do xmlrpc to a slave builder and is explicitly forbidden
         # in a webapp process.
         view = self.initialize_view()
 
=== removed file 'lib/lp/soyuz/doc/buildd-dispatching.txt'
--- lib/lp/soyuz/doc/buildd-dispatching.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/soyuz/doc/buildd-dispatching.txt	1970-01-01 00:00:00 +0000
@@ -1,371 +0,0 @@
-= Buildd Dispatching =
-
-    >>> import transaction
-    >>> import logging
-    >>> logger = logging.getLogger()
-    >>> logger.setLevel(logging.DEBUG)
-
-The buildd dispatching basically consists of finding a available
-slave in IDLE state, pushing any required files to it, then requesting
-that it starts the build procedure. These tasks are implemented by the
-BuilderSet and Builder classes.
-
-Setup the test builder:
-
-    >>> from canonical.buildd.tests import BuilddSlaveTestSetup
-    >>> fixture = BuilddSlaveTestSetup()
-    >>> fixture.setUp()
-
-Setup a suitable chroot for Hoary i386:
-
-    >>> from StringIO import StringIO
-    >>> from canonical.librarian.interfaces import ILibrarianClient
-    >>> librarian_client = getUtility(ILibrarianClient)
-
-    >>> content = 'anything'
-    >>> alias_id = librarian_client.addFile(
-    ...    'foo.tar.gz', len(content), StringIO(content), 'text/plain')
-
-    >>> from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
-    >>> from lp.registry.interfaces.distribution import IDistributionSet
-    >>> from lp.registry.interfaces.pocket import PackagePublishingPocket
-
-    >>> hoary = getUtility(IDistributionSet)['ubuntu']['hoary']
-    >>> hoary_i386 = hoary['i386']
-
-    >>> chroot = getUtility(ILibraryFileAliasSet)[alias_id]
-    >>> pc = hoary_i386.addOrUpdateChroot(chroot=chroot)
-
-Activate builders present in sampledata, we need to be logged in as a
-member of launchpad-buildd-admin:
-
-    >>> from canonical.launchpad.ftests import login
-    >>> login('[e-mail]')
-
-Set IBuilder.builderok of all present builders:
-
-    >>> from lp.buildmaster.interfaces.builder import IBuilderSet
-    >>> builder_set = getUtility(IBuilderSet)
-
-    >>> builder_set.count()
-    2
-
-    >>> from canonical.launchpad.ftests import syncUpdate
-    >>> for b in builder_set:
-    ...     b.builderok = True
-    ...     syncUpdate(b)
-
-Clean up previous BuildQueue results from sampledata:
-
-    >>> from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
-    >>> lost_job = getUtility(IBuildQueueSet).get(1)
-    >>> lost_job.builder.name
-    u'bob'
-    >>> lost_job.destroySelf()
-    >>> transaction.commit()
-
-If the specified buildd slave reset command (used inside resumeSlaveHost())
-fails, the slave will still be marked as failed.
-
-    >>> from canonical.config import config
-    >>> reset_fail_config = '''
-    ...     [builddmaster]
-    ...     vm_resume_command: /bin/false'''
-    >>> config.push('reset fail', reset_fail_config)
-    >>> frog_builder = builder_set['frog']
-    >>> frog_builder.handleTimeout(logger, 'The universe just collapsed')
-    WARNING:root:Resetting builder: http://localhost:9221/ -- The universe just collapsed
-    ...
-    WARNING:root:Failed to reset builder: http://localhost:9221/ -- Resuming failed:
-    ...
-    WARNING:root:Disabling builder: http://localhost:9221/ -- The universe just collapsed
-    ...
-    <BLANKLINE>
-
-Since we were unable to reset the 'frog' builder it was marked as 'failed'.
-
-    >>> frog_builder.builderok
-    False
-
-Restore default value for resume command.
-
-    >>> ignored_config = config.pop('reset fail')
-
-The 'bob' builder is available for build jobs.
-
-    >>> bob_builder = builder_set['bob']
-    >>> bob_builder.name
-    u'bob'
-    >>> bob_builder.virtualized
-    False
-    >>> bob_builder.is_available
-    True
-    >>> bob_builder.builderok
-    True
-
-
-== Builder dispatching API ==
-
-Now let's check the build candidates which will be considered for the
-builder 'bob':
-
-    >>> from zope.security.proxy import removeSecurityProxy
-    >>> job = removeSecurityProxy(bob_builder)._findBuildCandidate()
-
-The single BuildQueue found is a non-virtual pending build:
-
-    >>> job.id
-    2
-    >>> from lp.soyuz.interfaces.binarypackagebuild import (
-    ...     IBinaryPackageBuildSet)
-    >>> build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(job)
-    >>> build.status.name
-    'NEEDSBUILD'
-    >>> job.builder is None
-    True
-    >>> job.date_started is None
-    True
-    >>> build.is_virtualized
-    False
-
-The build start time is not set yet either.
-
-    >>> print build.date_first_dispatched
-    None
-
-Update the SourcePackageReleaseFile corresponding to this job:
-
-    >>> content = 'anything'
-    >>> alias_id = librarian_client.addFile(
-    ...    'foo.dsc', len(content), StringIO(content), 'application/dsc')
-
-    >>> sprf = build.source_package_release.files[0]
-    >>> naked_sprf = removeSecurityProxy(sprf)
-    >>> naked_sprf.libraryfile = getUtility(ILibraryFileAliasSet)[alias_id]
-    >>> flush_database_updates()
-
-Check the dispatching method itself:
-
-    >>> dispatched_job = bob_builder.findAndStartJob()
-    >>> job == dispatched_job
-    True
-    >>> bob_builder.builderok = True
-
-    >>> flush_database_updates()
-
-Verify if the job (BuildQueue) was updated appropriately:
-
-    >>> job.builder.id == bob_builder.id
-    True
-
-    >>> dispatched_build = getUtility(
-    ...     IBinaryPackageBuildSet).getByQueueEntry(job)
-    >>> dispatched_build == build
-    True
-
-    >>> build.status.name
-    'BUILDING'
-
-Shutdown builder, mark the build record as failed and remove the
-buildqueue record, so the build was eliminated:
-
-    >>> fixture.tearDown()
-
-    >>> from lp.buildmaster.enums import BuildStatus
-    >>> build.status = BuildStatus.FAILEDTOBUILD
-    >>> job.destroySelf()
-    >>> flush_database_updates()
-
-
-== PPA build dispatching ==
-
-Create a new Build record of the same source targeted for a PPA archive:
-
-    >>> from lp.registry.interfaces.person import IPersonSet
-    >>> cprov = getUtility(IPersonSet).getByName('cprov')
-
-    >>> ppa_build = sprf.sourcepackagerelease.createBuild(
-    ...     hoary_i386, PackagePublishingPocket.RELEASE, cprov.archive)
-
-Create BuildQueue record and inspect some parameters:
-
-    >>> ppa_job = ppa_build.queueBuild()
-    >>> ppa_job.id
-    3
-    >>> ppa_job.builder == None
-    True
-    >>> ppa_job.date_started == None
-    True
-
-The build job's archive requires virtualized builds.
-
-    >>> build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(ppa_job)
-    >>> build.archive.require_virtualized
-    True
-
-But the builder is not virtualized.
-
-    >>> bob_builder.virtualized
-    False
-
-Hence, the builder will not be able to pick up the PPA build job created
-above.
-
-    >>> bob_builder.vm_host = 'localhost.ppa'
-    >>> syncUpdate(bob_builder)
-
-    >>> job = removeSecurityProxy(bob_builder)._findBuildCandidate()
-    >>> print job
-    None
-
-In order to enable 'bob' to find and build the PPA job, we have to
-change it to virtualized.  This is because PPA builds will only build
-on virtualized builders.  We also need to make sure this build's source
-is published, or it will also be ignored (by superseding it).  We can
-do this by copying the existing publication in Ubuntu.
-
-    >>> from lp.soyuz.model.publishing import (
-    ...     SourcePackagePublishingHistory)
-    >>> [old_pub] = SourcePackagePublishingHistory.selectBy(
-    ...    distroseries=build.distro_series,
-    ...    sourcepackagerelease=build.source_package_release)
-    >>> new_pub = old_pub.copyTo(
-    ...     old_pub.distroseries, old_pub.pocket, build.archive)
-
-    >>> bob_builder.virtualized = True
-    >>> syncUpdate(bob_builder)
-
-    >>> job = removeSecurityProxy(bob_builder)._findBuildCandidate()
-    >>> ppa_job.id == job.id
-    True
-
-For further details regarding IBuilder._findBuildCandidate() please see
-lib/lp/soyuz/tests/test_builder.py.
-
-Start buildd-slave to be able to dispatch jobs.
-
-    >>> fixture = BuilddSlaveTestSetup()
-    >>> fixture.setUp()
-
-Before dispatching we can check if the builder is protected against
-mistakes in code that results in a attempt to build a virtual job in
-a non-virtual build.
-
-    >>> bob_builder.virtualized = False
-    >>> flush_database_updates()
-    >>> removeSecurityProxy(bob_builder)._dispatchBuildCandidate(ppa_job)
-    Traceback (most recent call last):
-    ...
-    AssertionError: Attempt to build non-virtual item on a virtual builder.
-
-Mark the builder as virtual again, so we can dispatch the ppa job
-successfully.
-
-    >>> bob_builder.virtualized = True
-    >>> flush_database_updates()
-
-    >>> dispatched_job = bob_builder.findAndStartJob()
-    >>> ppa_job == dispatched_job
-    True
-
-    >>> flush_database_updates()
-
-PPA job is building.
-
-    >>> ppa_job.builder.name
-    u'bob'
-
-    >>> build.status.name
-    'BUILDING'
-
-Shutdown builder slave, mark the ppa build record as failed, remove the
-buildqueue record and make 'bob' builder non-virtual again,  so the
-environment is back to the initial state.
-
-    >>> fixture.tearDown()
-
-    >>> build.status = BuildStatus.FAILEDTOBUILD
-    >>> ppa_job.destroySelf()
-    >>> bob_builder.virtualized = False
-    >>> flush_database_updates()
-
-
-== Security build dispatching ==
-
-Setup chroot for warty/i386.
-
-    >>> warty = getUtility(IDistributionSet)['ubuntu']['warty']
-    >>> warty_i386 = warty['i386']
-    >>> pc = warty_i386.addOrUpdateChroot(chroot=chroot)
-
-Create a new Build record for test source targeted to warty/i386
-architecture and SECURITY pocket:
-
-    >>> sec_build = sprf.sourcepackagerelease.createBuild(
-    ...     warty_i386, PackagePublishingPocket.SECURITY, hoary.main_archive)
-
-Create BuildQueue record and inspect some parameters:
-
-    >>> sec_job = sec_build.queueBuild()
-    >>> sec_job.id
-    4
-    >>> print sec_job.builder
-    None
-    >>> print sec_job.date_started
-    None
-    >>> sec_build.is_virtualized
-    False
-
-In normal conditions the next available candidate would be the job
-targeted to SECURITY pocket. However, the builders are forbidden to
-accept such jobs until we have finished the EMBARGOED archive
-implementation.
-
-    >>> fixture = BuilddSlaveTestSetup()
-    >>> fixture.setUp()
-    >>> removeSecurityProxy(bob_builder)._dispatchBuildCandidate(sec_job)
-    Traceback (most recent call last):
-    ...
-    AssertionError: Soyuz is not yet capable of building SECURITY uploads.
-    >>> fixture.tearDown()
-
-To solve this problem temporarily until we start building security
-uploads, we will mark builds targeted to the SECURITY pocket as
-FAILEDTOBUILD during the _findBuildCandidate look-up.
-
-We will also create another build candidate in breezy-autotest/i386 to
-check if legitimate pending candidates will remain valid.
-
-    >>> breezy = getUtility(IDistributionSet)['ubuntu']['breezy-autotest']
-    >>> breezy_i386 = breezy['i386']
-    >>> pc = breezy_i386.addOrUpdateChroot(chroot=chroot)
-
-    >>> pending_build = sprf.sourcepackagerelease.createBuild(
-    ...     breezy_i386, PackagePublishingPocket.UPDATES, hoary.main_archive)
-    >>> pending_job = pending_build.queueBuild()
-