Merge lp:~al-maisan/launchpad/ibuilder-api-cleanup-505647 into lp:launchpad

[Muharem Hrnjadovic]

[Muharem Hrnjadovic (al-maisan)]

Hello there!

This branch cleans up the IBuilder interface and model code related to
candidate job selection.
The findBuildCandidate()/dispatchBuildCandidate() methods are made internal
and a new findAndStartJob() method that hides them is introduced.

Tests to run:

   bin/test -vv -t build

No "make lint" errors.

[Björn Tillenius (bjornt)]

On Mon, Jan 11, 2010 at 03:08:17AM -0000, Muharem Hrnjadovic wrote:
> === modified file 'lib/lp/buildmaster/manager.py'
> --- lib/lp/buildmaster/manager.py 2009-07-26 14:19:49 +0000
> +++ lib/lp/buildmaster/manager.py 2010-01-11 03:08:16 +0000
> @@ -340,17 +340,9 @@
> transaction.commit()
> continue
>
> - candidate = builder.findBuildCandidate()
> - if candidate is None:
> - self.logger.debug(
> - "No build candidates available for builder.")
> - continue
> -
> slave = RecordingSlave(builder.name, builder.url, builder.vm_host)
> - builder.setSlaveForTesting(slave)
> -
> - builder.dispatchBuildCandidate(candidate)
> - if builder.currentjob is not None:
> + candidate = builder.findAndStartJob(buildd_slave=slave)
> + if candidate is not None:
> recording_slaves.append(slave)
> transaction.commit()

Here you changed it to commit when no candidates are found...

> === modified file 'lib/lp/soyuz/scripts/buildd.py'
> --- lib/lp/soyuz/scripts/buildd.py 2009-10-26 18:40:04 +0000
> +++ lib/lp/soyuz/scripts/buildd.py 2010-01-11 03:08:16 +0000
> @@ -201,12 +201,10 @@
> if not builder.is_available:
> self.logger.warn('builder is not available. Ignored.')
> continue
> - candidate = builder.findBuildCandidate()
> +
> + candidate = builder.findAndStartJob()
> if candidate is None:
> - self.logger.debug(
> - "No candidates available for builder.")
> continue
> - builder.dispatchBuildCandidate(candidate)
> self.txn.commit()

... but here you retain the behaviour of not commiting when no candidate
is found. Why was one place changed, but not the other?

> === modified file 'lib/lp/soyuz/tests/test_builder.py'
> --- lib/lp/soyuz/tests/test_builder.py 2009-12-02 15:18:46 +0000
> +++ lib/lp/soyuz/tests/test_builder.py 2010-01-11 03:08:16 +0000

> @@ -34,13 +35,15 @@
> # Create some i386 builders ready to build PPA builds. Two
> # already exist in sampledata so we'll use those first.
> self.builder1 = getUtility(IBuilderSet)['bob']
> - self.builder2 = getUtility(IBuilderSet)['frog']
> + self.frog_builder = removeSecurityProxy(
> + getUtility(IBuilderSet)['frog'])

Why doesn't frog_builder have a security proxy? If it's just to call
private methods, it's better to remove the proxy when you call the
method. If you start passing frog_builder to methods in your tests, you
might get things passing in the tests, while failing in real code.
Better to be safe and only unwrap the objects when you know it's safe.

> self.builder3 = self.factory.makeBuilder(name='builder3')
> - self.builder4 = self.factory.makeBuilder(name='builder4')
> + self.builder4 = removeSecurityProxy(
> + self.factory.makeBuilder(name='builder4'))

Same comment here.

> self.builder5 = self.factory.makeBuilder(name='builder5')
> self.b...

review: Needs Information

[Muharem Hrnjadovic (al-maisan)]

Björn Tillenius wrote:
> Review: Needs Information
> On Mon, Jan 11, 2010 at 03:08:17AM -0000, Muharem Hrnjadovic wrote:
>> === modified file 'lib/lp/buildmaster/manager.py'
>> --- lib/lp/buildmaster/manager.py 2009-07-26 14:19:49 +0000
>> +++ lib/lp/buildmaster/manager.py 2010-01-11 03:08:16 +0000
>> @@ -340,17 +340,9 @@
>> transaction.commit()
>> continue
>>
>> - candidate = builder.findBuildCandidate()
>> - if candidate is None:
>> - self.logger.debug(
>> - "No build candidates available for builder.")
>> - continue
>> -
>> slave = RecordingSlave(builder.name, builder.url, builder.vm_host)
>> - builder.setSlaveForTesting(slave)
>> -
>> - builder.dispatchBuildCandidate(candidate)
>> - if builder.currentjob is not None:
>> + candidate = builder.findAndStartJob(buildd_slave=slave)
>> + if candidate is not None:
>> recording_slaves.append(slave)
>> transaction.commit()
>
> Here you changed it to commit when no candidates are found...
Good catch! I have indented the commit() line above to preserve the old
behaviour.

>> === modified file 'lib/lp/soyuz/scripts/buildd.py'
>> --- lib/lp/soyuz/scripts/buildd.py 2009-10-26 18:40:04 +0000
>> +++ lib/lp/soyuz/scripts/buildd.py 2010-01-11 03:08:16 +0000
>> @@ -201,12 +201,10 @@
>> if not builder.is_available:
>> self.logger.warn('builder is not available. Ignored.')
>> continue
>> - candidate = builder.findBuildCandidate()
>> +
>> + candidate = builder.findAndStartJob()
>> if candidate is None:
>> - self.logger.debug(
>> - "No candidates available for builder.")
>> continue
>> - builder.dispatchBuildCandidate(candidate)
>> self.txn.commit()
>
> ... but here you retain the behaviour of not commiting when no candidate
> is found. Why was one place changed, but not the other?
The issue above is fixed now.

>> === modified file 'lib/lp/soyuz/tests/test_builder.py'
>> --- lib/lp/soyuz/tests/test_builder.py 2009-12-02 15:18:46 +0000
>> +++ lib/lp/soyuz/tests/test_builder.py 2010-01-11 03:08:16 +0000
>
>> @@ -34,13 +35,15 @@
>> # Create some i386 builders ready to build PPA builds. Two
>> # already exist in sampledata so we'll use those first.
>> self.builder1 = getUtility(IBuilderSet)['bob']
>> - self.builder2 = getUtility(IBuilderSet)['frog']
>> + self.frog_builder = removeSecurityProxy(
>> + getUtility(IBuilderSet)['frog'])
>
> Why doesn't frog_builder have a security proxy? If it's just to call
> private methods, it's better to remove the proxy when you call the
> method. If you start passing frog_builder to methods in your tests, you
> might get things passing in the tests, while failing in real code.
> Better to be safe and only unwrap the objects when you know it's safe.
Good point! Revised the code accordingly.

>> self.builder3 = self.factory.makeBuilder(name='builder3')
>> - ...

[Björn Tillenius (bjornt)]

review: Approve

=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2009-07-26 14:19:49 +0000
+++ lib/lp/buildmaster/manager.py	2010-01-11 21:56:15 +0000
@@ -340,19 +340,11 @@
                     transaction.commit()
                 continue
 
-            candidate = builder.findBuildCandidate()
-            if candidate is None:
-                self.logger.debug(
-                    "No build candidates available for builder.")
-                continue
-
             slave = RecordingSlave(builder.name, builder.url, builder.vm_host)
-            builder.setSlaveForTesting(slave)
-
-            builder.dispatchBuildCandidate(candidate)
+            candidate = builder.findAndStartJob(buildd_slave=slave)
             if builder.currentjob is not None:
                 recording_slaves.append(slave)
-            transaction.commit()
+                transaction.commit()
 
         return recording_slaves
 

=== modified file 'lib/lp/soyuz/doc/buildd-dispatching.txt'
--- lib/lp/soyuz/doc/buildd-dispatching.txt	2009-11-13 19:34:17 +0000
+++ lib/lp/soyuz/doc/buildd-dispatching.txt	2010-01-11 21:56:15 +0000
@@ -128,7 +128,8 @@
 Now let's check the build candidates which will be considered for the
 builder 'bob':
 
-    >>> job = bob_builder.findBuildCandidate()
+    >>> from zope.security.proxy import removeSecurityProxy
+    >>> job = removeSecurityProxy(bob_builder)._findBuildCandidate()
 
 The single BuildQueue found is a non-virtual pending build:
 
@@ -157,14 +158,16 @@
     ...    'foo.dsc', len(content), StringIO(content), 'application/dsc')
 
     >>> sprf = build.sourcepackagerelease.files[0]
-    >>> from zope.security.proxy import removeSecurityProxy
     >>> naked_sprf = removeSecurityProxy(sprf)
     >>> naked_sprf.libraryfile = getUtility(ILibraryFileAliasSet)[alias_id]
     >>> flush_database_updates()
 
 Check the dispatching method itself:
 
-    >>> bob_builder.dispatchBuildCandidate(job)
+    >>> dispatched_job = bob_builder.findAndStartJob()
+    >>> job == dispatched_job
+    True
+
     >>> flush_database_updates()
 
 Verify if the job (BuildQueue) was updated appropriately:
@@ -224,7 +227,7 @@
     >>> bob_builder.vm_host = 'localhost.ppa'
     >>> syncUpdate(bob_builder)
 
-    >>> job = bob_builder.findBuildCandidate()
+    >>> job = removeSecurityProxy(bob_builder)._findBuildCandidate()
     >>> print job
     None
 
@@ -245,11 +248,11 @@
     >>> bob_builder.virtualized = True
     >>> syncUpdate(bob_builder)
 
-    >>> job = bob_builder.findBuildCandidate()
+    >>> job = removeSecurityProxy(bob_builder)._findBuildCandidate()
     >>> ppa_job.id == job.id
     True
 
-For further details regarding IBuilder.findBuildCandidate() please see
+For further details regarding IBuilder._findBuildCandidate() please see
 lib/lp/soyuz/tests/test_builder.py.
 
 Start buildd-slave to be able to dispatch jobs.
@@ -262,7 +265,7 @@
 
     >>> bob_builder.virtualized = False
     >>> flush_database_updates()
-    >>> bob_builder.dispatchBuildCandidate(ppa_job)
+    >>> removeSecurityProxy(bob_builder)._dispatchBuildCandidate(ppa_job)
     Traceback (most recent call last):
     ...
     AssertionError: Attempt to build non-virtual item on a virtual builder.
@@ -273,7 +276,10 @@
     >>> bob_builder.virtualized = True
     >>> flush_database_updates()
 
-    >>> bob_builder.dispatchBuildCandidate(ppa_job)
+    >>> dispatched_job = bob_builder.findAndStartJob()
+    >>> ppa_job == dispatched_job
+    True
+
     >>> flush_database_updates()
 
 PPA job is building.
@@ -328,7 +334,7 @@
 implementation.
 
     >>> BuilddSlaveTestSetup().setUp()
-    >>> bob_builder.dispatchBuildCandidate(sec_job)
+    >>> removeSecurityProxy(bob_builder)._dispatchBuildCandidate(sec_job)
     Traceback (most recent call last):
     ...
     AssertionError: Soyuz is not yet capable of building SECURITY uploads.
@@ -336,7 +342,7 @@
 
 To solve this problem temporarily until we start building security
 uploads, we will mark builds targeted to the SECURITY pocket as
-FAILEDTOBUILD during the findBuildCandidate look-up.
+FAILEDTOBUILD during the _findBuildCandidate look-up.
 
 We will also create another build candidate in breezy-autotest/i386 to
 check if legitimate pending candidates will remain valid.
@@ -360,7 +366,7 @@
 
     >>> new_pub = old_pub.copyTo(
     ...     pending_build.distroseries, old_pub.pocket, pending_build.archive)
-    >>> candidate = bob_builder.findBuildCandidate()
+    >>> candidate = removeSecurityProxy(bob_builder)._findBuildCandidate()
     >>> flush_database_updates()
     >>> candidate.id == pending_job.id
     True

=== modified file 'lib/lp/soyuz/doc/buildd-slavescanner.txt'
--- lib/lp/soyuz/doc/buildd-slavescanner.txt	2010-01-05 16:30:29 +0000
+++ lib/lp/soyuz/doc/buildd-slavescanner.txt	2010-01-11 21:56:15 +0000
@@ -754,11 +754,8 @@
 
 == Build Dispatching ==
 
-Build dispatching can be entirely done via IBuilder content class via
-the following API:
-
- * findCandidate:  returns a suitable BuildQueue candidate
- * dispatchBuildCandidate: dispatch a build for a given candidate.
+Build dispatching can be entirely done via IBuilder content class
+using the findAndStartJob method.
 
 We will use SoyuzTestPublisher to simulate the required context in the
 next tests. Let's initialise it.
@@ -805,14 +802,14 @@
 superseded source package releases in the queue and marks the
 corresponding build record as SUPERSEDED.
 
-    >>> old_candidate = a_builder.findBuildCandidate()
+    >>> old_candidate = removeSecurityProxy(a_builder)._findBuildCandidate()
     >>> build = getUtility(IBuildSet).getByQueueEntry(old_candidate)
     >>> print build.buildstate.name
     NEEDSBUILD
 
 The 'candidate' is constant until we dispatch it.
 
-    >>> new_candidate = a_builder.findBuildCandidate()
+    >>> new_candidate = removeSecurityProxy(a_builder)._findBuildCandidate()
     >>> new_candidate.id == old_candidate.id
     True
 
@@ -820,7 +817,7 @@
 whether the candidate will still be found.
 
     >>> build.archive.enabled = False
-    >>> new_candidate = a_builder.findBuildCandidate()
+    >>> new_candidate = removeSecurityProxy(a_builder)._findBuildCandidate()
     >>> new_candidate is None
     True
 
@@ -829,7 +826,7 @@
 candidate will be found again.
 
     >>> build.archive.enabled = True
-    >>> new_candidate = a_builder.findBuildCandidate()
+    >>> new_candidate = removeSecurityProxy(a_builder)._findBuildCandidate()
     >>> new_candidate.id == old_candidate.id
     True
 
@@ -852,7 +849,7 @@
 
 Now, there we have another build candidate.
 
-    >>> new_candidate = a_builder.findBuildCandidate()
+    >>> new_candidate = removeSecurityProxy(a_builder)._findBuildCandidate()
     >>> new_candidate.id != old_candidate.id
     True
 
@@ -882,9 +879,10 @@
 
 Let's try to find a new build candidate:
 
-    >>> another_candidate = a_builder.findBuildCandidate()
+    >>> another_candidate = removeSecurityProxy(
+    ...     a_builder)._findBuildCandidate()
 
-Since there are no more candidates at all, findBuildCandidate()
+Since there are no more candidates at all, _findBuildCandidate()
 returned None:
 
     >>> print another_candidate
@@ -897,7 +895,8 @@
     >>> commit()
     >>> LaunchpadZopelessLayer.switchDbUser(config.builddmaster.dbuser)
 
-    >>> another_candidate = a_builder.findBuildCandidate()
+    >>> another_candidate = removeSecurityProxy(
+    ...     a_builder)._findBuildCandidate()
     >>> another_candidate.id == new_candidate.id
     True
 
@@ -921,7 +920,8 @@
     >>> print build.buildstate.name
     NEEDSBUILD
 
-    >>> another_candidate = a_builder.findBuildCandidate()
+    >>> another_candidate = removeSecurityProxy(
+    ...     a_builder)._findBuildCandidate()
     >>> print another_candidate
     None
 
@@ -954,7 +954,7 @@
     >>> a_builder.is_available
     True
     >>> candidate = a_build.createBuildQueueEntry()
-    >>> a_builder.dispatchBuildCandidate(candidate)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(candidate)
     ensurepresent called, url=...
     ensurepresent called,
         url=http://localhost:58000/3/firefox_0.9.2.orig.tar.gz
@@ -982,7 +982,7 @@
     >>> a_builder.is_available
     True
     >>> candidate = a_build.createBuildQueueEntry()
-    >>> a_builder.dispatchBuildCandidate(candidate)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(candidate)
     ensurepresent called, url=...
     ensurepresent called,
         url=http://localhost:58000/3/firefox_0.9.2.orig.tar.gz
@@ -1034,7 +1034,7 @@
 So, at moment, partner archive is still not relevant for builds in
 hoary/i386. It's not passed to the builder.
 
-    >>> a_builder.dispatchBuildCandidate(candidate)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(candidate)
     ensurepresent called, url=...
     ensurepresent called,
         url=http://localhost:58000/3/firefox_0.9.2.orig.tar.gz
@@ -1084,7 +1084,8 @@
 binary in hoary/i386, the partner archive gets included in the builder
 sources_list.
 
-    >>> a_builder.dispatchBuildCandidate(partner_candidate)
+    >>> removeSecurityProxy(
+    ...     a_builder)._dispatchBuildCandidate(partner_candidate)
     ensurepresent called, url=...
     ensurepresent called, url=http://localhost:58000/.../foo_666.dsc
     OkSlave BUILDING
@@ -1126,7 +1127,7 @@
     >>> create_binary_publication_for(
     ...    cprov_archive, hoary, PackagePublishingStatus.PUBLISHED)
 
-    >>> a_builder.dispatchBuildCandidate(candidate)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(candidate)
     ensurepresent called, url=...
     ensurepresent called,
         url=http://localhost:58000/3/firefox_0.9.2.orig.tar.gz
@@ -1183,7 +1184,7 @@
 
 This is so that the mangling tools will run over the built packages.
 
-    >>> a_builder.dispatchBuildCandidate(candidate)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(candidate)
     ensurepresent called, url=...
     ensurepresent called,
         url=http://private-ppa.launchpad.dev/cprov/ppa/ubuntu/pool/main/m/mozilla-firefox/firefox_0.9.2.orig.tar.gz
@@ -1223,7 +1224,7 @@
     >>> LaunchpadZopelessLayer.switchDbUser(config.builddmaster.dbuser)
     >>> login(ANONYMOUS)
 
-    >>> a_builder.dispatchBuildCandidate(candidate)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(candidate)
     ensurepresent called, ...
     ...
     Ogre-component: main
@@ -1302,7 +1303,7 @@
     >>> setupBuildQueue(candidate, a_builder)
     >>> last_stub_mail_count = len(stub.test_emails)
 
-    >>> a_builder.dispatchBuildCandidate(candidate)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(candidate)
     ensurepresent called, url=...
     ensurepresent called,
         url=http://localhost:58000/3/firefox_0.9.2.orig.tar.gz
@@ -1331,7 +1332,7 @@
     >>> create_binary_publication_for(
     ...    mark_archive, hoary, PackagePublishingStatus.PUBLISHED)
 
-    >>> a_builder.dispatchBuildCandidate(candidate)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(candidate)
     ensurepresent called, url=...
     ensurepresent called,
         url=http://localhost:58000/3/firefox_0.9.2.orig.tar.gz
@@ -1376,7 +1377,7 @@
 
     >>> hoary_i386.distroseries.status.name
     'DEVELOPMENT'
-    >>> a_builder.dispatchBuildCandidate(updates_bqItem)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(updates_bqItem)
     Traceback (most recent call last):
     ...
     AssertionError: i386 build of evolution 1.0 in ubuntu hoary UPDATES (...) can not be built for pocket UPDATES: invalid pocket due to the series status of hoary.
@@ -1401,7 +1402,7 @@
     >>> removeSecurityProxy(build).pocket = (
     ...     PackagePublishingPocket.UPDATES)
     >>> last_stub_mail_count = len(stub.test_emails)
-    >>> a_builder.dispatchBuildCandidate(bqItem3)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(bqItem3)
     ensurepresent called, url=...
     ensurepresent called,
         url=http://localhost:58000/3/firefox_0.9.2.orig.tar.gz
@@ -1423,7 +1424,7 @@
     >>> removeSecurityProxy(build).pocket = (
     ...     PackagePublishingPocket.PROPOSED)
     >>> last_stub_mail_count = len(stub.test_emails)
-    >>> a_builder.dispatchBuildCandidate(bqItem3)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(bqItem3)
     ensurepresent called, url=...
     ensurepresent called,
         url=http://localhost:58000/3/firefox_0.9.2.orig.tar.gz
@@ -1446,7 +1447,7 @@
     >>> removeSecurityProxy(build).pocket = (
     ...     PackagePublishingPocket.BACKPORTS)
     >>> last_stub_mail_count = len(stub.test_emails)
-    >>> a_builder.dispatchBuildCandidate(bqItem3)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(bqItem3)
     ensurepresent called, url=...
     ensurepresent called,
         url=http://localhost:58000/3/firefox_0.9.2.orig.tar.gz
@@ -1477,13 +1478,13 @@
 because Embargoed-Archives and Restricted-UI implementations are not
 yet ready.
 
-    >>> a_builder.dispatchBuildCandidate(bqItem3)
+    >>> removeSecurityProxy(a_builder)._dispatchBuildCandidate(bqItem3)
     Traceback (most recent call last):
     ...
     AssertionError: Soyuz is not yet capable of building SECURITY uploads.
 
-Builds for security pocket are marked as FAILEDTOBUILD inside
-findBuildCandidate method, see doc/buildd-dispatching.txt
+Builds for security pocket are marked as FAILEDTOBUILD inside the
+_findBuildCandidate() method, see doc/buildd-dispatching.txt
 
 
 == Builder Status Handler ==

=== modified file 'lib/lp/soyuz/interfaces/builder.py'
--- lib/lp/soyuz/interfaces/builder.py	2009-12-03 14:38:48 +0000
+++ lib/lp/soyuz/interfaces/builder.py	2010-01-11 21:56:15 +0000
@@ -242,24 +242,6 @@
         :return: A librarian file alias.
         """
 
-    def findBuildCandidate():
-        """Return the candidate for building.
-
-        The pending BuildQueue item with the highest score for this builder
-        ProcessorFamily or None if no candidate is available.
-
-        For public PPA builds, subsequent builds for a given ppa and
-        architecture will not be returned until the current build for
-        the ppa and architecture is finished.
-        """
-
-    def dispatchBuildCandidate(candidate):
-        """Dispatch the given job to this builder.
-
-        This method can only be executed in the builddmaster machine, since
-        it will actually issues the XMLRPC call to the buildd-slave.
-        """
-
     def handleTimeout(logger, error_message):
         """Handle buildd slave communication timeout situations.
 
@@ -274,6 +256,14 @@
         :param error_message: The error message to be used for logging.
         """
 
+    def findAndStartJob(buildd_slave=None):
+        """Find a job to run and send it to the buildd slave.
+
+        :param buildd_slave: An optional buildd slave that this builder should
+            talk to.
+        :return: the `IBuildQueue` instance found or None if no job was found.
+        """
+
 
 class IBuilderSet(Interface):
     """Collections of builders.

=== modified file 'lib/lp/soyuz/model/builder.py'
--- lib/lp/soyuz/model/builder.py	2009-12-24 06:57:25 +0000
+++ lib/lp/soyuz/model/builder.py	2010-01-11 21:56:15 +0000
@@ -386,9 +386,9 @@
         return True
 
     # XXX cprov 20071116: It should become part of the public
-    # findBuildCandidate once we start to detect superseded builds
+    # _findBuildCandidate once we start to detect superseded builds
     # at build creation time.
-    def _findBuildCandidate(self):
+    def _findBinaryBuildCandidate(self):
         """Return the highest priority build candidate for this builder.
 
         Returns a pending IBuildQueue record queued for this builder
@@ -487,10 +487,21 @@
         logger = logging.getLogger('slave-scanner')
         return logger
 
-    def findBuildCandidate(self):
-        """See `IBuilder`."""
+    def _findBuildCandidate(self):
+        """Find a candidate job for dispatch to an idle buildd slave.
+
+        The pending BuildQueue item with the highest score for this builder
+        ProcessorFamily or None if no candidate is available.
+
+        For public PPA builds, subsequent builds for a given ppa and
+        architecture will not be returned until the current build for
+        the ppa and architecture is finished.
+
+        :return: A binary build candidate job.
+        """
+
         logger = self._getSlaveScannerLogger()
-        candidate = self._findBuildCandidate()
+        candidate = self._findBinaryBuildCandidate()
 
         # Mark build records targeted to old source versions as SUPERSEDED
         # and build records target to SECURITY pocket as FAILEDTOBUILD.
@@ -507,7 +518,7 @@
                     % (build.id, candidate.id))
                 build.buildstate = BuildStatus.FAILEDTOBUILD
                 candidate.destroySelf()
-                candidate = self._findBuildCandidate()
+                candidate = self._findBinaryBuildCandidate()
                 continue
 
             publication = build.current_source_publication
@@ -520,7 +531,7 @@
                     % (build.id, candidate.id))
                 build.buildstate = BuildStatus.SUPERSEDED
                 candidate.destroySelf()
-                candidate = self._findBuildCandidate()
+                candidate = self._findBinaryBuildCandidate()
                 continue
 
             return candidate
@@ -528,8 +539,14 @@
         # No candidate was found.
         return None
 
-    def dispatchBuildCandidate(self, candidate):
-        """See `IBuilder`."""
+    def _dispatchBuildCandidate(self, candidate):
+        """Dispatch the pending job to the associated buildd slave.
+
+        This method can only be executed in the builddmaster machine, since
+        it will actually issues the XMLRPC call to the buildd-slave.
+
+        :param candidate: The job to dispatch.
+        """
         logger = self._getSlaveScannerLogger()
         try:
             self.startBuild(candidate, logger)
@@ -563,6 +580,21 @@
                 exc_info=True)
             self.failbuilder(error_message)
 
+    def findAndStartJob(self, buildd_slave=None):
+        """See IBuilder."""
+        logger = self._getSlaveScannerLogger()
+        candidate = self._findBuildCandidate()
+
+        if candidate is None:
+            logger.debug("No build candidates available for builder.")
+            return None
+
+        if buildd_slave is not None:
+            self.setSlaveForTesting(buildd_slave)
+
+        self._dispatchBuildCandidate(candidate)
+        return candidate
+
 
 class BuilderSet(object):
     """See IBuilderSet"""

=== modified file 'lib/lp/soyuz/scripts/buildd.py'
--- lib/lp/soyuz/scripts/buildd.py	2009-10-26 18:40:04 +0000
+++ lib/lp/soyuz/scripts/buildd.py	2010-01-11 21:56:15 +0000
@@ -201,12 +201,10 @@
             if not builder.is_available:
                 self.logger.warn('builder is not available. Ignored.')
                 continue
-            candidate = builder.findBuildCandidate()
+
+            candidate = builder.findAndStartJob()
             if candidate is None:
-                self.logger.debug(
-                    "No candidates available for builder.")
                 continue
-            builder.dispatchBuildCandidate(candidate)
             self.txn.commit()
 
         self.logger.info("Slave Scan Process Finished.")

=== modified file 'lib/lp/soyuz/tests/test_builder.py'
--- lib/lp/soyuz/tests/test_builder.py	2009-12-02 15:18:46 +0000
+++ lib/lp/soyuz/tests/test_builder.py	2010-01-11 21:56:15 +0000
@@ -6,10 +6,11 @@
 import unittest
 
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.testing import LaunchpadZopelessLayer
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
-    BuildBehaviorMismatch, IBuildFarmJobBehavior)
+    IBuildFarmJobBehavior)
 from lp.buildmaster.model.buildfarmjobbehavior import IdleBuildBehavior
 from lp.soyuz.interfaces.archive import ArchivePurpose
 from lp.soyuz.interfaces.build import BuildStatus, IBuildSet
@@ -34,13 +35,13 @@
         # Create some i386 builders ready to build PPA builds.  Two
         # already exist in sampledata so we'll use those first.
         self.builder1 = getUtility(IBuilderSet)['bob']
-        self.builder2 = getUtility(IBuilderSet)['frog']
+        self.frog_builder = getUtility(IBuilderSet)['frog']
         self.builder3 = self.factory.makeBuilder(name='builder3')
         self.builder4 = self.factory.makeBuilder(name='builder4')
         self.builder5 = self.factory.makeBuilder(name='builder5')
         self.builders = [
             self.builder1,
-            self.builder2,
+            self.frog_builder,
             self.builder3,
             self.builder4,
             self.builder5,
@@ -81,7 +82,8 @@
         # there's only one builder available.
 
         # Asking frog to find a candidate should give us the joesppa build.
-        next_job = self.frog_builder.findBuildCandidate()
+        next_job = removeSecurityProxy(
+            self.frog_builder)._findBuildCandidate()
         build = getUtility(IBuildSet).getByQueueEntry(next_job)
         self.assertEqual('joesppa', build.archive.name)
 
@@ -89,7 +91,8 @@
         # returned.
         self.bob_builder.builderok = False
         self.bob_builder.manual = False
-        next_job = self.frog_builder.findBuildCandidate()
+        next_job = removeSecurityProxy(
+            self.frog_builder)._findBuildCandidate()
         build = getUtility(IBuildSet).getByQueueEntry(next_job)
         self.assertEqual('joesppa', build.archive.name)
 
@@ -162,7 +165,7 @@
     def test_findBuildCandidate_first_build_started(self):
         # A PPA cannot start a build if it would use 80% or more of the
         # builders.
-        next_job = self.builder4.findBuildCandidate()
+        next_job = removeSecurityProxy(self.builder4)._findBuildCandidate()
         build = getUtility(IBuildSet).getByQueueEntry(next_job)
         self.failIfEqual('joesppa', build.archive.name)
 
@@ -170,7 +173,7 @@
         # When joe's first ppa build finishes, his fourth i386 build
         # will be the next build candidate.
         self.joe_builds[0].buildstate = BuildStatus.FAILEDTOBUILD
-        next_job = self.builder4.findBuildCandidate()
+        next_job = removeSecurityProxy(self.builder4)._findBuildCandidate()
         build = getUtility(IBuildSet).getByQueueEntry(next_job)
         self.failUnlessEqual('joesppa', build.archive.name)
 
@@ -179,7 +182,7 @@
         # for the one architecture.
         self.ppa_joe.private = True
         self.ppa_joe.buildd_secret = 'sekrit'
-        next_job = self.builder4.findBuildCandidate()
+        next_job = removeSecurityProxy(self.builder4)._findBuildCandidate()
         build = getUtility(IBuildSet).getByQueueEntry(next_job)
         self.failUnlessEqual('joesppa', build.archive.name)
 
@@ -205,7 +208,8 @@
         # Normal archives are not restricted to serial builds per
         # arch.
 
-        next_job = self.builder2.findBuildCandidate()
+        next_job = removeSecurityProxy(
+            self.frog_builder)._findBuildCandidate()
         build = getUtility(IBuildSet).getByQueueEntry(next_job)
         self.failUnlessEqual('primary', build.archive.name)
         self.failUnlessEqual('gedit', build.sourcepackagerelease.name)
@@ -213,8 +217,9 @@
         # Now even if we set the build building, we'll still get the
         # second non-ppa build for the same archive as the next candidate.
         build.buildstate = BuildStatus.BUILDING
-        build.builder = self.builder2
-        next_job = self.builder2.findBuildCandidate()
+        build.builder = self.frog_builder
+        next_job = removeSecurityProxy(
+            self.frog_builder)._findBuildCandidate()
         build = getUtility(IBuildSet).getByQueueEntry(next_job)
         self.failUnlessEqual('primary', build.archive.name)
         self.failUnlessEqual('firefox', build.sourcepackagerelease.name)