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)