diff --git a/cinder/tests/unit/volume/drivers/dell_emc/vmax/test_vmax.py b/cinder/tests/unit/volume/drivers/dell_emc/vmax/test_vmax.py index 6a748e23505..b8fe00709e7 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/vmax/test_vmax.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/vmax/test_vmax.py @@ -77,6 +77,7 @@ class VMAXCommonData(object): remote_array = '000197800124' device_id = '00001' device_id2 = '00002' + device_id3 = '00003' rdf_group_name = '23_24_007' rdf_group_no = '70' u4v_version = '84' @@ -193,7 +194,7 @@ class VMAXCommonData(object): host=fake_host, volume=test_volume) test_legacy_snapshot = fake_snapshot.fake_snapshot_obj( - context=ctx, id='12345', name='my_snap', size=2, + context=ctx, id=test_volume.id, name='my_snap', size=2, provider_location=six.text_type(legacy_provider_location), host=fake_host, volume=test_volume) @@ -439,7 +440,7 @@ class VMAXCommonData(object): volume_details = [{"cap_gb": 2, "num_of_storage_groups": 1, "volumeId": device_id, - "volume_identifier": "1", + "volume_identifier": "OS-%s" % test_volume.id, "wwn": volume_wwn, "snapvx_target": 'false', "snapvx_source": 'false', @@ -448,10 +449,15 @@ class VMAXCommonData(object): {"cap_gb": 1, "num_of_storage_groups": 1, "volumeId": device_id2, - "volume_identifier": "OS-2", + "volume_identifier": "OS-%s" % test_volume.id, "wwn": '600012345', "storageGroupId": [defaultstoragegroup_name, - storagegroup_name_f]}] + storagegroup_name_f]}, + {"cap_gb": 1, + "num_of_storage_groups": 0, + "volumeId": device_id3, + "volume_identifier": '123', + "wwn": '600012345'}] volume_list = [ {"resultList": {"result": [{"volumeId": device_id}]}}, @@ -1753,14 +1759,14 @@ class VMAXRestTest(test.TestCase): def test_delete_volume(self): device_id = self.data.device_id - with mock.patch.object(self.rest, 'delete_resource'): - with mock.patch.object( - self.rest, '_modify_volume', - side_effect=[None, exception.VolumeBackendAPIException]): + with mock.patch.object(self.rest, 'delete_resource'),\ + mock.patch.object( + self.rest, '_modify_volume', side_effect=[ + None, None, None, exception.VolumeBackendAPIException]): for x in range(0, 2): self.rest.delete_volume(self.data.array, device_id) mod_call_count = self.rest._modify_volume.call_count - self.assertEqual(2, mod_call_count) + self.assertEqual(4, mod_call_count) self.rest.delete_resource.assert_called_once_with( self.data.array, 'sloprovisioning', 'volume', device_id) @@ -1771,10 +1777,26 @@ class VMAXRestTest(test.TestCase): "volumeIdentifier": { "identifier_name": 'new_name', "volumeIdentifierChoice": "identifier_name"}}}} - with mock.patch.object(self.rest, '_modify_volume'): + payload2 = {"editVolumeActionParam": {"modifyVolumeIdentifierParam": { + "volumeIdentifier": {"volumeIdentifierChoice": "none"}}}} + with mock.patch.object(self.rest, '_modify_volume') as mock_mod: self.rest.rename_volume(self.data.array, device_id, 'new_name') - self.rest._modify_volume.assert_called_once_with( + mock_mod.assert_called_once_with( self.data.array, device_id, payload) + mock_mod.reset_mock() + self.rest.rename_volume(self.data.array, device_id, None) + self.rest._modify_volume.assert_called_once_with( + self.data.array, device_id, payload2) + + def test_check_volume_device_id(self): + element_name = self.utils.get_volume_element_name( + self.data.test_volume.id) + found_dev_id = self.rest.check_volume_device_id( + self.data.array, self.data.device_id, element_name) + self.assertEqual(self.data.device_id, found_dev_id) + found_dev_id2 = self.rest.check_volume_device_id( + self.data.array, self.data.device_id3, element_name) + self.assertIsNone(found_dev_id2) def test_find_mv_connections_for_vol(self): device_id = self.data.device_id @@ -3150,16 +3172,6 @@ class VMAXCommonTest(test.TestCase): founddevice_id = self.common._find_device_on_array(volume, extra_specs) self.assertEqual(ref_device_id, founddevice_id) - def test_find_device_on_array_different_device_id(self): - volume = self.data.test_volume - extra_specs = self.data.extra_specs - with mock.patch.object( - self.rest, 'find_volume_device_id', - return_value='01234'): - founddevice_id = self.common._find_device_on_array( - volume, extra_specs) - self.assertIsNone(founddevice_id) - def test_find_device_on_array_provider_location_not_string(self): volume = fake_volume.fake_volume_obj( context='cxt', provider_location=None) @@ -3799,17 +3811,17 @@ class VMAXCommonTest(test.TestCase): rest.VMAXRest, 'is_vol_in_rep_session', return_value=(False, False, None)) def test_check_lun_valid_for_cinder_management(self, mock_rep, mock_mv): - external_ref = {u'source-name': u'00001'} + external_ref = {u'source-name': u'00003'} self.common._check_lun_valid_for_cinder_management( - self.data.array, '00001', + self.data.array, self.data.device_id3, self.data.test_volume.id, external_ref) @mock.patch.object( rest.VMAXRest, 'get_volume', side_effect=[ None, - VMAXCommonData.volume_details[0], - VMAXCommonData.volume_details[0], + VMAXCommonData.volume_details[2], + VMAXCommonData.volume_details[2], VMAXCommonData.volume_details[1]]) @mock.patch.object( rest.VMAXRest, 'get_masking_views_from_storage_group', @@ -3821,16 +3833,16 @@ class VMAXCommonTest(test.TestCase): side_effect=[(True, False, []), (False, False, None)]) def test_check_lun_valid_for_cinder_management_exception( self, mock_rep, mock_sg, mock_mvs, mock_get_vol): - external_ref = {u'source-name': u'00001'} + external_ref = {u'source-name': u'00003'} for x in range(0, 3): self.assertRaises( exception.ManageExistingInvalidReference, self.common._check_lun_valid_for_cinder_management, - self.data.array, '00001', + self.data.array, self.data.device_id3, self.data.test_volume.id, external_ref) self.assertRaises(exception.ManageExistingAlreadyManaged, self.common._check_lun_valid_for_cinder_management, - self.data.array, '00001', + self.data.array, self.data.device_id3, self.data.test_volume.id, external_ref) def test_manage_existing_get_size(self): diff --git a/cinder/volume/drivers/dell_emc/vmax/common.py b/cinder/volume/drivers/dell_emc/vmax/common.py index af72729185b..f8d8bae2da3 100644 --- a/cinder/volume/drivers/dell_emc/vmax/common.py +++ b/cinder/volume/drivers/dell_emc/vmax/common.py @@ -885,14 +885,11 @@ class VMAXCommon(object): admin_metadata = volume.admin_metadata if 'targetVolumeName' in admin_metadata: target_vol_name = admin_metadata['targetVolumeName'] - founddevice_id = self.rest.find_volume_device_id( - array, target_vol_name) + founddevice_id = self.rest.check_volume_device_id( + array, target_vol_name, device_id) else: - founddevice_id = self.rest.find_volume_device_id( - array, element_name) - # Allow for an external app to delete the volume. - if device_id and device_id != founddevice_id: - founddevice_id = None + founddevice_id = self.rest.check_volume_device_id( + array, device_id, element_name) if founddevice_id is None: LOG.debug("Volume %(volume_name)s not found on the array.", diff --git a/cinder/volume/drivers/dell_emc/vmax/rest.py b/cinder/volume/drivers/dell_emc/vmax/rest.py index 69d6f5c8a46..898225eb651 100644 --- a/cinder/volume/drivers/dell_emc/vmax/rest.py +++ b/cinder/volume/drivers/dell_emc/vmax/rest.py @@ -697,6 +697,26 @@ class VMAXRest(object): volume_dict = {'array': array, 'device_id': device_id} return volume_dict + def check_volume_device_id(self, array, device_id, element_name): + """Check if the identifiers match for a given volume. + + :param array: the array serial number + :param device_id: the device id + :param element_name: name associated with cinder, e.g.OS- + :return: found_device_id + """ + found_device_id = None + vol_details = self.get_volume(array, device_id) + if vol_details: + vol_identifier = vol_details.get('volume_identifier', None) + LOG.debug('Element name = %(en)s, Vol identifier = %(vi)s, ' + 'Device id = %(di)s, vol details = %(vd)s', + {'en': element_name, 'vi': vol_identifier, + 'di': device_id, 'vd': vol_details}) + if vol_identifier == element_name: + found_device_id = device_id + return found_device_id + def add_vol_to_sg(self, array, storagegroup_name, device_id, extra_specs): """Add a volume to a storage group. @@ -988,13 +1008,17 @@ class VMAXRest(object): :param array: the array serial number :param device_id: the volume device id - :param new_name: the new name for the volume + :param new_name: the new name for the volume, can be None """ + if new_name is not None: + vol_identifier_dict = { + "identifier_name": new_name, + "volumeIdentifierChoice": "identifier_name"} + else: + vol_identifier_dict = {"volumeIdentifierChoice": "none"} rename_vol_payload = {"editVolumeActionParam": { "modifyVolumeIdentifierParam": { - "volumeIdentifier": { - "identifier_name": new_name, - "volumeIdentifierChoice": "identifier_name"}}}} + "volumeIdentifier": vol_identifier_dict}}} self._modify_volume(array, device_id, rename_vol_payload) def delete_volume(self, array, device_id): @@ -1008,6 +1032,8 @@ class VMAXRest(object): "freeVolumeParam": {"free_volume": 'true'}}} try: self._modify_volume(array, device_id, payload) + # Rename volume, removing the OS- + self.rename_volume(array, device_id, None) except Exception as e: LOG.warning('Deallocate volume failed with %(e)s.' 'Attempting delete.', {'e': e})