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

[Christian Ebner <c.ebner@proxmox.com>]

On 11/22/24 10:01, Fabian Grünbichler wrote:
> 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..

Oof, correct, will fix that. Thanks for catching this!

> 
> 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