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
=== modified file 'lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py 2010-03-12 16:50:45 +0000
+++ lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py 2010-03-31 04:20:47 +0000
@@ -48,13 +48,15 @@
48 :param logger: A logger to be used to log diagnostic information.48 :param logger: A logger to be used to log diagnostic information.
49 """49 """
5050
51 def slaveStatus(raw_slave_status):51 def updateSlaveStatus(raw_slave_status, status):
52 """Return a dict of custom slave status values for this behavior.52 """Update the slave status dict with custom values for this behavior.
5353
54 :param raw_slave_status: The value returned by the build slave's54 :param raw_slave_status: The value returned by the build slave's
55 status() method.55 status() method.
56 :return: a dict of extra key/values to be included in the result56 :param status: A dict of the processed slave status values provided
57 of IBuilder.slaveStatus().57 by all types: builder_status, build_id, and optionally build_status
58 or logtail. This should have any behaviour-specific values
59 added to it.
58 """60 """
5961
60 def verifySlaveBuildID(slave_build_id):62 def verifySlaveBuildID(slave_build_id):
6163
=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py 2010-03-26 01:52:07 +0000
+++ lib/lp/buildmaster/model/builder.py 2010-03-31 04:20:47 +0000
@@ -438,8 +438,17 @@
438 status_sentence = self.slave.status()438 status_sentence = self.slave.status()
439439
440 status = {'builder_status': status_sentence[0]}440 status = {'builder_status': status_sentence[0]}
441 status.update(441
442 self.current_build_behavior.slaveStatus(status_sentence))442 # Extract detailed status, ID and log information if present.
443 if status['builder_status'] == 'BuilderStatus.WAITING':
444 status['build_status'] = status_sentence[1]
445 status['build_id'] = status_sentence[2]
446 else:
447 status['build_id'] = status_sentence[1]
448 if status['builder_status'] == 'BuilderStatus.BUILDING':
449 status['logtail'] = status_sentence[2]
450
451 self.current_build_behavior.updateSlaveStatus(status_sentence, status)
443 return status452 return status
444453
445 def slaveStatusSentence(self):454 def slaveStatusSentence(self):
446455
=== modified file 'lib/lp/buildmaster/model/buildfarmjobbehavior.py'
--- lib/lp/buildmaster/model/buildfarmjobbehavior.py 2010-03-12 16:10:12 +0000
+++ lib/lp/buildmaster/model/buildfarmjobbehavior.py 2010-03-31 04:20:47 +0000
@@ -58,11 +58,11 @@
58 """The default behavior is a no-op."""58 """The default behavior is a no-op."""
59 pass59 pass
6060
61 def slaveStatus(self, raw_slave_status):61 def updateSlaveStatus(self, raw_slave_status, status):
62 """See `IBuildFarmJobBehavior`.62 """See `IBuildFarmJobBehavior`.
6363
64 The default behavior is that we don't add any extra values."""64 The default behavior is that we don't add any extra values."""
65 return {}65 pass
6666
67 def _helpVerifyBuildIDComponent(self, raw_id, item_type, finder):67 def _helpVerifyBuildIDComponent(self, raw_id, item_type, finder):
68 """Helper for verifying parts of a `BuildFarmJob` name.68 """Helper for verifying parts of a `BuildFarmJob` name.
6969
=== modified file 'lib/lp/code/model/recipebuilder.py'
--- lib/lp/code/model/recipebuilder.py 2010-03-13 10:40:45 +0000
+++ lib/lp/code/model/recipebuilder.py 2010-03-31 04:20:47 +0000
@@ -140,35 +140,23 @@
140 (build.title, build.id, build.pocket.name,140 (build.title, build.id, build.pocket.name,
141 build.distroseries.name))141 build.distroseries.name))
142142
143 def slaveStatus(self, raw_slave_status):143 def updateSlaveStatus(self, raw_slave_status, status):
144 """Parse and return the binary build specific status info.144 """Parse the recipe build specific status info into the status dict.
145145
146 This includes:146 This includes:
147 * build_id => string
148 * build_status => string or None
149 * logtail => string or None
150 * filemap => dictionary or None147 * filemap => dictionary or None
151 * dependencies => string or None148 * dependencies => string or None
152 """149 """
153 builder_status = raw_slave_status[0]150 build_status_with_files = (
154 extra_info = {}151 'BuildStatus.OK',
155 if builder_status == 'BuilderStatus.WAITING':152 'BuildStatus.PACKAGEFAIL',
156 extra_info['build_status'] = raw_slave_status[1]153 'BuildStatus.DEPFAIL',
157 extra_info['build_id'] = raw_slave_status[2]154 )
158 build_status_with_files = [155 if (status['builder_status'] == 'BuilderStatus.WAITING' and
159 'BuildStatus.OK',156 status['build_status'] in build_status_with_files):
160 'BuildStatus.PACKAGEFAIL',157 status['filemap'] = raw_slave_status[3]
161 'BuildStatus.DEPFAIL',158 status['dependencies'] = raw_slave_status[4]
162 ]
163 if extra_info['build_status'] in build_status_with_files:
164 extra_info['filemap'] = raw_slave_status[3]
165 extra_info['dependencies'] = raw_slave_status[4]
166 else:
167 extra_info['build_id'] = raw_slave_status[1]
168 if builder_status == 'BuilderStatus.BUILDING':
169 extra_info['logtail'] = raw_slave_status[2]
170159
171 return extra_info
172160
173 def getVerifiedBuild(self, raw_id):161 def getVerifiedBuild(self, raw_id):
174 """See `IBuildFarmJobBehavior`."""162 """See `IBuildFarmJobBehavior`."""
175163
=== modified file 'lib/lp/soyuz/model/binarypackagebuildbehavior.py'
--- lib/lp/soyuz/model/binarypackagebuildbehavior.py 2010-01-22 04:01:17 +0000
+++ lib/lp/soyuz/model/binarypackagebuildbehavior.py 2010-03-31 04:20:47 +0000
@@ -129,35 +129,22 @@
129 (build.title, build.id, build.pocket.name,129 (build.title, build.id, build.pocket.name,
130 build.distroseries.name))130 build.distroseries.name))
131131
132 def slaveStatus(self, raw_slave_status):132 def updateSlaveStatus(self, raw_slave_status, status):
133 """Parse and return the binary build specific status info.133 """Parse the binary build specific status info into the status dict.
134134
135 This includes:135 This includes:
136 * build_id => string
137 * build_status => string or None
138 * logtail => string or None
139 * filemap => dictionary or None136 * filemap => dictionary or None
140 * dependencies => string or None137 * dependencies => string or None
141 """138 """
142 builder_status = raw_slave_status[0]139 build_status_with_files = (
143 extra_info = {}140 'BuildStatus.OK',
144 if builder_status == 'BuilderStatus.WAITING':141 'BuildStatus.PACKAGEFAIL',
145 extra_info['build_status'] = raw_slave_status[1]142 'BuildStatus.DEPFAIL',
146 extra_info['build_id'] = raw_slave_status[2]143 )
147 build_status_with_files = [144 if (status['builder_status'] == 'BuilderStatus.WAITING' and
148 'BuildStatus.OK',145 status['build_status'] in build_status_with_files):
149 'BuildStatus.PACKAGEFAIL',146 status['filemap'] = raw_slave_status[3]
150 'BuildStatus.DEPFAIL',147 status['dependencies'] = raw_slave_status[4]
151 ]
152 if extra_info['build_status'] in build_status_with_files:
153 extra_info['filemap'] = raw_slave_status[3]
154 extra_info['dependencies'] = raw_slave_status[4]
155 else:
156 extra_info['build_id'] = raw_slave_status[1]
157 if builder_status == 'BuilderStatus.BUILDING':
158 extra_info['logtail'] = raw_slave_status[2]
159
160 return extra_info
161148
162 def _cachePrivateSourceOnSlave(self, logger):149 def _cachePrivateSourceOnSlave(self, logger):
163 """Ask the slave to download source files for a private build.150 """Ask the slave to download source files for a private build.
164151
=== modified file 'lib/lp/translations/model/translationtemplatesbuildbehavior.py'
--- lib/lp/translations/model/translationtemplatesbuildbehavior.py 2010-03-24 12:26:27 +0000
+++ lib/lp/translations/model/translationtemplatesbuildbehavior.py 2010-03-31 04:20:47 +0000
@@ -93,21 +93,11 @@
93 queue.addOrUpdateEntriesFromTarball(93 queue.addOrUpdateEntriesFromTarball(
94 tarball, False, branch.owner, productseries=series)94 tarball, False, branch.owner, productseries=series)
9595
96 def slaveStatus(self, raw_slave_status):96 def updateSlaveStatus(self, raw_slave_status, status):
97 """See `IBuildFarmJobBehavior`."""97 """See `IBuildFarmJobBehavior`."""
98 builder_status = raw_slave_status[0]98 if status['builder_status'] == 'BuilderStatus.WAITING':
9999 if len(raw_slave_status) >= 4:
100 if builder_status == 'BuilderStatus.WAITING':100 status['filemap'] = raw_slave_status[3]
101 extra_info = {
102 'build_status': raw_slave_status[1],
103 'build_id': raw_slave_status[2],
104 }
105 if len(raw_slave_status) >= 3:
106 extra_info['filemap'] = raw_slave_status[3]
107 return extra_info
108 else:
109 # Nothing special to do for other states.
110 return {}
111101
112 def updateBuild_WAITING(self, queue_item, slave_status, logtail, logger):102 def updateBuild_WAITING(self, queue_item, slave_status, logtail, logger):
113 """Deal with a finished ("WAITING" state, perversely) build job.103 """Deal with a finished ("WAITING" state, perversely) build job.
114104
=== modified file 'lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py'
--- lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py 2010-03-25 23:19:10 +0000
+++ lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py 2010-03-31 04:20:47 +0000
@@ -185,7 +185,12 @@
185 self.assertEqual(0, builder.cleanSlave.call_count)185 self.assertEqual(0, builder.cleanSlave.call_count)
186 self.assertEqual(0, behavior._uploadTarball.call_count)186 self.assertEqual(0, behavior._uploadTarball.call_count)
187187
188 slave_status = behavior.slaveStatus(builder.slave.status())188 slave_status = {
189 'builder_status': builder.slave.status()[0],
190 'build_status': builder.slave.status()[1],
191 'build_id': builder.slave.status()[2]
192 }
193 behavior.updateSlaveStatus(builder.slave.status(), slave_status)
189 behavior.updateBuild_WAITING(queue_item, slave_status, None, logging)194 behavior.updateBuild_WAITING(queue_item, slave_status, None, logging)
190195
191 self.assertEqual(1, queue_item.destroySelf.call_count)196 self.assertEqual(1, queue_item.destroySelf.call_count)