VMAX driver - widen locking around storage groups

There is an issue around the locking of storage groups.
There is insufficent locking when multiple processes may
be adding and removing volumes from the same storage group
e.g. deleting the last replicated volume while another
process is creating one. This patch rectifies this issue.

Change-Id: I0138ace1e3d8f1e62d5422864481221915907a25
Closes-Bug: #1660374
(cherry picked from commit 84d463380a)
This commit is contained in:
Helen Walsh 2017-01-30 16:11:22 +00:00
parent 5a02c7d740
commit 035d6d3a30
5 changed files with 202 additions and 103 deletions

View File

@ -251,6 +251,7 @@ class EMCVMAXCommonData(object):
fabric_name_prefix = "fakeFabric"
end_point_map = {connector['wwpns'][0]: [target_wwns[0]],
connector['wwpns'][1]: [target_wwns[1]]}
device_map = {}
for wwn in connector['wwpns']:
fabric_name = ''.join([fabric_name_prefix,
@ -5841,12 +5842,12 @@ class EMCV3DriverTestCase(test.TestCase):
self.data = EMCVMAXCommonData()
self.data.storage_system = 'SYMMETRIX-+-000197200056'
self.flags(rpc_backend='oslo_messaging._drivers.impl_fake')
self.tempdir = tempfile.mkdtemp()
super(EMCV3DriverTestCase, self).setUp()
self.config_file_path = None
self.create_fake_config_file_v3()
self.flags(rpc_backend='oslo_messaging._drivers.impl_fake')
self.addCleanup(self._cleanup)
self.set_configuration()
@ -6013,22 +6014,24 @@ class EMCV3DriverTestCase(test.TestCase):
'OS-fakehost-SRP_1-NONE-NONE-MV',
maskingViewDict['maskingViewName'])
def test_last_vol_in_SG_with_MV(self):
@mock.patch.object(
emc_vmax_masking.EMCVMAXMasking,
'_delete_mv_ig_and_sg')
def test_last_vol_in_SG_with_MV(self, mock_delete):
conn = self.fake_ecom_connection()
common = self.driver.common
controllerConfigService = (
self.driver.common.utils.find_controller_configuration_service(
common.utils.find_controller_configuration_service(
conn, self.data.storage_system))
extraSpecs = self.default_extraspec()
storageGroupName = self.data.storagegroupname
storageGroupInstanceName = (
self.driver.common.utils.find_storage_masking_group(
common.utils.find_storage_masking_group(
conn, controllerConfigService, storageGroupName))
vol = self.default_vol()
self.driver.common.masking._delete_mv_ig_and_sg = mock.Mock()
self.assertTrue(self.driver.common.masking._last_vol_in_SG(
self.assertTrue(common.masking._last_vol_in_SG(
conn, controllerConfigService, storageGroupInstanceName,
storageGroupName, vol, vol['name'], extraSpecs))
@ -6516,6 +6519,13 @@ class EMCV3DriverTestCase(test.TestCase):
self.data.test_volume,
self.data.connector)
@mock.patch.object(
emc_vmax_masking.EMCVMAXMasking,
'remove_and_reset_members')
@mock.patch.object(
emc_vmax_utils.EMCVMAXUtils,
'get_volume_element_name',
return_value='1')
@mock.patch.object(
emc_vmax_common.EMCVMAXCommon,
'check_ig_instance_name',
@ -6541,20 +6551,20 @@ class EMCV3DriverTestCase(test.TestCase):
'get_volume_type_extra_specs',
return_value={'volume_backend_name': 'V3_BE'})
def test_detach_v3_success(self, mock_volume_type, mock_maskingview,
mock_ig, mock_igc, mock_mv, mock_check_ig):
mock_ig, mock_igc, mock_mv, mock_check_ig,
mock_element_name, mock_remove):
common = self.driver.common
common.get_target_wwns = mock.Mock(
return_value=EMCVMAXCommonData.target_wwns)
common.masking.utils.find_storage_masking_group = mock.Mock(
return_value=self.data.storagegroups[0])
self.driver.common._initial_setup = mock.Mock(
return_value=self.default_extraspec())
data = self.driver.terminate_connection(self.data.test_volume_v3,
self.data.connector)
common.get_target_wwns.assert_called_once_with(
EMCVMAXCommonData.storage_system, EMCVMAXCommonData.connector)
numTargetWwns = len(EMCVMAXCommonData.target_wwns)
self.assertEqual(numTargetWwns, len(data['data']))
with mock.patch.object(common, 'get_target_wwns',
return_value=EMCVMAXCommonData.target_wwns):
with mock.patch.object(common, '_initial_setup',
return_value=self.default_extraspec()):
data = self.driver.terminate_connection(
self.data.test_volume_v3, self.data.connector)
common.get_target_wwns.assert_called_once_with(
EMCVMAXCommonData.storage_system,
EMCVMAXCommonData.connector)
numTargetWwns = len(EMCVMAXCommonData.target_wwns)
self.assertEqual(numTargetWwns, len(data['data']))
# Bug https://bugs.launchpad.net/cinder/+bug/1440154
@mock.patch.object(
@ -8165,6 +8175,36 @@ class EMCVMAXFCTest(test.TestCase):
portGroupInstanceName, initiatorGroupInstanceName)
self.assertEqual(0, len(mvInstances))
@mock.patch.object(
emc_vmax_provision.EMCVMAXProvision,
'remove_device_from_storage_group')
def test_remove_device_from_storage_group(self, mock_remove):
conn = FakeEcomConnection()
common = self.driver.common
controllerConfigService = (
common.utils.find_controller_configuration_service(
conn, self.data.storage_system))
volumeInstanceName = (
conn.EnumerateInstanceNames("EMC_StorageVolume")[0])
volumeName = 'vol1'
extraSpecs = {'volume_backend_name': 'V3_BE',
'isV3': True,
'storagetype:pool': 'SRP_1',
'storagetype:workload': 'DSS',
'storagetype:slo': 'Bronze'}
masking = common.masking
volumeInstance = conn.GetInstance(volumeInstanceName)
storageGroupName = self.data.storagegroupname
storageGroupInstanceName = (
common.utils.find_storage_masking_group(
conn, controllerConfigService, storageGroupName))
masking.remove_device_from_storage_group(
conn, controllerConfigService, storageGroupInstanceName,
volumeInstance, volumeName, extraSpecs)
masking.provision.remove_device_from_storage_group.assert_called_with(
conn, controllerConfigService, storageGroupInstanceName,
volumeInstanceName, volumeName, extraSpecs)
@ddt.ddt
class EMCVMAXUtilsTest(test.TestCase):
@ -8671,7 +8711,10 @@ class EMCVMAXProvisionTest(test.TestCase):
self.driver = driver
self.driver.utils = emc_vmax_utils.EMCVMAXUtils(object)
def test_remove_device_from_storage_group(self):
@mock.patch.object(
emc_vmax_provision.EMCVMAXProvision,
'remove_device_from_storage_group')
def test_remove_device_from_storage_group(self, mock_remove):
conn = FakeEcomConnection()
controllerConfigService = (
self.driver.utils.find_controller_configuration_service(
@ -8685,8 +8728,6 @@ class EMCVMAXProvisionTest(test.TestCase):
'storagetype:workload': 'DSS',
'storagetype:slo': 'Bronze'}
masking = self.driver.common.masking
masking.provision.remove_device_from_storage_group = mock.Mock()
masking = self.driver.common.masking
volumeInstance = conn.GetInstance(volumeInstanceName)
storageGroupName = self.data.storagegroupname
storageGroupInstanceName = (

View File

@ -2327,10 +2327,10 @@ class EMCVMAXCommon(object):
{'volumeName': volumeName,
'storageGroupInstanceNames': storageGroupInstanceNames})
for storageGroupInstanceName in storageGroupInstanceNames:
self.provision.remove_device_from_storage_group(
self.masking.remove_device_from_storage_group(
self.conn, controllerConfigurationService,
storageGroupInstanceName,
volumeInstanceName, volumeName, extraSpecs)
storageGroupInstanceName, volumeInstanceName,
volumeName, extraSpecs)
def _find_lunmasking_scsi_protocol_controller(self, storageSystemName,
connector):

View File

@ -13,10 +13,10 @@
# License for the specific language governing permissions and limitations
# under the License.
from oslo_concurrency import lockutils
from oslo_log import log as logging
import six
from cinder import coordination
from cinder import exception
from cinder.i18n import _, _LE, _LI, _LW
from cinder.volume.drivers.emc import emc_vmax_fast
@ -54,13 +54,13 @@ class EMCVMAXMasking(object):
def setup_masking_view(self, conn, maskingViewDict, extraSpecs):
@lockutils.synchronized(maskingViewDict['maskingViewName'],
"emc-mv-", True)
def do_get_or_create_masking_view_and_map_lun():
@coordination.synchronized("emc-mv-{maskingViewDict[maskingViewName]}")
def do_get_or_create_masking_view_and_map_lun(maskingViewDict):
return self.get_or_create_masking_view_and_map_lun(conn,
maskingViewDict,
extraSpecs)
return do_get_or_create_masking_view_and_map_lun()
return do_get_or_create_masking_view_and_map_lun(
maskingViewDict)
def get_or_create_masking_view_and_map_lun(self, conn, maskingViewDict,
extraSpecs):
@ -641,10 +641,11 @@ class EMCVMAXMasking(object):
"before removing volume %(volumeName)s.",
{'length': len(assocVolumeInstanceNames),
'volumeName': volumeName})
volInstance = conn.GetInstance(volumeInstanceName, LocalOnly=False)
self.provision.remove_device_from_storage_group(
self._remove_volume_from_sg(
conn, controllerConfigService, storageGroupInstanceName,
volumeInstanceName, volumeName, maskingViewDict['extraSpecs'])
volInstance, maskingViewDict['extraSpecs'])
assocVolumeInstanceNames = self.get_devices_from_storage_group(
conn, storageGroupInstanceName)
@ -1730,26 +1731,30 @@ class EMCVMAXMasking(object):
{'volumeName': volumeName})
return failedRet
assocVolumeInstanceNames = self.get_devices_from_storage_group(
conn, defaultStorageGroupInstanceName)
@coordination.synchronized("emc-sg-{sgName}")
def do_remove_vol_from_sg(sgName):
assocVolumeInstanceNames = self.get_devices_from_storage_group(
conn, defaultStorageGroupInstanceName)
LOG.debug(
"There are %(length)lu associated with the default storage "
"group for fast before removing volume %(volumeName)s.",
{'length': len(assocVolumeInstanceNames),
'volumeName': volumeName})
LOG.debug(
"There are %(length)lu associated with the default storage group "
"for fast before removing volume %(volumeName)s.",
{'length': len(assocVolumeInstanceNames),
'volumeName': volumeName})
self.provision.remove_device_from_storage_group(
conn, controllerConfigService,
defaultStorageGroupInstanceName, volumeInstanceName,
volumeName, extraSpecs)
self.provision.remove_device_from_storage_group(
conn, controllerConfigService, defaultStorageGroupInstanceName,
volumeInstanceName, volumeName, extraSpecs)
assocVolumeInstanceNames = self.get_devices_from_storage_group(
conn, defaultStorageGroupInstanceName)
LOG.debug(
"There are %(length)lu associated with the default storage "
"group %(sg)s after removing volume %(volumeName)s.",
{'length': len(assocVolumeInstanceNames),
'sg': sgName, 'volumeName': volumeName})
assocVolumeInstanceNames = self.get_devices_from_storage_group(
conn, defaultStorageGroupInstanceName)
LOG.debug(
"There are %(length)lu associated with the default storage group "
"for fast after removing volume %(volumeName)s.",
{'length': len(assocVolumeInstanceNames),
'volumeName': volumeName})
do_remove_vol_from_sg(defaultStorageGroupInstanceName['ElementName'])
# Required for unit tests.
emptyStorageGroupInstanceName = (
@ -1891,7 +1896,6 @@ class EMCVMAXMasking(object):
def _remove_volume_from_sg(
self, conn, controllerConfigService, storageGroupInstanceName,
volumeInstance, extraSpecs):
"""Remove volume from storage group
:param conn: the ecom connection
@ -1902,32 +1906,78 @@ class EMCVMAXMasking(object):
"""
instance = conn.GetInstance(storageGroupInstanceName, LocalOnly=False)
storageGroupName = instance['ElementName']
mvInstanceName = self.get_masking_view_from_storage_group(
conn, storageGroupInstanceName)
if mvInstanceName is None:
LOG.debug("Unable to get masking view %(maskingView)s "
"from storage group.",
{'maskingView': mvInstanceName})
@lockutils.synchronized(storageGroupName + 'remove',
"emc-remove-sg", True)
def do_remove_volume_from_sg():
volumeInstanceNames = self.get_devices_from_storage_group(
conn, storageGroupInstanceName)
numVolInStorageGroup = len(volumeInstanceNames)
LOG.debug(
"There are %(numVol)d volumes in the storage group "
"%(maskingGroup)s.",
{'numVol': numVolInStorageGroup,
'maskingGroup': storageGroupInstanceName})
@coordination.synchronized("emc-sg-{storageGroup}")
def do_remove_volume_from_sg(storageGroup):
volumeInstanceNames = self.get_devices_from_storage_group(
conn, storageGroupInstanceName)
numVolInStorageGroup = len(volumeInstanceNames)
LOG.debug(
"There are %(numVol)d volumes in the storage group "
"%(maskingGroup)s.",
{'numVol': numVolInStorageGroup,
'maskingGroup': storageGroup})
if numVolInStorageGroup == 1:
# Last volume in the storage group.
self._last_vol_in_SG(
conn, controllerConfigService, storageGroupInstanceName,
storageGroupName, volumeInstance,
volumeInstance['ElementName'], extraSpecs)
else:
# Not the last volume so remove it from storage group
self._multiple_vols_in_SG(
conn, controllerConfigService, storageGroupInstanceName,
volumeInstance, volumeInstance['ElementName'],
numVolInStorageGroup, extraSpecs)
return do_remove_volume_from_sg()
if numVolInStorageGroup == 1:
# Last volume in the storage group.
self._last_vol_in_SG(
conn, controllerConfigService,
storageGroupInstanceName,
storageGroupName, volumeInstance,
volumeInstance['ElementName'], extraSpecs)
else:
# Not the last volume so remove it from storage group
self._multiple_vols_in_SG(
conn, controllerConfigService,
storageGroupInstanceName, volumeInstance,
volumeInstance['ElementName'],
numVolInStorageGroup, extraSpecs)
return do_remove_volume_from_sg(storageGroupName)
else:
# need to lock masking view when we are locking the storage
# group to avoid possible deadlock situations from concurrent
# processes
maskingViewInstance = conn.GetInstance(
mvInstanceName, LocalOnly=False)
maskingViewName = maskingViewInstance['ElementName']
@coordination.synchronized("emc-sg-{maskingView}")
def do_remove_volume_from_sg(maskingView):
@coordination.synchronized("emc-mv-{storageGroup}")
def inner_do_remove_volume_from_sg(storageGroup):
volumeInstanceNames = self.get_devices_from_storage_group(
conn, storageGroupInstanceName)
numVolInStorageGroup = len(volumeInstanceNames)
LOG.debug(
"There are %(numVol)d volumes in the storage group "
"%(maskingGroup)s associated with %(mvName)s",
{'numVol': numVolInStorageGroup,
'maskingGroup': storageGroup,
'mvName': maskingViewName})
if numVolInStorageGroup == 1:
# Last volume in the storage group.
self._last_vol_in_SG(
conn, controllerConfigService,
storageGroupInstanceName,
storageGroupName, volumeInstance,
volumeInstance['ElementName'], extraSpecs)
else:
# Not the last volume so remove it from storage group
self._multiple_vols_in_SG(
conn, controllerConfigService,
storageGroupInstanceName,
volumeInstance, volumeInstance['ElementName'],
numVolInStorageGroup, extraSpecs)
return inner_do_remove_volume_from_sg(storageGroupName)
return do_remove_volume_from_sg(maskingViewName)
def _last_vol_in_SG(
self, conn, controllerConfigService, storageGroupInstanceName,
@ -1958,9 +2008,6 @@ class EMCVMAXMasking(object):
mvInstanceName = self.get_masking_view_from_storage_group(
conn, storageGroupInstanceName)
if mvInstanceName is None:
LOG.debug("Unable to get masking view %(maskingView)s "
"from storage group.",
{'maskingView': mvInstanceName})
# Remove the volume from the storage group and delete the SG.
self._remove_last_vol_and_delete_sg(
conn, controllerConfigService,
@ -1973,8 +2020,6 @@ class EMCVMAXMasking(object):
mvInstanceName, LocalOnly=False)
maskingViewName = maskingViewInstance['ElementName']
@lockutils.synchronized(maskingViewName,
"emc-mv-", True)
def do_delete_mv_ig_and_sg():
return self._delete_mv_ig_and_sg(
conn, controllerConfigService, mvInstanceName,
@ -2739,3 +2784,20 @@ class EMCVMAXMasking(object):
LOG.error(_LE(
"Cannot get port group name."))
return portGroupName, errorMessage
@coordination.synchronized('emc-sg-'
'{storageGroupInstanceName[ElementName]}')
def remove_device_from_storage_group(
self, conn, controllerConfigService, storageGroupInstanceName,
volumeInstance, volumeName, extraSpecs):
"""Remove a device from a storage group.
:param conn: the connection to the ecom server
:param controllerConfigService: the controller config service
:param storageGroupInstanceName: the sg instance
:param volumeInstance: the volume instance
:param extraSpecs: the extra specifications
"""
return self.provision.remove_device_from_storage_group(
conn, controllerConfigService, storageGroupInstanceName,
volumeInstance, volumeName, extraSpecs)

View File

@ -14,10 +14,10 @@
# under the License.
import time
from oslo_concurrency import lockutils
from oslo_log import log as logging
import six
from cinder import coordination
from cinder import exception
from cinder.i18n import _
from cinder.volume.drivers.emc import emc_vmax_utils
@ -160,9 +160,8 @@ class EMCVMAXProvision(object):
"""
startTime = time.time()
@lockutils.synchronized(storageGroupName,
"emc-sg-", True)
def do_create_storage_group():
@coordination.synchronized("emc-sg-{storageGroupName}")
def do_create_storage_group(storageGroupName):
rc, job = conn.InvokeMethod(
'CreateGroup', controllerConfigService,
GroupName=storageGroupName,
@ -191,7 +190,7 @@ class EMCVMAXProvision(object):
conn, job, storageGroupName)
return foundStorageGroupInstanceName
return do_create_storage_group()
return do_create_storage_group(storageGroupName)
def create_storage_group_no_members(
self, conn, controllerConfigService, groupName, extraSpecs):
@ -296,8 +295,6 @@ class EMCVMAXProvision(object):
raise exception.VolumeBackendAPIException(
data=exceptionMessage)
@lockutils.synchronized(storageGroupInstance['ElementName'],
"emc-sg-", True)
def do_remove_volume_from_sg():
startTime = time.time()
rc, jobDict = conn.InvokeMethod('RemoveMembers',
@ -347,9 +344,9 @@ class EMCVMAXProvision(object):
raise exception.VolumeBackendAPIException(
data=exceptionMessage)
@lockutils.synchronized(storageGroupInstance['ElementName'],
"emc-sg-", True)
def do_add_volume_to_sg():
@coordination.synchronized('emc-sg-'
'{storageGroupInstance[ElementName]}')
def do_add_volume_to_sg(storageGroupInstance):
startTime = time.time()
rc, job = conn.InvokeMethod(
'AddMembers', controllerConfigService,
@ -374,7 +371,7 @@ class EMCVMAXProvision(object):
{'delta': self.utils.get_time_delta(startTime,
time.time())})
return rc
return do_add_volume_to_sg()
return do_add_volume_to_sg(storageGroupInstance)
def unbind_volume_from_storage_pool(
self, conn, storageConfigService,

View File

@ -15,10 +15,10 @@
import time
from oslo_concurrency import lockutils
from oslo_log import log as logging
import six
from cinder import coordination
from cinder import exception
from cinder.i18n import _, _LE, _LW
from cinder.volume.drivers.emc import emc_vmax_utils
@ -120,10 +120,10 @@ class EMCVMAXProvisionV3(object):
LOG.error(exceptionMessage)
raise exception.VolumeBackendAPIException(
data=exceptionMessage)
sgName = storageGroupInstance['ElementName']
@lockutils.synchronized(storageGroupInstance['ElementName'],
"emc-sg-", True)
def do_create_volume_from_sg():
@coordination.synchronized("emc-sg-{storageGroup}")
def do_create_volume_from_sg(storageGroup):
startTime = time.time()
rc, job = conn.InvokeMethod(
@ -160,7 +160,7 @@ class EMCVMAXProvisionV3(object):
volumeDict = self.get_volume_dict_from_job(conn, job['Job'])
return volumeDict, rc
return do_create_volume_from_sg()
return do_create_volume_from_sg(sgName)
def _find_new_storage_group(
self, conn, maskingGroupDict, storageGroupName):
@ -270,9 +270,8 @@ class EMCVMAXProvisionV3(object):
raise exception.VolumeBackendAPIException(
data=exceptionMessage)
@lockutils.synchronized(storageGroupInstance['ElementName'],
"emc-sg-", True)
def do_create_element_replica():
@coordination.synchronized("emc-sg-{storageGroupName}")
def do_create_element_replica(storageGroupName):
if targetInstance is None and rsdInstance is None:
rc, job = conn.InvokeMethod(
'CreateElementReplica', repServiceInstanceName,
@ -305,7 +304,7 @@ class EMCVMAXProvisionV3(object):
{'delta': self.utils.get_time_delta(startTime,
time.time())})
return rc, job
return do_create_element_replica()
return do_create_element_replica(storageGroupInstance['ElementName'])
def _create_element_replica_extra_params(
self, conn, repServiceInstanceName, cloneName, syncType,
@ -391,8 +390,8 @@ class EMCVMAXProvisionV3(object):
"""
startTime = time.time()
@lockutils.synchronized(groupName, "emc-sg-", True)
def do_create_storage_group_v3():
@coordination.synchronized("emc-sg-{sgGroupName}")
def do_create_storage_group_v3(sgGroupName):
if slo and workload:
rc, job = conn.InvokeMethod(
'CreateGroup',
@ -430,7 +429,7 @@ class EMCVMAXProvisionV3(object):
conn, job, groupName)
return foundStorageGroupInstanceName
return do_create_storage_group_v3()
return do_create_storage_group_v3(groupName)
def get_storage_pool_capability(self, conn, poolInstanceName):
"""Get the pool capability.