From 492b350a3fec957263430a1e67457a7386254c61 Mon Sep 17 00:00:00 2001 From: Tom Barron 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