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

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

On 10/25/24 12:17, Fabian Grünbichler wrote:
> 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?

Okay, dropped the Option wrapper here and adapted the 
PushParameters::new() method accordingly to always async fetch the 
version. This extra API call even when `remove_vanished` is not set and 
therefore the remote api version is not required does not hurt.

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

Yes, I did follow your suggestion and added a `supports_feature` instead 
of the `is_min_required` method. That is way more ergonomic and allows 
to append new feature strings without issues at certain commits.

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

True, made the unwrap_or_default conditionally here and the other 
mentions below.

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

Yes, added the warning in case of unsupported feature and 
remove_vanished flag set.

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



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel