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

This reverts commit 5edc77a18c.

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.

Change-Id: I893105cbd270300be9ec48b3127e66022f739314

cinder/tests/unit/volume/test_volume_migration.py

@@ -104,52 +104,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
@@ -1062,21 +1016,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': ('<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,35 +2553,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,
+                       new_type_id=None) -> None:
-                       diff=None) -> None:
         """Migrate the volume to the specified host (called on source host)."""
         try:
             # NOTE(flaper87): Verify the driver is enabled
@@ -2602,7 +2579,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,
@@ -2621,8 +2598,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:
@@ -3059,7 +3034,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,

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

@@ -1,7 +0,0 @@
----
-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.