From e1f6de6f836e1d534d0196ba43e6642bc5e279c6 Mon Sep 17 00:00:00 2001 From: Helen Walsh Date: Mon, 14 Jun 2021 15:32:18 +0100 Subject: [PATCH] PowerMax Driver - Improve error handling around deletes 1. Bring storage group check within delete volume so that it can be used in every place it is called and not just the delete method. 2. Remove the reset identifier_name name as this makes the device available for use straight away even if is not deleted. 3. Allow for None value in unlink_snapshot. 4. Add extra debugging information for device id. 5. Retry on getting connection info for volume. 6. Validate a masking view on an attach operation. 7. Reset identifier name on a rollback. 8. Retry a cleanup if snap id not available at the time. 9. Pylint fixes. 10. Retry on a failed delete with a cleanup snapvx in exception. Change-Id: I0c0f3d2246b840326579748027b9cd15bf174ff8 --- .../dell_emc/powermax/powermax_data.py | 7 + .../powermax/powermax_fake_objects.py | 2 +- .../dell_emc/powermax/test_powermax_common.py | 99 ++++++++++- .../powermax/test_powermax_masking.py | 42 ++++- .../powermax/test_powermax_replication.py | 15 +- .../dell_emc/powermax/test_powermax_rest.py | 33 +++- .../drivers/dell_emc/powermax/common.py | 166 +++++++++++++----- .../drivers/dell_emc/powermax/masking.py | 122 ++++++++++++- .../drivers/dell_emc/powermax/provision.py | 5 +- .../volume/drivers/dell_emc/powermax/rest.py | 98 ++++++++--- 10 files changed, 496 insertions(+), 93 deletions(-) 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 1dca158b98f..c003f219536 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 @@ -665,6 +665,13 @@ class PowerMaxData(object): {'host_lun_address': '0003'}]}, {}] + maskingview_no_lun = { + 'maskingViewId': masking_view_name_f, + 'portGroupId': port_group_name_f, + 'storageGroupId': storagegroup_name_f, + 'hostId': initiatorgroup_name_f, + 'maskingViewConnection': []} + portgroup = [{'portGroupId': port_group_name_f, 'symmetrixPortKey': [ {'directorId': 'FA-1D', diff --git a/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_fake_objects.py b/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_fake_objects.py index 794f548b0df..aecfa40fa28 100644 --- a/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_fake_objects.py +++ b/cinder/tests/unit/volume/drivers/dell_emc/powermax/powermax_fake_objects.py @@ -146,7 +146,7 @@ class FakeRequestsSession(object): if '/private' in url: return_object = self.data.private_vol_details elif params: - if '1' in params.values(): + if '1' in params.values() or 'volume_identifier' in params: return_object = self.data.volume_list[0] elif '2' in params.values(): return_object = self.data.volume_list[1] 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 423774c1d9d..3a17bd78ca2 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 @@ -399,7 +399,7 @@ class PowerMaxCommonTest(test.TestCase): extra_specs = self.common._initial_setup(test_volume) self.assertRaises(exception.VolumeBackendAPIException, self.common._delete_volume, test_volume) - mck_cleanup.assert_called_once_with(array, device_id, extra_specs) + mck_cleanup.assert_called_with(array, device_id, extra_specs) mck_delete.assert_not_called() @mock.patch.object(common.PowerMaxCommon, '_delete_from_srp') @@ -417,7 +417,7 @@ class PowerMaxCommonTest(test.TestCase): extra_specs = self.common._initial_setup(test_volume) self.assertRaises(exception.VolumeBackendAPIException, self.common._delete_volume, test_volume) - mck_cleanup.assert_called_once_with(array, device_id, extra_specs) + mck_cleanup.assert_called_with(array, device_id, extra_specs) mck_delete.assert_not_called() @mock.patch.object(common.PowerMaxCommon, '_cleanup_device_snapvx') @@ -1941,12 +1941,15 @@ class PowerMaxCommonTest(test.TestCase): clone_name, snap_name, self.data.extra_specs, target_volume=clone_volume) + @mock.patch.object(rest.PowerMaxRest, 'get_storage_groups_from_volume', + return_value=[]) @mock.patch.object(rest.PowerMaxRest, 'get_snap_id', return_value=tpd.PowerMaxData.snap_id) @mock.patch.object( masking.PowerMaxMasking, 'remove_and_reset_members') - def test_cleanup_target_sync_present(self, mock_remove, mock_snaps): + def test_cleanup_target_sync_present( + self, mock_remove, mock_snaps, mock_sgs): array = self.data.array clone_volume = self.data.test_clone_volume source_device_id = self.data.device_id @@ -2974,6 +2977,7 @@ class PowerMaxCommonTest(test.TestCase): model_update, __ = self.common._delete_group(group, volumes) self.assertEqual(ref_model_update, model_update) + @mock.patch.object(common.PowerMaxCommon, '_delete_from_srp') @mock.patch.object(masking.PowerMaxMasking, 'remove_and_reset_members') @mock.patch.object(common.PowerMaxCommon, '_cleanup_device_snapvx') @mock.patch.object(common.PowerMaxCommon, '_get_members_of_volume_group', @@ -2983,7 +2987,7 @@ class PowerMaxCommonTest(test.TestCase): @mock.patch.object(volume_utils, 'is_group_a_type', return_value=False) def test_delete_group_snapshot_and_volume_cleanup( self, mock_check, mck_get_snaps, mock_members, mock_cleanup, - mock_remove): + mock_remove, mock_del): group = self.data.test_group_1 volumes = [fake_volume.fake_volume_obj( context='cxt', provider_location=None)] @@ -3837,6 +3841,13 @@ class PowerMaxCommonTest(test.TestCase): act_metadata = self.common.get_volume_metadata(array, device_id) self.assertEqual(ref_metadata, act_metadata) + def test_get_volume_metadata_device_none(self): + ref_metadata = {} + array = self.data.array + device_id = None + act_metadata = self.common.get_volume_metadata(array, device_id) + self.assertEqual(ref_metadata, act_metadata) + @mock.patch.object(rest.PowerMaxRest, 'get_volume_snap_info', return_value=tpd.PowerMaxData.priv_snap_response) def test_get_snapshot_metadata(self, mck_snap): @@ -3925,6 +3936,23 @@ class PowerMaxCommonTest(test.TestCase): model_update, existing_metadata, object_metadata) self.assertEqual(ref_model_update, model_update) + def test_update_metadata_no_object_metadata(self): + model_update = {'provider_location': six.text_type( + self.data.provider_location)} + ref_model_update = ( + {'provider_location': six.text_type(self.data.provider_location), + 'metadata': {'user-meta-key-1': 'user-meta-value-1', + 'user-meta-key-2': 'user-meta-value-2'}}) + + existing_metadata = {'user-meta-key-1': 'user-meta-value-1', + 'user-meta-key-2': 'user-meta-value-2'} + + object_metadata = {} + + model_update = self.common.update_metadata( + model_update, existing_metadata, object_metadata) + self.assertEqual(ref_model_update, model_update) + def test_update_metadata_model_list_exception(self): model_update = [{'provider_location': six.text_type( self.data.provider_location)}] @@ -4562,3 +4590,66 @@ class PowerMaxCommonTest(test.TestCase): self.assertTrue(rnr.rdf_pair_broken) self.assertTrue(rnr.resume_original_sg) self.assertFalse(rnr.is_partitioned) + + @mock.patch.object(common.PowerMaxCommon, '_cleanup_device_snapvx') + @mock.patch.object(rest.PowerMaxRest, 'get_volume_snapshot_list', + return_value=[]) + @mock.patch.object(rest.PowerMaxRest, 'find_snap_vx_sessions', + side_effect=[(None, tpd.PowerMaxData.snap_tgt_session), + (None, None)]) + def test_cleanup_device_retry(self, mock_snapvx, mock_ss_list, mock_clean): + self.common._cleanup_device_retry( + self.data.array, self.data.device_id, self.data.extra_specs) + self.assertEqual(2, mock_clean.call_count) + + @mock.patch.object(rest.PowerMaxRest, 'find_volume_device_id', + return_value=[tpd.PowerMaxData.device_id, + tpd.PowerMaxData.device_id2]) + def test_get_device_id_from_identifier_list(self, mock_dev_id): + ret_dev = self.common._get_device_id_from_identifier( + self.data.array, 'vol', self.data.device_id) + self.assertEqual(self.data.device_id, ret_dev) + + @mock.patch.object(rest.PowerMaxRest, 'find_volume_device_id', + return_value=tpd.PowerMaxData.device_id2) + def test_get_device_id_from_identifier_wrong(self, mock_dev_id): + ret_dev = self.common._get_device_id_from_identifier( + self.data.array, 'vol', self.data.device_id) + self.assertEqual(self.data.device_id2, ret_dev) + + @mock.patch.object(rest.PowerMaxRest, 'find_volume_device_id', + return_value=tpd.PowerMaxData.device_id) + def test_get_device_id_from_identifier_same(self, mock_dev_id): + ret_dev = self.common._get_device_id_from_identifier( + self.data.array, 'vol', self.data.device_id) + self.assertIsNone(ret_dev) + + @mock.patch.object(rest.PowerMaxRest, 'rename_volume') + @mock.patch.object(rest.PowerMaxRest, 'find_volume_identifier', + return_value='vol') + @mock.patch.object(rest.PowerMaxRest, 'find_volume_device_id', + return_value=tpd.PowerMaxData.device_id) + def test_reset_identifier_on_rollback_rename( + self, mock_dev, mock_ident, mock_rename): + self.common._reset_identifier_on_rollback(self.data.array, 'vol') + mock_rename.assert_called_once() + + @mock.patch.object(rest.PowerMaxRest, 'rename_volume') + @mock.patch.object(rest.PowerMaxRest, 'find_volume_identifier', + return_value='diff_vol_name') + @mock.patch.object(rest.PowerMaxRest, 'find_volume_device_id', + return_value=tpd.PowerMaxData.device_id) + def test_reset_identifier_on_rollback_no_rename( + self, mock_dev, mock_ident, mock_rename): + self.common._reset_identifier_on_rollback(self.data.array, 'vol') + mock_rename.assert_not_called() + + @mock.patch.object(common.PowerMaxCommon, '_cleanup_device_snapvx') + @mock.patch.object( + provision.PowerMaxProvision, 'delete_volume_from_srp', + side_effect=[exception.VolumeBackendAPIException, None]) + def test_test_delete_from_srp(self, mock_del, mock_clean): + self.common._delete_from_srp( + self.data.array, 'vol_name', self.data.device_id, + self.data.extra_specs) + mock_clean.assert_called_once() 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 40ce442760d..02e43f35280 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 @@ -96,6 +96,8 @@ class PowerMaxMaskingTest(test.TestCase): self.maskingviewdict, self.extra_specs) mock_get_or_create_mv.assert_called_once() + @mock.patch.object(masking.PowerMaxMasking, '_validate_attach', + return_value=True) @mock.patch.object(masking.PowerMaxMasking, '_check_adding_volume_to_storage_group') @mock.patch.object(masking.PowerMaxMasking, '_move_vol_from_default_sg', @@ -108,7 +110,7 @@ class PowerMaxMaskingTest(test.TestCase): Exception('Exception')]) def test_get_or_create_masking_view_and_map_lun( self, mock_masking_view_element, mock_masking, mock_move, - mock_add_volume): + mock_add_volume, mock_validate): rollback_dict = ( self.driver.masking.get_or_create_masking_view_and_map_lun( self.data.array, self.data.test_volume, @@ -864,6 +866,9 @@ class PowerMaxMaskingTest(test.TestCase): self.data.array, self.data.masking_view_name_i) mock_delete_mv.assert_called_once() + @mock.patch.object( + rest.PowerMaxRest, 'get_storage_groups_from_volume', + return_value=list()) @mock.patch.object(masking.PowerMaxMasking, 'return_volume_to_volume_group') @mock.patch.object(rest.PowerMaxRest, 'move_volume_between_storage_groups') @@ -871,7 +876,7 @@ class PowerMaxMaskingTest(test.TestCase): 'get_or_create_default_storage_group') @mock.patch.object(masking.PowerMaxMasking, 'add_volume_to_storage_group') def test_add_volume_to_default_storage_group( - self, mock_add_sg, mock_get_sg, mock_move, mock_return): + self, mock_add_sg, mock_get_sg, mock_move, mock_return, mock_sgs): self.mask.add_volume_to_default_storage_group( self.data.array, self.device_id, self.volume_name, self.extra_specs) @@ -1372,3 +1377,36 @@ class PowerMaxMaskingTest(test.TestCase): exception_message): self.mask._check_director_and_port_status( self.data.array, self.data.port_group_name_f) + + @mock.patch.object( + rest.PowerMaxRest, 'get_storage_groups_from_volume', + return_value=([tpd.PowerMaxData.storagegroup_name_f])) + @mock.patch.object( + rest.PowerMaxRest, 'get_masking_views_from_storage_group', + return_value=[tpd.PowerMaxData.masking_view_name_f]) + def test_validate_attach(self, mock_sgs, mock_mvs): + self.assertTrue(self.mask._validate_attach( + self.data.array, self.data.device_id, + self.data.storagegroup_name_f, self.data.masking_view_name_f)) + + @mock.patch.object( + rest.PowerMaxRest, 'get_storage_groups_from_volume', + return_value=([tpd.PowerMaxData.storagegroup_name_f])) + @mock.patch.object( + rest.PowerMaxRest, 'get_masking_views_from_storage_group', + return_value=[]) + def test_validate_attach_no_mvs(self, mock_sgs, mock_mvs): + self.assertFalse(self.mask._validate_attach( + self.data.array, self.data.device_id, + self.data.storagegroup_name_f, self.data.masking_view_name_f)) + + @mock.patch.object( + rest.PowerMaxRest, 'get_storage_groups_from_volume', + return_value=([tpd.PowerMaxData.defaultstoragegroup_name])) + @mock.patch.object( + rest.PowerMaxRest, 'get_masking_views_from_storage_group', + return_value=[]) + def test_validate_attach_incorrect_sg(self, mock_sgs, mock_mvs): + self.assertFalse(self.mask._validate_attach( + self.data.array, self.data.device_id, + self.data.storagegroup_name_f, self.data.masking_view_name_f)) 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..4ab8ba3f1dd 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 @@ -216,12 +216,21 @@ class PowerMaxReplicationTest(test.TestCase): self.iscsi_common.initialize_connection, self.data.test_volume, self.data.connector) + @mock.patch.object(masking.PowerMaxMasking, '_validate_attach', + return_value=True) + @mock.patch.object( + rest.PowerMaxRest, 'get_storage_groups_from_volume', + side_effect=[ + [], [], [], [], [tpd.PowerMaxData.storagegroup_name_f], [], + [tpd.PowerMaxData.storagegroup_name_f], + [tpd.PowerMaxData.storagegroup_name_f]], ) @mock.patch.object( masking.PowerMaxMasking, '_check_director_and_port_status') @mock.patch.object( masking.PowerMaxMasking, 'pre_multiattach', return_value=tpd.PowerMaxData.masking_view_dict_multiattach) - def test_attach_metro_volume(self, mock_pre, mock_check): + def test_attach_metro_volume( + self, mock_pre, mock_check, mock_sgs, mock_validate): rep_extra_specs = deepcopy(tpd.PowerMaxData.rep_extra_specs) rep_extra_specs[utils.PORTGROUPNAME] = self.data.port_group_name_f hostlunid, remote_port_group = self.common._attach_metro_volume( @@ -802,10 +811,12 @@ class PowerMaxReplicationTest(test.TestCase): self.data.test_rep_group, self.data.rep_extra_specs) mock_rm.assert_called_once() + @mock.patch.object(rest.PowerMaxRest, 'get_storage_groups_from_volume', + return_value=[]) @mock.patch.object(masking.PowerMaxMasking, 'remove_and_reset_members') @mock.patch.object(masking.PowerMaxMasking, 'remove_volumes_from_storage_group') - def test_cleanup_group_replication(self, mock_rm, mock_rm_reset): + def test_cleanup_group_replication(self, mock_rm, mock_rm_reset, mock_sgs): self.common._cleanup_group_replication( self.data.array, self.data.test_vol_grp_name, [self.data.device_id], self.extra_specs, self.data.rep_config_sync) 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 6be27cf6680..fbe8c198ac2 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 @@ -741,7 +741,9 @@ class PowerMaxRestTest(test.TestCase): mck_modify.assert_called_once_with(array, device_id, extend_vol_payload) - def test_legacy_delete_volume(self): + @mock.patch.object(rest.PowerMaxRest, 'get_storage_groups_from_volume', + return_value=[]) + def test_legacy_delete_volume(self, mock_sgs): device_id = self.data.device_id vb_except = exception.VolumeBackendAPIException with mock.patch.object(self.rest, 'delete_resource') as mock_delete, ( @@ -755,21 +757,28 @@ class PowerMaxRestTest(test.TestCase): mock_delete.assert_called_once_with( self.data.array, 'sloprovisioning', 'volume', device_id) - def test_delete_volume(self): + @mock.patch.object(rest.PowerMaxRest, 'get_storage_groups_from_volume', + return_value=[]) + def test_delete_volume(self, mock_sgs): device_id = self.data.device_id self.rest.ucode_major_level = utils.UCODE_5978 self.rest.ucode_minor_level = utils.UCODE_5978_HICKORY with mock.patch.object( - self.rest, 'delete_resource') as mock_delete, ( - mock.patch.object( - self.rest, '_modify_volume')) as mock_modify: - + self.rest, 'delete_resource') as mock_delete: self.rest.delete_volume(self.data.array, device_id) - mod_call_count = mock_modify.call_count - self.assertEqual(1, mod_call_count) mock_delete.assert_called_once_with( self.data.array, 'sloprovisioning', 'volume', device_id) + @mock.patch.object(rest.PowerMaxRest, 'get_storage_groups_from_volume', + return_value=['OS-SG']) + def test_delete_volume_in_sg(self, mock_sgs): + device_id = self.data.device_id + self.rest.ucode_major_level = utils.UCODE_5978 + self.rest.ucode_minor_level = utils.UCODE_5978_HICKORY + self.assertRaises( + exception.VolumeBackendAPIException, + self.rest.delete_volume, self.data.array, device_id) + def test_rename_volume(self): device_id = self.data.device_id payload = {'editVolumeActionParam': { @@ -847,6 +856,14 @@ class PowerMaxRestTest(test.TestCase): self.data.array, self.data.masking_view_name_f, device_id) self.assertEqual(ref_lun_id, host_lun_id) + def test_find_mv_connections_for_vol_missing_host_lun_address(self): + with mock.patch.object(self.rest, 'get_resource', + return_value=self.data.maskingview_no_lun): + host_lun_id = self.rest.find_mv_connections_for_vol( + self.data.array, self.data.masking_view_name_f, + self.data.device_id) + self.assertIsNone(host_lun_id) + def test_find_mv_connections_for_vol_failed(self): # no masking view info retrieved device_id = self.data.volume_details[0]['volumeId'] diff --git a/cinder/volume/drivers/dell_emc/powermax/common.py b/cinder/volume/drivers/dell_emc/powermax/common.py index fa802ed191a..601f3124140 100644 --- a/cinder/volume/drivers/dell_emc/powermax/common.py +++ b/cinder/volume/drivers/dell_emc/powermax/common.py @@ -2057,15 +2057,32 @@ class PowerMaxCommon(object): device_id = self._find_device_on_array(volume, extra_specs) if device_id is None: - LOG.error("Volume %(name)s not found on the array. " - "No volume to delete.", - {'name': volume_name}) + LOG.warning("Volume %(name)s not found on the array. " + "No volume to delete.", + {'name': volume_name}) return volume_name array = extra_specs[utils.ARRAY] if self.utils.is_replication_enabled(extra_specs): self._validate_rdfg_status(array, extra_specs) + self._cleanup_device_retry(array, device_id, extra_specs) + + # Remove from any storage groups and cleanup replication + self._remove_vol_and_cleanup_replication( + array, device_id, volume_name, extra_specs, volume) + self._delete_from_srp( + array, device_id, volume_name, extra_specs) + return volume_name + + @retry(retry_exc_tuple, interval=1, retries=3) + def _cleanup_device_retry(self, array, device_id, extra_specs): + """Cleanup snapvx on the device + + :param array: the serial number of the array -- str + :param device_id: the device id -- str + :param extra_specs: extra specs -- dict + """ # Check if the volume being deleted is a # source or target for copy session self._cleanup_device_snapvx(array, device_id, extra_specs) @@ -2083,24 +2100,12 @@ class PowerMaxCommon(object): if snapvx_target_details: source_device = snapvx_target_details.get('source_vol_id') snapshot_name = snapvx_target_details.get('snap_name') - raise exception.VolumeBackendAPIException(_( - 'Cannot delete device %s as it is currently a linked target ' - 'of snapshot %s. The source device of this link is %s. ' - 'Please try again once this snapshots is no longer ' - 'active.') % (device_id, snapshot_name, source_device)) - - # Remove from any storage groups and cleanup replication - self._remove_vol_and_cleanup_replication( - array, device_id, volume_name, extra_specs, volume) - # Check if volume is in any storage group - sg_list = self.rest.get_storage_groups_from_volume(array, device_id) - if sg_list: - LOG.error("Device %(device_id)s is in storage group(s) " - "%(sg_list)s prior to delete. Delete will fail.", - {'device_id': device_id, 'sg_list': sg_list}) - self._delete_from_srp( - array, device_id, volume_name, extra_specs) - return volume_name + if snapshot_name: + raise exception.VolumeBackendAPIException(_( + 'Cannot delete device %s as it is currently a linked ' + 'target of snapshot %s. The source device of this link ' + 'is %s. Please try again once this snapshot is no longer ' + 'active.') % (device_id, snapshot_name, source_device)) def _create_volume(self, volume, volume_name, volume_size, extra_specs): """Create a volume. @@ -2162,15 +2167,40 @@ class PowerMaxCommon(object): array, volume, volume_name, volume_size, extra_specs, storagegroup_name, rep_mode)) - # Compare volume ID against identifier on array. Update if needed. - # This can occur in cases where multiple edits are occurring at once. - found_device_id = self.rest.find_volume_device_id(array, volume_name) - returning_device_id = volume_dict['device_id'] - if found_device_id != returning_device_id: - volume_dict['device_id'] = found_device_id + device_id = self._get_device_id_from_identifier( + array, volume_name, volume_dict['device_id']) + if device_id: + volume_dict['device_id'] = device_id return volume_dict, rep_update, rep_info_dict + def _get_device_id_from_identifier( + self, array, volume_name, orig_device_id): + """Get the device(s) using the identifier name + + :param array: the serial number of the array -- str + :param volume_name: the user supplied volume name -- str + :param orig_device_id: the original device id -- str + :returns: device id -- str + """ + # Compare volume ID against identifier on array. Update if needed. + # This can occur in cases where multiple edits are occurring at once. + dev_id_from_identifier = self.rest.find_volume_device_id( + array, volume_name) + if isinstance(dev_id_from_identifier, list): + if orig_device_id in dev_id_from_identifier: + return orig_device_id + else: + if dev_id_from_identifier != orig_device_id: + LOG.warning( + "The device id %(dev_ident)s associated with %(vol_name)s " + "is not the same as device %(dev_orig)s.", + {'dev_ident': dev_id_from_identifier, + 'vol_name': volume_name, + 'dev_orig': orig_device_id}) + return dev_id_from_identifier + return None + @coordination.synchronized("emc-nonrdf-vol-{storagegroup_name}-{array}") def _create_non_replicated_volume( self, array, volume, volume_name, storagegroup_name, volume_size, @@ -2195,6 +2225,7 @@ class PowerMaxCommon(object): return volume_dict except Exception as e: try: + self._reset_identifier_on_rollback(array, volume_name) # Attempt cleanup of storage group post exception. updated_devices = set(self.rest.get_volumes_in_storage_group( array, storagegroup_name)) @@ -2518,6 +2549,7 @@ class PowerMaxCommon(object): LOG.info("The tag list %(tag_list)s has been verified.", {'tag_list': array_tag_list}) + @retry(retry_exc_tuple, interval=3, retries=3) def _delete_from_srp(self, array, device_id, volume_name, extra_specs): """Delete from srp. @@ -2534,11 +2566,17 @@ class PowerMaxCommon(object): self.provision.delete_volume_from_srp( array, device_id, volume_name) except Exception as e: - error_message = (_("Failed to delete volume %(volume_name)s. " - "Exception received: %(e)s") % - {'volume_name': volume_name, - 'e': six.text_type(e)}) + error_message = (_( + "Failed to delete volume %(volume_name)s with device id " + "%(dev)s. Exception received: %(e)s.") % + {'volume_name': volume_name, + 'dev': device_id, + 'e': six.text_type(e)}) LOG.error(error_message) + LOG.warning("Attempting device cleanup after a failed delete of: " + "%(name)s. device_id: %(device_id)s.", + {'name': volume_name, 'device_id': device_id}) + self._cleanup_device_snapvx(array, device_id, extra_specs) raise exception.VolumeBackendAPIException(message=error_message) def _remove_vol_and_cleanup_replication( @@ -2953,7 +2991,7 @@ class PowerMaxCommon(object): return source_device_id - @retry(retry_exc_tuple, interval=1, retries=3) + @retry(retry_exc_tuple, interval=10, retries=3) def _cleanup_device_snapvx( self, array, device_id, extra_specs): """Perform any snapvx cleanup before creating clones or snapshots @@ -2990,17 +3028,15 @@ class PowerMaxCommon(object): :param array: the array serial number :param extra_specs: extra specifications """ - try: - session_unlinked = self._unlink_snapshot( - session, array, extra_specs) - if session_unlinked: - self._delete_temp_snapshot(session, array) - except exception.VolumeBackendAPIException as e: - # Ignore and continue as snapshot has been unlinked - # successfully with incorrect status code returned - if ('404' and session['snap_name'] and - 'does not exist' in six.text_type(e)): - pass + session_unlinked = self._unlink_snapshot( + session, array, extra_specs) + if session_unlinked: + self._delete_temp_snapshot(session, array) + else: + LOG.warning( + "Snap name %(snap)s is still linked. The delete of " + "the temporary snapshot has not occurred.", + {'snap': session.get('snap_name')}) def _unlink_snapshot(self, session, array, extra_specs): """Helper for unlinking temporary snapshot during cleanup. @@ -3013,10 +3049,19 @@ class PowerMaxCommon(object): snap_name = session.get('snap_name') source = session.get('source_vol_id') snap_id = session.get('snapid') + if snap_id is None: + exception_message = ( + _("Unable to get snapid from session %(session)s for source " + "device %(dev)s. Retrying...") + % {'dev': source, 'session': session}) + # Warning only as there will be a retry + LOG.warning(exception_message) + raise exception.VolumeBackendAPIException( + message=exception_message) snap_info = self.rest.get_volume_snap( array, source, snap_name, snap_id) - is_linked = snap_info.get('linkedDevices') + is_linked = snap_info.get('linkedDevices') if snap_info else None target, cm_enabled = None, False if session.get('target_vol_id'): @@ -3252,7 +3297,7 @@ class PowerMaxCommon(object): volume_identifier = None # Check if volume is already cinder managed if volume_details.get('volume_identifier'): - volume_identifier = volume_details['volume_identifier'] + volume_identifier = volume_details.get('volume_identifier') if volume_identifier.startswith(utils.VOLUME_ELEMENT_NAME_PREFIX): raise exception.ManageExistingAlreadyManaged( volume_ref=volume_id) @@ -3569,6 +3614,7 @@ class PowerMaxCommon(object): "they can be managed into Cinder.") return manageable_vols + volumes = volumes or list() for device in volumes: # Determine if volume is valid for management if self.utils.is_volume_manageable(device): @@ -3677,6 +3723,7 @@ class PowerMaxCommon(object): "Cinder.") return manageable_snaps + volumes = volumes or list() for device in volumes: # Determine if volume is valid for management manageable_snaps = self._is_snapshot_valid_for_management( @@ -4760,7 +4807,7 @@ class PowerMaxCommon(object): if self.rest.is_next_gen_array(target_array_serial): target_workload = 'NONE' except IndexError: - LOG.error("Error parsing array, pool, SLO and workload.") + LOG.debug("Error parsing array, pool, SLO and workload.") return false_ret if self.promotion: @@ -6092,7 +6139,7 @@ class PowerMaxCommon(object): # Get the volume group dict for getting the group name volume_group = (self._find_volume_group(array, source_group)) if volume_group and volume_group.get('name'): - vol_grp_name = volume_group['name'] + vol_grp_name = volume_group.get('name') if vol_grp_name is None: LOG.warning("Cannot find generic volume group %(grp_ss_id)s. " "on array %(array)s", @@ -7107,6 +7154,8 @@ class PowerMaxCommon(object): :param device_id: the device ID :returns: dict -- volume metadata """ + if device_id is None: + return dict() vol_info = self.rest._get_private_volume(array, device_id) vol_header = vol_info['volumeHeader'] array_model, __ = self.rest.get_array_model_info(array) @@ -7576,3 +7625,26 @@ class PowerMaxCommon(object): management_sg_name, rdf_group_number, missing_volumes_str) is_valid = False return is_valid + + def _reset_identifier_on_rollback(self, array, volume_name): + """Reset the user supplied name on a rollback + + :param array: the serial number -- str + :param volume_name: the volume name assigned -- str + """ + # Find volume based on identifier name + dev_id_from_identifier = self.rest.find_volume_device_id( + array, volume_name) + if dev_id_from_identifier and isinstance( + dev_id_from_identifier, str): + vol_identifier_name = self.rest.find_volume_identifier( + array, dev_id_from_identifier) + if vol_identifier_name and ( + vol_identifier_name == volume_name): + LOG.warning( + "Attempting to reset name of %(vol_name)s on device " + "%(dev_ident)s on a create volume rollback operation.", + {'vol_name': volume_name, + 'dev_ident': dev_id_from_identifier}) + self.rest.rename_volume( + array, dev_id_from_identifier, None) diff --git a/cinder/volume/drivers/dell_emc/powermax/masking.py b/cinder/volume/drivers/dell_emc/powermax/masking.py index 5fad8cd212a..545bfef13b1 100644 --- a/cinder/volume/drivers/dell_emc/powermax/masking.py +++ b/cinder/volume/drivers/dell_emc/powermax/masking.py @@ -90,12 +90,14 @@ class PowerMaxMasking(object): error_message = self._get_or_create_masking_view( serial_number, masking_view_dict, default_sg_name, extra_specs) - LOG.debug( - "The masking view in the attach operation is " - "%(masking_name)s. The storage group " - "in the masking view is %(storage_name)s.", - {'masking_name': maskingview_name, - 'storage_name': storagegroup_name}) + # Check that the device is in the correct storage group + if not self._validate_attach( + serial_number, device_id, storagegroup_name, + maskingview_name): + error_message = ("The attach validation for device " + "%(dev)s was unsuccessful.") + raise exception.VolumeBackendAPIException( + message=error_message) rollback_dict['portgroup_name'] = ( self.rest.get_element_from_masking_view( serial_number, maskingview_name, portgroup=True)) @@ -131,6 +133,71 @@ class PowerMaxMasking(object): return rollback_dict + def _validate_attach( + self, serial_number, device_id, dest_sg_name, dest_mv_name): + """Validate that the attach was successful. + + :param serial_number: the array serial number + :param device_id: the device id + :param dest_sg_name: the correct storage group + :param dest_mv_name: the correct masking view + :returns: bool + """ + sg_list = self.rest.get_storage_groups_from_volume( + serial_number, device_id) + + def _validate_masking_view(): + mv_list = self.rest.get_masking_views_from_storage_group( + serial_number, dest_sg_name) + if not mv_list: + LOG.error( + "The masking view list of %(sg_name)s where device " + "%(dev)s exists is empty.", + {'sg_name': dest_sg_name, + 'dev': device_id}) + return False + + if dest_mv_name.lower() in ( + mv_name.lower() for mv_name in mv_list): + LOG.debug( + "The masking view in the attach operation is " + "%(masking_name)s. The storage group " + "in the masking view is %(storage_name)s. " + "The device id is %(dev)s.", + {'masking_name': dest_mv_name, + 'storage_name': dest_sg_name, + 'dev': device_id}) + return True + else: + LOG.error( + "The masking view %(masking_name)s is not in " + "the masking view list %(mv_list)s. " + "%(sg_name)s is the storage group where device " + "%(dev)s exists.", + {'masking_name': dest_mv_name, + 'mv_list': mv_list, + 'sg_name': dest_sg_name, + 'dev': device_id}) + return False + + if not sg_list: + LOG.error( + "Device %(dev)s does not exist in any storage group.", + {'dev': device_id}) + return False + if dest_sg_name.lower() in ( + sg_name.lower() for sg_name in sg_list): + return _validate_masking_view() + else: + LOG.error( + "The storage group %(sg_name)s is not in " + "the storage group list %(sg_list)s " + "where device %(dev)s exists.", + {'sg_name': dest_sg_name, + 'sg_list': sg_list, + 'dev': device_id}) + return False + def _move_vol_from_default_sg( self, serial_number, device_id, volume_name, default_sg_name, dest_storagegroup, extra_specs, @@ -725,6 +792,13 @@ class PowerMaxMasking(object): {'volume_name': volume_name, 'sg_name': storagegroup_name}) else: + storage_group_list = self.rest.get_storage_groups_from_volume( + serial_number, device_id) + if storage_group_list: + msg = self._check_add_volume_suitability( + serial_number, device_id, volume_name, storagegroup_name) + if msg: + return msg try: force = True if extra_specs.get(utils.IS_RE) else False self.add_volume_to_storage_group( @@ -738,6 +812,42 @@ class PowerMaxMasking(object): LOG.error(msg) return msg + def _check_add_volume_suitability( + self, serial_number, device_id, volume_name, add_sg_name): + """Check if possible to add a volume to a storage group + + If a volume already belongs to a storage group that is associated + with FAST it is not possible to add that same volume to another + storage group which is also associated with FAST + + :param serial_number: the array serial number + :param device_id: the device id + :param volume_name: the client supplied volume name + :param add_sg_name: storage group that the volume is to be added to + :returns: msg -- str or None + """ + storage_group = self.rest.get_storage_group( + serial_number, add_sg_name) + if storage_group and storage_group.get('slo') and ( + storage_group.get('slo').lower() == 'none'): + return None + storage_group_list = self.rest.get_storage_groups_from_volume( + serial_number, device_id) + if storage_group_list: + msg = ("Volume %(vol)s with device id %(dev)s belongs " + "to storage group(s) %(sgs)s. Cannot add volume " + "to another storage group associated with FAST." + % {'vol': volume_name, 'dev': device_id, + 'sgs': storage_group_list}) + for storage_group_name in storage_group_list: + storage_group = self.rest.get_storage_group( + serial_number, storage_group_name) + if storage_group and storage_group.get('slo') and ( + storage_group.get('slo').lower() != 'none'): + LOG.error(msg) + return msg + return None + def add_volume_to_storage_group( self, serial_number, device_id, storagegroup_name, volume_name, extra_specs, force=False): diff --git a/cinder/volume/drivers/dell_emc/powermax/provision.py b/cinder/volume/drivers/dell_emc/powermax/provision.py index dce8f42d7b0..e27885f9866 100644 --- a/cinder/volume/drivers/dell_emc/powermax/provision.py +++ b/cinder/volume/drivers/dell_emc/powermax/provision.py @@ -119,8 +119,9 @@ class PowerMaxProvision(object): :param volume_name: the volume name """ start_time = time.time() - LOG.debug("Delete volume %(volume_name)s from srp.", - {'volume_name': volume_name}) + LOG.debug("Delete volume %(volume_name)s with device id %(dev)s " + "from srp.", {'volume_name': volume_name, + 'dev': device_id}) self.rest.delete_volume(array, device_id) LOG.debug("Delete volume took: %(delta)s H:MM:SS.", {'delta': self.utils.get_time_delta( diff --git a/cinder/volume/drivers/dell_emc/powermax/rest.py b/cinder/volume/drivers/dell_emc/powermax/rest.py index f18cf6b126e..3458d504736 100644 --- a/cinder/volume/drivers/dell_emc/powermax/rest.py +++ b/cinder/volume/drivers/dell_emc/powermax/rest.py @@ -1224,6 +1224,11 @@ class PowerMaxRest(object): element_name = self.utils.get_volume_element_name(name_id) if vol_identifier == element_name: found_device_id = device_id + else: + LOG.error("We cannot verify that device %(dev)s was " + "created/managed by openstack by its " + "identifier name.", {'dev': device_id}) + return found_device_id def add_vol_to_sg(self, array, storagegroup_name, device_id, extra_specs, @@ -1412,6 +1417,9 @@ class PowerMaxRest(object): :returns: volume dict :raises: VolumeBackendAPIException """ + if not device_id: + LOG.warning('No device id supplied to get_volume.') + return dict() version = self.get_uni_version()[1] volume_dict = self.get_resource( array, SLOPROVISIONING, 'volume', resource_name=device_id, @@ -1530,6 +1538,9 @@ class PowerMaxRest(object): :param device_id: the volume device id :param new_name: the new name for the volume, can be None """ + if not device_id: + LOG.warning('No device id supplied to rename operation.') + return if new_name is not None: vol_identifier_dict = { "identifier_name": new_name, @@ -1547,17 +1558,26 @@ class PowerMaxRest(object): :param array: the array serial number :param device_id: volume device id """ + # Check if volume is in any storage group + sg_list = self.get_storage_groups_from_volume( + array, device_id) + if sg_list: + exception_message = (_( + "Device %(device_id)s is in storage group(s) " + "%(sg_list)s prior to delete.") + % {'device_id': device_id, 'sg_list': sg_list}) + LOG.error(exception_message) + raise exception.VolumeBackendAPIException(exception_message) if ((self.ucode_major_level >= utils.UCODE_5978) and (self.ucode_minor_level > utils.UCODE_5978_ELMSR)): # Use Rapid TDEV Deallocation to delete after ELMSR try: - # Rename volume, removing the OS- - self.rename_volume(array, device_id, None) self.delete_resource(array, SLOPROVISIONING, "volume", device_id) except Exception as e: - LOG.warning('Delete volume failed with %(e)s.', {'e': e}) + LOG.warning('Delete volume %(dev)s failed with %(e)s.', + {'dev': device_id, 'e': e}) raise else: # Pre-Foxtail, deallocation and delete are separate calls @@ -1569,12 +1589,14 @@ class PowerMaxRest(object): self._modify_volume(array, device_id, payload) pass except Exception as e: - LOG.warning('Deallocate volume failed with %(e)s.' - 'Attempting delete.', {'e': e}) + LOG.warning('Deallocate volume %(dev)s failed with %(e)s.' + 'Attempting delete.', + {'dev': device_id, 'e': e}) # Try to delete the volume if deallocate failed. self.delete_resource(array, SLOPROVISIONING, "volume", device_id) + @retry(retry_exc_tuple, interval=2, retries=3) def find_mv_connections_for_vol(self, array, maskingview, device_id): """Find the host_lun_id for a volume in a masking view. @@ -1595,16 +1617,35 @@ class PowerMaxRest(object): 'for %(mv)s.', {'mv': maskingview}) else: try: - host_lun_id = ( - connection_info[ - 'maskingViewConnection'][0]['host_lun_address']) - host_lun_id = int(host_lun_id, 16) + masking_view_conn = connection_info.get( + 'maskingViewConnection') + if masking_view_conn and isinstance( + masking_view_conn, list): + host_lun_id = masking_view_conn[0].get( + 'host_lun_address') + if host_lun_id: + host_lun_id = int(host_lun_id, 16) + else: + exception_message = ( + _('Unable to get host_lun_address for ' + 'device %(dev)s on masking view %(mv)s. ' + 'Retrying...') + % {'dev': device_id, 'mv': maskingview}) + LOG.warning(exception_message) + raise exception.VolumeBackendAPIException( + message=exception_message) except Exception as e: - LOG.error("Unable to retrieve connection information " - "for volume %(vol)s in masking view %(mv)s. " - "Exception received: %(e)s.", - {'vol': device_id, 'mv': maskingview, - 'e': e}) + exception_message = ( + _("Unable to retrieve connection information " + "for volume %(vol)s in masking view %(mv)s. " + "Exception received: %(e)s. Retrying...") + % {'vol': device_id, 'mv': maskingview, + 'e': e}) + LOG.warning(exception_message) + raise exception.VolumeBackendAPIException( + message=exception_message) + LOG.debug("The hostlunid is %(hli)s for %(dev)s.", + {'hli': host_lun_id, 'dev': device_id}) return host_lun_id def get_storage_groups_from_volume(self, array, device_id): @@ -1615,7 +1656,16 @@ class PowerMaxRest(object): :returns: storagegroup_list """ sg_list = [] + if not device_id: + return sg_list vol = self.get_volume(array, device_id) + if vol and isinstance(vol, list): + LOG.warning( + "Device id %(dev_id)s has brought back " + "multiple volume objects.", + {'vol_name': device_id}) + return sg_list + if vol and vol.get('storageGroupId'): sg_list = vol['storageGroupId'] num_storage_groups = len(sg_list) @@ -1647,13 +1697,19 @@ class PowerMaxRest(object): """ device_id = None params = {"volume_identifier": volume_name} - - volume_list = self.get_volume_list(array, params) - if not volume_list: - LOG.debug("Cannot find record for volume %(volumeId)s.", - {'volumeId': volume_name}) + device_list = self.get_volume_list(array, params) + if not device_list: + LOG.debug("Cannot find record for volume %(vol_name)s.", + {'vol_name': volume_name}) else: - device_id = volume_list[0] + LOG.debug("The device id list is %(dev_list)s for %(vol_name)s.", + {'dev_list': device_list, + 'vol_name': volume_name}) + device_id = device_list[0] if len(device_list) == 1 else ( + device_list) + if isinstance(device_id, list): + LOG.warning("More than one devices returned for %(vol_name)s.", + {'vol_name': volume_name}) return device_id def find_volume_identifier(self, array, device_id): @@ -1664,7 +1720,7 @@ class PowerMaxRest(object): :returns: the volume identifier -- string """ vol = self.get_volume(array, device_id) - return vol['volume_identifier'] + return vol.get('volume_identifier') if vol else None def get_size_of_device_on_array(self, array, device_id): """Get the size of the volume from the array.