Merge lp:~yamahata/nova/boot-from-volume-1 into lp:~hudson-openstack/nova/trunk

[Vish Ishaya]

This change adds the basic boot-from-volume support to the image service.

Specifically following API will supports --block-device-mapping with volume/snapshot and root device name

- register image

- describe image

- create image(newly support)

At the moment swap and ephemeral aren't supported yet. They will be supported with the next step

Next step

- describe instance attribute with euca command

- get metadata for bundle volume

- swap/ephemeral device support

[Isaku Yamahata (yamahata)]

Now here is the unit tests(with some fixes).
So it's ready for merge now, I think.

The next step is to support the following.
- describe instance attribute
- get metadata for bundle volume
- swap/ephemeral device support.

On Wed, Jun 22, 2011 at 05:01:54AM -0000, Isaku Yamahata wrote:
> Isaku Yamahata has proposed merging lp:~yamahata/nova/boot-from-volume-1 into lp:nova with lp:~yamahata/nova/boot-from-volume-0 as a prerequisite.
>
> Requested reviews:
> Nova Core (nova-core)
>
> For more details, see:
> https://code.launchpad.net/~yamahata/nova/boot-from-volume-1/+merge/64825
>
> This is early review request before going further.
> If this direction is okay, I'll add unit tests and then move on to the next step.
>
> This change adds the basic boot-from-volume support to the image service.
> Specifically following API will supports --block-device-mapping with volume/snapshot and root device name
> - register image
> - describe image
> - create image(newly support)
>
> At the moment swap and ephemeral aren't supported. Are these wanted?
>
> NOTE
> - bundle volume is broken
>
> TODO
> - unit tests
>
> Next step
> - describe instance attribute with euca command
> - get metadata for bundle volume
> - swap/ephemeral device support(Is this wanted? or unnecessary?)
> --
> https://code.launchpad.net/~yamahata/nova/boot-from-volume-1/+merge/64825
> You are the owner of lp:~yamahata/nova/boot-from-volume-1.

> === modified file 'nova/api/ec2/__init__.py'
> --- nova/api/ec2/__init__.py 2011-06-15 16:46:24 +0000
> +++ nova/api/ec2/__init__.py 2011-06-22 04:55:48 +0000
> @@ -262,6 +262,8 @@
> 'TerminateInstances': ['projectmanager', 'sysadmin'],
> 'RebootInstances': ['projectmanager', 'sysadmin'],
> 'UpdateInstance': ['projectmanager', 'sysadmin'],
> + 'StartInstances': ['projectmanager', 'sysadmin'],
> + 'StopInstances': ['projectmanager', 'sysadmin'],
> 'DeleteVolume': ['projectmanager', 'sysadmin'],
> 'DescribeImages': ['all'],
> 'DeregisterImage': ['projectmanager', 'sysadmin'],
> @@ -269,6 +271,7 @@
> 'DescribeImageAttribute': ['all'],
> 'ModifyImageAttribute': ['projectmanager', 'sysadmin'],
> 'UpdateImage': ['projectmanager', 'sysadmin'],
> + 'CreateImage': ['projectmanager', 'sysadmin'],
> },
> 'AdminController': {
> # All actions have the same permission: ['none'] (the default)
> @@ -325,13 +328,13 @@
> except exception.VolumeNotFound as ex:
> LOG.info(_('VolumeNotFound raised: %s'), unicode(ex),
> context=context)
> - ec2_id = ec2utils.id_to_ec2_id(ex.volume_id, 'vol-%08x')
> + ec2_id = ec2utils.id_to_ec2_vol_id(ex.volume_id)
> message = _('Volume %s not found') % ec2_id
> return self._error(req, context, type(ex).__name__, message)
> except exception.SnapshotNotFound as ex:
> LOG.info(_('SnapshotNotFound raised: %s'), unicode(ex),
> context=context)
> - ...

[Sandy Walsh (sandy-walsh)]

Impressive branch. I don't have a set up for testing it in depth, so I can't verify correctness.

I would like to see mocked out unit tests for each new method/function. Many of the _ internal methods have no tests at all.

Minor things:
+379/380 ... commented out?
+405 ... potential black hole?

review: Needs Fixing

[Isaku Yamahata (yamahata)]

Thank you for review.

On Wed, Jun 22, 2011 at 06:04:33PM -0000, Sandy Walsh wrote:

> I would like to see mocked out unit tests for each new method/function. Many of the _ internal methods have no tests at all.

Now I added more unit tests for those methods/functions.
I think they covers what you meant.
  - nova/api/ec2/cloud.py
  _parse_block_device_mapping(), _format_block_device_mapping(),
  _format_mappings(), _format_instance_bdm()

  - nova/compute/api.py
  _update_image_block_device_mapping(), _update_block_device_mapping()

  - nova/volume/api.py
  create_snapshot(),create_snapshot_force()

> Minor things:
> +379/380 ... commented out?

Removed them

> +405 ... potential black hole?

Implemented timeout. I adopted 1 hour to timeout.
Although I'm not sure how long it should be, the length wouldn't matter
so much because timeout is just for safety.

thanks,
--
yamahata

[Sandy Walsh (sandy-walsh)]

Awesome ... I have no other immediate feedback. I'll leave it to others closer to the domain.

Nice work Yamahata!

review: Approve

[Vish Ishaya (vishvananda)]

excited to get this in!

review: Approve

[OpenStack Infra (hudson-openstack)]

No proposals found for merge of lp:~yamahata/nova/boot-from-volume-0 into lp:nova.

[Vish Ishaya (vishvananda)]

we seem to have lost the ability to merge branches that have an already merged prereq, so i'm rerequesting without the prereq.

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~yamahata/nova/boot-from-volume-1 into lp:nova failed. Below is the output from the failed tests.

ERROR

======================================================================
ERROR: <nose.suite.ContextSuite context=nova.tests>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.6/nose/suite.py", line 183, in run
    self.setUp()
  File "/usr/lib/pymodules/python2.6/nose/suite.py", line 264, in setUp
    self.setupContext(ancestor)
  File "/usr/lib/pymodules/python2.6/nose/suite.py", line 287, in setupContext
    try_run(context, names)
  File "/usr/lib/pymodules/python2.6/nose/util.py", line 487, in try_run
    return func()
  File "/tmp/tmp7ronKQ/nova/tests/__init__.py", line 54, in setup
    migration.db_sync()
  File "/tmp/tmp7ronKQ/nova/db/migration.py", line 35, in db_sync
    return IMPL.db_sync(version=version)
  File "/tmp/tmp7ronKQ/nova/db/sqlalchemy/migration.py", line 41, in db_sync
    db_version()
  File "/tmp/tmp7ronKQ/nova/db/sqlalchemy/migration.py", line 49, in db_version
    return versioning_api.db_version(FLAGS.sql_connection, repo_path)
  File "<string>", line 2, in db_version
  File "/usr/lib/pymodules/python2.6/migrate/versioning/util/__init__.py", line 160, in with_engine
    return f(*a, **kw)
  File "/usr/lib/pymodules/python2.6/migrate/versioning/api.py", line 147, in db_version
    schema = ControlledSchema(engine, repository)
  File "/usr/lib/pymodules/python2.6/migrate/versioning/schema.py", line 26, in __init__
    repository = Repository(repository)
  File "/usr/lib/pymodules/python2.6/migrate/versioning/repository.py", line 80, in __init__
    self._versions))
  File "/usr/lib/pymodules/python2.6/migrate/versioning/version.py", line 83, in __init__
    self.versions[VerNum(num)] = Version(num, path, files)
  File "/usr/lib/pymodules/python2.6/migrate/versioning/version.py", line 153, in __init__
    self.add_script(os.path.join(path, script))
  File "/usr/lib/pymodules/python2.6/migrate/versioning/version.py", line 174, in add_script
    self._add_script_py(path)
  File "/usr/lib/pymodules/python2.6/migrate/versioning/version.py", line 197, in _add_script_py
    'per version, but you have: %s and %s' % (self.python, path))
ScriptError: You can only have one Python script per version, but you have: /tmp/tmp7ronKQ/nova/db/sqlalchemy/migrate_repo/versions/027_add_root_device_name.py and /tmp/tmp7ronKQ/nova/db/sqlalchemy/migrate_repo/versions/027_add_provider_firewall_rules.py
-------------------- >> begin captured logging << --------------------
2011-06-25 02:25:11,546 WARNING nova.virt.libvirt.firewall [-] Libvirt module could not be loaded. NWFilterFirewall will not work correctly.
2011-06-25 02:25:12,239 DEBUG nova.utils [-] backend <module 'nova.db.sqlalchemy.migration' from '/tmp/tmp7ronKQ/nova/db/sqlalchemy/migration.py'> from (pid=22373) __get_backend /tmp/tmp7ronKQ/nova/utils.py:406
2011-06-25 02:25:12,240 DEBUG migrate.versioning.util [-] Constructing engine from (pid=22373) construct_engine /usr/lib/pymodules/python2.6/migrate/versioning/util/__init__.py:138
2011-06-25 02:25:12,244 DEBUG mi...

[Vish Ishaya (vishvananda)]

looks like the migration number needs to be bumped by a few...

review: Needs Fixing

[Isaku Yamahata (yamahata)]

Thank you for review. I fixed it and confirmed that unit tests passed.

On Sat, Jun 25, 2011 at 02:38:23AM -0000, Vish Ishaya wrote:
> Review: Needs Fixing
> looks like the migration number needs to be bumped by a few...
> --
> https://code.launchpad.net/~yamahata/nova/boot-from-volume-1/+merge/65850
> You are the owner of lp:~yamahata/nova/boot-from-volume-1.
>

--
yamahata

[Brian Waldon (bcwaldon)]

Migration needs to be updated yet again. We're moving fast!

[Mark Washenberger (markwash)]

Setting to WIP until the versions are updated. Sorry for the difficulties here. I'll check back in this weekend to see if we can move this merge along while things are a bit more quiet.

[Isaku Yamahata (yamahata)]

I resolved the conflict merging nova trunk.

On Fri, Jul 01, 2011 at 04:01:26PM -0000, Mark Washenberger wrote:
> Setting to WIP until the versions are updated. Sorry for the difficulties here. I'll check back in this weekend to see if we can move this merge along while things are a bit more quiet.
> --
> https://code.launchpad.net/~yamahata/nova/boot-from-volume-1/+merge/65850
> You are the owner of lp:~yamahata/nova/boot-from-volume-1.
>

--
yamahata

[Sandy Walsh (sandy-walsh)]

works for me

review: Approve

[Brian Waldon (bcwaldon)]

Seeing some smoketest failures, specifically test_004_can_access_metadata_over_public_ip (smoketests.test_netadmin.SecurityGroupTests)

review: Needs Fixing

[Brian Waldon (bcwaldon)]

> Seeing some smoketest failures, specifically
> test_004_can_access_metadata_over_public_ip
> (smoketests.test_netadmin.SecurityGroupTests)

Hmm, tests passing now. I think it was a problem on my end. Ignore me!

review: Abstain

[Vish Ishaya (vishvananda)]

looks good now

review: Approve

=== modified file 'nova/api/ec2/__init__.py'
--- nova/api/ec2/__init__.py	2011-06-24 12:01:51 +0000
+++ nova/api/ec2/__init__.py	2011-07-08 09:39:31 +0000
@@ -262,6 +262,8 @@
                 'TerminateInstances': ['projectmanager', 'sysadmin'],
                 'RebootInstances': ['projectmanager', 'sysadmin'],
                 'UpdateInstance': ['projectmanager', 'sysadmin'],
+                'StartInstances': ['projectmanager', 'sysadmin'],
+                'StopInstances': ['projectmanager', 'sysadmin'],
                 'DeleteVolume': ['projectmanager', 'sysadmin'],
                 'DescribeImages': ['all'],
                 'DeregisterImage': ['projectmanager', 'sysadmin'],
@@ -269,6 +271,7 @@
                 'DescribeImageAttribute': ['all'],
                 'ModifyImageAttribute': ['projectmanager', 'sysadmin'],
                 'UpdateImage': ['projectmanager', 'sysadmin'],
+                'CreateImage': ['projectmanager', 'sysadmin'],
             },
             'AdminController': {
                 # All actions have the same permission: ['none'] (the default)
@@ -325,13 +328,13 @@
         except exception.VolumeNotFound as ex:
             LOG.info(_('VolumeNotFound raised: %s'), unicode(ex),
                      context=context)
-            ec2_id = ec2utils.id_to_ec2_id(ex.volume_id, 'vol-%08x')
+            ec2_id = ec2utils.id_to_ec2_vol_id(ex.volume_id)
             message = _('Volume %s not found') % ec2_id
             return self._error(req, context, type(ex).__name__, message)
         except exception.SnapshotNotFound as ex:
             LOG.info(_('SnapshotNotFound raised: %s'), unicode(ex),
                      context=context)
-            ec2_id = ec2utils.id_to_ec2_id(ex.snapshot_id, 'snap-%08x')
+            ec2_id = ec2utils.id_to_ec2_snap_id(ex.snapshot_id)
             message = _('Snapshot %s not found') % ec2_id
             return self._error(req, context, type(ex).__name__, message)
         except exception.NotFound as ex:
 
=== modified file 'nova/api/ec2/cloud.py'
--- nova/api/ec2/cloud.py	2011-07-01 15:47:33 +0000
+++ nova/api/ec2/cloud.py	2011-07-08 09:39:31 +0000
@@ -27,6 +27,7 @@
 import os
 import urllib
 import tempfile
+import time
 import shutil
 
 from nova import compute
@@ -75,6 +76,95 @@
     return {'private_key': private_key, 'fingerprint': fingerprint}
 
 
+# TODO(yamahata): hypervisor dependent default device name
+_DEFAULT_ROOT_DEVICE_NAME = '/dev/sda1'
+
+
+def _parse_block_device_mapping(bdm):
+    """Parse BlockDeviceMappingItemType into flat hash
+    BlockDevicedMapping.<N>.DeviceName
+    BlockDevicedMapping.<N>.Ebs.SnapshotId
+    BlockDevicedMapping.<N>.Ebs.VolumeSize
+    BlockDevicedMapping.<N>.Ebs.DeleteOnTermination
+    BlockDevicedMapping.<N>.Ebs.NoDevice
+    BlockDevicedMapping.<N>.VirtualName
+    => remove .Ebs and allow volume id in SnapshotId
+    """
+    ebs = bdm.pop('ebs', None)
+    if ebs:
+        ec2_id = ebs.pop('snapshot_id', None)
+        if ec2_id:
+            id = ec2utils.ec2_id_to_id(ec2_id)
+            if ec2_id.startswith('snap-'):
+                bdm['snapshot_id'] = id
+            elif ec2_id.startswith('vol-'):
+                bdm['volume_id'] = id
+            ebs.setdefault('delete_on_termination', True)
+        bdm.update(ebs)
+    return bdm
+
+
+def _properties_get_mappings(properties):
+    return ec2utils.mappings_prepend_dev(properties.get('mappings', []))
+
+
+def _format_block_device_mapping(bdm):
+    """Contruct BlockDeviceMappingItemType
+    {'device_name': '...', 'snapshot_id': , ...}
+    => BlockDeviceMappingItemType
+    """
+    keys = (('deviceName', 'device_name'),
+             ('virtualName', 'virtual_name'))
+    item = {}
+    for name, k in keys:
+        if k in bdm:
+            item[name] = bdm[k]
+    if bdm.get('no_device'):
+        item['noDevice'] = True
+    if ('snapshot_id' in bdm) or ('volume_id' in bdm):
+        ebs_keys = (('snapshotId', 'snapshot_id'),
+                    ('snapshotId', 'volume_id'),        # snapshotId is abused
+                    ('volumeSize', 'volume_size'),
+                    ('deleteOnTermination', 'delete_on_termination'))
+        ebs = {}
+        for name, k in ebs_keys:
+            if k in bdm:
+                if k == 'snapshot_id':
+                    ebs[name] = ec2utils.id_to_ec2_snap_id(bdm[k])
+                elif k == 'volume_id':
+                    ebs[name] = ec2utils.id_to_ec2_vol_id(bdm[k])
+                else:
+                    ebs[name] = bdm[k]
+        assert 'snapshotId' in ebs
+        item['ebs'] = ebs
+    return item
+
+
+def _format_mappings(properties, result):
+    """Format multiple BlockDeviceMappingItemType"""
+    mappings = [{'virtualName': m['virtual'], 'deviceName': m['device']}
+                for m in _properties_get_mappings(properties)
+                if (m['virtual'] == 'swap' or
+                    m['virtual'].startswith('ephemeral'))]
+
+    block_device_mapping = [_format_block_device_mapping(bdm) for bdm in
+                            properties.get('block_device_mapping', [])]
+
+    # NOTE(yamahata): overwrite mappings with block_device_mapping
+    for bdm in block_device_mapping:
+        for i in range(len(mappings)):
+            if bdm['deviceName'] == mappings[i]['deviceName']:
+                del mappings[i]
+                break
+        mappings.append(bdm)
+
+    # NOTE(yamahata): trim ebs.no_device == true. Is this necessary?
+    mappings = [bdm for bdm in mappings if not (bdm.get('noDevice', False))]
+
+    if mappings:
+        result['blockDeviceMapping'] = mappings
+
+
 class CloudController(object):
     """ CloudController provides the critical dispatch between
  inbound API calls through the endpoint and messages
@@ -176,7 +266,7 @@
                     # TODO(vish): replace with real data
                     'ami': 'sda1',
                     'ephemeral0': 'sda2',
-                    'root': '/dev/sda1',
+                    'root': _DEFAULT_ROOT_DEVICE_NAME,
                     'swap': 'sda3'},
                 'hostname': hostname,
                 'instance-action': 'none',
@@ -304,9 +394,8 @@
 
     def _format_snapshot(self, context, snapshot):
         s = {}
-        s['snapshotId'] = ec2utils.id_to_ec2_id(snapshot['id'], 'snap-%08x')
-        s['volumeId'] = ec2utils.id_to_ec2_id(snapshot['volume_id'],
-                                              'vol-%08x')
+        s['snapshotId'] = ec2utils.id_to_ec2_snap_id(snapshot['id'])
+        s['volumeId'] = ec2utils.id_to_ec2_vol_id(snapshot['volume_id'])
         s['status'] = snapshot['status']
         s['startTime'] = snapshot['created_at']
         s['progress'] = snapshot['progress']
@@ -683,7 +772,7 @@
             instance_data = '%s[%s]' % (instance_ec2_id,
                                         volume['instance']['host'])
         v = {}
-        v['volumeId'] = ec2utils.id_to_ec2_id(volume['id'], 'vol-%08x')
+        v['volumeId'] = ec2utils.id_to_ec2_vol_id(volume['id'])
         v['status'] = volume['status']
         v['size'] = volume['size']
         v['availabilityZone'] = volume['availability_zone']
@@ -705,8 +794,7 @@
         else:
             v['attachmentSet'] = [{}]
         if volume.get('snapshot_id') != None:
-            v['snapshotId'] = ec2utils.id_to_ec2_id(volume['snapshot_id'],
-                                                    'snap-%08x')
+            v['snapshotId'] = ec2utils.id_to_ec2_snap_id(volume['snapshot_id'])
         else:
             v['snapshotId'] = None
 
@@ -769,7 +857,7 @@
                 'instanceId': ec2utils.id_to_ec2_id(instance_id),
                 'requestId': context.request_id,
                 'status': volume['attach_status'],
-                'volumeId': ec2utils.id_to_ec2_id(volume_id, 'vol-%08x')}
+                'volumeId': ec2utils.id_to_ec2_vol_id(volume_id)}
 
     def detach_volume(self, context, volume_id, **kwargs):
         volume_id = ec2utils.ec2_id_to_id(volume_id)
@@ -781,7 +869,7 @@
                 'instanceId': ec2utils.id_to_ec2_id(instance['id']),
                 'requestId': context.request_id,
                 'status': volume['attach_status'],
-                'volumeId': ec2utils.id_to_ec2_id(volume_id, 'vol-%08x')}
+                'volumeId': ec2utils.id_to_ec2_vol_id(volume_id)}
 
     def _convert_to_set(self, lst, label):
         if lst is None or lst == []:
@@ -805,6 +893,37 @@
         assert len(i) == 1
         return i[0]
 
+    def _format_instance_bdm(self, context, instance_id, root_device_name,
+                             result):
+        """Format InstanceBlockDeviceMappingResponseItemType"""
+        root_device_type = 'instance-store'
+        mapping = []
+        for bdm in db.block_device_mapping_get_all_by_instance(context,
+                                                               instance_id):
+            volume_id = bdm['volume_id']
+            if (volume_id is None or bdm['no_device']):
+                continue
+
+            if (bdm['device_name'] == root_device_name and
+                (bdm['snapshot_id'] or bdm['volume_id'])):
+                assert not bdm['virtual_name']
+                root_device_type = 'ebs'
+
+            vol = self.volume_api.get(context, volume_id=volume_id)
+            LOG.debug(_("vol = %s\n"), vol)
+            # TODO(yamahata): volume attach time
+            ebs = {'volumeId': volume_id,
+                   'deleteOnTermination': bdm['delete_on_termination'],
+                   'attachTime': vol['attach_time'] or '-',
+                   'status': vol['status'], }
+            res = {'deviceName': bdm['device_name'],
+                   'ebs': ebs, }
+            mapping.append(res)
+
+        if mapping:
+            result['blockDeviceMapping'] = mapping
+        result['rootDeviceType'] = root_device_type
+
     def _format_instances(self, context, instance_id=None, **kwargs):
         # TODO(termie): this method is poorly named as its name does not imply
         #               that it will be making a variety of database calls
@@ -866,6 +985,10 @@
             i['amiLaunchIndex'] = instance['launch_index']
             i['displayName'] = instance['display_name']
             i['displayDescription'] = instance['display_description']
+            i['rootDeviceName'] = (instance.get('root_device_name') or
+                                   _DEFAULT_ROOT_DEVICE_NAME)
+            self._format_instance_bdm(context, instance_id,
+                                      i['rootDeviceName'], i)
             host = instance['host']
             zone = self._get_availability_zone_by_host(context, host)
             i['placement'] = {'availabilityZone': zone}
@@ -953,23 +1076,7 @@
             ramdisk = self._get_image(context, kwargs['ramdisk_id'])
             kwargs['ramdisk_id'] = ramdisk['id']
         for bdm in kwargs.get('block_device_mapping', []):
-            # NOTE(yamahata)
-            # BlockDevicedMapping.<N>.DeviceName
-            # BlockDevicedMapping.<N>.Ebs.SnapshotId
-            # BlockDevicedMapping.<N>.Ebs.VolumeSize
-            # BlockDevicedMapping.<N>.Ebs.DeleteOnTermination
-            # BlockDevicedMapping.<N>.VirtualName
-            # => remove .Ebs and allow volume id in SnapshotId
-            ebs = bdm.pop('ebs', None)
-            if ebs:
-                ec2_id = ebs.pop('snapshot_id')
-                id = ec2utils.ec2_id_to_id(ec2_id)
-                if ec2_id.startswith('snap-'):
-                    bdm['snapshot_id'] = id
-                elif ec2_id.startswith('vol-'):
-                    bdm['volume_id'] = id
-                ebs.setdefault('delete_on_termination', True)
-                bdm.update(ebs)
+            _parse_block_device_mapping(bdm)
 
         image = self._get_image(context, kwargs['image_id'])
 
@@ -1124,6 +1231,20 @@
         i['imageType'] = display_mapping.get(image_type)
         i['isPublic'] = image.get('is_public') == True
         i['architecture'] = image['properties'].get('architecture')
+
+        properties = image['properties']
+        root_device_name = ec2utils.properties_root_device_name(properties)
+        root_device_type = 'instance-store'
+        for bdm in properties.get('block_device_mapping', []):
+            if (bdm.get('device_name') == root_device_name and
+                ('snapshot_id' in bdm or 'volume_id' in bdm) and
+                not bdm.get('no_device')):
+                root_device_type = 'ebs'
+        i['rootDeviceName'] = (root_device_name or _DEFAULT_ROOT_DEVICE_NAME)
+        i['rootDeviceType'] = root_device_type
+
+        _format_mappings(properties, i)
+
         return i
 
     def describe_images(self, context, image_id=None, **kwargs):
@@ -1148,30 +1269,64 @@
         self.image_service.delete(context, internal_id)
         return {'imageId': image_id}
 
+    def _register_image(self, context, metadata):
+        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_type)
+        return image_id
+
     def register_image(self, context, image_location=None, **kwargs):
         if image_location is None and 'name' in kwargs:
             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_type)
+
+        if 'root_device_name' in kwargs:
+            metadata['properties']['root_device_name'] = \
+            kwargs.get('root_device_name')
+
+        mappings = [_parse_block_device_mapping(bdm) for bdm in
+                    kwargs.get('block_device_mapping', [])]
+        if mappings:
+            metadata['properties']['block_device_mapping'] = mappings
+
+        image_id = self._register_image(context, metadata)
         msg = _("Registered image %(image_location)s with"
                 " id %(image_id)s") % locals()
         LOG.audit(msg, context=context)
         return {'imageId': image_id}
 
     def describe_image_attribute(self, context, image_id, attribute, **kwargs):
-        if attribute != 'launchPermission':
+        def _block_device_mapping_attribute(image, result):
+            _format_mappings(image['properties'], result)
+
+        def _launch_permission_attribute(image, result):
+            result['launchPermission'] = []
+            if image['is_public']:
+                result['launchPermission'].append({'group': 'all'})
+
+        def _root_device_name_attribute(image, result):
+            result['rootDeviceName'] = \
+                    ec2utils.properties_root_device_name(image['properties'])
+            if result['rootDeviceName'] is None:
+                result['rootDeviceName'] = _DEFAULT_ROOT_DEVICE_NAME
+
+        supported_attributes = {
+            'blockDeviceMapping': _block_device_mapping_attribute,
+            'launchPermission': _launch_permission_attribute,
+            'rootDeviceName': _root_device_name_attribute,
+            }
+
+        fn = supported_attributes.get(attribute)
+        if fn is None:
             raise exception.ApiError(_('attribute not supported: %s')
                                      % attribute)
         try:
             image = self._get_image(context, image_id)
         except exception.NotFound:
             raise exception.ImageNotFound(image_id=image_id)
-        result = {'imageId': image_id, 'launchPermission': []}
-        if image['is_public']:
-            result['launchPermission'].append({'group': 'all'})
+
+        result = {'imageId': image_id}
+        fn(image, result)
         return result
 
     def modify_image_attribute(self, context, image_id, attribute,
@@ -1202,3 +1357,109 @@
         internal_id = ec2utils.ec2_id_to_id(image_id)
         result = self.image_service.update(context, internal_id, dict(kwargs))
         return result
+
+    # TODO(yamahata): race condition
+    # At the moment there is no way to prevent others from
+    # manipulating instances/volumes/snapshots.
+    # As other code doesn't take it into consideration, here we don't
+    # care of it for now. Ostrich algorithm
+    def create_image(self, context, instance_id, **kwargs):
+        # NOTE(yamahata): name/description are ignored by register_image(),
+        #                 do so here
+        no_reboot = kwargs.get('no_reboot', False)
+
+        ec2_instance_id = instance_id
+        instance_id = ec2utils.ec2_id_to_id(ec2_instance_id)
+        instance = self.compute_api.get(context, instance_id)
+
+        # stop the instance if necessary
+        restart_instance = False
+        if not no_reboot:
+            state_description = instance['state_description']
+
+            # if the instance is in subtle state, refuse to proceed.
+            if state_description not in ('running', 'stopping', 'stopped'):
+                raise exception.InstanceNotRunning(instance_id=ec2_instance_id)
+
+            if state_description == 'running':
+                restart_instance = True
+                self.compute_api.stop(context, instance_id=instance_id)
+
+            # wait instance for really stopped
+            start_time = time.time()
+            while state_description != 'stopped':
+                time.sleep(1)
+                instance = self.compute_api.get(context, instance_id)
+                state_description = instance['state_description']
+                # NOTE(yamahata): timeout and error. 1 hour for now for safety.
+                #                 Is it too short/long?
+                #                 Or is there any better way?
+                timeout = 1 * 60 * 60 * 60
+                if time.time() > start_time + timeout:
+                    raise exception.ApiError(
+                        _('Couldn\'t stop instance with in %d sec') % timeout)
+
+        src_image = self._get_image(context, instance['image_ref'])
+        properties = src_image['properties']
+        if instance['root_device_name']:
+            properties['root_device_name'] = instance['root_device_name']
+
+        mapping = []
+        bdms = db.block_device_mapping_get_all_by_instance(context,
+                                                           instance_id)
+        for bdm in bdms:
+            if bdm.no_device:
+                continue
+            m = {}
+            for attr in ('device_name', 'snapshot_id', 'volume_id',
+                         'volume_size', 'delete_on_termination', 'no_device',
+                         'virtual_name'):
+                val = getattr(bdm, attr)
+                if val is not None:
+                    m[attr] = val
+
+            volume_id = m.get('volume_id')
+            if m.get('snapshot_id') and volume_id:
+                # create snapshot based on volume_id
+                vol = self.volume_api.get(context, volume_id=volume_id)
+                # NOTE(yamahata): Should we wait for snapshot creation?
+                #                 Linux LVM snapshot creation completes in
+                #                 short time, it doesn't matter for now.
+                snapshot = self.volume_api.create_snapshot_force(
+                    context, volume_id=volume_id, name=vol['display_name'],
+                    description=vol['display_description'])
+                m['snapshot_id'] = snapshot['id']
+                del m['volume_id']
+
+            if m:
+                mapping.append(m)
+
+        for m in _properties_get_mappings(properties):
+            virtual_name = m['virtual']
+            if virtual_name in ('ami', 'root'):
+                continue
+
+            assert (virtual_name == 'swap' or
+                    virtual_name.startswith('ephemeral'))
+            device_name = m['device']
+            if device_name in [b['device_name'] for b in mapping
+                               if not b.get('no_device', False)]:
+                continue
+
+            # NOTE(yamahata): swap and ephemeral devices are specified in
+            #                 AMI, but disabled for this instance by user.
+            #                 So disable those device by no_device.
+            mapping.append({'device_name': device_name, 'no_device': True})
+
+        if mapping:
+            properties['block_device_mapping'] = mapping
+
+        for attr in ('status', 'location', 'id'):
+            src_image.pop(attr, None)
+
+        image_id = self._register_image(context, src_image)
+
+        if restart_instance:
+            self.compute_api.start(context, instance_id=instance_id)
+
+        return {'imageId': image_id}
 
=== modified file 'nova/api/ec2/ec2utils.py'
--- nova/api/ec2/ec2utils.py	2011-06-24 12:01:51 +0000
+++ nova/api/ec2/ec2utils.py	2011-07-08 09:39:31 +0000
@@ -34,6 +34,17 @@
     return template % instance_id
 
 
+def id_to_ec2_snap_id(instance_id):
+    """Convert an snapshot ID (int) to an ec2 snapshot ID
+    (snap-[base 16 number])"""
+    return id_to_ec2_id(instance_id, 'snap-%08x')
+
+
+def id_to_ec2_vol_id(instance_id):
+    """Convert an volume ID (int) to an ec2 volume ID (vol-[base 16 number])"""
+    return id_to_ec2_id(instance_id, 'vol-%08x')
+
+
 _c2u = re.compile('(((?<=[a-z])[A-Z])|([A-Z](?![A-Z]|$)))')
 
 
@@ -124,3 +135,32 @@
                 args[key] = value
 
     return args
+
+
+def properties_root_device_name(properties):
+    """get root device name from image meta data.
+    If it isn't specified, return None.
+    """
+    root_device_name = None
+
+    # NOTE(yamahata): see image_service.s3.s3create()
+    for bdm in properties.get('mappings', []):
+        if bdm['virtual'] == 'root':
+            root_device_name = bdm['device']
+
+    # NOTE(yamahata): register_image's command line can override
+    #                 <machine>.manifest.xml
+    if 'root_device_name' in properties:
+        root_device_name = properties['root_device_name']
+
+    return root_device_name
+
+
+def mappings_prepend_dev(mappings):
+    """Prepend '/dev/' to 'device' entry of swap/ephemeral virtual type"""
+    for m in mappings:
+        virtual = m['virtual']
+        if ((virtual == 'swap' or virtual.startswith('ephemeral')) and
+            (not m['device'].startswith('/'))):
+            m['device'] = '/dev/' + m['device']
+    return mappings
 
=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-07-01 19:44:10 +0000
+++ nova/compute/api.py	2011-07-08 09:39:31 +0000
@@ -32,6 +32,7 @@
 from nova import rpc
 from nova import utils
 from nova import volume
+from nova.api.ec2 import ec2utils
 from nova.compute import instance_types
 from nova.compute import power_state
 from nova.compute.utils import terminate_volumes
@@ -217,6 +218,9 @@
         if reservation_id is None:
             reservation_id = utils.generate_uid('r')
 
+        root_device_name = ec2utils.properties_root_device_name(
+            image['properties'])
+
         base_options = {
             'reservation_id': reservation_id,
             'image_ref': image_href,
@@ -241,11 +245,61 @@
             'availability_zone': availability_zone,
             'os_type': os_type,
             'architecture': architecture,
-            'vm_mode': vm_mode}
-
-        return (num_instances, base_options)
-
-    def create_db_entry_for_new_instance(self, context, base_options,
+            'vm_mode': vm_mode,
+            'root_device_name': root_device_name}
+
+        return (num_instances, base_options, image)
+
+    def _update_image_block_device_mapping(self, elevated_context, instance_id,
+                                           mappings):
+        """tell vm driver to create ephemeral/swap device at boot time by
+        updating BlockDeviceMapping
+        """
+        for bdm in ec2utils.mappings_prepend_dev(mappings):
+            LOG.debug(_("bdm %s"), bdm)
+
+            virtual_name = bdm['virtual']
+            if virtual_name == 'ami' or virtual_name == 'root':
+                continue
+
+            assert (virtual_name == 'swap' or
+                    virtual_name.startswith('ephemeral'))
+            values = {
+                'instance_id': instance_id,
+                'device_name': bdm['device'],
+                'virtual_name': virtual_name, }
+            self.db.block_device_mapping_update_or_create(elevated_context,
+                                                          values)
+
+    def _update_block_device_mapping(self, elevated_context, instance_id,
+                                     block_device_mapping):
+        """tell vm driver to attach volume at boot time by updating
+        BlockDeviceMapping
+        """
+        for bdm in block_device_mapping:
+            LOG.debug(_('bdm %s'), bdm)
+            assert 'device_name' in bdm
+
+            values = {'instance_id': instance_id}
+            for key in ('device_name', 'delete_on_termination', 'virtual_name',
+                        'snapshot_id', 'volume_id', 'volume_size',
+                        'no_device'):
+                values[key] = bdm.get(key)
+
+            # NOTE(yamahata): NoDevice eliminates devices defined in image
+            #                 files by command line option.
+            #                 (--block-device-mapping)
+            if bdm.get('virtual_name') == 'NoDevice':
+                values['no_device'] = True
+                for k in ('delete_on_termination', 'volume_id',
+                          'snapshot_id', 'volume_id', 'volume_size',
+                          'virtual_name'):
+                    values[k] = None
+
+            self.db.block_device_mapping_update_or_create(elevated_context,
+                                                          values)
+
+    def create_db_entry_for_new_instance(self, context, image, base_options,
              security_group, block_device_mapping, num=1):
         """Create an entry in the DB for this new instance,
         including any related table updates (such as security group,
@@ -278,23 +332,14 @@
                                                 instance_id,
                                                 security_group_id)
 
-        block_device_mapping = block_device_mapping or []
-        # NOTE(yamahata)
-        # tell vm driver to attach volume at boot time by updating
-        # BlockDeviceMapping
-        for bdm in block_device_mapping:
-            LOG.debug(_('bdm %s'), bdm)
-            assert 'device_name' in bdm
-            values = {
-                'instance_id': instance_id,
-                'device_name': bdm['device_name'],
-                'delete_on_termination': bdm.get('delete_on_termination'),
-                'virtual_name': bdm.get('virtual_name'),
-                'snapshot_id': bdm.get('snapshot_id'),
-                'volume_id': bdm.get('volume_id'),
-                'volume_size': bdm.get('volume_size'),
-                'no_device': bdm.get('no_device')}
-            self.db.block_device_mapping_create(elevated, values)
+        # BlockDeviceMapping table
+        self._update_image_block_device_mapping(elevated, instance_id,
+            image['properties'].get('mappings', []))
+        self._update_block_device_mapping(elevated, instance_id,
+            image['properties'].get('block_device_mapping', []))
+        # override via command line option
+        self._update_block_device_mapping(elevated, instance_id,
+                                          block_device_mapping)
 
         # Set sane defaults if not specified
         updates = {}
@@ -356,7 +401,7 @@
         """Provision the instances by passing the whole request to
         the Scheduler for execution. Returns a Reservation ID
         related to the creation of all of these instances."""
-        num_instances, base_options = self._check_create_parameters(
+        num_instances, base_options, image = self._check_create_parameters(
                                context, instance_type,
                                image_href, kernel_id, ramdisk_id,
                                min_count, max_count,
@@ -394,7 +439,7 @@
         Returns a list of instance dicts.
         """
 
-        num_instances, base_options = self._check_create_parameters(
+        num_instances, base_options, image = self._check_create_parameters(
                                context, instance_type,
                                image_href, kernel_id, ramdisk_id,
                                min_count, max_count,
@@ -404,10 +449,11 @@
                                injected_files, admin_password, zone_blob,
                                reservation_id)
 
+        block_device_mapping = block_device_mapping or []
         instances = []
         LOG.debug(_("Going to run %s instances..."), num_instances)
         for num in range(num_instances):
-            instance = self.create_db_entry_for_new_instance(context,
+            instance = self.create_db_entry_for_new_instance(context, image,
                                     base_options, security_group,
                                     block_device_mapping, num=num)
             instances.append(instance)
 
=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-07-01 14:26:05 +0000
+++ nova/compute/manager.py	2011-07-08 09:39:31 +0000
@@ -220,6 +220,17 @@
         for bdm in self.db.block_device_mapping_get_all_by_instance(
             context, instance_id):
             LOG.debug(_("setting up bdm %s"), bdm)
+
+            if bdm['no_device']:
+                continue
+            if bdm['virtual_name']:
+                # TODO(yamahata):
+                # block devices for swap and ephemeralN will be
+                # created by virt driver locally in compute node.
+                assert (bdm['virtual_name'] == 'swap' or
+                        bdm['virtual_name'].startswith('ephemeral'))
+                continue
+
             if ((bdm['snapshot_id'] is not None) and
                 (bdm['volume_id'] is None)):
                 # TODO(yamahata): default name and description
@@ -252,15 +263,6 @@
                 block_device_mapping.append({'device_path': dev_path,
                                              'mount_device':
                                              bdm['device_name']})
-            elif bdm['virtual_name'] is not None:
-                # TODO(yamahata): ephemeral/swap device support
-                LOG.debug(_('block_device_mapping: '
-                            'ephemeral device is not supported yet'))
-            else:
-                # TODO(yamahata): NoDevice support
-                assert bdm['no_device']
-                LOG.debug(_('block_device_mapping: '
-                            'no device is not supported yet'))
 
         return block_device_mapping
 
 
=== modified file 'nova/db/api.py'
--- nova/db/api.py	2011-06-30 19:20:59 +0000
+++ nova/db/api.py	2011-07-08 09:39:31 +0000
@@ -989,10 +989,16 @@
 
 
 def block_device_mapping_update(context, bdm_id, values):
-    """Create an entry of block device mapping"""
+    """Update an entry of block device mapping"""
     return IMPL.block_device_mapping_update(context, bdm_id, values)
 
 
+def block_device_mapping_update_or_create(context, values):
+    """Update an entry of block device mapping.
+    If not existed, create a new entry"""
+    return IMPL.block_device_mapping_update_or_create(context, values)
+
+
 def block_device_mapping_get_all_by_instance(context, instance_id):
     """Get all block device mapping belonging to a instance"""
     return IMPL.block_device_mapping_get_all_by_instance(context, instance_id)
 
=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-07-01 15:07:08 +0000
+++ nova/db/sqlalchemy/api.py	2011-07-08 09:39:31 +0000
@@ -2208,6 +2208,23 @@
 
 
 @require_context
+def block_device_mapping_update_or_create(context, values):
+    session = get_session()
+    with session.begin():
+        result = session.query(models.BlockDeviceMapping).\
+                 filter_by(instance_id=values['instance_id']).\
+                 filter_by(device_name=values['device_name']).\
+                 filter_by(deleted=False).\
+                 first()
+        if not result:
+            bdm_ref = models.BlockDeviceMapping()
+            bdm_ref.update(values)
+            bdm_ref.save(session=session)
+        else:
+            result.update(values)
+
+
+@require_context
 def block_device_mapping_get_all_by_instance(context, instance_id):
     session = get_session()
     result = session.query(models.BlockDeviceMapping).\
 
=== added file 'nova/db/sqlalchemy/migrate_repo/versions/032_add_root_device_name.py'
--- nova/db/sqlalchemy/migrate_repo/versions/032_add_root_device_name.py	1970-01-01 00:00:00 +0000
+++ nova/db/sqlalchemy/migrate_repo/versions/032_add_root_device_name.py	2011-07-08 09:39:31 +0000
@@ -0,0 +1,47 @@
+# Copyright 2011 OpenStack LLC.
+# Copyright 2011 Isaku Yamahata
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from sqlalchemy import Column, Integer, MetaData, Table, String
+
+meta = MetaData()
+
+
+# Just for the ForeignKey and column creation to succeed, these are not the
+# actual definitions of instances or services.
+instances = Table('instances', meta,
+        Column('id', Integer(), primary_key=True, nullable=False),
+        )
+
+#
+# New Column
+#
+root_device_name = Column(
+    'root_device_name',
+    String(length=255, convert_unicode=False, assert_unicode=None,
+           unicode_error=None, _warn_on_bytestring=False),
+    nullable=True)
+
+
+def upgrade(migrate_engine):
+    # Upgrade operations go here. Don't create your own engine;
+    # bind migrate_engine to your metadata
+    meta.bind = migrate_engine
+    instances.create_column(root_device_name)
+
+
+def downgrade(migrate_engine):
+    # Operations to reverse the above upgrade go here.
+    meta.bind = migrate_engine
+    instances.drop_column('root_device_name')
 
=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2011-06-30 19:20:59 +0000
+++ nova/db/sqlalchemy/models.py	2011-07-08 09:39:31 +0000
@@ -236,6 +236,8 @@
     vm_mode = Column(String(255))
     uuid = Column(String(36))
 
+    root_device_name = Column(String(255))
+
     # TODO(vish): see Ewan's email about state improvements, probably
     #             should be in a driver base class or some such
     # vmstate_state = running, halted, suspended, paused
 
=== modified file 'nova/image/fake.py'
--- nova/image/fake.py	2011-06-24 12:01:51 +0000
+++ nova/image/fake.py	2011-07-08 09:39:31 +0000
@@ -137,7 +137,11 @@
         try:
             image_id = metadata['id']
         except KeyError:
-            image_id = random.randint(0, 2 ** 31 - 1)
+            while True:
+                image_id = random.randint(0, 2 ** 31 - 1)
+                if not self.images.get(str(image_id)):
+                    break
+
         image_id = str(image_id)
 
         if self.images.get(image_id):
@@ -176,3 +180,8 @@
 
 def FakeImageService():
     return _fakeImageService
+
+
+def FakeImageService_reset():
+    global _fakeImageService
+    _fakeImageService = _FakeImageService()
 
=== modified file 'nova/image/s3.py'
--- nova/image/s3.py	2011-06-01 03:16:22 +0000
+++ nova/image/s3.py	2011-07-08 09:39:31 +0000
@@ -102,18 +102,7 @@
         key.get_contents_to_filename(local_filename)
         return local_filename
 
-    def _s3_create(self, context, metadata):
-        """Gets a manifext from s3 and makes an image."""
-
-        image_path = tempfile.mkdtemp(dir=FLAGS.image_decryption_dir)
-
-        image_location = metadata['properties']['image_location']
-        bucket_name = image_location.split('/')[0]
-        manifest_path = image_location[len(bucket_name) + 1:]
-        bucket = self._conn(context).get_bucket(bucket_name)
-        key = bucket.get_key(manifest_path)
-        manifest = key.get_contents_as_string()
-
+    def _s3_parse_manifest(self, context, metadata, manifest):
         manifest = ElementTree.fromstring(manifest)
         image_format = 'ami'
         image_type = 'machine'
@@ -141,6 +130,28 @@
         except Exception:
             arch = 'x86_64'
 
+        # NOTE(yamahata):
+        # EC2 ec2-budlne-image --block-device-mapping accepts
+        # <virtual name>=<device name> where
+        # virtual name = {ami, root, swap, ephemeral<N>}
+        #                where N is no negative integer
+        # device name = the device name seen by guest kernel.
+        # They are converted into
+        # block_device_mapping/mapping/{virtual, device}
+        #
+        # Do NOT confuse this with ec2-register's block device mapping
+        # argument.
+        mappings = []
+        try:
+            block_device_mapping = manifest.findall('machine_configuration/'
+                                                    'block_device_mapping/'
+                                                    'mapping')
+            for bdm in block_device_mapping:
+                mappings.append({'virtual': bdm.find('virtual').text,
+                                 'device': bdm.find('device').text})
+        except Exception:
+            mappings = []
+
         properties = metadata['properties']
         properties['project_id'] = context.project_id
         properties['architecture'] = arch
@@ -151,6 +162,9 @@
         if ramdisk_id:
             properties['ramdisk_id'] = ec2utils.ec2_id_to_id(ramdisk_id)
 
+        if mappings:
+            properties['mappings'] = mappings
+
         metadata.update({'disk_format': image_format,
                          'container_format': image_format,
                          'status': 'queued',
@@ -158,6 +172,21 @@
                          'properties': properties})
         metadata['properties']['image_state'] = 'pending'
         image = self.service.create(context, metadata)
+        return manifest, image
+
+    def _s3_create(self, context, metadata):
+        """Gets a manifext from s3 and makes an image."""
+
+        image_path = tempfile.mkdtemp(dir=FLAGS.image_decryption_dir)
+
+        image_location = metadata['properties']['image_location']
+        bucket_name = image_location.split('/')[0]
+        manifest_path = image_location[len(bucket_name) + 1:]
+        bucket = self._conn(context).get_bucket(bucket_name)
+        key = bucket.get_key(manifest_path)
+        manifest = key.get_contents_as_string()
+
+        manifest, image = self._s3_parse_manifest(context, metadata, manifest)
         image_id = image['id']
 
         def delayed_create():
 
=== modified file 'nova/test.py'
--- nova/test.py	2011-06-29 17:58:10 +0000
+++ nova/test.py	2011-07-08 09:39:31 +0000
@@ -31,6 +31,7 @@
 
 import mox
 import nose.plugins.skip
+import nova.image.fake
 import shutil
 import stubout
 from eventlet import greenthread
@@ -119,6 +120,9 @@
                 if hasattr(fake.FakeConnection, '_instance'):
                     del fake.FakeConnection._instance
 
+            if FLAGS.image_service == 'nova.image.fake.FakeImageService':
+                nova.image.fake.FakeImageService_reset()
+
             # Reset any overriden flags
             self.reset_flags()
 
@@ -248,3 +252,15 @@
         for d1, d2 in zip(L1, L2):
             self.assertDictMatch(d1, d2, approx_equal=approx_equal,
                                  tolerance=tolerance)
+
+    def assertSubDictMatch(self, sub_dict, super_dict):
+        """Assert a sub_dict is subset of super_dict."""
+        self.assertTrue(set(sub_dict.keys()).issubset(set(super_dict.keys())))
+        for k, sub_value in sub_dict.items():
+            super_value = super_dict[k]
+            if isinstance(sub_value, dict):
+                self.assertSubDictMatch(sub_value, super_value)
+            elif 'DONTCARE' in (sub_value, super_value):
+                continue
+            else:
+                self.assertEqual(sub_value, super_value)
 
=== added file 'nova/tests/image/test_s3.py'
--- nova/tests/image/test_s3.py	1970-01-01 00:00:00 +0000
+++ nova/tests/image/test_s3.py	2011-07-08 09:39:31 +0000
@@ -0,0 +1,122 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 Isaku Yamahata
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from nova import context
+from nova import flags
+from nova import test
+from nova.image import s3
+
+FLAGS = flags.FLAGS
+
+
+ami_manifest_xml = """<?xml version="1.0" ?>
+<manifest>
+        <version>2011-06-17</version>
+        <bundler>
+                <name>test-s3</name>
+                <version>0</version>
+                <release>0</release>
+        </bundler>
+        <machine_configuration>
+                <architecture>x86_64</architecture>
+                <block_device_mapping>
+                        <mapping>
+                                <virtual>ami</virtual>
+                                <device>sda1</device>
+                        </mapping>
+                        <mapping>
+                                <virtual>root</virtual>
+                                <device>/dev/sda1</device>
+                        </mapping>
+                        <mapping>
+                                <virtual>ephemeral0</virtual>
+                                <device>sda2</device>
+                        </mapping>
+                        <mapping>
+                                <virtual>swap</virtual>
+                                <device>sda3</device>
+                        </mapping>
+                </block_device_mapping>
+        </machine_configuration>
+</manifest>
+"""
+
+
+class TestS3ImageService(test.TestCase):
+    def setUp(self):
+        super(TestS3ImageService, self).setUp()
+        self.orig_image_service = FLAGS.image_service
+        FLAGS.image_service = 'nova.image.fake.FakeImageService'
+        self.image_service = s3.S3ImageService()
+        self.context = context.RequestContext(None, None)
+
+    def tearDown(self):
+        super(TestS3ImageService, self).tearDown()
+        FLAGS.image_service = self.orig_image_service
+
+    def _assertEqualList(self, list0, list1, keys):
+        self.assertEqual(len(list0), len(list1))
+        key = keys[0]
+        for x in list0:
+            self.assertEqual(len(x), len(keys))
+            self.assertTrue(key in x)
+            for y in list1:
+                self.assertTrue(key in y)
+                if x[key] == y[key]:
+                    for k in keys:
+                        self.assertEqual(x[k], y[k])
+
+    def test_s3_create(self):
+        metadata = {'properties': {
+            'root_device_name': '/dev/sda1',
+            'block_device_mapping': [
+                {'device_name': '/dev/sda1',
+                 'snapshot_id': 'snap-12345678',
+                 'delete_on_termination': True},
+                {'device_name': '/dev/sda2',
+                 'virutal_name': 'ephemeral0'},
+                {'device_name': '/dev/sdb0',
+                 'no_device': True}]}}
+        _manifest, image = self.image_service._s3_parse_manifest(
+            self.context, metadata, ami_manifest_xml)
+        image_id = image['id']
+
+        ret_image = self.image_service.show(self.context, image_id)
+        self.assertTrue('properties' in ret_image)
+        properties = ret_image['properties']
+
+        self.assertTrue('mappings' in properties)
+        mappings = properties['mappings']
+        expected_mappings = [
+            {"device": "sda1", "virtual": "ami"},
+            {"device": "/dev/sda1", "virtual": "root"},
+            {"device": "sda2", "virtual": "ephemeral0"},
+            {"device": "sda3", "virtual": "swap"}]
+        self._assertEqualList(mappings, expected_mappings,
+            ['device', 'virtual'])
+
+        self.assertTrue('block_device_mapping', properties)
+        block_device_mapping = properties['block_device_mapping']
+        expected_bdm = [
+            {'device_name': '/dev/sda1',
+             'snapshot_id': 'snap-12345678',
+             'delete_on_termination': True},
+            {'device_name': '/dev/sda2',
+             'virutal_name': 'ephemeral0'},
+            {'device_name': '/dev/sdb0',
+             'no_device': True}]
+        self.assertEqual(block_device_mapping, expected_bdm)
 
=== modified file 'nova/tests/test_api.py'
--- nova/tests/test_api.py	2011-06-24 12:01:51 +0000
+++ nova/tests/test_api.py	2011-07-08 09:39:31 +0000
@@ -92,7 +92,9 @@
         conv = ec2utils._try_convert
         self.assertEqual(conv('None'), None)
         self.assertEqual(conv('True'), True)
+        self.assertEqual(conv('true'), True)
         self.assertEqual(conv('False'), False)
+        self.assertEqual(conv('false'), False)
         self.assertEqual(conv('0'), 0)
         self.assertEqual(conv('42'), 42)
         self.assertEqual(conv('3.14'), 3.14)
@@ -107,6 +109,8 @@
     def test_ec2_id_to_id(self):
         self.assertEqual(ec2utils.ec2_id_to_id('i-0000001e'), 30)
         self.assertEqual(ec2utils.ec2_id_to_id('ami-1d'), 29)
+        self.assertEqual(ec2utils.ec2_id_to_id('snap-0000001c'), 28)
+        self.assertEqual(ec2utils.ec2_id_to_id('vol-0000001b'), 27)
 
     def test_bad_ec2_id(self):
         self.assertRaises(exception.InvalidEc2Id,
@@ -116,6 +120,72 @@
     def test_id_to_ec2_id(self):
         self.assertEqual(ec2utils.id_to_ec2_id(30), 'i-0000001e')
         self.assertEqual(ec2utils.id_to_ec2_id(29, 'ami-%08x'), 'ami-0000001d')
+        self.assertEqual(ec2utils.id_to_ec2_snap_id(28), 'snap-0000001c')
+        self.assertEqual(ec2utils.id_to_ec2_vol_id(27), 'vol-0000001b')
+
+    def test_dict_from_dotted_str(self):
+        in_str = [('BlockDeviceMapping.1.DeviceName', '/dev/sda1'),
+                  ('BlockDeviceMapping.1.Ebs.SnapshotId', 'snap-0000001c'),
+                  ('BlockDeviceMapping.1.Ebs.VolumeSize', '80'),
+                  ('BlockDeviceMapping.1.Ebs.DeleteOnTermination', 'false'),
+                  ('BlockDeviceMapping.2.DeviceName', '/dev/sdc'),
+                  ('BlockDeviceMapping.2.VirtualName', 'ephemeral0')]
+        expected_dict = {
+            'block_device_mapping': {
+            '1': {'device_name': '/dev/sda1',
+                  'ebs': {'snapshot_id': 'snap-0000001c',
+                          'volume_size': 80,
+                          'delete_on_termination': False}},
+            '2': {'device_name': '/dev/sdc',
+                  'virtual_name': 'ephemeral0'}}}
+        out_dict = ec2utils.dict_from_dotted_str(in_str)
+
+        self.assertDictMatch(out_dict, expected_dict)
+
+    def test_properties_root_defice_name(self):
+        mappings = [{"device": "/dev/sda1", "virtual": "root"}]
+        properties0 = {'mappings': mappings}
+        properties1 = {'root_device_name': '/dev/sdb', 'mappings': mappings}
+
+        root_device_name = ec2utils.properties_root_device_name(properties0)
+        self.assertEqual(root_device_name, '/dev/sda1')
+
+        root_device_name = ec2utils.properties_root_device_name(properties1)
+        self.assertEqual(root_device_name, '/dev/sdb')
+
+    def test_mapping_prepend_dev(self):
+        mappings = [
+            {'virtual': 'ami',
+             'device': 'sda1'},
+            {'virtual': 'root',
+             'device': '/dev/sda1'},
+
+            {'virtual': 'swap',
+             'device': 'sdb1'},
+            {'virtual': 'swap',
+             'device': '/dev/sdb2'},
+
+            {'virtual': 'ephemeral0',
+            'device': 'sdc1'},
+            {'virtual': 'ephemeral1',
+             'device': '/dev/sdc1'}]
+        expected_result = [
+            {'virtual': 'ami',
+             'device': 'sda1'},
+            {'virtual': 'root',
+             'device': '/dev/sda1'},
+
+            {'virtual': 'swap',
+             'device': '/dev/sdb1'},
+            {'virtual': 'swap',
+             'device': '/dev/sdb2'},
+
+            {'virtual': 'ephemeral0',
+             'device': '/dev/sdc1'},
+            {'virtual': 'ephemeral1',
+             'device': '/dev/sdc1'}]
+        self.assertDictListMatch(ec2utils.mappings_prepend_dev(mappings),
+                                 expected_result)
 
 
 class ApiEc2TestCase(test.TestCase):
 
=== added file 'nova/tests/test_bdm.py'
--- nova/tests/test_bdm.py	1970-01-01 00:00:00 +0000
+++ nova/tests/test_bdm.py	2011-07-08 09:39:31 +0000
@@ -0,0 +1,233 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 Isaku Yamahata
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""
+Tests for Block Device Mapping Code.
+"""
+
+from nova.api.ec2 import cloud
+from nova import test
+
+
+class BlockDeviceMappingEc2CloudTestCase(test.TestCase):
+    """Test Case for Block Device Mapping"""
+
+    def setUp(self):
+        super(BlockDeviceMappingEc2CloudTestCase, self).setUp()
+
+    def tearDown(self):
+        super(BlockDeviceMappingEc2CloudTestCase, self).tearDown()
+
+    def _assertApply(self, action, bdm_list):
+        for bdm, expected_result in bdm_list:
+            self.assertDictMatch(action(bdm), expected_result)
+
+    def test_parse_block_device_mapping(self):
+        bdm_list = [
+            ({'device_name': '/dev/fake0',
+              'ebs': {'snapshot_id': 'snap-12345678',
+                      'volume_size': 1}},
+            {'device_name': '/dev/fake0',
+             'snapshot_id': 0x12345678,
+             'volume_size': 1,
+             'delete_on_termination': True}),
+
+            ({'device_name': '/dev/fake1',
+              'ebs': {'snapshot_id': 'snap-23456789',
+                      'delete_on_termination': False}},
+             {'device_name': '/dev/fake1',
+              'snapshot_id': 0x23456789,
+              'delete_on_termination': False}),
+
+            ({'device_name': '/dev/fake2',
+              'ebs': {'snapshot_id': 'vol-87654321',
+                      'volume_size': 2}},
+            {'device_name': '/dev/fake2',
+             'volume_id': 0x87654321,
+             'volume_size': 2,
+             'delete_on_termination': True}),
+
+            ({'device_name': '/dev/fake3',
+              'ebs': {'snapshot_id': 'vol-98765432',
+                      'delete_on_termination': False}},
+             {'device_name': '/dev/fake3',
+              'volume_id': 0x98765432,
+              'delete_on_termination': False}),
+
+            ({'device_name': '/dev/fake4',
+              'ebs': {'no_device': True}},
+             {'device_name': '/dev/fake4',
+              'no_device': True}),
+
+            ({'device_name': '/dev/fake5',
+             'virtual_name': 'ephemeral0'},
+            {'device_name': '/dev/fake5',
+             'virtual_name': 'ephemeral0'}),
+
+            ({'device_name': '/dev/fake6',
+             'virtual_name': 'swap'},
+            {'device_name': '/dev/fake6',
+             'virtual_name': 'swap'}),
+            ]
+        self._assertApply(cloud._parse_block_device_mapping, bdm_list)
+
+    def test_format_block_device_mapping(self):
+        bdm_list = [
+            ({'device_name': '/dev/fake0',
+              'snapshot_id': 0x12345678,
+              'volume_size': 1,
+              'delete_on_termination': True},
+             {'deviceName': '/dev/fake0',
+              'ebs': {'snapshotId': 'snap-12345678',
+                      'volumeSize': 1,
+                      'deleteOnTermination': True}}),
+
+            ({'device_name': '/dev/fake1',
+              'snapshot_id': 0x23456789},
+             {'deviceName': '/dev/fake1',
+              'ebs': {'snapshotId': 'snap-23456789'}}),
+
+            ({'device_name': '/dev/fake2',
+              'snapshot_id': 0x23456789,
+              'delete_on_termination': False},
+             {'deviceName': '/dev/fake2',
+              'ebs': {'snapshotId': 'snap-23456789',
+                      'deleteOnTermination': False}}),
+
+            ({'device_name': '/dev/fake3',
+              'volume_id': 0x12345678,
+              'volume_size': 1,
+              'delete_on_termination': True},
+             {'deviceName': '/dev/fake3',
+              'ebs': {'snapshotId': 'vol-12345678',
+                      'volumeSize': 1,
+                      'deleteOnTermination': True}}),
+
+            ({'device_name': '/dev/fake4',
+              'volume_id': 0x23456789},
+             {'deviceName': '/dev/fake4',
+              'ebs': {'snapshotId': 'vol-23456789'}}),
+
+            ({'device_name': '/dev/fake5',
+              'volume_id': 0x23456789,
+              'delete_on_termination': False},
+             {'deviceName': '/dev/fake5',
+              'ebs': {'snapshotId': 'vol-23456789',
+                      'deleteOnTermination': False}}),
+            ]
+        self._assertApply(cloud._format_block_device_mapping, bdm_list)
+
+    def test_format_mapping(self):
+        properties = {
+            'mappings': [
+                {'virtual': 'ami',
+                 'device': 'sda1'},
+                {'virtual': 'root',
+                 'device': '/dev/sda1'},
+
+                {'virtual': 'swap',
+                 'device': 'sdb1'},
+                {'virtual': 'swap',
+                 'device': 'sdb2'},
+                {'virtual': 'swap',
+                 'device': 'sdb3'},
+                {'virtual': 'swap',
+                 'device': 'sdb4'},
+
+                {'virtual': 'ephemeral0',
+                 'device': 'sdc1'},
+                {'virtual': 'ephemeral1',
+                 'device': 'sdc2'},
+                {'virtual': 'ephemeral2',
+                 'device': 'sdc3'},
+                ],
+
+            'block_device_mapping': [
+                # root
+                {'device_name': '/dev/sda1',
+                 'snapshot_id': 0x12345678,
+                 'delete_on_termination': False},
+
+
+                # overwrite swap
+                {'device_name': '/dev/sdb2',
+                 'snapshot_id': 0x23456789,
+                 'delete_on_termination': False},
+                {'device_name': '/dev/sdb3',
+                 'snapshot_id': 0x3456789A},
+                {'device_name': '/dev/sdb4',
+                 'no_device': True},
+
+                # overwrite ephemeral
+                {'device_name': '/dev/sdc2',
+                 'snapshot_id': 0x3456789A,
+                 'delete_on_termination': False},
+                {'device_name': '/dev/sdc3',
+                 'snapshot_id': 0x456789AB},
+                {'device_name': '/dev/sdc4',
+                 'no_device': True},
+
+                # volume
+                {'device_name': '/dev/sdd1',
+                 'snapshot_id': 0x87654321,
+                 'delete_on_termination': False},
+                {'device_name': '/dev/sdd2',
+                 'snapshot_id': 0x98765432},
+                {'device_name': '/dev/sdd3',
+                 'snapshot_id': 0xA9875463},
+                {'device_name': '/dev/sdd4',
+                 'no_device': True}]}
+
+        expected_result = {
+            'blockDeviceMapping': [
+                # root
+                {'deviceName': '/dev/sda1',
+                 'ebs': {'snapshotId': 'snap-12345678',
+                         'deleteOnTermination': False}},
+
+                # swap
+                {'deviceName': '/dev/sdb1',
+                 'virtualName': 'swap'},
+                {'deviceName': '/dev/sdb2',
+                 'ebs': {'snapshotId': 'snap-23456789',
+                         'deleteOnTermination': False}},
+                {'deviceName': '/dev/sdb3',
+                 'ebs': {'snapshotId': 'snap-3456789a'}},
+
+                # ephemeral
+                {'deviceName': '/dev/sdc1',
+                 'virtualName': 'ephemeral0'},
+                {'deviceName': '/dev/sdc2',
+                 'ebs': {'snapshotId': 'snap-3456789a',
+                         'deleteOnTermination': False}},
+                {'deviceName': '/dev/sdc3',
+                 'ebs': {'snapshotId': 'snap-456789ab'}},
+
+                # volume
+                {'deviceName': '/dev/sdd1',
+                 'ebs': {'snapshotId': 'snap-87654321',
+                         'deleteOnTermination': False}},
+                {'deviceName': '/dev/sdd2',
+                 'ebs': {'snapshotId': 'snap-98765432'}},
+                {'deviceName': '/dev/sdd3',
+                 'ebs': {'snapshotId': 'snap-a9875463'}}]}
+
+        result = {}
+        cloud._format_mappings(properties, result)
+        print result
+        self.assertEqual(result['blockDeviceMapping'].sort(),
+                         expected_result['blockDeviceMapping'].sort())
 
=== modified file 'nova/tests/test_cloud.py'
--- nova/tests/test_cloud.py	2011-07-01 15:47:33 +0000
+++ nova/tests/test_cloud.py	2011-07-08 09:39:31 +0000
@@ -45,7 +45,8 @@
 class CloudTestCase(test.TestCase):
     def setUp(self):
         super(CloudTestCase, self).setUp()
-        self.flags(connection_type='fake')
+        self.flags(connection_type='fake',
+                   stub_network=True)
 
         self.conn = rpc.Connection.instance()
 
@@ -289,7 +290,7 @@
         vol2 = db.volume_create(self.context, {})
         result = self.cloud.describe_volumes(self.context)
         self.assertEqual(len(result['volumeSet']), 2)
-        volume_id = ec2utils.id_to_ec2_id(vol2['id'], 'vol-%08x')
+        volume_id = ec2utils.id_to_ec2_vol_id(vol2['id'])
         result = self.cloud.describe_volumes(self.context,
                                              volume_id=[volume_id])
         self.assertEqual(len(result['volumeSet']), 1)
@@ -305,7 +306,7 @@
         snap = db.snapshot_create(self.context, {'volume_id': vol['id'],
                                                  'volume_size': vol['size'],
                                                  'status': "available"})
-        snapshot_id = ec2utils.id_to_ec2_id(snap['id'], 'snap-%08x')
+        snapshot_id = ec2utils.id_to_ec2_snap_id(snap['id'])
 
         result = self.cloud.create_volume(self.context,
                                           snapshot_id=snapshot_id)
@@ -344,7 +345,7 @@
         snap2 = db.snapshot_create(self.context, {'volume_id': vol['id']})
         result = self.cloud.describe_snapshots(self.context)
         self.assertEqual(len(result['snapshotSet']), 2)
-        snapshot_id = ec2utils.id_to_ec2_id(snap2['id'], 'snap-%08x')
+        snapshot_id = ec2utils.id_to_ec2_snap_id(snap2['id'])
         result = self.cloud.describe_snapshots(self.context,
                                                snapshot_id=[snapshot_id])
         self.assertEqual(len(result['snapshotSet']), 1)
@@ -358,7 +359,7 @@
     def test_create_snapshot(self):
         """Makes sure create_snapshot works."""
         vol = db.volume_create(self.context, {'status': "available"})
-        volume_id = ec2utils.id_to_ec2_id(vol['id'], 'vol-%08x')
+        volume_id = ec2utils.id_to_ec2_vol_id(vol['id'])
 
         result = self.cloud.create_snapshot(self.context,
                                             volume_id=volume_id)
@@ -375,7 +376,7 @@
         vol = db.volume_create(self.context, {'status': "available"})
         snap = db.snapshot_create(self.context, {'volume_id': vol['id'],
                                                   'status': "available"})
-        snapshot_id = ec2utils.id_to_ec2_id(snap['id'], 'snap-%08x')
+        snapshot_id = ec2utils.id_to_ec2_snap_id(snap['id'])
 
         result = self.cloud.delete_snapshot(self.context,
                                             snapshot_id=snapshot_id)
@@ -414,6 +415,185 @@
         db.service_destroy(self.context, comp1['id'])
         db.service_destroy(self.context, comp2['id'])
 
+    def _block_device_mapping_create(self, instance_id, mappings):
+        volumes = []
+        for bdm in mappings:
+            db.block_device_mapping_create(self.context, bdm)
+            if 'volume_id' in bdm:
+                values = {'id': bdm['volume_id']}
+                for bdm_key, vol_key in [('snapshot_id', 'snapshot_id'),
+                                         ('snapshot_size', 'volume_size'),
+                                         ('delete_on_termination',
+                                          'delete_on_termination')]:
+                    if bdm_key in bdm:
+                        values[vol_key] = bdm[bdm_key]
+                vol = db.volume_create(self.context, values)
+                db.volume_attached(self.context, vol['id'],
+                                   instance_id, bdm['device_name'])
+                volumes.append(vol)
+        return volumes
+
+    def _setUpBlockDeviceMapping(self):
+        inst1 = db.instance_create(self.context,
+                                  {'image_ref': 1,
+                                   'root_device_name': '/dev/sdb1'})
+        inst2 = db.instance_create(self.context,
+                                  {'image_ref': 2,
+                                   'root_device_name': '/dev/sdc1'})
+
+        instance_id = inst1['id']
+        mappings0 = [
+            {'instance_id': instance_id,
+             'device_name': '/dev/sdb1',
+             'snapshot_id': '1',
+             'volume_id': '2'},
+            {'instance_id': instance_id,
+             'device_name': '/dev/sdb2',
+             'volume_id': '3',
+             'volume_size': 1},
+            {'instance_id': instance_id,
+             'device_name': '/dev/sdb3',
+             'delete_on_termination': True,
+             'snapshot_id': '4',
+             'volume_id': '5'},
+            {'instance_id': instance_id,
+             'device_name': '/dev/sdb4',
+             'delete_on_termination': False,
+             'snapshot_id': '6',
+             'volume_id': '7'},
+            {'instance_id': instance_id,
+             'device_name': '/dev/sdb5',
+             'snapshot_id': '8',
+             'volume_id': '9',
+             'volume_size': 0},
+            {'instance_id': instance_id,
+             'device_name': '/dev/sdb6',
+             'snapshot_id': '10',
+             'volume_id': '11',
+             'volume_size': 1},
+            {'instance_id': instance_id,
+             'device_name': '/dev/sdb7',
+             'no_device': True},
+            {'instance_id': instance_id,
+             'device_name': '/dev/sdb8',
+             'virtual_name': 'swap'},
+            {'instance_id': instance_id,
+             'device_name': '/dev/sdb9',
+             'virtual_name': 'ephemeral3'}]
+
+        volumes = self._block_device_mapping_create(instance_id, mappings0)
+        return (inst1, inst2, volumes)
+
+    def _tearDownBlockDeviceMapping(self, inst1, inst2, volumes):
+        for vol in volumes:
+            db.volume_destroy(self.context, vol['id'])
+        for id in (inst1['id'], inst2['id']):
+            for bdm in db.block_device_mapping_get_all_by_instance(
+                self.context, id):
+                db.block_device_mapping_destroy(self.context, bdm['id'])
+        db.instance_destroy(self.context, inst2['id'])
+        db.instance_destroy(self.context, inst1['id'])
+
+    _expected_instance_bdm1 = {
+        'instanceId': 'i-00000001',
+        'rootDeviceName': '/dev/sdb1',
+        'rootDeviceType': 'ebs'}
+
+    _expected_block_device_mapping0 = [
+        {'deviceName': '/dev/sdb1',
+         'ebs': {'status': 'in-use',
+                 'deleteOnTermination': False,
+                 'volumeId': 2,
+                 }},
+        {'deviceName': '/dev/sdb2',
+         'ebs': {'status': 'in-use',
+                 'deleteOnTermination': False,
+                 'volumeId': 3,
+                 }},
+        {'deviceName': '/dev/sdb3',
+         'ebs': {'status': 'in-use',
+                 'deleteOnTermination': True,
+                 'volumeId': 5,
+                 }},
+        {'deviceName': '/dev/sdb4',
+         'ebs': {'status': 'in-use',
+                 'deleteOnTermination': False,
+                 'volumeId': 7,
+                 }},
+        {'deviceName': '/dev/sdb5',
+         'ebs': {'status': 'in-use',
+                 'deleteOnTermination': False,
+                 'volumeId': 9,
+                 }},
+        {'deviceName': '/dev/sdb6',
+         'ebs': {'status': 'in-use',
+                 'deleteOnTermination': False,
+                 'volumeId': 11, }}]
+        # NOTE(yamahata): swap/ephemeral device case isn't supported yet.
+
+    _expected_instance_bdm2 = {
+        'instanceId': 'i-00000002',
+        'rootDeviceName': '/dev/sdc1',
+        'rootDeviceType': 'instance-store'}
+
+    def test_format_instance_bdm(self):
+        (inst1, inst2, volumes) = self._setUpBlockDeviceMapping()
+
+        result = {}
+        self.cloud._format_instance_bdm(self.context, inst1['id'], '/dev/sdb1',
+                                        result)
+        self.assertSubDictMatch(
+            {'rootDeviceType': self._expected_instance_bdm1['rootDeviceType']},
+            result)
+        self._assertEqualBlockDeviceMapping(
+            self._expected_block_device_mapping0, result['blockDeviceMapping'])
+
+        result = {}
+        self.cloud._format_instance_bdm(self.context, inst2['id'], '/dev/sdc1',
+                                        result)
+        self.assertSubDictMatch(
+            {'rootDeviceType': self._expected_instance_bdm2['rootDeviceType']},
+            result)
+
+        self._tearDownBlockDeviceMapping(inst1, inst2, volumes)
+
+    def _assertInstance(self, instance_id):
+        ec2_instance_id = ec2utils.id_to_ec2_id(instance_id)
+        result = self.cloud.describe_instances(self.context,
+                                               instance_id=[ec2_instance_id])
+        result = result['reservationSet'][0]
+        self.assertEqual(len(result['instancesSet']), 1)
+        result = result['instancesSet'][0]
+        self.assertEqual(result['instanceId'], ec2_instance_id)
+        return result
+
+    def _assertEqualBlockDeviceMapping(self, expected, result):
+        self.assertEqual(len(expected), len(result))
+        for x in expected:
+            found = False
+            for y in result:
+                if x['deviceName'] == y['deviceName']:
+                    self.assertSubDictMatch(x, y)
+                    found = True
+                    break
+            self.assertTrue(found)
+
+    def test_describe_instances_bdm(self):
+        """Make sure describe_instances works with root_device_name and
+        block device mappings
+        """
+        (inst1, inst2, volumes) = self._setUpBlockDeviceMapping()
+
+        result = self._assertInstance(inst1['id'])
+        self.assertSubDictMatch(self._expected_instance_bdm1, result)
+        self._assertEqualBlockDeviceMapping(
+            self._expected_block_device_mapping0, result['blockDeviceMapping'])
+
+        result = self._assertInstance(inst2['id'])
+        self.assertSubDictMatch(self._expected_instance_bdm2, result)
+
+        self._tearDownBlockDeviceMapping(inst1, inst2, volumes)
+
     def test_describe_images(self):
         describe_images = self.cloud.describe_images
 
@@ -443,6 +623,161 @@
         self.assertRaises(exception.ImageNotFound, describe_images,
                           self.context, ['ami-fake'])
 
+    def assertDictListUnorderedMatch(self, L1, L2, key):
+        self.assertEqual(len(L1), len(L2))
+        for d1 in L1:
+            self.assertTrue(key in d1)
+            for d2 in L2:
+                self.assertTrue(key in d2)
+                if d1[key] == d2[key]:
+                    self.assertDictMatch(d1, d2)
+
+    def _setUpImageSet(self, create_volumes_and_snapshots=False):
+        mappings1 = [
+            {'device': '/dev/sda1', 'virtual': 'root'},
+
+            {'device': 'sdb0', 'virtual': 'ephemeral0'},
+            {'device': 'sdb1', 'virtual': 'ephemeral1'},
+            {'device': 'sdb2', 'virtual': 'ephemeral2'},
+            {'device': 'sdb3', 'virtual': 'ephemeral3'},
+            {'device': 'sdb4', 'virtual': 'ephemeral4'},
+
+            {'device': 'sdc0', 'virtual': 'swap'},
+            {'device': 'sdc1', 'virtual': 'swap'},
+            {'device': 'sdc2', 'virtual': 'swap'},
+            {'device': 'sdc3', 'virtual': 'swap'},
+            {'device': 'sdc4', 'virtual': 'swap'}]
+        block_device_mapping1 = [
+            {'device_name': '/dev/sdb1', 'snapshot_id': 01234567},
+            {'device_name': '/dev/sdb2', 'volume_id': 01234567},
+            {'device_name': '/dev/sdb3', 'virtual_name': 'ephemeral5'},
+            {'device_name': '/dev/sdb4', 'no_device': True},
+
+            {'device_name': '/dev/sdc1', 'snapshot_id': 12345678},
+            {'device_name': '/dev/sdc2', 'volume_id': 12345678},
+            {'device_name': '/dev/sdc3', 'virtual_name': 'ephemeral6'},
+            {'device_name': '/dev/sdc4', 'no_device': True}]
+        image1 = {
+            'id': 1,
+            'properties': {
+                'kernel_id': 1,
+                'type': 'machine',
+                'image_state': 'available',
+                'mappings': mappings1,
+                'block_device_mapping': block_device_mapping1,
+                }
+            }
+
+        mappings2 = [{'device': '/dev/sda1', 'virtual': 'root'}]
+        block_device_mapping2 = [{'device_name': '/dev/sdb1',
+                                  'snapshot_id': 01234567}]
+        image2 = {
+            'id': 2,
+            'properties': {
+                'kernel_id': 2,
+                'type': 'machine',
+                'root_device_name': '/dev/sdb1',
+                'mappings': mappings2,
+                'block_device_mapping': block_device_mapping2}}
+
+        def fake_show(meh, context, image_id):
+            for i in [image1, image2]:
+                if i['id'] == image_id:
+                    return i
+            raise exception.ImageNotFound(image_id=image_id)
+
+        def fake_detail(meh, context):
+            return [image1, image2]
+
+        self.stubs.Set(fake._FakeImageService, 'show', fake_show)
+        self.stubs.Set(fake._FakeImageService, 'detail', fake_detail)
+
+        volumes = []
+        snapshots = []
+        if create_volumes_and_snapshots:
+            for bdm in block_device_mapping1:
+                if 'volume_id' in bdm:
+                    vol = self._volume_create(bdm['volume_id'])
+                    volumes.append(vol['id'])
+                if 'snapshot_id' in bdm:
+                    snap = db.snapshot_create(self.context,
+                                              {'id': bdm['snapshot_id'],
+                                               'volume_id': 76543210,
+                                               'status': "available",
+                                               'volume_size': 1})
+                    snapshots.append(snap['id'])
+        return (volumes, snapshots)
+
+    def _assertImageSet(self, result, root_device_type, root_device_name):
+        self.assertEqual(1, len(result['imagesSet']))
+        result = result['imagesSet'][0]
+        self.assertTrue('rootDeviceType' in result)
+        self.assertEqual(result['rootDeviceType'], root_device_type)
+        self.assertTrue('rootDeviceName' in result)
+        self.assertEqual(result['rootDeviceName'], root_device_name)
+        self.assertTrue('blockDeviceMapping' in result)
+
+        return result
+
+    _expected_root_device_name1 = '/dev/sda1'
+    # NOTE(yamahata): noDevice doesn't make sense when returning mapping
+    #                 It makes sense only when user overriding existing
+    #                 mapping.
+    _expected_bdms1 = [
+        {'deviceName': '/dev/sdb0', 'virtualName': 'ephemeral0'},
+        {'deviceName': '/dev/sdb1', 'ebs': {'snapshotId':
+                                            'snap-00053977'}},
+        {'deviceName': '/dev/sdb2', 'ebs': {'snapshotId':
+                                            'vol-00053977'}},
+        {'deviceName': '/dev/sdb3', 'virtualName': 'ephemeral5'},
+        # {'deviceName': '/dev/sdb4', 'noDevice': True},
+
+        {'deviceName': '/dev/sdc0', 'virtualName': 'swap'},
+        {'deviceName': '/dev/sdc1', 'ebs': {'snapshotId':
+                                            'snap-00bc614e'}},
+        {'deviceName': '/dev/sdc2', 'ebs': {'snapshotId':
+                                            'vol-00bc614e'}},
+        {'deviceName': '/dev/sdc3', 'virtualName': 'ephemeral6'},
+        # {'deviceName': '/dev/sdc4', 'noDevice': True}
+        ]
+
+    _expected_root_device_name2 = '/dev/sdb1'
+    _expected_bdms2 = [{'deviceName': '/dev/sdb1',
+                       'ebs': {'snapshotId': 'snap-00053977'}}]
+
+    # NOTE(yamahata):
+    # InstanceBlockDeviceMappingItemType
+    # rootDeviceType
+    # rootDeviceName
+    # blockDeviceMapping
+    #  deviceName
+    #  virtualName
+    #  ebs
+    #    snapshotId
+    #    volumeSize
+    #    deleteOnTermination
+    #  noDevice
+    def test_describe_image_mapping(self):
+        """test for rootDeviceName and blockDeiceMapping"""
+        describe_images = self.cloud.describe_images
+        self._setUpImageSet()
+
+        result = describe_images(self.context, ['ami-00000001'])
+        result = self._assertImageSet(result, 'instance-store',
+                                      self._expected_root_device_name1)
+
+        self.assertDictListUnorderedMatch(result['blockDeviceMapping'],
+                                          self._expected_bdms1, 'deviceName')
+
+        result = describe_images(self.context, ['ami-00000002'])
+        result = self._assertImageSet(result, 'ebs',
+                                      self._expected_root_device_name2)
+
+        self.assertDictListUnorderedMatch(result['blockDeviceMapping'],
+                                          self._expected_bdms2, 'deviceName')
+
+        self.stubs.UnsetAll()
+
     def test_describe_image_attribute(self):
         describe_image_attribute = self.cloud.describe_image_attribute
 
@@ -456,6 +791,32 @@
                                           'launchPermission')
         self.assertEqual([{'group': 'all'}], result['launchPermission'])
 
+    def test_describe_image_attribute_root_device_name(self):
+        describe_image_attribute = self.cloud.describe_image_attribute
+        self._setUpImageSet()
+
+        result = describe_image_attribute(self.context, 'ami-00000001',
+                                          'rootDeviceName')
+        self.assertEqual(result['rootDeviceName'],
+                         self._expected_root_device_name1)
+        result = describe_image_attribute(self.context, 'ami-00000002',
+                                          'rootDeviceName')
+        self.assertEqual(result['rootDeviceName'],
+                         self._expected_root_device_name2)
+
+    def test_describe_image_attribute_block_device_mapping(self):
+        describe_image_attribute = self.cloud.describe_image_attribute
+        self._setUpImageSet()
+
+        result = describe_image_attribute(self.context, 'ami-00000001',
+                                          'blockDeviceMapping')
+        self.assertDictListUnorderedMatch(result['blockDeviceMapping'],
+                                          self._expected_bdms1, 'deviceName')
+        result = describe_image_attribute(self.context, 'ami-00000002',
+                                          'blockDeviceMapping')
+        self.assertDictListUnorderedMatch(result['blockDeviceMapping'],
+                                          self._expected_bdms2, 'deviceName')
+
     def test_modify_image_attribute(self):
         modify_image_attribute = self.cloud.modify_image_attribute
 
@@ -683,7 +1044,7 @@
     def test_update_of_volume_display_fields(self):
         vol = db.volume_create(self.context, {})
         self.cloud.update_volume(self.context,
-                                 ec2utils.id_to_ec2_id(vol['id'], 'vol-%08x'),
+                                 ec2utils.id_to_ec2_vol_id(vol['id']),
                                  display_name='c00l v0lum3')
         vol = db.volume_get(self.context, vol['id'])
         self.assertEqual('c00l v0lum3', vol['display_name'])
@@ -692,7 +1053,7 @@
     def test_update_of_volume_wont_update_private_fields(self):
         vol = db.volume_create(self.context, {})
         self.cloud.update_volume(self.context,
-                                 ec2utils.id_to_ec2_id(vol['id'], 'vol-%08x'),
+                                 ec2utils.id_to_ec2_vol_id(vol['id']),
                                  mountpoint='/not/here')
         vol = db.volume_get(self.context, vol['id'])
         self.assertEqual(None, vol['mountpoint'])
@@ -770,11 +1131,13 @@
 
         self._restart_compute_service()
 
-    def _volume_create(self):
+    def _volume_create(self, volume_id=None):
         kwargs = {'status': 'available',
                   'host': self.volume.host,
                   'size': 1,
                   'attach_status': 'detached', }
+        if volume_id:
+            kwargs['id'] = volume_id
         return db.volume_create(self.context, kwargs)
 
     def _assert_volume_attached(self, vol, instance_id, mountpoint):
@@ -803,10 +1166,10 @@
                   'max_count': 1,
                   'block_device_mapping': [{'device_name': '/dev/vdb',
                                             'volume_id': vol1['id'],
-                                            'delete_on_termination': False, },
+                                            'delete_on_termination': False},
                                            {'device_name': '/dev/vdc',
                                             'volume_id': vol2['id'],
-                                            'delete_on_termination': True, },
+                                            'delete_on_termination': True},
                                            ]}
         ec2_instance_id = self._run_instance_wait(**kwargs)
         instance_id = ec2utils.ec2_id_to_id(ec2_instance_id)
@@ -938,7 +1301,7 @@
     def test_run_with_snapshot(self):
         """Makes sure run/stop/start instance with snapshot works."""
         vol = self._volume_create()
-        ec2_volume_id = ec2utils.id_to_ec2_id(vol['id'], 'vol-%08x')
+        ec2_volume_id = ec2utils.id_to_ec2_vol_id(vol['id'])
 
         ec2_snapshot1_id = self._create_snapshot(ec2_volume_id)
         snapshot1_id = ec2utils.ec2_id_to_id(ec2_snapshot1_id)
@@ -997,3 +1360,33 @@
             self.cloud.delete_snapshot(self.context, snapshot_id)
             greenthread.sleep(0.3)
         db.volume_destroy(self.context, vol['id'])
+
+    def test_create_image(self):
+        """Make sure that CreateImage works"""
+        # enforce periodic tasks run in short time to avoid wait for 60s.
+        self._restart_compute_service(periodic_interval=0.3)
+
+        (volumes, snapshots) = self._setUpImageSet(
+            create_volumes_and_snapshots=True)
+
+        kwargs = {'image_id': 'ami-1',
+                  'instance_type': FLAGS.default_instance_type,
+                  'max_count': 1}
+        ec2_instance_id = self._run_instance_wait(**kwargs)
+
+        # TODO(yamahata): s3._s3_create() can't be tested easily by unit test
+        #                 as there is no unit test for s3.create()
+        ## result = self.cloud.create_image(self.context, ec2_instance_id,
+        ##                                  no_reboot=True)
+        ## ec2_image_id = result['imageId']
+        ## created_image = self.cloud.describe_images(self.context,
+        ##                                            [ec2_image_id])
+
+        self.cloud.terminate_instances(self.context, [ec2_instance_id])
+        for vol in volumes:
+            db.volume_destroy(self.context, vol)
+        for snap in snapshots:
+            db.snapshot_destroy(self.context, snap)
+        # TODO(yamahata): clean up snapshot created by CreateImage.
+
+        self._restart_compute_service()
 
=== modified file 'nova/tests/test_compute.py'
--- nova/tests/test_compute.py	2011-06-30 19:20:59 +0000
+++ nova/tests/test_compute.py	2011-07-08 09:39:31 +0000
@@ -810,3 +810,114 @@
         LOG.info(_("After force-killing instances: %s"), instances)
         self.assertEqual(len(instances), 1)
         self.assertEqual(power_state.SHUTOFF, instances[0]['state'])
+
+    @staticmethod
+    def _parse_db_block_device_mapping(bdm_ref):
+        attr_list = ('delete_on_termination', 'device_name', 'no_device',
+                     'virtual_name', 'volume_id', 'volume_size', 'snapshot_id')
+        bdm = {}
+        for attr in attr_list:
+            val = bdm_ref.get(attr, None)
+            if val:
+                bdm[attr] = val
+
+        return bdm
+
+    def test_update_block_device_mapping(self):
+        instance_id = self._create_instance()
+        mappings = [
+                {'virtual': 'ami', 'device': 'sda1'},
+                {'virtual': 'root', 'device': '/dev/sda1'},
+
+                {'virtual': 'swap', 'device': 'sdb1'},
+                {'virtual': 'swap', 'device': 'sdb2'},
+                {'virtual': 'swap', 'device': 'sdb3'},
+                {'virtual': 'swap', 'device': 'sdb4'},
+
+                {'virtual': 'ephemeral0', 'device': 'sdc1'},
+                {'virtual': 'ephemeral1', 'device': 'sdc2'},
+                {'virtual': 'ephemeral2', 'device': 'sdc3'}]
+        block_device_mapping = [
+                # root
+                {'device_name': '/dev/sda1',
+                 'snapshot_id': 0x12345678,
+                 'delete_on_termination': False},
+
+
+                # overwrite swap
+                {'device_name': '/dev/sdb2',
+                 'snapshot_id': 0x23456789,
+                 'delete_on_termination': False},
+                {'device_name': '/dev/sdb3',
+                 'snapshot_id': 0x3456789A},
+                {'device_name': '/dev/sdb4',
+                 'no_device': True},
+
+                # overwrite ephemeral
+                {'device_name': '/dev/sdc2',
+                 'snapshot_id': 0x456789AB,
+                 'delete_on_termination': False},
+                {'device_name': '/dev/sdc3',
+                 'snapshot_id': 0x56789ABC},
+                {'device_name': '/dev/sdc4',
+                 'no_device': True},
+
+                # volume
+                {'device_name': '/dev/sdd1',
+                 'snapshot_id': 0x87654321,
+                 'delete_on_termination': False},
+                {'device_name': '/dev/sdd2',
+                 'snapshot_id': 0x98765432},
+                {'device_name': '/dev/sdd3',
+                 'snapshot_id': 0xA9875463},
+                {'device_name': '/dev/sdd4',
+                 'no_device': True}]
+
+        self.compute_api._update_image_block_device_mapping(
+            self.context, instance_id, mappings)
+
+        bdms = [self._parse_db_block_device_mapping(bdm_ref)
+                for bdm_ref in db.block_device_mapping_get_all_by_instance(
+                    self.context, instance_id)]
+        expected_result = [
+            {'virtual_name': 'swap', 'device_name': '/dev/sdb1'},
+            {'virtual_name': 'swap', 'device_name': '/dev/sdb2'},
+            {'virtual_name': 'swap', 'device_name': '/dev/sdb3'},
+            {'virtual_name': 'swap', 'device_name': '/dev/sdb4'},
+            {'virtual_name': 'ephemeral0', 'device_name': '/dev/sdc1'},
+            {'virtual_name': 'ephemeral1', 'device_name': '/dev/sdc2'},
+            {'virtual_name': 'ephemeral2', 'device_name': '/dev/sdc3'}]
+        bdms.sort()
+        expected_result.sort()
+        self.assertDictListMatch(bdms, expected_result)
+
+        self.compute_api._update_block_device_mapping(
+            self.context, instance_id, block_device_mapping)
+        bdms = [self._parse_db_block_device_mapping(bdm_ref)
+                for bdm_ref in db.block_device_mapping_get_all_by_instance(
+                    self.context, instance_id)]
+        expected_result = [
+            {'snapshot_id': 0x12345678, 'device_name': '/dev/sda1'},
+
+            {'virtual_name': 'swap', 'device_name': '/dev/sdb1'},
+            {'snapshot_id': 0x23456789, 'device_name': '/dev/sdb2'},
+            {'snapshot_id': 0x3456789A, 'device_name': '/dev/sdb3'},
+            {'no_device': True, 'device_name': '/dev/sdb4'},
+
+            {'virtual_name': 'ephemeral0', 'device_name': '/dev/sdc1'},
+            {'snapshot_id': 0x456789AB, 'device_name': '/dev/sdc2'},
+            {'snapshot_id': 0x56789ABC, 'device_name': '/dev/sdc3'},
+            {'no_device': True, 'device_name': '/dev/sdc4'},
+
+            {'snapshot_id': 0x87654321, 'device_name': '/dev/sdd1'},
+            {'snapshot_id': 0x98765432, 'device_name': '/dev/sdd2'},
+            {'snapshot_id': 0xA9875463, 'device_name': '/dev/sdd3'},
+            {'no_device': True, 'device_name': '/dev/sdd4'}]
+        bdms.sort()
+        expected_result.sort()
+        self.assertDictListMatch(bdms, expected_result)
+
+        for bdm in db.block_device_mapping_get_all_by_instance(
+            self.context, instance_id):
+            db.block_device_mapping_destroy(self.context, bdm['id'])
+        self.compute.terminate_instance(self.context, instance_id)
 
=== modified file 'nova/tests/test_volume.py'
--- nova/tests/test_volume.py	2011-06-06 17:20:08 +0000
+++ nova/tests/test_volume.py	2011-07-08 09:39:31 +0000
@@ -27,8 +27,10 @@
 from nova import db
 from nova import flags
 from nova import log as logging
+from nova import rpc
 from nova import test
 from nova import utils
+from nova import volume
 
 FLAGS = flags.FLAGS
 LOG = logging.getLogger('nova.tests.volume')
@@ -43,6 +45,11 @@
         self.flags(connection_type='fake')
         self.volume = utils.import_object(FLAGS.volume_manager)
         self.context = context.get_admin_context()
+        self.instance_id = db.instance_create(self.context, {})['id']
+
+    def tearDown(self):
+        db.instance_destroy(self.context, self.instance_id)
+        super(VolumeTestCase, self).tearDown()
 
     @staticmethod
     def _create_volume(size='0', snapshot_id=None):
@@ -223,6 +230,30 @@
                           snapshot_id)
         self.volume.delete_volume(self.context, volume_id)
 
+    def test_create_snapshot_force(self):
+        """Test snapshot in use can be created forcibly."""
+
+        def fake_cast(ctxt, topic, msg):
+            pass
+        self.stubs.Set(rpc, 'cast', fake_cast)
+
+        volume_id = self._create_volume()
+        self.volume.create_volume(self.context, volume_id)
+        db.volume_attached(self.context, volume_id, self.instance_id,
+                           '/dev/sda1')
+
+        volume_api = volume.api.API()
+        self.assertRaises(exception.ApiError,
+                          volume_api.create_snapshot,
+                          self.context, volume_id,
+                          'fake_name', 'fake_description')
+        snapshot_ref = volume_api.create_snapshot_force(self.context,
+                                                        volume_id,
+                                                        'fake_name',
+                                                        'fake_description')
+        db.snapshot_destroy(self.context, snapshot_ref['id'])
+        db.volume_destroy(self.context, volume_id)
+
 
 class DriverTestCase(test.TestCase):
     """Base Test class for Drivers."""
 
=== modified file 'nova/volume/api.py'
--- nova/volume/api.py	2011-06-24 12:01:51 +0000
+++ nova/volume/api.py	2011-07-08 09:39:31 +0000
@@ -140,9 +140,10 @@
                  {"method": "remove_volume",
                   "args": {'volume_id': volume_id}})
 
-    def create_snapshot(self, context, volume_id, name, description):
+    def _create_snapshot(self, context, volume_id, name, description,
+                         force=False):
         volume = self.get(context, volume_id)
-        if volume['status'] != "available":
+        if ((not force) and (volume['status'] != "available")):
             raise exception.ApiError(_("Volume status must be available"))
 
         options = {
@@ -164,6 +165,14 @@
                            "snapshot_id": snapshot['id']}})
         return snapshot
 
+    def create_snapshot(self, context, volume_id, name, description):
+        return self._create_snapshot(context, volume_id, name, description,
+                                     False)
+
+    def create_snapshot_force(self, context, volume_id, name, description):
+        return self._create_snapshot(context, volume_id, name, description,
+                                     True)
+
     def delete_snapshot(self, context, snapshot_id):
         snapshot = self.get_snapshot(context, snapshot_id)
         if snapshot['status'] != "available":