From de31445ca9d79466b7c0a26dfe4e7fcee58b9fb5 Mon Sep 17 00:00:00 2001 From: Helen Walsh Date: Fri, 2 Jun 2017 22:53:01 +0000 Subject: [PATCH] VMAX driver - failure in initiator group cleanup There is an issue where the initiator group does not get cleaned up if the host name is hyphenated. This patch rectifies the issue by getting the host name from the connector object instead of parsing it from the masking view name. This fix is being applied directly to stable ocata because the SMI-S driver has been replaced by REST in Pike. Change-Id: I064ec043c136a46c9d4f3439f849c1843f76a4cf Closes-Bug: #1694546 --- .../unit/volume/drivers/dell_emc/test_vmax.py | 38 ++--- cinder/volume/drivers/dell_emc/vmax/common.py | 6 +- .../volume/drivers/dell_emc/vmax/masking.py | 131 ++++++++++-------- 3 files changed, 94 insertions(+), 81 deletions(-) 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 4e6f3bbee..faf5f96ea 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/test_vmax.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/test_vmax.py @@ -258,6 +258,7 @@ class VMAXCommonData(object): 'wwpns': [wwpn1, wwpn2], 'wwnns': ["223456789012345", "223456789054321"], 'host': 'fakehost'} + short_host_name = 'fakehost' target_wwns = [wwn[::-1] for wwn in connector['wwpns']] @@ -2823,7 +2824,6 @@ class VMAXISCSIDriverNoFastTestCase(test.TestCase): def test_last_volume_delete_initiator_group_exception(self): extraSpecs = {'volume_backend_name': 'ISCSINoFAST'} conn = self.fake_ecom_connection() - host = self.data.lunmaskctrl_name.split("-")[1] controllerConfigService = ( self.driver.utils.find_controller_configuration_service( conn, self.data.storage_system)) @@ -2844,14 +2844,13 @@ class VMAXISCSIDriverNoFastTestCase(test.TestCase): exception.VolumeBackendAPIException, self.driver.common.masking._last_volume_delete_initiator_group, conn, controllerConfigService, initiatorGroupInstanceName, - extraSpecs, host) + extraSpecs, self.data.short_host_name) # Bug 1504192 - if the last volume is being unmapped and the masking view # goes away, cleanup the initiators and associated initiator group. def test_last_volume_delete_initiator_group(self): extraSpecs = {'volume_backend_name': 'ISCSINoFAST'} conn = self.fake_ecom_connection() - host = self.data.lunmaskctrl_name.split("-")[1] controllerConfigService = ( self.driver.utils.find_controller_configuration_service( conn, self.data.storage_system)) @@ -2866,14 +2865,14 @@ class VMAXISCSIDriverNoFastTestCase(test.TestCase): # initiator group will not be deleted. self.driver.common.masking._last_volume_delete_initiator_group( conn, controllerConfigService, initiatorGroupInstanceName, - extraSpecs, host) + extraSpecs, self.data.short_host_name) # Path 2: initiator group name is not the default name so the # initiator group will not be deleted. initGroup2 = initiatorGroupInstanceName initGroup2['ElementName'] = "different-name-ig" self.driver.common.masking._last_volume_delete_initiator_group( conn, controllerConfigService, initGroup2, - extraSpecs, host) + extraSpecs, self.data.short_host_name) # Path 3: No Masking view and IG is the default IG, so initiators # associated with the Initiator group and the initiator group will # be deleted. @@ -2883,7 +2882,7 @@ class VMAXISCSIDriverNoFastTestCase(test.TestCase): mock.Mock(return_value=True)) self.driver.common.masking._last_volume_delete_initiator_group( conn, controllerConfigService, initiatorGroupInstanceName, - extraSpecs, host) + extraSpecs, self.data.short_host_name) job = { 'Job': {'InstanceID': '9999', 'status': 'success', 'type': None}} conn.InvokeMethod = mock.Mock(return_value=(4096, job)) @@ -2893,7 +2892,7 @@ class VMAXISCSIDriverNoFastTestCase(test.TestCase): # to complete. self.driver.common.masking._last_volume_delete_initiator_group( conn, controllerConfigService, initiatorGroupInstanceName, - extraSpecs, host) + extraSpecs, self.data.short_host_name) # Tests removal of last volume in a storage group V2 def test_remove_and_reset_members(self): @@ -6189,9 +6188,10 @@ class EMCV3DriverTestCase(test.TestCase): 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')) + @mock.patch.object(utils.VMAXUtils, + 'get_existing_instance', + return_value='value') + def test_last_vol_in_SG_no_MV_fail(self, mock_ins): conn = self.fake_ecom_connection() controllerConfigService = ( self.driver.common.utils.find_controller_configuration_service( @@ -6206,7 +6206,7 @@ class EMCV3DriverTestCase(test.TestCase): self.driver.common.masking._last_vol_in_SG, conn, controllerConfigService, storagegroup, storagegroup['ElementName'], vol, - vol['name'], extraSpecs) + vol['name'], extraSpecs, self.data.connector) @mock.patch.object( utils.VMAXUtils, @@ -7910,12 +7910,12 @@ class VMAXMaskingTest(test.TestCase): controllerConfigService = ( self.driver.utils.find_controller_configuration_service( conn, self.data.storage_system)) - masking._remove_volume_from_sg = mock.Mock() - masking._cleanup_deletion_v3( - conn, controllerConfigService, volumeInstance, extraSpecs) - masking._remove_volume_from_sg.assert_called_with( - conn, controllerConfigService, storageGroupInstanceName, - volumeInstance, extraSpecs) + with mock.patch.object(masking, '_remove_volume_from_sg') as mock_rm: + masking._cleanup_deletion_v3( + conn, controllerConfigService, volumeInstance, extraSpecs) + mock_rm.assert_called_with( + conn, controllerConfigService, storageGroupInstanceName, + volumeInstance, extraSpecs, None) # Bug 1552426 - failed rollback on V3 when MV issue def test_check_ig_rollback(self): @@ -7932,7 +7932,6 @@ class VMAXMaskingTest(test.TestCase): 'pool': 'SRP_1', } igGroupName = self.data.initiatorgroup_name - host = igGroupName.split("-")[1] igInstance = masking._find_initiator_masking_group( conn, controllerConfigService, self.data.initiatorNames) # path 1: The masking view creation process created a now stale @@ -7943,7 +7942,8 @@ class VMAXMaskingTest(test.TestCase): igGroupName, connector, extraSpecs) (masking._last_volume_delete_initiator_group. assert_called_once_with(conn, controllerConfigService, - igInstance, extraSpecs, host)) + igInstance, extraSpecs, + self.data.short_host_name)) # path 2: No initiator group was created before the masking # view process failed. with mock.patch.object(masking, diff --git a/cinder/volume/drivers/dell_emc/vmax/common.py b/cinder/volume/drivers/dell_emc/vmax/common.py index 9b761f5a5..a7c9f40dd 100644 --- a/cinder/volume/drivers/dell_emc/vmax/common.py +++ b/cinder/volume/drivers/dell_emc/vmax/common.py @@ -810,7 +810,11 @@ class VMAXCommon(object): volumename = volume['name'] LOG.info(_LI("Terminate connection: %(volume)s."), {'volume': volumename}) - + if not connector: + exception_message = (_("The connector object from nova " + "cannot be None.")) + raise exception.VolumeBackendAPIException( + data=exception_message) self._unmap_lun(volume, connector) def extend_volume(self, volume, newSize): diff --git a/cinder/volume/drivers/dell_emc/vmax/masking.py b/cinder/volume/drivers/dell_emc/vmax/masking.py index 38c3788ea..52714c72c 100644 --- a/cinder/volume/drivers/dell_emc/vmax/masking.py +++ b/cinder/volume/drivers/dell_emc/vmax/masking.py @@ -1603,13 +1603,15 @@ class VMAXMasking(object): initiatorGroupInstance = conn.GetInstance( foundInitiatorGroupInstanceName, LocalOnly=False) if initiatorGroupInstance['ElementName'] == igGroupName: - host = igGroupName.split("-")[1] + short_host_name = self.utils.get_host_short_name( + connector['host']) LOG.debug("Searching for masking views associated with " "%(igGroupName)s", {'igGroupName': igGroupName}) self._last_volume_delete_initiator_group( conn, controllerConfigService, - foundInitiatorGroupInstanceName, extraSpecs, host) + foundInitiatorGroupInstanceName, extraSpecs, + short_host_name) def _get_port_group_from_masking_view( self, conn, maskingViewName, storageSystemName): @@ -1870,7 +1872,8 @@ class VMAXMasking(object): storageGroupInstanceName = None if extraSpecs[ISV3]: self._cleanup_deletion_v3( - conn, controllerConfigService, volumeInstance, extraSpecs) + conn, controllerConfigService, volumeInstance, extraSpecs, + connector) else: if connector: storageGroupInstanceName = ( @@ -1881,7 +1884,7 @@ class VMAXMasking(object): self._remove_volume_from_sg( conn, controllerConfigService, storageGroupInstanceName, - volumeInstance, extraSpecs) + volumeInstance, extraSpecs, connector) else: LOG.warning(_LW("Cannot get storage from connector.")) @@ -1893,13 +1896,15 @@ class VMAXMasking(object): return storageGroupInstanceName def _cleanup_deletion_v3( - self, conn, controllerConfigService, volumeInstance, extraSpecs): + self, conn, controllerConfigService, volumeInstance, extraSpecs, + connector=None): """Pre cleanup before VMAX3 deletion operation :param conn: the ecom connection :param controllerConfigService: storage system instance name :param volumeInstance: the volume instance :param extraSpecs: the extra specifications + :param connector: the connector object - default None """ storageGroupInstanceNames = ( self.get_associated_masking_groups_from_device( @@ -1908,20 +1913,19 @@ class VMAXMasking(object): if storageGroupInstanceNames: sgNum = len(storageGroupInstanceNames) if len(storageGroupInstanceNames) > 1: - LOG.warning(_LW("Volume %(volumeName)s is belong to " - "%(sgNum)s storage groups."), - {'volumeName': volumeInstance['ElementName'], - 'sgNum': sgNum}) + LOG.debug("Volume %(volumeName)s belongs to %(sgNum)s " + "storage groups.", + {'volumeName': volumeInstance['ElementName'], + 'sgNum': sgNum}) for storageGroupInstanceName in storageGroupInstanceNames: self._remove_volume_from_sg( conn, controllerConfigService, - storageGroupInstanceName, - volumeInstance, - extraSpecs) + storageGroupInstanceName, volumeInstance, extraSpecs, + connector) def _remove_volume_from_sg( self, conn, controllerConfigService, storageGroupInstanceName, - volumeInstance, extraSpecs): + volumeInstance, extraSpecs, connector=None): """Remove volume from storage group :param conn: the ecom connection @@ -1929,6 +1933,7 @@ class VMAXMasking(object): :param storageGroupInstanceName: the SG instance name :param volumeInstance: the volume instance :param extraSpecs: the extra specifications + :param connector: the connector object - default None """ instance = conn.GetInstance(storageGroupInstanceName, LocalOnly=False) storageGroupName = instance['ElementName'] @@ -1992,7 +1997,8 @@ class VMAXMasking(object): conn, controllerConfigService, storageGroupInstanceName, storageGroupName, volumeInstance, - volumeInstance['ElementName'], extraSpecs) + volumeInstance['ElementName'], + extraSpecs, connector) else: # Not the last volume so remove it from storage group self._multiple_vols_in_SG( @@ -2005,7 +2011,8 @@ class VMAXMasking(object): def _last_vol_in_SG( self, conn, controllerConfigService, storageGroupInstanceName, - storageGroupName, volumeInstance, volumeName, extraSpecs): + storageGroupName, volumeInstance, volumeName, extraSpecs, + connector=None): """Steps if the volume is the last in a storage group. 1. Check if the volume is in a masking view. @@ -2024,6 +2031,7 @@ class VMAXMasking(object): :param volumeInstance: the volume instance :param volumeName: the volume name :param extraSpecs: the extra specifications + :param connector: the connector object """ status = False LOG.debug("Only one volume remains in storage group " @@ -2045,14 +2053,11 @@ class VMAXMasking(object): maskingViewInstance = conn.GetInstance( mvInstanceName, LocalOnly=False) maskingViewName = maskingViewInstance['ElementName'] - - def do_delete_mv_ig_and_sg(): - return self._delete_mv_ig_and_sg( - conn, controllerConfigService, mvInstanceName, - maskingViewName, storageGroupInstanceName, - storageGroupName, volumeInstance, volumeName, - extraSpecs, mv_count) - do_delete_mv_ig_and_sg() + self._delete_mv_ig_and_sg( + conn, controllerConfigService, mvInstanceName, + maskingViewName, storageGroupInstanceName, + storageGroupName, volumeInstance, volumeName, + extraSpecs, mv_count, connector) status = True mv_count -= 1 return status @@ -2092,7 +2097,7 @@ class VMAXMasking(object): def _delete_mv_ig_and_sg( self, conn, controllerConfigService, mvInstanceName, maskingViewName, storageGroupInstanceName, storageGroupName, - volumeInstance, volumeName, extraSpecs, mv_count): + volumeInstance, volumeName, extraSpecs, mv_count, connector): """Delete the Masking view, the storage Group and the initiator group. :param conn: connection to the ecom server @@ -2105,10 +2110,11 @@ class VMAXMasking(object): :param volumeName: the volume name :param extraSpecs: extra specs :param mv_count: number of masking views + :param connector: the connector object """ isV3 = extraSpecs[ISV3] fastPolicyName = extraSpecs.get(FASTPOLICY, None) - host = maskingViewName.split("-")[1] + short_host_name = self.utils.get_host_short_name(connector['host']) storageSystemInstanceName = self.utils.find_storage_system( conn, controllerConfigService) @@ -2117,10 +2123,10 @@ class VMAXMasking(object): self._last_volume_delete_masking_view( conn, controllerConfigService, mvInstanceName, maskingViewName, extraSpecs) - self._last_volume_delete_initiator_group( - conn, controllerConfigService, - initiatorGroupInstanceName, extraSpecs, host) - + if initiatorGroupInstanceName: + self._last_volume_delete_initiator_group( + conn, controllerConfigService, + initiatorGroupInstanceName, extraSpecs, short_host_name) if not isV3: isTieringPolicySupported, tierPolicyServiceInstanceName = ( self._get_tiering_info(conn, storageSystemInstanceName, @@ -2731,7 +2737,7 @@ class VMAXMasking(object): def _last_volume_delete_initiator_group( self, conn, controllerConfigService, - initiatorGroupInstanceName, extraSpecs, host=None): + initiatorGroupInstanceName, extraSpecs, host): """Delete the initiator group. Delete the Initiator group if it has been created by the VMAX driver, @@ -2739,48 +2745,51 @@ class VMAXMasking(object): :param conn: the ecom connection :param controllerConfigService: controller config service - :param igInstanceNames: initiator group instance name + :param initiatorGroupInstanceName: initiator group instance name :param extraSpecs: extra specifications :param host: the short name of the host """ - defaultInitiatorGroupName = None initiatorGroupInstance = conn.GetInstance(initiatorGroupInstanceName) initiatorGroupName = initiatorGroupInstance['ElementName'] - protocol = self.utils.get_short_protocol_type(self.protocol) - if host: + + @coordination.synchronized('emc-ig-{initiatorGroupName}') + def _inner_last_volume_delete_initiator_group(initiatorGroupName): + protocol = self.utils.get_short_protocol_type(self.protocol) defaultInitiatorGroupName = (( "OS-%(shortHostName)s-%(protocol)s-IG" % {'shortHostName': host, 'protocol': protocol})) - if initiatorGroupName == defaultInitiatorGroupName: - maskingViewInstanceNames = ( - self.get_masking_views_by_initiator_group( - conn, initiatorGroupInstanceName)) - if len(maskingViewInstanceNames) == 0: - LOG.debug( - "Last volume associated with the initiator group - " - "deleting the associated initiator group " - "%(initiatorGroupName)s.", - {'initiatorGroupName': initiatorGroupName}) - self._delete_initiators_from_initiator_group( - conn, controllerConfigService, initiatorGroupInstanceName, - initiatorGroupName) - self._delete_initiator_group(conn, controllerConfigService, - initiatorGroupInstanceName, - initiatorGroupName, extraSpecs) - else: - LOG.warning(_LW("Initiator group %(initiatorGroupName)s is " - "associated with masking views and can't be " - "deleted. Number of associated masking view " - "is: %(nmv)d."), - {'initiatorGroupName': initiatorGroupName, - 'nmv': len(maskingViewInstanceNames)}) - else: - LOG.warning(_LW("Initiator group %(initiatorGroupName)s was " - "not created by the VMAX driver so will " - "not be deleted by the VMAX driver."), + if initiatorGroupName == defaultInitiatorGroupName: + maskingViewInstanceNames = ( + self.get_masking_views_by_initiator_group( + conn, initiatorGroupInstanceName)) + if len(maskingViewInstanceNames) == 0: + LOG.debug( + "Last volume associated with the initiator group - " + "deleting the associated initiator group " + "%(initiatorGroupName)s.", {'initiatorGroupName': initiatorGroupName}) + self._delete_initiators_from_initiator_group( + conn, controllerConfigService, + initiatorGroupInstanceName, initiatorGroupName) + self._delete_initiator_group( + conn, controllerConfigService, + initiatorGroupInstanceName, + initiatorGroupName, extraSpecs) + else: + LOG.warning(_LW("Initiator group %(initiatorGroupName)s " + "is associated with masking views and " + "can't be deleted. Number of associated " + "masking view is: %(nmv)d."), + {'initiatorGroupName': initiatorGroupName, + 'nmv': len(maskingViewInstanceNames)}) + else: + LOG.warning(_LW("Initiator group %(initiatorGroupName)s was " + "not created by the VMAX driver so will " + "not be deleted by the VMAX driver."), + {'initiatorGroupName': initiatorGroupName}) + _inner_last_volume_delete_initiator_group(initiatorGroupName) def _create_hardware_ids( self, conn, initiatorNames, storageSystemName):