openstack/cinder
Code Issues Proposed changes

Driver assisted migration on retype when it's safe

Browse Source 
This is a revision of I2532cfc9b98788a1a1e765f07d0c9f8c98bc77f6
that corrects the issue that forced it to be reverted by
I893105cbd270300be9ec48b3127e66022f739314. The revised code avoids
attempting driver assisted migration when the volume has attachments.

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

As long as the volume has no attachments, 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.

Co-Authored-By: Alan Bishop <abishop@redhat.com>
Closes-Bug: #1886543
Change-Id: I0c6b2c584e8e0053ad740ee25f29ca1d442bdeea
This commit is contained in:
Gorka Eguileor 2025-01-09 09:45:27 -08:00 committed by Alan Bishop
parent 962fe29e77
commit b8610a01b6
3 changed files with 123 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

83
cinder/tests/unit/volume/test_volume_migration.py
View File

@ -105,6 +105,71 @@ 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)
@mock.patch('cinder.volume.manager.VolumeManager._migrate_volume_generic')
def test_migrate_volume_driver_attached_volume(self, mock_generic):
"""Test driver volume migration with an attachment."""
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')
volume = tests_utils.attach_volume(
self.context, volume, fake.INSTANCE_ID, 'host', '/dev/vda')
host_obj = {'host': 'newhost', 'capabilities': {}}
self.volume.migrate_volume(self.context, volume, host_obj, False,
fake.VOLUME_TYPE2_ID)
# Driver assisted migration should not be attempted when the volume
# has attachments.
mock_driver.assert_not_called()
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
@ -1024,3 +1089,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)

36
cinder/volume/manager.py
View File

@ -2613,12 +2613,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:
volume_utils.require_driver_initialized(self.driver)
@ -2636,7 +2659,12 @@ class VolumeManager(manager.CleanableManager,
volume.migration_status = 'migrating'
volume.save()
if not force_host_copy and new_type_id is None:
# Do not attempt driver assisted migration when the volume has
# attachments. Nova must be involved when migrating an attached
# volume, and that's handled by the generic migration code.
if (len(volume.volume_attachment) == 0 and
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,
@ -2655,6 +2683,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:
@ -3129,7 +3159,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,

7
releasenotes/notes/retype-assisted-migration-6cdc7f9b21beb859.yaml Normal file
View File

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