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

[Christian Ebner]

On 10/15/24 09:44, Fabian Grünbichler wrote:
> 
>> Christian Ebner <c.ebner at proxmox.com> hat am 15.10.2024 09:30 CEST geschrieben:
>>
>>   
>> 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 at 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...
> 
> hmm, this is a bit tricky.
> 
> for pull, we just log and ignore protected snapshots when removing a vanished group. we also log and ignore any other errors at that point though (just without accounting for removed vs. protected).
> 
> but for push, we require the opt-in feature to know that removing the group failed because of a protected snapshot. I guess we could try with the new parameter, if that fails (with a parameter error?), retry without, and if that fails as well, just log whatever error the server returned hoping it's meaningful? or we could make it more robust by querying the remote version at the start of the sync and base the decision on that..

Ah, yes! Fetching the version and use that sounds like a reasonable way 
to go here. Although, there can still be leftovers on error, but this 
can at least be meaningfully logged.