Merge lp:~jtv/maas/boot-image-dict into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Turn the “hierarchical dicts” in the new import code into a proper class, BootImageDict.

The import code is a bewildering constellation of dict-based data structures. In order to be able to test, maintain, and debug this code, we need a bit more structure. This came up in my ongoing attempt to write unit tests for the new code.

Much of the dict-ey code is about dicts that index by architecture, subarchitecture, release, and label. Another, very similar kind of dict uses the same constructor but contains something else. I'm prying the two apart, turning that first kind into a class with limited methods, so we have a clear idea of what does and doesn't happen to its contents. In the process I'm conquering the fear of hidden interactions with Simplestreams code.

One thing the encapsulation allowed me to do is to simplify the dict's structure. It now maps ImageSpec, a tuple type, to boot resource — another kind of dict that I'd like to turn into a class. And boot_reverse turns a BootImageDict inside out to return that other kind of dict that uses the same constructor. You guessed it: that too I would like to turn into a class.

The one complication was in dumping the dict to JSON: you can't have tuples as keys in a JSON object. Moreover, you can't just change the format of the maas.meta file, which is where the JSON goes, because we have no idea what its purpose is or where it is used. And so inside the new dump_json method, I do convert back to the old format. This turns out to be a lot easier than continually managing the full deeply-nested structure, as I did in an earlier stage in the evolution of this branch.

The testing for dump_json overlaps with test code that I previously added to the end-to-end test. I can probably take that back out again now, in favour of the new unit test. But I wanted to have a solid anchor for the refactoring, so that I didn't accidentally change the format.

Jeroen

[Graham Binns (gmb)]

This is great work; thanks Jeroen. A couple of quibbles:

[1]

19 +class BootImageDict:
20 + """Mapping of boot-image data.
21 +
22 + Maps `ImageSpec` tuples to metadata for Simplestreams products.
23 +
24 + This class is deliberately a bit more restrictive and less ad-hoc than a
25 + dict. It helps keep a clear view of the data structures in this module.

You've said that it's "a bit more restrictive and less ad-hoc than a
dict" but it's still called a BootImageDict and it implements items(),
which makes it look a bit like it *might* be a dict. But if you try to
use it like a dict, it'll go boom:

>>> from provisioningserver.import_images.boot_resources import BootImageDict
>>> image_dict = BootImageDict()
>>> image_dict['spam'] = 'eggs'
TypeError: 'BootImageDict' object does not support item assignment

If it's not going to behave like a dict we shouldn't call it a dict. If
it's meant to behave like a dict it should at least implement the
requisite methods, or raise a sensible error when those methods are
called on it (as shouldn't happen).

[2]

86 + """Complement `BootImageDict` with entries from another.

Missing "one" after "Complement".

review: Approve

[Jeroen T. Vermeulen (jtv)]

Renamed to BootImageMapping, and “one” added. Thanks for an insightful review.

[MAAS Lander (maas-lander)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'src/provisioningserver/import_images/boot_resources.py'
--- src/provisioningserver/import_images/boot_resources.py	2014-04-04 06:51:23 +0000
+++ src/provisioningserver/import_images/boot_resources.py	2014-04-07 15:13:16 +0000
@@ -92,23 +92,44 @@
     ])
 
 
-def iterate_boot_resources(boot_dict):
-    """Iterate a multi-level dict of boot images.
-
-    Yields each combination of architecture, subarchitecture, release, and
-    label for which `boot` has an entry, as an `ImageSpec`.
-
-    :param boot: Four-level dict of dicts representing boot images: the top
-        level maps the architectures to sub-dicts, each of which maps
-        subarchitectures to further dicts, each of which in turn maps
-        releases to yet more dicts, each of which maps release labels to any
-        kind of item it likes.
+class BootImageMapping:
+    """Mapping of boot-image data.
+
+    Maps `ImageSpec` tuples to metadata for Simplestreams products.
+
+    This class is deliberately a bit more restrictive and less ad-hoc than a
+    dict.  It helps keep a clear view of the data structures in this module.
     """
-    for arch, subarches in sorted(boot_dict.items()):
-        for subarch, releases in sorted(subarches.items()):
-            for release, labels in sorted(releases.items()):
-                for label in sorted(labels.keys()):
-                    yield ImageSpec(arch, subarch, release, label)
+
+    def __init__(self):
+        self.mapping = {}
+
+    def items(self):
+        """Iterate over `ImageSpec` keys, and their stored values."""
+        for image_spec, item in sorted(self.mapping.items()):
+            yield image_spec, item
+
+    def setdefault(self, image_spec, item):
+        """Set metadata for `image_spec` to item, if not already set."""
+        assert isinstance(image_spec, ImageSpec)
+        self.mapping.setdefault(image_spec, item)
+
+    def dump_json(self):
+        """Produce JSON representing the mapped boot images.
+
+        Tries to keep the output deterministic, so that identical data is
+        likely to produce identical JSON.
+        """
+        # The meta files represent the mapping as a nested hierarchy of dicts.
+        # Keep that format.
+        data = {}
+        for image, resource in self.items():
+            arch, subarch, release, label = image
+            data.setdefault(arch, {})
+            data[arch].setdefault(subarch, {})
+            data[arch][subarch].setdefault(release, {})
+            data[arch][subarch][release][label] = resource
+        return json.dumps(data, sort_keys=True)
 
 
 def value_passes_filter_list(filter_list, property_value):
@@ -156,67 +177,70 @@
     return False
 
 
-def boot_merge(boot1, boot2, filters=None):
-    """Add entries from the second multi-level dict to the first one.
-
-    Function copies d[arch][subarch][release]=value chains from the second
-    dictionary to the first one if they don't exist there and pass optional
-    check done by filters.
-
-    :param boot1: first dict which will be extended in-place.
-    :param boot2: second dict which will be used as a source of new entries.
-    :param filters: list of dicts each of which contains 'arch', 'subarch',
-        'release' keys; function takes d[arch][subarch][release] chain to the
-        first dict only if filters contain at least one dict with
-        arch in d['arches'], subarch in d['subarch'], d['release'] == release;
-        dict may have '*' as a value for 'arch' and 'release' keys and as a
-        member of 'subarch' list -- in that case key-specific check always
-        passes.
+def boot_merge(destination, additions, filters=None):
+    """Complement one `BootImageMapping` with entries from another.
+
+    This adds entries from `additions` (that match `filters`, if given) to
+    `destination`, but only for those image specs for which `destination` does
+    not have entries yet.
+
+    :param destination: `BootImageMapping` to be updated.  It will be extended
+        in-place.
+    :param additions: A second `BootImageMapping`, which will be used as a
+        source of additional entries.
+    :param filters: List of dicts, each of which contains 'arch', 'subarch',
+        and 'release' keys.  If given, entries are only considered for copying
+        from `additions` to `destination` if they match at least one of the
+        filters.  Entries in the filter may be the string `*` (or for entries
+        that are lists, may contain the string `*`) to make them match any
+        value.
     """
-    for arch, subarch, release, label in iterate_boot_resources(boot2):
+    for image, resource in additions.items():
+        arch, subarch, release, label = image
         if image_passes_filter(filters, arch, subarch, release, label):
             logger.debug(
                 "Merging boot resource for %s/%s/%s/%s.",
                 arch, subarch, release, label)
-            boot_resource = boot2[arch][subarch][release][label]
             # Do not override an existing entry with the same
             # arch/subarch/release/label: the first entry found takes
             # precedence.
-            if not boot1[arch][subarch][release][label]:
-                boot1[arch][subarch][release][label] = boot_resource
-
-
-def boot_reverse(boot):
-    """Determine a set of subarches which should be deployed by boot resource.
-
-    Function reverses h[arch][subarch][release]=boot_resource hierarchy to form
-    boot resource to subarch relation. Many subarches may be deployed by a
-    single boot resource (in which case boot_resource=[subarch1, subarch2]
-    relation will be created). We note only subarchitectures and ignore
-    architectures because boot resource is tightly coupled with architecture
-    it can deploy according to metadata format. We can figure out for which
-    architecture we need to use a specific boot resource by looking at its
-    description in metadata. We can't do the same with subarch because we may
-    want to use boot resource only for a specific subset of subarches it can be
-    used for. To represent boot resource to subarch relation we generate the
-    following multi-level dictionary: d[content_id][product_name]=[subarches]
-    where 'content_id' and 'product_name' values come from metadata information
-    and allow us to uniquely identify a specific boot resource.
-
-    :param boot: Hierarchy of dicts d[arch][subarch][release]=boot_resource
+            destination.setdefault(image, resource)
+
+
+def boot_reverse(boot_images_dict):
+    """Determine the subarches supported by each boot resource.
+
+    Many subarches may be deployed by a single boot resource.  We note only
+    subarchitectures here and ignore architectures because the metadata format
+    tightly couples a boot resource to its architecture.
+
+    We can figure out for which architecture we need to use a specific boot
+    resource by looking at its description in the metadata.  We can't do the
+    same with subarch, because we may want to use a boot resource only for a
+    specific subset of subarches.
+
+    This function represents the relationship between boot resources and
+    subarchitectures as a multi-level dictionary::
+
+        d[content_id][product_name] = [subarches]
+
+    Here, `content_id` and `product_name` come from the metadata information
+    and uniquely identify a specific boot resource.
+
+    :param boot: A `BootImageMapping` containing the images' metadata.
     :return Hierarchy of dictionaries d[content_id][product_name]=[subarches]
         which describes boot resource to subarches relation for all available
         boot resources (products).
     """
     reverse = create_empty_hierarchy()
 
-    for arch, subarch, release, label in iterate_boot_resources(boot):
-        boot_resource = boot[arch][subarch][release][label]
+    for image, boot_resource in boot_images_dict.items():
         content_id = boot_resource['content_id']
         product_name = boot_resource['product_name']
         version_name = boot_resource['version_name']
         existent = list(reverse[content_id][product_name][version_name])
-        reverse[content_id][product_name][version_name] = [subarch] + existent
+        reverse[content_id][product_name][version_name] = (
+            [image.subarch] + existent)
 
     return reverse
 
@@ -293,15 +317,15 @@
     """Gather metadata about boot images available in a Simplestreams repo.
 
     Used inside `download_image_descriptions`.  Stores basic metadata about
-    each image it finds upstream in a given dict, indexed by arch, subarch,
-    release, and label.  Each item is a dict containing the basic metadata
-    for retrieving a boot image.
-
-    :ivar boot_images_dict: A nested `defaultdict` for boot images.
-        Image metadata will be stored here as it is discovered.  Simplestreams
-        does not interact with this variable, and `BasicMirrorWriter` in itself
-        is stateless.  It relies on a subclass (such as this one) to store
-        data.
+    each image it finds upstream in a given `BootImageMapping`.  Each stored
+    item is a dict containing the basic metadata for retrieving a boot image.
+
+    Simplestreams' `BasicMirrorWriter` in itself is stateless.  It relies on
+    a subclass (such as this one) to store data.
+
+    :ivar boot_images_dict: A `BootImageMapping`.  Image metadata will be
+        stored here as it is discovered.  Simplestreams does not interact with
+        this variable.
     """
 
     def __init__(self, boot_images_dict):
@@ -321,11 +345,11 @@
         arch, subarches = item['arch'], item['subarches']
         release = item['release']
         label = item['label']
+        base_image = ImageSpec(arch, None, release, label)
         compact_item = clean_up_repo_item(item)
         for subarch in subarches.split(','):
-            if not self.boot_images_dict[arch][subarch][release][label]:
-                self.boot_images_dict[arch][subarch][release][label] = (
-                    compact_item)
+            self.boot_images_dict.setdefault(
+                base_image._replace(subarch=subarch), compact_item)
 
 
 def download_image_descriptions(path, keyring=None):
@@ -339,7 +363,7 @@
     mirror, rpath = path_from_mirror_url(path, None)
     policy = get_signing_policy(rpath, keyring)
     reader = UrlMirrorReader(mirror, policy=policy)
-    boot_images_dict = create_empty_hierarchy()
+    boot_images_dict = BootImageMapping()
     dumper = RepoDumper(boot_images_dict)
     dumper.sync(reader, rpath)
     return boot_images_dict
@@ -347,7 +371,7 @@
 
 def download_all_image_descriptions(config):
     """Download image metadata for all sources in `config`."""
-    boot = create_empty_hierarchy()
+    boot = BootImageMapping()
     for source in config['boot']['sources']:
         repo_boot = download_image_descriptions(
             source['path'], keyring=source['keyring'])
@@ -548,7 +572,7 @@
     storage = config['boot']['storage']
 
     boot = download_all_image_descriptions(config)
-    meta_file_content = json.dumps(boot, sort_keys=True)
+    meta_file_content = boot.dump_json()
     if meta_contains(storage, meta_file_content):
         # The current maas.meta already contains the new config.  No need to
         # rewrite anything.

=== modified file 'src/provisioningserver/import_images/tests/test_boot_resources.py'
--- src/provisioningserver/import_images/tests/test_boot_resources.py	2014-04-07 08:09:47 +0000
+++ src/provisioningserver/import_images/tests/test_boot_resources.py	2014-04-07 15:13:16 +0000
@@ -46,22 +46,75 @@
         )
 
 
-class TestIterateBootResources(MAASTestCase):
-    """Tests for `iterate_boot_resources`."""
-
-    def test_empty_hierarchy_yields_nothing(self):
-        self.assertItemsEqual(
-            [],
-            boot_resources.iterate_boot_resources(
-                boot_resources.create_empty_hierarchy()))
-
-    def test_finds_boot_resource(self):
-        image_spec = make_image_spec()
-        arch, subarch, release, label = image_spec
-        self.assertItemsEqual(
-            [image_spec],
-            boot_resources.iterate_boot_resources(
-                {arch: {subarch: {release: {label: factory.make_name()}}}}))
+class TestBootImageMapping(MAASTestCase):
+    """Tests for `BootImageMapping`."""
+
+    def test_initially_empty(self):
+        self.assertItemsEqual([], boot_resources.BootImageMapping().items())
+
+    def test_items_returns_items(self):
+        image = make_image_spec()
+        resource = factory.make_name('resource')
+        image_dict = set_resource(image_spec=image, resource=resource)
+        self.assertItemsEqual([(image, resource)], image_dict.items())
+
+    def test_setdefault_sets_unset_item(self):
+        image_dict = boot_resources.BootImageMapping()
+        image = make_image_spec()
+        resource = factory.make_name('resource')
+        image_dict.setdefault(image, resource)
+        self.assertItemsEqual([(image, resource)], image_dict.items())
+
+    def test_setdefault_leaves_set_item_unchanged(self):
+        image = make_image_spec()
+        old_resource = factory.make_name('resource')
+        image_dict = set_resource(image_spec=image, resource=old_resource)
+        image_dict.setdefault(image, factory.make_name('newresource'))
+        self.assertItemsEqual([(image, old_resource)], image_dict.items())
+
+    def test_dump_json_is_consistent(self):
+        image = make_image_spec()
+        resource = factory.make_name('resource')
+        image_dict_1 = set_resource(image_spec=image, resource=resource)
+        image_dict_2 = set_resource(image_spec=image, resource=resource)
+        self.assertEqual(image_dict_1.dump_json(), image_dict_2.dump_json())
+
+    def test_dump_json_represents_empty_dict_as_empty_object(self):
+        self.assertEqual('{}', boot_resources.BootImageMapping().dump_json())
+
+    def test_dump_json_represents_entry(self):
+        image = make_image_spec()
+        resource = factory.make_name('resource')
+        image_dict = set_resource(image_spec=image, resource=resource)
+        self.assertEqual(
+            {
+                image.arch: {
+                    image.subarch: {
+                        image.release: {image.label: resource},
+                    },
+                },
+            },
+            json.loads(image_dict.dump_json()))
+
+    def test_dump_json_combines_similar_entries(self):
+        image = make_image_spec()
+        other_release = factory.make_name('other-release')
+        resource1 = factory.make_name('resource')
+        resource2 = factory.make_name('other-resource')
+        image_dict = boot_resources.BootImageMapping()
+        set_resource(image_dict, image, resource1)
+        set_resource(
+            image_dict, image._replace(release=other_release), resource2)
+        self.assertEqual(
+            {
+                image.arch: {
+                    image.subarch: {
+                        image.release: {image.label: resource1},
+                        other_release: {image.label: resource2},
+                    },
+                },
+            },
+            json.loads(image_dict.dump_json()))
 
 
 class TestValuePassesFilterList(MAASTestCase):
@@ -151,7 +204,7 @@
         image1 = make_image_spec()
         image2 = make_image_spec()
         resource = self.make_boot_resource()
-        boot_dict = {}
+        boot_dict = boot_resources.BootImageMapping()
         # Create two images in boot_dict, both containing the same resource.
         for image in [image1, image2]:
             set_resource(
@@ -262,23 +315,14 @@
 
 
 def set_resource(boot_dict=None, image_spec=None, resource=None):
-    """Add a boot resource to `boot_dict`, creating it if necessary."""
+    """Add boot resource to a `BootImageMapping`, creating it if necessary."""
     if boot_dict is None:
-        boot_dict = {}
+        boot_dict = boot_resources.BootImageMapping()
     if image_spec is None:
         image_spec = make_image_spec()
     if resource is None:
         resource = factory.make_name('boot-resource')
-    arch, subarch, release, label = image_spec
-    # Drill down into the dict; along the way, create any missing levels of
-    # nested dicts.
-    nested_dict = boot_dict
-    for level in (arch, subarch, release):
-        nested_dict.setdefault(level, {})
-        nested_dict = nested_dict[level]
-    # At the bottom level, indexed by "label," insert "resource" as the
-    # value.
-    nested_dict[label] = resource
+    boot_dict.mapping[image_spec] = resource
     return boot_dict
 
 
@@ -288,12 +332,12 @@
     def test_integrates(self):
         # End-to-end scenario for boot_merge: start with an empty boot
         # resources dict, and receive one resource from Simplestreams.
-        total_resources = boot_resources.create_empty_hierarchy()
+        total_resources = boot_resources.BootImageMapping()
         resources_from_repo = set_resource()
-        boot_resources.boot_merge(total_resources, resources_from_repo.copy())
+        boot_resources.boot_merge(total_resources, resources_from_repo)
         # Since we started with an empty dict, the result contains the same
         # item that we got from Simplestreams, and nothing else.
-        self.assertEqual(resources_from_repo, total_resources)
+        self.assertEqual(resources_from_repo.mapping, total_resources.mapping)
 
     def test_obeys_filters(self):
         filters = [
@@ -312,13 +356,13 @@
 
     def test_does_not_overwrite_existing_entry(self):
         image = make_image_spec()
-        original_resources = set_resource(
+        total_resources = set_resource(
             resource="Original resource", image_spec=image)
-        total_resources = original_resources.copy()
+        original_resources = total_resources.mapping.copy()
         resources_from_repo = set_resource(
             resource="New resource", image_spec=image)
-        boot_resources.boot_merge(total_resources, resources_from_repo.copy())
-        self.assertEqual(original_resources, total_resources)
+        boot_resources.boot_merge(total_resources, resources_from_repo)
+        self.assertEqual(original_resources, total_resources.mapping)
 
 
 class TestGetSigningPolicy(MAASTestCase):