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
This commit is contained in:
Helen Walsh 2017-01-30 16:11:22 +00:00
parent 474b443401
commit 84d463380a
5 changed files with 202 additions and 120 deletions

View File

@ -3434,42 +3434,33 @@ class VMAXISCSIDriverNoFastTestCase(test.TestCase):
self.driver.initialize_connection(self.data.test_volume,
self.data.connector)
@mock.patch.object(
masking.VMAXMasking,
'_check_adding_volume_to_storage_group',
return_value=None)
@mock.patch.object(
common.VMAXCommon,
'_is_same_host',
return_value=False)
@mock.patch.object(
utils.VMAXUtils,
'find_storage_masking_group',
return_value=VMAXCommonData.default_sg_instance_name)
@mock.patch.object(
common.VMAXCommon,
'find_device_number',
return_value={'hostlunid': 1,
'storagesystem': VMAXCommonData.storage_system})
@mock.patch.object(
masking.VMAXMasking,
'_wrap_get_storage_group_from_volume',
return_value=None)
@mock.patch.object(
volume_types,
'get_volume_type_extra_specs',
return_value={'volume_backend_name': 'ISCSINoFAST'})
def test_map_live_migration_no_fast_success(self,
_mock_volume_type,
mock_wrap_group,
mock_wrap_device,
mock_storage_group,
mock_same_host,
mock_check):
mock_same_host):
utils.LIVE_MIGRATION_FILE = (self.tempdir +
'/livemigrationarray')
self.driver.initialize_connection(self.data.test_volume,
self.data.connector)
extraSpecs = self.data.extra_specs
rollback_dict = self.driver.common._populate_masking_dict(
self.data.test_volume, self.data.connector, extraSpecs)
with mock.patch.object(self.driver.common.masking,
'setup_masking_view',
return_value=rollback_dict):
self.driver.initialize_connection(self.data.test_volume,
self.data.connector)
@mock.patch.object(
masking.VMAXMasking,
@ -6158,22 +6149,24 @@ class EMCV3DriverTestCase(test.TestCase):
storagegroup['ElementName'] = 'no_masking_view'
return storagegroup
def test_last_vol_in_SG_with_MV(self):
@mock.patch.object(
masking.VMAXMasking,
'_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))
@ -6675,6 +6668,9 @@ class EMCV3DriverTestCase(test.TestCase):
self.data.test_volume,
self.data.connector)
@mock.patch.object(
masking.VMAXMasking,
'remove_and_reset_members')
@mock.patch.object(
utils.VMAXUtils,
'get_volume_element_name',
@ -6705,20 +6701,18 @@ class EMCV3DriverTestCase(test.TestCase):
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_element_name):
mock_element_name, mock_remove):
common = self.driver.common
common.get_target_wwns = mock.Mock(
return_value=VMAXCommonData.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(
VMAXCommonData.storage_system, VMAXCommonData.connector)
numTargetWwns = len(VMAXCommonData.target_wwns)
self.assertEqual(numTargetWwns, len(data['data']))
with mock.patch.object(common, 'get_target_wwns',
return_value=VMAXCommonData.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(
VMAXCommonData.storage_system, VMAXCommonData.connector)
numTargetWwns = len(VMAXCommonData.target_wwns)
self.assertEqual(numTargetWwns, len(data['data']))
# Bug https://bugs.launchpad.net/cinder/+bug/1440154
@mock.patch.object(
@ -8338,6 +8332,36 @@ class VMAXFCTest(test.TestCase):
self.assertEqual('fibre_channel', data['driver_volume_type'])
self.assertEqual(2, len(data['data']['target_wwn']))
@mock.patch.object(
provision.VMAXProvision,
'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 VMAXUtilsTest(test.TestCase):
@ -9266,7 +9290,10 @@ class VMAXProvisionTest(test.TestCase):
self.driver = driver
self.driver.utils = utils.VMAXUtils(object)
def test_remove_device_from_storage_group(self):
@mock.patch.object(
provision.VMAXProvision,
'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(
@ -9280,8 +9307,6 @@ class VMAXProvisionTest(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

@ -2651,10 +2651,10 @@ class VMAXCommon(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.dell_emc.vmax import fast
@ -54,13 +54,13 @@ class VMAXMasking(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):
@ -649,10 +649,11 @@ class VMAXMasking(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)
@ -1740,26 +1741,31 @@ class VMAXMasking(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 "
"for fast after removing volume %(volumeName)s.",
{'length': len(assocVolumeInstanceNames),
'volumeName': volumeName})
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})
do_remove_vol_from_sg(defaultStorageGroupInstanceName['ElementName'])
# Required for unit tests.
emptyStorageGroupInstanceName = (
@ -1901,7 +1907,6 @@ class VMAXMasking(object):
def _remove_volume_from_sg(
self, conn, controllerConfigService, storageGroupInstanceName,
volumeInstance, extraSpecs):
"""Remove volume from storage group
:param conn: the ecom connection
@ -1912,32 +1917,76 @@ class VMAXMasking(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-mv-{maskingView}")
@coordination.synchronized("emc-sg-{storageGroup}")
def do_remove_volume_from_sg(maskingView, 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': maskingView})
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(maskingViewName, storageGroupName)
def _last_vol_in_SG(
self, conn, controllerConfigService, storageGroupInstanceName,
@ -1968,9 +2017,6 @@ class VMAXMasking(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,
@ -1983,8 +2029,6 @@ class VMAXMasking(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,
@ -2761,3 +2805,20 @@ class VMAXMasking(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.dell_emc.vmax import utils
@ -160,9 +160,8 @@ class VMAXProvision(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 VMAXProvision(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 VMAXProvision(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',
@ -348,9 +345,9 @@ class VMAXProvision(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,
@ -375,7 +372,7 @@ class VMAXProvision(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.dell_emc.vmax import utils
@ -126,10 +126,10 @@ class VMAXProvisionV3(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(
@ -166,7 +166,7 @@ class VMAXProvisionV3(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):
@ -298,9 +298,8 @@ class VMAXProvisionV3(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,
@ -333,7 +332,7 @@ class VMAXProvisionV3(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_remote_element_replica(
self, conn, repServiceInstanceName, cloneName, syncType,
@ -474,8 +473,8 @@ class VMAXProvisionV3(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 doDisableCompression:
if slo and workload:
rc, job = conn.InvokeMethod(
@ -524,7 +523,7 @@ class VMAXProvisionV3(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.