nextSlave can choose slave according to buildrequest

There is a need for user to specify the policy for restricting
a build to a particular slave, or prioritize slave give build properties

We first try to associate a slave for each build requests
then filter out requests that do not have slaves
the remaining process is unchanged:

choose buildrequest -> merge buildrequests -> claim


Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>

master/buildbot/config.py

@@ -19,6 +19,8 @@
 import re
 import sys
 import warnings
+
+from types import MethodType
 from buildbot.util import safeTranslate
 from buildbot import interfaces
 from buildbot import locks
@@ -674,6 +676,15 @@ def __init__(self, name=None, slavename=None, slavenames=None,
         self.nextSlave = nextSlave
         if nextSlave and not callable(nextSlave):
             errors.addError('nextSlave must be a callable')
+
+        # we support the previous nextSlave API
+        # this is temporary hack until we use a major release
+        # to really make a clean API (we first need data API to land)
+        if nextSlave and (nextSlave.func_code.co_argcount == 2 or
+                          (isinstance(nextSlave,MethodType) and
+                           nextSlave.func_code.co_argcount == 3)):
+            self.nextSlave = lambda x,y,z:nextSlave(x,y)
+
         self.nextBuild = nextBuild
         if nextBuild and not callable(nextBuild):
             errors.addError('nextBuild must be a callable')

master/buildbot/process/builder.py

@@ -492,15 +492,14 @@ def maybeStartBuild(self):
 
         # match them up until we're out of options
         while available_slavebuilders and unclaimed_requests:
-            # first, choose a slave (using nextSlave)
-            slavebuilder = yield self._chooseSlave(available_slavebuilders)
-
-            if not slavebuilder:
+            # first, decorate requests with a choosen slave (using nextSlave)
+            choosenSlaves = yield self._chooseSlave(unclaimed_requests, available_slavebuilders)
+            if not choosenSlaves:
                 break
+            unclaimed_requests = filter(lambda brdict:brdict['brid'] in choosenSlaves,
+                                        unclaimed_requests)
 
-            if slavebuilder not in available_slavebuilders:
-                log.msg(("nextSlave chose a nonexistent slave for builder "
-                         "'%s'; cannot start build") % self.name)
+            if not unclaimed_requests:
                 break
 
             # then choose a request (using nextBuild)
@@ -514,6 +513,7 @@ def maybeStartBuild(self):
                          "'%s'; cannot start build") % self.name)
                 break
 
+            slavebuilder = choosenSlaves[brdict['brid']]
             # merge the chosen request with any compatible requests in the
             # queue
             brdicts = yield self._mergeRequests(brdict, unclaimed_requests,
@@ -568,20 +568,32 @@ def maybeStartBuild(self):
 
     # a few utility functions to make the maybeStartBuild a bit shorter and
     # easier to read
-
-    def _chooseSlave(self, available_slavebuilders):
+    @defer.inlineCallbacks
+    def _chooseSlave(self, buildrequests, available_slavebuilders):
         """
-        Choose the next slave, using the C{nextSlave} configuration if
+        Choose the next slaves, using the C{nextSlave} configuration if
         available, and falling back to C{random.choice} otherwise.
 
+        @param buildrequests: list of requests,  we want to find potential slave
+
         @param available_slavebuilders: list of slavebuilders to choose from
-        @returns: SlaveBuilder or None via Deferred
+        @returns: dictionary mapping buildrequests to SlaveBuilder via Deferred
         """
-        if self.config.nextSlave:
-            return defer.maybeDeferred(lambda :
-                    self.config.nextSlave(self, available_slavebuilders))
-        else:
-            return defer.succeed(random.choice(available_slavebuilders))
+        ret = {}
+        for brdict in buildrequests:
+            if self.config.nextSlave:
+                br = yield self._brdictToBuildRequest(brdict)
+                sb = yield defer.maybeDeferred(lambda :
+                        self.config.nextSlave(self, available_slavebuilders, br))
+                if sb not in available_slavebuilders:
+                    log.msg(("nextSlave chose a nonexistent slave for builder "
+                             "'%s'; cannot start build") % self.name)
+                    sb = None
+            else:
+                sb = random.choice(available_slavebuilders)
+            if sb:
+                ret[brdict['brid']] = sb
+        defer.returnValue(ret)
 
     def _chooseBuild(self, buildrequests):
         """

master/buildbot/test/unit/test_process_builder.py

@@ -20,6 +20,7 @@
 from twisted.internet import defer
 from buildbot import config
 from buildbot.status import master
+from buildbot.process import properties
 from buildbot.test.fake import fakedb, fakemaster
 from buildbot.process import builder, factory
 from buildbot.db import buildrequests
@@ -203,19 +204,7 @@ def test_maybeStartBuild_limited_by_requests(self):
     def test_maybeStartBuild_chooseSlave_None(self):
         yield self.makeBuilder()
 
-        self.bldr._chooseSlave = lambda avail : defer.succeed(None)
-        self.setSlaveBuilders({'test-slave1':1, 'test-slave2':1})
-        rows = self.base_rows + [
-            fakedb.BuildRequest(id=11, buildsetid=11, buildername="bldr"),
-        ]
-        yield self.do_test_maybeStartBuild(rows=rows,
-                exp_claims=[], exp_builds=[])
-
-    @defer.inlineCallbacks
-    def test_maybeStartBuild_chooseSlave_bogus(self):
-        yield self.makeBuilder()
-
-        self.bldr._chooseSlave = lambda avail : defer.succeed(mock.Mock())
+        self.bldr._chooseSlave = lambda brs, avail : defer.succeed(None)
         self.setSlaveBuilders({'test-slave1':1, 'test-slave2':1})
         rows = self.base_rows + [
             fakedb.BuildRequest(id=11, buildsetid=11, buildername="bldr"),
@@ -227,7 +216,7 @@ def test_maybeStartBuild_chooseSlave_bogus(self):
     def test_maybeStartBuild_chooseSlave_fails(self):
         yield self.makeBuilder()
 
-        self.bldr._chooseSlave = lambda avail : defer.fail(RuntimeError("xx"))
+        self.bldr._chooseSlave = lambda brs, avail : defer.fail(RuntimeError("xx"))
         self.setSlaveBuilders({'test-slave1':1, 'test-slave2':1})
         rows = self.base_rows + [
             fakedb.BuildRequest(id=11, buildsetid=11, buildername="bldr"),
@@ -351,13 +340,25 @@ def test_maybeStartBuild_merge_ordering(self):
 
     # _chooseSlave
 
-    def do_test_chooseSlave(self, nextSlave, exp_choice=None, exp_fail=None):
-        slavebuilders = [ mock.Mock(name='sb%d' % i) for i in range(4) ]
+    def do_test_chooseSlave(self, nextSlave, exp_choice=None, exp_fail=None, properties=None):
+        def mkrq(n):
+            brdict = dict(brobj=mock.Mock(name='br%d' % n,
+                                          properties=properties))
+            brdict['brobj'].brdict = brdict
+            brdict['brid'] = n
+            return brdict
+        requests = [ mkrq(i) for i in range(4) ]
+        slavebuilders = [ mock.Mock(name='sb%d' % i,
+                                    slave=mock.Mock(slavename='s%d' % i))
+                          for i in range(4) ]
 
         d = self.makeBuilder(nextSlave=nextSlave)
-        d.addCallback(lambda _ : self.bldr._chooseSlave(slavebuilders))
+        d.addCallback(lambda _ : self.bldr._chooseSlave(requests, slavebuilders))
         def check(sb):
-            self.assertIdentical(sb, slavebuilders[exp_choice])
+            if exp_choice>=0:
+                self.assertIdentical(sb[0], slavebuilders[exp_choice])
+            else:
+                self.failUnless(sb == {})
         def failed(f):
             f.trap(exp_fail)
         d.addCallbacks(check, failed)
@@ -379,6 +380,18 @@ def nextSlave(bldr, lst):
             return defer.succeed(lst[1])
         return self.do_test_chooseSlave(nextSlave, exp_choice=1)
 
+    def test_chooseSlave_nextSlave_bogus(self):
+        def nextSlave(bldr, lst):
+            self.assertIdentical(bldr, self.bldr)
+            return defer.succeed(mock.Mock())
+        return self.do_test_chooseSlave(nextSlave, exp_choice=-1)
+
+    def test_chooseSlave_nextSlave_None(self):
+        def nextSlave(bldr, lst):
+            self.assertIdentical(bldr, self.bldr)
+            return defer.succeed(None)
+        return self.do_test_chooseSlave(nextSlave, exp_choice=-1)
+
     def test_chooseSlave_nextSlave_exception(self):
         def nextSlave(bldr, lst):
             raise RuntimeError
@@ -389,6 +402,50 @@ def nextSlave(bldr, lst):
             return defer.fail(failure.Failure(RuntimeError()))
         return self.do_test_chooseSlave(nextSlave, exp_fail=RuntimeError)
 
+    def test_chooseSlave_nextSlave_withbr(self):
+        def nextSlave(bldr, lst, br):
+            self.assertIdentical(bldr, self.bldr)
+            # make sure we get a br object
+            self.assertIdentical(br,br.brdict['brobj'])
+            return lst[1]
+        return self.do_test_chooseSlave(nextSlave, exp_choice=1)
+
+    def nextSlave_as_a_method(self, bldr, lst, br):
+        # make sure 'self' is correct object
+        self.assertIdentical(bldr, self.bldr)
+        # make sure we get a br object
+        self.assertIdentical(br,br.brdict['brobj'])
+        return lst[1]
+    def test_chooseSlave_nextSlave_method(self):
+        return self.do_test_chooseSlave(self.nextSlave_as_a_method, exp_choice=1)
+
+    def nextSlave_as_a_method_old_api(self, bldr, lst):
+        # make sure 'self' is correct object
+        self.assertIdentical(bldr, self.bldr)
+        return lst[1]
+    def test_chooseSlave_nextSlave_method_old_api(self):
+        return self.do_test_chooseSlave(self.nextSlave_as_a_method_old_api, exp_choice=1)
+
+    def doc_next_slave_example(self):
+        # this is the code that is shown in the manual as an example for nextSlave
+        def myNextSlave(builder, slavebuilders, buildrequest):
+            forced_slave = buildrequest.properties.getProperty("forced_slave")
+            if forced_slave == None:
+                return random.choice(slavebuilders)
+            for slave in slavebuilders:
+                if forced_slave == slave.slave.slavename:
+                    return slave
+            return None
+        return myNextSlave
+    def test_chooseSlave_nextSlave_doc_example(self):
+        self.patch(random, "choice", lambda lst : lst[2])
+        return self.do_test_chooseSlave(self.doc_next_slave_example(), exp_choice=3,
+                                        properties=properties.Properties(forced_slave="s3"))
+    def test_chooseSlave_nextSlave_doc_example_no_prop(self):
+        self.patch(random, "choice", lambda lst : lst[2])
+        return self.do_test_chooseSlave(self.doc_next_slave_example(), exp_choice=2,
+                                        properties=properties.Properties())
+
     # _chooseBuild
 
     def do_test_chooseBuild(self, nextBuild, exp_choice=None, exp_fail=None):

master/docs/manual/cfg-builders.rst

@@ -68,11 +68,30 @@ Other optional keys may be set on each ``BuilderConfig``:
 
 ``nextSlave``
     If provided, this is a function that controls which slave will be assigned
-    future jobs. The function is passed two arguments, the :class:`Builder`
-    object which is assigning a new job, and a list of :class:`BuildSlave`
-    objects. The function should return one of the :class:`BuildSlave`
+    future jobs. The function is passed three arguments, the :class:`Builder`
+    object which is assigning a new job, a list of :class:`SlaveBuilder` objects,
+    and a candidate :class:`BuildRequest`.
+    The function will be called for each :class:`BuildRequest` that may be started.
+    The function should return one of the :class:`SlaveBuilder`
     objects, or ``None`` if none of the available slaves should be
-    used.
+    used. This function can optionally return a Deferred which should fire with
+    the same results. A common use of this hook is to force a build to use a given
+    slave or a given set of slaves::
+
+        def myNextSlave(builder, slavebuilders, buildrequest):
+            forced_slave = buildrequest.properties.getProperty("forced_slave")
+            if forced_slave == None:
+                return random.choice(slavebuilders)
+            for slave in slavebuilders:
+                if forced_slave == slave.slave.slavename:
+                    return slave
+            return None
+        return myNextSlave
+
+..
+    ^                                                                   ^
+    |  If you change this example code, please update the unit test in  |
+    |  buildbot.test.unit.test_process_builder                          |
 
 ``nextBuild``
     If provided, this is a function that controls which build request will be
@@ -82,6 +101,8 @@ Other optional keys may be set on each ``BuilderConfig``:
     :class:`BuildRequest` objects, or ``None`` if none of the pending
     builds should be started. This function can optionally return a
     Deferred which should fire with the same results.
+    nextBuild is called after nextSlave, with only the list of
+    :class:`BuildRequest` that have a potential slave
 
 ``locks``
     This argument specifies a list of locks that apply to this builder; see