From b8610a01b61cf86db2f31e26eb0d4dab4a812ace Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Thu, 9 Jan 2025 09:45:27 -0800 Subject: [PATCH] Driver assisted migration on retype when it's safe 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 Closes-Bug: #1886543 Change-Id: I0c6b2c584e8e0053ad740ee25f29ca1d442bdeea --- .../unit/volume/test_volume_migration.py | 83 +++++++++++++++++++ cinder/volume/manager.py | 36 +++++++- ...e-assisted-migration-6cdc7f9b21beb859.yaml | 7 ++ 3 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/retype-assisted-migration-6cdc7f9b21beb859.yaml diff --git a/cinder/tests/unit/volume/test_volume_migration.py b/cinder/tests/unit/volume/test_volume_migration.py index f60698495c3..232f0e205bf 100644 --- a/cinder/tests/unit/volume/test_volume_migration.py +++ b/cinder/tests/unit/volume/test_volume_migration.py @@ -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': (' 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) diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index d691710ddd1..7ee0a562556 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -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, diff --git a/releasenotes/notes/retype-assisted-migration-6cdc7f9b21beb859.yaml b/releasenotes/notes/retype-assisted-migration-6cdc7f9b21beb859.yaml new file mode 100644 index 00000000000..54e513d8a17 --- /dev/null +++ b/releasenotes/notes/retype-assisted-migration-6cdc7f9b21beb859.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + `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.