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

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

On 10/11/24 11:32, Fabian Grünbichler wrote:
> 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..

Since this requires now to set an additional `ignore-protected` flag on 
the api endpoint in order to allow to opt-in (see response to previous 
patch) to not return with error when deletion failed, this cannot be 
handled but will rather fail.

It would be possible to retry without the opt-in flag on failure, but 
then again might fail on protected snapshots. Further, without the 
response body, the statistics will be messed up (since empty).

So not sure, should the remove vanished simply be considered 
incompatible with older version or retried until we fail/succeed for 
good? And live with the missing stats, possible leftover snapshots, ecc...

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

OK, will be a warning in the next version of the patches.

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

Right, that was indeed missing and will be added as well.

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



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