diff --git a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py index b29d98e3278..ca3866e6804 100644 --- a/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py +++ b/cinder/tests/unit/volume/drivers/hpe/test_hpe3par.py @@ -116,6 +116,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' @@ -181,6 +182,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', @@ -282,6 +292,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, @@ -293,7 +311,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"] @@ -673,21 +699,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'): @@ -2346,16 +2370,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 + @@ -2539,7 +2565,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() @@ -2559,14 +2588,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 @@ -3176,61 +3200,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}, @@ -3249,83 +3321,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( @@ -3333,17 +3433,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} @@ -3352,10 +3448,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) @@ -6206,7 +6303,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, @@ -6215,7 +6311,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} @@ -6312,7 +6408,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, @@ -6321,7 +6416,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} @@ -7032,6 +7127,7 @@ class TestHPE3PARDriverBase(HPE3PARBaseDriver): self.assertIn(new_key, properties) +@ddt.ddt class TestHPE3PARFCDriver(HPE3PARBaseDriver): properties = { @@ -7071,7 +7167,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() @@ -7105,16 +7204,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'}) @@ -7134,11 +7233,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', @@ -7147,7 +7246,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), @@ -7627,13 +7726,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] @@ -7650,7 +7752,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), @@ -7661,7 +7763,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 + @@ -7681,7 +7783,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 + @@ -7690,7 +7792,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 + @@ -8755,6 +8857,7 @@ class TestHPE3PARFCDriver(HPE3PARBaseDriver): self.migrate_volume_attached() +@ddt.ddt class TestHPE3PARISCSIDriver(HPE3PARBaseDriver): TARGET_IQN = 'iqn.2000-05.com.3pardata:21810002ac00383d' @@ -8816,7 +8919,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() @@ -8833,15 +8939,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'}) @@ -8851,11 +8957,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']), @@ -10849,13 +10955,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 = { @@ -10868,7 +10977,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) @@ -10876,7 +10985,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), @@ -10884,10 +10993,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 7bc32d3c9cc..b65902705de 100644 --- a/cinder/volume/drivers/hpe/hpe_3par_common.py +++ b/cinder/volume/drivers/hpe/hpe_3par_common.py @@ -55,6 +55,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 @@ -574,7 +575,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, " @@ -689,8 +690,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'))) @@ -709,7 +713,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'] @@ -828,7 +832,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: @@ -862,7 +866,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 @@ -997,12 +1001,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']: @@ -1093,7 +1100,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) @@ -1119,6 +1126,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: @@ -1198,7 +1206,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}) @@ -1260,7 +1271,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 = ( @@ -1308,7 +1319,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 @@ -1322,8 +1334,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 @@ -1355,16 +1375,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) @@ -1741,7 +1762,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) @@ -1752,7 +1773,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: @@ -2048,7 +2069,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 @@ -2245,12 +2266,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 @@ -2300,7 +2323,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 @@ -2432,7 +2455,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 @@ -2441,6 +2464,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)} @@ -2455,8 +2479,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 @@ -2589,7 +2613,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. @@ -2687,10 +2711,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) @@ -2785,12 +2810,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'] @@ -2857,6 +2885,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. @@ -2867,77 +2982,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. @@ -2970,7 +3056,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( @@ -3088,6 +3174,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'] @@ -3399,8 +3487,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: @@ -3501,7 +3588,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 @@ -3527,9 +3614,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: @@ -3593,7 +3680,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: @@ -3615,7 +3702,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: @@ -3709,7 +3796,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 @@ -3717,7 +3804,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) @@ -3790,9 +3877,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 @@ -3980,7 +4066,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 @@ -4101,7 +4187,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. @@ -4140,7 +4226,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 = [] @@ -4262,8 +4348,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: @@ -4299,7 +4385,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 @@ -4326,7 +4412,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: @@ -4480,7 +4566,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. @@ -4588,7 +4674,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: @@ -4627,7 +4713,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 e929941e2b6..c19bd7bc17e 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)