From 1ae4ac4486bf3915e2b619e334dfb479a91bfa54 Mon Sep 17 00:00:00 2001 From: Helen Walsh Date: Mon, 19 Jul 2021 17:35:53 +0100 Subject: [PATCH] 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 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 --- .../dell_emc/powermax/test_powermax_common.py | 4 +- .../powermax/test_powermax_replication.py | 10 ++- .../dell_emc/powermax/test_powermax_rest.py | 66 +++++++++++++++++++ .../drivers/dell_emc/powermax/common.py | 7 +- .../volume/drivers/dell_emc/powermax/rest.py | 55 ++++++++++++++++ .../volume/drivers/dell_emc/powermax/utils.py | 3 + .../notes/bug-1936848-6ecc78e0e970419a.yaml | 8 +++ 7 files changed, 149 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/bug-1936848-6ecc78e0e970419a.yaml 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 d032902bfea..161f9b31b97 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 @@ -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] diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_replication.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_replication.py index 66c52ea6d5b..a5eae2b7cc8 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_replication.py +++ b/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): diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py index 45084ae4e38..a3fac79c978 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/test_powermax_rest.py +++ b/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 = '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 = '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) diff --git a/cinder/volume/drivers/dell_emc/powermax/common.py b/cinder/volume/drivers/dell_emc/powermax/common.py index 1935ef35b83..2022621ed0a 100644 --- a/cinder/volume/drivers/dell_emc/powermax/common.py +++ b/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}) diff --git a/cinder/volume/drivers/dell_emc/powermax/rest.py b/cinder/volume/drivers/dell_emc/powermax/rest.py index f18cf6b126e..2ee58b8dda8 100644 --- a/cinder/volume/drivers/dell_emc/powermax/rest.py +++ b/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 + :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) diff --git a/cinder/volume/drivers/dell_emc/powermax/utils.py b/cinder/volume/drivers/dell_emc/powermax/utils.py index 6956329addc..fc2b51294fc 100644 --- a/cinder/volume/drivers/dell_emc/powermax/utils.py +++ b/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 = [' True', 'True', 'true', True] IS_FALSE = [' False', 'False', 'false', False] +# filter +LIKE_FILTER = '' + class PowerMaxUtils(object): """Utility class for Rest based PowerMax volume drivers. diff --git a/releasenotes/notes/bug-1936848-6ecc78e0e970419a.yaml b/releasenotes/notes/bug-1936848-6ecc78e0e970419a.yaml new file mode 100644 index 00000000000..5cf26d331e3 --- /dev/null +++ b/releasenotes/notes/bug-1936848-6ecc78e0e970419a.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + PowerMax driver `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.