From f9f6d99e55250ea8627545fd2aa2965ee5ca91d4 Mon Sep 17 00:00:00 2001 From: Eric Harney Date: Wed, 25 Oct 2023 16:12:01 +0000 Subject: [PATCH] Revert "Driver assisted migration on retype when it's safe" This reverts commit 5edc77a18c505e22ee56e3ebda6d400cded6fa32. Reason for revert: Bug: #2019190 This revealed a bug in Cinder optimized migration code that can lead to data loss with RBD and potentially other drivers, by using the optimized migration path in more cases. Once the issues there are fixed, this should be re-introduced. Depends-On: https://review.opendev.org/c/openstack/cinder/+/900671 Change-Id: I893105cbd270300be9ec48b3127e66022f739314 (cherry picked from commit 13196c700cf36b996204853a90573b1a67139764) (cherry picked from commit 8d453fa7b6daf1c1b6105a71f7c660b9036541f3) --- .../unit/volume/test_volume_migration.py | 64 ------------------- cinder/volume/manager.py | 31 +-------- ...e-assisted-migration-6cdc7f9b21beb859.yaml | 7 -- 3 files changed, 3 insertions(+), 99 deletions(-) delete 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 965ee09792a..73a8fbc72b8 100644 --- a/cinder/tests/unit/volume/test_volume_migration.py +++ b/cinder/tests/unit/volume/test_volume_migration.py @@ -105,52 +105,6 @@ 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 @@ -1068,21 +1022,3 @@ 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 c2c52ec82be..104291563d0 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -2622,35 +2622,12 @@ 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, - diff=None) -> None: + new_type_id=None) -> None: """Migrate the volume to the specified host (called on source host).""" try: volume_utils.require_driver_initialized(self.driver) @@ -2668,7 +2645,7 @@ class VolumeManager(manager.CleanableManager, volume.migration_status = 'migrating' volume.save() - if not force_host_copy and self._can_use_driver_migration(diff): + if not force_host_copy and new_type_id is None: try: LOG.debug("Issue driver.migrate_volume.", resource=volume) moved, model_update = self.driver.migrate_volume(ctxt, @@ -2687,8 +2664,6 @@ 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: @@ -3146,7 +3121,7 @@ class VolumeManager(manager.CleanableManager, try: self.migrate_volume(context, volume, host, - new_type_id=new_type_id, diff=diff) + new_type_id=new_type_id) 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 deleted file mode 100644 index 54e513d8a17..00000000000 --- a/releasenotes/notes/retype-assisted-migration-6cdc7f9b21beb859.yaml +++ /dev/null @@ -1,7 +0,0 @@ ---- -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.