From c04d5cb892212f322f6c72e0118a66c1c47edb87 Mon Sep 17 00:00:00 2001 From: Peter Penchev Date: Wed, 14 Dec 2022 17:55:56 +0200 Subject: [PATCH] StorPool: fix the retype volume flow Change-Id: I786733aae17dea47e3e6f2a393a947bfb46e70a3 Closes-Bug: #2002995 --- .../unit/volume/drivers/test_storpool.py | 15 ++-- cinder/volume/drivers/storpool.py | 71 +++++++++++++------ .../notes/bug2002995-e423f17eaddae22d.yaml | 9 +++ 3 files changed, 66 insertions(+), 29 deletions(-) create mode 100644 releasenotes/notes/bug2002995-e423f17eaddae22d.yaml diff --git a/cinder/tests/unit/volume/drivers/test_storpool.py b/cinder/tests/unit/volume/drivers/test_storpool.py index 442bdbb8850..02df8134d94 100644 --- a/cinder/tests/unit/volume/drivers/test_storpool.py +++ b/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([]) diff --git a/cinder/volume/drivers/storpool.py b/cinder/volume/drivers/storpool.py index 6c67ae56753..62e058e14c4 100644 --- a/cinder/volume/drivers/storpool.py +++ b/cinder/volume/drivers/storpool.py @@ -338,24 +338,24 @@ 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(): - if k == '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 v[0] != v[1]: - if v[1] is not None: - update['template'] = v[1] - elif templ is not None: - update['template'] = templ - else: - update['replication'] = repl - elif v[0] != v[1]: - LOG.error('Retype of extra_specs "%s" not ' - 'supported yet.', k) + # Check for the StorPool extra specs. We intentionally ignore any + # 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 + 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] + elif templ is not None: + update['template'] = templ + else: + update['replication'] = repl if update: name = self._attach.volumeName(volume['id']) @@ -380,21 +380,46 @@ 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}) - return {'_name_id': new_volume['_name_id'] or new_volume['id']} - else: + 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 %(tname)s to %(oname)s: ' + 'could not rename a volume: ' '%(err)s', - {'tname': temp_name, 'oname': orig_name, 'err': e}) + {'err': e}) return {'_name_id': new_volume['_name_id'] or new_volume['id']} + + try: + self._attach.api().volumeUpdate(temp_name, + {'rename': orig_name}) + return {'_name_id': None} + except spapi.ApiError as e: + LOG.error('StorPool update_migrated_volume(): ' + 'could not rename %(tname)s to %(oname)s: ' + '%(err)s', + {'tname': temp_name, 'oname': orig_name, 'err': e}) + return {'_name_id': new_volume['_name_id'] or new_volume['id']} diff --git a/releasenotes/notes/bug2002995-e423f17eaddae22d.yaml b/releasenotes/notes/bug2002995-e423f17eaddae22d.yaml new file mode 100644 index 00000000000..9f979790cac --- /dev/null +++ b/releasenotes/notes/bug2002995-e423f17eaddae22d.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + StorPool driver `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.