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 4c3d7f2a407..4b978770f53 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 @@ -672,6 +672,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 1358ac21839..4a89470497e 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 171289e42df..0bb68314f7f 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 @@ -400,7 +400,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') @@ -418,7 +418,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') @@ -1942,12 +1942,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 @@ -3053,6 +3056,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', @@ -3062,7 +3066,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)] @@ -3966,6 +3970,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): @@ -4054,6 +4065,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)}] @@ -4691,3 +4719,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 87da912ff15..8ca91f64c9f 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, @@ -920,6 +922,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') @@ -927,7 +932,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) @@ -1428,3 +1433,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 744b1e754d8..3b89375e918 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 3e4543a1f28..5189a4845f5 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'): @@ -3290,7 +3335,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) @@ -3659,6 +3704,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): @@ -3767,6 +3813,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( @@ -4850,7 +4897,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: @@ -6182,7 +6229,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", @@ -7197,6 +7244,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) @@ -7666,3 +7715,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 4904e564a4c..fc46a3ec55d 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, @@ -767,6 +834,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( @@ -780,6 +854,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.