[pve-devel] [PATCH proxmox-offline-mirror] Add possibility to create named snapshots

[Fabian Grünbichler]

On May 8, 2025 5:17 pm, Laurențiu Leahu-Vlăducu wrote:
> Thanks for your feedback! Some answers inline.
> 
> On 08.05.25 11:14, Fabian Grünbichler wrote:
>> On May 6, 2025 11:09 am, Laurențiu Leahu-Vlăducu wrote:
>>> @@ -807,6 +801,42 @@ pub fn create_snapshot(
>>>           total: Progress::new(),
>>>       };
>>>   
>>> +    let mut config: ParsedMirrorConfig = config.try_into()?;
>>> +    config.auth = auth;
>>> +
>>> +    let snapshot_relative_path = snapshot.to_string();
>>> +    let snapshot_relative_path = Path::new(&snapshot_relative_path);
>>> +    let snapshot_absolute_path = config.pool.get_path(snapshot_relative_path);
>>> +
>>> +    if snapshot_absolute_path.is_ok_and(|path| path.exists()) {
>>> +        if dry_run {
>>> +            let msg = "WARNING: snapshot {snapshot} already exists! Continuing due to dry run...";
>>> +            eprintln!("{msg}");
>>> +            progress.warnings.push(msg.into());
>>> +        } else {
>>> +            bail!("Snapshot {snapshot} already exists!");
>>> +        }
>>> +    }
>>> +
>>> +    let lockfile_path = config
>>> +        .pool
>>> +        .get_path(Path::new(&format!(".{snapshot}.lock")));
>> 
>> what does this new lock protect against? what is its scope? note that a
>> pool can be shared between mirrors..
> 
> Until now, it was only possible to create snapshots containing a 
> timestamp, meaning that unless you were so fast to create multiple 
> snapshots per second, it was impossible to begin creating two snapshots 
> at once. However, you can now theoretically try to create two snapshots 
> with the same name at the same time (e.g. over two SSH connections) - 
> that is, creating a second snapshot before the first one finished 
> creating. The lockfile protects against such an issue.

shouldn't that be covered by checking for/creating the .tmp path while
holding the regular pool lock?

if we switch to "label" operations, then the modifying operations for
those could just be implemented on the lock guard like we do for the
rest, and all this is moot anyway ;)

but maybe we should add the creation of the `prefix` dir to those
operations as well, including a check for it not already existing? right
now it is implicitly created by link_file_do when fetching the release
file..