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 af050534aeb..6d15083e9a9 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 @@ -2141,6 +2141,25 @@ class PowerMaxCommonTest(test.TestCase): self.common.update_group, self.data.test_group_1, [], []) + @mock.patch.object(volume_utils, 'is_group_a_type', + return_value=False) + @mock.patch.object(volume_utils, 'is_group_a_cg_snapshot_type', + return_value=True) + def test_update_group_remove_volumes(self, mock_cg_type, mock_type_check): + group = self.data.test_group_1 + add_vols = [] + remove_vols = [self.data.test_volume_group_member] + ref_model_update = {'status': fields.GroupStatus.AVAILABLE} + with mock.patch.object( + rest.PowerMaxRest, 'is_volume_in_storagegroup', + return_value=False) as mock_exists: + model_update, __, __ = self.common.update_group(group, + add_vols, + remove_vols) + mock_exists.assert_called_once() + + self.assertEqual(ref_model_update, model_update) + @mock.patch.object(volume_utils, 'is_group_a_type', return_value=False) def test_delete_group(self, mock_check): group = self.data.test_group_1 @@ -2193,6 +2212,28 @@ class PowerMaxCommonTest(test.TestCase): group, volumes) self.assertEqual(ref_model_update, model_update) + @mock.patch.object(volume_utils, 'is_group_a_type', return_value=False) + @mock.patch.object(volume_utils, 'is_group_a_cg_snapshot_type', + return_value=True) + @mock.patch.object(rest.PowerMaxRest, 'get_volumes_in_storage_group', + return_value=[ + tpd.PowerMaxData.test_volume_group_member]) + @mock.patch.object(common.PowerMaxCommon, '_get_members_of_volume_group', + return_value=[tpd.PowerMaxData.device_id]) + @mock.patch.object(common.PowerMaxCommon, '_find_device_on_array', + return_value= tpd.PowerMaxData.device_id) + @mock.patch.object(masking.PowerMaxMasking, + 'remove_volumes_from_storage_group') + def test_delete_group_clone_check( + self, mock_rem, mock_find, mock_mems, mock_vols, mock_chk1, + mock_chk2): + group = self.data.test_group_1 + volumes = [self.data.test_volume_group_member] + with mock.patch.object( + self.common, '_clone_check') as mock_clone_chk: + self.common._delete_group(group, volumes) + mock_clone_chk.assert_called_once() + @mock.patch.object( common.PowerMaxCommon, '_remove_vol_and_cleanup_replication') @mock.patch.object( @@ -2907,6 +2948,22 @@ class PowerMaxCommonTest(test.TestCase): self.common._clone_check(array, device_id, extra_specs) self.assertEqual(3, mck_del.call_count) + @mock.patch.object( + common.PowerMaxCommon, '_unlink_targets_and_delete_temp_snapvx') + @mock.patch.object(rest.PowerMaxRest, 'find_snap_vx_sessions', + return_value=(tpd.PowerMaxData.snap_src_sessions, + tpd.PowerMaxData.snap_tgt_session)) + @mock.patch.object(rest.PowerMaxRest, 'is_vol_in_rep_session', + return_value=(True, True, False)) + def test_clone_check_force_unlink(self, mck_rep, mck_find, mck_del): + array = self.data.array + device_id = self.data.device_id + extra_specs = self.data.extra_specs + self.common.snapvx_unlink_limit = 3 + self.common._clone_check( + array, device_id, extra_specs, force_unlink=True) + self.assertEqual(3, mck_del.call_count) + @mock.patch.object(common.PowerMaxCommon, '_unlink_targets_and_delete_temp_snapvx') def test_delete_valid_snapshot(self, mck_unlink): diff --git a/cinder/volume/drivers/dell_emc/powermax/common.py b/cinder/volume/drivers/dell_emc/powermax/common.py index 3bae3429053..2154ad55c16 100644 --- a/cinder/volume/drivers/dell_emc/powermax/common.py +++ b/cinder/volume/drivers/dell_emc/powermax/common.py @@ -440,7 +440,6 @@ class PowerMaxCommon(object): :returns: model_update - dict """ model_update, rep_driver_data = dict(), dict() - group_name, group_id = None, None volume_id = volume.id extra_specs = self._initial_setup(volume) @@ -459,14 +458,10 @@ class PowerMaxCommon(object): rep_driver_data = rep_update['replication_driver_data'] model_update.update(rep_update) - # Add volume to group, if required - if volume.group_id is not None: - if (volume_utils.is_group_a_cg_snapshot_type(volume.group) - or volume.group.is_replicated): - group_id = volume.group_id - group_name = self._add_new_volume_to_volume_group( - volume, volume_dict['device_id'], volume_name, - extra_specs, rep_driver_data) + # Add volume to group + group_name = self._add_to_group( + volume, volume_dict['device_id'], volume_name, volume.group_id, + volume.group, extra_specs, rep_driver_data) # Gather Metadata model_update.update( @@ -482,7 +477,7 @@ class PowerMaxCommon(object): extra_specs[utils.ARRAY]) self.volume_metadata.capture_create_volume( - volume_dict['device_id'], volume, group_name, group_id, + volume_dict['device_id'], volume, group_name, volume.group_id, extra_specs, rep_info_dict, 'create', array_tag_list=array_tag_list) @@ -491,6 +486,29 @@ class PowerMaxCommon(object): return model_update + def _add_to_group( + self, volume, device_id, volume_name, group_id, group, + extra_specs, rep_driver_data=None): + """Add a volume to a volume group + + :param volume: volume object + :param device_id: the device id + :param volume_name: volume name + :param group_id: the group id + :param group: group object + :param extra_specs: extra specifications + :param rep_driver_data: replication data (optional) + :returns: group_id - string + """ + group_name = None + if group_id is not None: + if (volume_utils.is_group_a_cg_snapshot_type(group) + or group.is_replicated): + group_name = self._add_new_volume_to_volume_group( + volume, device_id, volume_name, + extra_specs, rep_driver_data) + return group_name + def _add_new_volume_to_volume_group(self, volume, device_id, volume_name, extra_specs, rep_driver_data=None): """Add a new volume to a volume group. @@ -563,6 +581,7 @@ class PowerMaxCommon(object): :returns: model_update, dict """ model_update, rep_info_dict = {}, {} + rep_driver_data = None extra_specs = self._initial_setup(clone_volume) array = extra_specs[utils.ARRAY] source_device_id = self._find_device_on_array( @@ -578,8 +597,15 @@ class PowerMaxCommon(object): clone_volume, source_volume, extra_specs) # Update model with replication session info if applicable if rep_update: + rep_driver_data = rep_update['replication_driver_data'] model_update.update(rep_update) + # Add volume to group + group_name = self._add_to_group( + clone_volume, clone_dict['device_id'], clone_volume.name, + source_volume.group_id, source_volume.group, extra_specs, + rep_driver_data) + model_update.update( {'provider_location': six.text_type(clone_dict)}) model_update = self.update_metadata( @@ -590,9 +616,11 @@ class PowerMaxCommon(object): utils.REP_CONFIG].get(utils.BACKEND_ID, 'None') array_tag_list = self.get_tags_of_storage_array( extra_specs[utils.ARRAY]) + self.volume_metadata.capture_create_volume( - clone_dict['device_id'], clone_volume, None, None, - extra_specs, rep_info_dict, 'createFromVolume', + clone_dict['device_id'], clone_volume, group_name, + source_volume.group_id, extra_specs, rep_info_dict, + 'createFromVolume', temporary_snapvx=clone_dict.get('snap_name'), source_device_id=clone_dict.get('source_device_id'), array_tag_list=array_tag_list) @@ -2762,12 +2790,14 @@ class PowerMaxCommon(object): return source_device_id - def _clone_check(self, array, device_id, extra_specs): + def _clone_check( + self, array, device_id, extra_specs, force_unlink=False): """Perform any snapvx cleanup before creating clones or snapshots :param array: the array serial :param device_id: the device ID of the volume :param extra_specs: extra specifications + :param force_unlink: force unlink even if not expired """ snapvx_tgt, snapvx_src, __ = self.rest.is_vol_in_rep_session( array, device_id) @@ -2779,32 +2809,36 @@ class PowerMaxCommon(object): array, src_device_id) count = 0 if tgt_session and count < self.snapvx_unlink_limit: - self._delete_valid_snapshot(array, tgt_session, - extra_specs) + self._delete_valid_snapshot( + array, tgt_session, extra_specs, force_unlink) count += 1 if src_sessions: src_sessions.sort( key=lambda k: k['generation'], reverse=True) for session in src_sessions: if count < self.snapvx_unlink_limit: - self._delete_valid_snapshot(array, session, - extra_specs) + self._delete_valid_snapshot( + array, session, extra_specs, force_unlink) count += 1 else: break do_unlink_and_delete_snap(device_id) - def _delete_valid_snapshot(self, array, session, extra_specs): + def _delete_valid_snapshot( + self, array, session, extra_specs, force_unlink=False): """Delete a snapshot if valid candidate for deletion. :param array: the array serial :param session: the snapvx session :param extra_specs: extra specifications + :param force_unlink: force unlink even if not expired """ is_legacy = 'EMC_SMI' in session['snap_name'] is_temp = utils.CLONE_SNAPSHOT_NAME in session['snap_name'] is_expired = session['expired'] + if force_unlink: + is_expired = True is_valid = True if is_legacy or (is_temp and is_expired) else False if is_valid: try: @@ -4691,6 +4725,8 @@ class PowerMaxCommon(object): device_id = self._find_device_on_array( vol, extra_specs) if device_id in volume_device_ids: + self._clone_check( + array, device_id, extra_specs, force_unlink=True) self.masking.remove_and_reset_members( array, vol, device_id, vol.name, extra_specs, False) @@ -5045,9 +5081,16 @@ class PowerMaxCommon(object): if group.is_replicated: # Need force flag when manipulating RDF enabled SGs interval_retries_dict[utils.FORCE_VOL_REMOVE] = True - self.masking.remove_volumes_from_storage_group( - array, remove_device_ids, - vol_grp_name, interval_retries_dict) + # Check if the volumes exist in the storage group + temp_list = deepcopy(remove_device_ids) + for device_id in temp_list: + if not self.rest.is_volume_in_storagegroup( + array, device_id, vol_grp_name): + remove_device_ids.remove(device_id) + if remove_device_ids: + self.masking.remove_volumes_from_storage_group( + array, remove_device_ids, + vol_grp_name, interval_retries_dict) if group.is_replicated: # Remove remote volumes from the remote storage group self._remove_remote_vols_from_volume_group(