openstack/deb-cinder
Code Issues Proposed changes

Fix retype failure when original has no volume type

Browse Source 
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
This commit is contained in:
Tom Barron 2016-02-19 18:19:14 -05:00 committed by Ryan McNair
parent e0c98f0f68
commit 492b350a3f
4 changed files with 24 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
cinder/objects/volume.py
View File

@ -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.

1
cinder/tests/unit/fake_constants.py
View File

@ -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'

22
cinder/tests/unit/objects/test_volume.py
View File

@ -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()

5
cinder/volume/manager.py
View File

@ -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)