Re: [pbs-devel] [PATCH proxmox-backup 3/4] server: push: add error context to all target api calls

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

Quoting Christian Ebner (2024-11-21 16:43:36)
> Make it clear from the context that these error messages stem from
> the response of an api call rather than a local error.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  src/server/push.rs | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/src/server/push.rs b/src/server/push.rs
> index 86cef5520..fe2e11220 100644
> --- a/src/server/push.rs
> +++ b/src/server/push.rs
> @@ -219,7 +219,9 @@ async fn remove_target_namespace(
>      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}")
> +            format_err!(
> +                "Failed to remove remote namespace {target_namespace}, remote returned: {err}"
> +            )

this is attached to the wrong error - it should be attached to the client.delete call right above..

this here should instead add the context that we failed to parse the returned
value (which should never happen, that means we missed some API breakage..)

>          })
>      } else {
>          Ok(BackupGroupDeleteStats::default())
> @@ -236,7 +238,8 @@ async fn fetch_target_groups(
>      let args = Some(serde_json::json!({ "ns": target_namespace.name() }));
>  
>      let mut result = params.target.client.get(&api_path, args).await?;
> -    let groups: Vec<GroupListItem> = serde_json::from_value(result["data"].take())?;
> +    let groups: Vec<GroupListItem> = serde_json::from_value(result["data"].take())
> +        .map_err(|err| format_err!("Failed to fetch remote groups, remote returned: {err}"))?;

same here, just with get instead of delete ;)

>  
>      let (mut owned, not_owned) = groups.into_iter().fold(
>          (Vec::new(), HashSet::new()),
> @@ -277,8 +280,9 @@ async fn remove_target_group(
>  
>      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}"))
> +        serde_json::from_value(data).map_err(|err| {
> +            format_err!("Failed to remove remote group {backup_group}, remote returned: {err}")
> +        })

here as well

>      } else {
>          Ok(BackupGroupDeleteStats::default())
>      }
> @@ -313,7 +317,7 @@ async fn check_or_create_target_namespace(
>              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}")
> +                    bail!("Creation of remote namespace {current} failed, remote returned: {err}")
>                  }
>              }
>              existing_target_namespaces.push(current.clone());
> @@ -585,7 +589,8 @@ async fn fetch_target_snapshots(
>          args["ns"] = serde_json::to_value(target_namespace)?;
>      }
>      let mut result = params.target.client.get(&api_path, Some(args)).await?;
> -    let snapshots: Vec<SnapshotListItem> = serde_json::from_value(result["data"].take())?;
> +    let snapshots: Vec<SnapshotListItem> = serde_json::from_value(result["data"].take())
> +        .map_err(|err| format_err!("Failed to fetch remote snapshots, remote returned: {err}"))?;

here as well

>  
>      Ok(snapshots)
>  }
> @@ -603,7 +608,12 @@ async fn forget_target_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
> +        .map_err(|err| format_err!("Failed to remove remote snapshot, remote returned: {err}"))?;

this should probably be just "Request to remote returned {err}", since the call
site already logs the snapshot name and the fact that this is removal failing
;)

>  
>      Ok(())
>  }
> -- 
> 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