diff --git a/cinder/tests/unit/volume/drivers/dell_emc/test_vmax.py b/cinder/tests/unit/volume/drivers/dell_emc/test_vmax.py index 041342e3b..deb84b104 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/test_vmax.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/test_vmax.py @@ -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 = ( diff --git a/cinder/volume/drivers/dell_emc/vmax/common.py b/cinder/volume/drivers/dell_emc/vmax/common.py index 8a95e84d1..4c959242e 100644 --- a/cinder/volume/drivers/dell_emc/vmax/common.py +++ b/cinder/volume/drivers/dell_emc/vmax/common.py @@ -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): diff --git a/cinder/volume/drivers/dell_emc/vmax/masking.py b/cinder/volume/drivers/dell_emc/vmax/masking.py index 11ded8eeb..05059b6ec 100644 --- a/cinder/volume/drivers/dell_emc/vmax/masking.py +++ b/cinder/volume/drivers/dell_emc/vmax/masking.py @@ -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) diff --git a/cinder/volume/drivers/dell_emc/vmax/provision.py b/cinder/volume/drivers/dell_emc/vmax/provision.py index 66a90bca2..27cfb9b4b 100644 --- a/cinder/volume/drivers/dell_emc/vmax/provision.py +++ b/cinder/volume/drivers/dell_emc/vmax/provision.py @@ -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, diff --git a/cinder/volume/drivers/dell_emc/vmax/provision_v3.py b/cinder/volume/drivers/dell_emc/vmax/provision_v3.py index 66617ea8c..22ffd4965 100644 --- a/cinder/volume/drivers/dell_emc/vmax/provision_v3.py +++ b/cinder/volume/drivers/dell_emc/vmax/provision_v3.py @@ -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.