From 3d10a8663bc9265b996fb01e1ae91d5feee7665a Mon Sep 17 00:00:00 2001 From: Helen Walsh Date: Thu, 20 Apr 2017 12:10:07 +0100 Subject: [PATCH] VMAX driver - Detaching volumes if part of two or more MVs When Live migration is used extensively there can be scenarios where a regular attached volume can belong to two or more Masking Views. Because of this, we did not remove the volume from the storage group, which is not typical behaviour. In this fix we use a temporary file to determine if a terminate_connection is a regular detach or a part of the live migration process. Change-Id: Ide38fa21d65859a5516c577a9983124d998a2e95 Closes-Bug: #1684595 (cherry picked from commit 9d2466bb292cac59c283dd98325284bf8a2921a7) --- .../unit/volume/drivers/emc/test_emc_vmax.py | 118 +++++++++++++----- cinder/volume/drivers/emc/emc_vmax_common.py | 28 ++++- cinder/volume/drivers/emc/emc_vmax_fc.py | 3 +- cinder/volume/drivers/emc/emc_vmax_iscsi.py | 3 +- cinder/volume/drivers/emc/emc_vmax_masking.py | 17 ++- cinder/volume/drivers/emc/emc_vmax_utils.py | 70 ++++++++++- 6 files changed, 194 insertions(+), 45 deletions(-) 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 b08fa2c9f..bae144dce 100644 --- a/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py +++ b/cinder/tests/unit/volume/drivers/emc/test_emc_vmax.py @@ -15,6 +15,7 @@ import os import shutil +import sys import tempfile import unittest from xml.dom import minidom @@ -3235,6 +3236,9 @@ class EMCVMAXISCSIDriverNoFastTestCase(test.TestCase): self.driver.delete_volume, self.data.failed_delete_vol) + @mock.patch.object( + emc_vmax_utils.EMCVMAXUtils, + 'insert_live_migration_record') @mock.patch.object( emc_vmax_common.EMCVMAXCommon, '_is_same_host', @@ -3255,41 +3259,20 @@ class EMCVMAXISCSIDriverNoFastTestCase(test.TestCase): return_value={'volume_backend_name': 'ISCSINoFAST'}) def test_already_mapped_no_fast_success( self, _mock_volume_type, mock_wrap_group, mock_wrap_device, - mock_is_same_host): + mock_is_same_host, mock_rec): self.driver.common._get_correct_port_group = mock.Mock( return_value=self.data.port_group) self.driver.initialize_connection(self.data.test_volume, self.data.connector) - @mock.patch.object( - emc_vmax_masking.EMCVMAXMasking, - '_check_adding_volume_to_storage_group', - return_value=None) - @mock.patch.object( - emc_vmax_utils.EMCVMAXUtils, - 'find_storage_masking_group', - return_value=EMCVMAXCommonData.default_sg_instance_name) - @mock.patch.object( - emc_vmax_masking.EMCVMAXMasking, - '_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_new_masking_view_no_fast_success( - self, _mock_volume_type, mock_wrap_group, - mock_storage_group, mock_add_volume): - self.driver.common._wrap_find_device_number = mock.Mock( - return_value=({}, False, {})) - self.driver.initialize_connection(self.data.test_volume, - self.data.connector) - @mock.patch.object( emc_vmax_masking.EMCVMAXMasking, '_get_port_group_from_masking_view', return_value={'CreationClassName': 'CIM_TargetMaskingGroup', 'ElementName': 'OS-portgroup-PG'}) + @mock.patch.object( + emc_vmax_utils.EMCVMAXUtils, + 'insert_live_migration_record') @mock.patch.object( emc_vmax_common.EMCVMAXCommon, '_get_port_group_from_source', @@ -3342,11 +3325,15 @@ class EMCVMAXISCSIDriverNoFastTestCase(test.TestCase): mock_same_host, mock_check, mock_sg_from_mv, - mocx_sg_from_src, - mock_pg_from_mv): + mock_pg_from_mv, + mock_file, + mock_pg_from_mv2): self.driver.initialize_connection(self.data.test_volume, self.data.connector) + @mock.patch.object( + emc_vmax_utils.EMCVMAXUtils, + 'insert_live_migration_record') @mock.patch.object( emc_vmax_masking.EMCVMAXMasking, '_get_initiator_group_from_masking_view', @@ -3369,7 +3356,7 @@ class EMCVMAXISCSIDriverNoFastTestCase(test.TestCase): return_value={'volume_backend_name': 'ISCSINoFAST'}) def test_map_existing_masking_view_no_fast_success( self, _mock_volume_type, mock_wrap_group, mock_storage_group, - mock_initiator_group, mock_ig_from_mv): + mock_initiator_group, mock_ig_from_mv, mock_rec): self.driver.initialize_connection(self.data.test_volume, self.data.connector) @@ -4251,6 +4238,9 @@ class EMCVMAXISCSIDriverFastTestCase(test.TestCase): self.driver.delete_volume, self.data.failed_delete_vol) + @mock.patch.object( + emc_vmax_utils.EMCVMAXUtils, + 'insert_live_migration_record') @mock.patch.object( emc_vmax_common.EMCVMAXCommon, '_is_same_host', @@ -4271,7 +4261,7 @@ class EMCVMAXISCSIDriverFastTestCase(test.TestCase): return_value={'volume_backend_name': 'ISCSIFAST'}) def test_already_mapped_fast_success( self, _mock_volume_type, mock_wrap_group, mock_wrap_device, - mock_is_same_host): + mock_is_same_host, mock_rec): self.driver.common._get_correct_port_group = mock.Mock( return_value=self.data.port_group) self.driver.initialize_connection(self.data.test_volume, @@ -4857,6 +4847,9 @@ class EMCVMAXFCDriverNoFastTestCase(test.TestCase): self.driver.delete_volume, self.data.failed_delete_vol) + @mock.patch.object( + emc_vmax_utils.EMCVMAXUtils, + 'insert_live_migration_record') @mock.patch.object( emc_vmax_common.EMCVMAXCommon, '_is_same_host', @@ -4871,7 +4864,8 @@ class EMCVMAXFCDriverNoFastTestCase(test.TestCase): return_value={'volume_backend_name': 'FCNoFAST', 'FASTPOLICY': 'FC_GOLD1'}) def test_map_lookup_service_no_fast_success( - self, _mock_volume_type, mock_maskingview, mock_is_same_host): + self, _mock_volume_type, mock_maskingview, mock_is_same_host, + mock_rec): self.data.test_volume['volume_name'] = "vmax-1234567" common = self.driver.common common.get_target_wwns_from_masking_view = mock.Mock( @@ -5458,6 +5452,9 @@ class EMCVMAXFCDriverFastTestCase(test.TestCase): self.driver.delete_volume, self.data.failed_delete_vol) + @mock.patch.object( + emc_vmax_utils.EMCVMAXUtils, + 'insert_live_migration_record') @mock.patch.object( emc_vmax_common.EMCVMAXCommon, '_is_same_host', @@ -5472,7 +5469,7 @@ class EMCVMAXFCDriverFastTestCase(test.TestCase): return_value={'volume_backend_name': 'FCFAST', 'FASTPOLICY': 'FC_GOLD1'}) def test_map_fast_success(self, _mock_volume_type, mock_maskingview, - mock_is_same_host): + mock_is_same_host, mock_rec): common = self.driver.common common.get_target_wwns_list = mock.Mock( return_value=EMCVMAXCommonData.target_wwns) @@ -6533,6 +6530,9 @@ class EMCV3DriverTestCase(test.TestCase): self.data.test_ctxt, self.data.test_CG, add_volumes, remove_volumes) + @mock.patch.object( + emc_vmax_utils.EMCVMAXUtils, + 'insert_live_migration_record') @mock.patch.object( emc_vmax_common.EMCVMAXCommon, '_is_same_host', @@ -6546,7 +6546,8 @@ class EMCV3DriverTestCase(test.TestCase): 'get_volume_type_extra_specs', return_value={'volume_backend_name': 'V3_BE'}) def test_map_v3_success( - self, _mock_volume_type, mock_maskingview, mock_is_same_host): + self, _mock_volume_type, mock_maskingview, mock_is_same_host, + mock_rec): common = self.driver.common common.get_target_wwns_list = mock.Mock( return_value=EMCVMAXCommonData.target_wwns) @@ -8546,6 +8547,59 @@ class EMCVMAXUtilsTest(test.TestCase): conn, initiatorgroup) self.assertIsNone(foundIg) + @mock.patch('builtins.open' if sys.version_info >= (3,) + else '__builtin__.open') + def test_insert_live_migration_record(self, mock_open): + volume = {'id': '12345678-87654321'} + tempdir = tempfile.mkdtemp() + emc_vmax_utils.LIVE_MIGRATION_FILE = ( + tempdir + '/livemigrationarray') + lm_file_name = ("%(prefix)s-%(volid)s" + % {'prefix': emc_vmax_utils.LIVE_MIGRATION_FILE, + 'volid': volume['id'][:8]}) + self.driver.utils.insert_live_migration_record(volume) + mock_open.assert_called_once_with(lm_file_name, "w") + self.driver.utils.delete_live_migration_record(volume) + shutil.rmtree(tempdir) + + def test_delete_live_migration_record(self): + volume = {'id': '12345678-87654321'} + tempdir = tempfile.mkdtemp() + utils.LIVE_MIGRATION_FILE = ( + tempdir + '/livemigrationarray') + lm_file_name = ("%(prefix)s-%(volid)s" + % {'prefix': utils.LIVE_MIGRATION_FILE, + 'volid': volume['id'][:8]}) + m = mock.mock_open() + with mock.patch('{}.open'.format(__name__), m, create=True): + with open(lm_file_name, "w") as f: + f.write('live migration details') + self.driver.utils.insert_live_migration_record(volume) + self.driver.utils.delete_live_migration_record(volume) + m.assert_called_once_with(lm_file_name, "w") + shutil.rmtree(tempdir) + + def test_get_live_migration_record(self): + volume = {'id': '12345678-87654321'} + tempdir = tempfile.mkdtemp() + emc_vmax_utils.LIVE_MIGRATION_FILE = ( + tempdir + '/livemigrationarray') + lm_file_name = ("%(prefix)s-%(volid)s" + % {'prefix': emc_vmax_utils.LIVE_MIGRATION_FILE, + 'volid': volume['id'][:8]}) + self.driver.utils.insert_live_migration_record(volume) + record = self.driver.utils.get_live_migration_record(volume) + self.assertEqual(volume['id'], record[0]) + os.remove(lm_file_name) + shutil.rmtree(tempdir) + + def test_get_live_migration_file_name(self): + volume = {'id': '12345678-87654321'} + lm_live_migration = self.driver.utils.get_live_migration_file_name( + volume) + self.assertIn('/livemigrationarray-12345678', lm_live_migration) + self.assertIn('/tmp/', lm_live_migration) + class EMCVMAXCommonTest(test.TestCase): def setUp(self): diff --git a/cinder/volume/drivers/emc/emc_vmax_common.py b/cinder/volume/drivers/emc/emc_vmax_common.py index 8ead1eed9..5fff6a8b3 100644 --- a/cinder/volume/drivers/emc/emc_vmax_common.py +++ b/cinder/volume/drivers/emc/emc_vmax_common.py @@ -348,7 +348,12 @@ class EMCVMAXCommon(object): vol_instance = self._find_lun(volume) storage_system = vol_instance['SystemName'] - if self._is_volume_multiple_masking_views(vol_instance): + livemigrationrecord = self.utils.get_live_migration_record(volume) + if livemigrationrecord: + self.utils.delete_live_migration_record(volume) + + if livemigrationrecord and self._is_volume_multiple_masking_views( + vol_instance): return configservice = self.utils.find_controller_configuration_service( @@ -430,12 +435,14 @@ class EMCVMAXCommon(object): "The device number is %(deviceNumber)s."), {'volume': volumeName, 'deviceNumber': deviceNumber}) + self.utils.insert_live_migration_record(volume) # Special case, we still need to get the iscsi ip address. portGroupName = ( self._get_correct_port_group( deviceInfoDict, maskingViewDict['storageSystemName'])) else: if isLiveMigration: + self.utils.insert_live_migration_record(volume) maskingViewDict['storageGroupInstanceName'] = ( self._get_storage_group_from_source(sourceInfoDict)) maskingViewDict['portGroupInstanceName'] = ( @@ -493,6 +500,9 @@ class EMCVMAXCommon(object): (rollbackDict['isV3'] is not None)): (self.masking._check_if_rollback_action_for_masking_required( self.conn, rollbackDict)) + livemigrationrecord = self.utils.get_live_migration_record(volume) + if livemigrationrecord: + self.utils.delete_live_migration_record(volume) exception_message = (_("Error Attaching volume %(vol)s.") % {'vol': volumeName}) raise exception.VolumeBackendAPIException( @@ -1636,14 +1646,16 @@ class EMCVMAXCommon(object): """ maskedvols = [] data = {} + isLiveMigration = False + source_data = {} foundController = None foundNumDeviceNumber = None foundMaskingViewName = None volumeName = volume['name'] volumeInstance = self._find_lun(volume) storageSystemName = volumeInstance['SystemName'] - isLiveMigration = False - source_data = {} + if not volumeInstance: + return data, isLiveMigration, source_data unitnames = self.conn.ReferenceNames( volumeInstance.path, @@ -1656,7 +1668,15 @@ class EMCVMAXCommon(object): if index > -1: unitinstance = self.conn.GetInstance(unitname, LocalOnly=False) - numDeviceNumber = int(unitinstance['DeviceNumber'], 16) + if unitinstance['DeviceNumber']: + numDeviceNumber = int(unitinstance['DeviceNumber'], 16) + else: + LOG.debug( + "Device number not found for volume " + "%(volumeName)s %(volumeInstance)s.", + {'volumeName': volumeName, + 'volumeInstance': volumeInstance.path}) + break foundNumDeviceNumber = numDeviceNumber foundController = controller controllerInstance = self.conn.GetInstance(controller, diff --git a/cinder/volume/drivers/emc/emc_vmax_fc.py b/cinder/volume/drivers/emc/emc_vmax_fc.py index 0b3e7cfd8..c972e7d2c 100644 --- a/cinder/volume/drivers/emc/emc_vmax_fc.py +++ b/cinder/volume/drivers/emc/emc_vmax_fc.py @@ -72,9 +72,10 @@ class EMCVMAXFCDriver(driver.FibreChannelDriver): - QoS support (blueprint vmax-qos) 2.4.1 - Pre-zoned port group fix (bug #456285) 2.4.2 - Rollback error on Live Migration (bug #1686174) + 2.4.3 - Detaching volumes if part of two or more MVs (bug #1684595) """ - VERSION = "2.4.2" + VERSION = "2.4.3" # ThirdPartySystems wiki CI_WIKI_NAME = "EMC_VMAX_CI" diff --git a/cinder/volume/drivers/emc/emc_vmax_iscsi.py b/cinder/volume/drivers/emc/emc_vmax_iscsi.py index 33b7f3f7b..cd8400d09 100644 --- a/cinder/volume/drivers/emc/emc_vmax_iscsi.py +++ b/cinder/volume/drivers/emc/emc_vmax_iscsi.py @@ -78,9 +78,10 @@ class EMCVMAXISCSIDriver(driver.ISCSIDriver): https://blueprints.launchpad.net/cinder/+spec/vmax-iscsi-multipath 2.4.1 - Pre-zoned port group fix (bug #456285) 2.4.2 - Rollback error on Live Migration (bug #1686174) + 2.4.3 - Detaching volumes if part of two or more MVs (bug #1684595) """ - VERSION = "2.4.2" + VERSION = "2.4.3" # ThirdPartySystems wiki CI_WIKI_NAME = "EMC_VMAX_CI" diff --git a/cinder/volume/drivers/emc/emc_vmax_masking.py b/cinder/volume/drivers/emc/emc_vmax_masking.py index 8f9c116a9..e36f032b8 100644 --- a/cinder/volume/drivers/emc/emc_vmax_masking.py +++ b/cinder/volume/drivers/emc/emc_vmax_masking.py @@ -2110,10 +2110,16 @@ class EMCVMAXMasking(object): self._last_volume_delete_masking_view( conn, controllerConfigService, mvInstanceName, maskingViewName, extraSpecs) - self._last_volume_delete_initiator_group( - conn, controllerConfigService, - initiatorGroupInstanceName, extraSpecs, host) + initiatorGroupInstance = conn.GetInstance(initiatorGroupInstanceName) + if initiatorGroupInstance: + initiatorGroupName = initiatorGroupInstance['ElementName'] + @coordination.synchronized('emc-ig-{initiatorGroupName}') + def inner_do_delete_initiator_group(initiatorGroupName): + self._last_volume_delete_initiator_group( + conn, controllerConfigService, + initiatorGroupInstanceName, extraSpecs, host) + inner_do_delete_initiator_group(initiatorGroupName) if not isV3: isTieringPolicySupported, tierPolicyServiceInstanceName = ( self._get_tiering_info(conn, storageSystemInstanceName, @@ -2671,8 +2677,8 @@ class EMCVMAXMasking(object): LOG.debug("Deletion of initiator path %(hardwareIdPath)s " "is successful.", {'hardwareIdPath': hardwareIdPath}) else: - LOG.warning(_LW("Deletion of initiator path %(hardwareIdPath)s " - "is failed."), {'hardwareIdPath': hardwareIdPath}) + LOG.debug("Deletion of initiator path %(hardwareIdPath)s " + "failed.", {'hardwareIdPath': hardwareIdPath}) def _delete_initiators_from_initiator_group(self, conn, controllerConfigService, @@ -2737,7 +2743,6 @@ class EMCVMAXMasking(object): "OS-%(shortHostName)s-%(protocol)s-IG" % {'shortHostName': host, 'protocol': protocol})) - if initiatorGroupName == defaultInitiatorGroupName: maskingViewInstanceNames = ( self.get_masking_views_by_initiator_group( diff --git a/cinder/volume/drivers/emc/emc_vmax_utils.py b/cinder/volume/drivers/emc/emc_vmax_utils.py index 4fe00af53..a1a8617e8 100644 --- a/cinder/volume/drivers/emc/emc_vmax_utils.py +++ b/cinder/volume/drivers/emc/emc_vmax_utils.py @@ -15,11 +15,14 @@ import datetime import hashlib +import os import random import re +import tempfile from xml.dom import minidom from oslo_log import log as logging +from oslo_serialization import jsonutils from oslo_service import loopingcall from oslo_utils import units import six @@ -50,7 +53,7 @@ EMC_ROOT = 'root/emc' CONCATENATED = 'concatenated' CINDER_EMC_CONFIG_FILE_PREFIX = '/etc/cinder/cinder_emc_config_' CINDER_EMC_CONFIG_FILE_POSTFIX = '.xml' -LIVE_MIGRATION_FILE = '/etc/cinder/livemigrationarray' +LIVE_MIGRATION_FILE = tempfile.gettempdir() + '/livemigrationarray' ISCSI = 'iscsi' FC = 'fc' JOB_RETRIES = 60 @@ -2720,3 +2723,68 @@ class EMCVMAXUtils(object): "%(igName)s.", {'igName': foundinitiatorGroupInstanceName}) return foundinitiatorGroupInstanceName + + def insert_live_migration_record(self, volume): + """Insert a record of live migration destination into a temporary file + + :param volume: the volume dictionary + """ + lm_file_name = self.get_live_migration_file_name(volume) + live_migration_details = self.get_live_migration_record(volume) + if live_migration_details: + return + else: + live_migration_details = {volume['id']: [volume['id']]} + try: + with open(lm_file_name, "w") as f: + jsonutils.dump(live_migration_details, f) + except Exception: + exceptionMessage = (_( + "Error in processing live migration file.")) + LOG.exception(exceptionMessage) + raise exception.VolumeBackendAPIException( + data=exceptionMessage) + + def delete_live_migration_record(self, volume): + """Delete record of live migration + + Delete record of live migration destination from file and if + after deletion of record, delete file if empty. + + :param volume: the volume dictionary + """ + lm_file_name = self.get_live_migration_file_name(volume) + live_migration_details = self.get_live_migration_record(volume) + if live_migration_details: + if volume['id'] in live_migration_details: + os.remove(lm_file_name) + + def get_live_migration_record(self, volume): + """get record of live migration destination from a temporary file + + :param volume: the volume dictionary + :returns: returns a single record + """ + returned_record = None + lm_file_name = self.get_live_migration_file_name(volume) + if os.path.isfile(lm_file_name): + with open(lm_file_name, "rb") as f: + live_migration_details = jsonutils.load(f) + if volume['id'] in live_migration_details: + returned_record = live_migration_details[volume['id']] + else: + LOG.debug("%(Volume)s doesn't exist in live migration " + "record.", + {'Volume': volume['id']}) + return returned_record + + def get_live_migration_file_name(self, volume): + """get name of temporary live migration file + + :param volume: the volume dictionary + :returns: returns file name + """ + lm_file_name = ("%(prefix)s-%(volid)s" + % {'prefix': LIVE_MIGRATION_FILE, + 'volid': volume['id'][:8]}) + return lm_file_name