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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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..

>  
>      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?

> -
> -    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..

> +
> +    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..

> +
>      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..

> +        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