diff --git a/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py b/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py index c132b8f9f86..2f5373e6afb 100644 --- a/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py +++ b/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py @@ -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)) @@ -6532,6 +6535,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', @@ -6557,20 +6567,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( @@ -8183,6 +8193,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): @@ -8689,7 +8729,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( @@ -8703,8 +8746,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 = ( diff --git a/cinder/volume/drivers/emc/emc_vmax_common.py b/cinder/volume/drivers/emc/emc_vmax_common.py index 06a172db2f6..f37ae3a5c9a 100644 --- a/cinder/volume/drivers/emc/emc_vmax_common.py +++ b/cinder/volume/drivers/emc/emc_vmax_common.py @@ -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): diff --git a/cinder/volume/drivers/emc/emc_vmax_masking.py b/cinder/volume/drivers/emc/emc_vmax_masking.py index 7678836b096..bf08caf7c50 100644 --- a/cinder/volume/drivers/emc/emc_vmax_masking.py +++ b/cinder/volume/drivers/emc/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.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) diff --git a/cinder/volume/drivers/emc/emc_vmax_provision.py b/cinder/volume/drivers/emc/emc_vmax_provision.py index 70855698c40..56fadf104c5 100644 --- a/cinder/volume/drivers/emc/emc_vmax_provision.py +++ b/cinder/volume/drivers/emc/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.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, diff --git a/cinder/volume/drivers/emc/emc_vmax_provision_v3.py b/cinder/volume/drivers/emc/emc_vmax_provision_v3.py index 2273b0a736f..4e1cf346ad1 100644 --- a/cinder/volume/drivers/emc/emc_vmax_provision_v3.py +++ b/cinder/volume/drivers/emc/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.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, @@ -399,8 +398,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', @@ -438,7 +437,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.