From d8062063a35ab2307edbe36826f350f2767ea08d Mon Sep 17 00:00:00 2001 From: Alan Bishop Date: Mon, 18 May 2020 18:56:43 -0700 Subject: [PATCH] 3PAR - Fix renaming volume after migration After migrating a volume, if the original and new volumes are on the same 3PAR backend, then the volume names are swapped. However, if the original volume is not in the same 3PAR backend, the new volume is renamed to the original volume's name. Closes-Bug: 1858119 Change-Id: I1f9d4143d1ad2637a2173090b290f6f96b64c492 --- .../unit/volume/drivers/hpe/test_hpe3par.py | 101 ++++++++++++++++-- cinder/volume/drivers/hpe/hpe_3par_common.py | 68 +++++++++--- ...-3par-migrate-rename-662d984e070a1de2.yaml | 8 ++ 3 files changed, 154 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/fix-3par-migrate-rename-662d984e070a1de2.yaml diff --git a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py index e2fcce802d8..9c92cacb8d1 100644 --- a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py +++ b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py @@ -18,6 +18,7 @@ import ast import copy from unittest import mock +import ddt from oslo_config import cfg from oslo_utils import units from oslo_utils import uuidutils @@ -818,6 +819,7 @@ class HPE3PARBaseDriver(test.TestCase): result) +@ddt.ddt class TestHPE3PARDriverBase(HPE3PARBaseDriver): def setup_driver(self, config=None, mock_conf=None, wsapi_version=None): @@ -3165,7 +3167,14 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): expected = [] mock_client.assert_has_calls(expected) - def test_update_migrated_volume(self): + @ddt.data({'temp_rename_side_effect': None, + 'rename_side_effect': None}, + {'temp_rename_side_effect': hpeexceptions.HTTPNotFound, + 'rename_side_effect': None}) + @ddt.unpack + def test_update_migrated_volume(self, + temp_rename_side_effect, + rename_side_effect): mock_client = self.setup_driver() fake_old_volume = {'id': self.VOLUME_ID} provider_location = 'foo' @@ -3176,26 +3185,100 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): with mock.patch.object(hpecommon.HPE3PARCommon, '_create_client') as mock_create_client: mock_create_client.return_value = mock_client + mock_client.modifyVolume.side_effect = [ + temp_rename_side_effect, + rename_side_effect, + None + ] actual_update = self.driver.update_migrated_volume( context.get_admin_context(), fake_old_volume, fake_new_volume, original_volume_status) - expected_update = {'_name_id': None, - 'provider_location': None} + if rename_side_effect is None: + expected_update = {'_name_id': None, + 'provider_location': None} + else: + expected_update = {'_name_id': fake_new_volume['_name_id'], + 'provider_location': provider_location} self.assertEqual(expected_update, actual_update) + # Initial temp rename always takes place expected = [ mock.call.modifyVolume( 'osv-0DM4qZEVSKON-DXN-NwVpw', {'newName': u'tsv-0DM4qZEVSKON-DXN-NwVpw'}), - mock.call.modifyVolume( - 'osv-0DM4qZEVSKON-AAAAAAAAA', - {'newName': u'osv-0DM4qZEVSKON-DXN-NwVpw'}), - mock.call.modifyVolume( - 'tsv-0DM4qZEVSKON-DXN-NwVpw', - {'newName': u'osv-0DM4qZEVSKON-AAAAAAAAA'}) ] + # Primary rename will occur unless the temp rename fails + if temp_rename_side_effect != hpeexceptions.HTTPConflict: + expected += [ + mock.call.modifyVolume( + 'osv-0DM4qZEVSKON-AAAAAAAAA', + {'newName': u'osv-0DM4qZEVSKON-DXN-NwVpw'}), + ] + + # Final temp rename will occur if both of the previous renames + # succeed. + if (temp_rename_side_effect is None and + rename_side_effect is None): + expected += [ + mock.call.modifyVolume( + 'tsv-0DM4qZEVSKON-DXN-NwVpw', + {'newName': u'osv-0DM4qZEVSKON-AAAAAAAAA'}) + ] + + mock_client.assert_has_calls( + self.standard_login + + expected + + self.standard_logout) + + @ddt.data({'temp_rename_side_effect': hpeexceptions.HTTPConflict, + 'rename_side_effect': None}, + {'temp_rename_side_effect': None, + 'rename_side_effect': hpeexceptions.HTTPConflict}, + {'temp_rename_side_effect': hpeexceptions.HTTPNotFound, + 'rename_side_effect': hpeexceptions.HTTPConflict}) + @ddt.unpack + def test_update_migrated_volume_failed(self, + temp_rename_side_effect, + rename_side_effect): + mock_client = self.setup_driver() + fake_old_volume = {'id': self.VOLUME_ID} + provider_location = 'foo' + fake_new_volume = {'id': self.CLONE_ID, + '_name_id': self.CLONE_ID, + 'provider_location': provider_location} + original_volume_status = 'available' + with mock.patch.object(hpecommon.HPE3PARCommon, + '_create_client') as mock_create_client: + mock_create_client.return_value = mock_client + mock_client.modifyVolume.side_effect = [ + temp_rename_side_effect, + rename_side_effect, + None + ] + self.assertRaises(hpeexceptions.HTTPConflict, + self.driver.update_migrated_volume, + context.get_admin_context(), + fake_old_volume, + fake_new_volume, + original_volume_status) + + # Initial temp rename always takes place + expected = [ + mock.call.modifyVolume( + 'osv-0DM4qZEVSKON-DXN-NwVpw', + {'newName': u'tsv-0DM4qZEVSKON-DXN-NwVpw'}), + ] + + # Primary rename will occur unless the temp rename fails + if temp_rename_side_effect != hpeexceptions.HTTPConflict: + expected += [ + mock.call.modifyVolume( + 'osv-0DM4qZEVSKON-AAAAAAAAA', + {'newName': u'osv-0DM4qZEVSKON-DXN-NwVpw'}), + ] + mock_client.assert_has_calls( self.standard_login + expected + diff --git a/cinder/volume/drivers/hpe/hpe_3par_common.py b/cinder/volume/drivers/hpe/hpe_3par_common.py index 4e23c15b55d..7bc32d3c9cc 100644 --- a/cinder/volume/drivers/hpe/hpe_3par_common.py +++ b/cinder/volume/drivers/hpe/hpe_3par_common.py @@ -2867,30 +2867,70 @@ class HPE3PARCommon(object): """ LOG.debug("Update volume name for %(id)s", {'id': new_volume['id']}) - name_id = None - provider_location = None + new_volume_renamed = False if original_volume_status == 'available': # volume isn't attached and can be updated original_name = self._get_3par_vol_name(volume['id']) - temp_name = self._get_3par_vol_name(volume['id'], temp_vol=True) current_name = self._get_3par_vol_name(new_volume['id']) + temp_name = self._get_3par_vol_name(volume['id'], temp_vol=True) + + # In case the original volume is on the same backend, try + # renaming it to a temporary name. + original_volume_renamed = False + try: + volumeTempMods = {'newName': temp_name} + self.client.modifyVolume(original_name, volumeTempMods) + original_volume_renamed = True + except hpeexceptions.HTTPNotFound: + pass + except Exception as e: + LOG.error("Changing the original volume name from %(orig)s to " + "%(temp)s failed because %(reason)s", + {'orig': original_name, 'temp': temp_name, + 'reason': e}) + raise + + # change the current volume name to the original try: volumeMods = {'newName': original_name} - volumeTempMods = {'newName': temp_name} - volumeCurrentMods = {'newName': current_name} - # swap volume name in backend - self.client.modifyVolume(original_name, volumeTempMods) self.client.modifyVolume(current_name, volumeMods) - self.client.modifyVolume(temp_name, volumeCurrentMods) - LOG.info("Volume name changed from %(tmp)s to %(orig)s", - {'tmp': current_name, 'orig': original_name}) + new_volume_renamed = True + LOG.info("Current volume changed from %(cur)s to %(orig)s", + {'cur': current_name, 'orig': original_name}) except Exception as e: - LOG.error("Changing the volume name from %(tmp)s to " + LOG.error("Changing the migrating volume name from %(cur)s to " "%(orig)s failed because %(reason)s", - {'tmp': current_name, 'orig': original_name, + {'cur': current_name, 'orig': original_name, 'reason': e}) - name_id = new_volume['_name_id'] or new_volume['id'] - provider_location = new_volume['provider_location'] + if original_volume_renamed: + LOG.error("To restore the original volume, it must be " + "manually renamed from %(temp)s to %(orig)s", + {'temp': temp_name, 'orig': original_name}) + raise + + # If it was renamed, rename the original volume again to the + # migrated volume's name (effectively swapping the names). If + # this operation fails, the newly migrated volume is OK but the + # original volume (with the temp name) may need to be manually + # cleaned up on the backend. + if original_volume_renamed: + try: + volumeCurrentMods = {'newName': current_name} + self.client.modifyVolume(temp_name, volumeCurrentMods) + except Exception as e: + LOG.error("Changing the original volume name from " + "%(tmp)s to %(cur)s failed because %(reason)s", + {'tmp': temp_name, 'cur': current_name, + 'reason': e}) + # Don't fail the migration, but help the user fix the + # source volume stuck in error_deleting. + LOG.error("To delete the original volume, it must be " + "manually renamed from %(temp)s to %(orig)s", + {'temp': temp_name, 'orig': current_name}) + + if new_volume_renamed: + name_id = None + provider_location = None else: # the backend can't change the name. name_id = new_volume['_name_id'] or new_volume['id'] diff --git a/releasenotes/notes/fix-3par-migrate-rename-662d984e070a1de2.yaml b/releasenotes/notes/fix-3par-migrate-rename-662d984e070a1de2.yaml new file mode 100644 index 00000000000..059595f2eb7 --- /dev/null +++ b/releasenotes/notes/fix-3par-migrate-rename-662d984e070a1de2.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Fix the HPE 3PAR driver's attempt to rename the backend volume after + it was migrated. If the original volume resides on the same 3PAR backend + then the pre and post migration volume names are swapped. Otherwise, the + newly migrated volume is renamed to match the original name. + (bug 1858119)