Re: [PATCH proxmox-backup] api: backup: cleanup backup group created by benchmark

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 04.05.26 um 10:22 schrieb Fabian Grünbichler:
>> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
>> index 86ec49487..8848ca99c 100644
>> --- a/src/api2/backup/mod.rs
>> +++ b/src/api2/backup/mod.rs
>> @@ -288,9 +288,14 @@ fn upgrade_to_backup_protocol(
>>                  if benchmark {
>>                      env.log("benchmark finished successfully");
>>                      proxmox_async::runtime::block_in_place(|| {
>> -                        env.datastore.remove_backup_dir(
>> -                            env.backup_dir.backup_ns(),
>> -                            env.backup_dir.as_ref(),
>> +                        let datastore = env.datastore.clone();
>> +                        let namespace = env.backup_dir.backup_ns().clone();
>> +                        let snapshot = env.backup_dir.dir().clone();
>> +                        // draps all locks
> 
> nit: `draps` ;)

That I fixed.

> 
>> +                        drop(env);
>> +                        datastore.remove_backup_dir(
>> +                            &namespace,
>> +                            &snapshot,
>>                              true,
>>                          )
> 
> doesn't this also affect the "cleanup-on-error" paths a few lines below
> this?
> 
> dropping the full env is also a bit problematic because it opens up a
> race condition if there are back-to-back benchmarks (or backups):
> - benchmark starts
> - benchmark is finished, drops env
> - next benchmark starts and locks group and previous "snapshot"
> - cleanup fails to obtain lock(s) and doesn't run
> 
> for benchmarks that is not so bad, but for backups it would leave
> half-written backup snapshots around (until they are cleaned up by other
> means?).
> 
> ideally, we would not drop the locks here but just run the cleanup using
> the locks we already have, which is what "force" is doing.
> 
> we currently only set force
> - for the three calls here in the backup env
> - when cleaning up a newly created snapshot as part of pull error
>   handling
> 
> in all those case we are holding an exclusive lock on the group and on
> the snapshot already. so we could just skip the group locking as well
> when force is set? (ideally we'd find a way to actually encode this in
> the signature, e.g. by replacing `force` with references to the lock
> guards?)
> 

Argh, missed your reply before pushing this out, sorry. Will address in
a follow-up with your proposed approach - extend `force` to also skip the
group lock and revert the env-drop on top, so all four force=true sites
use the same approach.

The signature refactor (lock-guard references instead of the bool) makes
sense too, but skipping that for now to get the more minimal fix to the
repo.