[pbs-devel] [PATCH proxmox-backup v2] api: make prune-group a real workertask

[Gabriel Goller]

Thanks for the review!
A v2 will be on the mailing list shortly!

> > @@ -960,6 +966,7 @@ pub fn prune(
> >      keep_options: KeepOptions,
> >      store: String,
> >      ns: Option<BackupNamespace>,
> > +    use_task: bool,
>
> Mmh, the number of arguments of this function makes clippy complain
> - would it make sense to use `_param` below and decode the parameter
> instead?
>
> That being said, perhaps we should find a more general solution for
> handling that many arguments at once (the arg-struct thing), as I've
> noticed that this is a recurring pattern at this point.
>
> That's not too relevant for this series otherwise though - if extracting
> the `bool` from `Value` (below) is too awkward, I guess it's fine to
> just disable the lint here (for now).

Removed the `use_task` parameter and extracted it from the `param` one.

> >  
> > -        let msg = format!("{}/{}/{} {}", group.ty, group.id, timestamp, mark,);
> > +            let msg = format!("{}/{}/{} {}", group.ty, group.id, timestamp, mark);
> >  
> > -        task_log!(worker, "{}", msg);
> > +            task_log!(worker, "{}", msg);
>
> While you're touching that code, would be nice to inline variables
> to be formatted into the format strings:
>
> task_log!(worker, "{msg}");

Done!

> > -        if !(dry_run || keep) {
> > -            if let Err(err) = info.backup_dir.destroy(false) {
> > -                task_warn!(
> > -                    worker,
> > -                    "failed to remove dir {:?}: {}",
> > -                    info.backup_dir.relative_path(),
> > -                    err,
> > -                );
> > +            if !(dry_run || keep) {
>
> Early exit on `dry_run` is above, so this condition should just be
> `!keep`.
>

Didn't write this code, but I think it's ok, so I went ahead and made
the change.

> One last thing: Since the *complete* result is logged above (where the worker is spawned),
> maybe we should reconsider what we log here exactly? For the record, this is what
> the output looks like with the patch applied:
>
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]: retention options: --keep-last 1
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]: Starting prune on datastore 'test-pool', root namespace group "vm/100"
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]: vm/100/2023-11-28T13:07:05Z remove
> Mar 07 10:22:29 pbs-dev proxmox-backup-[5636]: pbs-dev proxmox-backup-proxy[5636]: removing backup snapshot "/mnt/datastore/test-pool/vm/100/2023-11-28T13:07:05Z"
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]: vm/100/2023-12-04T08:57:22Z keep
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]: [
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:     PruneResult {
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:         backup_type: Vm,
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:         backup_id: "100",
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:         backup_time: 1701176825,
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:         keep: false,
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:         protected: false,
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:         ns: None,
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:     },
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:     PruneResult {
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:         backup_type: Vm,
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:         backup_id: "100",
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:         backup_time: 1701680242,
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:         keep: true,
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:         protected: false,
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:         ns: None,
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]:     },
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]: ]
> Mar 07 10:22:29 pbs-dev proxmox-backup-proxy[5636]: TASK OK
>
> Perhaps it's better to log whether `result` is `Err` or `Ok` instead?

Yes, I decided to remove this log-statement altogether... We already
print a '<snapshot-name> keep' or '<snapshot-name> remove' for every
snapshot, so the PruneResult is only duplicate information.