openstack/cinder
Code Issues Proposed changes

VMAX driver - Volume identifier issues

Browse Source 
When creating a VMAX volume through cinder, a 'volume_identifier' is
set on the device on the backend. This takes the form 'OS-<cinderUUID>'.
There are a couple of issues with how this is currently being done:
1. When the VMAX driver retrieves the volume using the 'device_id' value
saved in the provider location, it checks that the volume identifier
matches OS-<cinderUUID>. However, the way this is currently being
checked can cause an issue if the create volume operation ends up being
retried in cinder - sometimes there can be two devices on the backend
with the same volume identifier, and the code may not pick up the
correct device.
2. On a deallocate operation, the volume identifier is not being unset
on the device. This patch rectifies these issues.

Change-Id: I8218768216e87f6fdda1dd77dd09ca2ca46111c7
Closes-Bug: 1717933
This commit is contained in:
Ciara Stacke 2017-09-20 10:07:36 +01:00
parent 209e1564df
commit cbce7226bb
3 changed files with 74 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

69
cinder/tests/unit/volume/drivers/dell_emc/vmax/test_vmax.py
View File

@ -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'
@ -197,8 +198,7 @@ class VMAXCommonData(object):
host=fake_host, volume=test_volume)
test_legacy_snapshot = fake_snapshot.fake_snapshot_obj(
context=ctx, id='8d38ccfc-3d29-454c-858b-8348a8f9cc95',
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)
@ -457,7 +457,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',
@ -466,10 +466,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}]}},
@ -1771,14 +1776,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)
@ -1789,10 +1794,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
@ -3168,16 +3189,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)
@ -3818,17 +3829,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',
@ -3840,16 +3851,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):

11
cinder/volume/drivers/dell_emc/vmax/common.py
View File

@ -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.",

34
cinder/volume/drivers/dell_emc/vmax/rest.py
View File

@ -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-<cinderUUID>
: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-<cinderUUID>
self.rename_volume(array, device_id, None)
except Exception as e:
LOG.warning('Deallocate volume failed with %(e)s.'
'Attempting delete.', {'e': e})