From d94cc2888c5097306c43de3b2b424b408d52161a Mon Sep 17 00:00:00 2001 From: Xing Yang Date: Mon, 15 Dec 2014 11:47:44 -0500 Subject: [PATCH] Fixed a problem in terminate_connection in VMAX driver The VMAX driver unmaps the volume from the wrong VM in terminate_connection during live migration. This patche fixed this problem. Closes-Bug: #1395845 Change-Id: Ida7136165b959a9e0d7d50c240b447bf237caa6c --- cinder/tests/test_emc_vmax.py | 73 ++++++++++- cinder/volume/drivers/emc/emc_vmax_common.py | 123 +++++++++++++----- cinder/volume/drivers/emc/emc_vmax_masking.py | 58 +++++---- 3 files changed, 190 insertions(+), 64 deletions(-) diff --git a/cinder/tests/test_emc_vmax.py b/cinder/tests/test_emc_vmax.py index abd7aa7af..1c70bce8d 100644 --- a/cinder/tests/test_emc_vmax.py +++ b/cinder/tests/test_emc_vmax.py @@ -57,6 +57,10 @@ class SE_StorageHardwareID(dict): pass +class SYMM_LunMasking(dict): + pass + + class Fake_CIMProperty(): def fake_getCIMProperty(self): @@ -84,6 +88,11 @@ class Fake_CIMProperty(): cimproperty.value = False return cimproperty + def fake_getElementNameCIMProperty(self): + cimproperty = Fake_CIMProperty() + cimproperty.value = 'OS-myhost-MV' + return cimproperty + class Fake_CIM_TierPolicyServiceCapabilities(): @@ -502,9 +511,14 @@ class FakeEcomConnection(): dependent['ElementName'] = self.data.test_volume['name'] dependent['SystemName'] = self.data.storage_system - antecedent = {} + antecedent = SYMM_LunMasking() antecedent['CreationClassName'] = self.data.lunmask_creationclass2 antecedent['SystemName'] = self.data.storage_system + classcimproperty = Fake_CIMProperty() + elementName = ( + classcimproperty.fake_getElementNameCIMProperty()) + properties = {u'ElementName': elementName} + antecedent.properties = properties unitname['Dependent'] = dependent unitname['Antecedent'] = antecedent @@ -1263,6 +1277,58 @@ class EMCVMAXISCSIDriverNoFastTestCase(test.TestCase): self.driver.delete_volume, self.data.failed_delete_vol) + @mock.patch.object( + volume_types, + 'get_volume_type_extra_specs', + return_value={'volume_backend_name': 'ISCSINoFAST'}) + @mock.patch.object( + EMCVMAXMasking, + '_wrap_get_storage_group_from_volume', + return_value=None) + @mock.patch.object( + EMCVMAXCommon, + '_wrap_find_device_number', + return_value={'storagesystem': EMCVMAXCommonData.storage_system}) + @mock.patch.object( + EMCVMAXUtils, + 'find_storage_masking_group', + return_value='value') + def test_map_new_masking_view_no_fast_success(self, _mock_volume_type, + mock_wrap_group, + mock_wrap_device, + mock_storage_group): + self.driver.initialize_connection(self.data.test_volume, + self.data.connector) + + @mock.patch.object( + volume_types, + 'get_volume_type_extra_specs', + return_value={'volume_backend_name': 'ISCSINoFAST'}) + @mock.patch.object( + EMCVMAXMasking, + '_wrap_get_storage_group_from_volume', + return_value=None) + @mock.patch.object( + EMCVMAXCommon, + '_wrap_find_device_number', + return_value={'hostlunid': 1, + 'storagesystem': EMCVMAXCommonData.storage_system}) + @mock.patch.object( + EMCVMAXUtils, + 'find_storage_masking_group', + return_value='value') + @mock.patch.object( + EMCVMAXCommon, + '_is_same_host', + return_value=False) + def test_map_live_migration_no_fast_success(self, _mock_volume_type, + mock_wrap_group, + mock_wrap_device, + mock_storage_group, + mock_same_host): + self.driver.initialize_connection(self.data.test_volume, + self.data.connector) + @mock.patch.object( volume_types, 'get_volume_type_extra_specs', @@ -1276,8 +1342,9 @@ class EMCVMAXISCSIDriverNoFastTestCase(test.TestCase): '_wrap_find_device_number', return_value={'hostlunid': 1, 'storagesystem': EMCVMAXCommonData.storage_system}) - def test_map_no_fast_success(self, _mock_volume_type, mock_wrap_group, - mock_wrap_device): + def test_already_mapped_no_fast_success(self, _mock_volume_type, + mock_wrap_group, + mock_wrap_device): self.driver.initialize_connection(self.data.test_volume, self.data.connector) diff --git a/cinder/volume/drivers/emc/emc_vmax_common.py b/cinder/volume/drivers/emc/emc_vmax_common.py index 101894ee5..d2a42aabd 100644 --- a/cinder/volume/drivers/emc/emc_vmax_common.py +++ b/cinder/volume/drivers/emc/emc_vmax_common.py @@ -279,42 +279,89 @@ class EMCVMAXCommon(object): deviceInfoDict = self._wrap_find_device_number(volume, connector) if ('hostlunid' in deviceInfoDict and deviceInfoDict['hostlunid'] is not None): - # Device is already mapped so we will leave the state as is - deviceNumber = deviceInfoDict['hostlunid'] - LOG.info(_LI("Volume %(volume)s is already mapped. " - "The device number is %(deviceNumber)s ") - % {'volume': volumeName, - 'deviceNumber': deviceNumber}) + isSameHost = self._is_same_host(connector, deviceInfoDict) + if isSameHost: + # Device is already mapped so we will leave the state as is. + deviceNumber = deviceInfoDict['hostlunid'] + LOG.info(_LI("Volume %(volume)s is already mapped. " + "The device number is %(deviceNumber)s.") + % {'volume': volumeName, + 'deviceNumber': deviceNumber}) + else: + deviceInfoDict = self._attach_volume(volume, connector, + extraSpecs, True) else: - maskingViewDict = self._populate_masking_dict( - volume, connector, extraSpecs) - rollbackDict = self.masking.get_or_create_masking_view_and_map_lun( - self.conn, maskingViewDict) - - # Find host lun id again after the volume is exported to the host - deviceInfoDict = self.find_device_number(volume, connector) - if 'hostlunid' not in deviceInfoDict: - # Did not successfully attach to host, - # so a rollback for FAST is required - LOG.error(_LE("Error Attaching volume %(vol)s ") - % {'vol': volumeName}) - if rollbackDict['fastPolicyName'] is not None: - ( - self.masking - ._check_if_rollback_action_for_masking_required( - self.conn, - rollbackDict['controllerConfigService'], - rollbackDict['volumeInstance'], - rollbackDict['volumeName'], - rollbackDict['fastPolicyName'], - rollbackDict['defaultStorageGroupInstanceName'])) - exception_message = ("Error Attaching volume %(vol)s" - % {'vol': volumeName}) - raise exception.VolumeBackendAPIException( - data=exception_message) + deviceInfoDict = self._attach_volume(volume, connector, extraSpecs) + + return deviceInfoDict + + def _attach_volume(self, volume, connector, extraSpecs, + isLiveMigration=None): + """Attach a volume to a host. + + If live migration is being undertaken then the volume + remains attached to the source host. + + :params volume: the volume object + :params connector: the connector object + :params extraSpecs: the volume extra specs + :param isLiveMigration: boolean, can be None + :returns: deviceInfoDict + """ + volumeName = volume['name'] + maskingViewDict = self._populate_masking_dict( + volume, connector, extraSpecs) + if isLiveMigration: + maskingViewDict['isLiveMigration'] = True + else: + maskingViewDict['isLiveMigration'] = False + + rollbackDict = self.masking.get_or_create_masking_view_and_map_lun( + self.conn, maskingViewDict) + + # Find host lun id again after the volume is exported to the host. + deviceInfoDict = self.find_device_number(volume, connector) + if 'hostlunid' not in deviceInfoDict: + # Did not successfully attach to host, + # so a rollback for FAST is required. + LOG.error(_LE("Error Attaching volume %(vol)s.") + % {'vol': volumeName}) + if rollbackDict['fastPolicyName'] is not None: + ( + self.masking + ._check_if_rollback_action_for_masking_required( + self.conn, + rollbackDict['controllerConfigService'], + rollbackDict['volumeInstance'], + rollbackDict['volumeName'], + rollbackDict['fastPolicyName'], + rollbackDict['defaultStorageGroupInstanceName'])) + exception_message = ("Error Attaching volume %(vol)s." + % {'vol': volumeName}) + raise exception.VolumeBackendAPIException( + data=exception_message) return deviceInfoDict + def _is_same_host(self, connector, deviceInfoDict): + """Check if the host is the same. + + Check if the host to attach to is the same host + that is already attached. This is necessary for + live migration. + + :params connector: the connector object + :params deviceInfoDict: the device information + :returns: boolean True/False + """ + if 'host' in connector: + currentHost = connector['host'] + if ('maskingview' in deviceInfoDict and + deviceInfoDict['maskingview'] is not None): + if currentHost in deviceInfoDict['maskingview']: + return True + return False + def _wrap_find_device_number(self, volume, connector): """Aid for unit testing @@ -1299,6 +1346,15 @@ class EMCVMAXCommon(object): numDeviceNumber = int(unitinstance['DeviceNumber'], 16) foundNumDeviceNumber = numDeviceNumber + controllerInstance = self.conn.GetInstance(controller, + LocalOnly=False) + propertiesList = controllerInstance.properties.items() + for properties in propertiesList: + if properties[0] == 'ElementName': + cimProperties = properties[1] + foundMaskingViewName = cimProperties.value + break + break if foundNumDeviceNumber is None: @@ -1309,7 +1365,8 @@ class EMCVMAXCommon(object): 'volumeInstance': volumeInstance.path}) data = {'hostlunid': foundNumDeviceNumber, - 'storagesystem': storageSystemName} + 'storagesystem': storageSystemName, + 'maskingview': foundMaskingViewName} LOG.debug("Device info: %(data)s." % {'data': data}) diff --git a/cinder/volume/drivers/emc/emc_vmax_masking.py b/cinder/volume/drivers/emc/emc_vmax_masking.py index fefb7408d..96518296d 100644 --- a/cinder/volume/drivers/emc/emc_vmax_masking.py +++ b/cinder/volume/drivers/emc/emc_vmax_masking.py @@ -67,39 +67,41 @@ class EMCVMAXMasking(object): maskingViewName = maskingViewDict['maskingViewName'] volumeName = maskingViewDict['volumeName'] pgGroupName = maskingViewDict['pgGroupName'] + isLiveMigration = maskingViewDict['isLiveMigration'] fastPolicyName = maskingViewDict['fastPolicy'] defaultStorageGroupInstanceName = None - # we need a rollback scenario for FAST. - # We must make sure that volume is returned to default storage - # group if anything goes wrong - if fastPolicyName is not None: - defaultStorageGroupInstanceName = ( - self.fast.get_and_verify_default_storage_group( - conn, controllerConfigService, volumeInstance.path, - volumeName, fastPolicyName)) - if defaultStorageGroupInstanceName is None: - exceptionMessage = (_( - "Cannot get the default storage group for FAST policy: " - "%(fastPolicyName)s. ") - % {'fastPolicyName': fastPolicyName}) - LOG.error(exceptionMessage) - raise exception.VolumeBackendAPIException( - data=exceptionMessage) + if isLiveMigration is False: + # We need a rollback scenario for FAST. + # We must make sure that volume is returned to default storage + # group if anything goes wrong. + if fastPolicyName is not None: + defaultStorageGroupInstanceName = ( + self.fast.get_and_verify_default_storage_group( + conn, controllerConfigService, volumeInstance.path, + volumeName, fastPolicyName)) + if defaultStorageGroupInstanceName is None: + exceptionMessage = (_( + "Cannot get the default storage group for FAST policy:" + " %(fastPolicyName)s.") + % {'fastPolicyName': fastPolicyName}) + LOG.error(exceptionMessage) + raise exception.VolumeBackendAPIException( + data=exceptionMessage) - retStorageGroupInstanceName = ( - self.remove_device_from_default_storage_group( - conn, controllerConfigService, volumeInstance.path, - volumeName, fastPolicyName)) - if retStorageGroupInstanceName is None: - exceptionMessage = (_( - "Failed to remove volume %(volumeName)s from default SG: " - "%(volumeName)s. ") - % {'volumeName': volumeName}) - LOG.error(exceptionMessage) - raise exception.VolumeBackendAPIException( - data=exceptionMessage) + retStorageGroupInstanceName = ( + self.remove_device_from_default_storage_group( + conn, controllerConfigService, volumeInstance.path, + volumeName, fastPolicyName)) + if retStorageGroupInstanceName is None: + exceptionMessage = (_( + "Failed to remove volume %(volumeName)s from default " + "SG: %(volumeName)s.") + % {'volumeName': volumeName}) + LOG.error(exceptionMessage) + raise exception.VolumeBackendAPIException( + data=exceptionMessage) try: maskingViewInstanceName = self._find_masking_view( -- 2.45.2