From b2f696361ae2737665bfea826240eac8bb5f4afd Mon Sep 17 00:00:00 2001 From: Helen Walsh Date: Thu, 17 Sep 2020 11:23:13 +0100 Subject: [PATCH] PowerMax Driver - Allowing for all types of boolean in extra specs Allowing for True, 'True', 'true', False, 'False', 'false' in 'storagetype:disablecompression' to that retype won't default to host assisted migration. Also checking that the array is compression capable in a retype operation. Change-Id: I79724532202a19d02bf624dde6b3ba0c53080f98 --- .../dell_emc/powermax/test_powermax_common.py | 22 ++++++--- .../dell_emc/powermax/test_powermax_utils.py | 47 ++++++++++++------- .../drivers/dell_emc/powermax/common.py | 29 ++++++------ .../volume/drivers/dell_emc/powermax/utils.py | 7 ++- 4 files changed, 64 insertions(+), 41 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 5cf1ed30cfa..b709fe0bdb4 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 @@ -2139,7 +2139,10 @@ class PowerMaxCommonTest(test.TestCase): device_id, volume, host, volume_name, new_type, extra_specs) self.assertFalse(migrate_status) - def test_slo_workload_migration_same_host_change_compression(self): + @mock.patch.object(rest.PowerMaxRest, 'is_compression_capable', + return_value=True) + def test_slo_workload_migration_same_host_change_compression( + self, mock_cap): device_id = self.data.device_id volume_name = self.data.test_volume.name extra_specs = self.data.extra_specs @@ -2391,12 +2394,17 @@ class PowerMaxCommonTest(test.TestCase): self.data.workload, False) self.assertEqual(ref_return, return_val) # Already in correct sg - host4 = {'host': self.data.fake_host} - return_val = self.common._is_valid_for_storage_assisted_migration( - device_id, host4, self.data.array, - self.data.srp, volume_name, False, False, self.data.slo, - self.data.workload, False) - self.assertEqual(ref_return, return_val) + with mock.patch.object( + self.common.provision, + 'get_slo_workload_settings_from_storage_group', + return_value='Diamond+DSS') as mock_settings: + host4 = {'host': self.data.fake_host} + return_val = self.common._is_valid_for_storage_assisted_migration( + device_id, host4, self.data.array, + self.data.srp, volume_name, False, False, self.data.slo, + self.data.workload, False) + self.assertEqual(ref_return, return_val) + mock_settings.assert_called_once() def test_is_valid_for_storage_assisted_migration_next_gen(self): device_id = self.data.device_id diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_utils.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_utils.py index 3a7ffb0e970..b2d3cc6f155 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_utils.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_utils.py @@ -219,42 +219,53 @@ class PowerMaxUtilsTest(test.TestCase): def test_is_compression_disabled_true(self): # Compression disabled in extra specs extra_specs = self.data.extra_specs_disable_compression - do_disable_compression = self.utils.is_compression_disabled( - extra_specs) - self.assertTrue(do_disable_compression) + self.assertTrue(self.utils.is_compression_disabled(extra_specs)) # Compression disabled by no SL/WL combination extra_specs = deepcopy(self.data.vol_type_extra_specs_none_pool) - self.assertTrue(self.utils.is_compression_disabled( - extra_specs)) + self.assertTrue(self.utils.is_compression_disabled(extra_specs)) + extra_specs3 = deepcopy(extra_specs) + extra_specs3.update({utils.DISABLECOMPRESSION: ' True'}) + self.assertTrue(self.utils.is_compression_disabled(extra_specs3)) + extra_specs4 = deepcopy(extra_specs) + extra_specs4.update({utils.DISABLECOMPRESSION: 'True'}) + self.assertTrue(self.utils.is_compression_disabled(extra_specs4)) def test_is_compression_disabled_false(self): # Path 1: no compression extra spec set extra_specs = self.data.extra_specs - do_disable_compression = self.utils.is_compression_disabled( - extra_specs) - self.assertFalse(do_disable_compression) + self.assertFalse(self.utils.is_compression_disabled(extra_specs)) # Path 2: compression extra spec set to false extra_specs2 = deepcopy(extra_specs) extra_specs2.update({utils.DISABLECOMPRESSION: 'false'}) - do_disable_compression2 = self.utils.is_compression_disabled( - extra_specs) - self.assertFalse(do_disable_compression2) + self.assertFalse(self.utils.is_compression_disabled(extra_specs2)) + extra_specs3 = deepcopy(extra_specs) + extra_specs3.update({utils.DISABLECOMPRESSION: ' False'}) + self.assertFalse(self.utils.is_compression_disabled(extra_specs3)) + extra_specs4 = deepcopy(extra_specs) + extra_specs4.update({utils.DISABLECOMPRESSION: 'False'}) + self.assertFalse(self.utils.is_compression_disabled(extra_specs4)) def test_change_compression_type_true(self): source_compr_disabled = True - new_type_compr_disabled = { + new_type_compr_disabled_1 = { 'extra_specs': {utils.DISABLECOMPRESSION: 'false'}} - ans = self.utils.change_compression_type( - source_compr_disabled, new_type_compr_disabled) - self.assertTrue(ans) + self.assertTrue(self.utils.change_compression_type( + source_compr_disabled, new_type_compr_disabled_1)) + new_type_compr_disabled_2 = { + 'extra_specs': {utils.DISABLECOMPRESSION: ' False'}} + self.assertTrue(self.utils.change_compression_type( + source_compr_disabled, new_type_compr_disabled_2)) def test_change_compression_type_false(self): source_compr_disabled = True new_type_compr_disabled = { 'extra_specs': {utils.DISABLECOMPRESSION: 'true'}} - ans = self.utils.change_compression_type( - source_compr_disabled, new_type_compr_disabled) - self.assertFalse(ans) + self.assertFalse(self.utils.change_compression_type( + source_compr_disabled, new_type_compr_disabled)) + new_type_compr_disabled_2 = { + 'extra_specs': {utils.DISABLECOMPRESSION: ' True'}} + self.assertFalse(self.utils.change_compression_type( + source_compr_disabled, new_type_compr_disabled_2)) def test_is_replication_enabled(self): is_re = self.utils.is_replication_enabled( diff --git a/cinder/volume/drivers/dell_emc/powermax/common.py b/cinder/volume/drivers/dell_emc/powermax/common.py index 1c5bb1e0116..1f5fc53dc38 100644 --- a/cinder/volume/drivers/dell_emc/powermax/common.py +++ b/cinder/volume/drivers/dell_emc/powermax/common.py @@ -23,7 +23,6 @@ import time from oslo_config import cfg from oslo_config import types from oslo_log import log as logging -from oslo_utils import strutils import six from cinder import coordination @@ -2384,11 +2383,8 @@ class PowerMaxCommon(object): extra_specs[utils.SLO] = slo_from_extra_spec extra_specs[utils.WORKLOAD] = workload_from_extra_spec if self.rest.is_compression_capable(extra_specs[utils.ARRAY]): - if extra_specs.get(utils.DISABLECOMPRESSION): - # If not True remove it. - if not strutils.bool_from_string( - extra_specs[utils.DISABLECOMPRESSION]): - extra_specs.pop(utils.DISABLECOMPRESSION, None) + if not self.utils.is_compression_disabled(extra_specs): + extra_specs.pop(utils.DISABLECOMPRESSION, None) else: extra_specs.pop(utils.DISABLECOMPRESSION, None) @@ -3871,14 +3867,16 @@ class PowerMaxCommon(object): :param extra_specs: extra specifications :returns: boolean -- True if migration succeeded, False if error. """ + do_change_compression = False # Check if old type and new type have different replication types do_change_replication = self.utils.change_replication( extra_specs, new_type[utils.EXTRA_SPECS]) - is_compression_disabled = self.utils.is_compression_disabled( - extra_specs) - # Check if old type and new type have different compression types - do_change_compression = (self.utils.change_compression_type( - is_compression_disabled, new_type)) + if self.rest.is_compression_capable(extra_specs[utils.ARRAY]): + is_compression_disabled = self.utils.is_compression_disabled( + extra_specs) + # Check if old type and new type have different compression types + do_change_compression = (self.utils.change_compression_type( + is_compression_disabled, new_type)) is_tgt_rep = self.utils.is_replication_enabled( new_type[utils.EXTRA_SPECS]) is_valid, target_slo, target_workload = ( @@ -4102,7 +4100,7 @@ class PowerMaxCommon(object): self.volume_metadata.capture_retype_info( volume, device_id, array, srp, target_slo, target_workload, target_sg_name, is_rep_enabled, rep_mode, - target_extra_specs[utils.DISABLECOMPRESSION], + self.utils.is_compression_disabled(target_extra_specs), target_backend_id) return success, model_update @@ -4229,7 +4227,8 @@ class PowerMaxCommon(object): target_extra_specs[utils.PORTGROUPNAME] = ( extra_specs.get(utils.PORTGROUPNAME, None)) - disable_compression = target_extra_specs[utils.DISABLECOMPRESSION] + disable_compression = self.utils.is_compression_disabled( + target_extra_specs) source_sg_list = device_info['storageGroupId'] if mgmt_sg_name in source_sg_list: source_sg_list.remove(mgmt_sg_name) @@ -4341,8 +4340,8 @@ class PowerMaxCommon(object): 'storage groups.') storage_groups = self.rest.get_storage_group_list(array) if source_sg not in storage_groups: - disable_compression = extra_specs.get( - utils.DISABLECOMPRESSION, False) + disable_compression = self.utils.is_compression_disabled( + extra_specs) self.rest.create_storage_group( array, source_sg, extra_specs['srp'], extra_specs['slo'], extra_specs['workload'], extra_specs, disable_compression) diff --git a/cinder/volume/drivers/dell_emc/powermax/utils.py b/cinder/volume/drivers/dell_emc/powermax/utils.py index e635e41b1e3..1db3b3163fc 100644 --- a/cinder/volume/drivers/dell_emc/powermax/utils.py +++ b/cinder/volume/drivers/dell_emc/powermax/utils.py @@ -218,6 +218,10 @@ PORT_ID = 'portId' # Revert snapshot exception REVERT_SS_EXC = 'Link must be fully copied for this operation to proceed' +# extra specs +IS_TRUE = [' True', 'True', 'true', True] +IS_FALSE = [' False', 'False', 'false', False] + class PowerMaxUtils(object): """Utility class for Rest based PowerMax volume drivers. @@ -510,7 +514,7 @@ class PowerMaxUtils(object): compression_disabled = False if extra_specs.get(DISABLECOMPRESSION, False): - if strutils.bool_from_string(extra_specs.get(DISABLECOMPRESSION)): + if extra_specs.get(DISABLECOMPRESSION) in IS_TRUE: compression_disabled = True else: if extra_specs.get(SLO): @@ -2078,6 +2082,7 @@ class PowerMaxUtils(object): :returns: array_id, srp, service_level, workload -- str, str, str, str """ array_id, srp, service_level, workload = str(), str(), str(), str() + pool_details = pool_name.split('+') if len(pool_details) == 4: array_id = pool_details[3]