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

["Laurențiu Leahu-Vlăducu" <l.leahu-vladucu@proxmox.com>]

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:
>> In addition to the existing timed snapshots, it is now possible to
>> create named snapshots. For some use cases, it makes sense to give
>> snapshots a specific name, e.g. to specify the purpose of the snapshot
>> (instead of a timestamp). This can be useful when deploying snapshots
>> created for a certain purpose (for example, further testing).
>>
>> Partially fixes #6252
>>
>> Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
>> ---
>>   src/bin/proxmox-offline-mirror-helper.rs      |  6 +-
>>   src/bin/proxmox_offline_mirror_cmds/mirror.rs | 32 ++++++++---
>>   src/medium.rs                                 |  4 +-
>>   src/mirror.rs                                 | 56 +++++++++++++++----
>>   src/types.rs                                  | 52 +++++++++++++----
>>   5 files changed, 117 insertions(+), 33 deletions(-)
>>
>> diff --git a/src/bin/proxmox-offline-mirror-helper.rs b/src/bin/proxmox-offline-mirror-helper.rs
>> index 7911d52..a00d26d 100644
>> --- a/src/bin/proxmox-offline-mirror-helper.rs
>> +++ b/src/bin/proxmox-offline-mirror-helper.rs
>> @@ -151,8 +151,8 @@ async fn setup(_param: Value) -> Result<(), Error> {
>>                   let selected_mirror = read_selection_from_tty("Select mirror", &mirrors, None)?;
>>                   let snapshots: Vec<(Snapshot, String)> =
>>                       medium::list_snapshots(mountpoint, selected_mirror)?
>> -                        .into_iter()
>> -                        .map(|s| (s, s.to_string()))
>> +                        .iter()
>> +                        .map(|s| (s.clone(), s.to_string()))
> 
> doesn't hurt too much, but could still be avoided by doing to_string()
> first..
> 
>>                           .collect();
>>                   if snapshots.is_empty() {
>>                       println!("Mirror doesn't have any synced snapshots.");
>> @@ -173,7 +173,7 @@ async fn setup(_param: Value) -> Result<(), Error> {
>>                       selected_mirror.to_string(),
>>                       (
>>                           state.mirrors.get(*selected_mirror).unwrap(),
>> -                        **selected_snapshot,
>> +                        (**selected_snapshot).clone(),
>>                       ),
>>                   );
>>               }
>> diff --git a/src/bin/proxmox_offline_mirror_cmds/mirror.rs b/src/bin/proxmox_offline_mirror_cmds/mirror.rs
>> index 31a565e..7eba04d 100644
>> --- a/src/bin/proxmox_offline_mirror_cmds/mirror.rs
>> +++ b/src/bin/proxmox_offline_mirror_cmds/mirror.rs
>> @@ -17,7 +17,7 @@ use proxmox_schema::api;
>>   use proxmox_offline_mirror::{
>>       config::{MirrorConfig, SubscriptionKey},
>>       mirror,
>> -    types::{MIRROR_ID_SCHEMA, Snapshot},
>> +    types::{MIRROR_ID_SCHEMA, NAMED_SNAPSHOT_SCHEMA, Snapshot},
>>   };
>>   
>>   use super::get_config_path;
>> @@ -60,6 +60,10 @@ fn get_subscription_key(
>>               id: {
>>                   schema: MIRROR_ID_SCHEMA,
>>               },
>> +            name: {
>> +                schema: NAMED_SNAPSHOT_SCHEMA,
>> +                optional: true,
>> +            },
>>               "dry-run": {
>>                   type: bool,
>>                   optional: true,
>> @@ -73,6 +77,7 @@ fn get_subscription_key(
>>   async fn create_snapshot(
>>       config: Option<String>,
>>       id: String,
>> +    name: Option<String>,
>>       dry_run: bool,
>>       _param: Value,
>>   ) -> Result<(), Error> {
>> @@ -83,12 +88,12 @@ async fn create_snapshot(
>>   
>>       let subscription = get_subscription_key(&section_config, &config)?;
>>   
>> -    proxmox_offline_mirror::mirror::create_snapshot(
>> -        config,
>> -        &Snapshot::now(),
>> -        subscription,
>> -        dry_run,
>> -    )?;
>> +    let snapshot = match &name {
>> +        Some(name) => Snapshot::with_name(&name),
>> +        None => Snapshot::now(),
>> +    };
>> +
>> +    proxmox_offline_mirror::mirror::create_snapshot(config, &snapshot, subscription, dry_run)?;
> 
> I think it would make more sense to treat the name as an extra "label",
> rather than as a replacement for the timestamp..
> 
> because with the current approach:
> - we lose the information when a snapshot was created, which is
>    important for ordering
> - *anything* that matches the relaxed schema is now treated as snapshot,
>    including temporary dirs for named snapshots
> 
> if we attach the label *after* creating the snapshot (e.g., as a symlink
> pointing at a snapshot dir?), we avoid those issues (and even get
> additional benefits, like moving a label forward, having multiple labels
> for a single snapshot, ..).
> 
> of course, that means handling dangling symlinks, and handling of those
> symlinks in general in some parts..

Sure, this is probably the safer approach in the long run (also because 
of what you mentioned below). I'll send a v2 containing your suggestions 
soon.

> 
>>   
>>       Ok(())
>>   }
>> @@ -101,6 +106,10 @@ async fn create_snapshot(
>>                   optional: true,
>>                   description: "Path to mirroring config file.",
>>               },
>> +            name: {
>> +                schema: NAMED_SNAPSHOT_SCHEMA,
>> +                optional: true,
>> +            },
>>               "dry-run": {
>>                   type: bool,
>>                   optional: true,
>> @@ -114,6 +123,7 @@ async fn create_snapshot(
>>   /// from original repository.
>>   async fn create_snapshots(
>>       config: Option<String>,
>> +    name: Option<String>,
>>       dry_run: bool,
>>       _param: Value,
>>   ) -> Result<(), Error> {
>> @@ -135,9 +145,15 @@ async fn create_snapshots(
>>                   continue;
>>               }
>>           };
>> +
>> +        let snapshot = match &name {
>> +            Some(name) => Snapshot::with_name(&name),
>> +            None => Snapshot::now(),
>> +        };
>> +
>>           let res = proxmox_offline_mirror::mirror::create_snapshot(
>>               mirror,
>> -            &Snapshot::now(),
>> +            &snapshot,
>>               subscription,
>>               dry_run,
> 
> this means that one of the main purposes of this "naming" gets quite
> brittle - if a single mirror fails to be mirrored, the others will have
> that name attached and it's thus "used"?
> 
> if we use the symlink approach, we also avoid this problem -> simply
> snapshot all mirrors, and attach the label at the end if successful (or
> make the behaviour configurable, depending on use case)
> 
>>           );
>> diff --git a/src/medium.rs b/src/medium.rs
>> index bef6cc7..a2e2ee4 100644
>> --- a/src/medium.rs
>> +++ b/src/medium.rs
>> @@ -18,7 +18,7 @@ use crate::{
>>       generate_repo_file_line,
>>       mirror::pool,
>>       pool::Pool,
>> -    types::{Diff, SNAPSHOT_REGEX, Snapshot},
>> +    types::{COMBINED_SNAPSHOT_REGEX, Diff, Snapshot},
>>   };
>>   #[derive(Clone, Debug, Serialize, Deserialize)]
>>   #[serde(rename_all = "kebab-case")]
>> @@ -162,7 +162,7 @@ pub fn list_snapshots(medium_base: &Path, mirror: &str) -> Result<Vec<Snapshot>,
>>       proxmox_sys::fs::scandir(
>>           libc::AT_FDCWD,
>>           &mirror_base,
>> -        &SNAPSHOT_REGEX,
>> +        &COMBINED_SNAPSHOT_REGEX,
>>           |_l2_fd, snapshot, file_type| {
>>               if file_type != nix::dir::Type::Directory {
>>                   return Ok(());
>> diff --git a/src/mirror.rs b/src/mirror.rs
>> index c88f81d..2aef691 100644
>> --- a/src/mirror.rs
>> +++ b/src/mirror.rs
>> @@ -11,14 +11,14 @@ use globset::{Glob, GlobSet, GlobSetBuilder};
>>   use nix::libc;
>>   use proxmox_http::{HttpClient, HttpOptions, ProxyConfig, client::sync::Client};
>>   use proxmox_schema::{ApiType, Schema};
>> -use proxmox_sys::fs::file_get_contents;
>> +use proxmox_sys::fs::{CreateOptions, file_get_contents};
>>   
>>   use crate::{
>>       FetchResult, Progress,
>>       config::{MirrorConfig, SkipConfig, SubscriptionKey, WeakCryptoConfig},
>>       convert_repo_line,
>>       pool::Pool,
>> -    types::{Diff, SNAPSHOT_REGEX, Snapshot},
>> +    types::{COMBINED_SNAPSHOT_REGEX, Diff, Snapshot},
>>   };
>>   
>>   use proxmox_apt::deb822::{
>> @@ -476,7 +476,7 @@ pub fn list_snapshots(config: &MirrorConfig) -> Result<Vec<Snapshot>, Error> {
>>       proxmox_sys::fs::scandir(
>>           libc::AT_FDCWD,
>>           &path,
>> -        &SNAPSHOT_REGEX,
>> +        &COMBINED_SNAPSHOT_REGEX,
>>           |_l2_fd, snapshot, file_type| {
>>               if file_type != nix::dir::Type::Directory {
>>                   return Ok(());
>> @@ -793,12 +793,6 @@ pub fn create_snapshot(
>>           None
>>       };
>>   
>> -    let mut config: ParsedMirrorConfig = config.try_into()?;
>> -    config.auth = auth;
> 
> this is unrelated?

I moved the "progress" variable around (see below) to report a warning 
(instead of an error) in case a snapshot already exists, while doing a 
dry run. I then moved "config" around as well and apparently I added 
them in the reversed order then before. Sorry for the noise.

> 
>> -
>> -    let prefix = format!("{snapshot}.tmp");
>> -    let prefix = Path::new(&prefix);
>> -
>>       let mut progress = MirrorProgress {
>>           warnings: Vec::new(),
>>           skip_count: 0,
>> @@ -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.

> 
>> +
>> +    if let Err(lockfile_err) = lockfile_path {
>> +        bail!("cannot create lockfile for snapshot {snapshot}: {lockfile_err}");
>> +    }
>> +    let lockfile_path = lockfile_path.unwrap();
>> +
>> +    let _snapshot_lock = proxmox_sys::fs::open_file_locked(
>> +        &lockfile_path,
>> +        std::time::Duration::new(10, 0),
>> +        true,
>> +        CreateOptions::default(),
>> +    )?;
>> +
>> +    let prefix = format!("{snapshot}.tmp");
>> +    let prefix = Path::new(&prefix);
> 
> this might also already exist and should be checked..

Good point, thanks!

> 
>> +
>>       let parse_release = |res: FetchResult, name: &str| -> Result<ReleaseFile, Error> {
>>           println!("Parsing {name}..");
>>           let parsed: ReleaseFile = res.data[..].try_into()?;
>> @@ -1078,9 +1108,15 @@ pub fn create_snapshot(
>>       if !dry_run {
>>           println!("\nRotating temp. snapshot in-place: {prefix:?} -> \"{snapshot}\"");
>>           let locked = config.pool.lock()?;
>> -        locked.rename(prefix, Path::new(&format!("{snapshot}")))?;
>> +
>> +        if snapshot_relative_path.exists() {
>> +            bail!("Snapshot {snapshot} already exists!");
>> +        }
> 
> this check is wrong, because it doesn't account for the current working
> directory and the path being relative..

Good catch, I missed that one, thanks!

> 
>> +        locked.rename(prefix, snapshot_relative_path)?;
>>       }
>>   
>> +    std::fs::remove_file(lockfile_path)?;
>> +
>>       Ok(())
>>   }
>>   
>> diff --git a/src/types.rs b/src/types.rs
>> index 10dde65..49dc374 100644
>> --- a/src/types.rs
>> +++ b/src/types.rs
>> @@ -1,6 +1,6 @@
>>   use std::{fmt::Display, path::PathBuf, str::FromStr};
>>   
>> -use anyhow::Error;
>> +use anyhow::{Error, bail};
>>   use proxmox_schema::{ApiStringFormat, Schema, StringSchema, api, const_regex};
>>   use proxmox_serde::{forward_deserialize_to_from_str, forward_serialize_to_display};
>>   use proxmox_time::{epoch_i64, epoch_to_rfc3339_utc, parse_rfc3339};
>> @@ -32,6 +32,13 @@ pub const MEDIA_ID_SCHEMA: Schema = StringSchema::new("Medium name.")
>>       .max_length(32)
>>       .schema();
>>   
>> +/// Schema for named snapshots
>> +pub const NAMED_SNAPSHOT_SCHEMA: Schema = StringSchema::new("Custom name for snapshot.")
>> +    .format(&PROXMOX_SAFE_ID_FORMAT)
>> +    .min_length(3)
>> +    .max_length(32)
>> +    .schema();
>> +
>>   #[rustfmt::skip]
>>   #[macro_export]
>>   macro_rules! PROXMOX_SUBSCRIPTION_KEY_REGEX_STR { () => { r"(?:pom-|pve\d+[a-z]-|pbs[a-z]-|pmg[a-z]-).*" }; }
>> @@ -62,32 +69,51 @@ pub const PROXMOX_SERVER_ID_SCHEMA: Schema = StringSchema::new("Server ID.")
>>   
>>   #[rustfmt::skip]
>>   #[macro_export]
>> -macro_rules! SNAPSHOT_RE { () => (r"[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z") }
>> +macro_rules! TIMED_SNAPSHOT_RE { () => (r"[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z") }
>>   const_regex! {
>> -    pub(crate) SNAPSHOT_REGEX = concat!(r"^", SNAPSHOT_RE!() ,r"$");
>> +    pub(crate) TIMED_SNAPSHOT_REGEX = concat!(r"^", TIMED_SNAPSHOT_RE!() ,r"$");
>> +}
>> +
>> +#[rustfmt::skip]
>> +const_regex! {
>> +    pub(crate) COMBINED_SNAPSHOT_REGEX = concat!(r"^((", TIMED_SNAPSHOT_RE!() ,r")|", PROXMOX_SAFE_ID_REGEX_STR!(), ")$");
>> +}
>> +
>> +#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord)]
>> +enum SnapshotType {
>> +    TimedSnapshot(i64),
>> +    NamedSnapshot(String),
>>   }
>>   
>>   #[api(
>>       type: String,
>> -    format: &ApiStringFormat::Pattern(&SNAPSHOT_REGEX),
>>   )]
>> -#[derive(Debug, Clone, Copy, PartialEq, PartialOrd, Eq, Ord)]
>> +#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord)]
>>   /// Mirror snapshot
>> -pub struct Snapshot(i64);
>> +pub struct Snapshot(SnapshotType);
>>   
>>   forward_serialize_to_display!(Snapshot);
>>   forward_deserialize_to_from_str!(Snapshot);
>>   
>>   impl Snapshot {
>>       pub fn now() -> Self {
>> -        Self(epoch_i64())
>> +        Self(SnapshotType::TimedSnapshot(epoch_i64()))
>> +    }
>> +
>> +    pub fn with_name(name: &str) -> Self {
>> +        Self(SnapshotType::NamedSnapshot(name.into()))
>>       }
>>   }
>>   
>>   impl Display for Snapshot {
>>       fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
>> -        let formatted = epoch_to_rfc3339_utc(self.0).map_err(|_| std::fmt::Error)?;
>> -        f.write_str(&formatted)
>> +        match &self.0 {
>> +            SnapshotType::TimedSnapshot(time) => {
>> +                let formatted = epoch_to_rfc3339_utc(*time).map_err(|_| std::fmt::Error)?;
>> +                f.write_str(&formatted)
>> +            }
>> +            SnapshotType::NamedSnapshot(name) => f.write_str(&name),
>> +        }
>>       }
>>   }
>>   
>> @@ -95,7 +121,13 @@ impl FromStr for Snapshot {
>>       type Err = Error;
>>   
>>       fn from_str(s: &str) -> Result<Self, Self::Err> {
>> -        Ok(Self(parse_rfc3339(s)?))
>> +        if TIMED_SNAPSHOT_REGEX.is_match(s) {
>> +            Ok(Self(SnapshotType::TimedSnapshot(parse_rfc3339(s)?)))
>> +        } else if PROXMOX_SAFE_ID_REGEX.is_match(s) {
>> +            Ok(Self(SnapshotType::NamedSnapshot(s.into())))
>> +        } else {
>> +            bail!("invalid snapshot name")
>> +        }
>>       }
>>   }
>>   
>> -- 
>> 2.39.5
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel



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