[pbs-devel] applied: [PATCH v2 proxmox-backup] server: push: add error context to api calls and priv checks
Fabian Grünbichler
f.gruenbichler at proxmox.com
Fri Nov 22 14:14:08 CET 2024
with cargo fmt + a missing `format!` in one context folded in
Quoting Christian Ebner (2024-11-22 13:26:04)
> Add an anyhow context to errors and display the full error context
> in the log output. Further, make it clear which errors stem from api
> calls by explicitly mentioning this in the context message.
>
> This also fixes incorrect error handling by placing the error context
> on the api result instead of the serde deserialization error for
> cases this was handled incorrectly.
>
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
> changes since version 1:
> - fix incorrect api result error handling
> - use anyhow context
> - show full error context in log output
>
> src/server/push.rs | 88 +++++++++++++++++++++++++++++-----------------
> 1 file changed, 56 insertions(+), 32 deletions(-)
>
> diff --git a/src/server/push.rs b/src/server/push.rs
> index 4c489531c..1914cad75 100644
> --- a/src/server/push.rs
> +++ b/src/server/push.rs
> @@ -3,7 +3,7 @@
> use std::collections::HashSet;
> use std::sync::{Arc, Mutex};
>
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{bail, Context, Error};
> use futures::stream::{self, StreamExt, TryStreamExt};
> use tokio::sync::mpsc;
> use tokio_stream::wrappers::ReceiverStream;
> @@ -180,7 +180,12 @@ fn check_ns_remote_datastore_privs(
> // Fetch the list of namespaces found on target
> async fn fetch_target_namespaces(params: &PushParameters) -> Result<Vec<BackupNamespace>, Error> {
> let api_path = params.target.datastore_api_path("namespace");
> - let mut result = params.target.client.get(&api_path, None).await?;
> + let mut result = params
> + .target
> + .client
> + .get(&api_path, None)
> + .await
> + .context("Fetching remote namespaces failed, remote returned error")?;
> let namespaces: Vec<NamespaceListItem> = serde_json::from_value(result["data"].take())?;
> let mut namespaces: Vec<BackupNamespace> = namespaces
> .into_iter()
> @@ -201,7 +206,7 @@ async fn remove_target_namespace(
> }
>
> check_ns_remote_datastore_privs(params, target_namespace, PRIV_REMOTE_DATASTORE_MODIFY)
> - .map_err(|err| format_err!("Pruning remote datastore namespaces not allowed - {err}"))?;
> + .context("Pruning remote datastore namespaces not allowed")?;
>
> let api_path = params.target.datastore_api_path("namespace");
>
> @@ -214,13 +219,17 @@ async fn remove_target_namespace(
> args["error-on-protected"] = serde_json::to_value(false)?;
> }
>
> - let mut result = params.target.client.delete(&api_path, Some(args)).await?;
> + let mut result = params
> + .target
> + .client
> + .delete(&api_path, Some(args))
> + .await
> + .context(format!(
> + "Failed to remove remote namespace {target_namespace}, remote returned error"
> + ))?;
>
> if params.target.supports_prune_delete_stats {
> - let data = result["data"].take();
> - serde_json::from_value(data).map_err(|err| {
> - format_err!("removing target namespace {target_namespace} failed - {err}")
> - })
> + Ok(serde_json::from_value(result["data"].take())?)
> } else {
> Ok(BackupGroupDeleteStats::default())
> }
> @@ -235,7 +244,13 @@ async fn fetch_target_groups(
> let api_path = params.target.datastore_api_path("groups");
> let args = Some(serde_json::json!({ "ns": target_namespace.name() }));
>
> - let mut result = params.target.client.get(&api_path, args).await?;
> + let mut result = params
> + .target
> + .client
> + .get(&api_path, args)
> + .await
> + .context("Failed to fetch remote groups, remote returned error")?;
> +
> let groups: Vec<GroupListItem> = serde_json::from_value(result["data"].take())?;
>
> let (mut owned, not_owned) = groups.into_iter().fold(
> @@ -262,7 +277,7 @@ async fn remove_target_group(
> backup_group: &BackupGroup,
> ) -> Result<BackupGroupDeleteStats, Error> {
> check_ns_remote_datastore_privs(params, target_namespace, PRIV_REMOTE_DATASTORE_PRUNE)
> - .map_err(|err| format_err!("Pruning remote datastore contents not allowed - {err}"))?;
> + .context("Pruning remote datastore contents not allowed")?;
>
> let api_path = params.target.datastore_api_path("groups");
>
> @@ -273,12 +288,11 @@ async fn remove_target_group(
> args["error-on-protected"] = serde_json::to_value(false)?;
> }
>
> - let mut result = params.target.client.delete(&api_path, Some(args)).await?;
> + let mut result = params.target.client.delete(&api_path, Some(args)).await
> + .context(format!("Failed to remove remote group {backup_group}, remote returned error"))?;
>
> if params.target.supports_prune_delete_stats {
> - let data = result["data"].take();
> - serde_json::from_value(data)
> - .map_err(|err| format_err!("removing target group {backup_group} failed - {err}"))
> + Ok(serde_json::from_value(result["data"].take())?)
> } else {
> Ok(BackupGroupDeleteStats::default())
> }
> @@ -295,7 +309,7 @@ async fn check_or_create_target_namespace(
> // Sub-namespaces have to be created by creating parent components first.
>
> check_ns_remote_datastore_privs(params, target_namespace, PRIV_REMOTE_DATASTORE_MODIFY)
> - .map_err(|err| format_err!("Creating remote namespace not allowed - {err}"))?;
> + .context("Creating remote namespace not allowed")?;
>
> let mut parent = BackupNamespace::root();
> for component in target_namespace.components() {
> @@ -310,12 +324,12 @@ async fn check_or_create_target_namespace(
> if !parent.is_root() {
> args["parent"] = serde_json::to_value(parent.clone())?;
> }
> - match params.target.client.post(&api_path, Some(args)).await {
> - Ok(_) => info!("Successfully created new namespace {current} on remote"),
> - Err(err) => {
> - bail!("Remote creation of namespace {current} failed, remote returned: {err}")
> - }
> - }
> +
> + params.target.client.post(&api_path, Some(args)).await
> + .context("Creation of remote namespace {current} failed, remote returned error")?;
> +
> + info!("Successfully created new namespace {current} on remote");
> +
> existing_target_namespaces.push(current.clone());
> parent = current;
> }
> @@ -377,7 +391,7 @@ pub(crate) async fn push_store(mut params: PushParameters) -> Result<SyncStats,
> )
> .await
> {
> - warn!("Encountered error: {err}");
> + warn!("Encountered error: {err:#}");
> warn!("Failed to sync {source_store_and_ns} into {target_store_and_ns}!");
> errors = true;
> continue;
> @@ -404,7 +418,7 @@ pub(crate) async fn push_store(mut params: PushParameters) -> Result<SyncStats,
> }
> Err(err) => {
> errors = true;
> - info!("Encountered errors: {err}");
> + info!("Encountered errors: {err:#}");
> info!("Failed to sync {source_store_and_ns} into {target_store_and_ns}!");
> }
> }
> @@ -453,7 +467,7 @@ pub(crate) async fn push_store(mut params: PushParameters) -> Result<SyncStats,
> }
> }
> Err(err) => {
> - warn!("Encountered errors: {err}");
> + warn!("Encountered errors: {err:#}");
> warn!("Failed to remove vanished namespace {target_namespace} from remote!");
> continue;
> }
> @@ -483,7 +497,7 @@ pub(crate) async fn push_namespace(
> let target_namespace = params.map_to_target(namespace)?;
> // Check if user is allowed to perform backups on remote datastore
> check_ns_remote_datastore_privs(params, &target_namespace, PRIV_REMOTE_DATASTORE_BACKUP)
> - .map_err(|err| format_err!("Pushing to remote namespace not allowed - {err}"))?;
> + .context("Pushing to remote namespace not allowed")?;
>
> let mut list: Vec<BackupGroup> = params
> .source
> @@ -529,7 +543,7 @@ pub(crate) async fn push_namespace(
> match push_group(params, namespace, &group, &mut progress).await {
> Ok(sync_stats) => stats.add(sync_stats),
> Err(err) => {
> - warn!("Encountered errors: {err}");
> + warn!("Encountered errors: {err:#}");
> warn!("Failed to push group {group} to remote!");
> errors = true;
> }
> @@ -562,7 +576,7 @@ pub(crate) async fn push_namespace(
> }));
> }
> Err(err) => {
> - warn!("Encountered errors: {err}");
> + warn!("Encountered errors: {err:#}");
> warn!("Failed to remove vanished group {target_group} from remote!");
> errors = true;
> continue;
> @@ -584,7 +598,12 @@ async fn fetch_target_snapshots(
> if !target_namespace.is_root() {
> args["ns"] = serde_json::to_value(target_namespace)?;
> }
> - let mut result = params.target.client.get(&api_path, Some(args)).await?;
> + let mut result = params
> + .target
> + .client
> + .get(&api_path, Some(args))
> + .await
> + .context("Failed to fetch remote snapshots, remote returned error")?;
> let snapshots: Vec<SnapshotListItem> = serde_json::from_value(result["data"].take())?;
>
> Ok(snapshots)
> @@ -596,14 +615,19 @@ async fn forget_target_snapshot(
> snapshot: &BackupDir,
> ) -> Result<(), Error> {
> check_ns_remote_datastore_privs(params, target_namespace, PRIV_REMOTE_DATASTORE_PRUNE)
> - .map_err(|err| format_err!("Pruning remote datastore contents not allowed - {err}"))?;
> + .context("Pruning remote datastore contents not allowed")?;
>
> let api_path = params.target.datastore_api_path("snapshots");
> let mut args = serde_json::to_value(snapshot)?;
> if !target_namespace.is_root() {
> args["ns"] = serde_json::to_value(target_namespace)?;
> }
> - params.target.client.delete(&api_path, Some(args)).await?;
> + params
> + .target
> + .client
> + .delete(&api_path, Some(args))
> + .await
> + .context("Failed to remove remote snapshot, remote returned error")?;
>
> Ok(())
> }
> @@ -709,7 +733,7 @@ pub(crate) async fn push_group(
> );
> }
> Err(err) => {
> - warn!("Encountered errors: {err}");
> + warn!("Encountered errors: {err:#}");
> warn!(
> "Failed to remove vanished snapshot {name} from remote!",
> name = snapshot.backup
> @@ -754,7 +778,7 @@ pub(crate) async fn push_snapshot(
> Ok((manifest, _raw_size)) => manifest,
> Err(err) => {
> // No manifest in snapshot or failed to read, warn and skip
> - log::warn!("Encountered errors: {err}");
> + log::warn!("Encountered errors: {err:#}");
> log::warn!("Failed to load manifest for '{snapshot}'!");
> return Ok(stats);
> }
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
More information about the pbs-devel
mailing list