Merge lp:~mars/launchpad/test-ghost-update-2 into lp:~launchpad/launchpad/ghost-line

[Māris Fogels]

Testing the bundle-merge tarmac command

Testing the bundle-merge Tarmac command

[Māris Fogels (mars)]

review: Approve

[Launchpad PQM Bot (launchpad-pqm)]

No approved revision specified.

[lp:~mars/launchpad/test-ghost-update-2]

11786. By Māris Fogels on 2010-12-07

Merged r11806

11787. By Māris Fogels on 2010-12-07

Added a file for testing

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
--- lib/canonical/launchpad/icing/style-3-0.css.in	2010-09-23 11:17:45 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css.in	2010-12-07 16:29:13 +0000
@@ -284,6 +284,12 @@
 a.help.icon, a.sprite.maybe.help {
     border: none;
 }
+a.invalid-link {
+    disabled: True;
+    color: #909090;
+    text-decoration: none;
+    cursor: default;
+    }
 img, a img {
     /* No border on images that are links. */
     border: none;

=== modified file 'lib/lp/app/browser/configure.zcml'
--- lib/lp/app/browser/configure.zcml	2010-10-15 01:27:04 +0000
+++ lib/lp/app/browser/configure.zcml	2010-12-07 16:29:13 +0000
@@ -98,6 +98,12 @@
       template="../templates/launchpad-search-form.pt"
       permission="zope.Public" />
 
+  <browser:page
+      for="*"
+      name="+check-links"
+      class="lp.app.browser.linkchecker.LinkCheckerAPI"
+      permission="zope.Public"/>
+
   <!-- TALES namespaces. -->
 
   <!-- TALES lp: namespace (should be deprecated) -->

=== added file 'lib/lp/app/browser/linkchecker.py'
--- lib/lp/app/browser/linkchecker.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/browser/linkchecker.py	2010-12-07 16:29:13 +0000
@@ -0,0 +1,77 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+# pylint: disable-msg=E0211,E0213
+
+__metaclass__ = type
+__all__ = [
+    'LinkCheckerAPI',
+    ]
+
+import simplejson
+from zope.component import getUtility
+
+from lp.app.errors import NotFoundError
+from lp.code.errors import (
+    CannotHaveLinkedBranch,
+    InvalidNamespace,
+    NoLinkedBranch,
+    NoSuchBranch,
+    )
+from lp.code.interfaces.branchlookup import IBranchLookup
+from lp.registry.interfaces.product import InvalidProductName
+
+
+class LinkCheckerAPI:
+    """Validates Launchpad shortcut links.
+
+    This class provides the endpoint of an Ajax call to .../+check-links.
+    When invoked with a collection of links harvested from a page, it will
+    check the validity of each one and send a response containing those that
+    are invalid. Javascript on the page will set the style of invalid links to
+    something appropriate.
+
+    This initial implementation supports processing links like the following:
+        /+branch/foo/bar
+
+    The implementation can easily be extended to handle other forms by
+    providing a method to handle the link type extracted from the json
+    request.
+    """
+
+    def __init__(self, context, request):
+        # We currently only use the request.
+        # self.context = context
+        self.request = request
+
+        # Each link type has it's own validation method.
+        self.link_checkers = dict(
+            branch_links=self.check_branch_links,
+        )
+
+    def __call__(self):
+        result = {}
+        links_to_check_data = self.request.get('link_hrefs')
+        links_to_check = simplejson.loads(links_to_check_data)
+
+        for link_type in links_to_check:
+            links = links_to_check[link_type]
+            invalid_links = self.link_checkers[link_type](links)
+            result['invalid_'+link_type] = invalid_links
+
+        self.request.response.setHeader('Content-type', 'application/json')
+        return simplejson.dumps(result)
+
+    def check_branch_links(self, links):
+        """Check links of the form /+branch/foo/bar"""
+        invalid_links = []
+        branch_lookup = getUtility(IBranchLookup)
+        for link in links:
+            path = link.strip('/')[len('+branch/'):]
+            try:
+                branch_lookup.getByLPPath(path)
+            except (CannotHaveLinkedBranch, InvalidNamespace,
+                    InvalidProductName, NoLinkedBranch, NoSuchBranch,
+                    NotFoundError):
+                invalid_links.append(link)
+        return invalid_links

=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py	2010-09-25 14:29:32 +0000
+++ lib/lp/app/browser/stringformatter.py	2010-12-07 16:29:13 +0000
@@ -274,7 +274,9 @@
                 return FormattersAPI._linkify_bug_number(
                     lp_url, path, trailers)
             url = '/+branch/%s' % path
-            return '<a href="%s">%s</a>%s' % (
+            # Mark the links with a 'branch-short-link' class so they can be
+            # harvested and validated when the page is rendered.
+            return '<a href="%s" class="branch-short-link">%s</a>%s' % (
                 cgi.escape(url, quote=True),
                 cgi.escape(lp_url),
                 cgi.escape(trailers))

=== added file 'lib/lp/app/browser/tests/test_linkchecker.py'
--- lib/lp/app/browser/tests/test_linkchecker.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/browser/tests/test_linkchecker.py	2010-12-07 16:29:13 +0000
@@ -0,0 +1,83 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Unit tests for the LinkCheckerAPI."""
+
+__metaclass__ = type
+
+from random import shuffle
+
+import simplejson
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.browser.linkchecker import LinkCheckerAPI
+from lp.testing import TestCaseWithFactory
+
+
+class TestLinkCheckerAPI(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    BRANCH_URL_TEMPLATE = '/+branch/%s'
+
+    def check_invalid_links(self, result_json, link_type, invalid_links):
+        link_dict = simplejson.loads(result_json)
+        links_to_check = link_dict[link_type]
+        self.assertEqual(len(invalid_links), len(links_to_check))
+        self.assertEqual(set(invalid_links), set(links_to_check))
+
+    def make_valid_links(self):
+        branch = self.factory.makeProductBranch()
+        valid_branch_url = self.BRANCH_URL_TEMPLATE % branch.unique_name
+        product = self.factory.makeProduct()
+        product_branch = self.factory.makeProductBranch(product=product)
+        removeSecurityProxy(product).development_focus.branch = product_branch
+        valid_product_url = self.BRANCH_URL_TEMPLATE % product.name
+
+        return [
+            valid_branch_url,
+            valid_product_url,
+        ]
+
+    def make_invalid_links(self):
+        return [
+            self.BRANCH_URL_TEMPLATE % 'foo',
+            self.BRANCH_URL_TEMPLATE % 'bar',
+            ]
+
+    def invoke_branch_link_checker(
+        self, valid_branch_urls=None, invalid_branch_urls=None):
+        if valid_branch_urls is None:
+            valid_branch_urls = {}
+        if invalid_branch_urls is None:
+            invalid_branch_urls = {}
+
+        branch_urls = list(valid_branch_urls)
+        branch_urls.extend(invalid_branch_urls)
+        shuffle(branch_urls)
+
+        links_to_check = dict(branch_links=branch_urls)
+        link_json = simplejson.dumps(links_to_check)
+
+        request = LaunchpadTestRequest(link_hrefs=link_json)
+        link_checker = LinkCheckerAPI(object(), request)
+        result_json = link_checker()
+        self.check_invalid_links(
+            result_json, 'invalid_branch_links', invalid_branch_urls)
+
+    def test_only_valid_branchlinks(self):
+        branch_urls = self.make_valid_links()
+        self.invoke_branch_link_checker(valid_branch_urls=branch_urls)
+
+    def test_only_invalid_branchlinks(self):
+        branch_urls = self.make_invalid_links()
+        self.invoke_branch_link_checker(invalid_branch_urls=branch_urls)
+
+    def test_valid_and_invald_branchlinks(self):
+        valid_branch_urls = self.make_valid_links()
+        invalid_branch_urls = self.make_invalid_links()
+        self.invoke_branch_link_checker(
+            valid_branch_urls=valid_branch_urls,
+            invalid_branch_urls=invalid_branch_urls)

=== added file 'lib/lp/app/configure.zcml'
--- lib/lp/app/configure.zcml	1970-01-01 00:00:00 +0000
+++ lib/lp/app/configure.zcml	2010-12-07 16:29:13 +0000
@@ -0,0 +1,14 @@
+<!-- Copyright 2009 Canonical Ltd.  This software is licensed under the
+     GNU Affero General Public License version 3 (see the file LICENSE).
+-->
+
+<configure
+    xmlns="http://namespaces.zope.org/zope"
+    xmlns:browser="http://namespaces.zope.org/browser"
+    xmlns:i18n="http://namespaces.zope.org/i18n"
+    xmlns:xmlrpc="http://namespaces.zope.org/xmlrpc"
+    xmlns:lp="http://namespaces.canonical.com/lp"
+    i18n_domain="launchpad">
+    <include
+        package=".browser"/>
+</configure>

=== removed file 'lib/lp/app/configure.zcml'
--- lib/lp/app/configure.zcml	2009-07-17 02:25:09 +0000
+++ lib/lp/app/configure.zcml	1970-01-01 00:00:00 +0000
@@ -1,14 +0,0 @@
-<!-- Copyright 2009 Canonical Ltd.  This software is licensed under the
-     GNU Affero General Public License version 3 (see the file LICENSE).
--->
-
-<configure
-    xmlns="http://namespaces.zope.org/zope"
-    xmlns:browser="http://namespaces.zope.org/browser"
-    xmlns:i18n="http://namespaces.zope.org/i18n"
-    xmlns:xmlrpc="http://namespaces.zope.org/xmlrpc"
-    xmlns:lp="http://namespaces.canonical.com/lp"
-    i18n_domain="launchpad">
-    <include
-        package=".browser"/>
-</configure>

=== modified file 'lib/lp/app/doc/displaying-paragraphs-of-text.txt'
--- lib/lp/app/doc/displaying-paragraphs-of-text.txt	2010-10-09 16:36:22 +0000
+++ lib/lp/app/doc/displaying-paragraphs-of-text.txt	2010-12-07 16:29:13 +0000
@@ -357,17 +357,17 @@
     ...     'lp:///foo\n'
     ...     'lp:/foo\n')
     >>> print test_tales('foo/fmt:text-to-html', foo=text)
-    <p><a href="/+branch/~foo/bar/baz">lp:~foo/bar/baz</a><br />
-    <a href="/+branch/~foo/bar/bug-123">lp:~foo/bar/bug-123</a><br />
-    <a href="/+branch/~foo/+junk/baz">lp:~foo/+junk/baz</a><br />
-    <a href="/+branch/~foo/ubuntu/jaunty/evolution/baz">lp:~foo/ubuntu/jaunty/evolution/baz</a><br />
-    <a href="/+branch/foo/bar">lp:foo/bar</a><br />
-    <a href="/+branch/foo">lp:foo</a><br />
-    <a href="/+branch/foo">lp:foo</a>,<br />
-    <a href="/+branch/foo/bar">lp:foo/bar</a>.<br />
-    <a href="/+branch/foo/bar/baz">lp:foo/bar/baz</a><br />
-    <a href="/+branch/foo">lp:///foo</a><br />
-    <a href="/+branch/foo">lp:/foo</a></p>
+    <p><a href="/+branch/~foo/bar/baz" class="...">lp:~foo/bar/baz</a><br />
+    <a href="/+branch/~foo/bar/bug-123" class="...">lp:~foo/bar/bug-123</a><br />
+    <a href="/+branch/~foo/+junk/baz" class="...">lp:~foo/+junk/baz</a><br />
+    <a href="/+branch/~foo/ubuntu/jaunty/evolution/baz" class="...">lp:~foo/ubuntu/jaunty/evolution/baz</a><br />
+    <a href="/+branch/foo/bar" class="...">lp:foo/bar</a><br />
+    <a href="/+branch/foo" class="...">lp:foo</a><br />
+    <a href="/+branch/foo" class="...">lp:foo</a>,<br />
+    <a href="/+branch/foo/bar" class="...">lp:foo/bar</a>.<br />
+    <a href="/+branch/foo/bar/baz" class="...">lp:foo/bar/baz</a><br />
+    <a href="/+branch/foo" class="...">lp:///foo</a><br />
+    <a href="/+branch/foo" class="...">lp:/foo</a></p>
 
 Text that looks like a branch reference, but is followed only by digits is
 treated as a link to a bug.

=== added file 'lib/lp/app/javascript/lp-links.js'
--- lib/lp/app/javascript/lp-links.js	1970-01-01 00:00:00 +0000
+++ lib/lp/app/javascript/lp-links.js	2010-12-07 16:29:13 +0000
@@ -0,0 +1,105 @@
+/**
+ * Launchpad utilities for manipulating links.
+ *
+ * @module app
+ * @submodule links
+ */
+
+YUI.add('lp.app.links', function(Y) {
+
+    function harvest_links(Y, links_holder, link_class, link_type) {
+        // Get any links of the specified link_class and store them as the
+        // specified link_type in the specified links_holder
+        var link_info = new Array();
+        Y.all('.'+link_class).each(function(link) {
+            var href = link.getAttribute('href');
+            if( link_info.indexOf(href)<0 ) {
+                link_info.push(href);
+            }
+        });
+        if( link_info.length > 0 ) {
+            links_holder[link_type] = link_info;
+        }
+    }
+
+    function process_invalid_links(
+            Y, link_info, link_class, link_type, title) {
+        // We have a collection of invalid links possibly containing links of
+        // type link_type, so we need to remove the existing link_class,
+        // replace it with an invalid-link class, and set the link title.
+        var invalid_links = Y.Array(link_info['invalid_'+link_type]);
+
+        if( invalid_links.length > 0) {
+            Y.all('.'+link_class).each(function(link) {
+                var href = link.getAttribute('href');
+                if( invalid_links.indexOf(href)>=0 ) {
+                    var msg = title + href;
+                    link.removeClass(link_class);
+                    link.addClass('invalid-link');
+                    link.title = msg
+                    link.on('click', function(e) {
+                        e.halt();
+                        alert(msg);
+                    });
+                }
+            });
+        }
+    }
+
+    var links = Y.namespace('lp.app.links');
+
+    links.check_valid_lp_links = function() {
+        // Grabs any lp: style links on the page and checks that they are
+        // valid. Invalid ones have their class changed to "invalid-link".
+        // ATM, we only handle +branch links.
+
+        var links_to_check = {}
+
+        // We get all the links with defined css classes.
+        // At the moment, we just handle branch links, but in future...
+        harvest_links(Y, links_to_check, 'branch-short-link', 'branch_links');
+
+        // Do we have anything to do?
+        if( Y.Object.size(links_to_check) == 0 ) {
+            return;
+        }
+
+        // Get the final json to send
+        var json_link_info = Y.JSON.stringify(links_to_check);
+        var qs = '';
+        qs = LP.client.append_qs(qs, 'link_hrefs', json_link_info);
+
+        var config = {
+            on: {
+                failure: function(id, response, args) {
+                    // If we have firebug installed, log the error.
+                    if( console != undefined ) {
+                        console.log("Link Check Error: " + args + ': '
+                                + response.status + ' - ' +
+                                response.statusText + ' - '
+                                + response.responseXML);
+                    }
+                },
+                success: function(id, response) {
+                    var link_info = Y.JSON.parse(response.responseText)
+                    // ATM, we just handle branch links, but in future...
+                    process_invalid_links(Y, link_info, 'branch-short-link',
+                            'branch_links', "Invalid branch: ");
+                }
+            }
+        }
+        var uri = '+check-links';
+        var on = Y.merge(config.on);
+        var client = this;
+        var y_config = { method: "POST",
+                         headers: {'Accept': 'application/json'},
+                         on: on,
+                         'arguments': [client, uri],
+                         data: qs};
+        Y.io(uri, y_config);
+    };
+
+}, "0.1", {"requires": [
+    "base", "node", "io", "dom", "json"
+    ]});
+

=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt	2010-10-25 13:16:10 +0000
+++ lib/lp/app/templates/base-layout-macros.pt	2010-12-07 16:29:13 +0000
@@ -175,6 +175,8 @@
     <script type="text/javascript"
             tal:attributes="src string:${lp_js}/app/lp-mochi.js"></script>
     <script type="text/javascript"
+            tal:attributes="src string:${lp_js}/app/lp-links.js"></script>
+    <script type="text/javascript"
             tal:attributes="src string:${lp_js}/app/dragscroll.js"></script>
     <script type="text/javascript"
             tal:attributes="src string:${lp_js}/app/picker.js"></script>
@@ -304,6 +306,13 @@
         // anywhere outside of it.
         Y.on('click', handleClickOnPage, window);
     });
+
+    LPS.use('lp.app.links',
+      function(Y) {
+        Y.on('load', function(e) {
+            Y.lp.app.links.check_valid_lp_links();
+        }, window);
+    });
   </script>
 </metal:page-javascript>
 

=== modified file 'lib/lp/bugs/windmill/tests/test_bug_commenting.py'
--- lib/lp/bugs/windmill/tests/test_bug_commenting.py	2010-08-20 20:31:18 +0000
+++ lib/lp/bugs/windmill/tests/test_bug_commenting.py	2010-12-07 16:29:13 +0000
@@ -18,7 +18,7 @@
 WAIT_ELEMENT_COMPLETE = u'30000'
 WAIT_CHECK_CHANGE = u'1000'
 ADD_COMMENT_BUTTON = (
-    u'//input[@id="field.actions.save" and @class="button js-action"]')
+    u'//input[@id="field.actions.save" and contains(@class, "button")]')
 
 
 class TestBugCommenting(WindmillTestCase):

=== modified file 'lib/lp/buildmaster/doc/builder.txt'
--- lib/lp/buildmaster/doc/builder.txt	2010-09-24 12:10:52 +0000
+++ lib/lp/buildmaster/doc/builder.txt	2010-12-07 16:29:13 +0000
@@ -19,6 +19,9 @@
 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
@@ -83,7 +86,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'
 
@@ -167,7 +170,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))
@@ -185,3 +188,116 @@
 
     >>> 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-10-18 11:57:09 +0000
+++ lib/lp/buildmaster/interfaces/builder.py	2010-12-07 16:29:13 +0000
@@ -154,6 +154,11 @@
 
     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."))
@@ -168,74 +173,32 @@
     def resetFailureCount():
         """Set the failure_count back to zero."""
 
-    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 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 rescueIfLost(logger=None):
         """Reset the slave if its job information doesn't match the DB.
 
-        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.
+        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.
         """
 
     def updateStatus(logger=None):
-        """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.
-        """
+        """Update the builder's status by probing it."""
 
     def cleanSlave():
-        """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.
-        """
+        """Clean any temporary files from the slave."""
+
+    def failBuilder(reason):
+        """Mark builder as failed for a given reason."""
 
     def requestAbort():
         """Ask that a build be aborted.
@@ -243,9 +206,6 @@
         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():
@@ -257,35 +217,37 @@
         :raises: CannotResumeHost: if builder is not virtual or if the
             configuration command has failed.
 
-        :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.
+        :return: command stdout and stderr buffers as a tuple.
         """
 
+    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 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.
+        :return: 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 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.
+        :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`.
         """
 
     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):
@@ -293,10 +255,21 @@
 
         :param build_queue_item: A BuildQueueItem to build.
         :param logger: A logger to be used to log diagnostic information.
-
-        :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.
+        :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.
         """
 
     def handleTimeout(logger, error_message):
@@ -311,8 +284,6 @@
 
         :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):
@@ -320,9 +291,17 @@
 
         :param buildd_slave: An optional buildd slave that this builder should
             talk to.
-        :return: A Deferred whose value is the `IBuildQueue` instance
-            found or None if no job was found.
-        """
+        :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."""
 
 
 class IBuilderSet(Interface):

=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2010-10-20 12:28:46 +0000
+++ lib/lp/buildmaster/manager.py	2010-12-07 16:29:13 +0000
@@ -10,10 +10,13 @@
     'BuilddManager',
     'BUILDD_MANAGER_LOG_NAME',
     'FailDispatchResult',
+    'RecordingSlave',
     'ResetDispatchResult',
+    'buildd_success_result_map',
     ]
 
 import logging
+import os
 
 import transaction
 from twisted.application import service
@@ -21,27 +24,129 @@
     defer,
     reactor,
     )
-from twisted.internet.task import LoopingCall
+from twisted.protocols.policies import TimeoutMixin
 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.buildmaster.interfaces.buildfarmjobbehavior import (
-    BuildBehaviorMismatch,
-    )
-from lp.buildmaster.model.builder import Builder
-from lp.buildmaster.interfaces.builder import (
-    BuildDaemonError,
-    BuildSlaveFailure,
-    CannotBuild,
-    CannotFetchFile,
-    CannotResumeHost,
-    )
+from lp.services.twistedsupport.processmonitor import ProcessWithTimeout
 
 
 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.
@@ -54,12 +159,9 @@
     # builder.currentjob hides a complicated query, don't run it twice.
     # See bug 623281.
     current_job = builder.currentjob
-    if current_job is None:
-        job_failure_count = 0
-    else:
-        job_failure_count = current_job.specific_job.build.failure_count
+    build_job = current_job.specific_job.build
 
-    if builder.failure_count == job_failure_count and current_job is not None:
+    if builder.failure_count == build_job.failure_count:
         # 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
@@ -68,28 +170,17 @@
         current_job.reset()
         return
 
-    if builder.failure_count > job_failure_count:
+    if builder.failure_count > build_job.failure_count:
         # The builder has failed more than the jobs it's been
-        # 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)
+        # running, so let's disable it and re-schedule the build.
+        builder.failBuilder(fail_notes)
+        current_job.reset()
     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()
 
@@ -99,108 +190,133 @@
         # 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:
+
+        try:
+            slave = self.scan()
+            if slave is None:
+                self.scheduleNextScanCycle()
+            else:
+                # XXX: Ought to return Deferred.
+                self.resumeAndDispatch(slave)
+        except:
+            error = Failure()
             self.logger.info("Scanning failed with: %s\n%s" %
-                (failure.getErrorMessage(), failure.getTraceback()))
+                (error.getErrorMessage(), error.getTraceback()))
 
-        # Decide if we need to terminate the job or fail the
-        # builder.
-        try:
             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:
-                self.logger.info(
-                    "Builder %s failed a probe, count: %s" % (
-                        self.builder_name, builder.failure_count))
-            assessFailureCounts(builder, failure.getErrorMessage())
+
+            # 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()
-        except:
-            # 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()
-
+
+            self.scheduleNextScanCycle()
+
+    @write_transaction
     def scan(self):
         """Probe the builder and update/dispatch/collect as appropriate.
 
-        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.
+        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.
         """
         # We need to re-fetch the builder object on each cycle as the
         # Storm store is invalidated over transaction boundaries.
@@ -208,72 +324,240 @@
         self.builder = get_builder(self.builder_name)
 
         if self.builder.builderok:
-            d = self.builder.updateStatus(self.logger)
+            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)
         else:
+            # No resume required, build dispatching can commence.
             d = defer.succeed(None)
 
-        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:
+        # 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)
                     return None
-            return d.addCallback(job_started)
-
-        d.addCallback(status_updated)
-        d.addCallback(build_updated)
-        return d
+            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)
 
 
 class NewBuildersScanner:
@@ -294,21 +578,15 @@
         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."""
-        self.loop = LoopingCall(self.scan)
-        self.loop.clock = self._clock
-        self.stopping_deferred = self.loop.start(self.SCAN_INTERVAL)
-        return self.stopping_deferred
+        return self._clock.callLater(self.SCAN_INTERVAL, self.scan)
 
     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."""
@@ -331,7 +609,10 @@
             manager=self, clock=clock)
 
     def _setupLogger(self):
-        """Set up a 'slave-scanner' logger that redirects to twisted.
+        """Setup a 'slave-scanner' logger that redirects to twisted.
+
+        It is going to be used locally and within the thread running
+        the scan() method.
 
         Make it less verbose to avoid messing too much with the old code.
         """
@@ -362,29 +643,12 @@
         # 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.startCycle()
+            slave_scanner.scheduleNextScanCycle()
 
         # 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-10-20 11:54:27 +0000
+++ lib/lp/buildmaster/model/builder.py	2010-12-07 16:29:13 +0000
@@ -13,11 +13,12 @@
     ]
 
 import gzip
+import httplib
 import logging
 import os
 import socket
+import subprocess
 import tempfile
-import transaction
 import urllib2
 import xmlrpclib
 
@@ -33,13 +34,6 @@
     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
 
@@ -64,6 +58,7 @@
 from lp.buildmaster.interfaces.builder import (
     BuildDaemonError,
     BuildSlaveFailure,
+    CannotBuild,
     CannotFetchFile,
     CannotResumeHost,
     CorruptBuildCookie,
@@ -71,6 +66,9 @@
     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 (
@@ -80,9 +78,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.processmonitor import ProcessWithTimeout
-from lp.services.twistedsupport import cancel_on_timeout
+from lp.services.twistedsupport.xmlrpc import BlockingProxy
 # XXX Michael Nelson 2010-01-13 bug=491330
 # These dependencies on soyuz will be removed when getBuildRecords()
 # is moved.
@@ -94,9 +92,25 @@
 from lp.soyuz.model.processor import Processor
 
 
-class QuietQueryFactory(xmlrpc._QueryFactory):
-    """XMLRPC client factory that doesn't splatter the log with junk."""
-    noisy = False
+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 BuilderSlave(object):
@@ -111,7 +125,24 @@
     # many false positives in your test run and will most likely break
     # production.
 
-    def __init__(self, proxy, builder_url, vm_host, reactor=None):
+    # 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):
         """Initialize a BuilderSlave.
 
         :param proxy: An XML-RPC proxy, implementing 'callRemote'. It must
@@ -124,87 +155,63 @@
         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 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 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 abort(self):
         """Abort the current build."""
-        return self._with_timeout(self._server.callRemote('abort'))
+        return self._server.callRemote('abort')
 
     def clean(self):
         """Clean up the waiting files and reset the slave's internal state."""
-        return self._with_timeout(self._server.callRemote('clean'))
+        return self._server.callRemote('clean')
 
     def echo(self, *args):
         """Echo the arguments back."""
-        return self._with_timeout(self._server.callRemote('echo', *args))
+        return self._server.callRemote('echo', *args)
 
     def info(self):
         """Return the protocol version and the builder methods supported."""
-        return self._with_timeout(self._server.callRemote('info'))
+        return self._server.callRemote('info')
 
     def status(self):
         """Return the status of the build daemon."""
-        return self._with_timeout(self._server.callRemote('status'))
+        return 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._with_timeout(self._server.callRemote(
-            'ensurepresent', sha1sum, url, username, password))
+        return 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, 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
+    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
         """
+        # 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}
-        # 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
+        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)
 
     def cacheFile(self, logger, libraryfilealias):
         """Make sure that the file at 'libraryfilealias' is on the slave.
@@ -217,15 +224,13 @@
             "Asking builder on %s to ensure it has file %s (%s, %s)" % (
                 self._file_cache_url, libraryfilealias.filename, url,
                 libraryfilealias.content.sha1))
-        return self.sendFileToSlave(libraryfilealias.content.sha1, url)
+        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."""
-        d = self.ensurepresent(sha1, url, username, password)
-        def check_present((present, info)):
-            if not present:
-                raise CannotFetchFile(url, info)
-        return d.addCallback(check_present)
+        present, info = self.ensurepresent(sha1, url, username, password)
+        if not present:
+            raise CannotFetchFile(url, info)
 
     def build(self, buildid, builder_type, chroot_sha1, filemap, args):
         """Build a thing on this build slave.
@@ -238,18 +243,19 @@
         :param args: A dictionary of extra arguments. The contents depend on
             the build job type.
         """
-        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)
+        try:
+            return self._server.callRemote(
+                'build', buildid, builder_type, chroot_sha1, filemap, args)
+        except xmlrpclib.Fault, info:
+            raise BuildSlaveFailure(info)
 
 
 # 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
@@ -259,58 +265,61 @@
         'BuilderStatus.WAITING': 2
         }
 
-    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)
+    # 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()
         else:
-            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
+            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)
 
 
 def updateBuilderStatus(builder, logger=None):
@@ -318,7 +327,16 @@
     if logger:
         logger.debug('Checking %s' % builder.name)
 
-    return builder.rescueIfLost(logger)
+    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)
 
 
 class Builder(SQLBase):
@@ -346,10 +364,6 @@
     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'):
@@ -395,13 +409,18 @@
         """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`."""
-        return rescueBuilderIfLost(self, logger)
+        rescueBuilderIfLost(self, logger)
 
     def updateStatus(self, logger=None):
         """See `IBuilder`."""
-        return updateBuilderStatus(self, logger)
+        updateBuilderStatus(self, logger)
 
     def cleanSlave(self):
         """See IBuilder."""
@@ -421,23 +440,20 @@
     def resumeSlaveHost(self):
         """See IBuilder."""
         if not self.virtualized:
-            return defer.fail(CannotResumeHost('Builder is not virtualized.'))
+            raise CannotResumeHost('Builder is not virtualized.')
 
         if not self.vm_host:
-            return defer.fail(CannotResumeHost('Undefined vm_host.'))
+            raise CannotResumeHost('Undefined vm_host.')
 
         logger = self._getSlaveScannerLogger()
         logger.debug("Resuming %s (%s)" % (self.name, self.url))
 
-        d = self.slave.resume()
-        def got_resume_ok((stdout, stderr, returncode)):
-            return stdout, stderr
-        def got_resume_bad(failure):
-            stdout, stderr, code = failure.value
+        stdout, stderr, returncode = self.slave.resume()
+        if returncode != 0:
             raise CannotResumeHost(
                 "Resuming failed:\nOUT:\n%s\nERR:\n%s\n" % (stdout, stderr))
 
-        return d.addCallback(got_resume_ok).addErrback(got_resume_bad)
+        return stdout, stderr
 
     @cachedproperty
     def slave(self):
@@ -446,7 +462,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.makeBuilderSlave(self.url, self.vm_host)
+        return BuilderSlave.makeBlockingSlave(self.url, self.vm_host)
 
     def setSlaveForTesting(self, proxy):
         """See IBuilder."""
@@ -467,23 +483,18 @@
 
         # If we are building a virtual build, resume the virtual machine.
         if self.virtualized:
-            d = self.resumeSlaveHost()
-        else:
-            d = defer.succeed(None)
+            self.resumeSlaveHost()
 
-        def resume_done(ignored):
-            return self.current_build_behavior.dispatchBuildToSlave(
+        # Do it.
+        build_queue_item.markAsBuilding(self)
+        try:
+            self.current_build_behavior.dispatchBuildToSlave(
                 build_queue_item.id, logger)
-
-        def eb_slave_failure(failure):
-            failure.trap(BuildSlaveFailure)
-            e = failure.value
+        except BuildSlaveFailure, e:
+            logger.debug("Disabling builder: %s" % self.url, exc_info=1)
             self.failBuilder(
                 "Exception (%s) when setting up to new job" % (e,))
-
-        def eb_cannot_fetch_file(failure):
-            failure.trap(CannotFetchFile)
-            e = failure.value
+        except CannotFetchFile, e:
             message = """Slave '%s' (%s) was unable to fetch file.
             ****** URL ********
             %s
@@ -492,19 +503,10 @@
             *******************
             """ % (self.name, self.url, e.file_url, e.error_information)
             raise BuildDaemonError(message)
-
-        def eb_socket_error(failure):
-            failure.trap(socket.error)
-            e = failure.value
+        except socket.error, e:
             error_message = "Exception (%s) when setting up new job" % (e,)
-            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
+            self.handleTimeout(logger, error_message)
+            raise BuildSlaveFailure
 
     def failBuilder(self, reason):
         """See IBuilder"""
@@ -532,24 +534,22 @@
 
     def slaveStatus(self):
         """See IBuilder."""
-        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)
+        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
 
     def slaveStatusSentence(self):
         """See IBuilder."""
@@ -562,15 +562,13 @@
 
     def updateBuild(self, queueItem):
         """See `IBuilder`."""
-        return self.current_build_behavior.updateBuild(queueItem)
+        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
@@ -601,17 +599,18 @@
 
         return library_file.id
 
-    def isAvailable(self):
+    @property
+    def is_available(self):
         """See `IBuilder`."""
         if not self.builderok:
-            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)
+            return False
+        try:
+            slavestatus = self.slaveStatusSentence()
+        except (xmlrpclib.Fault, socket.error):
+            return False
+        if slavestatus[0] != BuilderStatus.IDLE:
+            return False
+        return True
 
     def _getSlaveScannerLogger(self):
         """Return the logger instance from buildd-slave-scanner.py."""
@@ -622,27 +621,6 @@
         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.
 
@@ -722,46 +700,52 @@
         :param candidate: The job to dispatch.
         """
         logger = self._getSlaveScannerLogger()
-        # Using maybeDeferred ensures that any exceptions are also
-        # wrapped up and caught later.
-        d = defer.maybeDeferred(self.startBuild, candidate, logger)
-        return d
+        try:
+            self.startBuild(candidate, logger)
+        except (BuildSlaveFailure, CannotBuild, BuildBehaviorMismatch), err:
+            logger.warn('Could not build: %s' % err)
 
     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)
-            d = self.resumeSlaveHost()
-            return d
-        else:
-            # XXX: This should really let the failure bubble up to the
-            # scan() method that does the failure counting.
+            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:
             # Mark builder as 'failed'.
             logger.warn(
-                "Disabling builder: %s -- %s" % (self.url, error_message))
+                "Disabling builder: %s -- %s" % (self.url, error_message),
+                exc_info=True)
             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.acquireBuildCandidate()
+        candidate = self._findBuildCandidate()
 
         if candidate is None:
             logger.debug("No build candidates available for builder.")
-            return defer.succeed(None)
+            return None
 
         if buildd_slave is not None:
             self.setSlaveForTesting(buildd_slave)
 
-        d = self._dispatchBuildCandidate(candidate)
-        return d.addCallback(lambda ignored: candidate)
+        self._dispatchBuildCandidate(candidate)
+        return candidate
 
     def getBuildQueue(self):
         """See `IBuilder`."""

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py	2010-10-20 11:54:27 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py	2010-12-07 16:29:13 +0000
@@ -16,18 +16,13 @@
 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 (
-    BuildSlaveFailure,
-    CorruptBuildCookie,
-    )
+from lp.buildmaster.interfaces.builder import CorruptBuildCookie
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     BuildBehaviorMismatch,
     IBuildFarmJobBehavior,
@@ -74,53 +69,54 @@
         """See `IBuildFarmJobBehavior`."""
         logger = logging.getLogger('slave-scanner')
 
-        d = self._builder.slaveStatus()
-
-        def got_failure(failure):
-            failure.trap(xmlrpclib.Fault, socket.error)
-            info = failure.value
+        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.
             info = ("Could not contact the builder %s, caught a (%s)"
                     % (queueItem.builder.url, info))
-            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
+            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)
 
     def updateBuild_IDLE(self, queueItem, slave_status, logtail, logger):
         """Somehow the builder forgot about the build job.
@@ -150,13 +146,11 @@
 
         Clean the builder for another jobs.
         """
-        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)
+        queueItem.builder.cleanSlave()
+        queueItem.builder = None
+        if queueItem.job.status != JobStatus.FAILED:
+            queueItem.job.fail()
+        queueItem.specific_job.jobAborted()
 
     def extractBuildStatus(self, slave_status):
         """Read build status name.
@@ -191,8 +185,6 @@
         # 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-26 20:43:50 +0000
+++ lib/lp/buildmaster/model/packagebuild.py	2010-12-07 16:29:13 +0000
@@ -163,8 +163,6 @@
     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,8 +178,6 @@
         # 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
@@ -278,8 +274,6 @@
             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-10-14 15:37:56 +0000
+++ lib/lp/buildmaster/tests/mock_slaves.py	2010-12-07 16:29:13 +0000
@@ -6,40 +6,21 @@
 __metaclass__ = type
 
 __all__ = [
-    'AbortedSlave',
-    'AbortingSlave',
+    'MockBuilder',
+    'LostBuildingBrokenSlave',
     'BrokenSlave',
+    'OkSlave',
     'BuildingSlave',
-    'CorruptBehavior',
-    'DeadProxy',
-    'LostBuildingBrokenSlave',
-    'MockBuilder',
-    'OkSlave',
-    'SlaveTestHelpers',
-    'TrivialBehavior',
+    'AbortedSlave',
     'WaitingSlave',
+    'AbortingSlave',
     ]
 
-import fixtures
-import os
-
 from StringIO import StringIO
 import xmlrpclib
 
-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.interfaces.builder import CannotFetchFile
 from lp.buildmaster.model.builder import (
-    BuilderSlave,
     rescueBuilderIfLost,
     updateBuilderStatus,
     )
@@ -78,9 +59,15 @@
             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):
@@ -90,10 +77,10 @@
         pass
 
     def rescueIfLost(self, logger=None):
-        return rescueBuilderIfLost(self, logger)
+        rescueBuilderIfLost(self, logger)
 
     def updateStatus(self, logger=None):
-        return defer.maybeDeferred(updateBuilderStatus, self, logger)
+        updateBuilderStatus(self, logger)
 
 
 # XXX: It would be *really* nice to run some set of tests against the real
@@ -108,44 +95,36 @@
         self.arch_tag = arch_tag
 
     def status(self):
-        return defer.succeed(('BuilderStatus.IDLE', ''))
+        return ('BuilderStatus.IDLE', '')
 
     def ensurepresent(self, sha1, url, user=None, password=None):
         self.call_log.append(('ensurepresent', url, user, password))
-        return defer.succeed((True, None))
+        return 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 defer.succeed(('BuildStatus.Building', info))
+        return ('BuildStatus.Building', info)
 
     def echo(self, *args):
         self.call_log.append(('echo',) + args)
-        return defer.succeed(args)
+        return 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 defer.succeed(('1.0', self.arch_tag, 'debian'))
-
-    def resume(self):
-        self.call_log.append('resume')
-        return defer.succeed(("", "", 0))
+        return ('1.0', self.arch_tag, 'debian')
 
     def sendFileToSlave(self, sha1, url, username="", password=""):
-        d = self.ensurepresent(sha1, url, username, password)
-        def check_present((present, info)):
-            if not present:
-                raise CannotFetchFile(url, info)
-        return d.addCallback(check_present)
+        present, info = self.ensurepresent(sha1, url, username, password)
+        if not present:
+            raise CannotFetchFile(url, info)
 
     def cacheFile(self, logger, libraryfilealias):
         return self.sendFileToSlave(
@@ -162,11 +141,9 @@
     def status(self):
         self.call_log.append('status')
         buildlog = xmlrpclib.Binary("This is a build log")
-        return defer.succeed(
-            ('BuilderStatus.BUILDING', self.build_id, buildlog))
+        return ('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")
@@ -178,15 +155,11 @@
     """A mock slave that looks like it's currently waiting."""
 
     def __init__(self, state='BuildStatus.OK', dependencies=None,
-                 build_id='1-1', filemap=None):
+                 build_id='1-1'):
         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.
@@ -194,12 +167,10 @@
 
     def status(self):
         self.call_log.append('status')
-        return defer.succeed((
-            'BuilderStatus.WAITING', self.state, self.build_id, self.filemap,
-            self.dependencies))
+        return ('BuilderStatus.WAITING', self.state, self.build_id, {},
+                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
@@ -213,19 +184,15 @@
 
     def status(self):
         self.call_log.append('status')
-        return defer.succeed(('BuilderStatus.ABORTING', '1-1'))
+        return ('BuilderStatus.ABORTING', '1-1')
 
 
 class AbortedSlave(OkSlave):
     """A mock slave that looks like it's aborted."""
 
-    def clean(self):
+    def status(self):
         self.call_log.append('status')
-        return defer.succeed(None)
-
-    def status(self):
-        self.call_log.append('clean')
-        return defer.succeed(('BuilderStatus.ABORTED', '1-1'))
+        return ('BuilderStatus.ABORTED', '1-1')
 
 
 class LostBuildingBrokenSlave:
@@ -239,108 +206,16 @@
 
     def status(self):
         self.call_log.append('status')
-        return defer.succeed(('BuilderStatus.BUILDING', '1000-10000'))
+        return ('BuilderStatus.BUILDING', '1000-10000')
 
     def abort(self):
         self.call_log.append('abort')
-        return defer.fail(xmlrpclib.Fault(8002, "Could not abort"))
+        raise 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')
-        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'})
+        raise xmlrpclib.Fault(8001, "Broken slave")

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2010-10-18 16:44:22 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2010-12-07 16:29:13 +0000
@@ -3,24 +3,20 @@
 
 """Test Builder features."""
 
+import errno
 import os
-import signal
+import socket
 import xmlrpclib
 
-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 testtools.content import Content
+from testtools.content_type import UTF8_TEXT
 
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.buildd.slave import BuilderStatus
-from canonical.config import config
+from canonical.buildd.tests.harness import BuilddSlaveTestSetup
 from canonical.database.sqlbase import flush_database_updates
-from canonical.launchpad.scripts import QuietFakeLogger
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR,
     IStoreSelector,
@@ -28,38 +24,21 @@
     )
 from canonical.testing.layers import (
     DatabaseFunctionalLayer,
-    LaunchpadZopelessLayer,
-    TwistedLaunchpadZopelessLayer,
-    TwistedLayer,
+    LaunchpadZopelessLayer
     )
 from lp.buildmaster.enums import BuildStatus
-from lp.buildmaster.interfaces.builder import (
-    CannotFetchFile,
-    IBuilder,
-    IBuilderSet,
-    )
+from lp.buildmaster.interfaces.builder import IBuilder, IBuilderSet
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     IBuildFarmJobBehavior,
     )
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
-from lp.buildmaster.interfaces.builder import CannotResumeHost
+from lp.buildmaster.model.builder import BuilderSlave
 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,
@@ -70,12 +49,9 @@
     )
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import (
-    ANONYMOUS,
-    login_as,
-    logout,
+    TestCase,
     TestCaseWithFactory,
     )
-from lp.testing.factory import LaunchpadObjectFactory
 from lp.testing.fakemethod import FakeMethod
 
 
@@ -116,121 +92,42 @@
         bq = builder.getBuildQueue()
         self.assertIs(None, bq)
 
-
-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_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)
 
     def test_slave(self):
         # Builder.slave is a BuilderSlave that points at the actual Builder.
@@ -239,147 +136,25 @@
         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
-        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)
+        builder.rescueIfLost()
+
+        self.assertEqual(1, aborted_slave.clean.call_count)
 
 
 class TestFindBuildCandidateBase(TestCaseWithFactory):
@@ -413,49 +188,6 @@
             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
@@ -588,16 +320,6 @@
         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):
 
@@ -610,14 +332,6 @@
         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):
 
@@ -760,48 +474,97 @@
             self.builder.current_build_behavior, BinaryPackageBuildBehavior)
 
 
-class TestSlave(TrialTestCase):
+class TestSlave(TestCase):
     """
     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.slave_helper.getClientSlave()
+        slave = self.getClientSlave()
         # We need to be in a BUILDING state before we can abort.
-        d = self.slave_helper.triggerGoodBuild(slave)
-        d.addCallback(lambda ignored: slave.abort())
-        d.addCallback(self.assertEqual, BuilderStatus.ABORTING)
-        return d
+        self.triggerGoodBuild(slave)
+        result = slave.abort()
+        self.assertEqual(result, BuilderStatus.ABORTING)
 
     def test_build(self):
         # Calling 'build' with an expected builder type, a good build id,
         # valid chroot & filemaps works and returns a BuilderStatus of
         # BUILDING.
         build_id = 'some-id'
-        slave = self.slave_helper.getClientSlave()
-        d = self.slave_helper.triggerGoodBuild(slave, build_id)
-        return d.addCallback(
-            self.assertEqual, [BuilderStatus.BUILDING, build_id])
+        slave = self.getClientSlave()
+        result = self.triggerGoodBuild(slave, build_id)
+        self.assertEqual([BuilderStatus.BUILDING, build_id], result)
 
     def test_clean(self):
-        slave = self.slave_helper.getClientSlave()
+        slave = self.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
@@ -811,248 +574,57 @@
     def test_echo(self):
         # Calling 'echo' contacts the server which returns the arguments we
         # gave it.
-        self.slave_helper.getServerSlave()
-        slave = self.slave_helper.getClientSlave()
-        d = slave.echo('foo', 'bar', 42)
-        return d.addCallback(self.assertEqual, ['foo', 'bar', 42])
+        self.getServerSlave()
+        slave = self.getClientSlave()
+        result = slave.echo('foo', 'bar', 42)
+        self.assertEqual(['foo', 'bar', 42], result)
 
     def test_info(self):
         # Calling 'info' gets some information about the slave.
-        self.slave_helper.getServerSlave()
-        slave = self.slave_helper.getClientSlave()
-        d = slave.info()
+        self.getServerSlave()
+        slave = self.getClientSlave()
+        result = slave.info()
         # We're testing the hard-coded values, since the version is hard-coded
         # into the remote slave, the supported build managers are hard-coded
         # into the tac file for the remote slave and config is returned from
         # the configuration file.
-        return d.addCallback(
-            self.assertEqual,
+        self.assertEqual(
             ['1.0',
              'i386',
              ['sourcepackagerecipe',
-              'translation-templates', 'binarypackage', 'debian']])
+              'translation-templates', 'binarypackage', 'debian']],
+            result)
 
     def test_initial_status(self):
         # Calling 'status' returns the current status of the slave. The
         # initial status is IDLE.
-        self.slave_helper.getServerSlave()
-        slave = self.slave_helper.getClientSlave()
-        d = slave.status()
-        return d.addCallback(self.assertEqual, [BuilderStatus.IDLE, ''])
+        self.getServerSlave()
+        slave = self.getClientSlave()
+        status = slave.status()
+        self.assertEqual([BuilderStatus.IDLE, ''], status)
 
     def test_status_after_build(self):
         # Calling 'status' returns the current status of the slave. After a
         # build has been triggered, the status is BUILDING.
-        slave = self.slave_helper.getClientSlave()
+        slave = self.getClientSlave()
         build_id = 'status-build-id'
-        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)
+        self.triggerGoodBuild(slave, build_id)
+        status = slave.status()
+        self.assertEqual([BuilderStatus.BUILDING, build_id], status[:2])
+        [log_file] = status[2:]
+        self.assertIsInstance(log_file, xmlrpclib.Binary)
 
     def test_ensurepresent_not_there(self):
         # ensurepresent checks to see if a file is there.
-        self.slave_helper.getServerSlave()
-        slave = self.slave_helper.getClientSlave()
-        d = slave.ensurepresent('blahblah', None, None, None)
-        d.addCallback(self.assertEqual, [False, 'No URL'])
-        return d
+        self.getServerSlave()
+        slave = self.getClientSlave()
+        result = slave.ensurepresent('blahblah', None, None, None)
+        self.assertEqual([False, 'No URL'], result)
 
     def test_ensurepresent_actually_there(self):
         # ensurepresent checks to see if a file is there.
-        tachandler = self.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)
+        tachandler = self.getServerSlave()
+        slave = self.getClientSlave()
+        self.makeCacheFile(tachandler, 'blahblah')
+        result = slave.ensurepresent('blahblah', None, None, None)
+        self.assertEqual([True, 'No URL'], result)

=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2010-10-19 13:58:21 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2010-12-07 16:29:13 +0000
@@ -6,7 +6,6 @@
 import os
 import signal
 import time
-import xmlrpclib
 
 import transaction
 
@@ -15,7 +14,9 @@
     reactor,
     task,
     )
+from twisted.internet.error import ConnectionClosed
 from twisted.internet.task import (
+    Clock,
     deferLater,
     )
 from twisted.python.failure import Failure
@@ -29,45 +30,577 @@
     ANONYMOUS,
     login,
     )
-from canonical.launchpad.scripts.logger import (
-    QuietFakeLogger,
-    )
+from canonical.launchpad.scripts.logger import BufferLogger
 from canonical.testing.layers import (
     LaunchpadScriptLayer,
-    TwistedLaunchpadZopelessLayer,
+    LaunchpadZopelessLayer,
     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 (
-    assessFailureCounts,
+    BaseDispatchResult,
+    buildd_success_result_map,
     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 (
-    BrokenSlave,
-    BuildingSlave,
-    OkSlave,
-    )
+from lp.buildmaster.tests.mock_slaves import BuildingSlave
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
-from lp.testing import TestCaseWithFactory
+from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
+from lp.testing import TestCase as LaunchpadTestCase
 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 = TwistedLaunchpadZopelessLayer
+    layer = LaunchpadZopelessLayer
 
     def setUp(self):
         """Setup TwistedLayer, TrialTestCase and BuilddSlaveTest.
@@ -75,18 +608,19 @@
         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()
@@ -94,7 +628,8 @@
         TwistedLayer.testTearDown()
 
     def _resetBuilder(self, builder):
-        """Reset the given builder and its job."""
+        """Reset the given builder and it's job."""
+        login('[e-mail]')
 
         builder.builderok = True
         job = builder.currentjob
@@ -102,6 +637,7 @@
             job.reset()
 
         transaction.commit()
+        login(ANONYMOUS)
 
     def assertBuildingJob(self, job, builder, logtail=None):
         """Assert the given job is building on the given builder."""
@@ -117,25 +653,55 @@
         self.assertEqual(build.status, BuildStatus.BUILDING)
         self.assertEqual(job.logtail, logtail)
 
-    def _getScanner(self, builder_name=None):
+    def _getManager(self):
         """Instantiate a SlaveScanner object.
 
         Replace its default logging handler by a testing version.
         """
-        if builder_name is None:
-            builder_name = BOB_THE_BUILDER_NAME
-        scanner = SlaveScanner(builder_name, QuietFakeLogger())
-        scanner.logger.name = 'slave-scanner'
+        manager = SlaveScanner(BOB_THE_BUILDER_NAME, BufferLogger())
+        manager.logger.name = 'slave-scanner'
 
-        return scanner
+        return manager
 
     def _checkDispatch(self, slave, builder):
-        # 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.")
+        """`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)
         self.assertEqual(0, builder.failure_count)
-        self.assertTrue(builder.currentjob is not None)
+
+        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.")
 
     def testScanDispatchForResetBuilder(self):
         # A job gets dispatched to the sampledata builder after it's reset.
@@ -143,27 +709,26 @@
         # 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.
-        self.layer.txn.commit()
-        self.layer.switchDbUser(config.builddmaster.dbuser)
-        scanner = self._getScanner()
-        d = defer.maybeDeferred(scanner.scan)
+        LaunchpadZopelessLayer.switchDbUser(config.builddmaster.dbuser)
+        manager = self._getManager()
+        d = defer.maybeDeferred(manager.scan)
         d.addCallback(self._checkDispatch, builder)
         return d
 
-    def _checkNoDispatch(self, slave, builder):
+    def _checkNoDispatch(self, recording_slave, builder):
         """Assert that no dispatch has occurred.
 
-        'slave' is None, so no interations would be passed
+        'recording_slave' is None, so no interations would be passed
         to the asynchonous dispatcher and the builder remained active
         and IDLE.
         """
-        self.assertTrue(slave is None, "Unexpected slave.")
+        self.assertTrue(
+            recording_slave is None, "Unexpected recording_slave.")
 
         builder = getUtility(IBuilderSet).get(builder.id)
         self.assertTrue(builder.builderok)
@@ -188,9 +753,9 @@
         login(ANONYMOUS)
 
         # Run 'scan' and check its result.
-        self.layer.switchDbUser(config.builddmaster.dbuser)
-        scanner = self._getScanner()
-        d = defer.maybeDeferred(scanner.singleCycle)
+        LaunchpadZopelessLayer.switchDbUser(config.builddmaster.dbuser)
+        manager = self._getManager()
+        d = defer.maybeDeferred(manager.scan)
         d.addCallback(self._checkNoDispatch, builder)
         return d
 
@@ -228,9 +793,9 @@
         login(ANONYMOUS)
 
         # Run 'scan' and check its result.
-        self.layer.switchDbUser(config.builddmaster.dbuser)
-        scanner = self._getScanner()
-        d = defer.maybeDeferred(scanner.scan)
+        LaunchpadZopelessLayer.switchDbUser(config.builddmaster.dbuser)
+        manager = self._getManager()
+        d = defer.maybeDeferred(manager.scan)
         d.addCallback(self._checkJobRescued, builder, job)
         return d
 
@@ -249,6 +814,8 @@
         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]
@@ -263,174 +830,188 @@
         self.assertBuildingJob(job, builder)
 
         # Run 'scan' and check its result.
-        self.layer.switchDbUser(config.builddmaster.dbuser)
-        scanner = self._getScanner()
-        d = defer.maybeDeferred(scanner.scan)
+        LaunchpadZopelessLayer.switchDbUser(config.builddmaster.dbuser)
+        manager = self._getManager()
+        d = defer.maybeDeferred(manager.scan)
         d.addCallback(self._checkJobUpdated, builder, job)
         return d
 
-    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):
+    def test_scan_assesses_failure_exceptions(self):
         # If scan() fails with an exception, failure_counts should be
-        # 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.
+        # incremented and tested.
         def failing_scan():
-            return defer.fail(Exception("fake exception"))
-        scanner = self._getScanner()
-        scanner.scan = failing_scan
+            raise Exception("fake exception")
+        manager = self._getManager()
+        manager.scan = failing_scan
+        manager.scheduleNextScanCycle = FakeMethod()
         from lp.buildmaster import manager as manager_module
         self.patch(manager_module, 'assessFailureCounts', FakeMethod())
-        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
+        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
         # exception.
-        d = scanner.singleCycle()
+        manager.startCycle()
 
         # Failure counts should be updated, and the assessment method
-        # 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)
+        # 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)
 
 
 class TestBuilddManager(TrialTestCase):
 
-    layer = TwistedLaunchpadZopelessLayer
+    layer = LaunchpadZopelessLayer
 
     def _stub_out_scheduleNextScanCycle(self):
         # stub out the code that adds a callLater, so that later tests
         # don't get surprises.
-        self.patch(SlaveScanner, 'startCycle', FakeMethod())
+        self.patch(SlaveScanner, 'scheduleNextScanCycle', FakeMethod())
 
     def test_addScanForBuilders(self):
         # Test that addScanForBuilders generates NewBuildersScanner objects.
@@ -459,62 +1040,10 @@
         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 = TwistedLaunchpadZopelessLayer
+    layer = LaunchpadZopelessLayer
 
     def _getScanner(self, manager=None, clock=None):
         return NewBuildersScanner(manager=manager, clock=clock)
@@ -555,8 +1084,11 @@
             new_builders, builder_scanner.checkForNewBuilders())
 
     def test_scan(self):
-        # See if scan detects new builders.
+        # See if scan detects new builders and schedules the next scan.
 
+        # 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"
 
@@ -572,6 +1104,9 @@
         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):
@@ -612,7 +1147,7 @@
     return False
 
 
-class TestBuilddManagerScript(TestCaseWithFactory):
+class TestBuilddManagerScript(LaunchpadTestCase):
 
     layer = LaunchpadScriptLayer
 
@@ -621,7 +1156,6 @@
         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-26 20:43:50 +0000
+++ lib/lp/buildmaster/tests/test_packagebuild.py	2010-12-07 16:29:13 +0000
@@ -97,8 +97,6 @@
         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,8 +309,6 @@
         # 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'},
                 })
@@ -323,8 +319,6 @@
     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'},
             })
@@ -335,8 +329,6 @@
     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'},
             })
@@ -347,8 +339,6 @@
         # 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'},
                 })
@@ -358,8 +348,6 @@
         # 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'},
                 })