Merge "PowerMax Driver - Allowing for all types of boolean in extra specs"

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}
+        with mock.patch.object(
-        return_val = self.common._is_valid_for_storage_assisted_migration(
+                self.common.provision,
-            device_id, host4, self.data.array,
+                'get_slo_workload_settings_from_storage_group',
-            self.data.srp, volume_name, False, False, self.data.slo,
+                return_value='Diamond+DSS') as mock_settings:
-            self.data.workload, False)
+            host4 = {'host': self.data.fake_host}
-        self.assertEqual(ref_return, return_val)
+            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

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(
+        self.assertTrue(self.utils.is_compression_disabled(extra_specs))
-            extra_specs)
-        self.assertTrue(do_disable_compression)
         # 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(
+        self.assertTrue(self.utils.is_compression_disabled(extra_specs))
-            extra_specs))
+        extra_specs3 = deepcopy(extra_specs)
+        extra_specs3.update({utils.DISABLECOMPRESSION: '<is> 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(
+        self.assertFalse(self.utils.is_compression_disabled(extra_specs))
-            extra_specs)
-        self.assertFalse(do_disable_compression)
         # 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(
+        self.assertFalse(self.utils.is_compression_disabled(extra_specs2))
-            extra_specs)
+        extra_specs3 = deepcopy(extra_specs)
-        self.assertFalse(do_disable_compression2)
+        extra_specs3.update({utils.DISABLECOMPRESSION: '<is> 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(
+        self.assertTrue(self.utils.change_compression_type(
-            source_compr_disabled, new_type_compr_disabled)
+            source_compr_disabled, new_type_compr_disabled_1))
-        self.assertTrue(ans)
+        new_type_compr_disabled_2 = {
+            'extra_specs': {utils.DISABLECOMPRESSION: '<is> 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(
+        self.assertFalse(self.utils.change_compression_type(
-            source_compr_disabled, new_type_compr_disabled)
+            source_compr_disabled, new_type_compr_disabled))
-        self.assertFalse(ans)
+        new_type_compr_disabled_2 = {
+            'extra_specs': {utils.DISABLECOMPRESSION: '<is> 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(

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 self.utils.is_compression_disabled(extra_specs):
-                # If not True remove it.
+                extra_specs.pop(utils.DISABLECOMPRESSION, None)
-                if not strutils.bool_from_string(
-                        extra_specs[utils.DISABLECOMPRESSION]):
-                    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(
+        if self.rest.is_compression_capable(extra_specs[utils.ARRAY]):
-            extra_specs)
+            is_compression_disabled = self.utils.is_compression_disabled(
-        # Check if old type and new type have different compression types
+                extra_specs)
-        do_change_compression = (self.utils.change_compression_type(
+            # Check if old type and new type have different compression types
-            is_compression_disabled, new_type))
+            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(
+                disable_compression = self.utils.is_compression_disabled(
-                    utils.DISABLECOMPRESSION, False)
+                    extra_specs)
                 self.rest.create_storage_group(
                     array, source_sg, extra_specs['srp'], extra_specs['slo'],
                     extra_specs['workload'], extra_specs, disable_compression)

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 = ['<is> True', 'True', 'true', True]
+IS_FALSE = ['<is> 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]