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.