Create image of volume-backed instance via native API

Fixes bug 1034730.

Avoids 'qemu-img snapshot' failure when native API create_image action
is applied to a volume-backed instance.

Applies the same logic as is used to create a placeholder image from
a volume-backed instance via the EC2 API CreateImage operation, with
the now-common code refactored into the ComputeAPI class.

Change-Id: I624584ae9adbf30629f0e814d340da6b9e6e59bd

5 files changed, 202 insertions, 76 deletions

diff --git a/nova/api/ec2/cloud.py b/nova/api/ec2/cloud.py
index 14aa39438..e26813b99 100644
--- a/nova/api/ec2/cloud.py
+++ b/nova/api/ec2/cloud.py
@@ -1538,71 +1538,7 @@ class CloudController(object):
         glance_uuid = instance['image_ref']
         ec2_image_id = ec2utils.glance_id_to_ec2_id(context, glance_uuid)
         src_image = self._get_image(context, ec2_image_id)
-        new_image = dict(src_image)
-        properties = new_image['properties']
-        if instance['root_device_name']:
-            properties['root_device_name'] = instance['root_device_name']
-
-        # meaningful image name
-        name_map = dict(instance=instance['uuid'], now=timeutils.isotime())
-        new_image['name'] = (name or
-                             _('image of %(instance)s at %(now)s') % name_map)
-
-        mapping = []
-        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')
-            snapshot_id = m.get('snapshot_id')
-            if snapshot_id and volume_id:
-                # create snapshot based on volume_id
-                volume = self.volume_api.get(context, volume_id)
-                # NOTE(yamahata): Should we wait for snapshot creation?
-                #                 Linux LVM snapshot creation completes in
-                #                 short time, it doesn't matter for now.
-                name = _('snapshot for %s') % new_image['name']
-                snapshot = self.volume_api.create_snapshot_force(
-                        context, volume, name, volume['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 block_device.is_swap_or_ephemeral(virtual_name)
-            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'):
-            new_image.pop(attr, None)
-
-        # the new image is simply a bucket of properties (particularly the
-        # block device mapping, kernel and ramdisk IDs) with no image data,
-        # hence the zero size
-        new_image['size'] = 0
+        image_meta = dict(src_image)
 
         def _unmap_id_property(properties, name):
             if properties[name]:
@@ -1610,11 +1546,18 @@ class CloudController(object):
                                                             properties[name])
 
         # ensure the ID properties are unmapped back to the glance UUID
-        _unmap_id_property(properties, 'kernel_id')
-        _unmap_id_property(properties, 'ramdisk_id')
+        _unmap_id_property(image_meta['properties'], 'kernel_id')
+        _unmap_id_property(image_meta['properties'], 'ramdisk_id')
+
+        # meaningful image name
+        name_map = dict(instance=instance['uuid'], now=timeutils.isotime())
+        name = name or _('image of %(instance)s at %(now)s') % name_map
+
+        new_image = self.compute_api.snapshot_volume_backed(context,
+                                                            instance,
+                                                            image_meta,
+                                                            name)
 
-        new_image = self.image_service.service.create(context, new_image,
-                                                      data='')
         ec2_id = ec2utils.glance_id_to_ec2_id(context, new_image['id'])
 
         if restart_instance:
diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py
index 9f462c565..fc4905a11 100644
--- a/nova/api/openstack/compute/servers.py
+++ b/nova/api/openstack/compute/servers.py
@@ -1173,14 +1173,29 @@ class Controller(wsgi.Controller):
 
         instance = self._get_server(context, req, id)
 
+        bdms = self.compute_api.get_instance_bdms(context, instance)
+
         try:
-            image = self.compute_api.snapshot(context,
-                                              instance,
-                                              image_name,
-                                              extra_properties=props)
+            if self.compute_api.is_volume_backed_instance(context, instance,
+                                                          bdms):
+                img = instance['image_ref']
+                src_image = self.compute_api.image_service.show(context, img)
+                image_meta = dict(src_image)
+
+                image = self.compute_api.snapshot_volume_backed(
+                                                       context,
+                                                       instance,
+                                                       image_meta,
+                                                       image_name,
+                                                       extra_properties=props)
+            else:
+                image = self.compute_api.snapshot(context,
+                                                  instance,
+                                                  image_name,
+                                                  extra_properties=props)
         except exception.InstanceInvalidState as state_error:
             common.raise_http_conflict_for_instance_invalid_state(state_error,
-                    'createImage')
+                        'createImage')
 
         # build location of newly-created image entity
         image_id = str(image['id'])
diff --git a/nova/compute/api.py b/nova/compute/api.py
index eb2f0520f..95ad4b7d0 100644
--- a/nova/compute/api.py
+++ b/nova/compute/api.py
@@ -1221,6 +1221,85 @@ class API(base.Base):
                 backup_type=backup_type, rotation=rotation)
         return recv_meta
 
+    @check_instance_state(vm_state=[vm_states.ACTIVE, vm_states.STOPPED])
+    def snapshot_volume_backed(self, context, instance, image_meta, name,
+                               extra_properties=None):
+        """Snapshot the given volume-backed instance.
+
+        :param instance: nova.db.sqlalchemy.models.Instance
+        :param image_meta: metadata for the new image
+        :param name: name of the backup or snapshot
+        :param extra_properties: dict of extra image properties to include
+
+        :returns: the new image metadata
+        """
+        image_meta['name'] = name
+        properties = image_meta['properties']
+        if instance['root_device_name']:
+            properties['root_device_name'] = instance['root_device_name']
+        properties.update(extra_properties or {})
+
+        bdms = self.get_instance_bdms(context, instance)
+
+        mapping = []
+        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')
+            snapshot_id = m.get('snapshot_id')
+            if snapshot_id and volume_id:
+                # create snapshot based on volume_id
+                volume = self.volume_api.get(context, volume_id)
+                # NOTE(yamahata): Should we wait for snapshot creation?
+                #                 Linux LVM snapshot creation completes in
+                #                 short time, it doesn't matter for now.
+                name = _('snapshot for %s') % image_meta['name']
+                snapshot = self.volume_api.create_snapshot_force(
+                    context, volume, name, volume['display_description'])
+                m['snapshot_id'] = snapshot['id']
+                del m['volume_id']
+
+            if m:
+                mapping.append(m)
+
+        for m in block_device.mappings_prepend_dev(properties.get('mappings',
+                                                                  [])):
+            virtual_name = m['virtual']
+            if virtual_name in ('ami', 'root'):
+                continue
+
+            assert block_device.is_swap_or_ephemeral(virtual_name)
+            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'):
+            image_meta.pop(attr, None)
+
+        # the new image is simply a bucket of properties (particularly the
+        # block device mapping, kernel and ramdisk IDs) with no image data,
+        # hence the zero size
+        image_meta['size'] = 0
+
+        return self.image_service.create(context, image_meta, data='')
+
     def _get_minram_mindisk_params(self, context, instance):
         try:
             #try to get source image of the instance
diff --git a/nova/tests/api/openstack/compute/test_server_actions.py b/nova/tests/api/openstack/compute/test_server_actions.py
index f1eafb405..1c7366caa 100644
--- a/nova/tests/api/openstack/compute/test_server_actions.py
+++ b/nova/tests/api/openstack/compute/test_server_actions.py
@@ -618,6 +618,93 @@ class ServerActionsControllerTest(test.TestCase):
         location = response.headers['Location']
         self.assertEqual('http://localhost/v2/fake/images/123', location)
 
+    def _do_test_create_volume_backed_image(self, extra_properties):
+
+        def _fake_id(x):
+            return '%s-%s-%s-%s' % (x * 8, x * 4, x * 4, x * 12)
+
+        body = dict(createImage=dict(name='snapshot_of_volume_backed'))
+
+        if extra_properties:
+            body['createImage']['metadata'] = extra_properties
+
+        image_service = nova.image.glance.get_default_image_service()
+
+        bdm = [dict(snapshot_id=_fake_id('a'),
+                    volume_size=1,
+                    device_name='sda1',
+                    delete_on_termination=False)]
+        props = dict(kernel_id=_fake_id('b'),
+                     ramdisk_id=_fake_id('c'),
+                     root_device_name='/dev/sda1',
+                     block_device_mapping=bdm)
+        original_image = dict(properties=props,
+                              container_format='ami',
+                              status='active',
+                              is_public=True)
+
+        image_service.create(None, original_image)
+
+        def fake_block_device_mapping_get_all_by_instance(context, inst_id):
+            class BDM(object):
+                def __init__(self):
+                    self.no_device = None
+                    self.values = dict(snapshot_id=_fake_id('a'),
+                                       volume_id=_fake_id('d'),
+                                       virtual_name=None,
+                                       volume_size=1,
+                                       device_name='sda1',
+                                       delete_on_termination=False)
+
+                def __getattr__(self, name):
+                    return self.values.get(name)
+
+                def __getitem__(self, key):
+                    return self.values.get(key)
+
+            return [BDM()]
+
+        self.stubs.Set(nova.db, 'block_device_mapping_get_all_by_instance',
+                       fake_block_device_mapping_get_all_by_instance)
+
+        instance = fakes.fake_instance_get(image_ref=original_image['id'],
+                                           vm_state=vm_states.ACTIVE,
+                                           root_device_name='/dev/sda1')
+        self.stubs.Set(nova.db, 'instance_get_by_uuid', instance)
+
+        def fake_volume_get(context, volume_id):
+            return dict(id=volume_id,
+                        size=1,
+                        host='fake',
+                        display_description='fake')
+
+        self.stubs.Set(nova.db, 'volume_get', fake_volume_get)
+
+        req = fakes.HTTPRequest.blank(self.url)
+        response = self.controller._action_create_image(req, FAKE_UUID, body)
+
+        location = response.headers['Location']
+        image_id = location.replace('http://localhost/v2/fake/images/', '')
+        snapshot = image_service.show(None, image_id)
+
+        self.assertEquals(snapshot['name'], 'snapshot_of_volume_backed')
+        properties = snapshot['properties']
+        self.assertEquals(properties['kernel_id'], _fake_id('b'))
+        self.assertEquals(properties['ramdisk_id'], _fake_id('c'))
+        self.assertEquals(properties['root_device_name'], '/dev/sda1')
+        bdms = properties['block_device_mapping']
+        self.assertEquals(len(bdms), 1)
+        self.assertEquals(bdms[0]['device_name'], 'sda1')
+        for k in extra_properties.keys():
+            self.assertEquals(properties[k], extra_properties[k])
+
+    def test_create_volume_backed_image_no_metadata(self):
+        self._do_test_create_volume_backed_image({})
+
+    def test_create_volume_backed_image_with_metadata(self):
+        self._do_test_create_volume_backed_image(dict(ImageType='Gold',
+                                                      ImageVersion='2.0'))
+
     def test_create_image_snapshots_disabled(self):
         """Don't permit a snapshot if the allow_instance_snapshots flag is
         False
diff --git a/nova/tests/api/openstack/fakes.py b/nova/tests/api/openstack/fakes.py
index 5ad2b094a..fd3ffa2fc 100644
--- a/nova/tests/api/openstack/fakes.py
+++ b/nova/tests/api/openstack/fakes.py
@@ -416,7 +416,8 @@ def stub_instance(id, user_id=None, project_id=None, host=None,
                   auto_disk_config=False, display_name=None,
                   include_fake_metadata=True, config_drive=None,
                   power_state=None, nw_cache=None, metadata=None,
-                  security_groups=None, limit=None, marker=None):
+                  security_groups=None, root_device_name=None,
+                  limit=None, marker=None):
 
     if user_id is None:
         user_id = 'fake_user'
@@ -493,7 +494,8 @@ def stub_instance(id, user_id=None, project_id=None, host=None,
         "name": "instance-%s" % id,
         "shutdown_terminate": True,
         "disable_terminate": False,
-        "security_groups": security_groups}
+        "security_groups": security_groups,
+        "root_device_name": root_device_name}
 
     instance.update(info_cache)