From 1084de1f1666c2c5e9a850ef39c124bf74652dad Mon Sep 17 00:00:00 2001
From: Helen Walsh <helen.walsh@emc.com>
Date: Mon, 31 May 2021 16:37:12 +0100
Subject: [PATCH] PowerMax Driver - QoS should not be set on parent storage
 group

The parent storage group is just a hold all for the child
storage groups which contain attributes like service level and QoS.
Setting QoS on the parent storage group can cause problems as
having a higher I/O limit on the child than the parent is not
allowed and this is not the intention.  The fix is to set
QoS on the child storage groups only.

Closes-Bug: #1930290
Change-Id: Ib2c1a44086da1fa3c64611344221c9307be22296
---
 .../dell_emc/powermax/powermax_data.py        |  5 ++++
 .../powermax/test_powermax_masking.py         | 24 +++++++++++++++++++
 .../drivers/dell_emc/powermax/masking.py      | 13 +++++-----
 ...powermax-bug-1930290-4f598329a6ced006.yaml |  7 ++++++
 4 files changed, 42 insertions(+), 7 deletions(-)
 create mode 100644 releasenotes/notes/powermax-bug-1930290-4f598329a6ced006.yaml

diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_data.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_data.py
index 73e5f8a962e..106a8cc39d3 100644
--- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_data.py
+++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_data.py
@@ -475,6 +475,11 @@ class PowerMaxData(object):
     extra_specs_tags = deepcopy(extra_specs)
     extra_specs_tags.update({utils.STORAGE_GROUP_TAGS: sg_tags})
 
+    extra_specs_qos = deepcopy(extra_specs)
+    qos_dict = {
+        'total_iops_sec': '4000',
+        'DistributionType': 'Always'}
+    extra_specs_qos['qos'] = qos_dict
     rep_extra_specs_mgmt = deepcopy(rep_extra_specs)
     rep_extra_specs_mgmt['srp'] = srp
     rep_extra_specs_mgmt['mgmt_sg_name'] = rdf_managed_async_grp
diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_masking.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_masking.py
index 291578ce53c..40ce442760d 100644
--- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_masking.py
+++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_masking.py
@@ -298,6 +298,30 @@ class PowerMaxMaskingTest(test.TestCase):
         self.assertEqual(2, mock_get_sg.call_count)
         self.assertEqual(1, mock_sg.call_count)
 
+    @mock.patch.object(
+        rest.PowerMaxRest, 'update_storagegroup_qos')
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_group',
+        return_value=tpd.PowerMaxData.storagegroup_name_i)
+    def test_get_or_create_storage_group_is_parent_qos(
+            self, mock_sg, mock_update_sg):
+        self.driver.masking._get_or_create_storage_group(
+            self.data.array, self.maskingviewdict,
+            self.data.storagegroup_name_i, self.data.extra_specs_qos, True)
+        mock_update_sg.assert_not_called()
+
+    @mock.patch.object(
+        rest.PowerMaxRest, 'update_storagegroup_qos')
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_group',
+        return_value=tpd.PowerMaxData.storagegroup_name_i)
+    def test_get_or_create_storage_group_is_child_qos(
+            self, mock_sg, mock_update_sg):
+        self.driver.masking._get_or_create_storage_group(
+            self.data.array, self.maskingviewdict,
+            self.data.storagegroup_name_i, self.data.extra_specs_qos, False)
+        mock_update_sg.assert_called_once()
+
     @mock.patch.object(masking.PowerMaxMasking, '_move_vol_from_default_sg',
                        return_value=None)
     @mock.patch.object(masking.PowerMaxMasking, '_get_or_create_storage_group',
diff --git a/cinder/volume/drivers/dell_emc/powermax/masking.py b/cinder/volume/drivers/dell_emc/powermax/masking.py
index 5808974e61a..820321e2b64 100644
--- a/cinder/volume/drivers/dell_emc/powermax/masking.py
+++ b/cinder/volume/drivers/dell_emc/powermax/masking.py
@@ -461,14 +461,13 @@ class PowerMaxMasking(object):
                       'volume_name': masking_view_dict[utils.VOL_NAME]})
             LOG.error(msg)
 
-        # If qos exists, update storage group to reflect qos parameters
-        if 'qos' in extra_specs:
-            self.rest.update_storagegroup_qos(
-                serial_number, storagegroup_name, extra_specs)
-
-        # If storagetype:storagegrouptags exist update storage group
-        # to add tags
         if not parent:
+            # If qos exists, update storage group to reflect qos parameters
+            if 'qos' in extra_specs:
+                self.rest.update_storagegroup_qos(
+                    serial_number, storagegroup_name, extra_specs)
+            # If storagetype:storagegrouptags exist update storage group
+            # to add tags
             self._add_tags_to_storage_group(
                 serial_number, storagegroup, extra_specs)
 
diff --git a/releasenotes/notes/powermax-bug-1930290-4f598329a6ced006.yaml b/releasenotes/notes/powermax-bug-1930290-4f598329a6ced006.yaml
new file mode 100644
index 00000000000..c911d8f6699
--- /dev/null
+++ b/releasenotes/notes/powermax-bug-1930290-4f598329a6ced006.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    PowerMax driver `bug #1930290
+    <https://bugs.launchpad.net/cinder/+bug/1930290>`_: This fixes the
+    QoS conflict issue on a child storage group by not setting QoS on a
+    parent storage group.