Adding BASE_IMAGE_ATTRS to ImageService

5 files changed, 145 insertions, 108 deletions

diff --git a/nova/api/openstack/images.py b/nova/api/openstack/images.py
index 0c56b5f0d..97e62c22d 100644
--- a/nova/api/openstack/images.py
+++ b/nova/api/openstack/images.py
@@ -119,43 +119,46 @@ def _translate_s3_like_images(image_metadata):
 def _translate_from_image_service_to_api(image_metadata):
     """Translate from ImageService to OpenStack API style attribute names
 
-    This involves 3 steps:
+    This involves 4 steps:
 
-        1. Translating required keys
+        1. Filter out attributes that the OpenStack API doesn't need
 
-        2. Translating optional keys (ex. progress, serverId)
+        2. Translate from base image attributes from names used by
+           BaseImageService to names used by OpenStack API
 
-        3. Formatting values according to API spec (for example dates must
+        3. Add in any image properties
+
+        4. Format values according to API spec (for example dates must
            look like "2010-08-10T12:00:00Z")
     """
     service_metadata = image_metadata.copy()
-    api_metadata = {}
-
-    # 1. Translate required keys
-    required_image_service2api = {
-        'id': 'id',
-        'name': 'name',
-        'updated_at': 'updated',
-        'created_at': 'created',
-        'status': 'status'}
-    for service_attr, api_attr in required_image_service2api.items():
-        api_metadata[api_attr] = service_metadata[service_attr]
-
-    # 2. Translate optional keys
-    optional_image_service2api = {'instance_id': 'serverId'}
-    for service_attr, api_attr in optional_image_service2api.items():
-        if service_attr in service_metadata:
-            api_metadata[api_attr] = service_metadata[service_attr]
-
-    # 2a. Progress special case
+    properties = service_metadata.pop('properties', {})
+
+    # 1. Filter out unecessary attributes
+    api_keys = ['id', 'name', 'updated_at', 'created_at', 'status']
+    api_metadata = utils.partition_dict(service_metadata, api_keys)[0]
+
+    # 2. Translate base image attributes
+    api_map = {'updated_at': 'updated', 'created_at': 'created'}
+    api_metadata = utils.map_dict_keys(api_metadata, api_map)
+
+    # 3. Add in any image properties
+    # 3a. serverId is used for backups and snapshots
+    try:
+        api_metadata['serverId'] = int(properties['instance_id'])
+    except KeyError:
+        pass  # skip if it's not present
+    except ValueError:
+        pass  # skip if it's not an integer
+
+    # 3b. Progress special case
     # TODO(sirp): ImageService doesn't have a notion of progress yet, so for
     # now just fake it
     if service_metadata['status'] == 'saving':
         api_metadata['progress'] = 0
 
-    # 3. Format values
-
-    # 3a. Format Image Status (API requires uppercase)
+    # 4. Format values
+    # 4a. Format Image Status (API requires uppercase)
     status_service2api = {'queued': 'QUEUED',
                           'preparing': 'PREPARING',
                           'saving': 'SAVING',
@@ -163,7 +166,7 @@ def _translate_from_image_service_to_api(image_metadata):
                           'killed': 'FAILED'}
     api_metadata['status'] = status_service2api[api_metadata['status']]
 
-    # 3b. Format timestamps
+    # 4b. Format timestamps
     def _format_timestamp(dt_str):
         """Return a timestamp formatted for OpenStack API
 
diff --git a/nova/image/glance.py b/nova/image/glance.py
index 2def6fb60..b7bb88002 100644
--- a/nova/image/glance.py
+++ b/nova/image/glance.py
@@ -36,7 +36,14 @@ GlanceClient = utils.import_class('glance.client.Client')
 
 class GlanceImageService(service.BaseImageService):
     """Provides storage and retrieval of disk image objects within Glance."""
-    IMAGE_PROPERTIES = ['instance_id', 'os_type']
+
+    GLANCE_ONLY_ATTRS = ["size", "location", "disk_format",
+                         "container_format"]
+
+    # NOTE(sirp): Overriding to use _translate_to_service provided by
+    # BaseImageService
+    SERVICE_IMAGE_ATTRS = service.BaseImageService.BASE_IMAGE_ATTRS +\
+                          GLANCE_ONLY_ATTRS
 
     def __init__(self):
         self.client = GlanceClient(FLAGS.glance_host, FLAGS.glance_port)
@@ -52,7 +59,7 @@ class GlanceImageService(service.BaseImageService):
         Calls out to Glance for a list of detailed image information
         """
         image_metas = self.client.get_images_detailed()
-        translate = self._translate_from_glance_to_image_service
+        translate = self._translate_to_base
         return [translate(image_meta) for image_meta in image_metas]
 
     def show(self, context, image_id):
@@ -60,11 +67,11 @@ class GlanceImageService(service.BaseImageService):
         Returns a dict containing image data for the given opaque image id.
         """
         try:
-            metadata = self.client.get_image_meta(image_id)
+            image_meta = self.client.get_image_meta(image_id)
         except glance_exception.NotFound:
             raise exception.NotFound
 
-        meta = self._translate_from_glance_to_image_service(metadata)
+        meta = self._translate_to_base(image_meta)
         return meta
 
     def show_by_name(self, context, name):
@@ -88,13 +95,14 @@ class GlanceImageService(service.BaseImageService):
         Calls out to Glance for metadata and data and writes data.
         """
         try:
-            metadata, image_chunks = self.client.get_image(image_id)
+            image_meta, image_chunks = self.client.get_image(image_id)
         except glance_exception.NotFound:
             raise exception.NotFound
+
         for chunk in image_chunks:
             data.write(chunk)
 
-        meta = self._translate_from_glance_to_image_service(metadata)
+        meta = self._translate_to_base(image_meta)
         return meta
 
     def create(self, context, metadata, data=None):
@@ -102,11 +110,10 @@ class GlanceImageService(service.BaseImageService):
         Store the image data and return the new image id.
 
         :raises AlreadyExists if the image already exist.
-
         """
         LOG.debug(_("Creating image in Glance. Metadata passed in %s"),
                   metadata)
-        meta = self._translate_from_image_service_to_glance(metadata)
+        meta = self._translate_to_service(metadata)
         LOG.debug(_("Metadata after formatting for Glance %s"), meta)
         return self.client.add_image(meta, data)
 
@@ -114,7 +121,6 @@ class GlanceImageService(service.BaseImageService):
         """Replace the contents of the given image with the new data.
 
         :raises NotFound if the image does not exist.
-
         """
         try:
             result = self.client.update_image(image_id, metadata, data)
@@ -127,7 +133,6 @@ class GlanceImageService(service.BaseImageService):
         Delete the given image.
 
         :raises NotFound if the image does not exist.
-
         """
         try:
             result = self.client.delete_image(image_id)
@@ -140,65 +145,3 @@ class GlanceImageService(service.BaseImageService):
         Clears out all images
         """
         pass
-
-    @classmethod
-    def _translate_from_image_service_to_glance(cls, metadata):
-        """Return a metadata dict suitable for passing to Glance.
-
-        The ImageService exposes metadata as a flat-dict; however, Glance
-        distinguishes between two different types of metadata:
-
-            1. First-class attributes: These are columns on the image table
-               and represent metadata that is common to all images on all IAAS
-               providers.
-
-            2. Properties: These are entries in the image_properties table and
-               represent image/IAAS-provider specific metadata.
-
-        To reconcile this difference, this function accepts a flat-dict of
-        metadata, figures out which attributes are stored as image properties
-        in Glance, and then adds those to a `properties` dict nested within
-        the metadata.
-
-        """
-        glance_metadata = metadata.copy()
-        properties = {}
-        for property_ in cls.IMAGE_PROPERTIES:
-            if property_ in glance_metadata:
-                value = glance_metadata.pop(property_)
-                properties[property_] = str(value)
-        glance_metadata['properties'] = properties
-        return glance_metadata
-
-    @classmethod
-    def _translate_from_glance_to_image_service(cls, metadata):
-        """Convert Glance-style image metadata to ImageService-style
-
-        The steps in involved are:
-
-            1. Extracting Glance properties and making them ImageService
-               attributes
-
-            2. Converting any strings to appropriate values
-        """
-        service_metadata = metadata.copy()
-
-        # 1. Extract properties
-        if 'properties' in service_metadata:
-            properties = service_metadata.pop('properties')
-            for property_ in cls.IMAGE_PROPERTIES:
-                if ((property_ in properties) and
-                    (property_ not in service_metadata)):
-                    value = properties[property_]
-                    service_metadata[property_] = value
-
-        # 2. Convert values
-        try:
-            service_metadata['instance_id'] = int(
-                service_metadata['instance_id'])
-        except KeyError:
-            pass  # instance_id is not required
-        except TypeError:
-            pass  # instance_id can be None
-
-        return service_metadata
diff --git a/nova/image/service.py b/nova/image/service.py
index c09052cab..ce4954fd1 100644
--- a/nova/image/service.py
+++ b/nova/image/service.py
@@ -16,9 +16,68 @@
 #    under the License.
 
 
+from nova import utils
+
+
 class BaseImageService(object):
+    """Base class for providing image search and retrieval services
+
+    ImageService exposes two concepts of metadata:
+
+        1. First-class attributes: This is metadata that is common to all
+           ImageService subclasses and is shared across all hypervisors. These
+           attributes are defined by IMAGE_ATTRS.
+
+        2. Properties: This is metdata that is specific to an ImageService,
+           and Image, or a particular hypervisor. Any attribute not present in
+           BASE_IMAGE_ATTRS should be considered an image property.
+
+    This means that ImageServices will return BASE_IMAGE_ATTRS as keys in the
+    metadata dict, all other attributes will be returned as keys in the nested
+    'properties' dict.
+    """
+    BASE_IMAGE_ATTRS = ['id', 'name', 'created_at', 'updated_at',
+                        'deleted_at', 'deleted', 'status', 'is_public']
 
-    """Base class for providing image search and retrieval services"""
+    # NOTE(sirp): ImageService subclasses may override this to aid translation
+    # between BaseImageService attributes and additional metadata stored by
+    # the ImageService subclass
+    SERVICE_IMAGE_ATTRS = []
+
+    @classmethod
+    def _translate_to_base(cls, metadata):
+        """Return a metadata dictionary that is BaseImageService compliant.
+
+        This is used by subclasses to expose only a metadata dictionary that
+        is the same across ImageService implementations.
+        """
+        return cls.propertify_metadata(metadata, cls.BASE_IMAGE_ATTRS)
+
+    @classmethod
+    def _translate_to_service(cls, metadata):
+        """Return a metadata dictionary that is usable by the ImageService
+        subclass.
+
+        As an example, Glance has additional attributes (like 'location'); the
+        BaseImageService considers these properties, but we need to translate
+        these back to first-class attrs for sending to Glance. This method
+        handles this by allowing you to specify the attributes an ImageService
+        considers first-class.
+        """
+        if not cls.SERVICE_IMAGE_ATTRS:
+            raise NotImplementedError(_("Cannot use this without specifying "
+                                        "SERVICE_IMAGE_ATTRS for subclass"))
+        return cls.propertify_metadata(metadata, cls.SERVICE_IMAGE_ATTRS)
+
+    @staticmethod
+    def propertify_metadata(metadata, keys):
+        """Return a dict with any unrecognized keys placed in the nested
+        'properties' dict.
+        """
+        flattened = utils.flatten_dict(metadata)
+        attributes, properties = utils.partition_dict(flattened, keys)
+        attributes['properties'] = properties
+        return attributes
 
     def index(self, context):
         """
diff --git a/nova/tests/api/openstack/test_images.py b/nova/tests/api/openstack/test_images.py
index 4604b331e..03f22842b 100644
--- a/nova/tests/api/openstack/test_images.py
+++ b/nova/tests/api/openstack/test_images.py
@@ -185,11 +185,10 @@ class GlanceImageServiceTest(test.TestCase,
         super(GlanceImageServiceTest, self).tearDown()
 
     def test_create_with_instance_id(self):
-        """
-        Ensure that a instance_id is stored in Glance as a image property
-        string and then converted back to an instance_id integer attribute.
-        """
-        fixture = {'instance_id': 42, 'name': 'test image'}
+        """Ensure instance_id is persisted as an image-property"""
+        fixture = {'name': 'test image',
+                   'properties': {'instance_id': '42'}}
+
         image_id = self.service.create(self.context, fixture)['id']
 
         expected = {'id': image_id,
@@ -197,9 +196,6 @@ class GlanceImageServiceTest(test.TestCase,
                     'properties': {'instance_id': '42'}}
         self.assertDictMatch(self.sent_to_glance['metadata'], expected)
 
-        # The ImageService shouldn't leak the fact that the instance_id
-        # happens to be stored as a property in Glance
-        expected = {'id': image_id, 'instance_id': 42, 'name': 'test image'}
         image_meta = self.service.show(self.context, image_id)
         self.assertDictMatch(image_meta, expected)
 
diff --git a/nova/utils.py b/nova/utils.py
index 199ee8701..96a51b425 100644
--- a/nova/utils.py
+++ b/nova/utils.py
@@ -597,3 +597,39 @@ def get_from_path(items, path):
         return results
     else:
         return get_from_path(results, remainder)
+
+
+def flatten_dict(dict_, flattened=None):
+    """Recursively flatten a nested dictionary"""
+    flattened = flattened or {}
+    for key, value in dict_.iteritems():
+        if hasattr(value, 'iteritems'):
+            flatten_dict(value, flattened)
+        else:
+            flattened[key] = value
+    return flattened
+
+
+def partition_dict(dict_, keys):
+    """Return two dicts, one containing only `keys` the other containing
+    everything but `keys`
+    """
+    intersection = {}
+    difference = {}
+    for key, value in dict_.iteritems():
+        if key in keys:
+            intersection[key] = value
+        else:
+            difference[key] = value
+    return intersection, difference
+
+
+def map_dict_keys(dict_, key_map):
+    """Return a dictionary in which the dictionaries keys are mapped to 
+    new keys.
+    """
+    mapped = {}
+    for key, value in dict_.iteritems():
+        mapped_key = key_map[key] if key in key_map else key
+        mapped[mapped_key] = value
+    return mapped