From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 227C11FF166 for ; Fri, 25 Oct 2024 12:18:02 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4AF581D9A8; Fri, 25 Oct 2024 12:18:02 +0200 (CEST) Date: Fri, 25 Oct 2024 12:17:55 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20241018084242.144010-1-c.ebner@proxmox.com> <20241018084242.144010-32-c.ebner@proxmox.com> In-Reply-To: <20241018084242.144010-32-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1729851453.pzm55i1ley.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH v5 proxmox-backup 31/31] server: sync job: use delete stats provided by the api X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On October 18, 2024 10:42 am, Christian Ebner wrote: > Use the API exposed additional delete statistics to generate the > task log output for sync jobs in push direction instead of fetching the > contents before and after deleting. > > Detect older api versions of the target server for fallback. > > Signed-off-by: Christian Ebner > --- > changes since version 4: > - no changes > > changes since version 3: > - fetch target api version to remain backwards compatible > > src/server/push.rs | 126 +++++++++++++++++++++++++++++---------------- > 1 file changed, 82 insertions(+), 44 deletions(-) > > diff --git a/src/server/push.rs b/src/server/push.rs > index bf6045214..a915b6964 100644 > --- a/src/server/push.rs > +++ b/src/server/push.rs > @@ -8,11 +8,12 @@ use anyhow::{bail, format_err, Error}; > use futures::stream::{self, StreamExt, TryStreamExt}; > use tokio::sync::mpsc; > use tokio_stream::wrappers::ReceiverStream; > -use tracing::info; > +use tracing::{info, warn}; > > use pbs_api_types::{ > - print_store_and_ns, Authid, BackupDir, BackupGroup, BackupNamespace, CryptMode, GroupFilter, > - GroupListItem, NamespaceListItem, Operation, RateLimitConfig, Remote, SnapshotListItem, > + print_store_and_ns, ApiVersion, ApiVersionInfo, Authid, BackupDir, BackupGroup, > + BackupGroupDeleteStats, BackupNamespace, CryptMode, GroupFilter, GroupListItem, > + NamespaceListItem, Operation, RateLimitConfig, Remote, SnapshotListItem, > PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_MODIFY, PRIV_REMOTE_DATASTORE_PRUNE, > }; > use pbs_client::{BackupRepository, BackupWriter, HttpClient, MergedChunkInfo, UploadOptions}; > @@ -41,6 +42,8 @@ pub(crate) struct PushTarget { > ns: BackupNamespace, > // Http client to connect to remote > client: HttpClient, > + // Api version reported by the target > + api_version: Option, > } > > /// Parameters for a push operation > @@ -106,6 +109,7 @@ impl PushParameters { > repo, > ns: remote_ns, > client, > + api_version: None, > }; > let group_filter = group_filter.unwrap_or_default(); > > @@ -124,6 +128,17 @@ impl PushParameters { > fn map_to_target(&self, namespace: &BackupNamespace) -> Result { > namespace.map_prefix(&self.source.ns, &self.target.ns) > } > + > + // Fetch and set the api version info for the target > + pub(crate) async fn fetch_target_api_version_info(&mut self) -> Result<(), Error> { > + let api_path = "api2/json/version"; > + let mut result = self.target.client.get(api_path, None).await?; > + let data = result["data"].take(); > + let version_info: ApiVersionInfo = serde_json::from_value(data)?; > + self.target.api_version = Some(ApiVersion::try_from(version_info)?); if the expectation is that this always works, should we do it when initializing the PushParameters and not wrap it in an Option? > + > + Ok(()) > + } > } > > // Check if the job user given in the push parameters has the provided privs on the remote > @@ -167,7 +182,7 @@ async fn fetch_target_namespaces(params: &PushParameters) -> Result async fn remove_target_namespace( > params: &PushParameters, > namespace: &BackupNamespace, > -) -> Result<(), Error> { > +) -> Result { > if namespace.is_root() { > bail!("cannot remove root namespace from target"); > } > @@ -181,14 +196,23 @@ async fn remove_target_namespace( > ); > > let target_ns = params.map_to_target(namespace)?; > - let args = serde_json::json!({ > + let mut args = serde_json::json!({ > "ns": target_ns.name(), > "delete-groups": true, > }); > > - params.target.client.delete(&api_path, Some(args)).await?; > + if let Some(version) = ¶ms.target.api_version { > + if version.is_min_required(ApiVersion::new(3, 2, 8)) { missed a version already ;) might make sense to either move this up as a constant, or even add a sort of supports_feature(..) to ApiVersion ? in any case, something like this should be called out somewhere prominently so that it can be adapted/bumped if need be when applying > + args["ignore-protected"] = serde_json::to_value(true)?; > + } > + } > > - Ok(()) > + let mut result = params.target.client.delete(&api_path, Some(args)).await?; > + let data = result["data"].take(); > + let delete_stats: BackupGroupDeleteStats = > + serde_json::from_value(data).unwrap_or_else(|_| BackupGroupDeleteStats::default()); and this here should only be done if the ApiVersion is too old, else it masks deserialization problems.. > + > + Ok(delete_stats) > } > > // Fetch the list of groups found on target in given namespace > @@ -229,7 +253,7 @@ async fn remove_target_group( > params: &PushParameters, > namespace: &BackupNamespace, > backup_group: &BackupGroup, > -) -> Result<(), Error> { > +) -> Result { > check_ns_remote_datastore_privs(params, namespace, PRIV_REMOTE_DATASTORE_PRUNE) > .map_err(|err| format_err!("Pruning remote datastore contents not allowed - {err}"))?; > > @@ -242,14 +266,23 @@ async fn remove_target_group( > "backup-id": backup_group.id, > "backup-type": backup_group.ty, > }); > + > + if let Some(version) = ¶ms.target.api_version { > + if version.is_min_required(ApiVersion::new(3, 2, 8)) { same here > + args["ignore-protected"] = serde_json::to_value(true)?; > + } > + } > if !namespace.is_root() { > let target_ns = params.map_to_target(namespace)?; > args["ns"] = serde_json::to_value(target_ns.name())?; > } > > - params.target.client.delete(&api_path, Some(args)).await?; > + let mut result = params.target.client.delete(&api_path, Some(args)).await?; > + let data = result["data"].take(); > + let delete_stats: BackupGroupDeleteStats = > + serde_json::from_value(data).unwrap_or_else(|_| BackupGroupDeleteStats::default()); and here > > - Ok(()) > + Ok(delete_stats) > } > > // Check if the namespace is already present on the target, create it otherwise > @@ -298,6 +331,7 @@ async fn check_or_create_target_namespace( > /// Push contents of source datastore matched by given push parameters to target. > pub(crate) async fn push_store(mut params: PushParameters) -> Result { > let mut errors = false; > + params.fetch_target_api_version_info().await?; > > // Generate list of source namespaces to push to target, limited by max-depth > let mut namespaces = params.source.list_namespaces(&mut params.max_depth).await?; > @@ -365,9 +399,25 @@ pub(crate) async fn push_store(mut params: PushParameters) -> Result if synced_namespaces.contains(&target_namespace) { > continue; > } > - if let Err(err) = remove_target_namespace(¶ms, &target_namespace).await { > - info!("failed to remove vanished namespace {target_namespace} - {err}"); > - continue; > + match remove_target_namespace(¶ms, &target_namespace).await { > + Ok(delete_stats) => { > + stats.add(SyncStats::from(RemovedVanishedStats { > + snapshots: delete_stats.removed_snapshots(), > + groups: delete_stats.removed_groups(), > + namespaces: 1, > + })); should we maybe print a warning once at the start of sync if the ApiVersion is too old, so that users know that these stats are incomplete? because in that case if no snapshots/groups were protected, we'll get a default BackupGroupDeleteStats here, so we actually lack the information about removed snapshots and groups.. > + if delete_stats.protected_snapshots() > 0 { > + warn!( > + "kept {protected_count} protected snapshots of namespace '{target_namespace}'", > + protected_count = delete_stats.protected_snapshots(), > + ); > + continue; > + } > + } > + Err(err) => { > + warn!("failed to remove vanished namespace {target_namespace} - {err}"); > + continue; > + } > } > info!("removed vanished namespace {target_namespace}"); > } > @@ -449,38 +499,26 @@ pub(crate) async fn push_namespace( > > info!("delete vanished group '{target_group}'"); > > - let count_before = match fetch_target_groups(params, namespace).await { > - Ok(snapshots) => snapshots.len(), > - Err(_err) => 0, // ignore errors > - }; > - > - if let Err(err) = remove_target_group(params, namespace, &target_group).await { > - info!("{err}"); > - errors = true; > - continue; > - } > - > - let mut count_after = match fetch_target_groups(params, namespace).await { > - Ok(snapshots) => snapshots.len(), > - Err(_err) => 0, // ignore errors > - }; > - > - let deleted_groups = if count_after > 0 { > - info!("kept some protected snapshots of group '{target_group}'"); > - 0 > - } else { > - 1 > - }; > - > - if count_after > count_before { > - count_after = count_before; > + match remove_target_group(params, namespace, &target_group).await { > + Ok(delete_stats) => { > + if delete_stats.protected_snapshots() > 0 { > + warn!( > + "kept {protected_count} protected snapshots of group '{target_group}'", > + protected_count = delete_stats.protected_snapshots(), > + ); > + } > + stats.add(SyncStats::from(RemovedVanishedStats { > + snapshots: delete_stats.removed_snapshots(), > + groups: delete_stats.removed_groups(), > + namespaces: 0, > + })); same here > + } > + Err(err) => { > + info!("failed to delete vanished group - {err}"); > + errors = true; > + continue; > + } > } > - > - stats.add(SyncStats::from(RemovedVanishedStats { > - snapshots: count_before - count_after, > - groups: deleted_groups, > - namespaces: 0, > - })); > } > } > > -- > 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