Merge lp:~julian-edwards/maas/report-subarches-in-bootimage into lp:~maas-committers/maas/trunk
- report-subarches-in-bootimage
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Julian Edwards | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 2340 | ||||
Proposed branch: | lp:~julian-edwards/maas/report-subarches-in-bootimage | ||||
Merge into: | lp:~maas-committers/maas/trunk | ||||
Diff against target: |
424 lines (+275/-9) 5 files modified
src/provisioningserver/boot/tests/test_tftppath.py (+154/-1) src/provisioningserver/boot/tftppath.py (+71/-8) src/provisioningserver/import_images/boot_image_mapping.py (+27/-0) src/provisioningserver/import_images/testing/factory.py (+9/-0) src/provisioningserver/import_images/tests/test_boot_image_mapping.py (+14/-0) |
||||
To merge this branch: | bzr merge lp:~julian-edwards/maas/report-subarches-in-bootimage | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jeroen T. Vermeulen (community) | Approve | ||
Review via email: mp+219987@code.launchpad.net |
Commit message
Make list_boot_images(), which is called from the periodic Celery task, report the supported_subarches which exists in the maas.meta file as subarches in the resource data for each image. This will enable subarch fallback to the right image when looking for a boot image.
Description of the change
I am throwing this up before I've tested it on my rig so I can get some early feedback. There's probably a fair bit that needs tidying up but given I don't overlap with you lot at the moment, please forgive my indulgence :)
Julian Edwards (julian-edwards) wrote : | # |
On 20/05/14 02:49, Jeroen T. Vermeulen wrote:
> Review: Approve
>
> Comments inline. Several things need, or may need, addressing before this can land, including the docstrings.
Thanks for reviewing.
I would have liked to have split this into smaller branches for better
focused changed, but a) it evolved over time, and b) I'd be blocked on
reviews that I won't get until my night-time. So, sorry for the
larger-
> Don't forget to run "make format" when you're done
Done!
>> +
>> + def test_extract_
>> + resource = dict(
>> + subarches=
>> + other_item=
>
> Shouldn't that be a list of subarches, i.e. [factory.
No, there is only one "subarches" per resource, it's just a string.
>> + def _make_path(self):
>> + osystem = factory.
>
> The underscore doesn't do much for us in a test case... If I were you I'd just leave it out as unnecessary reading.
I honestly disagree here. I don't know if this is an unwritten rule or
not, but things starting with underscore tell me that they are exclusive
to this class (or at least *should be*). Without it, I would wonder if
make_path came from the test case base class, especially given its
factory-like nature.
>> + params = extract_
>> +
>> + self.assertEqual(
>> + [
>
> Is ordering explicitly guaranteed? If not, best just use assertItemsEqual.
Ah, good spot!
>> -def extract_
>> +def extract_
>> + """Examine the maas.meta file for any required metadata.
>> +
>> + :param metadata: contents of the maas.meta file
>> + :param params: A dict of path components for the image
>> + (see extract_
>
> I am confused. This talks about a dict of path components; the documentation for extract_
I was referring to the return value of extract_
confused you I've removed the reference entirely and directly explained
in the docstring what is required.
>> +def extract_
>> """Represent a list of TFTP path elements as a list of boot-image dicts.
>>
>> - The path must consist of a full [osystem, architecture, subarchitecture,
>> - release] that identify a kind of boot that we may need an image for.
>> + :param path: Must consist of a full [osystem, architecture,
>> + subarchitecture, release] that identify a kind of boot for which we
>
> No need to say "Must consist of" any more.
I'm not sure what you are driving at, but I've re-worded it anyway.
>
>> + may need an image.
>> + :param maas_meta: Contents of the maas.meta file. This may be an
>> + empty string.
>> +
>> + :return: A dict, which may also include additional items of meta-data
>> + that are not elements in the path, such...
Julian Edwards (julian-edwards) wrote : | # |
This tested out on my rig, so landing it. There is one more problem to fix in a subsequent branch: the pxeconfig has the path to the node's subarch instead of the image's.
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~julian-edwards/maas/report-subarches-in-bootimage into lp:maas failed. Below is the output from the failed tests.
Ign http://
Get:1 http://
Get:2 http://
Ign http://
Ign http://
Hit http://
Get:3 http://
Hit http://
Get:4 http://
Get:5 http://
Hit http://
Get:6 http://
Hit http://
Get:7 http://
Hit http://
Hit http://
Hit http://
Get:8 http://
Hit http://
Hit http://
Hit http://
Ign http://
Ign http://
Ign http://
Ign http://
Get:9 http://
Get:10 http://
Get:11 http://
Get:12 http://
Hit http://
Hit http://
Ign http://
Ign http://
Fetched 410 kB in 0s (1,347 kB/s)
Reading package lists...
sudo DEBIAN_
--
Jeroen T. Vermeulen (jtv) wrote : | # |
Yes, I would feel better with the line breaks and a mention of NOQA, thanks.
Julian Edwards (julian-edwards) wrote : | # |
On 20/05/14 18:09, Jeroen T. Vermeulen wrote:
> Yes, I would feel better with the line breaks and a mention of NOQA, thanks.
>
Ok I'll do that in a followup branch.
Preview Diff
1 | === modified file 'src/provisioningserver/boot/tests/test_tftppath.py' |
2 | --- src/provisioningserver/boot/tests/test_tftppath.py 2014-05-02 15:13:14 +0000 |
3 | +++ src/provisioningserver/boot/tests/test_tftppath.py 2014-05-20 00:47:19 +0000 |
4 | @@ -25,6 +25,8 @@ |
5 | compose_image_path, |
6 | drill_down, |
7 | extend_path, |
8 | + extract_image_params, |
9 | + extract_metadata, |
10 | is_visible_subdir, |
11 | list_boot_images, |
12 | list_subdirs, |
13 | @@ -34,6 +36,14 @@ |
14 | OperatingSystem, |
15 | OperatingSystemRegistry, |
16 | ) |
17 | +from provisioningserver.import_images.boot_image_mapping import ( |
18 | + BootImageMapping, |
19 | + ) |
20 | +from provisioningserver.import_images.helpers import ImageSpec |
21 | +from provisioningserver.import_images.testing.factory import ( |
22 | + make_image_spec, |
23 | + set_resource, |
24 | + ) |
25 | from provisioningserver.testing.boot_images import ( |
26 | make_boot_image_storage_params, |
27 | ) |
28 | @@ -79,13 +89,15 @@ |
29 | ] |
30 | |
31 | |
32 | -def make_image(params, purpose): |
33 | +def make_image(params, purpose, metadata=None): |
34 | """Describe an image as a dict similar to what `list_boot_images` returns. |
35 | |
36 | The `params` are as returned from `make_boot_image_storage_params`. |
37 | """ |
38 | image = params.copy() |
39 | image['purpose'] = purpose |
40 | + if metadata is not None: |
41 | + image.update(metadata) |
42 | return image |
43 | |
44 | |
45 | @@ -133,6 +145,17 @@ |
46 | factory.make_file(image_dir, 'linux') |
47 | factory.make_file(image_dir, 'initrd.gz') |
48 | |
49 | + def make_meta_file(self, image_params, image_resource, tftproot): |
50 | + image = ImageSpec( |
51 | + arch=image_params["architecture"], |
52 | + subarch=image_params["subarchitecture"], |
53 | + release=image_params["release"], label=image_params["label"]) |
54 | + mapping = BootImageMapping() |
55 | + mapping.setdefault(image, image_resource) |
56 | + maas_meta = mapping.dump_json() |
57 | + with open(os.path.join(tftproot, "maas.meta"), "wb") as f: |
58 | + f.write(maas_meta) |
59 | + |
60 | def test_compose_image_path_follows_storage_directory_layout(self): |
61 | osystem = factory.make_name('osystem') |
62 | arch = factory.make_name('arch') |
63 | @@ -203,6 +226,21 @@ |
64 | ], |
65 | list_boot_images(self.tftproot)) |
66 | |
67 | + def test_list_boot_images_merges_maas_meta_data(self): |
68 | + params = make_boot_image_storage_params() |
69 | + self.make_image_dir(params, self.tftproot) |
70 | + # The required metadata is called "subarches" in maas.meta |
71 | + metadata = dict(subarches=factory.make_name("subarches")) |
72 | + self.make_meta_file(params, metadata, self.tftproot) |
73 | + purposes = ['install', 'commissioning', 'xinstall'] |
74 | + make_osystem(self, params['osystem'], purposes) |
75 | + # The API requires "supported_subarches". |
76 | + expected_metadata = dict(supported_subarches=metadata["subarches"]) |
77 | + self.assertItemsEqual( |
78 | + [make_image(params, purpose, expected_metadata) |
79 | + for purpose in purposes], |
80 | + list_boot_images(self.tftproot)) |
81 | + |
82 | def test_list_boot_images_empty_on_missing_osystems(self): |
83 | params = [make_boot_image_storage_params() for counter in range(3)] |
84 | for param in params: |
85 | @@ -296,3 +334,118 @@ |
86 | self.assertEqual( |
87 | [[deep_dir, subdir]], |
88 | drill_down(base_dir, [[shallow_dir], [deep_dir]])) |
89 | + |
90 | + def test_extract_metadata(self): |
91 | + resource = dict( |
92 | + subarches=factory.make_name("subarch"), |
93 | + other_item=factory.make_name("other"), |
94 | + ) |
95 | + image = make_image_spec() |
96 | + mapping = set_resource(image_spec=image, resource=resource) |
97 | + metadata = mapping.dump_json() |
98 | + |
99 | + # Lack of consistency across maas in naming arch vs architecture |
100 | + # and subarch vs subarchitecture means I can't just do a simple |
101 | + # dict parameter expansion here. |
102 | + params = { |
103 | + "architecture": image.arch, |
104 | + "subarchitecture": image.subarch, |
105 | + "release": image.release, |
106 | + "label": image.label, |
107 | + } |
108 | + extracted_data = extract_metadata(metadata, params) |
109 | + |
110 | + # We only expect the supported_subarches key from the resource data. |
111 | + expected = dict(supported_subarches=resource["subarches"]) |
112 | + self.assertEqual(expected, extracted_data) |
113 | + |
114 | + def _make_path(self): |
115 | + osystem = factory.make_name("os") |
116 | + arch = factory.make_name("arch") |
117 | + subarch = factory.make_name("subarch") |
118 | + release = factory.make_name("release") |
119 | + label = factory.make_name("label") |
120 | + path = (osystem, arch, subarch, release, label) |
121 | + return path, osystem, arch, subarch, release, label |
122 | + |
123 | + def _patch_osystem_registry(self, values): |
124 | + get_item = self.patch(OperatingSystemRegistry, "get_item") |
125 | + item_mock = Mock() |
126 | + item_mock.get_boot_image_purposes.return_value = values |
127 | + get_item.return_value = item_mock |
128 | + |
129 | + def test_extract_image_params_with_no_metadata(self): |
130 | + path, osystem, arch, subarch, release, label = self._make_path() |
131 | + |
132 | + # Patch OperatingSystemRegistry to return a fixed list of |
133 | + # values. |
134 | + purpose1 = factory.make_name("purpose") |
135 | + purpose2 = factory.make_name("purpose") |
136 | + purposes = [purpose1, purpose2] |
137 | + self._patch_osystem_registry(purposes) |
138 | + |
139 | + params = extract_image_params(path, "") |
140 | + |
141 | + self.assertItemsEqual( |
142 | + [ |
143 | + { |
144 | + "osystem": osystem, |
145 | + "architecture": arch, |
146 | + "subarchitecture": subarch, |
147 | + "release": release, |
148 | + "label": label, |
149 | + "purpose": purpose1, |
150 | + }, |
151 | + { |
152 | + "osystem": osystem, |
153 | + "architecture": arch, |
154 | + "subarchitecture": subarch, |
155 | + "release": release, |
156 | + "label": label, |
157 | + "purpose": purpose2, |
158 | + }, |
159 | + ], |
160 | + params) |
161 | + |
162 | + def test_extract_image_params_with_metadata(self): |
163 | + path, osystem, arch, subarch, release, label = self._make_path() |
164 | + |
165 | + # Patch OperatingSystemRegistry to return a fixed list of |
166 | + # values. |
167 | + purpose1 = factory.make_name("purpose") |
168 | + purpose2 = factory.make_name("purpose") |
169 | + purposes = [purpose1, purpose2] |
170 | + self._patch_osystem_registry(purposes) |
171 | + |
172 | + # Create some maas.meta content. |
173 | + image = ImageSpec( |
174 | + arch=arch, subarch=subarch, release=release, label=label) |
175 | + image_resource = dict(subarches=factory.make_name("subarches")) |
176 | + mapping = BootImageMapping() |
177 | + mapping.setdefault(image, image_resource) |
178 | + maas_meta = mapping.dump_json() |
179 | + |
180 | + params = extract_image_params(path, maas_meta) |
181 | + |
182 | + self.assertItemsEqual( |
183 | + [ |
184 | + { |
185 | + "osystem": osystem, |
186 | + "architecture": arch, |
187 | + "subarchitecture": subarch, |
188 | + "release": release, |
189 | + "label": label, |
190 | + "purpose": purpose1, |
191 | + "supported_subarches": image_resource["subarches"], |
192 | + }, |
193 | + { |
194 | + "osystem": osystem, |
195 | + "architecture": arch, |
196 | + "subarchitecture": subarch, |
197 | + "release": release, |
198 | + "label": label, |
199 | + "purpose": purpose2, |
200 | + "supported_subarches": image_resource["subarches"], |
201 | + }, |
202 | + ], |
203 | + params) |
204 | |
205 | === modified file 'src/provisioningserver/boot/tftppath.py' |
206 | --- src/provisioningserver/boot/tftppath.py 2014-05-19 13:16:53 +0000 |
207 | +++ src/provisioningserver/boot/tftppath.py 2014-05-20 00:47:19 +0000 |
208 | @@ -25,6 +25,10 @@ |
209 | import os.path |
210 | |
211 | from provisioningserver.driver import OperatingSystemRegistry |
212 | +from provisioningserver.import_images.boot_image_mapping import ( |
213 | + BootImageMapping, |
214 | + ) |
215 | +from provisioningserver.import_images.helpers import ImageSpec |
216 | |
217 | |
218 | logger = getLogger(__name__) |
219 | @@ -112,11 +116,43 @@ |
220 | extend_path(directory, path) for path in paths)) |
221 | |
222 | |
223 | -def extract_image_params(path): |
224 | +def extract_metadata(metadata, params): |
225 | + """Examine the maas.meta file for any required metadata. |
226 | + |
227 | + :param metadata: contents of the maas.meta file |
228 | + :param params: A dict of path components for the image |
229 | + (architecture, subarchitecture, release and label). |
230 | + :return: a dict of name/value metadata pairs. Currently, only |
231 | + "subarches" is extracted. |
232 | + """ |
233 | + mapping = BootImageMapping.load_json(metadata) |
234 | + |
235 | + image = ImageSpec( |
236 | + arch=params["architecture"], |
237 | + subarch=params["subarchitecture"], |
238 | + release=params["release"], |
239 | + label=params["label"], |
240 | + ) |
241 | + try: |
242 | + resource = mapping.mapping[image] |
243 | + except KeyError: |
244 | + return {} |
245 | + |
246 | + return dict(supported_subarches=resource["subarches"]) |
247 | + |
248 | + |
249 | +def extract_image_params(path, maas_meta): |
250 | """Represent a list of TFTP path elements as a list of boot-image dicts. |
251 | |
252 | - The path must consist of a full [osystem, architecture, subarchitecture, |
253 | - release] that identify a kind of boot that we may need an image for. |
254 | + :param path: Tuple or list that consists of a full [osystem, architecture, |
255 | + subarchitecture, release] that identify a kind of boot for which we |
256 | + may need an image. |
257 | + :param maas_meta: Contents of the maas.meta file. This may be an |
258 | + empty string. |
259 | + |
260 | + :return: A list of dicts, each of which may also include additional |
261 | + items of meta-data that are not elements in the path, such as |
262 | + "subarches". |
263 | """ |
264 | osystem, arch, subarch, release, label = path |
265 | osystem_obj = OperatingSystemRegistry.get_item(osystem, default=None) |
266 | @@ -125,13 +161,27 @@ |
267 | |
268 | purposes = osystem_obj.get_boot_image_purposes( |
269 | arch, subarch, release, label) |
270 | - return [ |
271 | + |
272 | + # Expand the path into a list of dicts, one for each boot purpose. |
273 | + params = [ |
274 | dict( |
275 | osystem=osystem, architecture=arch, subarchitecture=subarch, |
276 | release=release, label=label, purpose=purpose) |
277 | for purpose in purposes |
278 | ] |
279 | |
280 | + # Merge in the meta-data. |
281 | + for image_dict in params: |
282 | + metadata = extract_metadata(maas_meta, image_dict) |
283 | + image_dict.update(metadata) |
284 | + |
285 | + return params |
286 | + |
287 | + |
288 | +def maas_meta_file_path(tftproot): |
289 | + """Return a string containing the full path to maas.meta.""" |
290 | + return os.path.join(tftproot, 'maas.meta') |
291 | + |
292 | |
293 | def list_boot_images(tftproot): |
294 | """List the available boot images. |
295 | @@ -149,9 +199,9 @@ |
296 | # Directory does not exist, so return empty list. |
297 | logger.warning("No boot images have been imported yet.") |
298 | return [] |
299 | - else: |
300 | - # Other error. Propagate. |
301 | - raise |
302 | + |
303 | + # Other error. Propagate. |
304 | + raise |
305 | |
306 | # Starting point for iteration: paths that contain only the |
307 | # top-level subdirectory of tftproot, i.e. the architecture name. |
308 | @@ -163,8 +213,21 @@ |
309 | for level in ['arch', 'subarch', 'release', 'label']: |
310 | paths = drill_down(tftproot, paths) |
311 | |
312 | + # Get hold of image meta-data stored in the maas.meta file. |
313 | + meta_file_path = maas_meta_file_path(tftproot) |
314 | + try: |
315 | + with open(meta_file_path, "rb") as f: |
316 | + metadata = f.read() |
317 | + except IOError as e: |
318 | + if e.errno != errno.ENOENT: |
319 | + # Unexpected error, propagate. |
320 | + raise |
321 | + # No meta file (yet), it means no import has run so just skip |
322 | + # it. |
323 | + metadata = "" |
324 | + |
325 | # Each path we find this way should be a boot image. |
326 | # This gets serialised to JSON, so we really have to return a list, not |
327 | # just any iterable. |
328 | return list(chain.from_iterable( |
329 | - extract_image_params(path) for path in paths)) |
330 | + extract_image_params(path, metadata) for path in paths)) |
331 | |
332 | === modified file 'src/provisioningserver/import_images/boot_image_mapping.py' |
333 | --- src/provisioningserver/import_images/boot_image_mapping.py 2014-04-11 02:14:42 +0000 |
334 | +++ src/provisioningserver/import_images/boot_image_mapping.py 2014-05-20 00:47:19 +0000 |
335 | @@ -63,3 +63,30 @@ |
336 | data[arch][subarch].setdefault(release, {}) |
337 | data[arch][subarch][release][label] = resource |
338 | return json.dumps(data, sort_keys=True) |
339 | + |
340 | + @staticmethod |
341 | + def load_json(json_data): |
342 | + """Take a JSON representation and deserialize into an object. |
343 | + |
344 | + :param json_data: string produced by dump_json(), above. |
345 | + :return: A BootImageMapping |
346 | + |
347 | + If the json data is invalid, an empty BootImageMapping is returned. |
348 | + """ |
349 | + mapping = BootImageMapping() |
350 | + try: |
351 | + data = json.loads(json_data) |
352 | + except ValueError: |
353 | + return mapping |
354 | + |
355 | + for arch in data: |
356 | + for subarch in data[arch]: |
357 | + for release in data[arch][subarch]: |
358 | + for label in data[arch][subarch][release]: |
359 | + image = ImageSpec( |
360 | + arch=arch, subarch=subarch, release=release, |
361 | + label=label) |
362 | + resource = data[arch][subarch][release][label] |
363 | + mapping.setdefault(image, resource) |
364 | + |
365 | + return mapping |
366 | |
367 | === modified file 'src/provisioningserver/import_images/testing/factory.py' |
368 | --- src/provisioningserver/import_images/testing/factory.py 2014-04-08 06:53:34 +0000 |
369 | +++ src/provisioningserver/import_images/testing/factory.py 2014-05-20 00:47:19 +0000 |
370 | @@ -15,9 +15,12 @@ |
371 | __all__ = [ |
372 | 'make_boot_resource', |
373 | 'make_image_spec', |
374 | + 'make_maas_meta', |
375 | 'set_resource', |
376 | ] |
377 | |
378 | +from textwrap import dedent |
379 | + |
380 | from maastesting.factory import factory |
381 | from provisioningserver.import_images.boot_image_mapping import ( |
382 | BootImageMapping, |
383 | @@ -25,6 +28,12 @@ |
384 | from provisioningserver.import_images.helpers import ImageSpec |
385 | |
386 | |
387 | +def make_maas_meta(): |
388 | + """Return fake maas.meta data.""" |
389 | + return dedent("""\ |
390 | + {"amd64": {"generic": {"precise": {"release": {"content_id": "com.ubuntu.maas:v2:download", "path": "precise/amd64/20140410/raring/generic/boot-kernel", "product_name": "com.ubuntu.maas:v2:boot:12.04:amd64:hwe-r", "subarches": "generic,hwe-p,hwe-q,hwe-r", "version_name": "20140410"}}, "trusty": {"release": {"content_id": "com.ubuntu.maas:v2:download", "path": "trusty/amd64/20140416.1/root-image.gz", "product_name": "com.ubuntu.maas:v2:boot:14.04:amd64:hwe-t", "subarches": "generic,hwe-p,hwe-q,hwe-r,hwe-s,hwe-t", "version_name": "20140416.1"}}}, "hwe-s": {"precise": {"release": {"content_id": "com.ubuntu.maas:v2:download", "path": "precise/amd64/20140410/saucy/generic/boot-kernel", "product_name": "com.ubuntu.maas:v2:boot:12.04:amd64:hwe-s", "subarches": "generic,hwe-p,hwe-q,hwe-r,hwe-s", "version_name": "20140410"}}}}}""") # NOQA |
391 | + |
392 | + |
393 | def make_boot_resource(): |
394 | """Create a fake resource dict.""" |
395 | return { |
396 | |
397 | === modified file 'src/provisioningserver/import_images/tests/test_boot_image_mapping.py' |
398 | --- src/provisioningserver/import_images/tests/test_boot_image_mapping.py 2014-04-11 02:13:59 +0000 |
399 | +++ src/provisioningserver/import_images/tests/test_boot_image_mapping.py 2014-05-20 00:47:19 +0000 |
400 | @@ -23,6 +23,7 @@ |
401 | ) |
402 | from provisioningserver.import_images.testing.factory import ( |
403 | make_image_spec, |
404 | + make_maas_meta, |
405 | set_resource, |
406 | ) |
407 | |
408 | @@ -104,3 +105,16 @@ |
409 | }, |
410 | }, |
411 | json.loads(image_dict.dump_json())) |
412 | + |
413 | + def test_load_json_result_matches_dump_of_own_data(self): |
414 | + # Loading the test data and dumping it again should result in |
415 | + # identical test data. |
416 | + test_meta_file_content = make_maas_meta() |
417 | + mapping = BootImageMapping.load_json(test_meta_file_content) |
418 | + dumped = mapping.dump_json() |
419 | + self.assertEqual(test_meta_file_content, dumped) |
420 | + |
421 | + def test_load_json_returns_empty_mapping_for_invalid_json(self): |
422 | + bad_json = "" |
423 | + mapping = BootImageMapping.load_json(bad_json) |
424 | + self.assertEqual({}, mapping.mapping) |
Comments inline. Several things need, or may need, addressing before this can land, including the docstrings.