From 5490bddf804281f76fc9ae8289c1f44976f4b5a4 Mon Sep 17 00:00:00 2001 From: Xing Yang Date: Fri, 24 Apr 2015 22:42:28 -0400 Subject: [PATCH] Clean up failed clones in VMAX driver If the clone operation fails, clean up is not done properly in the VMAX driver. This patch catches the exception during the clone operation, removes the target volume, and then re-throw the exception. Closes-Bug: #1440154 Change-Id: I548d5da09cb66077f04b1c5f1aa168c5bd5cf02e --- cinder/tests/unit/test_emc_vmax.py | 127 +++++++++++++-- cinder/volume/drivers/emc/emc_vmax_common.py | 159 +++++++++++++++---- cinder/volume/drivers/emc/emc_vmax_fc.py | 3 +- cinder/volume/drivers/emc/emc_vmax_iscsi.py | 3 +- 4 files changed, 248 insertions(+), 44 deletions(-) diff --git a/cinder/tests/unit/test_emc_vmax.py b/cinder/tests/unit/test_emc_vmax.py index e7d0c6d48..e01c9cbd5 100644 --- a/cinder/tests/unit/test_emc_vmax.py +++ b/cinder/tests/unit/test_emc_vmax.py @@ -31,6 +31,7 @@ from cinder.volume.drivers.emc import emc_vmax_fast from cinder.volume.drivers.emc import emc_vmax_fc from cinder.volume.drivers.emc import emc_vmax_iscsi from cinder.volume.drivers.emc import emc_vmax_masking +from cinder.volume.drivers.emc import emc_vmax_provision from cinder.volume.drivers.emc import emc_vmax_provision_v3 from cinder.volume.drivers.emc import emc_vmax_utils from cinder.volume import volume_types @@ -1112,7 +1113,7 @@ class FakeEcomConnection(object): vols = [] vol = EMC_StorageVolume() - vol['name'] = self.data.test_volume['name'] + 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'] @@ -2545,12 +2546,60 @@ class EMCVMAXISCSIDriverNoFastTestCase(test.TestCase): self.driver.create_cloned_volume(self.data.test_volume, EMCVMAXCommonData.test_source_volume) - def test_create_clone_no_fast_failed(self): + # Bug https://bugs.launchpad.net/cinder/+bug/1440154 + @mock.patch.object( + emc_vmax_utils.EMCVMAXUtils, + 'get_volume_meta_head', + return_value=[EMCVMAXCommonData.test_volume]) + @mock.patch.object( + emc_vmax_common.EMCVMAXCommon, + '_get_pool_and_storage_system', + return_value=(None, EMCVMAXCommonData.storage_system)) + @mock.patch.object( + emc_vmax_utils.EMCVMAXUtils, + 'get_meta_members_capacity_in_bit', + return_value=[1234567, 7654321]) + @mock.patch.object( + FakeDB, + 'volume_get', + return_value=EMCVMAXCommonData.test_source_volume) + @mock.patch.object( + volume_types, + 'get_volume_type_extra_specs', + return_value={'volume_backend_name': 'ISCSINoFAST'}) + @mock.patch.object( + emc_vmax_provision.EMCVMAXProvision, + 'create_element_replica') + @mock.patch.object( + emc_vmax_utils.EMCVMAXUtils, + 'find_sync_sv_by_target', + return_value=(None, None)) + def test_create_clone_assert_clean_up_target_volume( + self, mock_sync, mock_create_replica, mock_volume_type, + mock_volume, mock_capacities, mock_pool, mock_meta_volume): self.data.test_volume['volume_name'] = "vmax-1234567" + e = exception.VolumeBackendAPIException('CreateElementReplica Ex') + common = self.driver.common + common._delete_from_pool = mock.Mock(return_value=0L) + conn = self.fake_ecom_connection() + storageConfigService = ( + common.utils.find_storage_configuration_service( + conn, self.data.storage_system)) + mock_create_replica.side_effect = e self.assertRaises(exception.VolumeBackendAPIException, self.driver.create_cloned_volume, self.data.test_volume, EMCVMAXCommonData.test_source_volume) + extraSpecs = common._initial_setup(self.data.test_volume) + fastPolicy = extraSpecs['storagetype:fastpolicy'] + targetInstance = ( + conn.EnumerateInstanceNames("EMC_StorageVolume")[0]) + common._delete_from_pool.assert_called_with(storageConfigService, + targetInstance, + targetInstance['Name'], + targetInstance['DeviceID'], + fastPolicy, + extraSpecs) @mock.patch.object( volume_types, @@ -4695,7 +4744,6 @@ class EMCV3DriverTestCase(test.TestCase): def test_create_cloned_volume_v3_success( self, mock_volume_db, mock_type, moke_pool, mock_size): self.data.test_volume['volume_name'] = "vmax-1234567" - cloneVol = {} cloneVol['name'] = 'vol1' cloneVol['id'] = '10' @@ -4707,18 +4755,6 @@ class EMCV3DriverTestCase(test.TestCase): cloneVol['provider_location'] = None cloneVol['NumberOfBlocks'] = 100 cloneVol['BlockSize'] = 512 - name = {} - name['classname'] = 'Symm_StorageVolume' - keys = {} - keys['CreationClassName'] = cloneVol['CreationClassName'] - keys['SystemName'] = cloneVol['SystemName'] - keys['DeviceID'] = cloneVol['DeviceID'] - keys['NumberOfBlocks'] = cloneVol['NumberOfBlocks'] - keys['BlockSize'] = cloneVol['BlockSize'] - keys['SystemCreationClassName'] = \ - cloneVol['SystemCreationClassName'] - name['keybindings'] = keys - self.driver.create_cloned_volume(cloneVol, self.data.test_volume) @mock.patch.object( @@ -4932,6 +4968,67 @@ class EMCV3DriverTestCase(test.TestCase): numTargetWwns = len(EMCVMAXCommonData.target_wwns) self.assertEqual(numTargetWwns, len(data['data'])) + # Bug https://bugs.launchpad.net/cinder/+bug/1440154 + @mock.patch.object( + emc_vmax_provision_v3.EMCVMAXProvisionV3, + '_get_supported_size_range_for_SLO', + return_value={'MaximumVolumeSize': '30000000000', + 'MinimumVolumeSize': '100000'}) + @mock.patch.object( + emc_vmax_common.EMCVMAXCommon, + '_get_pool_and_storage_system', + return_value=(None, EMCVMAXCommonData.storage_system)) + @mock.patch.object( + volume_types, + 'get_volume_type_extra_specs', + return_value={'volume_backend_name': 'V3_BE'}) + @mock.patch.object( + FakeDB, + 'volume_get', + return_value=EMCVMAXCommonData.test_source_volume) + @mock.patch.object( + emc_vmax_provision_v3.EMCVMAXProvisionV3, + 'create_element_replica') + @mock.patch.object( + emc_vmax_utils.EMCVMAXUtils, + 'find_sync_sv_by_target', + return_value=(None, None)) + def test_create_clone_v3_assert_clean_up_target_volume( + self, mock_sync, mock_create_replica, mock_volume_db, + mock_type, moke_pool, mock_size): + self.data.test_volume['volume_name'] = "vmax-1234567" + e = exception.VolumeBackendAPIException('CreateElementReplica Ex') + common = self.driver.common + volumeDict = {'classname': u'Symm_StorageVolume', + 'keybindings': EMCVMAXCommonData.keybindings} + common._create_v3_volume = ( + mock.Mock(return_value=(0L, volumeDict, self.data.storage_system))) + conn = self.fake_ecom_connection() + storageConfigService = [] + storageConfigService = {} + storageConfigService['SystemName'] = EMCVMAXCommonData.storage_system + storageConfigService['CreationClassName'] = \ + self.data.stconf_service_creationclass + common._delete_from_pool_v3 = mock.Mock(return_value=0L) + mock_create_replica.side_effect = e + self.assertRaises(exception.VolumeBackendAPIException, + self.driver.create_cloned_volume, + self.data.test_volume, + EMCVMAXCommonData.test_source_volume) + extraSpecs = common._initial_setup(self.data.test_volume) + targetInstance = ( + conn.EnumerateInstanceNames("EMC_StorageVolume")[0]) + storageGroupName = common.utils.get_v3_storage_group_name('SRP_1', + 'Bronze', + 'DSS') + deviceID = targetInstance['DeviceID'] + common._delete_from_pool_v3.assert_called_with(storageConfigService, + targetInstance, + targetInstance['Name'], + deviceID, + storageGroupName, + extraSpecs) + def _cleanup(self): bExists = os.path.exists(self.config_file_path) if bExists: diff --git a/cinder/volume/drivers/emc/emc_vmax_common.py b/cinder/volume/drivers/emc/emc_vmax_common.py index dd17b2645..3895cc7e7 100644 --- a/cinder/volume/drivers/emc/emc_vmax_common.py +++ b/cinder/volume/drivers/emc/emc_vmax_common.py @@ -3512,6 +3512,10 @@ class EMCVMAXCommon(object): exceptionMessage = (_( "Error Creating unbound volume.")) LOG.error(exceptionMessage) + # Remove target volume + self._delete_target_volume_v2(storageConfigService, + baseTargetVolumeInstance, + extraSpecs) raise exception.VolumeBackendAPIException( data=exceptionMessage) @@ -3519,26 +3523,43 @@ class EMCVMAXCommon(object): # base target composite volume. baseTargetVolumeInstance = self.utils.find_volume_instance( self.conn, baseVolumeDict, baseVolumeName) - elementCompositionService = ( - self.utils.find_element_composition_service( - self.conn, storageSystemName)) - compositeType = self.utils.get_composite_type( - extraSpecs[COMPOSITETYPE]) - _rc, modifiedVolumeDict = ( - self._modify_and_get_composite_volume_instance( - self.conn, - elementCompositionService, - baseTargetVolumeInstance, - unboundVolumeInstance.path, - targetVolumeName, - compositeType, - extraSpecs)) - if modifiedVolumeDict is None: + try: + elementCompositionService = ( + self.utils.find_element_composition_service( + self.conn, storageSystemName)) + compositeType = self.utils.get_composite_type( + extraSpecs[COMPOSITETYPE]) + _rc, modifiedVolumeDict = ( + self._modify_and_get_composite_volume_instance( + self.conn, + elementCompositionService, + baseTargetVolumeInstance, + unboundVolumeInstance.path, + targetVolumeName, + compositeType, + extraSpecs)) + if modifiedVolumeDict is None: + exceptionMessage = (_( + "Error appending volume %(volumename)s to " + "target base volume.") + % {'volumename': targetVolumeName}) + LOG.error(exceptionMessage) + raise exception.VolumeBackendAPIException( + data=exceptionMessage) + except Exception: exceptionMessage = (_( - "Error appending volume %(volumename)s to " - "target base volume.") - % {'volumename': targetVolumeName}) + "Exception appending meta volume to target volume " + "%(volumename)s.") + % {'volumename': baseVolumeName}) LOG.error(exceptionMessage) + # Remove append volume and target base volume + self._delete_target_volume_v2( + storageConfigService, unboundVolumeInstance, + extraSpecs) + self._delete_target_volume_v2( + storageConfigService, baseTargetVolumeInstance, + extraSpecs) + raise exception.VolumeBackendAPIException( data=exceptionMessage) @@ -3565,11 +3586,43 @@ class EMCVMAXCommon(object): """ sourceName = sourceVolume['name'] cloneName = cloneVolume['name'] - rc, job = self.provision.create_element_replica( - self.conn, repServiceInstanceName, cloneName, sourceName, - sourceInstance, targetInstance, extraSpecs) + + try: + rc, job = self.provision.create_element_replica( + self.conn, repServiceInstanceName, cloneName, sourceName, + sourceInstance, targetInstance, extraSpecs) + except Exception: + exceptionMessage = (_( + "Exception during create element replica. " + "Clone name: %(cloneName)s " + "Source name: %(sourceName)s " + "Extra specs: %(extraSpecs)s ") + % {'cloneName': cloneName, + 'sourceName': sourceName, + 'extraSpecs': extraSpecs}) + LOG.error(exceptionMessage) + + if targetInstance is not None: + # Check if the copy session exists. + storageSystem = targetInstance['SystemName'] + syncInstanceName = self.utils.find_sync_sv_by_target( + self.conn, storageSystem, targetInstance, False) + if syncInstanceName is not None: + # Remove the Clone relationship. + rc, job = self.provision.delete_clone_relationship( + self.conn, repServiceInstanceName, syncInstanceName, + extraSpecs, True) + storageConfigService = ( + self.utils.find_storage_configuration_service( + self.conn, storageSystem)) + self._delete_target_volume_v2( + storageConfigService, targetInstance, extraSpecs) + + raise exception.VolumeBackendAPIException( + data=exceptionMessage) cloneDict = self.provision.get_volume_dict_from_job( self.conn, job['Job']) + fastPolicyName = extraSpecs[FASTPOLICY] if isSnapshot: if fastPolicyName is not None: @@ -3586,7 +3639,8 @@ class EMCVMAXCommon(object): cloneVolume['provider_location'] = six.text_type(cloneDict) syncInstanceName, storageSystemName = ( - self._find_storage_sync_sv_sv(cloneVolume, sourceVolume)) + self._find_storage_sync_sv_sv(cloneVolume, sourceVolume, + extraSpecs)) # Remove the Clone relationship so it can be used as a regular lun. # 8 - Detach operation. @@ -3671,7 +3725,10 @@ class EMCVMAXCommon(object): :returns: dict -- cloneDict """ cloneName = cloneVolume['name'] - syncType = self.utils.get_num(8, '16') # Default syncType 8: clone. + # Default syncType 8: clone. + syncType = self.utils.get_num(8, '16') + # Default operation 8: Detach for clone. + operation = self.utils.get_num(8, '16') # Create target volume extraSpecs = self._initial_setup(cloneVolume) @@ -3695,11 +3752,42 @@ class EMCVMAXCommon(object): if isSnapshot: # SyncType 7: snap, VG3R default snapshot is snapVx. syncType = self.utils.get_num(7, '16') + # Operation 9: Dissolve for snapVx. + operation = self.utils.get_num(9, '16') + + try: + _rc, job = ( + self.provisionv3.create_element_replica( + self.conn, repServiceInstanceName, cloneName, syncType, + sourceInstance, extraSpecs, targetInstance)) + except Exception: + LOG.warning(_LW( + "Clone failed on V3. Cleaning up the target volume. " + "Clone name: %(cloneName)s "), + {'cloneName': cloneName}) + # Check if the copy session exists. + storageSystem = targetInstance['SystemName'] + syncInstanceName = self.utils.find_sync_sv_by_target( + self.conn, storageSystem, targetInstance, False) + if syncInstanceName is not None: + # Break the clone relationship. + rc, job = self.provisionv3.break_replication_relationship( + self.conn, repServiceInstanceName, syncInstanceName, + operation, extraSpecs, True) + storageConfigService = ( + self.utils.find_storage_configuration_service( + self.conn, storageSystem)) + deviceId = targetInstance['DeviceID'] + volumeName = targetInstance['Name'] + storageGroupName = self.utils.get_v3_storage_group_name( + extraSpecs[POOL], extraSpecs[SLO], + extraSpecs[WORKLOAD]) + rc = self._delete_from_pool_v3( + storageConfigService, targetInstance, volumeName, + deviceId, storageGroupName, extraSpecs) + # Re-throw the exception. + raise - _rc, job = ( - self.provisionv3.create_element_replica( - self.conn, repServiceInstanceName, cloneName, syncType, - sourceInstance, extraSpecs, targetInstance)) cloneDict = self.provisionv3.get_volume_dict_from_job( self.conn, job['Job']) @@ -3786,3 +3874,20 @@ class EMCVMAXCommon(object): volumeRef['status'] = 'error_deleting' modelUpdate['status'] = 'error_deleting' return modelUpdate, volumes + + def _delete_target_volume_v2( + self, storageConfigService, targetVolumeInstance, extraSpecs): + """Helper function to delete the clone target volume instance. + + :param storageConfigService: storage configuration service instance + :param targetVolumeInstance: clone target volume instance + :param extraSpecs: extra specifications + """ + deviceId = targetVolumeInstance['DeviceID'] + volumeName = targetVolumeInstance['Name'] + rc = self._delete_from_pool(storageConfigService, + targetVolumeInstance, + volumeName, deviceId, + extraSpecs[FASTPOLICY], + extraSpecs) + return rc diff --git a/cinder/volume/drivers/emc/emc_vmax_fc.py b/cinder/volume/drivers/emc/emc_vmax_fc.py index c56e58752..eb681d8af 100644 --- a/cinder/volume/drivers/emc/emc_vmax_fc.py +++ b/cinder/volume/drivers/emc/emc_vmax_fc.py @@ -35,9 +35,10 @@ class EMCVMAXFCDriver(driver.FibreChannelDriver): 2.0.0 - Add driver requirement functions 2.1.0 - Add consistency group functions 2.1.1 - Fixed issue with mismatched config (bug #1442376) + 2.1.3 - Clean up failed clones (bug #1440154) """ - VERSION = "2.1.1" + VERSION = "2.1.3" def __init__(self, *args, **kwargs): diff --git a/cinder/volume/drivers/emc/emc_vmax_iscsi.py b/cinder/volume/drivers/emc/emc_vmax_iscsi.py index f2d49a7c6..567cb1f15 100644 --- a/cinder/volume/drivers/emc/emc_vmax_iscsi.py +++ b/cinder/volume/drivers/emc/emc_vmax_iscsi.py @@ -43,9 +43,10 @@ class EMCVMAXISCSIDriver(driver.ISCSIDriver): 2.0.0 - Add driver requirement functions 2.1.0 - Add consistency group functions 2.1.1 - Fixed issue with mismatched config (bug #1442376) + 2.1.3 - Clean up failed clones (bug #1440154) """ - VERSION = "2.1.1" + VERSION = "2.1.3" def __init__(self, *args, **kwargs): -- 2.45.2