Fix retype failure when original has no volume type

cinder.objects.volume.Volume.finish_volume_migration()
should skip the volume_type_id field when swapping fields
between "source" and "dest" volumes.  The "dest" volume
has already been created with appropriate volume_type_id
and the "source" may have not have a volume type.

This commit also changes a LOG.error() to a LOG.exception()
in where finish_volume_migration is called in order to
show the exception traceback in its calling context.

Change-Id: I2caf4d3f4aa088d099548e6e88d1776b4cc5810c
Closes-bug: #1547546

diff --git a/cinder/objects/volume.py b/cinder/objects/volume.py
index ccabee9d327a375a28fa93a743e5151b8a3ce20d..abf1cf9b4d8bb484e363c7f84d2138620d983362 100644 (file)
--- a/cinder/objects/volume.py
+++ b/cinder/objects/volume.py
@@ -394,8 +394,8 @@ class Volume(base.CinderPersistentObject, base.CinderObject,
         # We swap fields between source (i.e. self) and destination at the
         # end of migration because we want to keep the original volume id
         # in the DB but now pointing to the migrated volume.
-        skip = ({'id', 'provider_location', 'glance_metadata'} |
-                set(self.obj_extra_fields))
+        skip = ({'id', 'provider_location', 'glance_metadata',
+                 'volume_type_id'} | set(self.obj_extra_fields))
         for key in set(dest_volume.fields.keys()) - skip:
             # Only swap attributes that are already set.  We do not want to
             # unexpectedly trigger a lazy-load.

diff --git a/cinder/tests/unit/fake_constants.py b/cinder/tests/unit/fake_constants.py
index 1ca3ab200030ffee49961c6df79460f85814a1de..59f452a3f03947b111c236ef1eda859a2aa4350e 100644 (file)
--- a/cinder/tests/unit/fake_constants.py
+++ b/cinder/tests/unit/fake_constants.py
@@ -40,4 +40,5 @@ volume5_id = '17b0e01d-3d2d-4c31-a1aa-c962420bc3dc'
 volume_name_id = 'ee73d33c-52ed-4cb7-a8a9-2687c1205c22'
 volume2_name_id = '63fbdd21-03bc-4309-b867-2893848f86af'
 volume_type_id = '4e9e6d23-eed0-426d-b90a-28f87a94b6fe'
+volume_type2_id = '23defc6f-6d21-4fb5-8d36-b887cbd5a19c'
 will_not_be_found_id = 'ce816f65-c5aa-46d6-bd62-5272752d584a'

diff --git a/cinder/tests/unit/objects/test_volume.py b/cinder/tests/unit/objects/test_volume.py
index a84fa0056c6f0b55c2d85de0c806dad9682cf0d2..2759faabe9a43d42e54e5fdabb090c1f6de0a78e 100644 (file)
--- a/cinder/tests/unit/objects/test_volume.py
+++ b/cinder/tests/unit/objects/test_volume.py
@@ -12,6 +12,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import ddt
 import mock
 import six
 
@@ -25,6 +26,7 @@ from cinder.tests.unit import fake_volume
 from cinder.tests.unit import objects as test_objects
 
 
+@ddt.ddt
 class TestVolume(test_objects.BaseObjectsTestCase):
     @staticmethod
     def _compare(test, db, obj):
@@ -335,9 +337,17 @@ class TestVolume(test_objects.BaseObjectsTestCase):
 
     @mock.patch('cinder.db.volume_metadata_update', return_value={})
     @mock.patch('cinder.db.volume_update')
-    def test_finish_volume_migration(self, volume_update, metadata_update):
-        src_volume_db = fake_volume.fake_db_volume(**{'id': fake.volume_id})
-        dest_volume_db = fake_volume.fake_db_volume(**{'id': fake.volume2_id})
+    @ddt.data({'src_vol_type_id': fake.volume_type_id,
+               'dest_vol_type_id': fake.volume_type2_id},
+              {'src_vol_type_id': None,
+               'dest_vol_type_id': fake.volume_type2_id})
+    @ddt.unpack
+    def test_finish_volume_migration(self, volume_update, metadata_update,
+                                     src_vol_type_id, dest_vol_type_id):
+        src_volume_db = fake_volume.fake_db_volume(
+            **{'id': fake.volume_id, 'volume_type_id': src_vol_type_id})
+        dest_volume_db = fake_volume.fake_db_volume(
+            **{'id': fake.volume2_id, 'volume_type_id': dest_vol_type_id})
         src_volume = objects.Volume._from_db_object(
             self.context, objects.Volume(), src_volume_db,
             expected_attrs=['metadata', 'glance_metadata'])
@@ -352,10 +362,14 @@ class TestVolume(test_objects.BaseObjectsTestCase):
         self.assertEqual(src_volume.id, updated_dest_volume._name_id)
         self.assertTrue(volume_update.called)
 
+        # Ensure that the destination volume type has not been overwritten
+        self.assertEqual(dest_vol_type_id,
+                         getattr(updated_dest_volume, 'volume_type_id'))
         # Ignore these attributes, since they were updated by
         # finish_volume_migration
         ignore_keys = ('id', 'provider_location', '_name_id',
-                       'migration_status', 'display_description', 'status')
+                       'migration_status', 'display_description', 'status',
+                       'volume_type_id')
         dest_vol_filtered = {k: v for k, v in updated_dest_volume.iteritems()
                              if k not in ignore_keys}
         src_vol_filtered = {k: v for k, v in src_volume.iteritems()

diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py
index 102fcdbf45c2d50799cf3a00751fd068de99e89d..fef6343e735d250dde0ea9fc7e2f97af65f5abfa 100644 (file)
--- a/cinder/volume/manager.py
+++ b/cinder/volume/manager.py
@@ -1781,8 +1781,9 @@ class VolumeManager(manager.SchedulerDependentManager):
                                                   new_volume.id)
         except Exception:
             with excutils.save_and_reraise_exception():
-                LOG.error(_LE("Failed to copy volume %(vol1)s to %(vol2)s"),
-                          {'vol1': volume.id, 'vol2': new_volume.id})
+                LOG.exception(_LE(
+                    "Failed to copy volume %(vol1)s to %(vol2)s"), {
+                        'vol1': volume.id, 'vol2': new_volume.id})
                 self._clean_temporary_volume(ctxt, volume,
                                              new_volume)