* [pve-devel] [PATCH proxmox-offline-mirror] Add possibility to create named snapshots @ 2025-05-06 9:09 Laurențiu Leahu-Vlăducu 2025-05-08 9:14 ` Fabian Grünbichler 0 siblings, 1 reply; 4+ messages in thread From: Laurențiu Leahu-Vlăducu @ 2025-05-06 9:09 UTC (permalink / raw) To: pve-devel 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())) .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)?; 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, ); 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; - - 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"))); + + 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); + 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!"); + } + 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH proxmox-offline-mirror] Add possibility to create named snapshots 2025-05-06 9:09 [pve-devel] [PATCH proxmox-offline-mirror] Add possibility to create named snapshots Laurențiu Leahu-Vlăducu @ 2025-05-08 9:14 ` Fabian Grünbichler 2025-05-08 15:17 ` Laurențiu Leahu-Vlăducu 0 siblings, 1 reply; 4+ messages in thread From: Fabian Grünbichler @ 2025-05-08 9:14 UTC (permalink / raw) To: Proxmox VE development discussion 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(§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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH proxmox-offline-mirror] Add possibility to create named snapshots 2025-05-08 9:14 ` Fabian Grünbichler @ 2025-05-08 15:17 ` Laurențiu Leahu-Vlăducu 2025-05-09 7:33 ` Fabian Grünbichler 0 siblings, 1 reply; 4+ messages in thread From: Laurențiu Leahu-Vlăducu @ 2025-05-08 15:17 UTC (permalink / raw) To: pve-devel 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(§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.. 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH proxmox-offline-mirror] Add possibility to create named snapshots 2025-05-08 15:17 ` Laurențiu Leahu-Vlăducu @ 2025-05-09 7:33 ` Fabian Grünbichler 0 siblings, 0 replies; 4+ messages in thread From: Fabian Grünbichler @ 2025-05-09 7:33 UTC (permalink / raw) To: Proxmox VE development discussion 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.. _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-05-09 7:33 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-05-06 9:09 [pve-devel] [PATCH proxmox-offline-mirror] Add possibility to create named snapshots Laurențiu Leahu-Vlăducu 2025-05-08 9:14 ` Fabian Grünbichler 2025-05-08 15:17 ` Laurențiu Leahu-Vlăducu 2025-05-09 7:33 ` Fabian Grünbichler
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.Service provided by Proxmox Server Solutions GmbH | Privacy | Legal