Re: [pbs-devel] [PATCH v5 proxmox-backup 31/31] server: sync job: use delete stats provided by the api

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

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 <c.ebner@proxmox.com>
> ---
> 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<ApiVersion>,
>  }
>  
>  /// 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<BackupNamespace, Error> {
>          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<Vec<BackupNa
>  async fn remove_target_namespace(
>      params: &PushParameters,
>      namespace: &BackupNamespace,
> -) -> Result<(), Error> {
> +) -> Result<BackupGroupDeleteStats, Error> {
>      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) = &params.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<BackupGroupDeleteStats, Error> {
>      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) = &params.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<SyncStats, Error> {
>      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<SyncStats,
>              if synced_namespaces.contains(&target_namespace) {
>                  continue;
>              }
> -            if let Err(err) = remove_target_namespace(&params, &target_namespace).await {
> -                info!("failed to remove vanished namespace {target_namespace} - {err}");
> -                continue;
> +            match remove_target_namespace(&params, &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