From 4461b8431cc02250467cfff8e762836fb1ef1486 Mon Sep 17 00:00:00 2001 From: Helen Walsh Date: Wed, 11 Nov 2015 22:09:59 +0000 Subject: [PATCH] EMC VMAX - Fix for last volume in VMAX3 storage group This patch fixes bug in the deletion the last volume in a storage group for the VMAX3. If a masking view did not exist we tried to get an instance of it which threw an exception. Now we handle both scenarios correctly. Closes-Bug: #1512796 Change-Id: Ideb2aaffe4fbc1ae2137d569d7b137ac0edef573 --- cinder/tests/unit/test_emc_vmax.py | 79 +++++++++ cinder/volume/drivers/emc/emc_vmax_fc.py | 1 + cinder/volume/drivers/emc/emc_vmax_iscsi.py | 1 + cinder/volume/drivers/emc/emc_vmax_masking.py | 159 +++++++++++++----- 4 files changed, 194 insertions(+), 46 deletions(-) diff --git a/cinder/tests/unit/test_emc_vmax.py b/cinder/tests/unit/test_emc_vmax.py index 1c688e65f..259e6d84c 100644 --- a/cinder/tests/unit/test_emc_vmax.py +++ b/cinder/tests/unit/test_emc_vmax.py @@ -5603,6 +5603,85 @@ class EMCV3DriverTestCase(test.TestCase): 'isV3': True, 'portgroupname': 'OS-portgroup-PG'} + def default_vol(self): + vol = EMC_StorageVolume() + vol['name'] = self.data.test_volume['name'] + vol['CreationClassName'] = 'Symm_StorageVolume' + vol['ElementName'] = self.data.test_volume['id'] + vol['DeviceID'] = self.data.test_volume['device_id'] + vol['Id'] = self.data.test_volume['id'] + vol['SystemName'] = self.data.storage_system + vol['NumberOfBlocks'] = self.data.test_volume['NumberOfBlocks'] + vol['BlockSize'] = self.data.test_volume['BlockSize'] + # Added vol to vol.path + vol['SystemCreationClassName'] = 'Symm_StorageSystem' + vol.path = vol + vol.path.classname = vol['CreationClassName'] + return vol + + def default_storage_group(self): + storagegroup = {} + storagegroup['CreationClassName'] = ( + self.data.storagegroup_creationclass) + storagegroup['ElementName'] = 'no_masking_view' + return storagegroup + + def test_last_vol_in_SG_with_MV(self): + conn = self.fake_ecom_connection() + controllerConfigService = ( + self.driver.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( + conn, controllerConfigService, storageGroupName)) + + vol = self.default_vol() + self.driver.common.masking._delete_mv_and_sg = mock.Mock() + self.assertTrue(self.driver.common.masking._last_vol_in_SG( + conn, controllerConfigService, storageGroupInstanceName, + storageGroupName, vol, vol['name'], extraSpecs)) + + def test_last_vol_in_SG_no_MV(self): + conn = self.fake_ecom_connection() + controllerConfigService = ( + self.driver.common.utils.find_controller_configuration_service( + conn, self.data.storage_system)) + + extraSpecs = self.default_extraspec() + self.driver.common.masking.get_masking_view_from_storage_group = ( + mock.Mock(return_value=None)) + self.driver.common.masking.utils.get_existing_instance = ( + mock.Mock(return_value=None)) + storagegroup = self.default_storage_group() + + vol = self.default_vol() + self.assertTrue(self.driver.common.masking._last_vol_in_SG( + conn, controllerConfigService, storagegroup, + storagegroup['ElementName'], vol, vol['name'], extraSpecs)) + + def test_last_vol_in_SG_no_MV_fail(self): + self.driver.common.masking.utils.get_existing_instance = ( + mock.Mock(return_value='value')) + conn = self.fake_ecom_connection() + controllerConfigService = ( + self.driver.common.utils.find_controller_configuration_service( + conn, self.data.storage_system)) + + extraSpecs = self.default_extraspec() + vol = self.default_vol() + storagegroup = self.default_storage_group() + storagegroup['ElementName'] = 'no_masking_view' + + self.assertRaises(exception.VolumeBackendAPIException, + self.driver.common.masking._last_vol_in_SG, + conn, controllerConfigService, + storagegroup, storagegroup['ElementName'], vol, + vol['name'], extraSpecs) + @mock.patch.object( emc_vmax_utils.EMCVMAXUtils, 'find_storageSystem', diff --git a/cinder/volume/drivers/emc/emc_vmax_fc.py b/cinder/volume/drivers/emc/emc_vmax_fc.py index 40843c0fb..484286439 100644 --- a/cinder/volume/drivers/emc/emc_vmax_fc.py +++ b/cinder/volume/drivers/emc/emc_vmax_fc.py @@ -53,6 +53,7 @@ class EMCVMAXFCDriver(driver.FibreChannelDriver): https://blueprints.launchpad.net/cinder/+spec/vmax3-extend-volume - Incorrect SG selected on an attach (#1515176) - Cleanup Zoning (bug #1501938) NOTE: FC only + - Last volume in SG fix """ VERSION = "2.3.0" diff --git a/cinder/volume/drivers/emc/emc_vmax_iscsi.py b/cinder/volume/drivers/emc/emc_vmax_iscsi.py index 3240754a5..05eeb97f8 100644 --- a/cinder/volume/drivers/emc/emc_vmax_iscsi.py +++ b/cinder/volume/drivers/emc/emc_vmax_iscsi.py @@ -59,6 +59,7 @@ class EMCVMAXISCSIDriver(driver.ISCSIDriver): https://blueprints.launchpad.net/cinder/+spec/vmax3-extend-volume - Incorrect SG selected on an attach (#1515176) - Cleanup Zoning (bug #1501938) NOTE: FC only + - Last volume in SG fix """ VERSION = "2.3.0" diff --git a/cinder/volume/drivers/emc/emc_vmax_masking.py b/cinder/volume/drivers/emc/emc_vmax_masking.py index d2bed73f8..c3f794f03 100644 --- a/cinder/volume/drivers/emc/emc_vmax_masking.py +++ b/cinder/volume/drivers/emc/emc_vmax_masking.py @@ -1784,11 +1784,11 @@ class EMCVMAXMasking(object): storageSystemInstanceName = self.utils.find_storage_system( conn, controllerConfigService) - numVolInMaskingView = len(volumeInstanceNames) + numVolInStorageGroup = len(volumeInstanceNames) LOG.debug( "There are %(numVol)d volumes in the storage group " "%(maskingGroup)s.", - {'numVol': numVolInMaskingView, + {'numVol': numVolInStorageGroup, 'maskingGroup': storageGroupInstanceName}) if not isV3: @@ -1796,22 +1796,57 @@ class EMCVMAXMasking(object): self._get_tiering_info(conn, storageSystemInstanceName, fastPolicyName)) - if numVolInMaskingView == 1: + if numVolInStorageGroup == 1: # Last volume in the storage group. - LOG.warning(_LW("Only one volume remains in storage group " - "%(sgname)s. Driver will attempt cleanup."), - {'sgname': storageGroupName}) - mvInstanceName = self.get_masking_view_from_storage_group( - conn, storageGroupInstanceName) - if mvInstanceName is None: - LOG.warning(_LW("Unable to get masking view %(maskingView)s " - "from storage group."), - {'maskingView': mvInstanceName}) - else: - maskingViewInstance = conn.GetInstance( - mvInstanceName, LocalOnly=False) - maskingViewName = maskingViewInstance['ElementName'] + self._last_vol_in_SG( + conn, controllerConfigService, storageGroupInstanceName, + storageGroupName, volumeInstance, volumeName, extraSpecs) + else: + # Not the last volume so remove it from storage group in + # the masking view. + self._multiple_vols_in_SG( + conn, controllerConfigService, storageGroupInstanceName, + storageGroupName, volumeInstance, volumeName, + numVolInStorageGroup, fastPolicyName, extraSpecs) + + return storageGroupInstanceName + + def _last_vol_in_SG( + self, conn, controllerConfigService, storageGroupInstanceName, + storageGroupName, volumeInstance, volumeName, extraSpecs): + """Steps if the volume is the last in a storage group. + + 1. Check if the volume is in a masking view. + 2. If it is in a masking view, delete the masking view, + remove the volume from the storage group, and delete + the storage group. + 3. If it is not in a masking view, remove the volume from the + storage group and delete the storage group. + + :param conn: the ecom connection + :param controllerConfigService: storage system instance name + :param storageGroupInstanceName: the SG instance name + :param storageGroupName: the Storage group name (String) + :param volumeInstance: the volume instance + :param volumeName: the volume name + :param extraSpecs: the extra specifications + """ + status = False + LOG.debug("Only one volume remains in storage group " + "%(sgname)s. Driver will attempt cleanup.", + {'sgname': storageGroupName}) + 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}) + else: + maskingViewInstance = conn.GetInstance( + mvInstanceName, LocalOnly=False) + maskingViewName = maskingViewInstance['ElementName'] + if mvInstanceName: maskingViewInstance = conn.GetInstance( mvInstanceName, LocalOnly=False) maskingViewName = maskingViewInstance['ElementName'] @@ -1822,41 +1857,73 @@ class EMCVMAXMasking(object): return self._delete_mv_and_sg( conn, controllerConfigService, mvInstanceName, maskingViewName, storageGroupInstanceName, - storageGroupName, volumeInstance, volumeName, extraSpecs) + storageGroupName, volumeInstance, volumeName, + extraSpecs) do_delete_mv_and_sg() + status = True else: - # Not the last volume so remove it from storage group in - # the masking view. - LOG.debug("Start: number of volumes in masking storage group: " - "%(numVol)d", {'numVol': len(volumeInstanceNames)}) - self.provision.remove_device_from_storage_group( - conn, controllerConfigService, storageGroupInstanceName, - volumeInstance.path, volumeName, extraSpecs) + # Remove the volume from the storage group and delete the SG. + self._remove_last_vol_and_delete_sg( + conn, controllerConfigService, + storageGroupInstanceName, + storageGroupName, volumeInstance.path, + volumeName, extraSpecs) + status = True + return status - LOG.debug( - "RemoveMembers for volume %(volumeName)s completed " - "successfully.", {'volumeName': volumeName}) + def _multiple_vols_in_SG( + self, conn, controllerConfigService, storageGroupInstanceName, + storageGroupName, volumeInstance, volumeName, numVolsInSG, + fastPolicyName, extraSpecs): + """Necessary steps if the volume is not the last in the SG. - # Add it back to the default storage group. - if isV3: - self._return_volume_to_default_storage_group_v3( - conn, controllerConfigService, storageGroupName, - volumeInstance, volumeName, storageSystemInstanceName, - extraSpecs) - else: - # V2 if FAST POLICY enabled, move the volume to the default SG. - if fastPolicyName is not None and isTieringPolicySupported: - self._cleanup_tiering( - conn, controllerConfigService, fastPolicyName, - volumeInstance, volumeName, extraSpecs) - - volumeInstanceNames = self.get_devices_from_storage_group( - conn, storageGroupInstanceName) - LOG.debug( - "End: number of volumes in masking storage group: %(numVol)d.", - {'numVol': len(volumeInstanceNames)}) + 1. Remove the volume from the SG. + 2. Return the volume to default SG if necessary. - return storageGroupInstanceName + :param conn: the ecom connection + :param controllerConfigService: storage system instance name + :param storageGroupInstanceName: the SG instance name + :param storageGroupName: the Storage group name (String) + :param volumeInstance: the volume instance + :param volumeName: the volume name + :param numVolsInSG: the number of volumes in the SG + :param fastPolicyName: the FAST policy name + :param extraSpecs: the extra specifications + """ + storageSystemInstanceName = self.utils.find_storage_system( + conn, controllerConfigService) + if not extraSpecs[ISV3]: + isTieringPolicySupported, __ = ( + self._get_tiering_info(conn, storageSystemInstanceName, + fastPolicyName)) + LOG.debug("Start: number of volumes in masking storage group: " + "%(numVol)d", {'numVol': numVolsInSG}) + self.provision.remove_device_from_storage_group( + conn, controllerConfigService, storageGroupInstanceName, + volumeInstance.path, volumeName, extraSpecs) + + LOG.debug( + "RemoveMembers for volume %(volumeName)s completed " + "successfully.", {'volumeName': volumeName}) + + # Add it back to the default storage group. + if extraSpecs[ISV3]: + self._return_volume_to_default_storage_group_v3( + conn, controllerConfigService, storageGroupName, + volumeInstance, volumeName, storageSystemInstanceName, + extraSpecs) + else: + # V2 if FAST POLICY enabled, move the volume to the default SG. + if fastPolicyName is not None and isTieringPolicySupported: + self._cleanup_tiering( + conn, controllerConfigService, fastPolicyName, + volumeInstance, volumeName, extraSpecs) + + volumeInstanceNames = self.get_devices_from_storage_group( + conn, storageGroupInstanceName) + LOG.debug( + "End: number of volumes in masking storage group: %(numVol)d.", + {'numVol': len(volumeInstanceNames)}) def _delete_mv_and_sg(self, conn, controllerConfigService, mvInstanceName, maskingViewName, storageGroupInstanceName, -- 2.45.2