Launchpad itself

Merge lp:~wgrant/launchpad/refactor-slavestatus into lp:launchpad

  1. refactor-slavestatus
  2. Merge into devel
Proposed by William Grant
Status: Merged
Approved by: Brad Crittenden
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~wgrant/launchpad/refactor-slavestatus
Merge into: lp:launchpad
Diff against target: 215 lines (+51/-70)
7 files modified
lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py (+6/-4)
lib/lp/buildmaster/model/builder.py (+11/-2)
lib/lp/buildmaster/model/buildfarmjobbehavior.py (+2/-2)
lib/lp/code/model/recipebuilder.py (+11/-23)
lib/lp/soyuz/model/binarypackagebuildbehavior.py (+11/-24)
lib/lp/translations/model/translationtemplatesbuildbehavior.py (+4/-14)
lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py (+6/-1)
To merge this branch: bzr merge lp:~wgrant/launchpad/refactor-slavestatus
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+22285@code.launchpad.net

Commit message

Factor out common IBuildFarmJobBehavior.slaveStatus behaviour into Builder.slaveStatus.

Description of the change

This branch shuffles common things from IBuildFarmJobBehavior.slaveStatus implementations up into Builder.slaveStatus. In particular build_id, build_status and logtail (all set by the base class on the slave) are extracted for all job types if present, in addition to builder_status which has always been extracted there. This vastly simplifies the slaveStatus methods on the IBFJBs.

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

William thanks for the fix. The logic of what you have done looks good.

However, I don't like the idiom you've employed of mutating the status dictionary inside the slaveStatus methods and then returning it. If you didn't want to mutate the original dictionary then making a copy, changing it, and returning that copy would make sense. Otherwise just modifying the status dictionary in place and not returning it is a reasonable thing to do (and preferred barring no solid reason to do the former). But to mutate and then return would lead one to think the dictionary was not modified in place, which we know it was.

Changing the method to mutate will necessitate a small change to your new test code.

I'm marking this approved rather than needs fixing with the expectation you'll make that change. If you would prefer to discuss it further we can and I'd be eager to hear your thoughts.

As usual, I'm happy to land this for you when it is ready.

review: Approve (code)
Revision history for this message
William Grant (wgrant) wrote :

> William thanks for the fix. The logic of what you have done looks good.
>
> However, I don't like the idiom you've employed of mutating the status
> dictionary inside the slaveStatus methods and then returning it. If you
> didn't want to mutate the original dictionary then making a copy, changing it,
> and returning that copy would make sense. Otherwise just modifying the status
> dictionary in place and not returning it is a reasonable thing to do (and
> preferred barring no solid reason to do the former). But to mutate and then
> return would lead one to think the dictionary was not modified in place, which
> we know it was.

Thanks for the review, Brad. I was a little concerned about that too, but wasn't
entirely sure of the cleanest solution. I've adopted your suggestion, and also
renamed the method to updateSlaveStatus to more accurately reflect its purpose.

If you approve, please land this once PQM opens.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py'
2--- lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py 2010-03-12 16:50:45 +0000
3+++ lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py 2010-03-31 04:20:47 +0000
4@@ -48,13 +48,15 @@
5 :param logger: A logger to be used to log diagnostic information.
6 """
7
8- def slaveStatus(raw_slave_status):
9- """Return a dict of custom slave status values for this behavior.
10+ def updateSlaveStatus(raw_slave_status, status):
11+ """Update the slave status dict with custom values for this behavior.
12
13 :param raw_slave_status: The value returned by the build slave's
14 status() method.
15- :return: a dict of extra key/values to be included in the result
16- of IBuilder.slaveStatus().
17+ :param status: A dict of the processed slave status values provided
18+ by all types: builder_status, build_id, and optionally build_status
19+ or logtail. This should have any behaviour-specific values
20+ added to it.
21 """
22
23 def verifySlaveBuildID(slave_build_id):
24
25=== modified file 'lib/lp/buildmaster/model/builder.py'
26--- lib/lp/buildmaster/model/builder.py 2010-03-26 01:52:07 +0000
27+++ lib/lp/buildmaster/model/builder.py 2010-03-31 04:20:47 +0000
28@@ -438,8 +438,17 @@
29 status_sentence = self.slave.status()
30
31 status = {'builder_status': status_sentence[0]}
32- status.update(
33- self.current_build_behavior.slaveStatus(status_sentence))
34+
35+ # Extract detailed status, ID and log information if present.
36+ if status['builder_status'] == 'BuilderStatus.WAITING':
37+ status['build_status'] = status_sentence[1]
38+ status['build_id'] = status_sentence[2]
39+ else:
40+ status['build_id'] = status_sentence[1]
41+ if status['builder_status'] == 'BuilderStatus.BUILDING':
42+ status['logtail'] = status_sentence[2]
43+
44+ self.current_build_behavior.updateSlaveStatus(status_sentence, status)
45 return status
46
47 def slaveStatusSentence(self):
48
49=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
50--- lib/lp/buildmaster/model/buildfarmjobbehavior.py 2010-03-12 16:10:12 +0000
51+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py 2010-03-31 04:20:47 +0000
52@@ -58,11 +58,11 @@
53 """The default behavior is a no-op."""
54 pass
55
56- def slaveStatus(self, raw_slave_status):
57+ def updateSlaveStatus(self, raw_slave_status, status):
58 """See `IBuildFarmJobBehavior`.
59
60 The default behavior is that we don't add any extra values."""
61- return {}
62+ pass
63
64 def _helpVerifyBuildIDComponent(self, raw_id, item_type, finder):
65 """Helper for verifying parts of a `BuildFarmJob` name.
66
67=== modified file 'lib/lp/code/model/recipebuilder.py'
68--- lib/lp/code/model/recipebuilder.py 2010-03-13 10:40:45 +0000
69+++ lib/lp/code/model/recipebuilder.py 2010-03-31 04:20:47 +0000
70@@ -140,35 +140,23 @@
71 (build.title, build.id, build.pocket.name,
72 build.distroseries.name))
73
74- def slaveStatus(self, raw_slave_status):
75- """Parse and return the binary build specific status info.
76+ def updateSlaveStatus(self, raw_slave_status, status):
77+ """Parse the recipe build specific status info into the status dict.
78
79 This includes:
80- * build_id => string
81- * build_status => string or None
82- * logtail => string or None
83 * filemap => dictionary or None
84 * dependencies => string or None
85 """
86- builder_status = raw_slave_status[0]
87- extra_info = {}
88- if builder_status == 'BuilderStatus.WAITING':
89- extra_info['build_status'] = raw_slave_status[1]
90- extra_info['build_id'] = raw_slave_status[2]
91- build_status_with_files = [
92- 'BuildStatus.OK',
93- 'BuildStatus.PACKAGEFAIL',
94- 'BuildStatus.DEPFAIL',
95- ]
96- if extra_info['build_status'] in build_status_with_files:
97- extra_info['filemap'] = raw_slave_status[3]
98- extra_info['dependencies'] = raw_slave_status[4]
99- else:
100- extra_info['build_id'] = raw_slave_status[1]
101- if builder_status == 'BuilderStatus.BUILDING':
102- extra_info['logtail'] = raw_slave_status[2]
103+ build_status_with_files = (
104+ 'BuildStatus.OK',
105+ 'BuildStatus.PACKAGEFAIL',
106+ 'BuildStatus.DEPFAIL',
107+ )
108+ if (status['builder_status'] == 'BuilderStatus.WAITING' and
109+ status['build_status'] in build_status_with_files):
110+ status['filemap'] = raw_slave_status[3]
111+ status['dependencies'] = raw_slave_status[4]
112
113- return extra_info
114
115 def getVerifiedBuild(self, raw_id):
116 """See `IBuildFarmJobBehavior`."""
117
118=== modified file 'lib/lp/soyuz/model/binarypackagebuildbehavior.py'
119--- lib/lp/soyuz/model/binarypackagebuildbehavior.py 2010-01-22 04:01:17 +0000
120+++ lib/lp/soyuz/model/binarypackagebuildbehavior.py 2010-03-31 04:20:47 +0000
121@@ -129,35 +129,22 @@
122 (build.title, build.id, build.pocket.name,
123 build.distroseries.name))
124
125- def slaveStatus(self, raw_slave_status):
126- """Parse and return the binary build specific status info.
127+ def updateSlaveStatus(self, raw_slave_status, status):
128+ """Parse the binary build specific status info into the status dict.
129
130 This includes:
131- * build_id => string
132- * build_status => string or None
133- * logtail => string or None
134 * filemap => dictionary or None
135 * dependencies => string or None
136 """
137- builder_status = raw_slave_status[0]
138- extra_info = {}
139- if builder_status == 'BuilderStatus.WAITING':
140- extra_info['build_status'] = raw_slave_status[1]
141- extra_info['build_id'] = raw_slave_status[2]
142- build_status_with_files = [
143- 'BuildStatus.OK',
144- 'BuildStatus.PACKAGEFAIL',
145- 'BuildStatus.DEPFAIL',
146- ]
147- if extra_info['build_status'] in build_status_with_files:
148- extra_info['filemap'] = raw_slave_status[3]
149- extra_info['dependencies'] = raw_slave_status[4]
150- else:
151- extra_info['build_id'] = raw_slave_status[1]
152- if builder_status == 'BuilderStatus.BUILDING':
153- extra_info['logtail'] = raw_slave_status[2]
154-
155- return extra_info
156+ build_status_with_files = (
157+ 'BuildStatus.OK',
158+ 'BuildStatus.PACKAGEFAIL',
159+ 'BuildStatus.DEPFAIL',
160+ )
161+ if (status['builder_status'] == 'BuilderStatus.WAITING' and
162+ status['build_status'] in build_status_with_files):
163+ status['filemap'] = raw_slave_status[3]
164+ status['dependencies'] = raw_slave_status[4]
165
166 def _cachePrivateSourceOnSlave(self, logger):
167 """Ask the slave to download source files for a private build.
168
169=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehavior.py'
170--- lib/lp/translations/model/translationtemplatesbuildbehavior.py 2010-03-24 12:26:27 +0000
171+++ lib/lp/translations/model/translationtemplatesbuildbehavior.py 2010-03-31 04:20:47 +0000
172@@ -93,21 +93,11 @@
173 queue.addOrUpdateEntriesFromTarball(
174 tarball, False, branch.owner, productseries=series)
175
176- def slaveStatus(self, raw_slave_status):
177+ def updateSlaveStatus(self, raw_slave_status, status):
178 """See `IBuildFarmJobBehavior`."""
179- builder_status = raw_slave_status[0]
180-
181- if builder_status == 'BuilderStatus.WAITING':
182- extra_info = {
183- 'build_status': raw_slave_status[1],
184- 'build_id': raw_slave_status[2],
185- }
186- if len(raw_slave_status) >= 3:
187- extra_info['filemap'] = raw_slave_status[3]
188- return extra_info
189- else:
190- # Nothing special to do for other states.
191- return {}
192+ if status['builder_status'] == 'BuilderStatus.WAITING':
193+ if len(raw_slave_status) >= 4:
194+ status['filemap'] = raw_slave_status[3]
195
196 def updateBuild_WAITING(self, queue_item, slave_status, logtail, logger):
197 """Deal with a finished ("WAITING" state, perversely) build job.
198
199=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
200--- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py 2010-03-25 23:19:10 +0000
201+++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py 2010-03-31 04:20:47 +0000
202@@ -185,7 +185,12 @@
203 self.assertEqual(0, builder.cleanSlave.call_count)
204 self.assertEqual(0, behavior._uploadTarball.call_count)
205
206- slave_status = behavior.slaveStatus(builder.slave.status())
207+ slave_status = {
208+ 'builder_status': builder.slave.status()[0],
209+ 'build_status': builder.slave.status()[1],
210+ 'build_id': builder.slave.status()[2]
211+ }
212+ behavior.updateSlaveStatus(builder.slave.status(), slave_status)
213 behavior.updateBuild_WAITING(queue_item, slave_status, None, logging)
214
215 self.assertEqual(1, queue_item.destroySelf.call_count)