From: Christian Ebner <c.ebner@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: [pbs-devel] [PATCH v2 proxmox-backup] server: push: add error context to api calls and priv checks
Date: Fri, 22 Nov 2024 13:26:04 +0100 [thread overview]
Message-ID: <20241122122604.335901-1-c.ebner@proxmox.com> (raw)
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
next reply other threads:[~2024-11-22 12:26 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-22 12:26 Christian Ebner [this message]
2024-11-22 13:14 ` [pbs-devel] applied: " Fabian Grünbichler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241122122604.335901-1-c.ebner@proxmox.com \
--to=c.ebner@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox