Merge lp:~julian-edwards/maas/report-subarches-in-bootimage into lp:~maas-committers/maas/trunk

[Julian Edwards]

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.

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 :)

[Jeroen T. Vermeulen (jtv)]

Comments inline. Several things need, or may need, addressing before this can land, including the docstrings.

review: Approve

[Julian Edwards (julian-edwards)]

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-than-I-wanted branch. Move back to BKK and they get smaller :)

> Don't forget to run "make format" when you're done

Done!

>> +
>> + def test_extract_metadata(self):
>> + resource = dict(
>> + subarches=factory.make_name("subarch"),
>> + other_item=factory.make_name("other"),
>
> Shouldn't that be a list of subarches, i.e. [factory.make_name("subarch")] (with the added brackets)?

No, there is only one "subarches" per resource, it's just a string.

>> + def _make_path(self):
>> + osystem = factory.make_name("os")
>
> 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_image_params(path, "")
>> +
>> + self.assertEqual(
>> + [
>
> Is ordering explicitly guaranteed? If not, best just use assertItemsEqual.

Ah, good spot!

>> -def extract_image_params(path):
>> +def extract_metadata(metadata, params):
>> + """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_image_params).
>
> I am confused. This talks about a dict of path components; the documentation for extract_image_params talks about a list of boot-image dicts; and the :return: documentation for extract_image_params talks about a dict of metadata. How do these things fit together?

I was referring to the return value of extract_image_params, but as it
confused you I've removed the reference entirely and directly explained
in the docstring what is required.

>> +def extract_image_params(path, maas_meta):
>> """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)]

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)]

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://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Get:2 http://security.ubuntu.com trusty-security Release [58.5 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release [58.5 kB]
Get:5 http://security.ubuntu.com trusty-security/main Sources [14.7 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Get:6 http://security.ubuntu.com trusty-security/universe Sources [4,212 B]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [47.0 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [17.7 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [35.9 kB]
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [23.8 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [83.4 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [64.3 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 410 kB in 0s (1,347 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-formencode python-httplib2 py...

[Jeroen T. Vermeulen (jtv)]

Yes, I would feel better with the line breaks and a mention of NOQA, thanks.

[Julian Edwards (julian-edwards)]

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.

=== modified file 'src/provisioningserver/boot/tests/test_tftppath.py'
--- src/provisioningserver/boot/tests/test_tftppath.py	2014-05-02 15:13:14 +0000
+++ src/provisioningserver/boot/tests/test_tftppath.py	2014-05-20 00:47:19 +0000
@@ -25,6 +25,8 @@
     compose_image_path,
     drill_down,
     extend_path,
+    extract_image_params,
+    extract_metadata,
     is_visible_subdir,
     list_boot_images,
     list_subdirs,
@@ -34,6 +36,14 @@
     OperatingSystem,
     OperatingSystemRegistry,
     )
+from provisioningserver.import_images.boot_image_mapping import (
+    BootImageMapping,
+    )
+from provisioningserver.import_images.helpers import ImageSpec
+from provisioningserver.import_images.testing.factory import (
+    make_image_spec,
+    set_resource,
+    )
 from provisioningserver.testing.boot_images import (
     make_boot_image_storage_params,
     )
@@ -79,13 +89,15 @@
             ]
 
 
-def make_image(params, purpose):
+def make_image(params, purpose, metadata=None):
     """Describe an image as a dict similar to what `list_boot_images` returns.
 
     The `params` are as returned from `make_boot_image_storage_params`.
     """
     image = params.copy()
     image['purpose'] = purpose
+    if metadata is not None:
+        image.update(metadata)
     return image
 
 
@@ -133,6 +145,17 @@
         factory.make_file(image_dir, 'linux')
         factory.make_file(image_dir, 'initrd.gz')
 
+    def make_meta_file(self, image_params, image_resource, tftproot):
+        image = ImageSpec(
+            arch=image_params["architecture"],
+            subarch=image_params["subarchitecture"],
+            release=image_params["release"], label=image_params["label"])
+        mapping = BootImageMapping()
+        mapping.setdefault(image, image_resource)
+        maas_meta = mapping.dump_json()
+        with open(os.path.join(tftproot, "maas.meta"), "wb") as f:
+            f.write(maas_meta)
+
     def test_compose_image_path_follows_storage_directory_layout(self):
         osystem = factory.make_name('osystem')
         arch = factory.make_name('arch')
@@ -203,6 +226,21 @@
             ],
             list_boot_images(self.tftproot))
 
+    def test_list_boot_images_merges_maas_meta_data(self):
+        params = make_boot_image_storage_params()
+        self.make_image_dir(params, self.tftproot)
+        # The required metadata is called "subarches" in maas.meta
+        metadata = dict(subarches=factory.make_name("subarches"))
+        self.make_meta_file(params, metadata, self.tftproot)
+        purposes = ['install', 'commissioning', 'xinstall']
+        make_osystem(self, params['osystem'], purposes)
+        # The API requires "supported_subarches".
+        expected_metadata = dict(supported_subarches=metadata["subarches"])
+        self.assertItemsEqual(
+            [make_image(params, purpose, expected_metadata)
+                for purpose in purposes],
+            list_boot_images(self.tftproot))
+
     def test_list_boot_images_empty_on_missing_osystems(self):
         params = [make_boot_image_storage_params() for counter in range(3)]
         for param in params:
@@ -296,3 +334,118 @@
         self.assertEqual(
             [[deep_dir, subdir]],
             drill_down(base_dir, [[shallow_dir], [deep_dir]]))
+
+    def test_extract_metadata(self):
+        resource = dict(
+            subarches=factory.make_name("subarch"),
+            other_item=factory.make_name("other"),
+            )
+        image = make_image_spec()
+        mapping = set_resource(image_spec=image, resource=resource)
+        metadata = mapping.dump_json()
+
+        # Lack of consistency across maas in naming arch vs architecture
+        # and subarch vs subarchitecture means I can't just do a simple
+        # dict parameter expansion here.
+        params = {
+            "architecture": image.arch,
+            "subarchitecture": image.subarch,
+            "release": image.release,
+            "label": image.label,
+            }
+        extracted_data = extract_metadata(metadata, params)
+
+        # We only expect the supported_subarches key from the resource data.
+        expected = dict(supported_subarches=resource["subarches"])
+        self.assertEqual(expected, extracted_data)
+
+    def _make_path(self):
+        osystem = factory.make_name("os")
+        arch = factory.make_name("arch")
+        subarch = factory.make_name("subarch")
+        release = factory.make_name("release")
+        label = factory.make_name("label")
+        path = (osystem, arch, subarch, release, label)
+        return path, osystem, arch, subarch, release, label
+
+    def _patch_osystem_registry(self, values):
+        get_item = self.patch(OperatingSystemRegistry, "get_item")
+        item_mock = Mock()
+        item_mock.get_boot_image_purposes.return_value = values
+        get_item.return_value = item_mock
+
+    def test_extract_image_params_with_no_metadata(self):
+        path, osystem, arch, subarch, release, label = self._make_path()
+
+        # Patch OperatingSystemRegistry to return a fixed list of
+        # values.
+        purpose1 = factory.make_name("purpose")
+        purpose2 = factory.make_name("purpose")
+        purposes = [purpose1, purpose2]
+        self._patch_osystem_registry(purposes)
+
+        params = extract_image_params(path, "")
+
+        self.assertItemsEqual(
+            [
+                {
+                    "osystem": osystem,
+                    "architecture": arch,
+                    "subarchitecture": subarch,
+                    "release": release,
+                    "label": label,
+                    "purpose": purpose1,
+                },
+                {
+                    "osystem": osystem,
+                    "architecture": arch,
+                    "subarchitecture": subarch,
+                    "release": release,
+                    "label": label,
+                    "purpose": purpose2,
+                },
+            ],
+            params)
+
+    def test_extract_image_params_with_metadata(self):
+        path, osystem, arch, subarch, release, label = self._make_path()
+
+        # Patch OperatingSystemRegistry to return a fixed list of
+        # values.
+        purpose1 = factory.make_name("purpose")
+        purpose2 = factory.make_name("purpose")
+        purposes = [purpose1, purpose2]
+        self._patch_osystem_registry(purposes)
+
+        # Create some maas.meta content.
+        image = ImageSpec(
+            arch=arch, subarch=subarch, release=release, label=label)
+        image_resource = dict(subarches=factory.make_name("subarches"))
+        mapping = BootImageMapping()
+        mapping.setdefault(image, image_resource)
+        maas_meta = mapping.dump_json()
+
+        params = extract_image_params(path, maas_meta)
+
+        self.assertItemsEqual(
+            [
+                {
+                    "osystem": osystem,
+                    "architecture": arch,
+                    "subarchitecture": subarch,
+                    "release": release,
+                    "label": label,
+                    "purpose": purpose1,
+                    "supported_subarches": image_resource["subarches"],
+                },
+                {
+                    "osystem": osystem,
+                    "architecture": arch,
+                    "subarchitecture": subarch,
+                    "release": release,
+                    "label": label,
+                    "purpose": purpose2,
+                    "supported_subarches": image_resource["subarches"],
+                },
+            ],
+            params)
 
=== modified file 'src/provisioningserver/boot/tftppath.py'
--- src/provisioningserver/boot/tftppath.py	2014-05-19 13:16:53 +0000
+++ src/provisioningserver/boot/tftppath.py	2014-05-20 00:47:19 +0000
@@ -25,6 +25,10 @@
 import os.path
 
 from provisioningserver.driver import OperatingSystemRegistry
+from provisioningserver.import_images.boot_image_mapping import (
+    BootImageMapping,
+    )
+from provisioningserver.import_images.helpers import ImageSpec
 
 
 logger = getLogger(__name__)
@@ -112,11 +116,43 @@
         extend_path(directory, path) for path in paths))
 
 
-def extract_image_params(path):
+def extract_metadata(metadata, params):
+    """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
+        (architecture, subarchitecture, release and label).
+    :return: a dict of name/value metadata pairs.  Currently, only
+        "subarches" is extracted.
+    """
+    mapping = BootImageMapping.load_json(metadata)
+
+    image = ImageSpec(
+        arch=params["architecture"],
+        subarch=params["subarchitecture"],
+        release=params["release"],
+        label=params["label"],
+        )
+    try:
+        resource = mapping.mapping[image]
+    except KeyError:
+        return {}
+
+    return dict(supported_subarches=resource["subarches"])
+
+
+def extract_image_params(path, maas_meta):
     """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: Tuple or list that consists of a full [osystem, architecture,
+        subarchitecture, release] that identify a kind of boot for which we
+        may need an image.
+    :param maas_meta: Contents of the maas.meta file.  This may be an
+        empty string.
+
+    :return: A list of dicts, each of which may also include additional
+        items of meta-data that are not elements in the path, such as
+        "subarches".
     """
     osystem, arch, subarch, release, label = path
     osystem_obj = OperatingSystemRegistry.get_item(osystem, default=None)
@@ -125,13 +161,27 @@
 
     purposes = osystem_obj.get_boot_image_purposes(
         arch, subarch, release, label)
-    return [
+
+    # Expand the path into a list of dicts, one for each boot purpose.
+    params = [
         dict(
             osystem=osystem, architecture=arch, subarchitecture=subarch,
             release=release, label=label, purpose=purpose)
         for purpose in purposes
         ]
 
+    # Merge in the meta-data.
+    for image_dict in params:
+        metadata = extract_metadata(maas_meta, image_dict)
+        image_dict.update(metadata)
+
+    return params
+
+
+def maas_meta_file_path(tftproot):
+    """Return a string containing the full path to maas.meta."""
+    return os.path.join(tftproot, 'maas.meta')
+
 
 def list_boot_images(tftproot):
     """List the available boot images.
@@ -149,9 +199,9 @@
             # Directory does not exist, so return empty list.
             logger.warning("No boot images have been imported yet.")
             return []
-        else:
-            # Other error. Propagate.
-            raise
+
+        # Other error. Propagate.
+        raise
 
     # Starting point for iteration: paths that contain only the
     # top-level subdirectory of tftproot, i.e. the architecture name.
@@ -163,8 +213,21 @@
     for level in ['arch', 'subarch', 'release', 'label']:
         paths = drill_down(tftproot, paths)
 
+    # Get hold of image meta-data stored in the maas.meta file.
+    meta_file_path = maas_meta_file_path(tftproot)
+    try:
+        with open(meta_file_path, "rb") as f:
+            metadata = f.read()
+    except IOError as e:
+        if e.errno != errno.ENOENT:
+            # Unexpected error, propagate.
+            raise
+        # No meta file (yet), it means no import has run so just skip
+        # it.
+        metadata = ""
+
     # Each path we find this way should be a boot image.
     # This gets serialised to JSON, so we really have to return a list, not
     # just any iterable.
     return list(chain.from_iterable(
-        extract_image_params(path) for path in paths))
+        extract_image_params(path, metadata) for path in paths))
 
=== modified file 'src/provisioningserver/import_images/boot_image_mapping.py'
--- src/provisioningserver/import_images/boot_image_mapping.py	2014-04-11 02:14:42 +0000
+++ src/provisioningserver/import_images/boot_image_mapping.py	2014-05-20 00:47:19 +0000
@@ -63,3 +63,30 @@
             data[arch][subarch].setdefault(release, {})
             data[arch][subarch][release][label] = resource
         return json.dumps(data, sort_keys=True)
+
+    @staticmethod
+    def load_json(json_data):
+        """Take a JSON representation and deserialize into an object.
+
+        :param json_data: string produced by dump_json(), above.
+        :return: A BootImageMapping
+
+        If the json data is invalid, an empty BootImageMapping is returned.
+        """
+        mapping = BootImageMapping()
+        try:
+            data = json.loads(json_data)
+        except ValueError:
+            return mapping
+
+        for arch in data:
+            for subarch in data[arch]:
+                for release in data[arch][subarch]:
+                    for label in data[arch][subarch][release]:
+                        image = ImageSpec(
+                            arch=arch, subarch=subarch, release=release,
+                            label=label)
+                        resource = data[arch][subarch][release][label]
+                        mapping.setdefault(image, resource)
+
+        return mapping
 
=== modified file 'src/provisioningserver/import_images/testing/factory.py'
--- src/provisioningserver/import_images/testing/factory.py	2014-04-08 06:53:34 +0000
+++ src/provisioningserver/import_images/testing/factory.py	2014-05-20 00:47:19 +0000
@@ -15,9 +15,12 @@
 __all__ = [
     'make_boot_resource',
     'make_image_spec',
+    'make_maas_meta',
     'set_resource',
     ]
 
+from textwrap import dedent
+
 from maastesting.factory import factory
 from provisioningserver.import_images.boot_image_mapping import (
     BootImageMapping,
@@ -25,6 +28,12 @@
 from provisioningserver.import_images.helpers import ImageSpec
 
 
+def make_maas_meta():
+    """Return fake maas.meta data."""
+    return dedent("""\
+        {"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
+
+
 def make_boot_resource():
     """Create a fake resource dict."""
     return {
 
=== modified file 'src/provisioningserver/import_images/tests/test_boot_image_mapping.py'
--- src/provisioningserver/import_images/tests/test_boot_image_mapping.py	2014-04-11 02:13:59 +0000
+++ src/provisioningserver/import_images/tests/test_boot_image_mapping.py	2014-05-20 00:47:19 +0000
@@ -23,6 +23,7 @@
     )
 from provisioningserver.import_images.testing.factory import (
     make_image_spec,
+    make_maas_meta,
     set_resource,
     )
 
@@ -104,3 +105,16 @@
                 },
             },
             json.loads(image_dict.dump_json()))
+
+    def test_load_json_result_matches_dump_of_own_data(self):
+        # Loading the test data and dumping it again should result in
+        # identical test data.
+        test_meta_file_content = make_maas_meta()
+        mapping = BootImageMapping.load_json(test_meta_file_content)
+        dumped = mapping.dump_json()
+        self.assertEqual(test_meta_file_content, dumped)
+
+    def test_load_json_returns_empty_mapping_for_invalid_json(self):
+        bad_json = ""
+        mapping = BootImageMapping.load_json(bad_json)
+        self.assertEqual({}, mapping.mapping)