From 492b350a3fec957263430a1e67457a7386254c61 Mon Sep 17 00:00:00 2001
From: Tom Barron <tpb@dyncloud.net>
Date: Fri, 19 Feb 2016 18:19:14 -0500
Subject: [PATCH] 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
---
cinder/objects/volume.py | 4 ++--
cinder/tests/unit/fake_constants.py | 1 +
cinder/tests/unit/objects/test_volume.py | 22 ++++++++++++++++++----
cinder/volume/manager.py | 5 +++--
4 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/cinder/objects/volume.py b/cinder/objects/volume.py
index ccabee9d3..abf1cf9b4 100644
--- 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 1ca3ab200..59f452a3f 100644
--- 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 a84fa0056..2759faabe 100644
--- 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 102fcdbf4..fef6343e7 100644
--- 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)
--
2.45.2