Raise exception when Glance metadata not found.

It'd be better to raise exception when trying to copy Glance metadata
from source (volume/snapshot) to destination (volume/snapshot) rather
than silent failure, which is exactly the reason there's unspotted error
inside glance_meta unittest but it was able to pass.
With this fix, one should _not_ directly call glance_metadata_copy()
without looking at source's bootable flags.  This patch also refactors
_create_volume_from_snapshot() and _create_from_source_volume() to only
do Glance metadata copy when needed (bootable is True).

Fix bug: 1211632

Change-Id: I28f065e21cf24c81f98b00e171fcbe4f64ec76b6

diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py
index a8149807171e97d3b6a3079f426ca58269719aa0..e588b7562e55902ab1cd61895ca3f06f41fe8e7e 100644 (file)
--- a/cinder/db/sqlalchemy/api.py
+++ b/cinder/db/sqlalchemy/api.py
@@ -1802,11 +1802,16 @@ def volume_type_extra_specs_update_or_create(context, volume_type_id,
 @require_context
 @require_volume_exists
 def _volume_glance_metadata_get(context, volume_id, session=None):
-    return model_query(context, models.VolumeGlanceMetadata, session=session).\
+    rows = model_query(context, models.VolumeGlanceMetadata, session=session).\
         filter_by(volume_id=volume_id).\
         filter_by(deleted=False).\
         all()
 
+    if not rows:
+        raise exception.GlanceMetadataNotFound(id=volume_id)
+
+    return rows
+
 
 @require_context
 @require_volume_exists
@@ -1819,11 +1824,16 @@ def volume_glance_metadata_get(context, volume_id):
 @require_context
 @require_snapshot_exists
 def _volume_snapshot_glance_metadata_get(context, snapshot_id, session=None):
-    return model_query(context, models.VolumeGlanceMetadata, session=session).\
+    rows = model_query(context, models.VolumeGlanceMetadata, session=session).\
         filter_by(snapshot_id=snapshot_id).\
         filter_by(deleted=False).\
         all()
 
+    if not rows:
+        raise exception.GlanceMetadataNotFound(id=snapshot_id)
+
+    return rows
+
 
 @require_context
 @require_snapshot_exists

diff --git a/cinder/exception.py b/cinder/exception.py
index d49bc0daac44096ac2ba950b43498ba37da5b33b..ef5261fdb6afb2f273856c499327972bd1b1f0b3 100644 (file)
--- a/cinder/exception.py
+++ b/cinder/exception.py
@@ -552,6 +552,10 @@ class GlanceMetadataExists(Invalid):
                 " exists for volume id %(volume_id)s")
 
 
+class GlanceMetadataNotFound(NotFound):
+    message = _("Glance metadata for volume/snapshot %(id)s cannot be found.")
+
+
 class ExportFailure(Invalid):
     message = _("Failed to export for volume: %(reason)s")
 

diff --git a/cinder/tests/test_volume_glance_metadata.py b/cinder/tests/test_volume_glance_metadata.py
index 5aa42d8b1c92a242b45742b4a2f3d97239f1b274..bc8afb96dcd73674e0ee87746a1c098fcec629b2 100644 (file)
--- a/cinder/tests/test_volume_glance_metadata.py
+++ b/cinder/tests/test_volume_glance_metadata.py
@@ -92,11 +92,8 @@ class VolumeGlanceMetadataTestCase(test.TestCase):
         vol_metadata = db.volume_glance_metadata_create(ctxt, 1, 'key1',
                                                         'value1')
         db.volume_glance_metadata_delete_by_volume(ctxt, 1)
-        metadata = db.volume_glance_metadata_get(ctxt, 1)
-        self.assertEqual(len(metadata), 0)
-        db.volume_glance_metadata_delete_by_volume(ctxt, 1)
-        metadata = db.volume_glance_metadata_get(ctxt, 1)
-        self.assertEqual(len(metadata), 0)
+        self.assertRaises(exception.GlanceMetadataNotFound,
+                          db.volume_glance_metadata_get, ctxt, 1)
 
     def test_vol_glance_metadata_copy_to_snapshot(self):
         ctxt = context.get_admin_context()
@@ -120,10 +117,9 @@ class VolumeGlanceMetadataTestCase(test.TestCase):
         db.volume_create(ctxt, {'id': 100, 'source_volid': 1})
         vol_meta = db.volume_glance_metadata_create(ctxt, 1, 'key1',
                                                     'value1')
-        db.volume_glance_metadata_copy_from_volume_to_volume(ctxt, 100, 1)
+        db.volume_glance_metadata_copy_from_volume_to_volume(ctxt, 1, 100)
 
-        expected_meta = {'id': '100',
-                         'key': 'key1',
+        expected_meta = {'key': 'key1',
                          'value': 'value1'}
 
         for meta in db.volume_glance_metadata_get(ctxt, 100):

diff --git a/cinder/volume/flows/create_volume.py b/cinder/volume/flows/create_volume.py
index fb11e178cb2cb4f243d36c147729bcaa869e3a81..ec87060a8f03fcb18918d8ac458342c41b339154 100644 (file)
--- a/cinder/volume/flows/create_volume.py
+++ b/cinder/volume/flows/create_volume.py
@@ -1176,18 +1176,20 @@ class CreateVolumeFromSpecTask(CinderTask):
             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)
+            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)
         return model_update
 
     def _enable_bootable_flag(self, context, volume_id):
@@ -1214,22 +1216,24 @@ class CreateVolumeFromSpecTask(CinderTask):
         # will not destroy the volume (although they could in the future).
         if srcvol_ref.bootable:
             self._enable_bootable_flag(context, volume_ref['id'])
-        try:
-            msg = _('Copying metadata from source volume %(source_volid)s'
-                    ' to cloned volume %(clone_vol_id)s')
-            LOG.debug(msg % {'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)
+            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)
         return model_update
 
     def _copy_image_to_volume(self, context, volume_ref,

diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index 40fe197507c75cd75da78f6932fd9358b7f42577..89b517c396d24ca93d79dc6228a1c0194f3876a1 100644 (file)
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -293,9 +293,19 @@ class VolumeManager(manager.SchedulerDependentManager):
         self.db.snapshot_update(context,
                                 snapshot_ref['id'], {'status': 'available',
                                                      'progress': '100%'})
-        self.db.volume_glance_metadata_copy_to_snapshot(context,
-                                                        snapshot_ref['id'],
-                                                        volume_id)
+
+        vol_ref = self.db.volume_get(context, volume_id)
+        if vol_ref.bootable:
+            try:
+                self.db.volume_glance_metadata_copy_to_snapshot(
+                    context, snapshot_ref['id'], volume_id)
+            except exception.CinderException as ex:
+                LOG.exception(_("Failed updating %(snapshot_id)s"
+                                " metadata using the provided volumes"
+                                " %(volume_id)s metadata") %
+                              {'volume_id': volume_id,
+                               'snapshot_id': snapshot_id})
+                raise exception.MetadataCopyFailure(reason=ex)
         LOG.info(_("snapshot %s: created successfully"), snapshot_ref['name'])
         self._notify_about_snapshot_usage(context, snapshot_ref, "create.end")
         return snapshot_id