launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/buildd-manager-fixes into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #671242 New buildd-manager disabling everything in sight
  https://bugs.launchpad.net/bugs/671242


The build manager is exhibiting strange behaviour as described in the linked 
bug (read it for more info).

The upshot is that IBuilder was relying on a cached property that was not 
being invalidated when the storm data was.  This is trivally fixed with an 
invalidation hook.
-- 
https://code.launchpad.net/~julian-edwards/launchpad/buildd-manager-fixes/+merge/40532
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/buildd-manager-fixes into lp:launchpad/devel.

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2010-10-27 14:25:19 +0000
+++ lib/lp/buildmaster/model/builder.py	2010-11-10 13:16:49 +0000
@@ -350,6 +350,11 @@
     # give up and mark it builderok=False.
     FAILURE_THRESHOLD = 5
 
+    def __storm_invalidated__(self):
+        """Clear cached properties."""
+        super(Builder, self).__storm_invalidated__()
+        self._current_build_behavior = None
+
     def _getCurrentBuildBehavior(self):
         """Return the current build behavior."""
         if not safe_hasattr(self, '_current_build_behavior'):

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2010-10-27 14:25:19 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2010-11-10 13:16:49 +0000
@@ -117,7 +117,7 @@
         self.assertIs(None, bq)
 
 
-class TestBuilderWithTrial(TrialTestCase):
+class TestBuilderWithTrialBase(TrialTestCase):
 
     layer = TwistedLaunchpadZopelessLayer
 
@@ -130,6 +130,9 @@
         login_as(ANONYMOUS)
         self.addCleanup(logout)
 
+
+class TestBuilderWithTrial(TestBuilderWithTrialBase):
+
     def test_updateStatus_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.
@@ -188,9 +191,7 @@
         d = builder.handleTimeout(QuietFakeLogger(), 'blah')
         return self.assertFailure(d, CannotResumeHost)
 
-    def _setupRecipeBuildAndBuilder(self):
-        # Helper function to make a builder capable of building a
-        # recipe, returning both.
+    def _setupBuilder(self):
         processor = self.factory.makeProcessor(name="i386")
         builder = self.factory.makeBuilder(
             processor=processor, virtualized=True, vm_host="bladh")
@@ -202,10 +203,24 @@
         chroot = self.factory.makeLibraryFileAlias()
         das.addOrUpdateChroot(chroot)
         distroseries.nominatedarchindep = das
+        return builder, distroseries, das
+
+    def _setupRecipeBuildAndBuilder(self):
+        # Helper function to make a builder capable of building a
+        # recipe, returning both.
+        builder, distroseries, distroarchseries = self._setupBuilder()
         build = self.factory.makeSourcePackageRecipeBuild(
             distroseries=distroseries)
         return builder, build
 
+    def _setupBinaryBuildAndBuilder(self):
+        # Helper function to make a builder capable of building a
+        # binary package, returning both.
+        builder, distroseries, distroarchseries = self._setupBuilder()
+        build = self.factory.makeBinaryPackageBuild(
+            distroarchseries=distroarchseries, builder=builder)
+        return builder, build
+
     def test_findAndStartJob_returns_candidate(self):
         # findAndStartJob finds the next queued job using _findBuildCandidate.
         # We don't care about the type of build at all.
@@ -308,8 +323,33 @@
             self.assertNotIn('clean', building_slave.call_log)
         return d.addCallback(check_slave_calls)
 
-
-class TestBuilderSlaveStatus(TestBuilderWithTrial):
+    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
+        # too so that we don't traceback when the wrong behaviour tries
+        # to access a non-existent job.
+        builder, build = self._setupBinaryBuildAndBuilder()
+        candidate = build.queueBuild()
+        building_slave = BuildingSlave()
+        builder.setSlaveForTesting(building_slave)
+        candidate.markAsBuilding(builder)
+
+        # At this point we should see a valid behaviour on the builder:
+        self.assertNotIdentical(
+            IdleBuildBehavior, builder.current_build_behavior)
+
+        # Now reset the job and try to rescue the builder.
+        candidate.destroySelf()
+        self.layer.txn.commit()
+        builder = getUtility(IBuilderSet)[builder.name]
+
+        d = builder.rescueIfLost()
+        def check_builder(ignored):
+            self.assertIdentical(
+                IdleBuildBehavior, builder.current_build_behavior)
+
+
+class TestBuilderSlaveStatus(TestBuilderWithTrialBase):
 
     # Verify what IBuilder.slaveStatus returns with slaves in different
     # states.