Merge lp:~wgrant/launchpad/ppa-reset-2.0-basics into lp:launchpad

[William Grant]

Reset virtual builders a cycle before any dispatch attempt, and persist the clean status in the DB. Preparation for the new Scalingstack reset protocol.

[Colin Watson (cjwatson)]

Looks good to me; just one minor suggestion.

review: Approve

=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py	2014-05-10 18:40:36 +0000
+++ lib/lp/buildmaster/interactor.py	2014-06-18 08:49:42 +0000
@@ -21,6 +21,7 @@
     removeSecurityProxy,
     )
 
+from lp.buildmaster.enums import BuilderCleanStatus
 from lp.buildmaster.interfaces.builder import (
     BuildDaemonError,
     CannotFetchFile,
@@ -216,7 +217,7 @@
 BuilderVitals = namedtuple(
     'BuilderVitals',
     ('name', 'url', 'virtualized', 'vm_host', 'builderok', 'manual',
-     'build_queue', 'version'))
+     'build_queue', 'version', 'clean_status'))
 
 _BQ_UNSPECIFIED = object()
 
@@ -226,7 +227,8 @@
         build_queue = builder.currentjob
     return BuilderVitals(
         builder.name, builder.url, builder.virtualized, builder.vm_host,
-        builder.builderok, builder.manual, build_queue, builder.version)
+        builder.builderok, builder.manual, build_queue, builder.version,
+        builder.clean_status)
 
 
 class BuilderInteractor(object):
@@ -249,44 +251,6 @@
         return behaviour
 
     @classmethod
-    @defer.inlineCallbacks
-    def rescueIfLost(cls, vitals, slave, slave_status, expected_cookie,
-                     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 given cookie. 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.
-
-        :return: A Deferred that fires when the dialog with the slave is
-            finished.  Its return value is True if the slave is lost,
-            False otherwise.
-        """
-        # Determine the slave's current build cookie.
-        status = slave_status['builder_status']
-        slave_cookie = slave_status.get('build_id')
-
-        if slave_cookie == expected_cookie:
-            # The master and slave agree about the current job. Continue.
-            defer.returnValue(False)
-        else:
-            # The master and slave disagree. The master is our master,
-            # so try to rescue the slave.
-            # An IDLE slave doesn't need rescuing (SlaveScanner.scan
-            # will rescue the DB side instead), and we just have to wait
-            # out an ABORTING one.
-            if status == 'BuilderStatus.WAITING':
-                yield slave.clean()
-            elif status == 'BuilderStatus.BUILDING':
-                yield slave.abort()
-            if logger:
-                logger.info(
-                    "Builder slave '%s' rescued from %r (expected %r)." %
-                    (vitals.name, slave_cookie, expected_cookie))
-            defer.returnValue(True)
-
-    @classmethod
     def resumeSlaveHost(cls, vitals, slave):
         """Resume the slave host to a known good condition.
 
@@ -323,6 +287,49 @@
 
     @classmethod
     @defer.inlineCallbacks
+    def cleanSlave(cls, vitals, slave):
+        """Prepare a slave for a new build.
+
+        :return: A Deferred that fires when this stage of the resume
+            operations finishes. If the value is True, the slave is now clean.
+            If it's False, the clean is still in progress and this must be
+            called again later.
+        """
+        if vitals.virtualized:
+            # If we are building a virtual build, resume the virtual
+            # machine.  Before we try and contact the resumed slave,
+            # we're going to send it a message.  This is to ensure
+            # it's accepting packets from the outside world, because
+            # testing has shown that the first packet will randomly
+            # fail for no apparent reason.  This could be a quirk of
+            # the Xen guest, we're not sure.  We also don't care
+            # about the result from this message, just that it's
+            # sent, hence the "addBoth".  See bug 586359.
+            yield cls.resumeSlaveHost(vitals, slave)
+            yield slave.echo("ping")
+            defer.returnValue(True)
+        else:
+            slave_status = yield slave.status()
+            status = slave_status.get('builder_status', None)
+            if status == 'BuilderStatus.IDLE':
+                # This is as clean as we can get it.
+                defer.returnValue(True)
+            elif status == 'BuilderStatus.BUILDING':
+                # Asynchronously abort() the slave and wait until WAITING.
+                yield slave.abort()
+                defer.returnValue(False)
+            elif status == 'BuilderStatus.ABORTING':
+                # Wait it out until WAITING.
+                defer.returnValue(False)
+            elif status == 'BuilderStatus.WAITING':
+                # Just a synchronous clean() call and we'll be idle.
+                yield slave.clean()
+                defer.returnValue(True)
+            raise BuildDaemonError(
+                "Invalid status during clean: %r" % status)
+
+    @classmethod
+    @defer.inlineCallbacks
     def _startBuild(cls, build_queue_item, vitals, builder, slave, behaviour,
                     logger):
         """Start a build on this builder.
@@ -342,17 +349,12 @@
             raise BuildDaemonError(
                 "Attempted to start a build on a known-bad builder.")
 
-        # If we are building a virtual build, resume the virtual
-        # machine.  Before we try and contact the resumed slave, we're
-        # going to send it a message.  This is to ensure it's accepting
-        # packets from the outside world, because testing has shown that
-        # the first packet will randomly fail for no apparent reason.
-        # This could be a quirk of the Xen guest, we're not sure.  We
-        # also don't care about the result from this message, just that
-        # it's sent, hence the "addBoth".  See bug 586359.
-        if builder.virtualized:
-            yield cls.resumeSlaveHost(vitals, slave)
-            yield slave.echo("ping")
+        if builder.clean_status != BuilderCleanStatus.CLEAN:
+            raise BuildDaemonError(
+                "Attempted to start build on a dirty slave.")
+
+        builder.setCleanStatus(BuilderCleanStatus.DIRTY)
+        transaction.commit()
 
         yield behaviour.dispatchBuildToSlave(build_queue_item.id, logger)
 
@@ -448,9 +450,9 @@
         :return: A Deferred that fires when the slave dialog is finished.
         """
         # IDLE is deliberately not handled here, because it should be
-        # impossible to get past rescueIfLost unless the slave matches
-        # the DB, and this method isn't called unless the DB says
-        # there's a job.
+        # impossible to get past the cookie check unless the slave
+        # matches the DB, and this method isn't called unless the DB
+        # says there's a job.
         builder_status = slave_status['builder_status']
         if builder_status == 'BuilderStatus.BUILDING':
             # Build still building, collect the logtail.
 
=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2014-02-05 23:22:30 +0000
+++ lib/lp/buildmaster/manager.py	2014-06-18 08:49:42 +0000
@@ -26,6 +26,7 @@
 from zope.component import getUtility
 
 from lp.buildmaster.enums import (
+    BuilderCleanStatus,
     BuildQueueStatus,
     BuildStatus,
     )
@@ -413,70 +414,91 @@
         vitals = self.builder_factory.getVitals(self.builder_name)
         interactor = self.interactor_factory()
         slave = self.slave_factory(vitals)
-        if vitals.builderok:
-            self.logger.debug("Scanning %s." % self.builder_name)
-            slave_status = yield slave.status()
-        else:
-            slave_status = None
-
-        # Confirm that the DB and slave sides are in a valid, mutually
-        # agreeable state.
-        lost_reason = None
-        if not vitals.builderok:
-            lost_reason = '%s is disabled' % vitals.name
-        else:
-            self.updateVersion(vitals, slave_status)
-            cancelled = yield self.checkCancellation(vitals, slave, interactor)
-            if cancelled:
-                return
-            assert slave_status is not None
-            lost = yield interactor.rescueIfLost(
-                vitals, slave, slave_status, self.getExpectedCookie(vitals),
-                self.logger)
-            if lost:
-                lost_reason = '%s is lost' % vitals.name
-
-        # The slave is lost or the builder is disabled. We can't
-        # continue to update the job status or dispatch a new job, so
-        # just rescue the assigned job, if any, so it can be dispatched
-        # to another slave.
-        if lost_reason is not None:
-            if vitals.build_queue is not None:
+
+        if vitals.build_queue is not None:
+            if vitals.clean_status != BuilderCleanStatus.DIRTY:
+                # This is probably a grave bug with security implications,
+                # as a slave that has a job must be cleaned afterwards.
+                raise BuildDaemonError("Non-dirty builder allegedly building.")
+
+            lost_reason = None
+            if not vitals.builderok:
+                lost_reason = '%s is disabled' % vitals.name
+            else:
+                slave_status = yield slave.status()
+                # Ensure that the slave has the job that we think it
+                # should.
+                slave_cookie = slave_status.get('build_id')
+                expected_cookie = self.getExpectedCookie(vitals)
+                if slave_cookie != expected_cookie:
+                    lost_reason = (
+                        '%s is lost (expected %r, got %r)' % (
+                            vitals.name, expected_cookie, slave_cookie))
+
+            if lost_reason is not None:
+                # The slave is either confused or disabled, so reset and
+                # requeue the job. The next scan cycle will clean up the
+                # slave if appropriate.
                 self.logger.warn(
                     "%s. Resetting BuildQueue %d.", lost_reason,
                     vitals.build_queue.id)
                 vitals.build_queue.reset()
                 transaction.commit()
-            return
-
-        # We've confirmed that the slave state matches the DB. Continue
-        # with updating the job status, or dispatching a new job if the
-        # builder is idle.
-        if vitals.build_queue is not None:
-            # Scan the slave and get the logtail, or collect the build
-            # if it's ready.  Yes, "updateBuild" is a bad name.
+                return
+
+            cancelled = yield self.checkCancellation(
+                vitals, slave, interactor)
+            if cancelled:
+                return
+
+            # The slave and DB agree on the builder's state.  Scan the
+            # slave and get the logtail, or collect the build if it's
+            # ready.  Yes, "updateBuild" is a bad name.
             assert slave_status is not None
             yield interactor.updateBuild(
                 vitals, slave, slave_status, self.builder_factory,
                 self.behaviour_factory)
-        elif vitals.manual:
-            # If the builder is in manual mode, don't dispatch anything.
-            self.logger.debug(
-                '%s is in manual mode, not dispatching.' % vitals.name)
         else:
-            # See if there is a job we can dispatch to the builder slave.
-            builder = self.builder_factory[self.builder_name]
-            # Try to dispatch the job. If it fails, don't attempt to
-            # just retry the scan; we need to reset the job so the
-            # dispatch will be reattempted.
-            d = interactor.findAndStartJob(vitals, builder, slave)
-            d.addErrback(functools.partial(self._scanFailed, False))
-            yield d
-            if builder.currentjob is not None:
-                # After a successful dispatch we can reset the
-                # failure_count.
-                builder.resetFailureCount()
-                transaction.commit()
+            if not vitals.builderok:
+                return
+            # We think the builder is idle. If it's clean, dispatch. If
+            # it's dirty, clean.
+            if vitals.clean_status == BuilderCleanStatus.CLEAN:
+                slave_status = yield slave.status()
+                if slave_status.get('builder_status') != 'BuilderStatus.IDLE':
+                    raise BuildDaemonError(
+                        'Allegedly clean slave not idle (%r instead)'
+                        % slave_status.get('builder_status'))
+                self.updateVersion(vitals, slave_status)
+                if vitals.manual:
+                    # If the builder is in manual mode, don't dispatch
+                    # anything.
+                    self.logger.debug(
+                        '%s is in manual mode, not dispatching.', vitals.name)
+                    return
+                # Try to find and dispatch a job. If it fails, don't
+                # attempt to just retry the scan; we need to reset
+                # the job so the dispatch will be reattempted.
+                builder = self.builder_factory[self.builder_name]
+                d = interactor.findAndStartJob(vitals, builder, slave)
+                d.addErrback(functools.partial(self._scanFailed, False))
+                yield d
+                if builder.currentjob is not None:
+                    # After a successful dispatch we can reset the
+                    # failure_count.
+                    builder.resetFailureCount()
+                    transaction.commit()
+            else:
+                # Ask the BuilderInteractor to clean the slave. It might
+                # be immediately cleaned on return, in which case we go
+                # straight back to CLEAN, or we might have to spin
+                # through another few cycles.
+                done = yield interactor.cleanSlave(vitals, slave)
+                if done:
+                    builder = self.builder_factory[self.builder_name]
+                    builder.setCleanStatus(BuilderCleanStatus.CLEAN)
+                    self.logger.debug('%s has been cleaned.', vitals.name)
+                    transaction.commit()
 
 
 class NewBuildersScanner:
 
=== modified file 'lib/lp/buildmaster/tests/mock_slaves.py'
--- lib/lp/buildmaster/tests/mock_slaves.py	2014-06-06 10:32:55 +0000
+++ lib/lp/buildmaster/tests/mock_slaves.py	2014-06-18 08:49:42 +0000
@@ -30,6 +30,7 @@
 from twisted.internet import defer
 from twisted.web import xmlrpc
 
+from lp.buildmaster.enums import BuilderCleanStatus
 from lp.buildmaster.interactor import BuilderSlave
 from lp.buildmaster.interfaces.builder import CannotFetchFile
 from lp.services.config import config
@@ -48,7 +49,7 @@
 
     def __init__(self, name='mock-builder', builderok=True, manual=False,
                  virtualized=True, vm_host=None, url='http://fake:0000',
-                 version=None):
+                 version=None, clean_status=BuilderCleanStatus.DIRTY):
         self.currentjob = None
         self.builderok = builderok
         self.manual = manual
@@ -58,6 +59,10 @@
         self.vm_host = vm_host
         self.failnotes = None
         self.version = version
+        self.clean_status = clean_status
+
+    def setCleanStatus(self, clean_status):
+        self.clean_status = clean_status
 
     def failBuilder(self, reason):
         self.builderok = False
 
=== modified file 'lib/lp/buildmaster/tests/test_interactor.py'
--- lib/lp/buildmaster/tests/test_interactor.py	2014-06-06 11:15:29 +0000
+++ lib/lp/buildmaster/tests/test_interactor.py	2014-06-18 08:49:42 +0000
@@ -16,19 +16,24 @@
     SynchronousDeferredRunTest,
     )
 from testtools.matchers import ContainsAll
+from testtools.testcase import ExpectedException
 from twisted.internet import defer
 from twisted.internet.task import Clock
 from twisted.python.failure import Failure
 from twisted.web.client import getPage
 from zope.security.proxy import removeSecurityProxy
 
-from lp.buildmaster.enums import BuildStatus
+from lp.buildmaster.enums import (
+    BuilderCleanStatus,
+    BuildStatus,
+    )
 from lp.buildmaster.interactor import (
     BuilderInteractor,
     BuilderSlave,
     extract_vitals_from_db,
     )
 from lp.buildmaster.interfaces.builder import (
+    BuildDaemonError,
     CannotFetchFile,
     CannotResumeHost,
     )
@@ -150,94 +155,75 @@
         slave = BuilderInteractor.makeSlaveFromVitals(vitals)
         self.assertEqual(5, slave.timeout)
 
-    @defer.inlineCallbacks
-    def test_rescueIfLost_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.
+
+class TestBuilderInteractorCleanSlave(TestCase):
+
+    run_tests_with = AsynchronousDeferredRunTest
+
+    @defer.inlineCallbacks
+    def assertCleanCalls(self, builder, slave, calls, done):
+        actually_done = yield BuilderInteractor.cleanSlave(
+            extract_vitals_from_db(builder), slave)
+        self.assertEqual(done, actually_done)
+        self.assertEqual(calls, slave.method_log)
+
+    @defer.inlineCallbacks
+    def test_virtual(self):
+        # We don't care what the status of a virtual builder is; we just
+        # reset its VM and check that it's back.
+        yield self.assertCleanCalls(
+            MockBuilder(
+                virtualized=True, vm_host='lol',
+                clean_status=BuilderCleanStatus.DIRTY),
+            OkSlave(), ['resume', 'echo'], True)
+
+    @defer.inlineCallbacks
+    def test_nonvirtual_idle(self):
+        # An IDLE non-virtual slave is already as clean as we can get it.
+        yield self.assertCleanCalls(
+            MockBuilder(
+                virtualized=False, clean_status=BuilderCleanStatus.DIRTY),
+            OkSlave(), ['status'], True)
+
+    @defer.inlineCallbacks
+    def test_nonvirtual_building(self):
+        # A BUILDING non-virtual slave needs to be aborted. It'll go
+        # through ABORTING and eventually be picked up from WAITING.
+        yield self.assertCleanCalls(
+            MockBuilder(
+                virtualized=False, clean_status=BuilderCleanStatus.DIRTY),
+            BuildingSlave(), ['status', 'abort'], False)
+
+    @defer.inlineCallbacks
+    def test_nonvirtual_aborting(self):
+        # An ABORTING non-virtual slave must be waited out. It should
+        # hit WAITING eventually.
+        yield self.assertCleanCalls(
+            MockBuilder(
+                virtualized=False, clean_status=BuilderCleanStatus.DIRTY),
+            AbortingSlave(), ['status'], False)
+
+    @defer.inlineCallbacks
+    def test_nonvirtual_waiting(self):
+        # A WAITING non-virtual slave just needs clean() called.
+        yield self.assertCleanCalls(
+            MockBuilder(
+                virtualized=False, clean_status=BuilderCleanStatus.DIRTY),
+            WaitingSlave(), ['status', 'clean'], True)
+
+    @defer.inlineCallbacks
+    def test_nonvirtual_broken(self):
+        # A broken non-virtual builder is probably unrecoverable, so the
+        # method just crashes.
+        vitals = extract_vitals_from_db(MockBuilder(
+            virtualized=False, clean_status=BuilderCleanStatus.DIRTY))
         slave = LostBuildingBrokenSlave()
-        slave_status = yield slave.status()
         try:
-            yield BuilderInteractor.rescueIfLost(
-                extract_vitals_from_db(MockBuilder()), slave, slave_status,
-                'trivial')
+            yield BuilderInteractor.cleanSlave(vitals, slave)
         except xmlrpclib.Fault:
-            self.assertIn('abort', slave.call_log)
+            self.assertEqual(['status', 'abort'], slave.call_log)
         else:
-            self.fail("xmlrpclib.Fault not raised")
-
-    @defer.inlineCallbacks
-    def test_recover_idle_slave(self):
-        # An idle slave is not rescued, even if it's not meant to be
-        # idle. SlaveScanner.scan() will clean up the DB side, because
-        # we still report that it's lost.
-        slave = OkSlave()
-        slave_status = yield slave.status()
-        lost = yield BuilderInteractor.rescueIfLost(
-            extract_vitals_from_db(MockBuilder()), slave, slave_status,
-            'trivial')
-        self.assertTrue(lost)
-        self.assertEqual(['status'], slave.call_log)
-
-    @defer.inlineCallbacks
-    def test_recover_ok_slave(self):
-        # An idle slave that's meant to be idle is not rescued.
-        slave = OkSlave()
-        slave_status = yield slave.status()
-        lost = yield BuilderInteractor.rescueIfLost(
-            extract_vitals_from_db(MockBuilder()), slave, slave_status, None)
-        self.assertFalse(lost)
-        self.assertEqual(['status'], slave.call_log)
-
-    @defer.inlineCallbacks
-    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(build_id='trivial')
-        slave_status = yield waiting_slave.status()
-        lost = yield BuilderInteractor.rescueIfLost(
-            extract_vitals_from_db(MockBuilder()), waiting_slave, slave_status,
-            'trivial')
-        self.assertFalse(lost)
-        self.assertEqual(['status'], waiting_slave.call_log)
-
-    @defer.inlineCallbacks
-    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(build_id='non-trivial')
-        slave_status = yield waiting_slave.status()
-        lost = yield BuilderInteractor.rescueIfLost(
-            extract_vitals_from_db(MockBuilder()), waiting_slave, slave_status,
-            'trivial')
-        self.assertTrue(lost)
-        self.assertEqual(['status', 'clean'], waiting_slave.call_log)
-
-    @defer.inlineCallbacks
-    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(build_id='trivial')
-        slave_status = yield building_slave.status()
-        lost = yield BuilderInteractor.rescueIfLost(
-            extract_vitals_from_db(MockBuilder()), building_slave,
-            slave_status, 'trivial')
-        self.assertFalse(lost)
-        self.assertEqual(['status'], building_slave.call_log)
-
-    @defer.inlineCallbacks
-    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(build_id='non-trivial')
-        slave_status = yield building_slave.status()
-        lost = yield BuilderInteractor.rescueIfLost(
-            extract_vitals_from_db(MockBuilder()), building_slave,
-            slave_status, 'trivial')
-        self.assertTrue(lost)
-        self.assertEqual(['status', 'abort'], building_slave.call_log)
+            self.fail("abort() should crash.")
 
 
 class TestBuilderSlaveStatus(TestCase):
@@ -321,6 +307,7 @@
         processor = self.factory.makeProcessor(name="i386")
         builder = self.factory.makeBuilder(
             processors=[processor], virtualized=True, vm_host="bladh")
+        builder.setCleanStatus(BuilderCleanStatus.CLEAN)
         self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(OkSlave()))
         distroseries = self.factory.makeDistroSeries()
         das = self.factory.makeDistroArchSeries(
@@ -377,21 +364,30 @@
 
         return d.addCallback(check_build_started)
 
-    def test_virtual_job_dispatch_pings_before_building(self):
-        # We need to send a ping to the builder to work around a bug
-        # where sometimes the first network packet sent is dropped.
-        builder, build = self._setupBinaryBuildAndBuilder()
-        candidate = build.queueBuild()
-        removeSecurityProxy(builder)._findBuildCandidate = FakeMethod(
-            result=candidate)
-        vitals = extract_vitals_from_db(builder)
-        slave = OkSlave()
-        d = BuilderInteractor.findAndStartJob(vitals, builder, slave)
-
-        def check_build_started(candidate):
-            self.assertIn(('echo', 'ping'), slave.call_log)
-
-        return d.addCallback(check_build_started)
+    @defer.inlineCallbacks
+    def test_findAndStartJob_requires_clean_slave(self):
+        # findAndStartJob ensures that its slave starts CLEAN.
+        builder, build = self._setupBinaryBuildAndBuilder()
+        builder.setCleanStatus(BuilderCleanStatus.DIRTY)
+        candidate = build.queueBuild()
+        removeSecurityProxy(builder)._findBuildCandidate = FakeMethod(
+            result=candidate)
+        vitals = extract_vitals_from_db(builder)
+        with ExpectedException(
+                BuildDaemonError,
+                "Attempted to start build on a dirty slave."):
+            yield BuilderInteractor.findAndStartJob(vitals, builder, OkSlave())
+
+    @defer.inlineCallbacks
+    def test_findAndStartJob_dirties_slave(self):
+        # findAndStartJob marks its builder DIRTY before dispatching.
+        builder, build = self._setupBinaryBuildAndBuilder()
+        candidate = build.queueBuild()
+        removeSecurityProxy(builder)._findBuildCandidate = FakeMethod(
+            result=candidate)
+        vitals = extract_vitals_from_db(builder)
+        yield BuilderInteractor.findAndStartJob(vitals, builder, OkSlave())
+        self.assertEqual(BuilderCleanStatus.DIRTY, builder.clean_status)
 
 
 class TestSlave(TestCase):
 
=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2014-06-06 10:32:55 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2014-06-18 08:49:42 +0000
@@ -13,6 +13,7 @@
     AsynchronousDeferredRunTest,
     )
 from testtools.matchers import Equals
+from testtools.testcase import ExpectedException
 import transaction
 from twisted.internet import (
     defer,
@@ -25,6 +26,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.buildmaster.enums import (
+    BuilderCleanStatus,
     BuildQueueStatus,
     BuildStatus,
     )
@@ -33,7 +35,10 @@
     BuilderSlave,
     extract_vitals_from_db,
     )
-from lp.buildmaster.interfaces.builder import IBuilderSet
+from lp.buildmaster.interfaces.builder import (
+    BuildDaemonError,
+    IBuilderSet,
+    )
 from lp.buildmaster.interfaces.buildfarmjobbehaviour import (
     IBuildFarmJobBehaviour,
     )
@@ -113,6 +118,7 @@
         job = builder.currentjob
         if job is not None:
             job.reset()
+        builder.setCleanStatus(BuilderCleanStatus.CLEAN)
 
         transaction.commit()
 
@@ -155,6 +161,7 @@
         # Set this to 1 here so that _checkDispatch can make sure it's
         # reset to 0 after a successful dispatch.
         builder.failure_count = 1
+        builder.setCleanStatus(BuilderCleanStatus.CLEAN)
 
         # Run 'scan' and check its result.
         switch_dbuser(config.builddmaster.dbuser)
@@ -258,6 +265,7 @@
     def test_scan_with_nothing_to_dispatch(self):
         factory = LaunchpadObjectFactory()
         builder = factory.makeBuilder()
+        builder.setCleanStatus(BuilderCleanStatus.CLEAN)
         self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(OkSlave()))
         transaction.commit()
         scanner = self._getScanner(builder_name=builder.name)
@@ -422,9 +430,9 @@
         self.assertFalse(builder.builderok)
 
     @defer.inlineCallbacks
-    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()
+    def test_fail_to_resume_leaves_it_dirty(self):
+        # If an attempt to resume a slave fails, its failure count is
+        # incremented and it is left DIRTY.
 
         # Make a slave with a failing resume() method.
         slave = OkSlave()
@@ -435,26 +443,21 @@
         builder = removeSecurityProxy(
             getUtility(IBuilderSet)[BOB_THE_BUILDER_NAME])
         self._resetBuilder(builder)
+        builder.setCleanStatus(BuilderCleanStatus.DIRTY)
+        builder.virtualized = True
         self.assertEqual(0, builder.failure_count)
         self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(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
         transaction.commit()
-        yield scanner.singleCycle()
-
-        # The failure_count will have been incremented on the builder, we
-        # can check that to see that a dispatch attempt did indeed occur.
+
+        # A spin of the scanner will see the DIRTY builder and reset it.
+        # Our patched reset will fail.
+        yield self._getScanner().singleCycle()
+
+        # The failure_count will have been incremented on the builder,
+        # and it will be left DIRTY.
         self.assertEqual(1, builder.failure_count)
-        # There should also be no builder set on the job.
-        self.assertIsNone(job.builder)
-        build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(job)
-        self.assertEqual(build.status, BuildStatus.NEEDSBUILD)
+        self.assertEqual(BuilderCleanStatus.DIRTY, builder.clean_status)
 
     @defer.inlineCallbacks
     def test_update_slave_version(self):
@@ -569,15 +572,25 @@
             builder.name, BuilderFactory(), BufferLogger(),
             slave_factory=get_slave, clock=clock)
 
-        # The slave is idle and there's a build candidate, so the first
-        # scan will reset the builder and dispatch the build.
+        # The slave is idle and dirty, so the first scan will clean it
+        # with a reset.
+        self.assertEqual(BuilderCleanStatus.DIRTY, builder.clean_status)
+        yield scanner.scan()
+        self.assertEqual(['resume', 'echo'], get_slave.result.method_log)
+        self.assertEqual(BuilderCleanStatus.CLEAN, builder.clean_status)
+        self.assertIs(None, builder.currentjob)
+
+        # The slave is idle and clean, and there's a build candidate, so
+        # the next scan will dispatch the build.
+        get_slave.result = OkSlave()
         yield scanner.scan()
         self.assertEqual(
-            ['status', 'resume', 'echo', 'ensurepresent', 'build'],
+            ['status', 'ensurepresent', 'build'],
             get_slave.result.method_log)
         self.assertEqual(bq, builder.currentjob)
         self.assertEqual(BuildQueueStatus.RUNNING, bq.status)
         self.assertEqual(BuildStatus.BUILDING, build.status)
+        self.assertEqual(BuilderCleanStatus.DIRTY, builder.clean_status)
 
         # build() has been called, so switch in a BUILDING slave.
         # Scans will now just do a status() each, as the logtail is
@@ -596,7 +609,8 @@
         # When the build finishes, the scanner will notice, call
         # handleStatus(), and then clean the builder.  Our fake
         # _handleStatus_OK doesn't do anything special, but there'd
-        # usually be file retrievals in the middle.
+        # usually be file retrievals in the middle. The builder remains
+        # dirty afterward.
         get_slave.result = WaitingSlave(
             build_id=IBuildFarmJobBehaviour(build).getBuildCookie())
         yield scanner.scan()
@@ -604,13 +618,15 @@
         self.assertIs(None, builder.currentjob)
         self.assertEqual(BuildStatus.UPLOADING, build.status)
         self.assertEqual(builder, build.builder)
+        self.assertEqual(BuilderCleanStatus.DIRTY, builder.clean_status)
 
-        # We're clean, so let's flip back to an idle slave and
-        # confirm that a scan does nothing special.
+        # We're idle and dirty, so let's flip back to an idle slave and
+        # confirm that the slave gets cleaned.
         get_slave.result = OkSlave()
         yield scanner.scan()
-        self.assertEqual(['status'], get_slave.result.method_log)
+        self.assertEqual(['resume', 'echo'], get_slave.result.method_log)
         self.assertIs(None, builder.currentjob)
+        self.assertEqual(BuilderCleanStatus.CLEAN, builder.clean_status)
 
 
 class TestPrefetchedBuilderFactory(TestCaseWithFactory):
@@ -740,74 +756,104 @@
 
     run_tests_with = AsynchronousDeferredRunTest
 
+    def getScanner(self, builder_factory=None, interactor=None, slave=None,
+                   behaviour=None):
+        if builder_factory is None:
+            builder_factory = MockBuilderFactory(
+                MockBuilder(virtualized=False), None)
+        if interactor is None:
+            interactor = BuilderInteractor()
+            interactor.updateBuild = FakeMethod()
+        if slave is None:
+            slave = OkSlave()
+        if behaviour is None:
+            behaviour = TrivialBehaviour()
+        return SlaveScanner(
+            'mock', builder_factory, BufferLogger(),
+            interactor_factory=FakeMethod(interactor),
+            slave_factory=FakeMethod(slave),
+            behaviour_factory=FakeMethod(behaviour))
+
     @defer.inlineCallbacks
     def test_scan_with_job(self):
         # SlaveScanner.scan calls updateBuild() when a job is building.
         slave = BuildingSlave('trivial')
         bq = FakeBuildQueue()
-
-        # Instrument updateBuild.
-        interactor = BuilderInteractor()
-        interactor.updateBuild = FakeMethod()
-
-        scanner = SlaveScanner(
-            'mock', MockBuilderFactory(MockBuilder(), bq), BufferLogger(),
-            interactor_factory=FakeMethod(interactor),
-            slave_factory=FakeMethod(slave),
-            behaviour_factory=FakeMethod(TrivialBehaviour()))
+        scanner = self.getScanner(
+            builder_factory=MockBuilderFactory(MockBuilder(), bq),
+            slave=slave)
 
         yield scanner.scan()
         self.assertEqual(['status'], slave.call_log)
-        self.assertEqual(1, interactor.updateBuild.call_count)
+        self.assertEqual(
+            1, scanner.interactor_factory.result.updateBuild.call_count)
         self.assertEqual(0, bq.reset.call_count)
 
     @defer.inlineCallbacks
-    def test_scan_aborts_lost_slave_with_job(self):
-        # SlaveScanner.scan uses BuilderInteractor.rescueIfLost to abort
-        # slaves that don't have the expected job.
+    def test_scan_recovers_lost_slave_with_job(self):
+        # SlaveScanner.scan identifies slaves that aren't building what
+        # they should be, resets the jobs, and then aborts the slaves.
         slave = BuildingSlave('nontrivial')
         bq = FakeBuildQueue()
-
-        # Instrument updateBuild.
-        interactor = BuilderInteractor()
-        interactor.updateBuild = FakeMethod()
-
-        scanner = SlaveScanner(
-            'mock', MockBuilderFactory(MockBuilder(), bq), BufferLogger(),
-            interactor_factory=FakeMethod(interactor),
-            slave_factory=FakeMethod(slave),
-            behaviour_factory=FakeMethod(TrivialBehaviour()))
+        builder = MockBuilder(virtualized=False)
+        scanner = self.getScanner(
+            builder_factory=MockBuilderFactory(builder, bq),
+            slave=slave)
 
         # A single scan will call status(), notice that the slave is lost,
-        # abort() the slave, then reset() the job without calling
-        # updateBuild().
+        # and reset() the job without calling updateBuild().
         yield scanner.scan()
-        self.assertEqual(['status', 'abort'], slave.call_log)
-        self.assertEqual(0, interactor.updateBuild.call_count)
+        self.assertEqual(['status'], slave.call_log)
+        self.assertEqual(
+            0, scanner.interactor_factory.result.updateBuild.call_count)
         self.assertEqual(1, bq.reset.call_count)
+        # The reset would normally have unset build_queue.
+        scanner.builder_factory.updateTestData(builder, None)
+
+        # The next scan will see a dirty idle builder with a BUILDING
+        # slave, and abort() it.
+        yield scanner.scan()
+        self.assertEqual(['status', 'status', 'abort'], slave.call_log)
 
     @defer.inlineCallbacks
-    def test_scan_aborts_lost_slave_when_idle(self):
-        # SlaveScanner.scan uses BuilderInteractor.rescueIfLost to abort
-        # slaves that aren't meant to have a job.
+    def test_scan_recovers_lost_slave_when_idle(self):
+        # SlaveScanner.scan identifies slaves that are building when
+        # they shouldn't be and aborts them.
         slave = BuildingSlave()
-
-        # Instrument updateBuild.
-        interactor = BuilderInteractor()
-        interactor.updateBuild = FakeMethod()
-
-        scanner = SlaveScanner(
-            'mock', MockBuilderFactory(MockBuilder(), None), BufferLogger(),
-            interactor_factory=FakeMethod(interactor),
-            slave_factory=FakeMethod(slave),
-            behaviour_factory=FakeMethod(None))
-
-        # A single scan will call status(), notice that the slave is lost,
-        # abort() the slave, then reset() the job without calling
-        # updateBuild().
+        scanner = self.getScanner(slave=slave)
         yield scanner.scan()
         self.assertEqual(['status', 'abort'], slave.call_log)
-        self.assertEqual(0, interactor.updateBuild.call_count)
+
+    @defer.inlineCallbacks
+    def test_scan_building_but_not_dirty_builder_explodes(self):
+        # Builders with a build assigned must be dirty for safety
+        # reasons. If we run into one that's clean, we blow up.
+        slave = BuildingSlave()
+        builder = MockBuilder(clean_status=BuilderCleanStatus.CLEAN)
+        bq = FakeBuildQueue()
+        scanner = self.getScanner(
+            slave=slave, builder_factory=MockBuilderFactory(builder, bq))
+
+        with ExpectedException(
+                BuildDaemonError, "Non-dirty builder allegedly building."):
+            yield scanner.scan()
+        self.assertEqual([], slave.call_log)
+
+    @defer.inlineCallbacks
+    def test_scan_clean_but_not_idle_slave_explodes(self):
+        # Clean builders by definition have slaves that are idle. If
+        # an ostensibly clean slave isn't idle, blow up.
+        slave = BuildingSlave()
+        builder = MockBuilder(clean_status=BuilderCleanStatus.CLEAN)
+        scanner = self.getScanner(
+            slave=slave, builder_factory=MockBuilderFactory(builder, None))
+
+        with ExpectedException(
+                BuildDaemonError,
+                "Allegedly clean slave not idle "
+                "\('BuilderStatus.BUILDING' instead\)"):
+            yield scanner.scan()
+        self.assertEqual(['status'], slave.call_log)
 
     def test_getExpectedCookie_caches(self):
         bf = MockBuilderFactory(MockBuilder(), FakeBuildQueue())
 
=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py'
--- lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py	2014-06-06 10:32:55 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuildbehaviour.py	2014-06-18 08:49:42 +0000
@@ -19,6 +19,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from lp.buildmaster.enums import (
+    BuilderCleanStatus,
     BuildQueueStatus,
     BuildStatus,
     )
@@ -131,10 +132,7 @@
         build_log = [
             ('build', cookie, 'binarypackage', chroot.content.sha1,
              filemap_names, extra_args)]
-        if builder.virtualized:
-            result = [('echo', 'ping')] + upload_logs + build_log
-        else:
-            result = upload_logs + build_log
+        result = upload_logs + build_log
         return result
 
     def test_non_virtual_ppa_dispatch(self):
@@ -146,6 +144,7 @@
         archive = self.factory.makeArchive(virtualized=False)
         slave = OkSlave()
         builder = self.factory.makeBuilder(virtualized=False)
+        builder.setCleanStatus(BuilderCleanStatus.CLEAN)
         vitals = extract_vitals_from_db(builder)
         build = self.factory.makeBinaryPackageBuild(
             builder=builder, archive=archive)
@@ -164,12 +163,11 @@
         return d
 
     def test_virtual_ppa_dispatch(self):
-        # Make sure the builder slave gets reset before a build is
-        # dispatched to it.
         archive = self.factory.makeArchive(virtualized=True)
         slave = OkSlave()
         builder = self.factory.makeBuilder(
             virtualized=True, vm_host="foohost")
+        builder.setCleanStatus(BuilderCleanStatus.CLEAN)
         vitals = extract_vitals_from_db(builder)
         build = self.factory.makeBinaryPackageBuild(
             builder=builder, archive=archive)
@@ -182,22 +180,17 @@
         d = interactor._startBuild(
             bq, vitals, builder, slave,
             interactor.getBuildBehaviour(bq, builder, slave), BufferLogger())
-
-        def check_build(ignored):
-            # We expect the first call to the slave to be a resume call,
-            # followed by the rest of the usual calls we expect.
-            expected_resume_call = slave.call_log.pop(0)
-            self.assertEqual('resume', expected_resume_call)
-            self.assertExpectedInteraction(
-                ignored, slave.call_log, builder, build, lf, archive,
-                ArchivePurpose.PPA)
-        return d.addCallback(check_build)
+        d.addCallback(
+            self.assertExpectedInteraction, slave.call_log, builder, build,
+            lf, archive, ArchivePurpose.PPA)
+        return d
 
     def test_partner_dispatch_no_publishing_history(self):
         archive = self.factory.makeArchive(
             virtualized=False, purpose=ArchivePurpose.PARTNER)
         slave = OkSlave()
         builder = self.factory.makeBuilder(virtualized=False)
+        builder.setCleanStatus(BuilderCleanStatus.CLEAN)
         vitals = extract_vitals_from_db(builder)
         build = self.factory.makeBinaryPackageBuild(
             builder=builder, archive=archive)