From 5edc77a18c505e22ee56e3ebda6d400cded6fa32 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Mon, 6 Jul 2020 18:28:24 +0200 Subject: [PATCH] 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 --- .../unit/volume/test_volume_migration.py | 64 +++++++++++++++++++ cinder/volume/manager.py | 31 ++++++++- ...e-assisted-migration-6cdc7f9b21beb859.yaml | 7 ++ 3 files changed, 99 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 dc827df7fe1..ddbf9841ae3 100644 --- a/cinder/tests/unit/volume/test_volume_migration.py +++ b/cinder/tests/unit/volume/test_volume_migration.py @@ -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': (' 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 c85190f1a63..62884fc8ed6 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -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, 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.