Driver assisted migration on retype when it's safe
When doing a retype of a volume that requires a migration, the manager only uses driver assisted migration when the source and the destination belong to the same backend (different pools). Driver assisted migration should also be tried for other cases, just like when we do a normal migration. One case were this would be beneficial is when doing migrations from one pool to another on the same storage system on single pool drivers (such as RBD/Ceph). This patch checks what are the changes between the types to see if it is safe to use driver assisted migration (from the perspective of keeping the resulting volume consistent with the volume type) and when it is it tries to use it. If driver assisted migration indicates that it couldn't move the volume, then we go with the generic volume migration like we used to. Closes-Bug: #1886543 Change-Id: I2532cfc9b98788a1a1e765f07d0c9f8c98bc77f6
This commit is contained in:
Gorka Eguileor
parent
2462abec42
commit
5edc77a18c
@ -104,6 +104,52 @@ class VolumeMigrationTestCase(base.BaseVolumeTestCase):
|
||||
self.assertEqual('newhost', volume.host)
|
||||
self.assertEqual('success', volume.migration_status)
|
||||
|
||||
@mock.patch('cinder.volume.manager.VolumeManager.'
|
||||
'_can_use_driver_migration')
|
||||
def test_migrate_volume_driver_for_retype(self, mock_can_use):
|
||||
"""Test volume migration done by driver on a retype."""
|
||||
# Mock driver and rpc functions
|
||||
mock_driver = self.mock_object(self.volume.driver, 'migrate_volume',
|
||||
return_value=(True, {}))
|
||||
|
||||
volume = tests_utils.create_volume(self.context, size=0,
|
||||
host=CONF.host,
|
||||
migration_status='migrating')
|
||||
host_obj = {'host': 'newhost', 'capabilities': {}}
|
||||
self.volume.migrate_volume(self.context, volume, host_obj, False,
|
||||
fake.VOLUME_TYPE2_ID, mock.sentinel.diff)
|
||||
|
||||
mock_can_use.assert_called_once_with(mock.sentinel.diff)
|
||||
mock_driver.assert_called_once_with(self.context, volume, host_obj)
|
||||
# check volume properties
|
||||
volume = objects.Volume.get_by_id(context.get_admin_context(),
|
||||
volume.id)
|
||||
self.assertEqual('newhost', volume.host)
|
||||
self.assertEqual('success', volume.migration_status)
|
||||
self.assertEqual(fake.VOLUME_TYPE2_ID, volume.volume_type_id)
|
||||
|
||||
@mock.patch('cinder.volume.manager.VolumeManager._migrate_volume_generic')
|
||||
@mock.patch('cinder.volume.manager.VolumeManager.'
|
||||
'_can_use_driver_migration')
|
||||
def test_migrate_volume_driver_for_retype_generic(self, mock_can_use,
|
||||
mock_generic):
|
||||
"""Test generic volume migration on a retype after driver can't."""
|
||||
# Mock driver and rpc functions
|
||||
mock_driver = self.mock_object(self.volume.driver, 'migrate_volume',
|
||||
return_value=(False, None))
|
||||
|
||||
volume = tests_utils.create_volume(self.context, size=0,
|
||||
host=CONF.host,
|
||||
migration_status='migrating')
|
||||
host_obj = {'host': 'newhost', 'capabilities': {}}
|
||||
self.volume.migrate_volume(self.context, volume, host_obj, False,
|
||||
fake.VOLUME_TYPE2_ID, mock.sentinel.diff)
|
||||
|
||||
mock_can_use.assert_called_once_with(mock.sentinel.diff)
|
||||
mock_driver.assert_called_once_with(self.context, volume, host_obj)
|
||||
mock_generic.assert_called_once_with(self.context, volume, host_obj,
|
||||
fake.VOLUME_TYPE2_ID)
|
||||
|
||||
def test_migrate_volume_driver_cross_az(self):
|
||||
"""Test volume migration done by driver."""
|
||||
# Mock driver and rpc functions
|
||||
@ -1016,3 +1062,21 @@ class VolumeMigrationTestCase(base.BaseVolumeTestCase):
|
||||
self.volume.retype(self.context, volume, new_vol_type.id,
|
||||
host_obj, migration_policy='on-demand')
|
||||
vt_get.assert_not_called()
|
||||
|
||||
@ddt.data(
|
||||
(None, True),
|
||||
({'encryption': {'cipher': ('v1', 'v2')}}, False),
|
||||
({'qos_specs': {'key1': ('v1', 'v2')}}, False),
|
||||
({'encryption': {}, 'qos_specs': {}, 'extra_specs': {}}, True),
|
||||
({'encryption': {}, 'qos_specs': {},
|
||||
'extra_specs': {'volume_backend_name': ('ceph1', 'ceph2'),
|
||||
'RESKEY:availability_zones': ('nova', 'nova2')}},
|
||||
True),
|
||||
({'encryption': {}, 'qos_specs': {},
|
||||
'extra_specs': {'thin_provisioning_support': ('<is> True', None)}},
|
||||
False),
|
||||
)
|
||||
@ddt.unpack
|
||||
def test__can_use_driver_migration(self, diff, expected):
|
||||
res = self.volume._can_use_driver_migration(diff)
|
||||
self.assertEqual(expected, res)
|
||||
|
||||
@ -2553,12 +2553,35 @@ class VolumeManager(manager.CleanableManager,
|
||||
resource=volume)
|
||||
return volume.id
|
||||
|
||||
def _can_use_driver_migration(self, diff):
|
||||
"""Return when we can use driver assisted migration on a retype."""
|
||||
# We can if there's no retype or there are no difference in the types
|
||||
if not diff:
|
||||
return True
|
||||
|
||||
extra_specs = diff.get('extra_specs')
|
||||
qos = diff.get('qos_specs')
|
||||
enc = diff.get('encryption')
|
||||
|
||||
# We cant' if QoS or Encryption changes and we can if there are no
|
||||
# extra specs changes.
|
||||
if qos or enc or not extra_specs:
|
||||
return not (qos or enc)
|
||||
|
||||
# We can use driver assisted migration if we only change the backend
|
||||
# name, and the AZ.
|
||||
extra_specs = extra_specs.copy()
|
||||
extra_specs.pop('volume_backend_name', None)
|
||||
extra_specs.pop('RESKEY:availability_zones', None)
|
||||
return not extra_specs
|
||||
|
||||
def migrate_volume(self,
|
||||
ctxt: context.RequestContext,
|
||||
volume,
|
||||
host,
|
||||
force_host_copy: bool = False,
|
||||
new_type_id=None) -> None:
|
||||
new_type_id=None,
|
||||
diff=None) -> None:
|
||||
"""Migrate the volume to the specified host (called on source host)."""
|
||||
try:
|
||||
# NOTE(flaper87): Verify the driver is enabled
|
||||
@ -2579,7 +2602,7 @@ class VolumeManager(manager.CleanableManager,
|
||||
|
||||
volume.migration_status = 'migrating'
|
||||
volume.save()
|
||||
if not force_host_copy and new_type_id is None:
|
||||
if not force_host_copy and self._can_use_driver_migration(diff):
|
||||
try:
|
||||
LOG.debug("Issue driver.migrate_volume.", resource=volume)
|
||||
moved, model_update = self.driver.migrate_volume(ctxt,
|
||||
@ -2598,6 +2621,8 @@ class VolumeManager(manager.CleanableManager,
|
||||
updates.update(status_update)
|
||||
if model_update:
|
||||
updates.update(model_update)
|
||||
if new_type_id:
|
||||
updates['volume_type_id'] = new_type_id
|
||||
volume.update(updates)
|
||||
volume.save()
|
||||
except Exception:
|
||||
@ -3034,7 +3059,7 @@ class VolumeManager(manager.CleanableManager,
|
||||
|
||||
try:
|
||||
self.migrate_volume(context, volume, host,
|
||||
new_type_id=new_type_id)
|
||||
new_type_id=new_type_id, diff=diff)
|
||||
except Exception:
|
||||
with excutils.save_and_reraise_exception():
|
||||
_retype_error(context, volume, old_reservations,
|
||||
|
||||
@ -0,0 +1,7 @@
|
||||
---
|
||||
fixes:
|
||||
- |
|
||||
`Bug #1886543 <https://bugs.launchpad.net/cinder/+bug/1886543>`_:
|
||||
On retypes requiring a migration, try to use the driver assisted mechanism
|
||||
when moving from one backend to another when we know it's safe from the
|
||||
volume type perspective.
|
||||
Loading…
Reference in New Issue
Block a user