StorPool: fix the retype volume flow

Change-Id: I786733aae17dea47e3e6f2a393a947bfb46e70a3
Closes-Bug: #2002995

cinder/tests/unit/volume/drivers/test_storpool.py

@@ -356,12 +356,6 @@ class StorPoolTestCase(test.TestCase):
                                                  'available')
         self.assertDictEqual({'_name_id': '1'}, res)
 
-        # Failure: a volume with the original volume's name already exists
-        res = self.driver.update_migrated_volume(None, {'id': '1'},
-                                                 {'id': '2', '_name_id': '1'},
-                                                 'available')
-        self.assertDictEqual({'_name_id': '1'}, res)
-
         # Success: rename the migrated volume to match the original
         res = self.driver.update_migrated_volume(None, {'id': '3'},
                                                  {'id': '2', '_name_id': '3'},
@@ -371,6 +365,15 @@ class StorPoolTestCase(test.TestCase):
                               volumes.keys())
         self.assertVolumeNames(('1', '3',))
 
+        # Success: swap volume names with an existing volume
+        res = self.driver.update_migrated_volume(None, {'id': '1'},
+                                                 {'id': '3', '_name_id': '1'},
+                                                 'available')
+        self.assertDictEqual({'_name_id': None}, res)
+        self.assertCountEqual([volumeName('1'), volumeName('3')],
+                              volumes.keys())
+        self.assertVolumeNames(('1', '3',))
+
         for vid in ('1', '3'):
             self.driver.delete_volume({'id': vid})
         self.assertVolumeNames([])

cinder/volume/drivers/storpool.py

@@ -338,13 +338,17 @@ class StorPoolDriver(driver.VolumeDriver):
         templ = self.configuration.storpool_template
         repl = self.configuration.storpool_replication
         if diff['extra_specs']:
-            for (k, v) in diff['extra_specs'].items():
+            # Check for the StorPool extra specs. We intentionally ignore any
-                if k == 'volume_backend_name':
+            # other extra_specs because the cinder scheduler should not even
+            # call us if there's a serious mismatch between the volume types."
+            if diff['extra_specs'].get('volume_backend_name'):
+                v = diff['extra_specs'].get('volume_backend_name')
                 if v[0] != v[1]:
                     # Retype of a volume backend not supported yet,
                     # the volume needs to be migrated.
                     return False
-                elif k == 'storpool_template':
+            if diff['extra_specs'].get('storpool_template'):
+                v = diff['extra_specs'].get('storpool_template')
                 if v[0] != v[1]:
                     if v[1] is not None:
                         update['template'] = v[1]
@@ -352,10 +356,6 @@ class StorPoolDriver(driver.VolumeDriver):
                         update['template'] = templ
                     else:
                         update['replication'] = repl
-                elif v[0] != v[1]:
-                    LOG.error('Retype of extra_specs "%s" not '
-                              'supported yet.', k)
-                    return False
 
         if update:
             name = self._attach.volumeName(volume['id'])
@@ -380,14 +380,39 @@ class StorPoolDriver(driver.VolumeDriver):
                       'created as part of the migration from '
                       '"%(oid)s".', {'tid': temp_id, 'oid': orig_id})
             return {'_name_id': new_volume['_name_id'] or new_volume['id']}
-        elif orig_name in vols:
+
-            LOG.error('StorPool update_migrated_volume(): both '
+        if orig_name in vols:
+            LOG.debug('StorPool update_migrated_volume(): both '
                       'the original volume "%(oid)s" and the migrated '
                       'StorPool volume "%(tid)s" seem to exist on '
                       'the StorPool cluster.',
                       {'oid': orig_id, 'tid': temp_id})
+            int_name = temp_name + '--temp--mig'
+            LOG.debug('Trying to swap volume names, intermediate "%(int)s"',
+                      {'int': int_name})
+            try:
+                LOG.debug('- rename "%(orig)s" to "%(int)s"',
+                          {'orig': orig_name, 'int': int_name})
+                self._attach.api().volumeUpdate(orig_name,
+                                                {'rename': int_name})
+
+                LOG.debug('- rename "%(temp)s" to "%(orig)s"',
+                          {'temp': temp_name, 'orig': orig_name})
+                self._attach.api().volumeUpdate(temp_name,
+                                                {'rename': orig_name})
+
+                LOG.debug('- rename "%(int)s" to "%(temp)s"',
+                          {'int': int_name, 'temp': temp_name})
+                self._attach.api().volumeUpdate(int_name,
+                                                {'rename': temp_name})
+                return {'_name_id': None}
+            except spapi.ApiError as e:
+                LOG.error('StorPool update_migrated_volume(): '
+                          'could not rename a volume: '
+                          '%(err)s',
+                          {'err': e})
                 return {'_name_id': new_volume['_name_id'] or new_volume['id']}
-        else:
+
         try:
             self._attach.api().volumeUpdate(temp_name,
                                             {'rename': orig_name})

releasenotes/notes/bug2002995-e423f17eaddae22d.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    StorPool driver `bug #2002995
+    <https://bugs.launchpad.net/cinder/+bug/2002995>`_: When retyping a
+    volume on a StorPool backend to a different volume type also on that
+    StorPool backend but using a different StorPool template, occasionally
+    the retype operation would fail or the old volume could be left attached
+    to a StorPool client. This issue has been fixed in this release.