← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~wgrant/launchpad/bm-buildercache into lp:launchpad

 

William Grant has proposed merging lp:~wgrant/launchpad/bm-buildercache into lp:launchpad.

Commit message:
Port BuilddManager, SlaveScanner and BuilderInteractor to more DB-agnostic abstracted classes.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bm-buildercache/+merge/185968

To reduce buildd-manager's ridiculous transaction frequency, BuilderInteractor needs to be able to operate largely without touching the DB. But it often needs to grab attributes of Builder, which will hit the DB if the object hasn't been retrieved in the current transaction.

This branch introduces a BuilderVitals namedtuple which stores the relevant bits for the duration of a scan. This doesn't reduce the transaction frequency yet, but in a later branch we'll retrieve these in a bulk query and BuilderInteractor can avoid the DB entirely. buildd-manager bits also now get their Builders and BuilderVitals from a BuildersCache object, which can be more easily mocked out by tests to avoid DB access.
-- 
https://code.launchpad.net/~wgrant/launchpad/bm-buildercache/+merge/185968
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bm-buildercache into lp:launchpad.
=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py	2013-09-05 22:22:47 +0000
+++ lib/lp/buildmaster/interactor.py	2013-09-17 05:16:04 +0000
@@ -5,8 +5,10 @@
 
 __all__ = [
     'BuilderInteractor',
+    'extract_vitals_from_db',
     ]
 
+from collections import namedtuple
 import logging
 from urlparse import urlparse
 
@@ -210,6 +212,19 @@
             'build', buildid, builder_type, chroot_sha1, filemap, args))
 
 
+BuilderVitals = namedtuple(
+    'BuilderVitals',
+    ('name', 'url', 'virtualized', 'vm_host', 'builderok', 'manual',
+     'build_queue'))
+
+
+def extract_vitals_from_db(builder, build_queue=None):
+    return BuilderVitals(
+        builder.name, builder.url, builder.virtualized, builder.vm_host,
+        builder.builderok, builder.manual,
+        build_queue or builder.currentjob)
+
+
 class BuilderInteractor(object):
 
     _cached_build_behavior = None
@@ -227,6 +242,9 @@
         self._override_slave = override_slave
         self._override_behavior = override_behavior
 
+        # XXX wgrant: The BuilderVitals should be passed in.
+        self.vitals = extract_vitals_from_db(builder)
+
     @property
     def slave(self):
         """See IBuilder."""
@@ -235,14 +253,14 @@
         # The slave cache is invalidated when the builder's URL, VM host
         # or virtualisation change.
         new_slave_attrs = (
-            self.builder.url, self.builder.vm_host, self.builder.virtualized)
+            self.vitals.url, self.vitals.vm_host, self.vitals.virtualized)
         if self._cached_slave_attrs != new_slave_attrs:
-            if self.builder.virtualized:
+            if self.vitals.virtualized:
                 timeout = config.builddmaster.virtualized_socket_timeout
             else:
                 timeout = config.builddmaster.socket_timeout
             self._cached_slave = BuilderSlave.makeBuilderSlave(
-                self.builder.url, self.builder.vm_host, timeout)
+                self.vitals.url, self.vitals.vm_host, timeout)
             self._cached_slave_attrs = new_slave_attrs
         return self._cached_slave
 
@@ -286,13 +304,13 @@
                 status['logtail'] = status_sentence[2]
         defer.returnValue((status_sentence, status))
 
-    def verifySlaveBuildCookie(self, slave_cookie):
+    def verifySlaveBuildCookie(self, behavior, slave_cookie):
         """See `IBuildFarmJobBehavior`."""
-        if self._current_build_behavior is None:
+        if behavior is None:
             if slave_cookie is not None:
                 raise CorruptBuildCookie('Slave building when should be idle.')
         else:
-            good_cookie = self._current_build_behavior.getBuildCookie()
+            good_cookie = behavior.getBuildCookie()
             if slave_cookie != good_cookie:
                 raise CorruptBuildCookie(
                     "Invalid slave build cookie: got %r, expected %r."
@@ -337,7 +355,8 @@
         # verifying that the slave cookie is None iff we expect the
         # slave to be idle.
         try:
-            self.verifySlaveBuildCookie(slave_cookie)
+            self.verifySlaveBuildCookie(
+                self._current_build_behavior, slave_cookie)
             defer.returnValue(False)
         except CorruptBuildCookie as reason:
             # An IDLE slave doesn't need rescuing (SlaveScanner.scan
@@ -350,7 +369,7 @@
             if logger:
                 logger.info(
                     "Builder '%s' rescued from '%s': '%s'" %
-                    (self.builder.name, slave_cookie, reason))
+                    (self.vitals.name, slave_cookie, reason))
             defer.returnValue(True)
 
     def cleanSlave(self):
@@ -386,14 +405,14 @@
             whose value is a (stdout, stderr) tuple for success, or a Failure
             whose value is a CannotResumeHost exception.
         """
-        if not self.builder.virtualized:
+        if not self.vitals.virtualized:
             return defer.fail(CannotResumeHost('Builder is not virtualized.'))
 
-        if not self.builder.vm_host:
+        if not self.vitals.vm_host:
             return defer.fail(CannotResumeHost('Undefined vm_host.'))
 
         logger = self._getSlaveScannerLogger()
-        logger.info("Resuming %s (%s)" % (self.builder.name, self.builder.url))
+        logger.info("Resuming %s (%s)" % (self.vitals.name, self.vitals.url))
 
         d = self.slave.resume()
 
@@ -408,7 +427,7 @@
         return d.addCallback(got_resume_ok).addErrback(got_resume_bad)
 
     @defer.inlineCallbacks
-    def _startBuild(self, build_queue_item, logger):
+    def _startBuild(self, build_queue_item, behavior, logger):
         """Start a build on this builder.
 
         :param build_queue_item: A BuildQueueItem to build.
@@ -418,14 +437,8 @@
             value is None, or a Failure that contains an exception
             explaining what went wrong.
         """
-        needed_bfjb = type(removeSecurityProxy(
-            IBuildFarmJobBehavior(build_queue_item.specific_job)))
-        if not zope_isinstance(self._current_build_behavior, needed_bfjb):
-            raise AssertionError(
-                "Inappropriate IBuildFarmJobBehavior: %r is not a %r" %
-                (self._current_build_behavior, needed_bfjb))
-        self._current_build_behavior.logStartBuild(logger)
-        self._current_build_behavior.verifyBuildRequest(logger)
+        behavior.logStartBuild(logger)
+        behavior.verifyBuildRequest(logger)
 
         # Set the build behavior depending on the provided build queue item.
         if not self.builder.builderok:
@@ -444,8 +457,7 @@
             yield self.resumeSlaveHost()
             yield self.slave.echo("ping")
 
-        yield self._current_build_behavior.dispatchBuildToSlave(
-            build_queue_item.id, logger)
+        yield behavior.dispatchBuildToSlave(build_queue_item.id, logger)
 
     def resetOrFail(self, logger, exception):
         """Handle "confirmed" build slave failures.
@@ -466,14 +478,14 @@
             or immediately if it's a non-virtual slave.
         """
         error_message = str(exception)
-        if self.builder.virtualized:
+        if self.vitals.virtualized:
             # Virtualized/PPA builder: attempt a reset, unless the failure
             # was itself a failure to reset.  (In that case, the slave
             # scanner will try again until we reach the failure threshold.)
             if not isinstance(exception, CannotResumeHost):
                 logger.warn(
                     "Resetting builder: %s -- %s" % (
-                        self.builder.url, error_message),
+                        self.vitals.url, error_message),
                     exc_info=True)
                 return self.resumeSlaveHost()
         else:
@@ -482,8 +494,8 @@
             # Mark builder as 'failed'.
             logger.warn(
                 "Disabling builder: %s -- %s" % (
-                    self.builder.url, error_message))
-            self.builder.failBuilder(error_message)
+                    self.vitals.url, error_message))
+            self.vitals.failBuilder(error_message)
             transaction.commit()
         return defer.succeed(None)
 
@@ -503,7 +515,13 @@
             logger.debug("No build candidates available for builder.")
             defer.returnValue(None)
 
-        yield self._startBuild(candidate, logger)
+        needed_bfjb = type(removeSecurityProxy(
+            IBuildFarmJobBehavior(candidate.specific_job)))
+        if not zope_isinstance(self._current_build_behavior, needed_bfjb):
+            raise AssertionError(
+                "Inappropriate IBuildFarmJobBehavior: %r is not a %r" %
+                (self._current_build_behavior, needed_bfjb))
+        yield self._startBuild(candidate, self._current_build_behavior, logger)
         defer.returnValue(candidate)
 
     @defer.inlineCallbacks

=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2013-09-05 07:50:48 +0000
+++ lib/lp/buildmaster/manager.py	2013-09-17 05:16:04 +0000
@@ -23,13 +23,17 @@
 from zope.component import getUtility
 
 from lp.buildmaster.enums import BuildStatus
-from lp.buildmaster.interactor import BuilderInteractor
+from lp.buildmaster.interactor import (
+    BuilderInteractor,
+    extract_vitals_from_db,
+    )
 from lp.buildmaster.interfaces.builder import (
     BuildDaemonError,
     BuildSlaveFailure,
     CannotBuild,
     CannotFetchFile,
     CannotResumeHost,
+    IBuilderSet,
     )
 from lp.buildmaster.model.builder import Builder
 from lp.services.propertycache import get_property_cache
@@ -38,11 +42,18 @@
 BUILDD_MANAGER_LOG_NAME = "slave-scanner"
 
 
-def get_builder(name):
-    """Helper to return the builder given the slave for this request."""
-    # Avoiding circular imports.
-    from lp.buildmaster.interfaces.builder import IBuilderSet
-    return getUtility(IBuilderSet)[name]
+class BuildersCache:
+
+    def __getitem__(self, name):
+        return getUtility(IBuilderSet).getByName(name)
+
+    def getVitals(self, name):
+        return extract_vitals_from_db(self[name])
+
+    def iterVitals(self):
+        return (
+            extract_vitals_from_db(b)
+            for b in getUtility(IBuilderSet).__iter__())
 
 
 @defer.inlineCallbacks
@@ -131,8 +142,9 @@
     # greater than abort_timeout in launchpad-buildd's slave BuildManager.
     CANCEL_TIMEOUT = 180
 
-    def __init__(self, builder_name, logger, clock=None):
+    def __init__(self, builder_name, builders_cache, logger, clock=None):
         self.builder_name = builder_name
+        self.builders_cache = builders_cache
         self.logger = logger
         # Use the clock if provided, so that tests can advance it.  Use the
         # reactor by default.
@@ -188,7 +200,7 @@
                 failure.getTraceback()))
 
         # Decide if we need to terminate the job or reset/fail the builder.
-        builder = get_builder(self.builder_name)
+        builder = self.builders_cache[self.builder_name]
         try:
             builder.handleFailure(self.logger)
             yield assessFailureCounts(
@@ -202,7 +214,7 @@
             transaction.abort()
 
     @defer.inlineCallbacks
-    def checkCancellation(self, builder):
+    def checkCancellation(self, builder, interactor):
         """See if there is a pending cancellation request.
 
         If the current build is in status CANCELLING then terminate it
@@ -212,7 +224,7 @@
             by resuming a slave host, so that there is no need to update its
             status.
         """
-        buildqueue = self.builder.currentjob
+        buildqueue = builder.currentjob
         if not buildqueue:
             self.date_cancel = None
             defer.returnValue(False)
@@ -224,7 +236,7 @@
         try:
             if self.date_cancel is None:
                 self.logger.info("Cancelling build '%s'" % build.title)
-                yield self.interactor.requestAbort()
+                yield interactor.requestAbort()
                 self.date_cancel = self._clock.seconds() + self.CANCEL_TIMEOUT
                 defer.returnValue(False)
             else:
@@ -241,16 +253,16 @@
         except Exception as e:
             self.logger.info(
                 "Build '%s' on %s failed to cancel" %
-                (build.title, self.builder.name))
+                (build.title, builder.name))
             self.date_cancel = None
             buildqueue.cancel()
             transaction.commit()
-            value = yield self.interactor.resetOrFail(self.logger, e)
+            value = yield interactor.resetOrFail(self.logger, e)
             # value is not None if we resumed a slave host.
             defer.returnValue(value is not None)
 
     @defer.inlineCallbacks
-    def scan(self, builder=None, interactor=None):
+    def scan(self, interactor=None):
         """Probe the builder and update/dispatch/collect as appropriate.
 
         :return: A Deferred that fires when the scan is complete.
@@ -259,54 +271,56 @@
         # Commit and refetch the Builder object to ensure we have the
         # latest data from the DB.
         transaction.commit()
-        self.builder = builder or get_builder(self.builder_name)
-        self.interactor = interactor or BuilderInteractor(self.builder)
+        vitals = self.builders_cache.getVitals(self.builder_name)
+        interactor = interactor or BuilderInteractor(
+            self.builders_cache[self.builder_name])
 
         # Confirm that the DB and slave sides are in a valid, mutually
         # agreeable state.
         lost_reason = None
-        if not self.builder.builderok:
-            lost_reason = '%s is disabled' % self.builder.name
+        if not vitals.builderok:
+            lost_reason = '%s is disabled' % vitals.name
         else:
-            cancelled = yield self.checkCancellation(self.builder)
+            cancelled = yield self.checkCancellation(
+                interactor.builder, interactor)
             if cancelled:
                 return
-            lost = yield self.interactor.rescueIfLost(self.logger)
+            lost = yield interactor.rescueIfLost(self.logger)
             if lost:
-                lost_reason = '%s is lost' % self.builder.name
+                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 self.builder.currentjob is not None:
+            if vitals.build_queue is not None:
                 self.logger.warn(
                     "%s. Resetting BuildQueue %d.", lost_reason,
-                    self.builder.currentjob.id)
-                self.builder.currentjob.reset()
+                    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 self.builder.currentjob is not None:
+        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.
-            yield self.interactor.updateBuild(self.builder.currentjob)
-        elif self.builder.manual:
+            yield interactor.updateBuild(vitals.build_queue)
+        elif vitals.manual:
             # If the builder is in manual mode, don't dispatch anything.
             self.logger.debug(
-                '%s is in manual mode, not dispatching.' %
-                self.builder.name)
+                '%s is in manual mode, not dispatching.' % vitals.name)
         else:
             # See if there is a job we can dispatch to the builder slave.
-            yield self.interactor.findAndStartJob()
-            if self.builder.currentjob is not None:
+            yield interactor.findAndStartJob()
+            builder = self.builders_cache[self.builder_name]
+            if builder.currentjob is not None:
                 # After a successful dispatch we can reset the
                 # failure_count.
-                self.builder.resetFailureCount()
+                builder.resetFailureCount()
                 transaction.commit()
 
 
@@ -323,10 +337,8 @@
         if clock is None:
             clock = reactor
         self._clock = clock
-        # Avoid circular import.
-        from lp.buildmaster.interfaces.builder import IBuilderSet
         self.current_builders = [
-            builder.name for builder in getUtility(IBuilderSet)]
+            vitals.name for vitals in self.manager.builders_cache.iterVitals()]
 
     def stop(self):
         """Terminate the LoopingCall."""
@@ -346,10 +358,8 @@
 
     def checkForNewBuilders(self):
         """See if any new builders were added."""
-        # Avoid circular import.
-        from lp.buildmaster.interfaces.builder import IBuilderSet
         new_builders = set(
-            builder.name for builder in getUtility(IBuilderSet))
+            vitals.name for vitals in self.manager.builders_cache.iterVitals())
         old_builders = set(self.current_builders)
         extra_builders = new_builders.difference(old_builders)
         self.current_builders.extend(extra_builders)
@@ -361,6 +371,7 @@
 
     def __init__(self, clock=None):
         self.builder_slaves = []
+        self.builders_cache = BuildersCache()
         self.logger = self._setupLogger()
         self.new_builders_scanner = NewBuildersScanner(
             manager=self, clock=clock)
@@ -387,10 +398,7 @@
 
         # Get a list of builders and set up scanners on each one.
 
-        # Avoiding circular imports.
-        from lp.buildmaster.interfaces.builder import IBuilderSet
-        builder_set = getUtility(IBuilderSet)
-        builders = [builder.name for builder in builder_set]
+        builders = [vitals.name for vitals in self.builders_cache.iterVitals()]
         self.addScanForBuilders(builders)
         self.new_builders_scanner.scheduleScan()
 
@@ -417,7 +425,8 @@
     def addScanForBuilders(self, builders):
         """Set up scanner objects for the builders specified."""
         for builder in builders:
-            slave_scanner = SlaveScanner(builder, self.logger)
+            slave_scanner = SlaveScanner(
+                builder, self.builders_cache, self.logger)
             self.builder_slaves.append(slave_scanner)
             slave_scanner.startCycle()
 

=== modified file 'lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py'
--- lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py	2013-09-02 08:11:43 +0000
+++ lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py	2013-09-17 05:16:04 +0000
@@ -253,6 +253,9 @@
 
     def test_handleStatus_ABORTED_recovers_building(self):
         self.builder.vm_host = "fake_vm_host"
+        self.interactor = BuilderInteractor(self.builder, self.slave)
+        self.behavior = removeSecurityProxy(
+            self.interactor._current_build_behavior)
         self.build.updateStatus(BuildStatus.BUILDING)
 
         def got_status(ignored):

=== modified file 'lib/lp/buildmaster/tests/test_interactor.py'
--- lib/lp/buildmaster/tests/test_interactor.py	2013-09-05 22:22:47 +0000
+++ lib/lp/buildmaster/tests/test_interactor.py	2013-09-17 05:16:04 +0000
@@ -80,25 +80,23 @@
             AssertionError, interactor.extractBuildStatus, slave_status)
 
     def test_verifySlaveBuildCookie_building_match(self):
-        interactor = BuilderInteractor(MockBuilder(), None, TrivialBehavior())
-        interactor.verifySlaveBuildCookie('trivial')
+        interactor = BuilderInteractor(MockBuilder())
+        interactor.verifySlaveBuildCookie(TrivialBehavior(), 'trivial')
 
     def test_verifySlaveBuildCookie_building_mismatch(self):
-        interactor = BuilderInteractor(MockBuilder(), None, TrivialBehavior())
+        interactor = BuilderInteractor(MockBuilder())
         self.assertRaises(
             CorruptBuildCookie,
-            interactor.verifySlaveBuildCookie, 'difficult')
+            interactor.verifySlaveBuildCookie, TrivialBehavior(), 'difficult')
 
     def test_verifySlaveBuildCookie_idle_match(self):
         interactor = BuilderInteractor(MockBuilder())
-        self.assertIs(None, interactor._current_build_behavior)
-        interactor.verifySlaveBuildCookie(None)
+        interactor.verifySlaveBuildCookie(None, None)
 
     def test_verifySlaveBuildCookie_idle_mismatch(self):
         interactor = BuilderInteractor(MockBuilder())
-        self.assertIs(None, interactor._current_build_behavior)
         self.assertRaises(
-            CorruptBuildCookie, interactor.verifySlaveBuildCookie, 'foo')
+            CorruptBuildCookie, interactor.verifySlaveBuildCookie, None, 'foo')
 
     def test_rescueIfLost_aborts_lost_and_broken_slave(self):
         # A slave that's 'lost' should be aborted; when the slave is

=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2013-09-05 22:22:47 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2013-09-17 05:16:04 +0000
@@ -27,6 +27,7 @@
 from lp.buildmaster.interactor import (
     BuilderInteractor,
     BuilderSlave,
+    extract_vitals_from_db,
     )
 from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
@@ -36,6 +37,7 @@
 from lp.buildmaster.manager import (
     assessFailureCounts,
     BuilddManager,
+    BuildersCache,
     NewBuildersScanner,
     SlaveScanner,
     )
@@ -124,7 +126,8 @@
         """
         if builder_name is None:
             builder_name = BOB_THE_BUILDER_NAME
-        scanner = SlaveScanner(builder_name, BufferLogger(), clock=clock)
+        scanner = SlaveScanner(
+            builder_name, BuildersCache(), BufferLogger(), clock=clock)
         scanner.logger.name = 'slave-scanner'
 
         return scanner
@@ -447,6 +450,19 @@
         self.reset = FakeMethod()
 
 
+class MockBuildersCache:
+
+    def __init__(self, builder, build_queue):
+        self._builder = builder
+        self._build_queue = build_queue
+
+    def __getitem__(self, name):
+        return self._builder
+
+    def getVitals(self, name):
+        return extract_vitals_from_db(self._builder, self._build_queue)
+
+
 class TestSlaveScannerWithoutDB(TestCase):
 
     run_tests_with = AsynchronousDeferredRunTest
@@ -456,18 +472,19 @@
         # SlaveScanner.scan calls updateBuild() when a job is building.
         interactor = BuilderInteractor(
             MockBuilder(), BuildingSlave('trivial'), TrivialBehavior())
-        scanner = SlaveScanner('mock', BufferLogger())
+        bq = FakeBuildQueue()
+        scanner = SlaveScanner(
+            'mock', MockBuildersCache(interactor.builder, bq), BufferLogger())
 
         # Instrument updateBuild and currentjob.reset
         interactor.updateBuild = FakeMethod()
-        interactor.builder.currentjob = FakeBuildQueue()
         # XXX: checkCancellation needs more than a FakeBuildQueue.
         scanner.checkCancellation = FakeMethod(defer.succeed(False))
 
-        yield scanner.scan(builder=interactor.builder, interactor=interactor)
+        yield scanner.scan(interactor=interactor)
         self.assertEqual(['status'], interactor.slave.call_log)
         self.assertEqual(1, interactor.updateBuild.call_count)
-        self.assertEqual(0, interactor.builder.currentjob.reset.call_count)
+        self.assertEqual(0, bq.reset.call_count)
 
     @defer.inlineCallbacks
     def test_scan_aborts_lost_slave_with_job(self):
@@ -475,28 +492,31 @@
         # slaves that don't have the expected job.
         interactor = BuilderInteractor(
             MockBuilder(), BuildingSlave('nontrivial'), TrivialBehavior())
-        scanner = SlaveScanner('mock', BufferLogger())
+        bq = FakeBuildQueue()
+        scanner = SlaveScanner(
+            'mock', MockBuildersCache(interactor.builder, bq), BufferLogger())
 
         # Instrument updateBuild and currentjob.reset
         interactor.updateBuild = FakeMethod()
-        interactor.builder.currentjob = FakeBuildQueue()
         # XXX: checkCancellation needs more than a FakeBuildQueue.
         scanner.checkCancellation = FakeMethod(defer.succeed(False))
 
         # A single scan will call status(), notice that the slave is
         # lost, abort() the slave, then reset() the job without calling
         # updateBuild().
-        yield scanner.scan(builder=interactor.builder, interactor=interactor)
+        yield scanner.scan(interactor=interactor)
         self.assertEqual(['status', 'abort'], interactor.slave.call_log)
         self.assertEqual(0, interactor.updateBuild.call_count)
-        self.assertEqual(1, interactor.builder.currentjob.reset.call_count)
+        self.assertEqual(1, bq.reset.call_count)
 
     @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.
         interactor = BuilderInteractor(MockBuilder(), BuildingSlave(), None)
-        scanner = SlaveScanner('mock', BufferLogger())
+        scanner = SlaveScanner(
+            'mock', MockBuildersCache(interactor.builder, None),
+            BufferLogger())
 
         # Instrument updateBuild.
         interactor.updateBuild = FakeMethod()
@@ -504,7 +524,7 @@
         # A single scan will call status(), notice that the slave is
         # lost, abort() the slave, then reset() the job without calling
         # updateBuild().
-        yield scanner.scan(builder=interactor.builder, interactor=interactor)
+        yield scanner.scan(interactor=interactor)
         self.assertEqual(['status', 'abort'], interactor.slave.call_log)
         self.assertEqual(0, interactor.updateBuild.call_count)
 
@@ -520,18 +540,18 @@
         builder_name = BOB_THE_BUILDER_NAME
         self.builder = getUtility(IBuilderSet)[builder_name]
         self.builder.virtualized = True
+        self.interactor = BuilderInteractor(self.builder)
 
     def _getScanner(self, clock=None):
-        scanner = SlaveScanner(None, BufferLogger(), clock=clock)
-        scanner.builder = self.builder
-        scanner.interactor = BuilderInteractor(self.builder)
+        scanner = SlaveScanner(
+            None, BuildersCache(), BufferLogger(), clock=clock)
         scanner.logger.name = 'slave-scanner'
         return scanner
 
     def test_ignores_nonvirtual(self):
         # If the builder is nonvirtual make sure we return False.
         self.builder.virtualized = False
-        d = self._getScanner().checkCancellation(self.builder)
+        d = self._getScanner().checkCancellation(self.builder, self.interactor)
         return d.addCallback(self.assertFalse)
 
     def test_ignores_no_buildqueue(self):
@@ -539,12 +559,12 @@
         # make sure we return False.
         buildqueue = self.builder.currentjob
         buildqueue.reset()
-        d = self._getScanner().checkCancellation(self.builder)
+        d = self._getScanner().checkCancellation(self.builder, self.interactor)
         return d.addCallback(self.assertFalse)
 
     def test_ignores_build_not_cancelling(self):
         # If the active build is not in a CANCELLING state, ignore it.
-        d = self._getScanner().checkCancellation(self.builder)
+        d = self._getScanner().checkCancellation(self.builder, self.interactor)
         return d.addCallback(self.assertFalse)
 
     @defer.inlineCallbacks
@@ -554,19 +574,20 @@
         slave = OkSlave()
         self.builder.vm_host = "fake_vm_host"
         self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
+        self.interactor = BuilderInteractor(self.builder)
         buildqueue = self.builder.currentjob
         build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(buildqueue)
         build.updateStatus(BuildStatus.CANCELLING)
         clock = task.Clock()
         scanner = self._getScanner(clock=clock)
 
-        result = yield scanner.checkCancellation(self.builder)
+        result = yield scanner.checkCancellation(self.builder, self.interactor)
         self.assertNotIn("resume", slave.call_log)
         self.assertFalse(result)
         self.assertEqual(BuildStatus.CANCELLING, build.status)
 
         clock.advance(SlaveScanner.CANCEL_TIMEOUT)
-        result = yield scanner.checkCancellation(self.builder)
+        result = yield scanner.checkCancellation(self.builder, self.interactor)
         self.assertEqual(1, slave.call_log.count("resume"))
         self.assertTrue(result)
         self.assertEqual(BuildStatus.CANCELLED, build.status)
@@ -578,10 +599,12 @@
         slave = LostBuildingBrokenSlave()
         self.builder.vm_host = "fake_vm_host"
         self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(slave))
+        self.interactor = BuilderInteractor(self.builder)
         buildqueue = self.builder.currentjob
         build = getUtility(IBinaryPackageBuildSet).getByQueueEntry(buildqueue)
         build.updateStatus(BuildStatus.CANCELLING)
-        result = yield self._getScanner().checkCancellation(self.builder)
+        result = yield self._getScanner().checkCancellation(
+            self.builder, self.interactor)
         self.assertEqual(1, slave.call_log.count("resume"))
         self.assertTrue(result)
         self.assertEqual(BuildStatus.CANCELLED, build.status)
@@ -724,8 +747,8 @@
 
     layer = LaunchpadZopelessLayer
 
-    def _getScanner(self, manager=None, clock=None):
-        return NewBuildersScanner(manager=manager, clock=clock)
+    def _getScanner(self, clock=None):
+        return NewBuildersScanner(manager=BuilddManager(), clock=clock)
 
     def test_init_stores_existing_builders(self):
         # Make sure that NewBuildersScanner initializes itself properly
@@ -779,7 +802,7 @@
             self.assertEqual("new_builders", new_builders)
 
         clock = task.Clock()
-        builder_scanner = self._getScanner(BuilddManager(), clock=clock)
+        builder_scanner = self._getScanner(clock=clock)
         builder_scanner.checkForNewBuilders = fake_checkForNewBuilders
         builder_scanner.manager.addScanForBuilders = fake_addScanForBuilders
         builder_scanner.scheduleScan = FakeMethod()

=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py'
--- lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py	2013-09-03 09:20:53 +0000
+++ lib/lp/soyuz/tests/test_binarypackagebuildbehavior.py	2013-09-17 05:16:04 +0000
@@ -151,7 +151,8 @@
         bq = build.queueBuild()
         bq.markAsBuilding(builder)
         interactor = BuilderInteractor(builder, slave)
-        d = interactor._startBuild(bq, BufferLogger())
+        d = interactor._startBuild(
+            bq, interactor._current_build_behavior, BufferLogger())
         d.addCallback(
             self.assertExpectedInteraction, slave.call_log, interactor, build,
             lf, archive, ArchivePurpose.PRIMARY, 'universe')
@@ -172,7 +173,8 @@
         bq = build.queueBuild()
         bq.markAsBuilding(builder)
         interactor = BuilderInteractor(builder, slave)
-        d = interactor._startBuild(bq, BufferLogger())
+        d = interactor._startBuild(
+            bq, interactor._current_build_behavior, BufferLogger())
 
         def check_build(ignored):
             # We expect the first call to the slave to be a resume call,
@@ -197,7 +199,8 @@
         bq = build.queueBuild()
         bq.markAsBuilding(builder)
         interactor = BuilderInteractor(builder, slave)
-        d = interactor._startBuild(bq, BufferLogger())
+        d = interactor._startBuild(
+            bq, interactor._current_build_behavior, BufferLogger())
         d.addCallback(
             self.assertExpectedInteraction, slave.call_log, interactor, build,
             lf, archive, ArchivePurpose.PARTNER)


Follow ups