From cfde6bd1c0129de86ce123c15742d948624b478f Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 19 May 2020 14:01:13 +0200 Subject: [PATCH] 3PAR: Fix live migration After live migrating a volume into the 3PAR driver the volume would be unusable. And we would no longer be able to do any operation with it. Migrated in-use volumes cannot be renamed when migrated and therefore need to use the "_name_id" field of the Volume object (volumes table in the DB) to store the original volume's ID so the driver can locate the volume. The problem is that, even though the 3PAR driver sets the "_name_id" field, it doesn't use it for any operation, so once the volume has been migrated and the ID of the volume no longer matches the one that was used to create the volume (value now in "_name_id") it will no longer be able to locate the volume. This patch adds the logic necessary to locate volumes using the "_name_id" field. Closes-Bug: #1697422 Change-Id: I27b9d4be419e1b52db02d4525b292ceeb5d5f867 (cherry picked from commit fc51678298a38dcd94bcfe92c590b38fdb7193e4) --- .../unit/volume/drivers/hpe/test_hpe3par.py | 435 +++++++++++------- cinder/volume/drivers/hpe/hpe_3par_common.py | 314 ++++++++----- cinder/volume/drivers/hpe/hpe_3par_iscsi.py | 8 +- ...-3par-live-migration-0065bd2626fdb4a1.yaml | 7 + 4 files changed, 482 insertions(+), 282 deletions(-) create mode 100644 releasenotes/notes/fix-3par-live-migration-0065bd2626fdb4a1.yaml diff --git a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py index 4d0680f1650..d2d9d186987 100644 --- a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py +++ b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py @@ -112,6 +112,7 @@ class HPE3PARBaseDriver(test.TestCase): SNAPSHOT_ID = '2f823bdc-e36e-4dc8-bd15-de1c7a28ff31' SNAPSHOT_NAME = 'snapshot-2f823bdc-e36e-4dc8-bd15-de1c7a28ff31' VOLUME_3PAR_NAME = 'osv-0DM4qZEVSKON-DXN-NwVpw' + VOLUME_NAME_ID_3PAR_NAME = 'osv-L4I73ONuTci9Fd4ceij-MQ' SNAPSHOT_3PAR_NAME = 'oss-L4I73ONuTci9Fd4ceij-MQ' RCG_3PAR_NAME = 'rcg-0DM4qZEVSKON-DXN-N' RCG_3PAR_GROUP_NAME = 'rcg-YET.38iJR1KQDyA50k' @@ -177,6 +178,15 @@ class HPE3PARBaseDriver(test.TestCase): volume_type_id=None, multiattach=False) + volume_name_id = fake_volume.fake_volume_obj( + context.get_admin_context(), + id=VOLUME_ID, + _name_id='2f823bdc-e36e-4dc8-bd15-de1c7a28ff31', + size=2, + host=FAKE_CINDER_HOST, + volume_type=None, + volume_type_id=None) + volume_src_cg = {'name': SRC_CG_VOLUME_NAME, 'id': SRC_CG_VOLUME_ID, 'display_name': 'Foo Volume', @@ -278,6 +288,14 @@ class HPE3PARBaseDriver(test.TestCase): 'volume_type': None, 'volume_type_id': 'hos'} + snapshot_volume = {'name': VOLUME_NAME, + 'id': VOLUME_ID_SNAP, + 'display_name': 'Foo Volume', + 'size': 2, + 'host': FAKE_CINDER_HOST, + 'volume_type': None, + 'volume_type_id': None} + snapshot = {'name': SNAPSHOT_NAME, 'id': SNAPSHOT_ID, 'user_id': USER_ID, @@ -289,7 +307,15 @@ class HPE3PARBaseDriver(test.TestCase): 'volume_size': 2, 'display_name': 'fakesnap', 'display_description': FAKE_DESC, - 'volume': volume} + 'volume': snapshot_volume} + + snapshot_name_id = {'id': SNAPSHOT_ID, + 'volume_id': volume_name_id.id, + 'volume_size': 2, + 'volume': volume_name_id, + 'display_name': 'display-name', + 'display_description': 'description', + 'volume_name': 'name'} wwn = ["123456789012345", "123456789054321"] @@ -669,21 +695,19 @@ class HPE3PARBaseDriver(test.TestCase): standard_logout = [ mock.call.logout()] - class fake_volume_object(object): - def __init__(self, vol_id='d03338a9-9115-48a3-8dfc-35cdfcdc15a7'): - self.id = vol_id - self.name = 'volume-d03338a9-9115-48a3-8dfc-35cdfcdc15a7' - self.display_name = 'Foo Volume' - self.size = 2 - self.host = 'fakehost@foo#OpenStackCPG' - self.volume_type = None - self.volume_type_id = None - self.volume = {'volume_type_id': self.volume_type_id, - 'host': self.host, 'id': self.id, 'volume_type': - self.volume_type} - - def get(self, parm): - return self.volume[parm] + @staticmethod + def fake_volume_object(vol_id='d03338a9-9115-48a3-8dfc-35cdfcdc15a7', + **kwargs): + values = dict(id=vol_id, + name='volume-%s' % vol_id, + display_name='Foo Volume', + size=2, + host='fakehost@foo#OpenStackCPG', + volume_type=None, + volume_type_id=None) + values.update(kwargs) + return fake_volume.fake_volume_obj(context.get_admin_context(), + **values) class fake_group_object(object): def __init__(self, grp_id='6044fedf-c889-4752-900f-2039d247a5df'): @@ -2345,16 +2369,18 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): mock.call.getTask(1)] mock_client.assert_has_calls(expected) - def test_delete_volume(self): + @ddt.data('volume', 'volume_name_id') + def test_delete_volume(self, volume_attr): # setup_mock_client drive with default configuration # and return the mock HTTP 3PAR client mock_client = self.setup_driver() with mock.patch.object(hpecommon.HPE3PARCommon, '_create_client') as mock_create_client: mock_create_client.return_value = mock_client - self.driver.delete_volume(self.volume) + self.driver.delete_volume(getattr(self, volume_attr)) - expected = [mock.call.deleteVolume(self.VOLUME_3PAR_NAME)] + name_3par = getattr(self, volume_attr.upper() + '_3PAR_NAME') + expected = [mock.call.deleteVolume(name_3par)] mock_client.assert_has_calls( self.standard_login + @@ -2538,7 +2564,10 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): self.standard_login + expected) - def test_create_cloned_volume(self): + @ddt.data('volume', 'volume_name_id') + def test_create_cloned_volume(self, volume_attr): + src_vref = getattr(self, volume_attr) + vol_name = getattr(self, volume_attr.upper() + '_3PAR_NAME') # setup_mock_client drive with default configuration # and return the mock HTTP 3PAR client mock_client = self.setup_driver() @@ -2558,14 +2587,9 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): 'size': 2, 'host': volume_utils.append_host(self.FAKE_HOST, HPE3PAR_CPG2), - 'source_volid': HPE3PARBaseDriver.VOLUME_ID} - src_vref = {'id': HPE3PARBaseDriver.VOLUME_ID, - 'name': HPE3PARBaseDriver.VOLUME_NAME, - 'size': 2, 'status': 'available'} + 'source_volid': src_vref.id} model_update = self.driver.create_cloned_volume(volume, src_vref) self.assertIsNone(model_update) - common = hpecommon.HPE3PARCommon(None) - vol_name = common._get_3par_vol_name(src_vref['id']) # snapshot name is random snap_name = mock.ANY optional = mock.ANY @@ -3175,61 +3199,109 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): temp_rename_side_effect, rename_side_effect): mock_client = self.setup_driver() - fake_old_volume = {'id': self.VOLUME_ID} + mock_client.modifyVolume.side_effect = [temp_rename_side_effect, + rename_side_effect, + None] + fake_old_volume = self.fake_volume_object(self.VOLUME_ID) provider_location = 'foo' - fake_new_volume = {'id': self.CLONE_ID, - '_name_id': self.CLONE_ID, - 'provider_location': provider_location} + fake_new_volume = self.fake_volume_object( + 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 - ] - actual_update = self.driver.update_migrated_volume( - context.get_admin_context(), fake_old_volume, - fake_new_volume, original_volume_status) - 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) + _3common = hpecommon.HPE3PARCommon + self.mock_object(_3common, '_create_client', return_value=mock_client) + mock_get_comment = self.mock_object(_3common, '_get_updated_comment', + side_effect=[mock.sentinel.comm1, + mock.sentinel.comm2]) - # Initial temp rename always takes place - expected = [ + actual_update = self.driver.update_migrated_volume( + context.get_admin_context(), fake_old_volume, + fake_new_volume, original_volume_status) + + 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'}) + ] + comment_expected = [] + + # Primary rename will occur unless the temp rename fails + if temp_rename_side_effect != hpeexceptions.HTTPConflict: + expected += [ mock.call.modifyVolume( - 'osv-0DM4qZEVSKON-DXN-NwVpw', - {'newName': u'tsv-0DM4qZEVSKON-DXN-NwVpw'}), + 'osv-0DM4qZEVSKON-AAAAAAAAA', + {'newName': u'osv-0DM4qZEVSKON-DXN-NwVpw', + 'comment': mock.sentinel.comm1}) ] + comment_expected.append(mock.call('osv-0DM4qZEVSKON-AAAAAAAAA', + volume_id=self.VOLUME_ID, + _name_id=None)) - # 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', + 'comment': mock.sentinel.comm2}) + ] + comment_expected.append(mock.call('osv-0DM4qZEVSKON-DXN-NwVpw', + volume_id=self.CLONE_ID, + _name_id=None)) - # 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) - mock_client.assert_has_calls( - self.standard_login + - expected + - self.standard_logout) + mock_get_comment.assert_has_calls(comment_expected) + + def test_update_migrated_volume_with_name_id(self): + """We don't use temp rename mechanism when source uses _name_id.""" + mock_client = self.setup_driver() + fake_old_volume = self.fake_volume_object( + self.VOLUME_ID, _name_id=self.SRC_CG_VOLUME_ID) + fake_new_volume = self.fake_volume_object(self.CLONE_ID) + + _3common = hpecommon.HPE3PARCommon + self.mock_object(_3common, '_create_client', return_value=mock_client) + mock_get_comment = self.mock_object(_3common, '_get_updated_comment', + side_effect=[mock.sentinel.comm]) + + actual_update = self.driver.update_migrated_volume( + context.get_admin_context(), fake_old_volume, + fake_new_volume, 'available') + expected_update = {'_name_id': None, + 'provider_location': None} + self.assertEqual(expected_update, actual_update) + + # # After successfully swapping names we have updated the comments + mock_get_comment.assert_called_once_with('osv-0DM4qZEVSKON-AAAAAAAAA', + volume_id=self.VOLUME_ID, + _name_id=None), + + expected = [ + mock.call.modifyVolume('osv-0DM4qZEVSKON-AAAAAAAAA', + {'newName': u'osv-0DM4qZEVSKON-DXN-NwVpw', + 'comment': mock.sentinel.comm}), + ] + + mock_client.assert_has_calls( + self.standard_login + + expected + + self.standard_logout) @ddt.data({'temp_rename_side_effect': hpeexceptions.HTTPConflict, 'rename_side_effect': None}, @@ -3248,83 +3320,111 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): '_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) + _3common = hpecommon.HPE3PARCommon + self.mock_object(_3common, '_create_client', return_value=mock_client) + mock_get_comment = self.mock_object(_3common, '_get_updated_comment', + side_effect=[mock.sentinel.comm]) + mock_update_comment = self.mock_object(_3common, '_update_comment') + 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': self.CLONE_ID, + 'provider_location': provider_location} + self.assertEqual(expected_update, actual_update) - # Initial temp rename always takes place - expected = [ + # 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-DXN-NwVpw', - {'newName': u'tsv-0DM4qZEVSKON-DXN-NwVpw'}), + 'osv-0DM4qZEVSKON-AAAAAAAAA', + {'newName': u'osv-0DM4qZEVSKON-DXN-NwVpw', + 'comment': mock.sentinel.comm}), ] + mock_get_comment.assert_called_once_with( + 'osv-0DM4qZEVSKON-AAAAAAAAA', + volume_id=self.VOLUME_ID, + _name_id=None) + else: + mock_get_comment.assert_not_called() - # 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_update_comment.assert_called_once_with( + 'osv-0DM4qZEVSKON-AAAAAAAAA', + volume_id=self.VOLUME_ID, + _name_id=self.CLONE_ID) - mock_client.assert_has_calls( - self.standard_login + - expected + - self.standard_logout) + mock_client.assert_has_calls( + self.standard_login + + expected + + self.standard_logout) def test_update_migrated_volume_attached(self): mock_client = self.setup_driver() + mock_client.getVolume.return_value = { + 'comment': '{"volume_id": %s, "_name_id": ""}' % self.CLONE_ID} + # Simulate old volume had already been live migrated fake_old_volume = {'id': self.VOLUME_ID} provider_location = 'foo' + fake_new_volume = {'id': self.CLONE_ID, - '_name_id': self.CLONE_ID, + '_name_id': '', 'provider_location': provider_location} original_volume_status = 'in-use' - with mock.patch.object(hpecommon.HPE3PARCommon, - '_create_client') as mock_create_client: - mock_create_client.return_value = mock_client - actual_update = self.driver.update_migrated_volume( - context.get_admin_context(), fake_old_volume, - fake_new_volume, original_volume_status) + _3common = hpecommon.HPE3PARCommon + self.mock_object(_3common, '_create_client', return_value=mock_client) + mock_update = self.mock_object(_3common, '_update_comment') + actual_update = self.driver.update_migrated_volume( + context.get_admin_context(), fake_old_volume, + fake_new_volume, original_volume_status) - expected_update = {'_name_id': fake_new_volume['_name_id'], - 'provider_location': provider_location} - self.assertEqual(expected_update, actual_update) + expected_update = {'_name_id': fake_new_volume['id'], + 'provider_location': provider_location} + self.assertEqual(expected_update, actual_update) - def test_create_snapshot(self): + vol_name = _3common._get_3par_vol_name(fake_new_volume) + mock_update.assert_called_once_with(vol_name, + volume_id=fake_old_volume['id'], + _name_id=fake_new_volume['id']) + + @ddt.data(('snapshot', 'osv-dh-F5VGRTseuujPjbeRBVg'), + ('snapshot_name_id', HPE3PARBaseDriver.VOLUME_NAME_ID_3PAR_NAME)) + @ddt.unpack + def test_create_snapshot(self, snapshot_attr, vol_name): # setup_mock_client drive with default configuration # and return the mock HTTP 3PAR client mock_client = self.setup_driver() + snapshot = getattr(self, snapshot_attr) with mock.patch.object(hpecommon.HPE3PARCommon, '_create_client') as mock_create_client: mock_create_client.return_value = mock_client - self.driver.create_snapshot(self.snapshot) + self.driver.create_snapshot(snapshot) - comment = Comment({ - "volume_id": "761fc5e5-5191-4ec7-aeba-33e36de44156", - "display_name": "fakesnap", - "description": "test description name", - "volume_name": - "volume-d03338a9-9115-48a3-8dfc-35cdfcdc15a7", - }) + comment = { + "volume_id": snapshot['volume_id'], + "display_name": snapshot['display_name'], + "description": snapshot['display_description'], + "volume_name": snapshot['volume_name'], + } + if snapshot['volume'].get('_name_id'): + comment["_name_id"] = snapshot['volume']['_name_id'] expected = [ mock.call.createSnapshot( 'oss-L4I73ONuTci9Fd4ceij-MQ', - 'osv-dh-F5VGRTseuujPjbeRBVg', + vol_name, { - 'comment': comment, + 'comment': Comment(comment), 'readOnly': True})] mock_client.assert_has_calls( @@ -3332,17 +3432,13 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): expected + self.standard_logout) - def test_revert_to_snapshot(self): + @ddt.data(('snapshot', 'osv-dh-F5VGRTseuujPjbeRBVg'), + ('snapshot_name_id', HPE3PARBaseDriver.VOLUME_NAME_ID_3PAR_NAME)) + @ddt.unpack + def test_revert_to_snapshot(self, snapshot_attr, vol_name): + snapshot = getattr(self, snapshot_attr) # setup_mock_client drive with default configuration # and return the mock HTTP 3PAR client - volume = {'name': self.VOLUME_NAME, - 'id': self.VOLUME_ID_SNAP, - 'display_name': 'Foo Volume', - 'size': 2, - 'host': self.FAKE_CINDER_HOST, - 'volume_type': None, - 'volume_type_id': None} - mock_client = self.setup_driver() mock_client.isOnlinePhysicalCopy.return_value = False mock_client.promoteVirtualCopy.return_value = {'taskid': 1} @@ -3351,10 +3447,11 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): with mock.patch.object(hpecommon.HPE3PARCommon, '_create_client') as mock_create_client: mock_create_client.return_value = mock_client - self.driver.revert_to_snapshot(self.ctxt, volume, self.snapshot) + self.driver.revert_to_snapshot(self.ctxt, snapshot['volume'], + snapshot) expected = [ - mock.call.isOnlinePhysicalCopy('osv-dh-F5VGRTseuujPjbeRBVg'), + mock.call.isOnlinePhysicalCopy(vol_name), mock.call.promoteVirtualCopy('oss-L4I73ONuTci9Fd4ceij-MQ', optional={}), mock.call.getTask(1) @@ -6161,7 +6258,6 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): vol_ss_enable.return_value = True mock_client = self.setup_driver() mock_client.getStorageSystemInfo.return_value = {'id': self.CLIENT_ID} - volume = self.fake_volume_object() type_info = {'cpg': 'OpenStackCPG', 'tpvv': True, 'tdvv': False, @@ -6170,7 +6266,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): typ_info.return_value = type_info source_volume = self.volume_src_cg - volume.volume['source_volid'] = source_volume['id'] + volume = self.fake_volume_object(source_volid=source_volume['id']) common = hpecommon.HPE3PARCommon(None) vol_name = common._get_3par_vol_name(volume.id) mock_client.getVolume.return_value = {'copyOf': vol_name} @@ -6267,7 +6363,6 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): task_id = 1 mock_client.copyVolume.return_value = {'taskid': task_id} mock_client.getStorageSystemInfo.return_value = {'id': self.CLIENT_ID} - volume = self.fake_volume_object() type_info = {'cpg': 'OpenStackCPG', 'tpvv': True, 'tdvv': False, @@ -6276,7 +6371,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): typ_info.return_value = type_info source_volume = self.volume_src_cg - volume.volume['source_volid'] = source_volume['id'] + volume = self.fake_volume_object(source_volid=source_volume['id']) common = hpecommon.HPE3PARCommon(None) vol_name = common._get_3par_vol_name(volume.id) mock_client.getVolume.return_value = {'copyOf': vol_name} @@ -6987,6 +7082,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): self.assertIn(new_key, properties) +@ddt.ddt class TestHPE3PARFCDriver(HPE3PARBaseDriver): properties = { @@ -7026,7 +7122,10 @@ class TestHPE3PARFCDriver(HPE3PARBaseDriver): mock_client.reset_mock() return mock_client - def test_initialize_connection(self): + @ddt.data('volume', 'volume_name_id') + def test_initialize_connection(self, volume_attr): + volume = getattr(self, volume_attr) + vol_name = getattr(self, volume_attr.upper() + '_3PAR_NAME') # setup_mock_client drive with default configuration # and return the mock HTTP 3PAR client mock_client = self.setup_driver() @@ -7060,16 +7159,16 @@ class TestHPE3PARFCDriver(HPE3PARBaseDriver): mock_client.getHostVLUNs.side_effect = [ hpeexceptions.HTTPNotFound('fake'), [{'active': True, - 'volumeName': self.VOLUME_3PAR_NAME, + 'volumeName': vol_name, 'remoteName': self.wwn[1], 'lun': 90, 'type': 0}], [{'active': True, - 'volumeName': self.VOLUME_3PAR_NAME, + 'volumeName': vol_name, 'remoteName': self.wwn[0], 'lun': 90, 'type': 0}]] location = ("%(volume_name)s,%(lun_id)s,%(host)s,%(nsp)s" % - {'volume_name': self.VOLUME_3PAR_NAME, + {'volume_name': vol_name, 'lun_id': 90, 'host': self.FAKE_HOST, 'nsp': 'something'}) @@ -7089,11 +7188,11 @@ class TestHPE3PARFCDriver(HPE3PARBaseDriver): '_create_client') as mock_create_client: mock_create_client.return_value = mock_client result = self.driver.initialize_connection( - self.volume, + volume, self.connector_multipath_enabled) expected = [ - mock.call.getVolume(self.VOLUME_3PAR_NAME), + mock.call.getVolume(vol_name), mock.call.getCPG(HPE3PAR_CPG), mock.call.getHost(self.FAKE_HOST), mock.call.queryHost(wwns=['123456789012345', @@ -7102,7 +7201,7 @@ class TestHPE3PARFCDriver(HPE3PARBaseDriver): mock.call.getPorts(), mock.call.getHostVLUNs(self.FAKE_HOST), mock.call.createVLUN( - self.VOLUME_3PAR_NAME, + vol_name, auto=True, hostname=self.FAKE_HOST, lun=None), @@ -7582,13 +7681,16 @@ class TestHPE3PARFCDriver(HPE3PARBaseDriver): self.standard_logout) self.assertDictEqual(expected_properties, result) - def test_terminate_connection(self): + @ddt.data('volume', 'volume_name_id') + def test_terminate_connection(self, volume_attr): + volume = getattr(self, volume_attr) + vol_name = getattr(self, volume_attr.upper() + '_3PAR_NAME') # setup_mock_client drive with default configuration # and return the mock HTTP 3PAR client mock_client = self.setup_driver() effects = [ - [{'active': False, 'volumeName': self.VOLUME_3PAR_NAME, + [{'active': False, 'volumeName': vol_name, 'lun': None, 'type': 0}], hpeexceptions.HTTPNotFound, hpeexceptions.HTTPNotFound] @@ -7605,7 +7707,7 @@ class TestHPE3PARFCDriver(HPE3PARBaseDriver): mock.call.queryHost(wwns=['123456789012345', '123456789054321']), mock.call.getHostVLUNs(self.FAKE_HOST), mock.call.deleteVLUN( - self.VOLUME_3PAR_NAME, + vol_name, None, hostname=self.FAKE_HOST), mock.call.getHostVLUNs(self.FAKE_HOST), @@ -7616,7 +7718,7 @@ class TestHPE3PARFCDriver(HPE3PARBaseDriver): with mock.patch.object(hpecommon.HPE3PARCommon, '_create_client') as mock_create_client: mock_create_client.return_value = mock_client - conn_info = self.driver.terminate_connection(self.volume, + conn_info = self.driver.terminate_connection(volume, self.connector) mock_client.assert_has_calls( self.standard_login + @@ -7636,7 +7738,7 @@ class TestHPE3PARFCDriver(HPE3PARBaseDriver): mock_client.deleteHost = mock.Mock( side_effect=[delete_with_vlun, delete_with_hostset]) - conn_info = self.driver.terminate_connection(self.volume, + conn_info = self.driver.terminate_connection(volume, self.connector) mock_client.assert_has_calls( self.standard_login + @@ -7645,7 +7747,7 @@ class TestHPE3PARFCDriver(HPE3PARBaseDriver): mock_client.reset_mock() mock_client.getHostVLUNs.side_effect = effects - conn_info = self.driver.terminate_connection(self.volume, + conn_info = self.driver.terminate_connection(volume, self.connector) mock_client.assert_has_calls( self.standard_login + @@ -8710,6 +8812,7 @@ class TestHPE3PARFCDriver(HPE3PARBaseDriver): self.migrate_volume_attached() +@ddt.ddt class TestHPE3PARISCSIDriver(HPE3PARBaseDriver): TARGET_IQN = 'iqn.2000-05.com.3pardata:21810002ac00383d' @@ -8771,7 +8874,10 @@ class TestHPE3PARISCSIDriver(HPE3PARBaseDriver): driver=hpedriver.HPE3PARISCSIDriver, is_primera=True) - def test_initialize_connection(self): + @ddt.data('volume', 'volume_name_id') + def test_initialize_connection(self, volume_attr): + volume = getattr(self, volume_attr) + vol_name = getattr(self, volume_attr.upper() + '_3PAR_NAME') # setup_mock_client drive with default configuration # and return the mock HTTP 3PAR client mock_client = self.setup_driver() @@ -8788,15 +8894,15 @@ class TestHPE3PARISCSIDriver(HPE3PARBaseDriver): mock_client.getHostVLUNs.side_effect = [ [{'hostname': self.FAKE_HOST, - 'volumeName': self.VOLUME_3PAR_NAME, + 'volumeName': vol_name, 'lun': self.TARGET_LUN, 'portPos': {'node': 8, 'slot': 1, 'cardPort': 1}}], [{'active': True, - 'volumeName': self.VOLUME_3PAR_NAME, + 'volumeName': vol_name, 'lun': self.TARGET_LUN, 'type': 0}]] location = ("%(volume_name)s,%(lun_id)s,%(host)s,%(nsp)s" % - {'volume_name': self.VOLUME_3PAR_NAME, + {'volume_name': vol_name, 'lun_id': self.TARGET_LUN, 'host': self.FAKE_HOST, 'nsp': 'something'}) @@ -8806,11 +8912,11 @@ class TestHPE3PARISCSIDriver(HPE3PARBaseDriver): '_create_client') as mock_create_client: mock_create_client.return_value = mock_client result = self.driver.initialize_connection( - self.volume, + volume, self.connector) expected = [ - mock.call.getVolume(self.VOLUME_3PAR_NAME), + mock.call.getVolume(vol_name), mock.call.getCPG(HPE3PAR_CPG), mock.call.getHost(self.FAKE_HOST), mock.call.queryHost(iqns=['iqn.1993-08.org.debian:01:222']), @@ -10804,13 +10910,16 @@ class TestHPE3PARISCSIDriver(HPE3PARBaseDriver): self.assertEqual(0, mock_client.deleteVLUN.call_count) self.assertEqual(0, mock_client.deleteHost.call_count) - def test_terminate_connection(self): + @ddt.data('volume', 'volume_name_id') + def test_terminate_connection(self, volume_attr): + volume = getattr(self, volume_attr) + vol_name = getattr(self, volume_attr.upper() + '_3PAR_NAME') # setup_mock_client drive with default configuration # and return the mock HTTP 3PAR client mock_client = self.setup_driver() mock_client.getHostVLUNs.return_value = [ {'active': False, - 'volumeName': self.VOLUME_3PAR_NAME, + 'volumeName': vol_name, 'lun': None, 'type': 0}] mock_client.queryHost.return_value = { @@ -10823,7 +10932,7 @@ class TestHPE3PARISCSIDriver(HPE3PARBaseDriver): '_create_client') as mock_create_client: mock_create_client.return_value = mock_client self.driver.terminate_connection( - self.volume, + volume, self.connector, force=True) @@ -10831,7 +10940,7 @@ class TestHPE3PARISCSIDriver(HPE3PARBaseDriver): mock.call.queryHost(iqns=[self.connector['initiator']]), mock.call.getHostVLUNs(self.FAKE_HOST), mock.call.deleteVLUN( - self.VOLUME_3PAR_NAME, + vol_name, None, hostname=self.FAKE_HOST), mock.call.getHostVLUNs(self.FAKE_HOST), @@ -10839,10 +10948,8 @@ class TestHPE3PARISCSIDriver(HPE3PARBaseDriver): 'fakehost', {'pathOperation': 2, 'iSCSINames': ['iqn.1993-08.org.debian:01:222']}), - mock.call.removeVolumeMetaData( - self.VOLUME_3PAR_NAME, CHAP_USER_KEY), - mock.call.removeVolumeMetaData( - self.VOLUME_3PAR_NAME, CHAP_PASS_KEY)] + mock.call.removeVolumeMetaData(vol_name, CHAP_USER_KEY), + mock.call.removeVolumeMetaData(vol_name, CHAP_PASS_KEY)] mock_client.assert_has_calls( self.standard_login + diff --git a/cinder/volume/drivers/hpe/hpe_3par_common.py b/cinder/volume/drivers/hpe/hpe_3par_common.py index 30139ef1e82..34edea46b7e 100644 --- a/cinder/volume/drivers/hpe/hpe_3par_common.py +++ b/cinder/volume/drivers/hpe/hpe_3par_common.py @@ -54,6 +54,7 @@ from cinder import context from cinder import exception from cinder import flow_utils from cinder.i18n import _ +from cinder import objects from cinder.objects import fields from cinder import utils from cinder.volume import configuration @@ -569,7 +570,7 @@ class HPE3PARCommon(object): return None def extend_volume(self, volume, new_size): - volume_name = self._get_3par_vol_name(volume['id']) + volume_name = self._get_3par_vol_name(volume) old_size = volume['size'] growth_size = int(new_size) - old_size LOG.debug("Extending Volume %(vol)s from %(old)s to %(new)s, " @@ -684,8 +685,11 @@ class HPE3PARCommon(object): for snapshot in snapshots: # Getting vol_name from snapshot, in case of group created # from group snapshot. - vol_name = ( - self._get_3par_vol_name(snapshot.get('volume_id'))) + # Don't use the "volume_id" from the snapshot directly in + # case the volume has been migrated and uses a different ID + # in the backend. This may trigger OVO lazy loading. Use + # dict compatibility to avoid changing all the unit tests. + vol_name = self._get_3par_vol_name(snapshot['volume']) if src_vol_name == vol_name: vol_name = ( self._get_3par_vol_name(snapshot.get('id'))) @@ -704,7 +708,7 @@ class HPE3PARCommon(object): vol_name = self._get_3par_vol_name(src_vol_name) snap_name = snap_vol_dict.get(vol_name) - volume_name = self._get_3par_vol_name(volume.get('id')) + volume_name = self._get_3par_vol_name(volume) type_info = self.get_volume_settings_from_type(volume) cpg = type_info['cpg'] snapcpg = type_info['snap_cpg'] @@ -823,7 +827,7 @@ class HPE3PARCommon(object): # TODO(kushal) : we will use volume as object when we re-write # the design for unit tests to use objects instead of dicts. for volume in add_volumes: - volume_name = self._get_3par_vol_name(volume.get('id')) + volume_name = self._get_3par_vol_name(volume) vol_snap_enable = self.is_volume_group_snap_type( volume.get('volume_type')) try: @@ -857,7 +861,7 @@ class HPE3PARCommon(object): raise exception.InvalidInput(reason=msg) for volume in remove_volumes: - volume_name = self._get_3par_vol_name(volume.get('id')) + volume_name = self._get_3par_vol_name(volume) if group.is_replicated: # Remove a volume from remote copy group @@ -992,12 +996,15 @@ class HPE3PARCommon(object): display_name = None # Generate the new volume information based on the new ID. - new_vol_name = self._get_3par_vol_name(volume['id']) + new_vol_name = self._get_3par_vol_name(volume) + # No need to worry about "_name_id" because this is a newly created + # volume that cannot have been migrated. name = 'volume-' + volume['id'] new_comment['volume_id'] = volume['id'] new_comment['name'] = name new_comment['type'] = 'OpenStack' + self._add_name_id_to_comment(new_comment, volume) volume_type = None if volume['volume_type_id']: @@ -1088,7 +1095,7 @@ class HPE3PARCommon(object): raise exception.InvalidInput(reason=err) # Make sure the snapshot is being associated with the correct volume. - parent_vol_name = self._get_3par_vol_name(volume['id']) + parent_vol_name = self._get_3par_vol_name(volume) if parent_vol_name != snap['copyOf']: err = (_("The provided snapshot '%s' is not a snapshot of " "the provided volume.") % target_snap_name) @@ -1114,6 +1121,7 @@ class HPE3PARCommon(object): new_snap_name = self._get_3par_snap_name(snapshot['id']) new_comment['volume_id'] = volume['id'] new_comment['volume_name'] = 'volume-' + volume['id'] + self._add_name_id_to_comment(new_comment, volume) if snapshot.get('display_description', None): new_comment['description'] = snapshot['display_description'] else: @@ -1193,7 +1201,10 @@ class HPE3PARCommon(object): """Removes the specified volume from Cinder management.""" # Rename the volume's name to unm-* format so that it can be # easily found later. - vol_name = self._get_3par_vol_name(volume['id']) + vol_name = self._get_3par_vol_name(volume) + # Rename using the user visible ID ignoring the internal "_name_id" + # that may have been generated during a retype. This makes it easier + # to locate volumes in the backend. new_vol_name = self._get_3par_unm_name(volume['id']) self.client.modifyVolume(vol_name, {'newName': new_vol_name}) @@ -1255,7 +1266,7 @@ class HPE3PARCommon(object): def _extend_volume(self, volume, volume_name, growth_size_mib, _convert_to_base=False): model_update = None - rcg_name = self._get_3par_rcg_name(volume['id']) + rcg_name = self._get_3par_rcg_name(volume) is_volume_replicated = self._volume_of_replicated_type( volume, hpe_tiramisu_check=True) volume_part_of_group = ( @@ -1303,7 +1314,8 @@ class HPE3PARCommon(object): {'vol': volume_name, 'ex': ex}) return model_update - def _get_3par_vol_name(self, volume_id, temp_vol=False): + @classmethod + def _get_3par_vol_name(cls, volume_id, temp_vol=False): """Get converted 3PAR volume name. Converts the openstack volume id from @@ -1317,8 +1329,16 @@ class HPE3PARCommon(object): We strip the padding '=' and replace + with . and / with - + + volume_id is a polymorphic parameter and can be either a string or a + volume (OVO or dict representation). """ - volume_name = self._encode_name(volume_id) + # Accept OVOs (what we should only receive), dict (so we don't have to + # change all our unit tests), and ORM (because we some methods still + # pass it, such as terminate_connection). + if isinstance(volume_id, (objects.Volume, objects.Volume.model, dict)): + volume_id = volume_id.get('_name_id') or volume_id['id'] + volume_name = cls._encode_name(volume_id) if temp_vol: # is this a temporary volume # this is done during migration @@ -1350,16 +1370,17 @@ class HPE3PARCommon(object): return "unm-%s" % unm_name # v2 replication conversion - def _get_3par_rcg_name(self, volume_id): - rcg_name = self._encode_name(volume_id) + def _get_3par_rcg_name(self, volume): + rcg_name = self._encode_name(volume.get('_name_id') or volume['id']) rcg = "rcg-%s" % rcg_name return rcg[:22] - def _get_3par_remote_rcg_name(self, volume_id, provider_location): - return self._get_3par_rcg_name(volume_id) + ".r" + ( + def _get_3par_remote_rcg_name(self, volume, provider_location): + return self._get_3par_rcg_name(volume) + ".r" + ( six.text_type(provider_location)) - def _encode_name(self, name): + @staticmethod + def _encode_name(name): uuid_str = name.replace("-", "") vol_uuid = uuid.UUID('urn:uuid:%s' % uuid_str) vol_encoded = base64.encode_as_text(vol_uuid.bytes) @@ -1728,7 +1749,7 @@ class HPE3PARCommon(object): In order to export a volume on a 3PAR box, we have to create a VLUN. """ - volume_name = self._get_3par_vol_name(volume['id']) + volume_name = self._get_3par_vol_name(volume) vlun_info = self._create_3par_vlun(volume_name, host['name'], nsp, lun_id=lun_id, remote_client=remote_client) @@ -1739,7 +1760,7 @@ class HPE3PARCommon(object): remote_client) def _delete_vlun(self, client_obj, volume, hostname, wwn=None, iqn=None): - volume_name = self._get_3par_vol_name(volume['id']) + volume_name = self._get_3par_vol_name(volume) if hostname: vluns = client_obj.getHostVLUNs(hostname) else: @@ -2035,7 +2056,7 @@ class HPE3PARCommon(object): raise exception.CinderException(ex) def get_cpg(self, volume, allowSnap=False): - volume_name = self._get_3par_vol_name(volume['id']) + volume_name = self._get_3par_vol_name(volume) vol = self.client.getVolume(volume_name) # Search for 'userCPG' in the get volume REST API, # if found return userCPG , else search for snapCPG attribute @@ -2232,12 +2253,14 @@ class HPE3PARCommon(object): '%(host)s)', {'disp_name': volume['display_name'], 'vol_name': volume['name'], - 'id': self._get_3par_vol_name(volume['id']), + 'id': self._get_3par_vol_name(volume), 'host': volume['host']}) try: comments = {'volume_id': volume['id'], 'name': volume['name'], 'type': 'OpenStack'} + self._add_name_id_to_comment(comments, volume) + # This flag denotes group level replication on # hpe 3par. hpe_tiramisu = False @@ -2287,7 +2310,7 @@ class HPE3PARCommon(object): extras['tdvv'] = tdvv capacity = self._capacity_from_size(volume['size']) - volume_name = self._get_3par_vol_name(volume['id']) + volume_name = self._get_3par_vol_name(volume) if compression is not None: extras['compression'] = compression @@ -2419,7 +2442,7 @@ class HPE3PARCommon(object): This is used by cloning a volume so that we can then issue extend volume against the original volume. """ - vol_name = self._get_3par_vol_name(volume['id']) + vol_name = self._get_3par_vol_name(volume) # create a brand new uuid for the temp snap snap_uuid = uuid.uuid4().hex @@ -2428,6 +2451,7 @@ class HPE3PARCommon(object): extra = {'volume_name': volume['name'], 'volume_id': volume['id']} + self._add_name_id_to_comment(extra, volume) optional = {'comment': json.dumps(extra)} @@ -2442,8 +2466,8 @@ class HPE3PARCommon(object): def create_cloned_volume(self, volume, src_vref): try: - vol_name = self._get_3par_vol_name(volume['id']) - src_vol_name = self._get_3par_vol_name(src_vref['id']) + vol_name = self._get_3par_vol_name(volume) + src_vol_name = self._get_3par_vol_name(src_vref) back_up_process = False vol_chap_enabled = False hpe_tiramisu = False @@ -2576,7 +2600,7 @@ class HPE3PARCommon(object): return try: - volume_name = self._get_3par_vol_name(volume['id']) + volume_name = self._get_3par_vol_name(volume) # Try and delete the volume, it might fail here because # the volume is part of a volume set which will have the # volume set name in the error. @@ -2674,10 +2698,11 @@ class HPE3PARCommon(object): try: if not snap_name: snap_name = self._get_3par_snap_name(snapshot['id']) - volume_name = self._get_3par_vol_name(volume['id']) + volume_name = self._get_3par_vol_name(volume) extra = {'volume_id': volume['id'], 'snapshot_id': snapshot['id']} + self._add_name_id_to_comment(extra, volume) type_id = volume.get('volume_type_id', None) @@ -2772,12 +2797,15 @@ class HPE3PARCommon(object): try: snap_name = self._get_3par_snap_name(snapshot['id']) - vol_name = self._get_3par_vol_name(snapshot['volume_id']) + # Don't use the "volume_id" from the snapshot directly in case the + # volume has been migrated and uses a different ID in the backend. + # This may trigger OVO lazy loading. Use dict compatibility to + # avoid changing all the unit tests. + vol_name = self._get_3par_vol_name(snapshot['volume']) - extra = {'volume_name': snapshot['volume_name']} - vol_id = snapshot.get('volume_id', None) - if vol_id: - extra['volume_id'] = vol_id + extra = {'volume_name': snapshot['volume_name'], + 'volume_id': snapshot.get('volume_id')} + self._add_name_id_to_comment(extra, snapshot['volume']) try: extra['display_name'] = snapshot['display_name'] @@ -2844,6 +2872,93 @@ class HPE3PARCommon(object): dbg_ret) return ret + def _rename_migrated(self, volume, dest_volume): + """Rename the destination volume after a migration. + + Returns whether the destination volume has the name matching the source + volume or not. + + That way we know whether we need to set the _name_id or not. + """ + def log_error(vol_type, error, src, dest, rename_name=None, + original_name=None): + LOG.error("Changing the %(vol_type)s volume name from %(src)s to " + "%(dest)s failed because %(reason)s", + {'vol_type': vol_type, 'src': src, 'dest': dest, + 'reason': error}) + if rename_name: + original_name = original_name or dest + # Don't fail the migration, but help the user fix the + # source volume stuck in error_deleting. + LOG.error("Migration will fail to delete the original volume. " + "It must be manually renamed from %(rename_name)s to" + " %(original_name)s in the backend, and then we " + "have to tell cinder to delete volume %(vol_id)s", + {'rename_name': rename_name, + 'original_name': original_name, + 'vol_id': dest_volume['id']}) + + original_volume_renamed = False + # We don't need to rename the source volume if it uses a _name_id, + # since the id we want to use to rename the new volume is available. + if volume['id'] == (volume.get('_name_id') or volume['id']): + original_name = self._get_3par_vol_name(volume) + temp_name = self._get_3par_vol_name(volume, temp_vol=True) + + # In case the original volume is on the same backend, try + # renaming it to a temporary name. + 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('original', e, original_name, temp_name) + return False + + # Change the destination volume name to the source's ID name + current_name = self._get_3par_vol_name(dest_volume) + volume_id_name = self._get_3par_vol_name(volume['id']) + try: + # After this call the volume manager will call + # finish_volume_migration and swap the fields, so we want to + # have the right info on the comments if we succeed in renaming + # the volumes in the backend. + new_comment = self._get_updated_comment(current_name, + volume_id=volume['id'], + _name_id=None) + volumeMods = {'newName': volume_id_name, 'comment': new_comment} + self.client.modifyVolume(current_name, volumeMods) + LOG.info("Current volume changed from %(cur)s to %(orig)s", + {'cur': current_name, 'orig': volume_id_name}) + except Exception as e: + if original_volume_renamed: + _name = temp_name + else: + _name = original_name = None + log_error('migrating', e, current_name, volume_id_name, _name, + original_name) + return False + + # 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: + old_comment = self._get_updated_comment( + original_name, + volume_id=dest_volume['id'], + _name_id=volume.get('_name_id')) + volumeCurrentMods = {'newName': current_name, + 'comment': old_comment} + self.client.modifyVolume(temp_name, volumeCurrentMods) + except Exception as e: + log_error('original', e, temp_name, current_name, temp_name) + return True + def update_migrated_volume(self, context, volume, new_volume, original_volume_status): """Rename the new (temp) volume to it's original name. @@ -2854,77 +2969,48 @@ class HPE3PARCommon(object): """ LOG.debug("Update volume name for %(id)s", {'id': new_volume['id']}) - new_volume_renamed = False + + # For available volumes we'll try renaming the destination volume to + # match the id of the source volume. if original_volume_status == 'available': - # volume isn't attached and can be updated - original_name = self._get_3par_vol_name(volume['id']) - 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} - self.client.modifyVolume(current_name, volumeMods) - 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 migrating volume name from %(cur)s to " - "%(orig)s failed because %(reason)s", - {'cur': current_name, 'orig': original_name, - 'reason': e}) - 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}) + new_volume_renamed = self._rename_migrated(volume, new_volume) + else: + new_volume_renamed = False if new_volume_renamed: name_id = None + # NOTE: I think this will break with replicated volumes. provider_location = None + else: # the backend can't change the name. name_id = new_volume['_name_id'] or new_volume['id'] provider_location = new_volume['provider_location'] + # Update the comment in the backend to reflect the _name_id + current_name = self._get_3par_vol_name(new_volume) + self._update_comment(current_name, volume_id=volume['id'], + _name_id=name_id) return {'_name_id': name_id, 'provider_location': provider_location} + @staticmethod + def _add_name_id_to_comment(comment, volume): + name_id = volume.get('_name_id') + if name_id: + comment['_name_id'] = name_id + + def _get_updated_comment(self, vol_name, **values): + vol = self.client.getVolume(vol_name) + comment = json.loads(vol['comment']) if vol['comment'] else {} + comment.update(values) + + def _update_comment(self, vol_name, **values): + """Update key-value pairs on the comment of a volume in the backend.""" + if not values: + return + comment = self._get_updated_comment(vol_name, **values) + self.client.modifyVolume(vol_name, {'comment': json.dumps(comment)}) + def _wait_for_task_completion(self, task_id): """This waits for a 3PAR background task complete or fail. @@ -2957,7 +3043,7 @@ class HPE3PARCommon(object): # Change the name such that it is unique since 3PAR # names must be unique across all CPGs - volume_name = self._get_3par_vol_name(volume['id']) + volume_name = self._get_3par_vol_name(volume) temp_vol_name = volume_name.replace("osv-", "omv-") compression = self.get_compression_policy( @@ -3075,6 +3161,8 @@ class HPE3PARCommon(object): v2['id'] = self._get_3par_vol_comment_value( v2['comment'], 'volume_id') + v2['_name_id'] = self._get_3par_vol_comment_value( + v2['comment'], '_name_id') v2['host'] = '#' + v1['userCPG'] @@ -3386,8 +3474,7 @@ class HPE3PARCommon(object): dictionary of its reported capabilities. Host validation is just skipped if host is None. """ - volume_id = volume['id'] - volume_name = self._get_3par_vol_name(volume_id) + volume_name = self._get_3par_vol_name(volume) new_type_name = None new_type_id = None if new_type: @@ -3488,7 +3575,7 @@ class HPE3PARCommon(object): old_volume_settings, host) def remove_temporary_snapshots(self, volume): - vol_name = self._get_3par_vol_name(volume['id']) + vol_name = self._get_3par_vol_name(volume) snapshots_list = self.client.getVolumeSnapshots(vol_name) tmp_snapshots_list = [snap for snap in snapshots_list @@ -3514,9 +3601,9 @@ class HPE3PARCommon(object): :param volume: A dictionary describing the volume to revert :param snapshot: A dictionary describing the latest snapshot """ - volume_name = self._get_3par_vol_name(volume['id']) + volume_name = self._get_3par_vol_name(volume) snapshot_name = self._get_3par_snap_name(snapshot['id']) - rcg_name = self._get_3par_rcg_name(volume['id']) + rcg_name = self._get_3par_rcg_name(volume) volume_part_of_group = ( self._volume_of_hpe_tiramisu_type_and_part_of_group(volume)) if volume_part_of_group: @@ -3580,7 +3667,7 @@ class HPE3PARCommon(object): """ existing_vlun = None try: - vol_name = self._get_3par_vol_name(volume['id']) + vol_name = self._get_3par_vol_name(volume) if remote_client: host_vluns = remote_client.getHostVLUNs(host['name']) else: @@ -3602,7 +3689,7 @@ class HPE3PARCommon(object): def find_existing_vluns(self, volume, host, remote_client=None): existing_vluns = [] try: - vol_name = self._get_3par_vol_name(volume['id']) + vol_name = self._get_3par_vol_name(volume) if remote_client: host_vluns = remote_client.getHostVLUNs(host['name']) else: @@ -3696,7 +3783,7 @@ class HPE3PARCommon(object): # Try and stop remote-copy on main array. We eat the # exception here because when an array goes down, the # groups will stop automatically. - rcg_name = self._get_3par_rcg_name(volume['id']) + rcg_name = self._get_3par_rcg_name(volume) self.client.stopRemoteCopy(rcg_name) except Exception: pass @@ -3704,7 +3791,7 @@ class HPE3PARCommon(object): try: # Failover to secondary array. remote_rcg_name = self._get_3par_remote_rcg_name( - volume['id'], volume['provider_location']) + volume, volume['provider_location']) cl = self._create_replication_client(failover_target) cl.recoverRemoteCopyGroupFromDisaster( remote_rcg_name, self.RC_ACTION_CHANGE_TO_PRIMARY) @@ -3777,9 +3864,8 @@ class HPE3PARCommon(object): if self._volume_of_replicated_type(volume, hpe_tiramisu_check=True): location = volume.get('provider_location') - remote_rcg_name = self._get_3par_remote_rcg_name( - volume['id'], - location) + remote_rcg_name = self._get_3par_remote_rcg_name(volume, + location) rcg = self.client.getRemoteCopyGroup(remote_rcg_name) if not self._are_targets_in_their_natural_direction(rcg): return False @@ -3967,7 +4053,7 @@ class HPE3PARCommon(object): return replicated_type def _is_volume_in_remote_copy_group(self, volume): - rcg_name = self._get_3par_rcg_name(volume['id']) + rcg_name = self._get_3par_rcg_name(volume) try: self.client.getRemoteCopyGroup(rcg_name) return True @@ -4088,7 +4174,7 @@ class HPE3PARCommon(object): reverse order, including the original volume. """ - rcg_name = self._get_3par_rcg_name(volume['id']) + rcg_name = self._get_3par_rcg_name(volume) # If the volume is already in a remote copy group, return True # after starting remote copy. If remote copy is already started, # issuing this command again will be fine. @@ -4127,7 +4213,7 @@ class HPE3PARCommon(object): vol_settings = self.get_volume_settings_from_type(volume) local_cpg = vol_settings['cpg'] - vol_name = self._get_3par_vol_name(volume['id']) + vol_name = self._get_3par_vol_name(volume) # Create remote copy group on main array. rcg_targets = [] @@ -4249,8 +4335,8 @@ class HPE3PARCommon(object): -Delete volume from main array """ if not rcg_name: - rcg_name = self._get_3par_rcg_name(volume['id']) - vol_name = self._get_3par_vol_name(volume['id']) + rcg_name = self._get_3par_rcg_name(volume) + vol_name = self._get_3par_vol_name(volume) # Stop remote copy. try: @@ -4286,7 +4372,7 @@ class HPE3PARCommon(object): def _delete_replicated_failed_over_volume(self, volume): location = volume.get('provider_location') - rcg_name = self._get_3par_remote_rcg_name(volume['id'], location) + rcg_name = self._get_3par_remote_rcg_name(volume, location) targets = self.client.getRemoteCopyGroup(rcg_name)['targets'] # When failed over, we want to temporarily disable config mirroring # in order to be allowed to delete the volume and remote copy group @@ -4313,7 +4399,7 @@ class HPE3PARCommon(object): def _delete_vvset(self, volume): # volume is part of a volume set. - volume_name = self._get_3par_vol_name(volume['id']) + volume_name = self._get_3par_vol_name(volume) vvset_name = self.client.findVolumeSet(volume_name) LOG.debug("Returned vvset_name = %s", vvset_name) if vvset_name is not None: @@ -4467,7 +4553,7 @@ class HPE3PARCommon(object): def _remove_vol_from_remote_copy_group(self, group, volume): rcg_name = self._get_3par_rcg_name_of_group(group.id) - vol_name = self._get_3par_vol_name(volume.get('id')) + vol_name = self._get_3par_vol_name(volume) try: # Delete volume from remote copy group on secondary array. @@ -4575,7 +4661,7 @@ class HPE3PARCommon(object): def _add_vol_to_remote(self, volume, rcg_name): # Add a volume to remote copy group. rcg_targets = [] - vol_name = self._get_3par_vol_name(volume.get('id')) + vol_name = self._get_3par_vol_name(volume) replication_mode_num = self._get_replication_mode_from_volume(volume) for target in self._replication_targets: if target['replication_mode'] == replication_mode_num: @@ -4614,7 +4700,7 @@ class HPE3PARCommon(object): pass for volume in volumes: - vol_name = self._get_3par_vol_name(volume.get('id')) + vol_name = self._get_3par_vol_name(volume) # Delete volume from remote copy group on secondary array. try: self.client.removeVolumeFromRemoteCopyGroup( diff --git a/cinder/volume/drivers/hpe/hpe_3par_iscsi.py b/cinder/volume/drivers/hpe/hpe_3par_iscsi.py index acf61de4623..24dd2fd60cc 100644 --- a/cinder/volume/drivers/hpe/hpe_3par_iscsi.py +++ b/cinder/volume/drivers/hpe/hpe_3par_iscsi.py @@ -510,7 +510,7 @@ class HPE3PARISCSIDriver(hpebasedriver.HPE3PARDriverBase): Ignore exceptions caused by the keys not being present on a volume. """ - vol_name = common._get_3par_vol_name(volume['id']) + vol_name = common._get_3par_vol_name(volume) try: common.client.removeVolumeMetaData(vol_name, CHAP_USER_KEY) @@ -616,7 +616,7 @@ class HPE3PARISCSIDriver(hpebasedriver.HPE3PARDriverBase): if not remote_target: # Get the CHAP secret if CHAP is enabled if common._client_conf['hpe3par_iscsi_chap_enabled']: - vol_name = common._get_3par_vol_name(volume['id']) + vol_name = common._get_3par_vol_name(volume) username = common.client.getVolumeMetaData( vol_name, CHAP_USER_KEY)['value'] password = common.client.getVolumeMetaData( @@ -723,7 +723,7 @@ class HPE3PARISCSIDriver(hpebasedriver.HPE3PARDriverBase): "Generating new CHAP key.") # Add CHAP credentials to the volume metadata - vol_name = common._get_3par_vol_name(volume['id']) + vol_name = common._get_3par_vol_name(volume) common.client.setVolumeMetaData( vol_name, CHAP_USER_KEY, chap_username) common.client.setVolumeMetaData( @@ -750,7 +750,7 @@ class HPE3PARISCSIDriver(hpebasedriver.HPE3PARDriverBase): """ common = self._login() try: - vol_name = common._get_3par_vol_name(volume['id']) + vol_name = common._get_3par_vol_name(volume) common.client.getVolume(vol_name) except hpeexceptions.HTTPNotFound: LOG.error("Volume %s doesn't exist on array.", vol_name) diff --git a/releasenotes/notes/fix-3par-live-migration-0065bd2626fdb4a1.yaml b/releasenotes/notes/fix-3par-live-migration-0065bd2626fdb4a1.yaml new file mode 100644 index 00000000000..5ce3b119c53 --- /dev/null +++ b/releasenotes/notes/fix-3par-live-migration-0065bd2626fdb4a1.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fix HPE 3PAR driver issue where volumes that were live migrated to it would + end up being inaccessible. We would no longer be able to use the volume + for any operation, such as attach, detach, delete, snapshot, etc. + (bug 1697422)