Merge "Driver assisted migration on retype when it's safe"

cinder/tests/unit/volume/test_volume_migration.py

@@ -105,6 +105,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
@@ -1015,3 +1061,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:
@@ -3047,7 +3072,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.