[pbs-devel] applied: [PATCH v2 proxmox-backup] server: push: add error context to api calls and priv checks

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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@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@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel