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

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

On September 12, 2024 4:33 pm, 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.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 2:
> - no changes
> 
>  src/server/push.rs | 65 ++++++++++++++++++++--------------------------
>  1 file changed, 28 insertions(+), 37 deletions(-)
> 
> diff --git a/src/server/push.rs b/src/server/push.rs
> index cfbb88728..dbface907 100644
> --- a/src/server/push.rs
> +++ b/src/server/push.rs
> @@ -11,9 +11,10 @@ use tokio_stream::wrappers::ReceiverStream;
>  use tracing::info;
>  
>  use pbs_api_types::{
> -    print_store_and_ns, Authid, BackupDir, BackupGroup, BackupNamespace, CryptMode, GroupFilter,
> -    GroupListItem, NamespaceListItem, Operation, RateLimitConfig, Remote, SnapshotListItem,
> -    PRIV_REMOTE_DATASTORE_BACKUP, PRIV_REMOTE_DATASTORE_MODIFY, PRIV_REMOTE_DATASTORE_PRUNE,
> +    print_store_and_ns, 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, UploadOptions};
>  use pbs_config::CachedUserInfo;
> @@ -228,7 +229,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}"))?;
>  
> @@ -246,9 +247,11 @@ async fn remove_target_group(
>          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)?;

what about older target servers that return Value::Null here? from a
quick glance, nothing else requires upgrading the target server to
"enable" push support, so this should probably gracefully handle that
combination as well..

>  
> -    Ok(())
> +    Ok(delete_stats)
>  }
>  
>  // Check if the namespace is already present on the target, create it otherwise
> @@ -451,38 +454,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 {
> +                        info!(
> +                            "kept {protected_count} protected snapshots of group '{target_group}'",
> +                            protected_count = delete_stats.protected_snapshots(),
> +                        );

should this be a warning? this kind of breaks the expectations of
syncing after all..

and wouldn't we also need a similar change for removing namespaces?

> +                    }
> +                    stats.add(SyncStats::from(RemovedVanishedStats {
> +                        snapshots: delete_stats.removed_snapshots(),
> +                        groups: delete_stats.removed_groups(),
> +                        namespaces: 0,
> +                    }));
> +                }
> +                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.2
> 
> 
> 
> _______________________________________________
> 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