Merge ~cjwatson/launchpad:bq-properly-clear-builder into launchpad:master

Proposed by Colin Watson
Status: Merged
Approved by: Colin Watson
Approved revision: bfea9d7db5ad5cda4644924182ec012dc1643c32
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad:bq-properly-clear-builder
Merge into: launchpad:master
Diff against target: 94 lines (+18/-2)
5 files modified
lib/lp/buildmaster/doc/buildqueue.txt (+2/-0)
lib/lp/buildmaster/interfaces/buildfarmjob.py (+7/-0)
lib/lp/buildmaster/model/buildfarmjob.py (+5/-1)
lib/lp/buildmaster/model/buildqueue.py (+2/-1)
lib/lp/buildmaster/tests/test_manager.py (+2/-0)
Reviewer Review Type Date Requested Status
Jürgen Gmach Approve
Review via email: mp+415207@code.launchpad.net

Commit message

Properly clear builder from BuildQueue.reset

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Jürgen Gmach (jugmac00) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/buildmaster/doc/buildqueue.txt b/lib/lp/buildmaster/doc/buildqueue.txt
2index e9bb146..6d6cbe3 100644
3--- a/lib/lp/buildmaster/doc/buildqueue.txt
4+++ b/lib/lp/buildmaster/doc/buildqueue.txt
5@@ -132,6 +132,8 @@ to another build. The score value of the job is preserved.
6 None
7 >>> print(job.logtail)
8 None
9+ >>> print(build.builder)
10+ None
11 >>> print(build.status.name)
12 NEEDSBUILD
13 >>> print(job.lastscore)
14diff --git a/lib/lp/buildmaster/interfaces/buildfarmjob.py b/lib/lp/buildmaster/interfaces/buildfarmjob.py
15index cb5b2f7..92fc062 100644
16--- a/lib/lp/buildmaster/interfaces/buildfarmjob.py
17+++ b/lib/lp/buildmaster/interfaces/buildfarmjob.py
18@@ -277,6 +277,13 @@ class IBuildFarmJobView(Interface):
19 title=_("Can be cancelled"), required=True, readonly=True,
20 description=_("Whether this build record can be cancelled.")))
21
22+ def clearBuilder():
23+ """Clear this build record's builder.
24+
25+ This is called by `BuildQueue.reset` as part of resetting jobs so
26+ that they can be re-dispatched.
27+ """
28+
29
30 class IBuildFarmJobEdit(Interface):
31 """`IBuildFarmJob` methods that require launchpad.Edit."""
32diff --git a/lib/lp/buildmaster/model/buildfarmjob.py b/lib/lp/buildmaster/model/buildfarmjob.py
33index 807854b..977c83e 100644
34--- a/lib/lp/buildmaster/model/buildfarmjob.py
35+++ b/lib/lp/buildmaster/model/buildfarmjob.py
36@@ -304,12 +304,16 @@ class BuildFarmJobMixin:
37 ]
38 return self.status in cancellable_statuses
39
40+ def clearBuilder(self):
41+ """See `IBuildFarmJob`."""
42+ self.build_farm_job.builder = self.builder = None
43+
44 def resetBuild(self):
45 """See `IBuildFarmJob`."""
46 self.build_farm_job.status = self.status = BuildStatus.NEEDSBUILD
47 self.build_farm_job.date_finished = self.date_finished = None
48 self.date_started = None
49- self.build_farm_job.builder = self.builder = None
50+ self.clearBuilder()
51 self.log = None
52 self.upload_log = None
53 self.dependencies = None
54diff --git a/lib/lp/buildmaster/model/buildqueue.py b/lib/lp/buildmaster/model/buildqueue.py
55index 1bc3b0d..ab531f3 100644
56--- a/lib/lp/buildmaster/model/buildqueue.py
57+++ b/lib/lp/buildmaster/model/buildqueue.py
58@@ -188,7 +188,7 @@ class BuildQueue(StormBase):
59 self.builder = builder
60 self.status = BuildQueueStatus.RUNNING
61 self.date_started = UTC_NOW
62- self.specific_build.updateStatus(BuildStatus.BUILDING)
63+ self.specific_build.updateStatus(BuildStatus.BUILDING, builder=builder)
64 if builder is not None:
65 del get_property_cache(builder).currentjob
66
67@@ -212,6 +212,7 @@ class BuildQueue(StormBase):
68 self.status = BuildQueueStatus.WAITING
69 self.date_started = None
70 self.logtail = None
71+ self.specific_build.clearBuilder()
72 self.specific_build.updateStatus(BuildStatus.NEEDSBUILD)
73 if builder is not None:
74 del get_property_cache(builder).currentjob
75diff --git a/lib/lp/buildmaster/tests/test_manager.py b/lib/lp/buildmaster/tests/test_manager.py
76index cf6f584..e5441ea 100644
77--- a/lib/lp/buildmaster/tests/test_manager.py
78+++ b/lib/lp/buildmaster/tests/test_manager.py
79@@ -1360,6 +1360,7 @@ class TestFailureAssessments(TestCaseWithFactory):
80 self.assertIn("Requeueing job %s" % self.build.build_cookie, log)
81 self.assertIn("Dirtying builder %s" % self.builder.name, log)
82 self.assertIs(None, self.builder.currentjob)
83+ self.assertIs(None, self.build.builder)
84 self.assertEqual(BuilderCleanStatus.DIRTY, self.builder.clean_status)
85 self.assertTrue(self.builder.builderok)
86
87@@ -1371,6 +1372,7 @@ class TestFailureAssessments(TestCaseWithFactory):
88 self.assertIn("Requeueing job", log)
89 self.assertIn("Failing builder", log)
90 self.assertIs(None, self.builder.currentjob)
91+ self.assertIs(None, self.build.builder)
92 self.assertEqual(BuilderCleanStatus.DIRTY, self.builder.clean_status)
93 self.assertFalse(self.builder.builderok)
94 self.assertEqual("failnotes", self.builder.failnotes)

Subscribers

People subscribed via source and target branches

to status/vote changes: