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 542d1106565..c91f502d329 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 @@ -2188,7 +2188,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 @@ -2440,12 +2443,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 135e14857f7..e79d9d94c0e 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 @@ -220,42 +220,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 6adc1d2eca9..a240d2f0987 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 @@ -2363,11 +2362,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) @@ -3870,14 +3866,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 = ( @@ -4110,7 +4108,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 @@ -4243,7 +4241,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) @@ -4361,8 +4360,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 cf64ef61332..fdaff5d910f 100644 --- a/cinder/volume/drivers/dell_emc/powermax/utils.py +++ b/cinder/volume/drivers/dell_emc/powermax/utils.py @@ -213,6 +213,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. @@ -505,7 +509,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): @@ -2057,6 +2061,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]