Merge lp:~vishvananda/nova/fix-describe-images into lp:~hudson-openstack/nova/trunk

[Vish Ishaya]

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

[termie (termie)]

- _image_ec2_id's docstring needs punctuation.
- image_register's docstring needs the closing quotes moved down a bit.

why did we change the argument order of image_register? is that method being tested anywhere? it seems like a possible compat issue so it'd be nice to know how far reaching it is

review: Needs Fixing

[Vish Ishaya (vishvananda)]

On Mar 30, 2011, at 1:41 PM, termie wrote:

> Review: Needs Fixing
> - _image_ec2_id's docstring needs punctuation.

Fixed.

> - image_register's docstring needs the closing quotes moved down a bit.

The docstrings in nova manage are displayed to the user, so they are all in this format. I'll go ahead and change this one, but it needs a whole-file cleanup.

>
> why did we change the argument order of image_register? is that method being tested anywhere? it seems like a possible compat issue so it'd be nice to know how far reaching it is

The old ordering made it impossible to register images with no kernel and ramdisk and a different format, because there isn't a way to pass "None" through nova-manage.
No other code is using this. It is user-facing. We don't have tests for nova-manage, and this is something that should be addressed, but I'm not sure that it is in the scope of this patch.

> --
> https://code.launchpad.net/~vishvananda/nova/fix-describe-images/+merge/55617
> You are the owner of lp:~vishvananda/nova/fix-describe-images.

[Devin Carlen (devcamcar)]

nice, lgtm!

review: Approve

[Brian Lamar (blamar)]

I see the initialism 'AMI' being used in non-EC2-specific code (bin/nova-manage), is AMI being used here as an analogue for "some sort of machine image"? If so, is there any way to better indicate a generic machine image. Sometimes I look at the code and see abbreviations such as AMI/ARI/AKI and I assume we're talking about Amazon-specific images.

> (is_public == 'T')

Can this just be a boolean?

review: Needs Information

[Vish Ishaya (vishvananda)]

> I see the initialism 'AMI' being used in non-EC2-specific code (bin/nova-
> manage), is AMI being used here as an analogue for "some sort of machine
> image"? If so, is there any way to better indicate a generic machine image.
> Sometimes I look at the code and see abbreviations such as AMI/ARI/AKI and I
> assume we're talking about Amazon-specific images.

AMI is supposed to only be used for images that have a separate kernel and ramdisk. Nova-manage sets the type to ami if you specify kernel and ramdisk. Otherwise it assumes a bare image.

To clients of the ec2 api, all images are displayed as ami's because there isn't really another image type shown there.

I hope that makes it clearer

>
> > (is_public == 'T')

nova-manage command line processing doesn't really support conversion to boolean, so this does the conversion to boolean from the text passed on the command line.
>
> Can this just be a boolean?

[Rick Harris (rconradharris)]

Looks good, thanks Vish.

review: Approve

[Devin Carlen (devcamcar)]

Looks like termie's concerns were addressed, so going to approve this.

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~vishvananda/nova/fix-describe-images into lp:nova failed. Below is the output from the failed tests.

AccountsTest
    test_account_create OK
    test_account_delete OK
    test_account_update OK
    test_get_account OK
AdminAPITest
    test_admin_disabled OK
    test_admin_enabled OK
APITest
    test_exceptions_are_converted_to_faults OK
Test
    test_authorize_token OK
    test_authorize_user OK
    test_bad_token OK
    test_bad_user_bad_key OK
    test_bad_user_good_key OK
    test_no_user OK
    test_token_expiry OK
TestFunctional
    test_token_doesnotexist OK
    test_token_expiry OK
TestLimiter
    test_authorize_token OK
LimiterTest
    test_limiter_custom_max_limit OK
    test_limiter_limit_and_offset OK
    test_limiter_limit_medium OK
    test_limiter_limit_over_max OK
    test_limiter_limit_zero OK
    test_limiter_negative_limit OK
    test_limiter_negative_offset OK
    test_limiter_nothing OK
    test_limiter_offset_bad OK
    test_limiter_offset_blank OK
    test_limiter_offset_medium OK
    test_limiter_offset_over_max OK
    test_limiter_offset_zero OK
ActionExtensionTest
    test_extended_action OK
    test_invalid_action OK
    test_invalid_action_body OK
ExtensionControllerTest
    test_get_by_alias OK
    test_index OK
ExtensionManagerTest
    test_get_resources OK
ResourceExtensionTest
    test_get_resources OK
    test_get_resources_with_controller OK
    test_no_extension_present OK
ResponseExtensionTest
    test_get_resources_with_mgr OK
    test_get_resources_with_stub_mgr OK
TestFaults
    test_400_fault_json OK
    test_400_fault_xml ...

[lp:~vishvananda/nova/fix-describe-images]

933. By Vish Ishaya on 2011-04-07

merged trunk

934. By Vish Ishaya on 2011-04-07

fix tests from moving access check into update and delete

[Vish Ishaya (vishvananda)]

My bad, forgot to update tests on my last change. Should be working now.

On Apr 7, 2011, at 3:38 PM, OpenStack Hudson wrote:

> The proposal to merge lp:~vishvananda/nova/fix-describe-images into lp:nova has been updated.
>
> Status: Approved => Needs review
>
> For more details, see:
> https://code.launchpad.net/~vishvananda/nova/fix-describe-images/+merge/55617
> --
> https://code.launchpad.net/~vishvananda/nova/fix-describe-images/+merge/55617
> You are the owner of lp:~vishvananda/nova/fix-describe-images.

=== modified file 'bin/nova-manage'
--- bin/nova-manage	2011-04-04 20:17:04 +0000
+++ bin/nova-manage	2011-04-07 23:45:54 +0000
@@ -894,20 +894,17 @@
     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,
+                'container_format': container_format,
                 'disk_format': disk_format,
-                'container_format': container_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 @@
         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 @@
         """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 @@
         """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 @@
                    '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'):

=== modified file 'nova/api/ec2/cloud.py'
--- nova/api/ec2/cloud.py	2011-04-07 19:52:14 +0000
+++ nova/api/ec2/cloud.py	2011-04-07 23:45:54 +0000
@@ -154,7 +154,7 @@
         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 @@
         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 @@
             self.compute_api.update(context, instance_id=instance_id, **kwargs)
         return True
 
-    _type_prefix_map = {'machine': 'ami',
-                        'kernel': 'aki',
-                        'ramdisk': 'ari'}
-
-    def _image_ec2_id(self, image_id, image_type='machine'):
-        prefix = self._type_prefix_map[image_type]
-        template = prefix + '-%08x'
+    @staticmethod
+    def _image_type(image_type):
+        """Converts to a three letter image type.
+
+        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 @@
     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 @@
             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)

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-04-07 23:04:43 +0000
+++ nova/api/openstack/servers.py	2011-04-07 23:45:54 +0000
@@ -565,7 +565,7 @@
                 _("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:

=== modified file 'nova/image/fake.py'
--- nova/image/fake.py	2011-03-30 02:11:12 +0000
+++ nova/image/fake.py	2011-04-07 23:45:54 +0000
@@ -44,10 +44,10 @@
                  '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__()

=== modified file 'nova/image/glance.py'
--- nova/image/glance.py	2011-03-29 16:06:52 +0000
+++ nova/image/glance.py	2011-04-07 23:45:54 +0000
@@ -151,6 +151,8 @@
 
         :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 @@
 
         :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 @@
         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):

=== modified file 'nova/image/local.py'
--- nova/image/local.py	2011-03-23 05:50:53 +0000
+++ nova/image/local.py	2011-04-07 23:45:54 +0000
@@ -84,7 +84,10 @@
     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 @@
         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 @@
 
     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):

=== modified file 'nova/image/s3.py'
--- nova/image/s3.py	2011-03-30 00:23:09 +0000
+++ nova/image/s3.py	2011-04-07 23:45:54 +0000
@@ -46,6 +46,7 @@
 
 
 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 @@
         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 @@
             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 @@
         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',

=== modified file 'nova/image/service.py'
--- nova/image/service.py	2011-03-24 21:13:55 +0000
+++ nova/image/service.py	2011-04-07 23:45:54 +0000
@@ -136,6 +136,33 @@
         """
         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.

=== modified file 'nova/tests/api/openstack/test_image_metadata.py'
--- nova/tests/api/openstack/test_image_metadata.py	2011-03-25 13:28:36 +0000
+++ nova/tests/api/openstack/test_image_metadata.py	2011-04-07 23:45:54 +0000
@@ -45,7 +45,6 @@
         'is_public': True,
         'deleted_at': None,
         'properties': {
-            'type': 'ramdisk',
             'key1': 'value1',
             'key2': 'value2'
         },
@@ -62,7 +61,6 @@
         'is_public': True,
         'deleted_at': None,
         'properties': {
-            'type': 'ramdisk',
             'key1': 'value1',
             'key2': 'value2'
         },

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-04-07 23:04:43 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-04-07 23:45:54 +0000
@@ -1643,29 +1643,27 @@
 
     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)

=== modified file 'nova/tests/image/test_glance.py'
--- nova/tests/image/test_glance.py	2011-03-28 17:47:18 +0000
+++ nova/tests/image/test_glance.py	2011-04-07 23:45:54 +0000
@@ -209,17 +209,17 @@
         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):

=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2011-04-07 21:48:29 +0000
+++ nova/virt/libvirt_conn.py	2011-04-07 23:45:54 +0000
@@ -424,7 +424,6 @@
                     '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',