launchpad-reviewers team mailing list archive

[Colin Watson <mp+415207@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:bq-properly-clear-builder into launchpad:master.

Commit message:
Properly clear builder from BuildQueue.reset

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1960132 in Launchpad itself: "Assertion failed: self.builder == builder"
  https://bugs.launchpad.net/launchpad/+bug/1960132

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/415207

Following https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/414070, the `builder` attribute of build farm jobs is now set while the build is in progress (and indeed this commit sets it slightly earlier as well, from `BuildQueue.markAsBuilding`, mainly to maximize the chance of discovering problems in tests).  This has various advantages.

However, `BuildQueue.reset` can be called when a builder fails, upon which the job is likely to be dispatched to a different builder.  It cleared `BuildQueue.builder` as part of resetting the build queue entry, but not the corresponding attributes of the `BuildFarmJob` or the specific build, which caused an assertion failure later on.  Repair this by clearing those attributes too.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:bq-properly-clear-builder into launchpad:master.

diff --git a/lib/lp/buildmaster/doc/buildqueue.txt b/lib/lp/buildmaster/doc/buildqueue.txt
index e9bb146..6d6cbe3 100644
--- a/lib/lp/buildmaster/doc/buildqueue.txt
+++ b/lib/lp/buildmaster/doc/buildqueue.txt
@@ -132,6 +132,8 @@ to another build. The score value of the job is preserved.
     None
     >>> print(job.logtail)
     None
+    >>> print(build.builder)
+    None
     >>> print(build.status.name)
     NEEDSBUILD
     >>> print(job.lastscore)
diff --git a/lib/lp/buildmaster/interfaces/buildfarmjob.py b/lib/lp/buildmaster/interfaces/buildfarmjob.py
index cb5b2f7..92fc062 100644
--- a/lib/lp/buildmaster/interfaces/buildfarmjob.py
+++ b/lib/lp/buildmaster/interfaces/buildfarmjob.py
@@ -277,6 +277,13 @@ class IBuildFarmJobView(Interface):
         title=_("Can be cancelled"), required=True, readonly=True,
         description=_("Whether this build record can be cancelled.")))
 
+    def clearBuilder():
+        """Clear this build record's builder.
+
+        This is called by `BuildQueue.reset` as part of resetting jobs so
+        that they can be re-dispatched.
+        """
+
 
 class IBuildFarmJobEdit(Interface):
     """`IBuildFarmJob` methods that require launchpad.Edit."""
diff --git a/lib/lp/buildmaster/model/buildfarmjob.py b/lib/lp/buildmaster/model/buildfarmjob.py
index 807854b..977c83e 100644
--- a/lib/lp/buildmaster/model/buildfarmjob.py
+++ b/lib/lp/buildmaster/model/buildfarmjob.py
@@ -304,12 +304,16 @@ class BuildFarmJobMixin:
             ]
         return self.status in cancellable_statuses
 
+    def clearBuilder(self):
+        """See `IBuildFarmJob`."""
+        self.build_farm_job.builder = self.builder = None
+
     def resetBuild(self):
         """See `IBuildFarmJob`."""
         self.build_farm_job.status = self.status = BuildStatus.NEEDSBUILD
         self.build_farm_job.date_finished = self.date_finished = None
         self.date_started = None
-        self.build_farm_job.builder = self.builder = None
+        self.clearBuilder()
         self.log = None
         self.upload_log = None
         self.dependencies = None
diff --git a/lib/lp/buildmaster/model/buildqueue.py b/lib/lp/buildmaster/model/buildqueue.py
index 1bc3b0d..ab531f3 100644
--- a/lib/lp/buildmaster/model/buildqueue.py
+++ b/lib/lp/buildmaster/model/buildqueue.py
@@ -188,7 +188,7 @@ class BuildQueue(StormBase):
         self.builder = builder
         self.status = BuildQueueStatus.RUNNING
         self.date_started = UTC_NOW
-        self.specific_build.updateStatus(BuildStatus.BUILDING)
+        self.specific_build.updateStatus(BuildStatus.BUILDING, builder=builder)
         if builder is not None:
             del get_property_cache(builder).currentjob
 
@@ -212,6 +212,7 @@ class BuildQueue(StormBase):
         self.status = BuildQueueStatus.WAITING
         self.date_started = None
         self.logtail = None
+        self.specific_build.clearBuilder()
         self.specific_build.updateStatus(BuildStatus.NEEDSBUILD)
         if builder is not None:
             del get_property_cache(builder).currentjob
diff --git a/lib/lp/buildmaster/tests/test_manager.py b/lib/lp/buildmaster/tests/test_manager.py
index cf6f584..e5441ea 100644
--- a/lib/lp/buildmaster/tests/test_manager.py
+++ b/lib/lp/buildmaster/tests/test_manager.py
@@ -1360,6 +1360,7 @@ class TestFailureAssessments(TestCaseWithFactory):
         self.assertIn("Requeueing job %s" % self.build.build_cookie, log)
         self.assertIn("Dirtying builder %s" % self.builder.name, log)
         self.assertIs(None, self.builder.currentjob)
+        self.assertIs(None, self.build.builder)
         self.assertEqual(BuilderCleanStatus.DIRTY, self.builder.clean_status)
         self.assertTrue(self.builder.builderok)
 
@@ -1371,6 +1372,7 @@ class TestFailureAssessments(TestCaseWithFactory):
         self.assertIn("Requeueing job", log)
         self.assertIn("Failing builder", log)
         self.assertIs(None, self.builder.currentjob)
+        self.assertIs(None, self.build.builder)
         self.assertEqual(BuilderCleanStatus.DIRTY, self.builder.clean_status)
         self.assertFalse(self.builder.builderok)
         self.assertEqual("failnotes", self.builder.failnotes)