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-vlad...@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(§ion_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
