Set bootable flag for volume cloned from image

In previous code we don't set the bootable flag for volume
cloned from image. This does not appear to break booting
from volume at present but the status displayed by the cinder
client is broken.

The bug is reported and fixed several months ago(bug #1185533)
but the author doesn't provide unit test with the patch. Now it
has been broken again by somebody else.The patch re-fixed this
bug together with unit tests.

Also add a function named _handle_bootable_volume_glance_meta,
it is a combination of enable_bootable_flag and glance_metadata
handling. There are 3 kinds of volume creation tasks may required
to copy/fetch the glance metadata. In previous code every kind of
task have its own handling code, this patch consolidate them together
to share some common code.

fixed bug #1211272
fixed bug #1185533

Change-Id: I86007e26efbebe58dc0c5995b6f14f68258144b5

diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py
index aa91f3d9e0cf98c870d9cc25b10a223fe205ec24..d7b64c464392dd3597082307c4ac31f989b189c2 100644 (file)
--- a/cinder/tests/test_volume.py
+++ b/cinder/tests/test_volume.py
@@ -913,9 +913,13 @@ class VolumeTestCase(test.TestCase):
         self.volume.delete_snapshot(self.context, snapshot_id)
         self.volume.delete_volume(self.context, volume_id)
 
-    def _create_volume_from_image(self, fakeout_copy_image_to_volume=False):
-        """Call copy image to volume, Test the status of volume after calling
-        copying image to volume.
+    def _create_volume_from_image(self, fakeout_copy_image_to_volume=False,
+                                  fakeout_clone_image=False):
+        """Test function of create_volume_from_image.
+
+        Test cases call this function to create a volume from image, caller
+        can choose whether to fake out copy_image_to_volume and conle_image,
+        after calling this, test cases should check status of the volume.
         """
         def fake_local_path(volume):
             return dst_path
@@ -927,9 +931,14 @@ class VolumeTestCase(test.TestCase):
         def fake_fetch_to_raw(context, image_service, image_id, vol_path):
             pass
 
+        def fake_clone_image(volume_ref, image_location, image_id):
+            return {'provider_location': None}, True
+
         dst_fd, dst_path = tempfile.mkstemp()
         os.close(dst_fd)
         self.stubs.Set(self.volume.driver, 'local_path', fake_local_path)
+        if fakeout_clone_image:
+            self.stubs.Set(self.volume.driver, 'clone_image', fake_clone_image)
         self.stubs.Set(image_utils, 'fetch_to_raw', fake_fetch_to_raw)
         if fakeout_copy_image_to_volume:
             self.stubs.Set(self.volume, '_copy_image_to_volume',
@@ -948,17 +957,31 @@ class VolumeTestCase(test.TestCase):
             volume = db.volume_get(self.context, volume_id)
             return volume
 
-    def test_create_volume_from_image_status_available(self):
-        """Verify that before copying image to volume, it is in available
-        state.
+    def test_create_volume_from_image_cloned_status_available(self):
+        """Test create volume from image via cloning.
+
+        Verify that after cloning image to volume, it is in available
+        state and is bootable.
         """
         volume = self._create_volume_from_image()
         self.assertEqual(volume['status'], 'available')
+        self.assertEqual(volume['bootable'], True)
+        self.volume.delete_volume(self.context, volume['id'])
+
+    def test_create_volume_from_image_not_cloned_status_available(self):
+        """Test create volume from image via full copy.
+
+        Verify that after copying image to volume, it is in available
+        state and is bootable.
+        """
+        volume = self._create_volume_from_image(fakeout_clone_image=True)
+        self.assertEqual(volume['status'], 'available')
+        self.assertEqual(volume['bootable'], True)
         self.volume.delete_volume(self.context, volume['id'])
 
     def test_create_volume_from_image_exception(self):
-        """Verify that create volume from image, the volume status is
-        'downloading'.
+        """Verify that create volume from a non-existing image, the volume
+        status is 'error' and is not bootable.
         """
         dst_fd, dst_path = tempfile.mkstemp()
         os.close(dst_fd)
@@ -984,6 +1007,7 @@ class VolumeTestCase(test.TestCase):
                           image_id)
         volume = db.volume_get(self.context, volume_id)
         self.assertEqual(volume['status'], "error")
+        self.assertEqual(volume['bootable'], False)
         # cleanup
         db.volume_destroy(self.context, volume_id)
         os.unlink(dst_path)

diff --git a/cinder/volume/flows/create_volume.py b/cinder/volume/flows/create_volume.py
index 1677cd98f2bb1b61aa418f410a87baf6db443b23..cfbdccc7441321ca6b0dc70f24bb1d67b70e7a77 100644 (file)
--- a/cinder/volume/flows/create_volume.py
+++ b/cinder/volume/flows/create_volume.py
@@ -1215,6 +1215,60 @@ class CreateVolumeFromSpecTask(CinderTask):
         }
         self.host = host
 
+    def _handle_bootable_volume_glance_meta(self, context, volume_id,
+                                            **kwargs):
+        """Enable bootable flag and properly handle glance metadata.
+
+        Caller should provide one and only one of snapshot_id,source_volid
+        and image_id. If an image_id specified, a image_meta should also be
+        provided, otherwise will be treated as an empty dictionary.
+        """
+
+        log_template = _("Copying metadata from %(src_type)s %(src_id)s to "
+                         "%(vol_id)s")
+        exception_template = _("Failed updating volume %(vol_id)s metadata"
+                               " using the provided %(src_type)s"
+                               " %(src_id)s metadata")
+        src_type = None
+        src_id = None
+        self._enable_bootable_flag(context, volume_id)
+        try:
+            if kwargs.get('snapshot_id'):
+                src_type = 'snapshot'
+                src_id = kwargs['snapshot_id']
+                snapshot_id = src_id
+                LOG.debug(log_template % {'src_type': src_type,
+                                          'src_id': src_id,
+                                          'vol_id': volume_id})
+                self.db.volume_glance_metadata_copy_to_volume(
+                    context, volume_id, snapshot_id)
+            elif kwargs.get('source_volid'):
+                src_type = 'source volume'
+                src_id = kwargs['source_volid']
+                source_volid = src_id
+                LOG.debug(log_template % {'src_type': src_type,
+                                          'src_id': src_id,
+                                          'vol_id': volume_id})
+                self.db.volume_glance_metadata_copy_from_volume_to_volume(
+                    context,
+                    source_volid,
+                    volume_id)
+            elif kwargs.get('image_id'):
+                src_type = 'image'
+                src_id = kwargs['image_id']
+                image_id = src_id
+                image_meta = kwargs.get('image_meta', {})
+                LOG.debug(log_template % {'src_type': src_type,
+                                          'src_id': src_id,
+                                          'vol_id': volume_id})
+                self._capture_volume_image_metadata(context, volume_id,
+                                                    image_id, image_meta)
+        except exception.CinderException as ex:
+            LOG.exception(exception_template % {'src_type': src_type,
+                                                'src_id': src_id,
+                                                'vol_id': volume_id})
+            raise exception.MetadataCopyFailure(reason=ex)
+
     def _create_from_snapshot(self, context, volume_ref, snapshot_id,
                               **kwargs):
         volume_id = volume_ref['id']
@@ -1237,21 +1291,8 @@ class CreateVolumeFromSpecTask(CinderTask):
                            'snapshot_ref_id': snapshot_ref['volume_id']})
             raise exception.MetadataUpdateFailure(reason=ex)
         if make_bootable:
-            self._enable_bootable_flag(context, volume_id)
-            try:
-                LOG.debug(_("Copying metadata from snapshot "
-                            "%(snap_volume_id)s to %(volume_id)s") %
-                          {'snap_volume_id': snapshot_id,
-                           'volume_id': volume_id})
-                self.db.volume_glance_metadata_copy_to_volume(
-                    context, volume_id, snapshot_id)
-            except exception.CinderException as ex:
-                LOG.exception(_("Failed updating volume %(volume_id)s "
-                                "metadata using the provided glance "
-                                "snapshot %(snapshot_id)s metadata") %
-                              {'volume_id': volume_id,
-                               'snapshot_id': snapshot_id})
-                raise exception.MetadataCopyFailure(reason=ex)
+            self._handle_bootable_volume_glance_meta(context, volume_id,
+                                                     snapshot_id=snapshot_id)
         return model_update
 
     def _enable_bootable_flag(self, context, volume_id):
@@ -1277,25 +1318,8 @@ class CreateVolumeFromSpecTask(CinderTask):
         # point the volume has already been created and further failures
         # will not destroy the volume (although they could in the future).
         if srcvol_ref.bootable:
-            self._enable_bootable_flag(context, volume_ref['id'])
-            try:
-                LOG.debug(_('Copying metadata from source volume '
-                            '%(source_volid)s to cloned volume '
-                            '%(clone_vol_id)s') % {
-                                'source_volid': source_volid,
-                                'clone_vol_id': volume_ref['id'],
-                            })
-                self.db.volume_glance_metadata_copy_from_volume_to_volume(
-                    context,
-                    source_volid,
-                    volume_ref['id'])
-            except exception.CinderException as ex:
-                LOG.exception(_("Failed updating cloned volume %(volume_id)s"
-                                " metadata using the provided source volumes"
-                                " %(source_volid)s metadata") %
-                              {'volume_id': volume_ref['id'],
-                               'source_volid': source_volid})
-                raise exception.MetadataCopyFailure(reason=ex)
+            self._handle_bootable_volume_glance_meta(context, volume_ref['id'],
+                                                     source_volid=source_volid)
         return model_update
 
     def _copy_image_to_volume(self, context, volume_ref,
@@ -1334,8 +1358,6 @@ class CreateVolumeFromSpecTask(CinderTask):
 
     def _capture_volume_image_metadata(self, context, volume_id,
                                        image_id, image_meta):
-        if not image_meta:
-            image_meta = {}
 
         # Save some base attributes into the volume metadata
         base_metadata = {
@@ -1391,7 +1413,6 @@ class CreateVolumeFromSpecTask(CinderTask):
         # and clone status.
         model_update, cloned = self.driver.clone_image(
             volume_ref, image_location, image_id)
-        make_bootable = False
         if not cloned:
             # TODO(harlowja): what needs to be rolled back in the clone if this
             # volume create fails?? Likely this should be a subflow or broken
@@ -1413,19 +1434,10 @@ class CreateVolumeFromSpecTask(CinderTask):
                                'updates': updates})
             self._copy_image_to_volume(context, volume_ref,
                                        image_id, image_location, image_service)
-            make_bootable = True
-        if make_bootable:
-            self._enable_bootable_flag(context, volume_ref['id'])
-        try:
-            self._capture_volume_image_metadata(context, volume_ref['id'],
-                                                image_id, image_meta)
-        except exception.CinderException as ex:
-            LOG.exception(_("Failed updating volume %(volume_id)s metadata"
-                            " using the provided image metadata"
-                            " %(image_meta)s from image %(image_id)s") %
-                          {'volume_id': volume_ref['id'],
-                           'image_meta': image_meta, 'image_id': image_id})
-            raise exception.MetadataUpdateFailure(reason=ex)
+
+        self._handle_bootable_volume_glance_meta(context, volume_ref['id'],
+                                                 image_id=image_id,
+                                                 image_meta=image_meta)
         return model_update
 
     def _create_raw_volume(self, context, volume_ref, **kwargs):