PowerMax Driver - Fix for renaming GVG

When a Generic Volume Group is renamed in OpenStack, the
corresponding storage group on the PowerMax does not reflect
this new name. The name format is <user-supplied-name><uuid>
on the PowerMax and now the former section has been changed.

To fix this issue we search based on the uuid alone and not
the combination of user supplied name and uuid. Once
the modified storage group object is returned successfully,
the storage group is renamed on the PowerMax.  Any subsequent
operation on the Generic Volume Group will proceed as before.

Closes-Bug: #1936848
Change-Id: Ia49ac163e6d9995368ef7931e51835f753b6623e

cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_common.py

@@ -2866,6 +2866,8 @@ class PowerMaxCommonTest(test.TestCase):
         mck_update.assert_called_once_with(group, add_vols, remove_vols)
         self.assertEqual(ref_model_update, model_update)
 
+    @mock.patch.object(common.PowerMaxCommon, '_find_volume_group',
+                       return_value=tpd.PowerMaxData.test_rep_group)
     @mock.patch.object(rest.PowerMaxRest, 'is_volume_in_storagegroup',
                        return_value=True)
     @mock.patch.object(
@@ -2882,7 +2884,7 @@ class PowerMaxCommonTest(test.TestCase):
         masking.PowerMaxMasking, 'remove_volumes_from_storage_group')
     def test_update_group_promotion(
             self, mck_rem, mock_cg_type, mock_type_check, mck_setup, mck_rep,
-            mck_in_sg):
+            mck_in_sg, mck_group):
         group = self.data.test_rep_group
         add_vols = []
         remove_vols = [self.data.test_volume_group_member]

cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_replication.py

@@ -695,7 +695,10 @@ class PowerMaxReplicationTest(test.TestCase):
         mck_validate.assert_called_once_with(
             self.common.rep_configs, extra_specs_list)
 
-    def test_enable_replication(self):
+    @mock.patch.object(common.PowerMaxCommon, '_find_volume_group',
+                       side_effect=[tpd.PowerMaxData.test_group,
+                                    None])
+    def test_enable_replication(self, mock_vg):
         # Case 1: Group not replicated
         with mock.patch.object(volume_utils, 'is_group_a_type',
                                return_value=False):
@@ -720,7 +723,10 @@ class PowerMaxReplicationTest(test.TestCase):
             self.assertEqual(fields.ReplicationStatus.ERROR,
                              model_update['replication_status'])
 
-    def test_disable_replication(self):
+    @mock.patch.object(common.PowerMaxCommon, '_find_volume_group',
+                       side_effect=[tpd.PowerMaxData.test_group,
+                                    None])
+    def test_disable_replication(self, mock_vg):
         # Case 1: Group not replicated
         with mock.patch.object(volume_utils, 'is_group_a_type',
                                return_value=False):

cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py

@@ -2469,3 +2469,69 @@ class PowerMaxRestTest(test.TestCase):
                 self.data.array, self.data.device_id,
                 self.data.test_snapshot_snap_name)
             self.assertEqual('0', snap_id)
+
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_group_list')
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_group_rep',
+        side_effect=[{'rdf': False}, None])
+    def test_get_or_rename_storage_group_rep(
+            self, mock_sg_rep, mock_sg_list):
+        # Success - no need for rename
+        rep_info = self.rest.get_or_rename_storage_group_rep(
+            self.data.array, self.data.storagegroup_name_f,
+            self.data.extra_specs)
+        mock_sg_list.assert_not_called()
+        self.assertIsNotNone(rep_info)
+
+        # Fail - cannot find sg but no filter set
+        rep_info = self.rest.get_or_rename_storage_group_rep(
+            self.data.array, self.data.storagegroup_name_f,
+            self.data.extra_specs)
+        mock_sg_list.assert_not_called()
+        self.assertIsNone(rep_info)
+
+    @mock.patch.object(
+        rest.PowerMaxRest, '_rename_storage_group')
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_group_list',
+        return_value=({'storageGroupId': ['user-name+uuid']}))
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_group_rep',
+        side_effect=[None, ({'rdf': False}), ({'rdf': False})])
+    def test_get_or_rename_storage_group_rep_exists(
+            self, mock_sg_rep, mock_sg_list, mock_rename):
+        sg_filter = '<like>uuid'
+        rep_info = self.rest.get_or_rename_storage_group_rep(
+            self.data.array, self.data.storagegroup_name_f,
+            self.data.extra_specs, sg_filter=sg_filter)
+        mock_sg_list.assert_called_once_with(
+            self.data.array,
+            params={'storageGroupId': sg_filter})
+        group_list_return = {'storageGroupId': ['user-name+uuid']}
+        mock_rename.assert_called_once_with(
+            self.data.array,
+            group_list_return['storageGroupId'][0],
+            self.data.storagegroup_name_f,
+            self.data.extra_specs)
+        self.assertIsNotNone(rep_info)
+
+    @mock.patch.object(
+        rest.PowerMaxRest, '_rename_storage_group')
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_group_list',
+        return_value=({'storageGroupId': ['user-name+uuid']}))
+    @mock.patch.object(
+        rest.PowerMaxRest, 'get_storage_group_rep',
+        side_effect=[None, None])
+    def test_get_or_rename_storage_group_rep_does_not_exist(
+            self, mock_sg_rep, mock_sg_list, mock_rename):
+        sg_filter = '<like>uuid'
+        rep_info = self.rest.get_or_rename_storage_group_rep(
+            self.data.array, self.data.storagegroup_name_f,
+            self.data.extra_specs, sg_filter=sg_filter)
+        mock_sg_list.assert_called_once_with(
+            self.data.array,
+            params={'storageGroupId': sg_filter})
+        mock_rename.assert_not_called()
+        self.assertIsNone(rep_info)

cinder/volume/drivers/dell_emc/powermax/common.py

@@ -535,6 +535,7 @@ class PowerMaxCommon(object):
         if group_id is not None:
             if group and (volume_utils.is_group_a_cg_snapshot_type(group)
                           or group.is_replicated):
+                self._find_volume_group(extra_specs[utils.ARRAY], group)
                 extra_specs[utils.FORCE_VOL_EDIT] = True
                 group_name = self._add_new_volume_to_volume_group(
                     volume, device_id, volume_name,
@@ -6071,8 +6072,12 @@ class PowerMaxCommon(object):
         :param group: the group object
         :returns: volume group dictionary
         """
+        __, interval_retries_dict = self._get_volume_group_info(group)
         group_name = self.utils.update_volume_group_name(group)
-        volume_group = self.rest.get_storage_group_rep(array, group_name)
+        sg_name_filter = utils.LIKE_FILTER + group.id
+        volume_group = self.rest.get_or_rename_storage_group_rep(
+            array, group_name, interval_retries_dict,
+            sg_filter=sg_name_filter)
         if not volume_group:
             LOG.warning("Volume group %(group_id)s cannot be found",
                         {'group_id': group_name})

cinder/volume/drivers/dell_emc/powermax/rest.py

@@ -3072,6 +3072,44 @@ class PowerMaxRest(object):
                 'src_device': device_id, 'tgt_device': r2_device_id,
                 'session_info': session_info}
 
+    def get_or_rename_storage_group_rep(
+            self, array, storage_group_name, extra_specs, sg_filter=None):
+        """Get storage group rep info if it exist.
+
+        If a generic volume group has been renamed we also need
+        to rename it on the array based on the uuid component.
+        We check for uuid if we cannot find it based on its old name.
+
+        :param array: the array serial number
+        :param storage_group_name: the name of the storage group
+        :param extra_specs: extra specification
+        :param sg_filter: uuid substring <like>
+        :returns: storage group dict or None
+        """
+        rep_details = self.get_storage_group_rep(array, storage_group_name)
+        if not rep_details:
+            # It is possible that the group has been renamed
+            if sg_filter:
+                sg_dict = self.get_storage_group_list(
+                    array, params={
+                        'storageGroupId': sg_filter})
+                sg_list = sg_dict.get('storageGroupId') if sg_dict else None
+                if sg_list and len(sg_list) == 1:
+                    rep_details = self.get_storage_group_rep(
+                        array, sg_list[0])
+                    # Update the new storage group name
+                    if rep_details:
+                        self._rename_storage_group(
+                            array, sg_list[0], storage_group_name, extra_specs)
+                        rep_details = self.get_storage_group_rep(
+                            array, storage_group_name)
+                        LOG.warning(
+                            "Volume group %(old)s has been renamed to %(new)s "
+                            "due to a rename operation in OpenStack.",
+                            {'old': sg_list[0], 'new': storage_group_name})
+
+        return rep_details
+
     def get_storage_group_rep(self, array, storage_group_name):
         """Given a name, return storage group details wrt replication.
 
@@ -3429,3 +3467,20 @@ class PowerMaxRest(object):
         """
         return (self.ucode_major_level >= utils.UCODE_5978 and
                 self.ucode_minor_level >= utils.UCODE_5978_HICKORY)
+
+    def _rename_storage_group(
+            self, array, old_name, new_name, extra_specs):
+        """Rename the storage group.
+
+        :param array: the array serial number
+        :param old_name: the original name
+        :param new_name: the new name
+        :param extra_specs: the extra specifications
+        """
+        payload = {"editStorageGroupActionParam": {
+            "renameStorageGroupParam": {
+                "new_storage_Group_name": new_name}}}
+        status_code, job = self.modify_storage_group(
+            array, old_name, payload)
+        self.wait_for_job(
+            'Rename storage group', status_code, job, extra_specs)

cinder/volume/drivers/dell_emc/powermax/utils.py

@@ -216,6 +216,9 @@ REVERT_SS_EXC = 'Link must be fully copied for this operation to proceed'
 IS_TRUE = ['<is> True', 'True', 'true', True]
 IS_FALSE = ['<is> False', 'False', 'false', False]
 
+# <like> filter
+LIKE_FILTER = '<like>'
+
 
 class PowerMaxUtils(object):
     """Utility class for Rest based PowerMax volume drivers.

releasenotes/notes/bug-1936848-6ecc78e0e970419a.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    PowerMax driver `bug #1936848
+    <https://bugs.launchpad.net/cinder/+bug/1936848>`_: Fixed
+    Generic Volume Group error where the name has been changed
+    in OpenStack and is not reflected on the corresponding storage
+    group on the PowerMax.