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

cinder/tests/unit/volume/test_volume_migration.py

@@ -104,6 +104,52 @@ def test_migrate_volume_driver(self):
         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 @@ def test_retype_volume_not_capable_to_replica(self):
             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)

cinder/volume/manager.py

@@ -2553,12 +2553,35 @@ def migrate_volume_completion(self,
                  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 @@ def migrate_volume(self,
 
         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 @@ def migrate_volume(self,
                         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 @@ def _retype_error(context, volume, old_reservations,
 
             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,

releasenotes/notes/retype-assisted-migration-6cdc7f9b21beb859.yaml

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