launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/axe-the-ibb into lp:launchpad with lp:~wgrant/launchpad/builderinteractor-cookies as a prerequisite.

Commit message:
Axe the IdleBuildBehavior.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/axe-the-ibb/+merge/182814

buildd-manager's Great Big New IdleBuildBehavior is killing our small businesses. It serves no purpose now, as BuilderInteractor only calls into the behavior when starting and finishing builds, and when verifying that a slave has the right job.
-- 
https://code.launchpad.net/~wgrant/launchpad/axe-the-ibb/+merge/182814
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/axe-the-ibb into lp:launchpad.

=== modified file 'lib/lp/buildmaster/doc/buildfarmjobbehavior.txt'
--- lib/lp/buildmaster/doc/buildfarmjobbehavior.txt	2013-08-29 04:21:49 +0000
+++ lib/lp/buildmaster/doc/buildfarmjobbehavior.txt	2013-08-29 04:21:49 +0000
@@ -79,27 +79,3 @@
 
 Please refer to the IBuildFarmJobBehavior interface to see the currently
 provided build-type specific customisation points.
-
-
-The IdleBuildBehavior
----------------------
-
-If a Builder does not have a currentjob (and therefore an appropriate
-build behavior) it will automatically get the special IdleBuildBehavior
-to ensure that it fulfills its contract to implement IBuildFarmJobBehavior.
-
-First, we'll reset the current build behavior and destroy the current job.
-
-    >>> bob.currentjob.destroySelf()
-
-Attempting to use any other build-related functionality when a builder is
-idle, such as making a call to log the start of a build, will raise an
-appropriate exception.
-
-    >>> from lp.buildmaster.interactor import BuilderInteractor
-    >>> interactor = BuilderInteractor(bob)
-    >>> interactor._current_build_behavior.logStartBuild(None)
-    Traceback (most recent call last):
-     ...
-    BuildBehaviorMismatch: Builder was idle when asked to log the start of a
-    build.

=== modified file 'lib/lp/buildmaster/interactor.py'
--- lib/lp/buildmaster/interactor.py	2013-08-29 04:21:49 +0000
+++ lib/lp/buildmaster/interactor.py	2013-08-29 04:21:49 +0000
@@ -35,7 +35,6 @@
 from lp.buildmaster.interfaces.buildfarmjobbehavior import (
     IBuildFarmJobBehavior,
     )
-from lp.buildmaster.model.buildfarmjobbehavior import IdleBuildBehavior
 from lp.services import encoding
 from lp.services.config import config
 from lp.services.helpers import filenameToContentType
@@ -272,9 +271,8 @@
         # builder.currentjob changes.
         currentjob = self.builder.currentjob
         if currentjob is None:
-            if not isinstance(
-                    self._cached_build_behavior, IdleBuildBehavior):
-                self._cached_build_behavior = IdleBuildBehavior()
+            self._cached_build_behavior = None
+            self._cached_currentjob = None
         elif currentjob != self._cached_currentjob:
             self._cached_build_behavior = IBuildFarmJobBehavior(
                 currentjob.specific_job)
@@ -327,7 +325,7 @@
 
     def verifySlaveBuildCookie(self, slave_build_cookie):
         """See `IBuildFarmJobBehavior`."""
-        if isinstance(self._current_build_behavior, IdleBuildBehavior):
+        if self._current_build_behavior is None:
             raise CorruptBuildCookie('No job assigned to builder')
         good_cookie = self._current_build_behavior.generateSlaveBuildCookie()
         if slave_build_cookie != good_cookie:

=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py	2013-08-29 04:21:49 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py	2013-08-29 04:21:49 +0000
@@ -7,7 +7,6 @@
 
 __all__ = [
     'BuildFarmJobBehaviorBase',
-    'IdleBuildBehavior',
     ]
 
 import datetime
@@ -308,25 +307,3 @@
         yield self._interactor.cleanSlave()
         self.build.buildqueue_record.reset()
         transaction.commit()
-
-
-class IdleBuildBehavior(BuildFarmJobBehaviorBase):
-
-    implements(IBuildFarmJobBehavior)
-
-    def __init__(self):
-        """The idle behavior is special in that a buildfarmjob is not
-        specified during initialization as it is not the result of an
-        adaption.
-        """
-        super(IdleBuildBehavior, self).__init__(None)
-
-    def logStartBuild(self, logger):
-        """See `IBuildFarmJobBehavior`."""
-        raise BuildBehaviorMismatch(
-            "Builder was idle when asked to log the start of a build.")
-
-    def dispatchBuildToSlave(self, build_queue_item_id, logger):
-        """See `IBuildFarmJobBehavior`."""
-        raise BuildBehaviorMismatch(
-            "Builder was idle when asked to dispatch a build to the slave.")

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2013-08-29 04:21:49 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2013-08-29 04:21:49 +0000
@@ -24,10 +24,7 @@
 from twisted.python.failure import Failure
 from twisted.web.client import getPage
 from zope.component import getUtility
-from zope.security.proxy import (
-    isinstance as zope_isinstance,
-    removeSecurityProxy,
-    )
+from zope.security.proxy import removeSecurityProxy
 
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interactor import (
@@ -42,7 +39,6 @@
     IBuilderSet,
     )
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
-from lp.buildmaster.model.buildfarmjobbehavior import IdleBuildBehavior
 from lp.buildmaster.model.buildqueue import BuildQueue
 from lp.buildmaster.tests.mock_slaves import (
     AbortedSlave,
@@ -218,6 +214,7 @@
 
         return d.addCallback(check_build_started)
 
+    @inlineCallbacks
     def test_recover_building_slave_with_job_that_finished_elsewhere(self):
         # See bug 671242
         # When a job is destroyed, the builder's behaviour should be reset
@@ -230,22 +227,12 @@
         candidate.markAsBuilding(builder)
 
         # At this point we should see a valid behaviour on the builder:
-        self.assertFalse(
-            zope_isinstance(
-                interactor._current_build_behavior, IdleBuildBehavior))
-
-        # Now reset the job and try to rescue the builder.
+        self.assertIsNot(None, interactor._current_build_behavior)
+        yield interactor.rescueIfLost()
+        self.assertIsNot(None, interactor._current_build_behavior)
         candidate.destroySelf()
-        self.layer.txn.commit()
-        builder = getUtility(IBuilderSet)[builder.name]
-        d = interactor.rescueIfLost()
-
-        def check_builder(ignored):
-            self.assertIsInstance(
-                removeSecurityProxy(interactor._current_build_behavior),
-                IdleBuildBehavior)
-
-        return d.addCallback(check_builder)
+        yield interactor.rescueIfLost()
+        self.assertIs(None, interactor._current_build_behavior)
 
 
 class TestBuilderInteractor(TestCase):
@@ -281,8 +268,7 @@
 
     def test_verifySlaveBuildCookie_idle(self):
         interactor = BuilderInteractor(MockBuilder())
-        self.assertTrue(
-            isinstance(interactor._current_build_behavior, IdleBuildBehavior))
+        self.assertIs(None, interactor._current_build_behavior)
         self.assertRaises(
             CorruptBuildCookie, interactor.verifySlaveBuildCookie, 'foo')
 
@@ -873,11 +859,8 @@
         self.buildfarmjob = self.build.buildqueue_record.specific_job
 
     def test_idle_behavior_when_no_current_build(self):
-        """We return an idle behavior when there is no behavior specified
-        nor a current build.
-        """
-        self.assertIsInstance(
-            self.interactor._current_build_behavior, IdleBuildBehavior)
+        """An idle builder has no build behavior."""
+        self.assertIs(None, self.interactor._current_build_behavior)
 
     def test_current_job_behavior(self):
         """The current behavior is set automatically from the current job."""