* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox