launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/reset-fail-count-bug-680442 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #680442 Builder failure_count should be reset when marking it as "ok"
  https://bugs.launchpad.net/bugs/680442


This branch makes sure that:
 * when someone marks a builder as "OK" we reset its failure_count so that if the next job dispatch fails, the builder is not immediately failed again.
 * when a job is assessed as failed, reset the builder failure_count for the same reason.

I had to change the builderok Storm column into a private attribute so that we can hook into its changes via a new builderok property().  This caused some queries in IBuilder to require updating to use the new column name.

This was done like that because unfortunately builderok is already exported on the webservice so we can't make it read only and use a set method that does everything.


-- 
https://code.launchpad.net/~julian-edwards/launchpad/reset-fail-count-bug-680442/+merge/41966
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/reset-fail-count-bug-680442 into lp:launchpad.

=== modified file 'lib/lp/buildmaster/manager.py'
--- lib/lp/buildmaster/manager.py	2010-11-17 21:47:47 +0000
+++ lib/lp/buildmaster/manager.py	2010-11-26 17:11:32 +0000
@@ -89,6 +89,7 @@
         # and remove the buildqueue request.  The failure should
         # have already caused any relevant slave data to be stored
         # on the build record so don't worry about that here.
+        builder.resetFailureCount()
         build_job = current_job.specific_job.build
         build_job.status = BuildStatus.FAILEDTOBUILD
         builder.currentjob.destroySelf()

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2010-11-25 17:14:03 +0000
+++ lib/lp/buildmaster/model/builder.py	2010-11-26 17:11:32 +0000
@@ -373,7 +373,7 @@
     owner = ForeignKey(
         dbName='owner', foreignKey='Person',
         storm_validator=validate_public_person, notNull=True)
-    builderok = BoolCol(dbName='builderok', notNull=True)
+    _builderok = BoolCol(dbName='builderok', notNull=True)
     failnotes = StringCol(dbName='failnotes')
     virtualized = BoolCol(dbName='virtualized', default=True, notNull=True)
     speedindex = IntCol(dbName='speedindex')
@@ -428,6 +428,16 @@
     current_build_behavior = property(
         _getCurrentBuildBehavior, _setCurrentBuildBehavior)
 
+    def _getBuilderok(self):
+        return self._builderok
+
+    def _setBuilderok(self, value):
+        self._builderok = value
+        if value is True:
+            self.resetFailureCount()
+
+    builderok = property(_getBuilderok, _setBuilderok)
+
     def gotFailure(self):
         """See `IBuilder`."""
         self.failure_count += 1
@@ -836,7 +846,7 @@
         return Builder(processor=processor, url=url, name=name, title=title,
                        description=description, owner=owner, active=active,
                        virtualized=virtualized, vm_host=vm_host,
-                       builderok=True, manual=manual)
+                       _builderok=True, manual=manual)
 
     def get(self, builder_id):
         """See IBuilderSet."""
@@ -884,5 +894,5 @@
 
     def getBuildersForQueue(self, processor, virtualized):
         """See `IBuilderSet`."""
-        return Builder.selectBy(builderok=True, processor=processor,
+        return Builder.selectBy(_builderok=True, processor=processor,
                                 virtualized=virtualized)

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2010-11-25 17:02:23 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2010-11-26 17:11:32 +0000
@@ -86,7 +86,7 @@
 from lp.testing.fakemethod import FakeMethod
 
 
-class TestBuilder(TestCaseWithFactory):
+class TestBuilderBasics(TestCaseWithFactory):
     """Basic unit tests for `Builder`."""
 
     layer = DatabaseFunctionalLayer
@@ -123,6 +123,14 @@
         bq = builder.getBuildQueue()
         self.assertIs(None, bq)
 
+    def test_setting_builderok_resets_failure_count(self):
+        builder = removeSecurityProxy(self.factory.makeBuilder())
+        builder.failure_count = 1
+        builder.builderok = False
+        self.assertEqual(1, builder.failure_count)
+        builder.builderok = True
+        self.assertEqual(0, builder.failure_count)
+
 
 class TestBuilder(TestCaseWithFactory):
 

=== modified file 'lib/lp/buildmaster/tests/test_manager.py'
--- lib/lp/buildmaster/tests/test_manager.py	2010-10-31 17:15:34 +0000
+++ lib/lp/buildmaster/tests/test_manager.py	2010-11-26 17:11:32 +0000
@@ -480,10 +480,13 @@
 
     def test_job_failing_more_than_builder_fails_job(self):
         self.builder.getCurrentBuildFarmJob().gotFailure()
+        self.builder.getCurrentBuildFarmJob().gotFailure()
+        self.builder.gotFailure()
 
         assessFailureCounts(self.builder, "failnotes")
         self.assertIs(None, self.builder.currentjob)
         self.assertEqual(self.build.status, BuildStatus.FAILEDTOBUILD)
+        self.assertEqual(0, self.builder.failure_count)
 
     def test_builder_failing_more_than_job_but_under_fail_threshold(self):
         self.builder.failure_count = Builder.FAILURE_THRESHOLD - 1

=== modified file 'lib/lp/soyuz/model/buildpackagejob.py'
--- lib/lp/soyuz/model/buildpackagejob.py	2010-08-27 11:19:54 +0000
+++ lib/lp/soyuz/model/buildpackagejob.py	2010-11-26 17:11:32 +0000
@@ -23,6 +23,7 @@
 from lp.buildmaster.model.buildfarmjob import BuildFarmJobOldDerived
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.sourcepackage import SourcePackageUrgency
+from lp.buildmaster.interfaces.builder import IBuilderSet
 from lp.soyuz.enums import (
     ArchivePurpose,
     PackagePublishingStatus,
@@ -229,8 +230,8 @@
         # The extra clause is only used if the number of available
         # builders is greater than one, or nothing would get dispatched
         # at all.
-        num_arch_builders = Builder.selectBy(
-            processor=processor, manual=False, builderok=True).count()
+        num_arch_builders = getUtility(IBuilderSet).getBuildersForQueue(
+            processor, virtualized).count()
         if num_arch_builders > 1:
             sub_query += """
             AND Archive.id NOT IN (