Fixes issues with describe instances due to improperly set metadata.

 * Removes image['properties']['type']
 * Uses image['container_format'] to key display of type and create ec2 ids.
 * Defaults to 'ami' if container_format cannot be deduced. This allows
   bare images to show up in describe instances and be launched even though they
   are not officially in 'ami' format.
 * Changes nova-manage register to set proper container format
 * Fixes tests
 * Fixes _do_get_kernel_and_ramdisk in openstack api to only try to get them if it is a true 'ami'
 * Replaces 'owner_id' with 'project_id' since that is expected by glance code
 * Moves the filtering scheme to base image service so all services filter the same way

12 files changed, 122 insertions, 121 deletions

diff --git a/bin/nova-manage b/bin/nova-manage
index b80a6e31d..bac291c01 100755
--- a/bin/nova-manage
+++ b/bin/nova-manage
@@ -894,20 +894,17 @@ class ImageCommands(object):
     def __init__(self, *args, **kwargs):
         self.image_service = utils.import_object(FLAGS.image_service)
 
-    def _register(self, image_type, disk_format, container_format,
+    def _register(self, container_format, disk_format,
                   path, owner, name=None, is_public='T',
                   architecture='x86_64', kernel_id=None, ramdisk_id=None):
-        meta = {'is_public': True,
+        meta = {'is_public': (is_public == 'T'),
                 'name': name,
-                'disk_format': disk_format,
                 'container_format': container_format,
+                'disk_format': disk_format,
                 'properties': {'image_state': 'available',
-                               'owner_id': owner,
-                               'type': image_type,
+                               'project_id': owner,
                                'architecture': architecture,
-                               'image_location': 'local',
-                               'is_public': (is_public == 'T')}}
-        print image_type, meta
+                               'image_location': 'local'}}
         if kernel_id:
             meta['properties']['kernel_id'] = int(kernel_id)
         if ramdisk_id:
@@ -932,16 +929,18 @@ class ImageCommands(object):
         ramdisk_id = self.ramdisk_register(ramdisk, owner, None,
                                     is_public, architecture)
         self.image_register(image, owner, name, is_public,
-                       architecture, kernel_id, ramdisk_id)
+                            architecture, 'ami', 'ami',
+                            kernel_id, ramdisk_id)
 
     def image_register(self, path, owner, name=None, is_public='T',
-                       architecture='x86_64', kernel_id=None, ramdisk_id=None,
-                       disk_format='ami', container_format='ami'):
+                       architecture='x86_64', container_format='bare',
+                       disk_format='raw', kernel_id=None, ramdisk_id=None):
         """Uploads an image into the image_service
         arguments: path owner [name] [is_public='T'] [architecture='x86_64']
+                   [container_format='bare'] [disk_format='raw']
                    [kernel_id=None] [ramdisk_id=None]
-                   [disk_format='ami'] [container_format='ami']"""
-        return self._register('machine', disk_format, container_format, path,
+        """
+        return self._register(container_format, disk_format, path,
                               owner, name, is_public, architecture,
                               kernel_id, ramdisk_id)
 
@@ -950,7 +949,7 @@ class ImageCommands(object):
         """Uploads a kernel into the image_service
         arguments: path owner [name] [is_public='T'] [architecture='x86_64']
         """
-        return self._register('kernel', 'aki', 'aki', path, owner, name,
+        return self._register('aki', 'aki', path, owner, name,
                               is_public, architecture)
 
     def ramdisk_register(self, path, owner, name=None, is_public='T',
@@ -958,7 +957,7 @@ class ImageCommands(object):
         """Uploads a ramdisk into the image_service
         arguments: path owner [name] [is_public='T'] [architecture='x86_64']
         """
-        return self._register('ramdisk', 'ari', 'ari', path, owner, name,
+        return self._register('ari', 'ari', path, owner, name,
                               is_public, architecture)
 
     def _lookup(self, old_image_id):
@@ -975,16 +974,17 @@ class ImageCommands(object):
                    'ramdisk': 'ari'}
         container_format = mapping[old['type']]
         disk_format = container_format
+        if container_format == 'ami' and not old.get('kernelId'):
+            container_format = 'bare'
+            disk_format = 'raw'
         new = {'disk_format': disk_format,
                'container_format': container_format,
-               'is_public': True,
+               'is_public': old['isPublic'],
                'name': old['imageId'],
                'properties': {'image_state': old['imageState'],
-                              'owner_id': old['imageOwnerId'],
+                              'project_id': old['imageOwnerId'],
                               'architecture': old['architecture'],
-                              'type': old['type'],
-                              'image_location': old['imageLocation'],
-                              'is_public': old['isPublic']}}
+                              'image_location': old['imageLocation']}}
         if old.get('kernelId'):
             new['properties']['kernel_id'] = self._lookup(old['kernelId'])
         if old.get('ramdiskId'):
diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py
index c3124b89d..4ed8a9ecf 100644
--- a/nova/api/ec2/cloud.py
+++ b/nova/api/ec2/cloud.py
@@ -154,7 +154,7 @@ class CloudController(object):
         floating_ip = db.instance_get_floating_address(ctxt,
                                                        instance_ref['id'])
         ec2_id = ec2utils.id_to_ec2_id(instance_ref['id'])
-        image_ec2_id = self._image_ec2_id(instance_ref['image_id'], 'machine')
+        image_ec2_id = self._image_ec2_id(instance_ref['image_id'], 'ami')
         data = {
             'user-data': base64.b64decode(instance_ref['user_data']),
             'meta-data': {
@@ -184,7 +184,7 @@ class CloudController(object):
         for image_type in ['kernel', 'ramdisk']:
             if '%s_id' % image_type in instance_ref:
                 ec2_id = self._image_ec2_id(instance_ref['%s_id' % image_type],
-                                            image_type)
+                                            self._image_type(image_type))
                 data['meta-data']['%s-id' % image_type] = ec2_id
 
         if False:  # TODO(vish): store ancestor ids
@@ -875,13 +875,27 @@ class CloudController(object):
             self.compute_api.update(context, instance_id=instance_id, **kwargs)
         return True
 
-    _type_prefix_map = {'machine': 'ami',
-                        'kernel': 'aki',
-                        'ramdisk': 'ari'}
+    @staticmethod
+    def _image_type(image_type):
+        """Converts to a three letter image type.
 
-    def _image_ec2_id(self, image_id, image_type='machine'):
-        prefix = self._type_prefix_map[image_type]
-        template = prefix + '-%08x'
+        aki, kernel => aki
+        ari, ramdisk => ari
+        anything else => ami
+
+        """
+        if image_type == 'kernel':
+            return 'aki'
+        if image_type == 'ramdisk':
+            return 'ari'
+        if image_type not in ['aki', 'ari']:
+            return 'ami'
+        return image_type
+
+    @staticmethod
+    def _image_ec2_id(image_id, image_type='ami'):
+        """Returns image ec2_id using id and three letter type."""
+        template = image_type + '-%08x'
         return ec2utils.id_to_ec2_id(int(image_id), template=template)
 
     def _get_image(self, context, ec2_id):
@@ -894,27 +908,34 @@ class CloudController(object):
     def _format_image(self, image):
         """Convert from format defined by BaseImageService to S3 format."""
         i = {}
-        image_type = image['properties'].get('type')
+        image_type = self._image_type(image.get('container_format'))
         ec2_id = self._image_ec2_id(image.get('id'), image_type)
         name = image.get('name')
         i['imageId'] = ec2_id
         kernel_id = image['properties'].get('kernel_id')
         if kernel_id:
-            i['kernelId'] = self._image_ec2_id(kernel_id, 'kernel')
+            i['kernelId'] = self._image_ec2_id(kernel_id, 'aki')
         ramdisk_id = image['properties'].get('ramdisk_id')
         if ramdisk_id:
-            i['ramdiskId'] = self._image_ec2_id(ramdisk_id, 'ramdisk')
+            i['ramdiskId'] = self._image_ec2_id(ramdisk_id, 'ari')
         i['imageOwnerId'] = image['properties'].get('owner_id')
         if name:
             i['imageLocation'] = "%s (%s)" % (image['properties'].
                                               get('image_location'), name)
         else:
             i['imageLocation'] = image['properties'].get('image_location')
-        i['imageState'] = image['properties'].get('image_state')
+        # NOTE(vish): fallback status if image_state isn't set
+        state = image.get('status')
+        if state == 'active':
+            state = 'available'
+        i['imageState'] = image['properties'].get('image_state', state)
         i['displayName'] = name
         i['description'] = image.get('description')
-        i['imageType'] = image_type
-        i['isPublic'] = str(image['properties'].get('is_public', '')) == 'True'
+        display_mapping = {'aki': 'kernel',
+                           'ari': 'ramdisk',
+                           'ami': 'machine'}
+        i['imageType'] = display_mapping.get(image_type)
+        i['isPublic'] = image.get('is_public') == True
         i['architecture'] = image['properties'].get('architecture')
         return i
 
@@ -946,8 +967,9 @@ class CloudController(object):
             image_location = kwargs['name']
         metadata = {'properties': {'image_location': image_location}}
         image = self.image_service.create(context, metadata)
+        image_type = self._image_type(image.get('container_format'))
         image_id = self._image_ec2_id(image['id'],
-                                      image['properties']['type'])
+                                      image_type)
         msg = _("Registered image %(image_location)s with"
                 " id %(image_id)s") % locals()
         LOG.audit(msg, context=context)
diff --git a/nova/api/openstack/servers.py b/nova/api/openstack/servers.py
index c36fde8c8..43e0c7963 100644
--- a/nova/api/openstack/servers.py
+++ b/nova/api/openstack/servers.py
@@ -565,7 +565,7 @@ class Controller(common.OpenstackController):
                 _("Cannot build from image %(image_id)s, status not active") %
                   locals())
 
-        if image_meta['properties']['disk_format'] != 'ami':
+        if image_meta.get('container_format') != 'ami':
             return None, None
 
         try:
diff --git a/nova/image/fake.py b/nova/image/fake.py
index 08302d6eb..d1c62757f 100644
--- a/nova/image/fake.py
+++ b/nova/image/fake.py
@@ -44,10 +44,10 @@ class FakeImageService(service.BaseImageService):
                  'created_at': timestamp,
                  'updated_at': timestamp,
                  'status': 'active',
-                 'type': 'machine',
+                 'container_format': 'ami',
+                 'disk_format': 'raw',
                  'properties': {'kernel_id': FLAGS.null_kernel,
-                                'ramdisk_id': FLAGS.null_kernel,
-                                'disk_format': 'ami'}
+                                'ramdisk_id': FLAGS.null_kernel}
                 }
         self.create(None, image)
         super(FakeImageService, self).__init__()
diff --git a/nova/image/glance.py b/nova/image/glance.py
index fdf468594..bf49ca96c 100644
--- a/nova/image/glance.py
+++ b/nova/image/glance.py
@@ -151,6 +151,8 @@ class GlanceImageService(service.BaseImageService):
 
         :raises NotFound if the image does not exist.
         """
+        # NOTE(vish): show is to check if image is available
+        self.show(context, image_id)
         try:
             image_meta = self.client.update_image(image_id, image_meta, data)
         except glance_exception.NotFound:
@@ -165,6 +167,8 @@ class GlanceImageService(service.BaseImageService):
 
         :raises NotFound if the image does not exist.
         """
+        # NOTE(vish): show is to check if image is available
+        self.show(context, image_id)
         try:
             result = self.client.delete_image(image_id)
         except glance_exception.NotFound:
@@ -186,33 +190,6 @@ class GlanceImageService(service.BaseImageService):
         image_meta = _convert_timestamps_to_datetimes(image_meta)
         return image_meta
 
-    @staticmethod
-    def _is_image_available(context, image_meta):
-        """
-        Images are always available if they are public or if the user is an
-        admin.
-
-        Otherwise, we filter by project_id (if present) and then fall-back to
-        images owned by user.
-        """
-        # FIXME(sirp): We should be filtering by user_id on the Glance side
-        # for security; however, we can't do that until we get authn/authz
-        # sorted out. Until then, filtering in Nova.
-        if image_meta['is_public'] or context.is_admin:
-            return True
-
-        properties = image_meta['properties']
-
-        if context.project_id and ('project_id' in properties):
-            return str(properties['project_id']) == str(project_id)
-
-        try:
-            user_id = properties['user_id']
-        except KeyError:
-            return False
-
-        return str(user_id) == str(context.user_id)
-
 
 # utility functions
 def _convert_timestamps_to_datetimes(image_meta):
diff --git a/nova/image/local.py b/nova/image/local.py
index 1fb6e1f13..d4fd62156 100644
--- a/nova/image/local.py
+++ b/nova/image/local.py
@@ -84,7 +84,10 @@ class LocalImageService(service.BaseImageService):
     def show(self, context, image_id):
         try:
             with open(self._path_to(image_id)) as metadata_file:
-                return json.load(metadata_file)
+                image_meta = json.load(metadata_file)
+                if not self._is_image_available(context, image_meta):
+                    raise exception.NotFound
+                return image_meta
         except (IOError, ValueError):
             raise exception.NotFound
 
@@ -119,10 +122,15 @@ class LocalImageService(service.BaseImageService):
         image_path = self._path_to(image_id, None)
         if not os.path.exists(image_path):
             os.mkdir(image_path)
-        return self.update(context, image_id, metadata, data)
+        return self._store(context, image_id, metadata, data)
 
     def update(self, context, image_id, metadata, data=None):
         """Replace the contents of the given image with the new data."""
+        # NOTE(vish): show is to check if image is available
+        self.show(context, image_id)
+        return self._store(context, image_id, metadata, data)
+
+    def _store(self, context, image_id, metadata, data=None):
         metadata['id'] = image_id
         try:
             if data:
@@ -140,9 +148,11 @@ class LocalImageService(service.BaseImageService):
 
     def delete(self, context, image_id):
         """Delete the given image.
-        Raises OSError if the image does not exist.
+        Raises NotFound if the image does not exist.
 
         """
+        # NOTE(vish): show is to check if image is available
+        self.show(context, image_id)
         try:
             shutil.rmtree(self._path_to(image_id, None))
         except (IOError, ValueError):
diff --git a/nova/image/s3.py b/nova/image/s3.py
index ddec5f3aa..554760d53 100644
--- a/nova/image/s3.py
+++ b/nova/image/s3.py
@@ -46,6 +46,7 @@ flags.DEFINE_string('image_decryption_dir', '/tmp',
 
 
 class S3ImageService(service.BaseImageService):
+    """Wraps an existing image service to support s3 based register"""
     def __init__(self, service=None, *args, **kwargs):
         if service == None:
             service = utils.import_object(FLAGS.image_service)
@@ -58,52 +59,23 @@ class S3ImageService(service.BaseImageService):
         return image
 
     def delete(self, context, image_id):
-        # FIXME(vish): call to show is to check filter
-        self.show(context, image_id)
         self.service.delete(context, image_id)
 
     def update(self, context, image_id, metadata, data=None):
-        # FIXME(vish): call to show is to check filter
-        self.show(context, image_id)
         image = self.service.update(context, image_id, metadata, data)
         return image
 
     def index(self, context):
-        images = self.service.index(context)
-        # FIXME(vish): index doesn't filter so we do it manually
-        return self._filter(context, images)
+        return self.service.index(context)
 
     def detail(self, context):
-        images = self.service.detail(context)
-        # FIXME(vish): detail doesn't filter so we do it manually
-        return self._filter(context, images)
-
-    @classmethod
-    def _is_visible(cls, context, image):
-        return (context.is_admin
-                or context.project_id == image['properties']['owner_id']
-                or image['properties']['is_public'] == 'True')
-
-    @classmethod
-    def _filter(cls, context, images):
-        filtered = []
-        for image in images:
-            if not cls._is_visible(context, image):
-                continue
-            filtered.append(image)
-        return filtered
+        return self.service.detail(context)
 
     def show(self, context, image_id):
-        image = self.service.show(context, image_id)
-        if not self._is_visible(context, image):
-            raise exception.NotFound
-        return image
+        return self.service.show(context, image_id)
 
     def show_by_name(self, context, name):
-        image = self.service.show_by_name(context, name)
-        if not self._is_visible(context, image):
-            raise exception.NotFound
-        return image
+        return self.service.show(context, name)
 
     @staticmethod
     def _conn(context):
@@ -167,7 +139,7 @@ class S3ImageService(service.BaseImageService):
             arch = 'x86_64'
 
         properties = metadata['properties']
-        properties['owner_id'] = context.project_id
+        properties['project_id'] = context.project_id
         properties['architecture'] = arch
 
         if kernel_id:
@@ -176,8 +148,6 @@ class S3ImageService(service.BaseImageService):
         if ramdisk_id:
             properties['ramdisk_id'] = ec2utils.ec2_id_to_id(ramdisk_id)
 
-        properties['is_public'] = False
-        properties['type'] = image_type
         metadata.update({'disk_format': image_format,
                          'container_format': image_format,
                          'status': 'queued',
diff --git a/nova/image/service.py b/nova/image/service.py
index b9897ecae..fddc72409 100644
--- a/nova/image/service.py
+++ b/nova/image/service.py
@@ -136,6 +136,33 @@ class BaseImageService(object):
         """
         raise NotImplementedError
 
+    @staticmethod
+    def _is_image_available(context, image_meta):
+        """
+        Images are always available if they are public or if the user is an
+        admin.
+
+        Otherwise, we filter by project_id (if present) and then fall-back to
+        images owned by user.
+        """
+        # FIXME(sirp): We should be filtering by user_id on the Glance side
+        # for security; however, we can't do that until we get authn/authz
+        # sorted out. Until then, filtering in Nova.
+        if image_meta['is_public'] or context.is_admin:
+            return True
+
+        properties = image_meta['properties']
+
+        if context.project_id and ('project_id' in properties):
+            return str(properties['project_id']) == str(context.project_id)
+
+        try:
+            user_id = properties['user_id']
+        except KeyError:
+            return False
+
+        return str(user_id) == str(context.user_id)
+
     @classmethod
     def _translate_to_base(cls, metadata):
         """Return a metadata dictionary that is BaseImageService compliant.
diff --git a/nova/tests/api/openstack/test_image_metadata.py b/nova/tests/api/openstack/test_image_metadata.py
index 9be753f84..7c3287006 100644
--- a/nova/tests/api/openstack/test_image_metadata.py
+++ b/nova/tests/api/openstack/test_image_metadata.py
@@ -45,7 +45,6 @@ class ImageMetaDataTest(unittest.TestCase):
         'is_public': True,
         'deleted_at': None,
         'properties': {
-            'type': 'ramdisk',
             'key1': 'value1',
             'key2': 'value2'
         },
@@ -62,7 +61,6 @@ class ImageMetaDataTest(unittest.TestCase):
         'is_public': True,
         'deleted_at': None,
         'properties': {
-            'type': 'ramdisk',
             'key1': 'value1',
             'key2': 'value2'
         },
diff --git a/nova/tests/api/openstack/test_servers.py b/nova/tests/api/openstack/test_servers.py
index 8f2931535..f10bc1d7b 100644
--- a/nova/tests/api/openstack/test_servers.py
+++ b/nova/tests/api/openstack/test_servers.py
@@ -1643,29 +1643,27 @@ class TestGetKernelRamdiskFromImage(test.TestCase):
 
     def test_not_ami(self):
         """Anything other than ami should return no kernel and no ramdisk"""
-        image_meta = {'id': 1, 'status': 'active',
-                      'properties': {'disk_format': 'vhd'}}
+        image_meta = {'id': 1, 'status': 'active', 'container_format': 'vhd'}
         kernel_id, ramdisk_id = self._get_k_r(image_meta)
         self.assertEqual(kernel_id, None)
         self.assertEqual(ramdisk_id, None)
 
     def test_ami_no_kernel(self):
         """If an ami is missing a kernel it should raise NotFound"""
-        image_meta = {'id': 1, 'status': 'active',
-                      'properties': {'disk_format': 'ami', 'ramdisk_id': 1}}
+        image_meta = {'id': 1, 'status': 'active', 'container_format': 'ami',
+                      'properties': {'ramdisk_id': 1}}
         self.assertRaises(exception.NotFound, self._get_k_r, image_meta)
 
     def test_ami_no_ramdisk(self):
         """If an ami is missing a ramdisk it should raise NotFound"""
-        image_meta = {'id': 1, 'status': 'active',
-                      'properties': {'disk_format': 'ami', 'kernel_id': 1}}
+        image_meta = {'id': 1, 'status': 'active', 'container_format': 'ami',
+                      'properties': {'kernel_id': 1}}
         self.assertRaises(exception.NotFound, self._get_k_r, image_meta)
 
     def test_ami_kernel_ramdisk_present(self):
         """Return IDs if both kernel and ramdisk are present"""
-        image_meta = {'id': 1, 'status': 'active',
-                      'properties': {'disk_format': 'ami', 'kernel_id': 1,
-                                     'ramdisk_id': 2}}
+        image_meta = {'id': 1, 'status': 'active', 'container_format': 'ami',
+                      'properties': {'kernel_id': 1, 'ramdisk_id': 2}}
         kernel_id, ramdisk_id = self._get_k_r(image_meta)
         self.assertEqual(kernel_id, 1)
         self.assertEqual(ramdisk_id, 2)
diff --git a/nova/tests/image/test_glance.py b/nova/tests/image/test_glance.py
index 9d0b14613..109905ded 100644
--- a/nova/tests/image/test_glance.py
+++ b/nova/tests/image/test_glance.py
@@ -209,17 +209,17 @@ class TestMutatorDateTimeTests(BaseGlanceTest):
         self.assertDateTimesEmpty(image_meta)
 
     def test_update_handles_datetimes(self):
+        self.client.images = {'image1': self._make_datetime_fixture()}
         self.client.update_response = self._make_datetime_fixture()
-        dummy_id = 'dummy_id'
         dummy_meta = {}
-        image_meta = self.service.update(self.context, 'dummy_id', dummy_meta)
+        image_meta = self.service.update(self.context, 'image1', dummy_meta)
         self.assertDateTimesFilled(image_meta)
 
     def test_update_handles_none_datetimes(self):
+        self.client.images = {'image1': self._make_datetime_fixture()}
         self.client.update_response = self._make_none_datetime_fixture()
-        dummy_id = 'dummy_id'
         dummy_meta = {}
-        image_meta = self.service.update(self.context, 'dummy_id', dummy_meta)
+        image_meta = self.service.update(self.context, 'image1', dummy_meta)
         self.assertDateTimesEmpty(image_meta)
 
     def _make_datetime_fixture(self):
diff --git a/nova/virt/libvirt_conn.py b/nova/virt/libvirt_conn.py
index 75602f3d0..b949e6c92 100644
--- a/nova/virt/libvirt_conn.py
+++ b/nova/virt/libvirt_conn.py
@@ -422,7 +422,6 @@ class LibvirtConnection(driver.ComputeDriver):
                     'container_format': base['container_format'],
                     'is_public': False,
                     'properties': {'architecture': base['architecture'],
-                                   'type': base['type'],
                                    'name': '%s.%s' % (base['name'], image_id),
                                    'kernel_id': instance['kernel_id'],
                                    'image_location': 'snapshot',