From cc0c69bd59f047646e52199b92c37eb7b7cddeec Mon Sep 17 00:00:00 2001 From: odonos12 Date: Wed, 22 Apr 2020 14:32:15 +0100 Subject: [PATCH] PowerMax Driver - Rep validation fix & Retype suspension fix Moved replication status validation call into create_rep_enabled_volume lower in _create_volume which has additional locking based on array+storage group to prevent race condition with multiple create volume requests. Added additional resume rdfg call during retype as when moving from from rep_enabled to another rep_enabled type the original storage group was being left in a suspended state. Closes-Bug: 1875433 Change-Id: I8a52d67cea80a5a4eb03f25a0913b1ba6cc24b3a (cherry picked from commit 465a1d8ab4a082caa136c963d242d8666ef551ca) --- .../dell_emc/powermax/test_powermax_common.py | 5 +- .../powermax/test_powermax_replication.py | 80 +++++++++++-------- .../drivers/dell_emc/powermax/common.py | 29 +++++-- cinder/volume/drivers/dell_emc/powermax/fc.py | 3 +- .../volume/drivers/dell_emc/powermax/iscsi.py | 3 +- .../volume/drivers/dell_emc/powermax/utils.py | 3 +- 6 files changed, 78 insertions(+), 45 deletions(-) diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py index 7e15b648fa7..614e531b439 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py @@ -283,6 +283,7 @@ class PowerMaxCommonTest(test.TestCase): self.assertEqual(self.data.replication_update, update) self.assertEqual(self.data.rep_info_dict, info) + @mock.patch.object(common.PowerMaxCommon, '_validate_rdfg_status') @mock.patch.object(common.PowerMaxCommon, 'gather_replication_updates', return_value=(tpd.PowerMaxData.replication_update, tpd.PowerMaxData.rep_info_dict)) @@ -294,7 +295,8 @@ class PowerMaxCommonTest(test.TestCase): ('', tpd.PowerMaxData.rep_extra_specs5, tpd.PowerMaxData.rep_info_dict, ''))) def test_create_replication_enabled_volume_not_first_volume( - self, mck_prepare, mck_create, mck_protect, mck_updates): + self, mck_prepare, mck_create, mck_protect, mck_updates, + mck_valid): array = self.data.array volume = self.data.test_volume volume_name = volume.name @@ -315,6 +317,7 @@ class PowerMaxCommonTest(test.TestCase): array, volume_name, storagegroup_name, volume_size, rep_extra_specs, rep_info_dict) mck_protect.assert_not_called() + mck_valid.assert_called_once_with(array, rep_extra_specs) rep_vol.update({'remote_device_id': self.data.device_id2}) mck_updates.assert_called_once_with( rep_extra_specs, rep_extra_specs5, rep_vol) diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_replication.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_replication.py index 023aa293584..a214cb689e3 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_replication.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_replication.py @@ -738,6 +738,7 @@ class PowerMaxReplicationTest(test.TestCase): extra_specs[utils.REP_CONFIG] = self.data.rep_config_async volume_dict, rep_update, rep_info_dict = self.common._create_volume( volume, volume_name, volume_size, extra_specs) + mck_valid.assert_not_called() self.assertEqual(self.data.provider_location, volume_dict) self.assertEqual(self.data.replication_update, rep_update) self.assertIsNone(rep_info_dict) @@ -891,50 +892,63 @@ class PowerMaxReplicationTest(test.TestCase): target_extra_specs) self.assertTrue(success) + @mock.patch.object(common.PowerMaxCommon, 'get_volume_metadata', + return_value='') + @mock.patch.object(common.PowerMaxCommon, 'update_metadata', + return_value=tpd.PowerMaxData.replication_model) + @mock.patch.object(provision.PowerMaxProvision, 'verify_slo_workload', + return_value=(True, True)) + @mock.patch.object(rest.PowerMaxRest, 'srdf_resume_replication') + @mock.patch.object(common.PowerMaxCommon, '_retype_volume', + return_value=(True, 'storage_group')) + @mock.patch.object(common.PowerMaxCommon, 'configure_volume_replication', + return_value=('status', 'data', + tpd.PowerMaxData.rep_info_dict, + tpd.PowerMaxData.rep_extra_specs_mgmt, + True)) + @mock.patch.object(common.PowerMaxCommon, '_sync_check') + @mock.patch.object(common.PowerMaxCommon, 'break_rdf_device_pair_session', + return_value=(tpd.PowerMaxData.rep_extra_specs_mgmt, + True)) @mock.patch.object(common.PowerMaxCommon, '_validate_rdfg_status') - @mock.patch.object( - common.PowerMaxCommon, 'get_volume_metadata', return_value='') - @mock.patch.object( - common.PowerMaxCommon, '_retype_remote_volume', return_value=True) - @mock.patch.object( - common.PowerMaxCommon, '_retype_volume', - return_value=(True, tpd.PowerMaxData.defaultstoragegroup_name)) - def test_migrate_volume_success_rep_to_rep(self, mck_retype, mck_remote, - mck_get, mck_valid): - self.common.rep_config = {'mode': utils.REP_SYNC, - 'array': self.data.array} - array_id = self.data.array + def test_migrate_volume_success_rep_to_rep( + self, mck_valid, mck_break, mck_sync, mck_rep, mck_retype, + mck_resume, mck_slo, mck_upd_meta, mck_get_meta): + array = self.data.array volume = self.data.test_volume device_id = self.data.device_id srp = self.data.srp target_slo = self.data.slo_silver target_workload = self.data.workload volume_name = volume.name - rep_config_sync = deepcopy(self.data.rep_config_sync) - rep_config_sync[utils.RDF_CONS_EXEMPT] = False extra_specs = deepcopy(self.data.rep_extra_specs) - extra_specs[utils.SLO] = self.data.slo_diamond - new_type = {'extra_specs': self.data.rep_extra_specs} - - target_extra_specs = deepcopy(new_type['extra_specs']) - target_extra_specs.update({ - utils.SRP: srp, utils.ARRAY: array_id, utils.SLO: target_slo, - utils.WORKLOAD: target_workload, - utils.INTERVAL: extra_specs[utils.INTERVAL], - utils.RETRIES: extra_specs[utils.RETRIES], - utils.DISABLECOMPRESSION: False, utils.REP_MODE: utils.REP_SYNC, - utils.REP_CONFIG: rep_config_sync}) - + extra_specs[utils.REP_CONFIG] = self.data.rep_config_sync + extra_specs[utils.REPLICATION_DEVICE_BACKEND_ID] = ( + self.data.rep_config_sync[utils.BACKEND_ID]) + target_extra_specs = deepcopy(self.data.rep_extra_specs) + target_extra_specs['rep_extra_specs'] = utils.REP_ASYNC + target_extra_specs['rdf_group_no'] = self.data.rdf_group_name_2 + target_extra_specs[utils.REP_CONFIG] = self.data.rep_config_async + target_extra_specs[utils.REPLICATION_DEVICE_BACKEND_ID] = ( + self.data.rep_config_async[utils.BACKEND_ID]) + new_type = {'extra_specs': target_extra_specs} success, model_update = self.common._migrate_volume( - array_id, volume, device_id, srp, target_slo, target_workload, + array, volume, device_id, srp, target_slo, target_workload, volume_name, new_type, extra_specs) - mck_retype.assert_called_once_with( - array_id, srp, device_id, volume, volume_name, extra_specs, - target_slo, target_workload, target_extra_specs) - mck_remote.assert_called_once_with( - array_id, volume, device_id, volume_name, utils.REP_SYNC, - True, target_extra_specs) + self.assertEqual(2, mck_valid.call_count) + mck_valid.assert_called_with(array, target_extra_specs) + mck_break.assert_called_once_with( + array, device_id, volume_name, extra_specs) + mck_sync.assert_called_once_with(array, device_id, extra_specs) + mck_rep.assert_called_once_with( + array, volume, device_id, target_extra_specs) + mck_retype.assert_called_once() + self.assertEqual(2, mck_resume.call_count) + mck_resume.assert_called_with( + array, self.data.rep_extra_specs_mgmt['mgmt_sg_name'], + extra_specs['rdf_group_no'], self.data.rep_extra_specs_mgmt) self.assertTrue(success) + self.assertEqual(self.data.replication_model, model_update) @mock.patch.object(masking.PowerMaxMasking, 'add_volume_to_storage_group') @mock.patch.object(provision.PowerMaxProvision, 'get_or_create_group') diff --git a/cinder/volume/drivers/dell_emc/powermax/common.py b/cinder/volume/drivers/dell_emc/powermax/common.py index df52cc99443..6a15f7b3318 100644 --- a/cinder/volume/drivers/dell_emc/powermax/common.py +++ b/cinder/volume/drivers/dell_emc/powermax/common.py @@ -2039,9 +2039,6 @@ class PowerMaxCommon(object): if self.utils.is_replication_enabled(extra_specs): is_re, rep_mode = True, extra_specs['rep_mode'] - if is_re: - self._validate_rdfg_status(array, extra_specs) - storagegroup_name = self.masking.get_or_create_default_storage_group( array, extra_specs[utils.SRP], extra_specs[utils.SLO], extra_specs[utils.WORKLOAD], extra_specs, @@ -2122,6 +2119,7 @@ class PowerMaxCommon(object): _is_first_vol_in_replicated_sg()) if not is_first_volume: + self._validate_rdfg_status(array, extra_specs) __, rep_extra_specs, rep_info_dict, __ = ( self.prepare_replication_details(extra_specs)) volume_info_dict = self.provision.create_volume_from_sg( @@ -3679,8 +3677,10 @@ class PowerMaxCommon(object): :returns: bool """ model_update, success = None, False - rep_info_dict, rep_extra_specs, rep_mode, rep_status, resume_rdf = ( - None, None, None, False, False) + rep_info_dict, rep_extra_specs, rep_mode, rep_status = ( + None, None, None, False) + resume_target_sg, resume_original_sg = False, False + resume_original_sg_dict = dict() target_extra_specs = new_type['extra_specs'] target_extra_specs.update({ @@ -3726,19 +3726,26 @@ class PowerMaxCommon(object): # Scenario 1: Rep -> Non-Rep # Scenario 2: Cleanup for Rep -> Diff Rep type if (was_rep_enabled and not is_rep_enabled) or backend_ids_differ: - rep_extra_specs, resume_rdf = ( + rep_extra_specs, resume_original_sg = ( self.break_rdf_device_pair_session( array, device_id, volume_name, extra_specs)) model_update = { 'replication_status': REPLICATION_DISABLED, 'replication_driver_data': None} + if resume_original_sg: + resume_original_sg_dict = { + utils.ARRAY: array, + utils.SG_NAME: rep_extra_specs['mgmt_sg_name'], + utils.RDF_GROUP_NO: rep_extra_specs['rdf_group_no'], + utils.EXTRA_SPECS: rep_extra_specs} + # Scenario 1: Non-Rep -> Rep # Scenario 2: Rep -> Diff Rep type if (not was_rep_enabled and is_rep_enabled) or backend_ids_differ: self._sync_check(array, device_id, extra_specs) (rep_status, rep_driver_data, rep_info_dict, - rep_extra_specs, resume_rdf) = ( + rep_extra_specs, resume_target_sg) = ( self.configure_volume_replication( array, volume, device_id, target_extra_specs)) model_update = { @@ -3771,10 +3778,16 @@ class PowerMaxCommon(object): array, volume, device_id, volume_name, rep_mode, is_rep_enabled, target_extra_specs) - if resume_rdf: + if resume_target_sg: self.rest.srdf_resume_replication( array, rep_extra_specs['mgmt_sg_name'], rep_extra_specs['rdf_group_no'], rep_extra_specs) + if resume_original_sg and resume_original_sg_dict: + self.rest.srdf_resume_replication( + resume_original_sg_dict[utils.ARRAY], + resume_original_sg_dict[utils.SG_NAME], + resume_original_sg_dict[utils.RDF_GROUP_NO], + resume_original_sg_dict[utils.EXTRA_SPECS]) if success: model_update = self.update_metadata( diff --git a/cinder/volume/drivers/dell_emc/powermax/fc.py b/cinder/volume/drivers/dell_emc/powermax/fc.py index ae7b45d0c91..a55d5461e9f 100644 --- a/cinder/volume/drivers/dell_emc/powermax/fc.py +++ b/cinder/volume/drivers/dell_emc/powermax/fc.py @@ -125,9 +125,10 @@ class PowerMaxFCDriver(san.SanDriver, driver.FibreChannelDriver): 4.2.1 - Concurrent live migrations failure (bug #1875478) 4.2.2 - U4P failover lock not released on exception (#1875640) 4.2.3 - Live migrate remove rep vol from sg (bug #1875432) + 4.2.4 - Rep validation & retype suspension fix (bug #1875433) """ - VERSION = "4.2.3" + VERSION = "4.2.4" # ThirdPartySystems wiki CI_WIKI_NAME = "EMC_VMAX_CI" diff --git a/cinder/volume/drivers/dell_emc/powermax/iscsi.py b/cinder/volume/drivers/dell_emc/powermax/iscsi.py index e5d0e3cc955..86abb317a43 100644 --- a/cinder/volume/drivers/dell_emc/powermax/iscsi.py +++ b/cinder/volume/drivers/dell_emc/powermax/iscsi.py @@ -130,9 +130,10 @@ class PowerMaxISCSIDriver(san.SanISCSIDriver): 4.2.1 - Concurrent live migrations failure (bug #1875478) 4.2.2 - U4P failover lock not released on exception (#1875640) 4.2.3 - Live migrate remove rep vol from sg (bug #1875432) + 4.2.4 - Rep validation & retype suspension fix (bug #1875433) """ - VERSION = "4.2.3" + VERSION = "4.2.4" # ThirdPartySystems wiki CI_WIKI_NAME = "EMC_VMAX_CI" diff --git a/cinder/volume/drivers/dell_emc/powermax/utils.py b/cinder/volume/drivers/dell_emc/powermax/utils.py index f7a00c6fb63..618edf75a88 100644 --- a/cinder/volume/drivers/dell_emc/powermax/utils.py +++ b/cinder/volume/drivers/dell_emc/powermax/utils.py @@ -78,7 +78,8 @@ RDF_FAILEDOVER_STATE = 'failed over' RDF_ACTIVE = 'active' RDF_ACTIVEACTIVE = 'activeactive' RDF_ACTIVEBIAS = 'activebias' -RDF_VALID_STATES_SYNC = [RDF_SYNC_STATE, RDF_SYNCINPROG_STATE] +RDF_VALID_STATES_SYNC = [RDF_SYNC_STATE, RDF_SUSPENDED_STATE, + RDF_SYNCINPROG_STATE] RDF_VALID_STATES_ASYNC = [RDF_CONSISTENT_STATE, RDF_SUSPENDED_STATE, RDF_SYNCINPROG_STATE] RDF_VALID_STATES_METRO = [RDF_ACTIVEBIAS, RDF_ACTIVEACTIVE,