* [PATCH proxmox-backup v4 1/7] datastore: add namespace-level locking
2026-03-11 15:13 [PATCH proxmox-backup v4 0/7] fixes #6195: add support for moving groups and namespaces Hannes Laimer
@ 2026-03-11 15:13 ` Hannes Laimer
2026-03-12 15:43 ` Christian Ebner
2026-03-11 15:13 ` [PATCH proxmox-backup v4 2/7] datastore: add move_group Hannes Laimer
` (6 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Hannes Laimer @ 2026-03-11 15:13 UTC (permalink / raw)
To: pbs-devel
Add exclusive/shared namespace locking keyed at
/run/proxmox-backup/locks/{store}/{ns}/.ns-lock.
Operations that read from or write into a namespace hold a shared lock
for their duration. Structural operations (move, delete) hold an
exclusive lock. The shared lock is hierarchical: locking a/b/c also
locks a/b and a, so an exclusive lock on any ancestor blocks all
active operations below it. Walking up the ancestor chain costs
O(depth), which is bounded by the maximum namespace depth of 8,
whereas locking all descendants would be arbitrarily expensive.
Backup jobs and pull/push sync acquire the shared lock via
create_locked_backup_group and pull_ns/push_namespace respectively.
Verify and prune acquire it per snapshot/group and skip gracefully if
the lock cannot be taken, since a concurrent move is a transient
condition.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
pbs-datastore/src/backup_info.rs | 92 ++++++++++++++++++++++++++++++++
pbs-datastore/src/datastore.rs | 45 +++++++++++++---
src/api2/admin/namespace.rs | 6 ++-
src/api2/backup/environment.rs | 4 ++
src/api2/backup/mod.rs | 14 +++--
src/api2/tape/restore.rs | 9 ++--
src/backup/verify.rs | 19 ++++++-
src/server/prune_job.rs | 11 ++++
src/server/pull.rs | 8 ++-
src/server/push.rs | 6 +++
10 files changed, 193 insertions(+), 21 deletions(-)
diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index c33eb307..476daa61 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -937,6 +937,98 @@ fn lock_file_path_helper(ns: &BackupNamespace, path: PathBuf) -> PathBuf {
to_return.join(format!("{first_eigthy}...{last_eighty}-{hash}"))
}
+/// Returns the lock file path for a backup namespace.
+///
+/// The lock file will be located at:
+/// `${DATASTORE_LOCKS_DIR}/${store_name}/${ns_colon_encoded}/.ns-lock`
+pub(crate) fn ns_lock_path(store_name: &str, ns: &BackupNamespace) -> PathBuf {
+ let ns_part = ns
+ .components()
+ .map(String::from)
+ .reduce(|acc, n| format!("{acc}:{n}"))
+ .unwrap_or_default();
+ Path::new(DATASTORE_LOCKS_DIR)
+ .join(store_name)
+ .join(ns_part)
+ .join(".ns-lock")
+}
+
+/// Holds namespace locks acquired for a structural operation or a read/write operation.
+///
+/// For exclusive locks the first guard is exclusive (on the namespace itself), and any remaining
+/// guards are shared (on ancestor namespaces). For shared locks all guards are shared.
+pub struct NamespaceLockGuard {
+ _guards: Vec<BackupLockGuard>,
+}
+
+fn lock_namespace_exclusive_single(
+ store_name: &str,
+ ns: &BackupNamespace,
+) -> Result<BackupLockGuard, Error> {
+ let path = ns_lock_path(store_name, ns);
+ lock_helper(store_name, &path, |p| {
+ open_backup_lockfile(p, Some(Duration::from_secs(0)), true)
+ .with_context(|| format!("unable to acquire exclusive namespace lock for '{ns}'"))
+ })
+}
+
+fn lock_namespace_shared_single(
+ store_name: &str,
+ ns: &BackupNamespace,
+) -> Result<BackupLockGuard, Error> {
+ let path = ns_lock_path(store_name, ns);
+ lock_helper(store_name, &path, |p| {
+ open_backup_lockfile(p, Some(Duration::from_secs(0)), false)
+ .with_context(|| format!("unable to acquire shared namespace lock for '{ns}'"))
+ })
+}
+
+/// Acquires an exclusive lock on `ns` and shared locks on all its non-root ancestors.
+///
+/// Used by operations that structurally modify a namespace (e.g. move). The shared ancestor locks
+/// ensure that a concurrent structural operation on any ancestor namespace blocks until this one
+/// completes, and vice versa - mirroring the hierarchical behavior of `lock_namespace_shared`.
+pub(crate) fn lock_namespace(
+ store_name: &str,
+ ns: &BackupNamespace,
+) -> Result<NamespaceLockGuard, Error> {
+ // Acquire the exclusive lock on ns first, then shared locks on ancestors.
+ // Order matters: taking exclusive before shared avoids a scenario where we hold shared on
+ // an ancestor and then block waiting for exclusive on ns (which another holder of that
+ // ancestor's shared lock might be waiting to promote - not currently possible but fragile).
+ let exclusive = lock_namespace_exclusive_single(store_name, ns)?;
+ let mut guards = vec![exclusive];
+ let mut current = ns.clone();
+ while !current.is_root() {
+ current = current.parent();
+ if !current.is_root() {
+ guards.push(lock_namespace_shared_single(store_name, ¤t)?);
+ }
+ }
+ Ok(NamespaceLockGuard { _guards: guards })
+}
+
+/// Acquires shared locks on a namespace and all its non-root ancestors.
+///
+/// Held by operations that read from or write into a namespace (backup, sync, verify, prune),
+/// preventing concurrent exclusive operations (e.g. move) on the namespace itself or any ancestor
+/// from proceeding while this guard is alive.
+///
+/// Locking up the ancestor chain (bounded by max namespace depth) rather than down the subtree
+/// keeps the cost O(depth) regardless of how wide the namespace tree is.
+pub(crate) fn lock_namespace_shared(
+ store_name: &str,
+ ns: &BackupNamespace,
+) -> Result<NamespaceLockGuard, Error> {
+ let mut guards = Vec::new();
+ let mut current = ns.clone();
+ while !current.is_root() {
+ guards.push(lock_namespace_shared_single(store_name, ¤t)?);
+ current = current.parent();
+ }
+ Ok(NamespaceLockGuard { _guards: guards })
+}
+
/// Helps implement the double stat'ing procedure. It avoids certain race conditions upon lock
/// deletion.
///
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index ef378c69..564c44a1 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -38,7 +38,8 @@ use pbs_config::{BackupLockGuard, ConfigVersionCache};
use proxmox_section_config::SectionConfigData;
use crate::backup_info::{
- BackupDir, BackupGroup, BackupInfo, OLD_LOCKING, PROTECTED_MARKER_FILENAME,
+ lock_namespace, lock_namespace_shared, BackupDir, BackupGroup, BackupInfo, NamespaceLockGuard,
+ OLD_LOCKING, PROTECTED_MARKER_FILENAME,
};
use crate::chunk_store::ChunkStore;
use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
@@ -1123,18 +1124,46 @@ impl DataStore {
Ok(())
}
- /// Create (if it does not already exists) and lock a backup group
+ /// Acquires an exclusive lock on a backup namespace and shared locks on all its ancestors.
///
- /// And set the owner to 'userid'. If the group already exists, it returns the
- /// current owner (instead of setting the owner).
+ /// Operations that structurally modify a namespace (move, delete) must hold this for their
+ /// duration to prevent concurrent readers or writers from accessing the namespace while it is
+ /// being relocated or destroyed.
+ pub fn lock_namespace(
+ self: &Arc<Self>,
+ ns: &BackupNamespace,
+ ) -> Result<NamespaceLockGuard, Error> {
+ lock_namespace(self.name(), ns)
+ }
+
+ /// Acquires shared locks on a backup namespace and all its non-root ancestors.
///
- /// This also acquires an exclusive lock on the directory and returns the lock guard.
+ /// Operations that read from or write into a namespace (backup, sync, verify, prune) must hold
+ /// this for their duration to prevent a concurrent `move_namespace` or `delete_namespace` from
+ /// relocating or destroying the namespace under them.
+ pub fn lock_namespace_shared(
+ self: &Arc<Self>,
+ ns: &BackupNamespace,
+ ) -> Result<NamespaceLockGuard, Error> {
+ lock_namespace_shared(self.name(), ns)
+ }
+
+ /// Create (if it does not already exist) and lock a backup group.
+ ///
+ /// Sets the owner to `auth_id`. If the group already exists, returns the current owner.
+ ///
+ /// Returns `(owner, ns_lock, group_lock)`. Both locks must be kept alive for the duration of
+ /// the backup session: the shared namespace lock prevents concurrent namespace moves or
+ /// deletions, and the exclusive group lock prevents concurrent backups to the same group.
pub fn create_locked_backup_group(
self: &Arc<Self>,
ns: &BackupNamespace,
backup_group: &pbs_api_types::BackupGroup,
auth_id: &Authid,
- ) -> Result<(Authid, BackupLockGuard), Error> {
+ ) -> Result<(Authid, NamespaceLockGuard, BackupLockGuard), Error> {
+ let ns_guard = lock_namespace_shared(self.name(), ns)
+ .with_context(|| format!("failed to acquire shared namespace lock for '{ns}'"))?;
+
let backup_group = self.backup_group(ns.clone(), backup_group.clone());
// create intermediate path first
@@ -1155,14 +1184,14 @@ impl DataStore {
return Err(err);
}
let owner = self.get_owner(ns, backup_group.group())?; // just to be sure
- Ok((owner, guard))
+ Ok((owner, ns_guard, guard))
}
Err(ref err) if err.kind() == io::ErrorKind::AlreadyExists => {
let guard = backup_group.lock().with_context(|| {
format!("while creating locked backup group '{backup_group:?}'")
})?;
let owner = self.get_owner(ns, backup_group.group())?; // just to be sure
- Ok((owner, guard))
+ Ok((owner, ns_guard, guard))
}
Err(err) => bail!("unable to create backup group {:?} - {}", full_path, err),
}
diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
index 30e24d8d..ec913001 100644
--- a/src/api2/admin/namespace.rs
+++ b/src/api2/admin/namespace.rs
@@ -1,4 +1,4 @@
-use anyhow::{bail, Error};
+use anyhow::{bail, Context, Error};
use pbs_config::CachedUserInfo;
use proxmox_router::{http_bail, ApiMethod, Permission, Router, RpcEnvironment};
@@ -164,6 +164,10 @@ pub fn delete_namespace(
let datastore = DataStore::lookup_datastore(&store, Operation::Write)?;
+ let _ns_lock = datastore
+ .lock_namespace(&ns)
+ .with_context(|| format!("failed to lock namespace '{ns}' for deletion"))?;
+
let (removed_all, stats) = datastore.remove_namespace_recursive(&ns, delete_groups)?;
if !removed_all {
let err_msg = if delete_groups {
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index ab623f1f..44483add 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -1,5 +1,6 @@
use anyhow::{bail, format_err, Context, Error};
use pbs_config::BackupLockGuard;
+use pbs_datastore::backup_info::NamespaceLockGuard;
use proxmox_sys::process_locker::ProcessLockSharedGuard;
use std::collections::HashMap;
@@ -100,6 +101,7 @@ struct SharedBackupState {
pub struct BackupLockGuards {
previous_snapshot: Option<BackupLockGuard>,
+ _namespace: Option<NamespaceLockGuard>,
group: Option<BackupLockGuard>,
snapshot: Option<BackupLockGuard>,
chunk_store: Option<ProcessLockSharedGuard>,
@@ -108,12 +110,14 @@ pub struct BackupLockGuards {
impl BackupLockGuards {
pub(crate) fn new(
previous_snapshot: Option<BackupLockGuard>,
+ namespace: NamespaceLockGuard,
group: BackupLockGuard,
snapshot: BackupLockGuard,
chunk_store: ProcessLockSharedGuard,
) -> Self {
Self {
previous_snapshot,
+ _namespace: Some(namespace),
group: Some(group),
snapshot: Some(snapshot),
chunk_store: Some(chunk_store),
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 6df0d34b..80e70dac 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -143,8 +143,9 @@ fn upgrade_to_backup_protocol(
"backup"
};
- // lock backup group to only allow one backup per group at a time
- let (owner, group_guard) = datastore.create_locked_backup_group(
+ // lock backup group to only allow one backup per group at a time,
+ // also acquires a shared namespace lock to prevent concurrent namespace moves
+ let (owner, ns_guard, group_guard) = datastore.create_locked_backup_group(
backup_group.backup_ns(),
backup_group.as_ref(),
&auth_id,
@@ -215,8 +216,13 @@ fn upgrade_to_backup_protocol(
// case of errors. The former is required for immediate subsequent backups (e.g.
// during a push sync) to be able to lock the group and snapshots.
let chunk_store_guard = datastore.try_shared_chunk_store_lock()?;
- let backup_lock_guards =
- BackupLockGuards::new(last_guard, group_guard, snap_guard, chunk_store_guard);
+ let backup_lock_guards = BackupLockGuards::new(
+ last_guard,
+ ns_guard,
+ group_guard,
+ snap_guard,
+ chunk_store_guard,
+ );
let mut env = BackupEnvironment::new(
env_type,
diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 92529a76..1cb85f2d 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -848,11 +848,8 @@ fn restore_list_worker(
Some(restore_owner),
)?;
- let (owner, _group_lock) = datastore.create_locked_backup_group(
- &ns,
- backup_dir.as_ref(),
- restore_owner,
- )?;
+ let (owner, _ns_guard, _group_lock) = datastore
+ .create_locked_backup_group(&ns, backup_dir.as_ref(), restore_owner)?;
if restore_owner != &owner {
bail!(
"cannot restore snapshot '{snapshot}' into group '{}', owner check \
@@ -1354,7 +1351,7 @@ fn restore_archive<'a>(
auth_id,
Some(restore_owner),
)?;
- let (owner, _group_lock) = datastore.create_locked_backup_group(
+ let (owner, _ns_guard, _group_lock) = datastore.create_locked_backup_group(
&backup_ns,
backup_dir.as_ref(),
restore_owner,
diff --git a/src/backup/verify.rs b/src/backup/verify.rs
index f52d7781..ce5d6f69 100644
--- a/src/backup/verify.rs
+++ b/src/backup/verify.rs
@@ -393,10 +393,27 @@ impl VerifyWorker {
return Ok(true);
}
+ let ns_lock = match self.datastore.lock_namespace_shared(backup_dir.backup_ns()) {
+ Ok(lock) => lock,
+ Err(err) => {
+ info!(
+ "SKIPPED: verify {}:{} - could not acquire namespace lock: {}",
+ self.datastore.name(),
+ backup_dir.dir(),
+ err,
+ );
+ return Ok(true);
+ }
+ };
+
let snap_lock = backup_dir.lock_shared();
match snap_lock {
- Ok(snap_lock) => self.verify_backup_dir_with_lock(backup_dir, upid, filter, snap_lock),
+ Ok(snap_lock) => {
+ let result = self.verify_backup_dir_with_lock(backup_dir, upid, filter, snap_lock);
+ drop(ns_lock);
+ result
+ }
Err(err) => {
info!(
"SKIPPED: verify {}:{} - could not acquire snapshot lock: {}",
diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
index bb86a323..8103f59c 100644
--- a/src/server/prune_job.rs
+++ b/src/server/prune_job.rs
@@ -54,6 +54,17 @@ pub fn prune_datastore(
)? {
let group = group?;
let ns = group.backup_ns();
+ let _ns_lock = match datastore.lock_namespace_shared(ns) {
+ Ok(lock) => lock,
+ Err(err) => {
+ warn!(
+ "skipping prune of group '{}:{}' - could not acquire namespace lock: {err}",
+ ns,
+ group.group(),
+ );
+ continue;
+ }
+ };
let list = group.list_backups()?;
let mut prune_info = compute_prune_info(list, &prune_options.keep)?;
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 0ac6b5b8..40173324 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -1061,6 +1061,12 @@ async fn pull_ns(
params: &mut PullParameters,
encountered_chunks: Arc<Mutex<EncounteredChunks>>,
) -> Result<(StoreProgress, SyncStats, bool), Error> {
+ let _ns_lock = params
+ .target
+ .store
+ .lock_namespace_shared(namespace)
+ .with_context(|| format!("failed to acquire shared namespace lock for '{namespace}'"))?;
+
let list: Vec<BackupGroup> = params.source.list_groups(namespace, ¶ms.owner).await?;
let unfiltered_count = list.len();
@@ -1093,7 +1099,7 @@ async fn pull_ns(
progress.done_snapshots = 0;
progress.group_snapshots = 0;
- let (owner, _lock_guard) =
+ let (owner, _ns_guard, _lock_guard) =
match params
.target
.store
diff --git a/src/server/push.rs b/src/server/push.rs
index 27c5b22d..47067d66 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -525,6 +525,12 @@ pub(crate) async fn push_namespace(
check_ns_remote_datastore_privs(params, &target_namespace, PRIV_REMOTE_DATASTORE_BACKUP)
.context("Pushing to remote namespace not allowed")?;
+ let _ns_lock = params
+ .source
+ .store
+ .lock_namespace_shared(namespace)
+ .with_context(|| format!("failed to acquire shared namespace lock for '{namespace}'"))?;
+
let mut list: Vec<BackupGroup> = params
.source
.list_groups(namespace, ¶ms.local_user)
--
2.47.3
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH proxmox-backup v4 1/7] datastore: add namespace-level locking
2026-03-11 15:13 ` [PATCH proxmox-backup v4 1/7] datastore: add namespace-level locking Hannes Laimer
@ 2026-03-12 15:43 ` Christian Ebner
2026-03-13 7:40 ` Hannes Laimer
2026-03-17 13:03 ` Christian Ebner
0 siblings, 2 replies; 20+ messages in thread
From: Christian Ebner @ 2026-03-12 15:43 UTC (permalink / raw)
To: Hannes Laimer, pbs-devel
One high level comment: I think it would make sense to allow to set an
overall timeout when acquiring the namespace locks. Most move operations
probably are rather fast anyways, so waiting a few seconds in a e.g.
prune job will probably be preferable over parts of the prune job being
skipped.
Another comment inline.
On 3/11/26 4:13 PM, Hannes Laimer wrote:
> Add exclusive/shared namespace locking keyed at
> /run/proxmox-backup/locks/{store}/{ns}/.ns-lock.
>
> Operations that read from or write into a namespace hold a shared lock
> for their duration. Structural operations (move, delete) hold an
> exclusive lock. The shared lock is hierarchical: locking a/b/c also
> locks a/b and a, so an exclusive lock on any ancestor blocks all
> active operations below it. Walking up the ancestor chain costs
> O(depth), which is bounded by the maximum namespace depth of 8,
> whereas locking all descendants would be arbitrarily expensive.
>
> Backup jobs and pull/push sync acquire the shared lock via
> create_locked_backup_group and pull_ns/push_namespace respectively.
> Verify and prune acquire it per snapshot/group and skip gracefully if
> the lock cannot be taken, since a concurrent move is a transient
> condition.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> pbs-datastore/src/backup_info.rs | 92 ++++++++++++++++++++++++++++++++
> pbs-datastore/src/datastore.rs | 45 +++++++++++++---
> src/api2/admin/namespace.rs | 6 ++-
> src/api2/backup/environment.rs | 4 ++
> src/api2/backup/mod.rs | 14 +++--
> src/api2/tape/restore.rs | 9 ++--
> src/backup/verify.rs | 19 ++++++-
> src/server/prune_job.rs | 11 ++++
> src/server/pull.rs | 8 ++-
> src/server/push.rs | 6 +++
> 10 files changed, 193 insertions(+), 21 deletions(-)
>
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index c33eb307..476daa61 100644
> --- a/pbs-datastore/src/backup_info.rs
> +++ b/pbs-datastore/src/backup_info.rs
> @@ -937,6 +937,98 @@ fn lock_file_path_helper(ns: &BackupNamespace, path: PathBuf) -> PathBuf {
> to_return.join(format!("{first_eigthy}...{last_eighty}-{hash}"))
> }
>
> +/// Returns the lock file path for a backup namespace.
> +///
> +/// The lock file will be located at:
> +/// `${DATASTORE_LOCKS_DIR}/${store_name}/${ns_colon_encoded}/.ns-lock`
> +pub(crate) fn ns_lock_path(store_name: &str, ns: &BackupNamespace) -> PathBuf {
> + let ns_part = ns
> + .components()
> + .map(String::from)
> + .reduce(|acc, n| format!("{acc}:{n}"))
> + .unwrap_or_default();
> + Path::new(DATASTORE_LOCKS_DIR)
> + .join(store_name)
> + .join(ns_part)
> + .join(".ns-lock")
> +}
> +
> +/// Holds namespace locks acquired for a structural operation or a read/write operation.
> +///
> +/// For exclusive locks the first guard is exclusive (on the namespace itself), and any remaining
> +/// guards are shared (on ancestor namespaces). For shared locks all guards are shared.
> +pub struct NamespaceLockGuard {
> + _guards: Vec<BackupLockGuard>,
> +}
comment: While it might not be that critical in practice, it probably
makes sense to also control the order with which the lock guards are
dropped by implementing the Drop trait and consuming the vector in order
to avoid possible inconsistencies with the assumed order.
> +
> +fn lock_namespace_exclusive_single(
> + store_name: &str,
> + ns: &BackupNamespace,
> +) -> Result<BackupLockGuard, Error> {
> + let path = ns_lock_path(store_name, ns);
> + lock_helper(store_name, &path, |p| {
> + open_backup_lockfile(p, Some(Duration::from_secs(0)), true)
> + .with_context(|| format!("unable to acquire exclusive namespace lock for '{ns}'"))
> + })
> +}
> +
> +fn lock_namespace_shared_single(
> + store_name: &str,
> + ns: &BackupNamespace,
> +) -> Result<BackupLockGuard, Error> {
> + let path = ns_lock_path(store_name, ns);
> + lock_helper(store_name, &path, |p| {
> + open_backup_lockfile(p, Some(Duration::from_secs(0)), false)
> + .with_context(|| format!("unable to acquire shared namespace lock for '{ns}'"))
> + })
> +}
> +
> +/// Acquires an exclusive lock on `ns` and shared locks on all its non-root ancestors.
> +///
> +/// Used by operations that structurally modify a namespace (e.g. move). The shared ancestor locks
> +/// ensure that a concurrent structural operation on any ancestor namespace blocks until this one
> +/// completes, and vice versa - mirroring the hierarchical behavior of `lock_namespace_shared`.
> +pub(crate) fn lock_namespace(
> + store_name: &str,
> + ns: &BackupNamespace,
> +) -> Result<NamespaceLockGuard, Error> {
> + // Acquire the exclusive lock on ns first, then shared locks on ancestors.
> + // Order matters: taking exclusive before shared avoids a scenario where we hold shared on
> + // an ancestor and then block waiting for exclusive on ns (which another holder of that
> + // ancestor's shared lock might be waiting to promote - not currently possible but fragile).
> + let exclusive = lock_namespace_exclusive_single(store_name, ns)?;
> + let mut guards = vec![exclusive];
> + let mut current = ns.clone();
> + while !current.is_root() {
> + current = current.parent();
> + if !current.is_root() {
> + guards.push(lock_namespace_shared_single(store_name, ¤t)?);
> + }
> + }
> + Ok(NamespaceLockGuard { _guards: guards })
> +}
> +
> +/// Acquires shared locks on a namespace and all its non-root ancestors.
> +///
> +/// Held by operations that read from or write into a namespace (backup, sync, verify, prune),
> +/// preventing concurrent exclusive operations (e.g. move) on the namespace itself or any ancestor
> +/// from proceeding while this guard is alive.
> +///
> +/// Locking up the ancestor chain (bounded by max namespace depth) rather than down the subtree
> +/// keeps the cost O(depth) regardless of how wide the namespace tree is.
> +pub(crate) fn lock_namespace_shared(
> + store_name: &str,
> + ns: &BackupNamespace,
> +) -> Result<NamespaceLockGuard, Error> {
> + let mut guards = Vec::new();
> + let mut current = ns.clone();
> + while !current.is_root() {
> + guards.push(lock_namespace_shared_single(store_name, ¤t)?);
> + current = current.parent();
> + }
> + Ok(NamespaceLockGuard { _guards: guards })
> +}
> +
> /// Helps implement the double stat'ing procedure. It avoids certain race conditions upon lock
> /// deletion.
> ///
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index ef378c69..564c44a1 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -38,7 +38,8 @@ use pbs_config::{BackupLockGuard, ConfigVersionCache};
> use proxmox_section_config::SectionConfigData;
>
> use crate::backup_info::{
> - BackupDir, BackupGroup, BackupInfo, OLD_LOCKING, PROTECTED_MARKER_FILENAME,
> + lock_namespace, lock_namespace_shared, BackupDir, BackupGroup, BackupInfo, NamespaceLockGuard,
> + OLD_LOCKING, PROTECTED_MARKER_FILENAME,
> };
> use crate::chunk_store::ChunkStore;
> use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
> @@ -1123,18 +1124,46 @@ impl DataStore {
> Ok(())
> }
>
> - /// Create (if it does not already exists) and lock a backup group
> + /// Acquires an exclusive lock on a backup namespace and shared locks on all its ancestors.
> ///
> - /// And set the owner to 'userid'. If the group already exists, it returns the
> - /// current owner (instead of setting the owner).
> + /// Operations that structurally modify a namespace (move, delete) must hold this for their
> + /// duration to prevent concurrent readers or writers from accessing the namespace while it is
> + /// being relocated or destroyed.
> + pub fn lock_namespace(
> + self: &Arc<Self>,
> + ns: &BackupNamespace,
> + ) -> Result<NamespaceLockGuard, Error> {
> + lock_namespace(self.name(), ns)
> + }
> +
> + /// Acquires shared locks on a backup namespace and all its non-root ancestors.
> ///
> - /// This also acquires an exclusive lock on the directory and returns the lock guard.
> + /// Operations that read from or write into a namespace (backup, sync, verify, prune) must hold
> + /// this for their duration to prevent a concurrent `move_namespace` or `delete_namespace` from
> + /// relocating or destroying the namespace under them.
> + pub fn lock_namespace_shared(
> + self: &Arc<Self>,
> + ns: &BackupNamespace,
> + ) -> Result<NamespaceLockGuard, Error> {
> + lock_namespace_shared(self.name(), ns)
> + }
> +
> + /// Create (if it does not already exist) and lock a backup group.
> + ///
> + /// Sets the owner to `auth_id`. If the group already exists, returns the current owner.
> + ///
> + /// Returns `(owner, ns_lock, group_lock)`. Both locks must be kept alive for the duration of
> + /// the backup session: the shared namespace lock prevents concurrent namespace moves or
> + /// deletions, and the exclusive group lock prevents concurrent backups to the same group.
> pub fn create_locked_backup_group(
> self: &Arc<Self>,
> ns: &BackupNamespace,
> backup_group: &pbs_api_types::BackupGroup,
> auth_id: &Authid,
> - ) -> Result<(Authid, BackupLockGuard), Error> {
> + ) -> Result<(Authid, NamespaceLockGuard, BackupLockGuard), Error> {
> + let ns_guard = lock_namespace_shared(self.name(), ns)
> + .with_context(|| format!("failed to acquire shared namespace lock for '{ns}'"))?;
> +
> let backup_group = self.backup_group(ns.clone(), backup_group.clone());
>
> // create intermediate path first
> @@ -1155,14 +1184,14 @@ impl DataStore {
> return Err(err);
> }
> let owner = self.get_owner(ns, backup_group.group())?; // just to be sure
> - Ok((owner, guard))
> + Ok((owner, ns_guard, guard))
> }
> Err(ref err) if err.kind() == io::ErrorKind::AlreadyExists => {
> let guard = backup_group.lock().with_context(|| {
> format!("while creating locked backup group '{backup_group:?}'")
> })?;
> let owner = self.get_owner(ns, backup_group.group())?; // just to be sure
> - Ok((owner, guard))
> + Ok((owner, ns_guard, guard))
> }
> Err(err) => bail!("unable to create backup group {:?} - {}", full_path, err),
> }
> diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
> index 30e24d8d..ec913001 100644
> --- a/src/api2/admin/namespace.rs
> +++ b/src/api2/admin/namespace.rs
> @@ -1,4 +1,4 @@
> -use anyhow::{bail, Error};
> +use anyhow::{bail, Context, Error};
>
> use pbs_config::CachedUserInfo;
> use proxmox_router::{http_bail, ApiMethod, Permission, Router, RpcEnvironment};
> @@ -164,6 +164,10 @@ pub fn delete_namespace(
>
> let datastore = DataStore::lookup_datastore(&store, Operation::Write)?;
>
> + let _ns_lock = datastore
> + .lock_namespace(&ns)
> + .with_context(|| format!("failed to lock namespace '{ns}' for deletion"))?;
> +
> let (removed_all, stats) = datastore.remove_namespace_recursive(&ns, delete_groups)?;
> if !removed_all {
> let err_msg = if delete_groups {
> diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
> index ab623f1f..44483add 100644
> --- a/src/api2/backup/environment.rs
> +++ b/src/api2/backup/environment.rs
> @@ -1,5 +1,6 @@
> use anyhow::{bail, format_err, Context, Error};
> use pbs_config::BackupLockGuard;
> +use pbs_datastore::backup_info::NamespaceLockGuard;
> use proxmox_sys::process_locker::ProcessLockSharedGuard;
>
> use std::collections::HashMap;
> @@ -100,6 +101,7 @@ struct SharedBackupState {
>
> pub struct BackupLockGuards {
> previous_snapshot: Option<BackupLockGuard>,
> + _namespace: Option<NamespaceLockGuard>,
> group: Option<BackupLockGuard>,
> snapshot: Option<BackupLockGuard>,
> chunk_store: Option<ProcessLockSharedGuard>,
> @@ -108,12 +110,14 @@ pub struct BackupLockGuards {
> impl BackupLockGuards {
> pub(crate) fn new(
> previous_snapshot: Option<BackupLockGuard>,
> + namespace: NamespaceLockGuard,
> group: BackupLockGuard,
> snapshot: BackupLockGuard,
> chunk_store: ProcessLockSharedGuard,
> ) -> Self {
> Self {
> previous_snapshot,
> + _namespace: Some(namespace),
> group: Some(group),
> snapshot: Some(snapshot),
> chunk_store: Some(chunk_store),
> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
> index 6df0d34b..80e70dac 100644
> --- a/src/api2/backup/mod.rs
> +++ b/src/api2/backup/mod.rs
> @@ -143,8 +143,9 @@ fn upgrade_to_backup_protocol(
> "backup"
> };
>
> - // lock backup group to only allow one backup per group at a time
> - let (owner, group_guard) = datastore.create_locked_backup_group(
> + // lock backup group to only allow one backup per group at a time,
> + // also acquires a shared namespace lock to prevent concurrent namespace moves
> + let (owner, ns_guard, group_guard) = datastore.create_locked_backup_group(
> backup_group.backup_ns(),
> backup_group.as_ref(),
> &auth_id,
> @@ -215,8 +216,13 @@ fn upgrade_to_backup_protocol(
> // case of errors. The former is required for immediate subsequent backups (e.g.
> // during a push sync) to be able to lock the group and snapshots.
> let chunk_store_guard = datastore.try_shared_chunk_store_lock()?;
> - let backup_lock_guards =
> - BackupLockGuards::new(last_guard, group_guard, snap_guard, chunk_store_guard);
> + let backup_lock_guards = BackupLockGuards::new(
> + last_guard,
> + ns_guard,
> + group_guard,
> + snap_guard,
> + chunk_store_guard,
> + );
>
> let mut env = BackupEnvironment::new(
> env_type,
> diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
> index 92529a76..1cb85f2d 100644
> --- a/src/api2/tape/restore.rs
> +++ b/src/api2/tape/restore.rs
> @@ -848,11 +848,8 @@ fn restore_list_worker(
> Some(restore_owner),
> )?;
>
> - let (owner, _group_lock) = datastore.create_locked_backup_group(
> - &ns,
> - backup_dir.as_ref(),
> - restore_owner,
> - )?;
> + let (owner, _ns_guard, _group_lock) = datastore
> + .create_locked_backup_group(&ns, backup_dir.as_ref(), restore_owner)?;
> if restore_owner != &owner {
> bail!(
> "cannot restore snapshot '{snapshot}' into group '{}', owner check \
> @@ -1354,7 +1351,7 @@ fn restore_archive<'a>(
> auth_id,
> Some(restore_owner),
> )?;
> - let (owner, _group_lock) = datastore.create_locked_backup_group(
> + let (owner, _ns_guard, _group_lock) = datastore.create_locked_backup_group(
> &backup_ns,
> backup_dir.as_ref(),
> restore_owner,
> diff --git a/src/backup/verify.rs b/src/backup/verify.rs
> index f52d7781..ce5d6f69 100644
> --- a/src/backup/verify.rs
> +++ b/src/backup/verify.rs
> @@ -393,10 +393,27 @@ impl VerifyWorker {
> return Ok(true);
> }
>
> + let ns_lock = match self.datastore.lock_namespace_shared(backup_dir.backup_ns()) {
> + Ok(lock) => lock,
> + Err(err) => {
> + info!(
> + "SKIPPED: verify {}:{} - could not acquire namespace lock: {}",
> + self.datastore.name(),
> + backup_dir.dir(),
> + err,
> + );
> + return Ok(true);
> + }
> + };
> +
> let snap_lock = backup_dir.lock_shared();
>
> match snap_lock {
> - Ok(snap_lock) => self.verify_backup_dir_with_lock(backup_dir, upid, filter, snap_lock),
> + Ok(snap_lock) => {
> + let result = self.verify_backup_dir_with_lock(backup_dir, upid, filter, snap_lock);
> + drop(ns_lock);
> + result
> + }
> Err(err) => {
> info!(
> "SKIPPED: verify {}:{} - could not acquire snapshot lock: {}",
> diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
> index bb86a323..8103f59c 100644
> --- a/src/server/prune_job.rs
> +++ b/src/server/prune_job.rs
> @@ -54,6 +54,17 @@ pub fn prune_datastore(
> )? {
> let group = group?;
> let ns = group.backup_ns();
> + let _ns_lock = match datastore.lock_namespace_shared(ns) {
> + Ok(lock) => lock,
> + Err(err) => {
> + warn!(
> + "skipping prune of group '{}:{}' - could not acquire namespace lock: {err}",
> + ns,
> + group.group(),
> + );
> + continue;
> + }
> + };
> let list = group.list_backups()?;
>
> let mut prune_info = compute_prune_info(list, &prune_options.keep)?;
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index 0ac6b5b8..40173324 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -1061,6 +1061,12 @@ async fn pull_ns(
> params: &mut PullParameters,
> encountered_chunks: Arc<Mutex<EncounteredChunks>>,
> ) -> Result<(StoreProgress, SyncStats, bool), Error> {
> + let _ns_lock = params
> + .target
> + .store
> + .lock_namespace_shared(namespace)
> + .with_context(|| format!("failed to acquire shared namespace lock for '{namespace}'"))?;
> +
> let list: Vec<BackupGroup> = params.source.list_groups(namespace, ¶ms.owner).await?;
>
> let unfiltered_count = list.len();
> @@ -1093,7 +1099,7 @@ async fn pull_ns(
> progress.done_snapshots = 0;
> progress.group_snapshots = 0;
>
> - let (owner, _lock_guard) =
> + let (owner, _ns_guard, _lock_guard) =
> match params
> .target
> .store
> diff --git a/src/server/push.rs b/src/server/push.rs
> index 27c5b22d..47067d66 100644
> --- a/src/server/push.rs
> +++ b/src/server/push.rs
> @@ -525,6 +525,12 @@ pub(crate) async fn push_namespace(
> check_ns_remote_datastore_privs(params, &target_namespace, PRIV_REMOTE_DATASTORE_BACKUP)
> .context("Pushing to remote namespace not allowed")?;
>
> + let _ns_lock = params
> + .source
> + .store
> + .lock_namespace_shared(namespace)
> + .with_context(|| format!("failed to acquire shared namespace lock for '{namespace}'"))?;
> +
> let mut list: Vec<BackupGroup> = params
> .source
> .list_groups(namespace, ¶ms.local_user)
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH proxmox-backup v4 1/7] datastore: add namespace-level locking
2026-03-12 15:43 ` Christian Ebner
@ 2026-03-13 7:40 ` Hannes Laimer
2026-03-13 7:56 ` Christian Ebner
2026-03-17 13:03 ` Christian Ebner
1 sibling, 1 reply; 20+ messages in thread
From: Hannes Laimer @ 2026-03-13 7:40 UTC (permalink / raw)
To: Christian Ebner, pbs-devel
On 2026-03-12 16:43, Christian Ebner wrote:
> One high level comment: I think it would make sense to allow to set an
> overall timeout when acquiring the namespace locks. Most move operations
> probably are rather fast anyways, so waiting a few seconds in a e.g.
> prune job will probably be preferable over parts of the prune job being
> skipped.
>
set as in have it configurable? Or just defining one for the locks?
At least with move in mind, I though that waiting for a lock could mean
that the things might be gone by the time we get the lock, 50/50 chance
of that happening actually. So i figured it'd be better to not have
that, technicaly shouldn't be a problem cause I think we do all
existance checks after locking, but still, waiting for something that is
currently being moved (away 1 in 2 times) seemed odd.
not super oppoed though for a few seconds of hard-coded waiting.
> Another comment inline.
>
> On 3/11/26 4:13 PM, Hannes Laimer wrote:
>> Add exclusive/shared namespace locking keyed at
>> /run/proxmox-backup/locks/{store}/{ns}/.ns-lock.
>>
>> Operations that read from or write into a namespace hold a shared lock
>> for their duration. Structural operations (move, delete) hold an
>> exclusive lock. The shared lock is hierarchical: locking a/b/c also
>> locks a/b and a, so an exclusive lock on any ancestor blocks all
>> active operations below it. Walking up the ancestor chain costs
>> O(depth), which is bounded by the maximum namespace depth of 8,
>> whereas locking all descendants would be arbitrarily expensive.
>>
>> Backup jobs and pull/push sync acquire the shared lock via
>> create_locked_backup_group and pull_ns/push_namespace respectively.
>> Verify and prune acquire it per snapshot/group and skip gracefully if
>> the lock cannot be taken, since a concurrent move is a transient
>> condition.
>>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>> pbs-datastore/src/backup_info.rs | 92 ++++++++++++++++++++++++++++++++
>> pbs-datastore/src/datastore.rs | 45 +++++++++++++---
>> src/api2/admin/namespace.rs | 6 ++-
>> src/api2/backup/environment.rs | 4 ++
>> src/api2/backup/mod.rs | 14 +++--
>> src/api2/tape/restore.rs | 9 ++--
>> src/backup/verify.rs | 19 ++++++-
>> src/server/prune_job.rs | 11 ++++
>> src/server/pull.rs | 8 ++-
>> src/server/push.rs | 6 +++
>> 10 files changed, 193 insertions(+), 21 deletions(-)
>>
>> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/
>> backup_info.rs
>> index c33eb307..476daa61 100644
>> --- a/pbs-datastore/src/backup_info.rs
>> +++ b/pbs-datastore/src/backup_info.rs
>> @@ -937,6 +937,98 @@ fn lock_file_path_helper(ns: &BackupNamespace,
>> path: PathBuf) -> PathBuf {
>> to_return.join(format!("{first_eigthy}...{last_eighty}-{hash}"))
>> }
>> +/// Returns the lock file path for a backup namespace.
>> +///
>> +/// The lock file will be located at:
>> +/// `${DATASTORE_LOCKS_DIR}/${store_name}/${ns_colon_encoded}/.ns-lock`
>> +pub(crate) fn ns_lock_path(store_name: &str, ns: &BackupNamespace) ->
>> PathBuf {
>> + let ns_part = ns
>> + .components()
>> + .map(String::from)
>> + .reduce(|acc, n| format!("{acc}:{n}"))
>> + .unwrap_or_default();
>> + Path::new(DATASTORE_LOCKS_DIR)
>> + .join(store_name)
>> + .join(ns_part)
>> + .join(".ns-lock")
>> +}
>> +
>> +/// Holds namespace locks acquired for a structural operation or a
>> read/write operation.
>> +///
>> +/// For exclusive locks the first guard is exclusive (on the
>> namespace itself), and any remaining
>> +/// guards are shared (on ancestor namespaces). For shared locks all
>> guards are shared.
>> +pub struct NamespaceLockGuard {
>> + _guards: Vec<BackupLockGuard>,
>> +}
>
> comment: While it might not be that critical in practice, it probably
> makes sense to also control the order with which the lock guards are
> dropped by implementing the Drop trait and consuming the vector in order
> to avoid possible inconsistencies with the assumed order.
>
makes sense, will add in v5, thanks!
>> +
>> +fn lock_namespace_exclusive_single(
>> + store_name: &str,
>> + ns: &BackupNamespace,
>> +) -> Result<BackupLockGuard, Error> {
>> + let path = ns_lock_path(store_name, ns);
>> + lock_helper(store_name, &path, |p| {
>> + open_backup_lockfile(p, Some(Duration::from_secs(0)), true)
>> + .with_context(|| format!("unable to acquire exclusive
>> namespace lock for '{ns}'"))
>> + })
>> +}
>> +
>> +fn lock_namespace_shared_single(
>> + store_name: &str,
>> + ns: &BackupNamespace,
>> +) -> Result<BackupLockGuard, Error> {
>> + let path = ns_lock_path(store_name, ns);
>> + lock_helper(store_name, &path, |p| {
>> + open_backup_lockfile(p, Some(Duration::from_secs(0)), false)
>> + .with_context(|| format!("unable to acquire shared
>> namespace lock for '{ns}'"))
>> + })
>> +}
>> +
>> +/// Acquires an exclusive lock on `ns` and shared locks on all its
>> non-root ancestors.
>> +///
>> +/// Used by operations that structurally modify a namespace (e.g.
>> move). The shared ancestor locks
>> +/// ensure that a concurrent structural operation on any ancestor
>> namespace blocks until this one
>> +/// completes, and vice versa - mirroring the hierarchical behavior
>> of `lock_namespace_shared`.
>> +pub(crate) fn lock_namespace(
>> + store_name: &str,
>> + ns: &BackupNamespace,
>> +) -> Result<NamespaceLockGuard, Error> {
>> + // Acquire the exclusive lock on ns first, then shared locks on
>> ancestors.
>> + // Order matters: taking exclusive before shared avoids a
>> scenario where we hold shared on
>> + // an ancestor and then block waiting for exclusive on ns (which
>> another holder of that
>> + // ancestor's shared lock might be waiting to promote - not
>> currently possible but fragile).
>> + let exclusive = lock_namespace_exclusive_single(store_name, ns)?;
>> + let mut guards = vec![exclusive];
>> + let mut current = ns.clone();
>> + while !current.is_root() {
>> + current = current.parent();
>> + if !current.is_root() {
>> + guards.push(lock_namespace_shared_single(store_name,
>> ¤t)?);
>> + }
>> + }
>> + Ok(NamespaceLockGuard { _guards: guards })
>> +}
>> +
>> +/// Acquires shared locks on a namespace and all its non-root ancestors.
>> +///
>> +/// Held by operations that read from or write into a namespace
>> (backup, sync, verify, prune),
>> +/// preventing concurrent exclusive operations (e.g. move) on the
>> namespace itself or any ancestor
>> +/// from proceeding while this guard is alive.
>> +///
>> +/// Locking up the ancestor chain (bounded by max namespace depth)
>> rather than down the subtree
>> +/// keeps the cost O(depth) regardless of how wide the namespace tree
>> is.
>> +pub(crate) fn lock_namespace_shared(
>> + store_name: &str,
>> + ns: &BackupNamespace,
>> +) -> Result<NamespaceLockGuard, Error> {
>> + let mut guards = Vec::new();
>> + let mut current = ns.clone();
>> + while !current.is_root() {
>> + guards.push(lock_namespace_shared_single(store_name,
>> ¤t)?);
>> + current = current.parent();
>> + }
>> + Ok(NamespaceLockGuard { _guards: guards })
>> +}
>> +
>> /// Helps implement the double stat'ing procedure. It avoids certain
>> race conditions upon lock
>> /// deletion.
>> ///
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/
>> datastore.rs
>> index ef378c69..564c44a1 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -38,7 +38,8 @@ use pbs_config::{BackupLockGuard, ConfigVersionCache};
>> use proxmox_section_config::SectionConfigData;
>> use crate::backup_info::{
>> - BackupDir, BackupGroup, BackupInfo, OLD_LOCKING,
>> PROTECTED_MARKER_FILENAME,
>> + lock_namespace, lock_namespace_shared, BackupDir, BackupGroup,
>> BackupInfo, NamespaceLockGuard,
>> + OLD_LOCKING, PROTECTED_MARKER_FILENAME,
>> };
>> use crate::chunk_store::ChunkStore;
>> use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
>> @@ -1123,18 +1124,46 @@ impl DataStore {
>> Ok(())
>> }
>> - /// Create (if it does not already exists) and lock a backup group
>> + /// Acquires an exclusive lock on a backup namespace and shared
>> locks on all its ancestors.
>> ///
>> - /// And set the owner to 'userid'. If the group already exists,
>> it returns the
>> - /// current owner (instead of setting the owner).
>> + /// Operations that structurally modify a namespace (move,
>> delete) must hold this for their
>> + /// duration to prevent concurrent readers or writers from
>> accessing the namespace while it is
>> + /// being relocated or destroyed.
>> + pub fn lock_namespace(
>> + self: &Arc<Self>,
>> + ns: &BackupNamespace,
>> + ) -> Result<NamespaceLockGuard, Error> {
>> + lock_namespace(self.name(), ns)
>> + }
>> +
>> + /// Acquires shared locks on a backup namespace and all its non-
>> root ancestors.
>> ///
>> - /// This also acquires an exclusive lock on the directory and
>> returns the lock guard.
>> + /// Operations that read from or write into a namespace (backup,
>> sync, verify, prune) must hold
>> + /// this for their duration to prevent a concurrent
>> `move_namespace` or `delete_namespace` from
>> + /// relocating or destroying the namespace under them.
>> + pub fn lock_namespace_shared(
>> + self: &Arc<Self>,
>> + ns: &BackupNamespace,
>> + ) -> Result<NamespaceLockGuard, Error> {
>> + lock_namespace_shared(self.name(), ns)
>> + }
>> +
>> + /// Create (if it does not already exist) and lock a backup group.
>> + ///
>> + /// Sets the owner to `auth_id`. If the group already exists,
>> returns the current owner.
>> + ///
>> + /// Returns `(owner, ns_lock, group_lock)`. Both locks must be
>> kept alive for the duration of
>> + /// the backup session: the shared namespace lock prevents
>> concurrent namespace moves or
>> + /// deletions, and the exclusive group lock prevents concurrent
>> backups to the same group.
>> pub fn create_locked_backup_group(
>> self: &Arc<Self>,
>> ns: &BackupNamespace,
>> backup_group: &pbs_api_types::BackupGroup,
>> auth_id: &Authid,
>> - ) -> Result<(Authid, BackupLockGuard), Error> {
>> + ) -> Result<(Authid, NamespaceLockGuard, BackupLockGuard), Error> {
>> + let ns_guard = lock_namespace_shared(self.name(), ns)
>> + .with_context(|| format!("failed to acquire shared
>> namespace lock for '{ns}'"))?;
>> +
>> let backup_group = self.backup_group(ns.clone(),
>> backup_group.clone());
>> // create intermediate path first
>> @@ -1155,14 +1184,14 @@ impl DataStore {
>> return Err(err);
>> }
>> let owner = self.get_owner(ns,
>> backup_group.group())?; // just to be sure
>> - Ok((owner, guard))
>> + Ok((owner, ns_guard, guard))
>> }
>> Err(ref err) if err.kind() ==
>> io::ErrorKind::AlreadyExists => {
>> let guard = backup_group.lock().with_context(|| {
>> format!("while creating locked backup group
>> '{backup_group:?}'")
>> })?;
>> let owner = self.get_owner(ns,
>> backup_group.group())?; // just to be sure
>> - Ok((owner, guard))
>> + Ok((owner, ns_guard, guard))
>> }
>> Err(err) => bail!("unable to create backup group {:?} -
>> {}", full_path, err),
>> }
>> diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
>> index 30e24d8d..ec913001 100644
>> --- a/src/api2/admin/namespace.rs
>> +++ b/src/api2/admin/namespace.rs
>> @@ -1,4 +1,4 @@
>> -use anyhow::{bail, Error};
>> +use anyhow::{bail, Context, Error};
>> use pbs_config::CachedUserInfo;
>> use proxmox_router::{http_bail, ApiMethod, Permission, Router,
>> RpcEnvironment};
>> @@ -164,6 +164,10 @@ pub fn delete_namespace(
>> let datastore = DataStore::lookup_datastore(&store,
>> Operation::Write)?;
>> + let _ns_lock = datastore
>> + .lock_namespace(&ns)
>> + .with_context(|| format!("failed to lock namespace '{ns}' for
>> deletion"))?;
>> +
>> let (removed_all, stats) =
>> datastore.remove_namespace_recursive(&ns, delete_groups)?;
>> if !removed_all {
>> let err_msg = if delete_groups {
>> diff --git a/src/api2/backup/environment.rs b/src/api2/backup/
>> environment.rs
>> index ab623f1f..44483add 100644
>> --- a/src/api2/backup/environment.rs
>> +++ b/src/api2/backup/environment.rs
>> @@ -1,5 +1,6 @@
>> use anyhow::{bail, format_err, Context, Error};
>> use pbs_config::BackupLockGuard;
>> +use pbs_datastore::backup_info::NamespaceLockGuard;
>> use proxmox_sys::process_locker::ProcessLockSharedGuard;
>> use std::collections::HashMap;
>> @@ -100,6 +101,7 @@ struct SharedBackupState {
>> pub struct BackupLockGuards {
>> previous_snapshot: Option<BackupLockGuard>,
>> + _namespace: Option<NamespaceLockGuard>,
>> group: Option<BackupLockGuard>,
>> snapshot: Option<BackupLockGuard>,
>> chunk_store: Option<ProcessLockSharedGuard>,
>> @@ -108,12 +110,14 @@ pub struct BackupLockGuards {
>> impl BackupLockGuards {
>> pub(crate) fn new(
>> previous_snapshot: Option<BackupLockGuard>,
>> + namespace: NamespaceLockGuard,
>> group: BackupLockGuard,
>> snapshot: BackupLockGuard,
>> chunk_store: ProcessLockSharedGuard,
>> ) -> Self {
>> Self {
>> previous_snapshot,
>> + _namespace: Some(namespace),
>> group: Some(group),
>> snapshot: Some(snapshot),
>> chunk_store: Some(chunk_store),
>> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
>> index 6df0d34b..80e70dac 100644
>> --- a/src/api2/backup/mod.rs
>> +++ b/src/api2/backup/mod.rs
>> @@ -143,8 +143,9 @@ fn upgrade_to_backup_protocol(
>> "backup"
>> };
>> - // lock backup group to only allow one backup per group at
>> a time
>> - let (owner, group_guard) = datastore.create_locked_backup_group(
>> + // lock backup group to only allow one backup per group at a
>> time,
>> + // also acquires a shared namespace lock to prevent
>> concurrent namespace moves
>> + let (owner, ns_guard, group_guard) =
>> datastore.create_locked_backup_group(
>> backup_group.backup_ns(),
>> backup_group.as_ref(),
>> &auth_id,
>> @@ -215,8 +216,13 @@ fn upgrade_to_backup_protocol(
>> // case of errors. The former is required for
>> immediate subsequent backups (e.g.
>> // during a push sync) to be able to lock the group
>> and snapshots.
>> let chunk_store_guard =
>> datastore.try_shared_chunk_store_lock()?;
>> - let backup_lock_guards =
>> - BackupLockGuards::new(last_guard, group_guard,
>> snap_guard, chunk_store_guard);
>> + let backup_lock_guards = BackupLockGuards::new(
>> + last_guard,
>> + ns_guard,
>> + group_guard,
>> + snap_guard,
>> + chunk_store_guard,
>> + );
>> let mut env = BackupEnvironment::new(
>> env_type,
>> diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
>> index 92529a76..1cb85f2d 100644
>> --- a/src/api2/tape/restore.rs
>> +++ b/src/api2/tape/restore.rs
>> @@ -848,11 +848,8 @@ fn restore_list_worker(
>> Some(restore_owner),
>> )?;
>> - let (owner, _group_lock) =
>> datastore.create_locked_backup_group(
>> - &ns,
>> - backup_dir.as_ref(),
>> - restore_owner,
>> - )?;
>> + let (owner, _ns_guard, _group_lock) = datastore
>> + .create_locked_backup_group(&ns,
>> backup_dir.as_ref(), restore_owner)?;
>> if restore_owner != &owner {
>> bail!(
>> "cannot restore snapshot
>> '{snapshot}' into group '{}', owner check \
>> @@ -1354,7 +1351,7 @@ fn restore_archive<'a>(
>> auth_id,
>> Some(restore_owner),
>> )?;
>> - let (owner, _group_lock) =
>> datastore.create_locked_backup_group(
>> + let (owner, _ns_guard, _group_lock) =
>> datastore.create_locked_backup_group(
>> &backup_ns,
>> backup_dir.as_ref(),
>> restore_owner,
>> diff --git a/src/backup/verify.rs b/src/backup/verify.rs
>> index f52d7781..ce5d6f69 100644
>> --- a/src/backup/verify.rs
>> +++ b/src/backup/verify.rs
>> @@ -393,10 +393,27 @@ impl VerifyWorker {
>> return Ok(true);
>> }
>> + let ns_lock = match
>> self.datastore.lock_namespace_shared(backup_dir.backup_ns()) {
>> + Ok(lock) => lock,
>> + Err(err) => {
>> + info!(
>> + "SKIPPED: verify {}:{} - could not acquire
>> namespace lock: {}",
>> + self.datastore.name(),
>> + backup_dir.dir(),
>> + err,
>> + );
>> + return Ok(true);
>> + }
>> + };
>> +
>> let snap_lock = backup_dir.lock_shared();
>> match snap_lock {
>> - Ok(snap_lock) =>
>> self.verify_backup_dir_with_lock(backup_dir, upid, filter, snap_lock),
>> + Ok(snap_lock) => {
>> + let result =
>> self.verify_backup_dir_with_lock(backup_dir, upid, filter, snap_lock);
>> + drop(ns_lock);
>> + result
>> + }
>> Err(err) => {
>> info!(
>> "SKIPPED: verify {}:{} - could not acquire
>> snapshot lock: {}",
>> diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
>> index bb86a323..8103f59c 100644
>> --- a/src/server/prune_job.rs
>> +++ b/src/server/prune_job.rs
>> @@ -54,6 +54,17 @@ pub fn prune_datastore(
>> )? {
>> let group = group?;
>> let ns = group.backup_ns();
>> + let _ns_lock = match datastore.lock_namespace_shared(ns) {
>> + Ok(lock) => lock,
>> + Err(err) => {
>> + warn!(
>> + "skipping prune of group '{}:{}' - could not
>> acquire namespace lock: {err}",
>> + ns,
>> + group.group(),
>> + );
>> + continue;
>> + }
>> + };
>> let list = group.list_backups()?;
>> let mut prune_info = compute_prune_info(list,
>> &prune_options.keep)?;
>> diff --git a/src/server/pull.rs b/src/server/pull.rs
>> index 0ac6b5b8..40173324 100644
>> --- a/src/server/pull.rs
>> +++ b/src/server/pull.rs
>> @@ -1061,6 +1061,12 @@ async fn pull_ns(
>> params: &mut PullParameters,
>> encountered_chunks: Arc<Mutex<EncounteredChunks>>,
>> ) -> Result<(StoreProgress, SyncStats, bool), Error> {
>> + let _ns_lock = params
>> + .target
>> + .store
>> + .lock_namespace_shared(namespace)
>> + .with_context(|| format!("failed to acquire shared namespace
>> lock for '{namespace}'"))?;
>> +
>> let list: Vec<BackupGroup> =
>> params.source.list_groups(namespace, ¶ms.owner).await?;
>> let unfiltered_count = list.len();
>> @@ -1093,7 +1099,7 @@ async fn pull_ns(
>> progress.done_snapshots = 0;
>> progress.group_snapshots = 0;
>> - let (owner, _lock_guard) =
>> + let (owner, _ns_guard, _lock_guard) =
>> match params
>> .target
>> .store
>> diff --git a/src/server/push.rs b/src/server/push.rs
>> index 27c5b22d..47067d66 100644
>> --- a/src/server/push.rs
>> +++ b/src/server/push.rs
>> @@ -525,6 +525,12 @@ pub(crate) async fn push_namespace(
>> check_ns_remote_datastore_privs(params, &target_namespace,
>> PRIV_REMOTE_DATASTORE_BACKUP)
>> .context("Pushing to remote namespace not allowed")?;
>> + let _ns_lock = params
>> + .source
>> + .store
>> + .lock_namespace_shared(namespace)
>> + .with_context(|| format!("failed to acquire shared namespace
>> lock for '{namespace}'"))?;
>> +
>> let mut list: Vec<BackupGroup> = params
>> .source
>> .list_groups(namespace, ¶ms.local_user)
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH proxmox-backup v4 1/7] datastore: add namespace-level locking
2026-03-13 7:40 ` Hannes Laimer
@ 2026-03-13 7:56 ` Christian Ebner
0 siblings, 0 replies; 20+ messages in thread
From: Christian Ebner @ 2026-03-13 7:56 UTC (permalink / raw)
To: Hannes Laimer, pbs-devel
On 3/13/26 8:39 AM, Hannes Laimer wrote:
> On 2026-03-12 16:43, Christian Ebner wrote:
>> One high level comment: I think it would make sense to allow to set an
>> overall timeout when acquiring the namespace locks. Most move operations
>> probably are rather fast anyways, so waiting a few seconds in a e.g.
>> prune job will probably be preferable over parts of the prune job being
>> skipped.
>>
>
> set as in have it configurable? Or just defining one for the locks?
> At least with move in mind, I though that waiting for a lock could mean
> that the things might be gone by the time we get the lock, 50/50 chance
> of that happening actually. So i figured it'd be better to not have
> that, technicaly shouldn't be a problem cause I think we do all
> existance checks after locking, but still, waiting for something that is
> currently being moved (away 1 in 2 times) seemed odd.
>
> not super oppoed though for a few seconds of hard-coded waiting.
Yes, I was more thinking of a small (hard coded) timeout, which is only
used when running inside a worker task where it is okay to wait for a
bit. Similar to what is done when trying to acquire the lock for backups
in PVE. Although I see the point that the entity might not be present
anymore after the timeout. But then at least it is skipped as done
already. But no strong opinion on this either.
>
>> Another comment inline.
>>
>> On 3/11/26 4:13 PM, Hannes Laimer wrote:
>>> Add exclusive/shared namespace locking keyed at
>>> /run/proxmox-backup/locks/{store}/{ns}/.ns-lock.
>>>
>>> Operations that read from or write into a namespace hold a shared lock
>>> for their duration. Structural operations (move, delete) hold an
>>> exclusive lock. The shared lock is hierarchical: locking a/b/c also
>>> locks a/b and a, so an exclusive lock on any ancestor blocks all
>>> active operations below it. Walking up the ancestor chain costs
>>> O(depth), which is bounded by the maximum namespace depth of 8,
>>> whereas locking all descendants would be arbitrarily expensive.
>>>
>>> Backup jobs and pull/push sync acquire the shared lock via
>>> create_locked_backup_group and pull_ns/push_namespace respectively.
>>> Verify and prune acquire it per snapshot/group and skip gracefully if
>>> the lock cannot be taken, since a concurrent move is a transient
>>> condition.
>>>
>>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>>> ---
>>> pbs-datastore/src/backup_info.rs | 92 ++++++++++++++++++++++++++++++++
>>> pbs-datastore/src/datastore.rs | 45 +++++++++++++---
>>> src/api2/admin/namespace.rs | 6 ++-
>>> src/api2/backup/environment.rs | 4 ++
>>> src/api2/backup/mod.rs | 14 +++--
>>> src/api2/tape/restore.rs | 9 ++--
>>> src/backup/verify.rs | 19 ++++++-
>>> src/server/prune_job.rs | 11 ++++
>>> src/server/pull.rs | 8 ++-
>>> src/server/push.rs | 6 +++
>>> 10 files changed, 193 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/
>>> backup_info.rs
>>> index c33eb307..476daa61 100644
>>> --- a/pbs-datastore/src/backup_info.rs
>>> +++ b/pbs-datastore/src/backup_info.rs
>>> @@ -937,6 +937,98 @@ fn lock_file_path_helper(ns: &BackupNamespace,
>>> path: PathBuf) -> PathBuf {
>>> to_return.join(format!("{first_eigthy}...{last_eighty}-{hash}"))
>>> }
>>> +/// Returns the lock file path for a backup namespace.
>>> +///
>>> +/// The lock file will be located at:
>>> +/// `${DATASTORE_LOCKS_DIR}/${store_name}/${ns_colon_encoded}/.ns-lock`
>>> +pub(crate) fn ns_lock_path(store_name: &str, ns: &BackupNamespace) ->
>>> PathBuf {
>>> + let ns_part = ns
>>> + .components()
>>> + .map(String::from)
>>> + .reduce(|acc, n| format!("{acc}:{n}"))
>>> + .unwrap_or_default();
>>> + Path::new(DATASTORE_LOCKS_DIR)
>>> + .join(store_name)
>>> + .join(ns_part)
>>> + .join(".ns-lock")
>>> +}
>>> +
>>> +/// Holds namespace locks acquired for a structural operation or a
>>> read/write operation.
>>> +///
>>> +/// For exclusive locks the first guard is exclusive (on the
>>> namespace itself), and any remaining
>>> +/// guards are shared (on ancestor namespaces). For shared locks all
>>> guards are shared.
>>> +pub struct NamespaceLockGuard {
>>> + _guards: Vec<BackupLockGuard>,
>>> +}
>>
>> comment: While it might not be that critical in practice, it probably
>> makes sense to also control the order with which the lock guards are
>> dropped by implementing the Drop trait and consuming the vector in order
>> to avoid possible inconsistencies with the assumed order.
>>
>
> makes sense, will add in v5, thanks!
>
>>> +
>>> +fn lock_namespace_exclusive_single(
>>> + store_name: &str,
>>> + ns: &BackupNamespace,
>>> +) -> Result<BackupLockGuard, Error> {
>>> + let path = ns_lock_path(store_name, ns);
>>> + lock_helper(store_name, &path, |p| {
>>> + open_backup_lockfile(p, Some(Duration::from_secs(0)), true)
>>> + .with_context(|| format!("unable to acquire exclusive
>>> namespace lock for '{ns}'"))
>>> + })
>>> +}
>>> +
>>> +fn lock_namespace_shared_single(
>>> + store_name: &str,
>>> + ns: &BackupNamespace,
>>> +) -> Result<BackupLockGuard, Error> {
>>> + let path = ns_lock_path(store_name, ns);
>>> + lock_helper(store_name, &path, |p| {
>>> + open_backup_lockfile(p, Some(Duration::from_secs(0)), false)
>>> + .with_context(|| format!("unable to acquire shared
>>> namespace lock for '{ns}'"))
>>> + })
>>> +}
>>> +
>>> +/// Acquires an exclusive lock on `ns` and shared locks on all its
>>> non-root ancestors.
>>> +///
>>> +/// Used by operations that structurally modify a namespace (e.g.
>>> move). The shared ancestor locks
>>> +/// ensure that a concurrent structural operation on any ancestor
>>> namespace blocks until this one
>>> +/// completes, and vice versa - mirroring the hierarchical behavior
>>> of `lock_namespace_shared`.
>>> +pub(crate) fn lock_namespace(
>>> + store_name: &str,
>>> + ns: &BackupNamespace,
>>> +) -> Result<NamespaceLockGuard, Error> {
>>> + // Acquire the exclusive lock on ns first, then shared locks on
>>> ancestors.
>>> + // Order matters: taking exclusive before shared avoids a
>>> scenario where we hold shared on
>>> + // an ancestor and then block waiting for exclusive on ns (which
>>> another holder of that
>>> + // ancestor's shared lock might be waiting to promote - not
>>> currently possible but fragile).
>>> + let exclusive = lock_namespace_exclusive_single(store_name, ns)?;
>>> + let mut guards = vec![exclusive];
>>> + let mut current = ns.clone();
>>> + while !current.is_root() {
>>> + current = current.parent();
>>> + if !current.is_root() {
>>> + guards.push(lock_namespace_shared_single(store_name,
>>> ¤t)?);
>>> + }
>>> + }
>>> + Ok(NamespaceLockGuard { _guards: guards })
>>> +}
>>> +
>>> +/// Acquires shared locks on a namespace and all its non-root ancestors.
>>> +///
>>> +/// Held by operations that read from or write into a namespace
>>> (backup, sync, verify, prune),
>>> +/// preventing concurrent exclusive operations (e.g. move) on the
>>> namespace itself or any ancestor
>>> +/// from proceeding while this guard is alive.
>>> +///
>>> +/// Locking up the ancestor chain (bounded by max namespace depth)
>>> rather than down the subtree
>>> +/// keeps the cost O(depth) regardless of how wide the namespace tree
>>> is.
>>> +pub(crate) fn lock_namespace_shared(
>>> + store_name: &str,
>>> + ns: &BackupNamespace,
>>> +) -> Result<NamespaceLockGuard, Error> {
>>> + let mut guards = Vec::new();
>>> + let mut current = ns.clone();
>>> + while !current.is_root() {
>>> + guards.push(lock_namespace_shared_single(store_name,
>>> ¤t)?);
>>> + current = current.parent();
>>> + }
>>> + Ok(NamespaceLockGuard { _guards: guards })
>>> +}
>>> +
>>> /// Helps implement the double stat'ing procedure. It avoids certain
>>> race conditions upon lock
>>> /// deletion.
>>> ///
>>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/
>>> datastore.rs
>>> index ef378c69..564c44a1 100644
>>> --- a/pbs-datastore/src/datastore.rs
>>> +++ b/pbs-datastore/src/datastore.rs
>>> @@ -38,7 +38,8 @@ use pbs_config::{BackupLockGuard, ConfigVersionCache};
>>> use proxmox_section_config::SectionConfigData;
>>> use crate::backup_info::{
>>> - BackupDir, BackupGroup, BackupInfo, OLD_LOCKING,
>>> PROTECTED_MARKER_FILENAME,
>>> + lock_namespace, lock_namespace_shared, BackupDir, BackupGroup,
>>> BackupInfo, NamespaceLockGuard,
>>> + OLD_LOCKING, PROTECTED_MARKER_FILENAME,
>>> };
>>> use crate::chunk_store::ChunkStore;
>>> use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
>>> @@ -1123,18 +1124,46 @@ impl DataStore {
>>> Ok(())
>>> }
>>> - /// Create (if it does not already exists) and lock a backup group
>>> + /// Acquires an exclusive lock on a backup namespace and shared
>>> locks on all its ancestors.
>>> ///
>>> - /// And set the owner to 'userid'. If the group already exists,
>>> it returns the
>>> - /// current owner (instead of setting the owner).
>>> + /// Operations that structurally modify a namespace (move,
>>> delete) must hold this for their
>>> + /// duration to prevent concurrent readers or writers from
>>> accessing the namespace while it is
>>> + /// being relocated or destroyed.
>>> + pub fn lock_namespace(
>>> + self: &Arc<Self>,
>>> + ns: &BackupNamespace,
>>> + ) -> Result<NamespaceLockGuard, Error> {
>>> + lock_namespace(self.name(), ns)
>>> + }
>>> +
>>> + /// Acquires shared locks on a backup namespace and all its non-
>>> root ancestors.
>>> ///
>>> - /// This also acquires an exclusive lock on the directory and
>>> returns the lock guard.
>>> + /// Operations that read from or write into a namespace (backup,
>>> sync, verify, prune) must hold
>>> + /// this for their duration to prevent a concurrent
>>> `move_namespace` or `delete_namespace` from
>>> + /// relocating or destroying the namespace under them.
>>> + pub fn lock_namespace_shared(
>>> + self: &Arc<Self>,
>>> + ns: &BackupNamespace,
>>> + ) -> Result<NamespaceLockGuard, Error> {
>>> + lock_namespace_shared(self.name(), ns)
>>> + }
>>> +
>>> + /// Create (if it does not already exist) and lock a backup group.
>>> + ///
>>> + /// Sets the owner to `auth_id`. If the group already exists,
>>> returns the current owner.
>>> + ///
>>> + /// Returns `(owner, ns_lock, group_lock)`. Both locks must be
>>> kept alive for the duration of
>>> + /// the backup session: the shared namespace lock prevents
>>> concurrent namespace moves or
>>> + /// deletions, and the exclusive group lock prevents concurrent
>>> backups to the same group.
>>> pub fn create_locked_backup_group(
>>> self: &Arc<Self>,
>>> ns: &BackupNamespace,
>>> backup_group: &pbs_api_types::BackupGroup,
>>> auth_id: &Authid,
>>> - ) -> Result<(Authid, BackupLockGuard), Error> {
>>> + ) -> Result<(Authid, NamespaceLockGuard, BackupLockGuard), Error> {
>>> + let ns_guard = lock_namespace_shared(self.name(), ns)
>>> + .with_context(|| format!("failed to acquire shared
>>> namespace lock for '{ns}'"))?;
>>> +
>>> let backup_group = self.backup_group(ns.clone(),
>>> backup_group.clone());
>>> // create intermediate path first
>>> @@ -1155,14 +1184,14 @@ impl DataStore {
>>> return Err(err);
>>> }
>>> let owner = self.get_owner(ns,
>>> backup_group.group())?; // just to be sure
>>> - Ok((owner, guard))
>>> + Ok((owner, ns_guard, guard))
>>> }
>>> Err(ref err) if err.kind() ==
>>> io::ErrorKind::AlreadyExists => {
>>> let guard = backup_group.lock().with_context(|| {
>>> format!("while creating locked backup group
>>> '{backup_group:?}'")
>>> })?;
>>> let owner = self.get_owner(ns,
>>> backup_group.group())?; // just to be sure
>>> - Ok((owner, guard))
>>> + Ok((owner, ns_guard, guard))
>>> }
>>> Err(err) => bail!("unable to create backup group {:?} -
>>> {}", full_path, err),
>>> }
>>> diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
>>> index 30e24d8d..ec913001 100644
>>> --- a/src/api2/admin/namespace.rs
>>> +++ b/src/api2/admin/namespace.rs
>>> @@ -1,4 +1,4 @@
>>> -use anyhow::{bail, Error};
>>> +use anyhow::{bail, Context, Error};
>>> use pbs_config::CachedUserInfo;
>>> use proxmox_router::{http_bail, ApiMethod, Permission, Router,
>>> RpcEnvironment};
>>> @@ -164,6 +164,10 @@ pub fn delete_namespace(
>>> let datastore = DataStore::lookup_datastore(&store,
>>> Operation::Write)?;
>>> + let _ns_lock = datastore
>>> + .lock_namespace(&ns)
>>> + .with_context(|| format!("failed to lock namespace '{ns}' for
>>> deletion"))?;
>>> +
>>> let (removed_all, stats) =
>>> datastore.remove_namespace_recursive(&ns, delete_groups)?;
>>> if !removed_all {
>>> let err_msg = if delete_groups {
>>> diff --git a/src/api2/backup/environment.rs b/src/api2/backup/
>>> environment.rs
>>> index ab623f1f..44483add 100644
>>> --- a/src/api2/backup/environment.rs
>>> +++ b/src/api2/backup/environment.rs
>>> @@ -1,5 +1,6 @@
>>> use anyhow::{bail, format_err, Context, Error};
>>> use pbs_config::BackupLockGuard;
>>> +use pbs_datastore::backup_info::NamespaceLockGuard;
>>> use proxmox_sys::process_locker::ProcessLockSharedGuard;
>>> use std::collections::HashMap;
>>> @@ -100,6 +101,7 @@ struct SharedBackupState {
>>> pub struct BackupLockGuards {
>>> previous_snapshot: Option<BackupLockGuard>,
>>> + _namespace: Option<NamespaceLockGuard>,
>>> group: Option<BackupLockGuard>,
>>> snapshot: Option<BackupLockGuard>,
>>> chunk_store: Option<ProcessLockSharedGuard>,
>>> @@ -108,12 +110,14 @@ pub struct BackupLockGuards {
>>> impl BackupLockGuards {
>>> pub(crate) fn new(
>>> previous_snapshot: Option<BackupLockGuard>,
>>> + namespace: NamespaceLockGuard,
>>> group: BackupLockGuard,
>>> snapshot: BackupLockGuard,
>>> chunk_store: ProcessLockSharedGuard,
>>> ) -> Self {
>>> Self {
>>> previous_snapshot,
>>> + _namespace: Some(namespace),
>>> group: Some(group),
>>> snapshot: Some(snapshot),
>>> chunk_store: Some(chunk_store),
>>> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
>>> index 6df0d34b..80e70dac 100644
>>> --- a/src/api2/backup/mod.rs
>>> +++ b/src/api2/backup/mod.rs
>>> @@ -143,8 +143,9 @@ fn upgrade_to_backup_protocol(
>>> "backup"
>>> };
>>> - // lock backup group to only allow one backup per group at
>>> a time
>>> - let (owner, group_guard) = datastore.create_locked_backup_group(
>>> + // lock backup group to only allow one backup per group at a
>>> time,
>>> + // also acquires a shared namespace lock to prevent
>>> concurrent namespace moves
>>> + let (owner, ns_guard, group_guard) =
>>> datastore.create_locked_backup_group(
>>> backup_group.backup_ns(),
>>> backup_group.as_ref(),
>>> &auth_id,
>>> @@ -215,8 +216,13 @@ fn upgrade_to_backup_protocol(
>>> // case of errors. The former is required for
>>> immediate subsequent backups (e.g.
>>> // during a push sync) to be able to lock the group
>>> and snapshots.
>>> let chunk_store_guard =
>>> datastore.try_shared_chunk_store_lock()?;
>>> - let backup_lock_guards =
>>> - BackupLockGuards::new(last_guard, group_guard,
>>> snap_guard, chunk_store_guard);
>>> + let backup_lock_guards = BackupLockGuards::new(
>>> + last_guard,
>>> + ns_guard,
>>> + group_guard,
>>> + snap_guard,
>>> + chunk_store_guard,
>>> + );
>>> let mut env = BackupEnvironment::new(
>>> env_type,
>>> diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
>>> index 92529a76..1cb85f2d 100644
>>> --- a/src/api2/tape/restore.rs
>>> +++ b/src/api2/tape/restore.rs
>>> @@ -848,11 +848,8 @@ fn restore_list_worker(
>>> Some(restore_owner),
>>> )?;
>>> - let (owner, _group_lock) =
>>> datastore.create_locked_backup_group(
>>> - &ns,
>>> - backup_dir.as_ref(),
>>> - restore_owner,
>>> - )?;
>>> + let (owner, _ns_guard, _group_lock) = datastore
>>> + .create_locked_backup_group(&ns,
>>> backup_dir.as_ref(), restore_owner)?;
>>> if restore_owner != &owner {
>>> bail!(
>>> "cannot restore snapshot
>>> '{snapshot}' into group '{}', owner check \
>>> @@ -1354,7 +1351,7 @@ fn restore_archive<'a>(
>>> auth_id,
>>> Some(restore_owner),
>>> )?;
>>> - let (owner, _group_lock) =
>>> datastore.create_locked_backup_group(
>>> + let (owner, _ns_guard, _group_lock) =
>>> datastore.create_locked_backup_group(
>>> &backup_ns,
>>> backup_dir.as_ref(),
>>> restore_owner,
>>> diff --git a/src/backup/verify.rs b/src/backup/verify.rs
>>> index f52d7781..ce5d6f69 100644
>>> --- a/src/backup/verify.rs
>>> +++ b/src/backup/verify.rs
>>> @@ -393,10 +393,27 @@ impl VerifyWorker {
>>> return Ok(true);
>>> }
>>> + let ns_lock = match
>>> self.datastore.lock_namespace_shared(backup_dir.backup_ns()) {
>>> + Ok(lock) => lock,
>>> + Err(err) => {
>>> + info!(
>>> + "SKIPPED: verify {}:{} - could not acquire
>>> namespace lock: {}",
>>> + self.datastore.name(),
>>> + backup_dir.dir(),
>>> + err,
>>> + );
>>> + return Ok(true);
>>> + }
>>> + };
>>> +
>>> let snap_lock = backup_dir.lock_shared();
>>> match snap_lock {
>>> - Ok(snap_lock) =>
>>> self.verify_backup_dir_with_lock(backup_dir, upid, filter, snap_lock),
>>> + Ok(snap_lock) => {
>>> + let result =
>>> self.verify_backup_dir_with_lock(backup_dir, upid, filter, snap_lock);
>>> + drop(ns_lock);
>>> + result
>>> + }
>>> Err(err) => {
>>> info!(
>>> "SKIPPED: verify {}:{} - could not acquire
>>> snapshot lock: {}",
>>> diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
>>> index bb86a323..8103f59c 100644
>>> --- a/src/server/prune_job.rs
>>> +++ b/src/server/prune_job.rs
>>> @@ -54,6 +54,17 @@ pub fn prune_datastore(
>>> )? {
>>> let group = group?;
>>> let ns = group.backup_ns();
>>> + let _ns_lock = match datastore.lock_namespace_shared(ns) {
>>> + Ok(lock) => lock,
>>> + Err(err) => {
>>> + warn!(
>>> + "skipping prune of group '{}:{}' - could not
>>> acquire namespace lock: {err}",
>>> + ns,
>>> + group.group(),
>>> + );
>>> + continue;
>>> + }
>>> + };
>>> let list = group.list_backups()?;
>>> let mut prune_info = compute_prune_info(list,
>>> &prune_options.keep)?;
>>> diff --git a/src/server/pull.rs b/src/server/pull.rs
>>> index 0ac6b5b8..40173324 100644
>>> --- a/src/server/pull.rs
>>> +++ b/src/server/pull.rs
>>> @@ -1061,6 +1061,12 @@ async fn pull_ns(
>>> params: &mut PullParameters,
>>> encountered_chunks: Arc<Mutex<EncounteredChunks>>,
>>> ) -> Result<(StoreProgress, SyncStats, bool), Error> {
>>> + let _ns_lock = params
>>> + .target
>>> + .store
>>> + .lock_namespace_shared(namespace)
>>> + .with_context(|| format!("failed to acquire shared namespace
>>> lock for '{namespace}'"))?;
>>> +
>>> let list: Vec<BackupGroup> =
>>> params.source.list_groups(namespace, ¶ms.owner).await?;
>>> let unfiltered_count = list.len();
>>> @@ -1093,7 +1099,7 @@ async fn pull_ns(
>>> progress.done_snapshots = 0;
>>> progress.group_snapshots = 0;
>>> - let (owner, _lock_guard) =
>>> + let (owner, _ns_guard, _lock_guard) =
>>> match params
>>> .target
>>> .store
>>> diff --git a/src/server/push.rs b/src/server/push.rs
>>> index 27c5b22d..47067d66 100644
>>> --- a/src/server/push.rs
>>> +++ b/src/server/push.rs
>>> @@ -525,6 +525,12 @@ pub(crate) async fn push_namespace(
>>> check_ns_remote_datastore_privs(params, &target_namespace,
>>> PRIV_REMOTE_DATASTORE_BACKUP)
>>> .context("Pushing to remote namespace not allowed")?;
>>> + let _ns_lock = params
>>> + .source
>>> + .store
>>> + .lock_namespace_shared(namespace)
>>> + .with_context(|| format!("failed to acquire shared namespace
>>> lock for '{namespace}'"))?;
>>> +
>>> let mut list: Vec<BackupGroup> = params
>>> .source
>>> .list_groups(namespace, ¶ms.local_user)
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH proxmox-backup v4 1/7] datastore: add namespace-level locking
2026-03-12 15:43 ` Christian Ebner
2026-03-13 7:40 ` Hannes Laimer
@ 2026-03-17 13:03 ` Christian Ebner
1 sibling, 0 replies; 20+ messages in thread
From: Christian Ebner @ 2026-03-17 13:03 UTC (permalink / raw)
To: Hannes Laimer, pbs-devel
On 3/12/26 4:43 PM, Christian Ebner wrote:
> One high level comment: I think it would make sense to allow to set an
> overall timeout when acquiring the namespace locks. Most move operations
> probably are rather fast anyways, so waiting a few seconds in a e.g.
> prune job will probably be preferable over parts of the prune job being
> skipped.
>
> Another comment inline.
>
> On 3/11/26 4:13 PM, Hannes Laimer wrote:
>> Add exclusive/shared namespace locking keyed at
>> /run/proxmox-backup/locks/{store}/{ns}/.ns-lock.
>>
>> Operations that read from or write into a namespace hold a shared lock
>> for their duration. Structural operations (move, delete) hold an
>> exclusive lock. The shared lock is hierarchical: locking a/b/c also
>> locks a/b and a, so an exclusive lock on any ancestor blocks all
>> active operations below it. Walking up the ancestor chain costs
>> O(depth), which is bounded by the maximum namespace depth of 8,
>> whereas locking all descendants would be arbitrarily expensive.
>>
>> Backup jobs and pull/push sync acquire the shared lock via
>> create_locked_backup_group and pull_ns/push_namespace respectively.
>> Verify and prune acquire it per snapshot/group and skip gracefully if
>> the lock cannot be taken, since a concurrent move is a transient
>> condition.
>>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>> pbs-datastore/src/backup_info.rs | 92 ++++++++++++++++++++++++++++++++
>> pbs-datastore/src/datastore.rs | 45 +++++++++++++---
>> src/api2/admin/namespace.rs | 6 ++-
>> src/api2/backup/environment.rs | 4 ++
>> src/api2/backup/mod.rs | 14 +++--
>> src/api2/tape/restore.rs | 9 ++--
>> src/backup/verify.rs | 19 ++++++-
>> src/server/prune_job.rs | 11 ++++
>> src/server/pull.rs | 8 ++-
>> src/server/push.rs | 6 +++
>> 10 files changed, 193 insertions(+), 21 deletions(-)
>>
>> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/
>> backup_info.rs
>> index c33eb307..476daa61 100644
>> --- a/pbs-datastore/src/backup_info.rs
>> +++ b/pbs-datastore/src/backup_info.rs
>> @@ -937,6 +937,98 @@ fn lock_file_path_helper(ns: &BackupNamespace,
>> path: PathBuf) -> PathBuf {
>> to_return.join(format!("{first_eigthy}...{last_eighty}-{hash}"))
>> }
>> +/// Returns the lock file path for a backup namespace.
>> +///
>> +/// The lock file will be located at:
>> +/// `${DATASTORE_LOCKS_DIR}/${store_name}/${ns_colon_encoded}/.ns-lock`
>> +pub(crate) fn ns_lock_path(store_name: &str, ns: &BackupNamespace) ->
>> PathBuf {
>> + let ns_part = ns
>> + .components()
>> + .map(String::from)
>> + .reduce(|acc, n| format!("{acc}:{n}"))
>> + .unwrap_or_default();
>> + Path::new(DATASTORE_LOCKS_DIR)
>> + .join(store_name)
>> + .join(ns_part)
As discussed off-list, the namespace directory itself might be used for
locking instead of a dedicated file, in order to reduce the number of
i-nodes which are located on this tmpfs.
>> + .join(".ns-lock")
>> +}
>> +
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH proxmox-backup v4 2/7] datastore: add move_group
2026-03-11 15:13 [PATCH proxmox-backup v4 0/7] fixes #6195: add support for moving groups and namespaces Hannes Laimer
2026-03-11 15:13 ` [PATCH proxmox-backup v4 1/7] datastore: add namespace-level locking Hannes Laimer
@ 2026-03-11 15:13 ` Hannes Laimer
2026-03-12 16:08 ` Christian Ebner
2026-03-11 15:13 ` [PATCH proxmox-backup v4 3/7] datastore: add move_namespace Hannes Laimer
` (5 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Hannes Laimer @ 2026-03-11 15:13 UTC (permalink / raw)
To: pbs-devel
BackupGroup::move_to() performs the actual group relocation: for the
filesystem backend a single rename(2) moves the group directory
atomically. The orphaned group lock file is removed afterwards. For
the S3 backend objects under the source group prefix are listed,
copied to their destination keys, and then deleted.
DataStore::move_group() is the public entry point. It acquires shared
namespace locks on both source and target namespaces and an exclusive
group lock, validates existence under those locks, ensures the target
type directory exists, then calls BackupGroup::move_to().
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
pbs-datastore/src/backup_info.rs | 143 ++++++++++++++++++++++++++++++-
pbs-datastore/src/datastore.rs | 47 ++++++++++
2 files changed, 189 insertions(+), 1 deletion(-)
diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index 476daa61..fa984289 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -9,7 +9,7 @@ use std::time::Duration;
use anyhow::{bail, format_err, Context, Error};
use const_format::concatcp;
-use proxmox_s3_client::S3PathPrefix;
+use proxmox_s3_client::{S3ObjectKey, S3PathPrefix};
use proxmox_sys::fs::{lock_dir_noblock, lock_dir_noblock_shared, replace_file, CreateOptions};
use proxmox_systemd::escape_unit;
@@ -273,6 +273,147 @@ impl BackupGroup {
Ok(delete_stats)
}
+ /// Move this group to a new namespace.
+ ///
+ /// For the filesystem backend, uses `rename` to atomically relocate the group directory. For
+ /// the S3 backend, copies all objects to the destination prefix first, then renames the local
+ /// cache directory, then deletes the source objects. A copy failure returns an error with the
+ /// group intact at source. A delete failure is logged as a warning - any un-deleted source
+ /// objects are orphaned and must be removed manually.
+ ///
+ /// The caller must have created the target type directory
+ /// (e.g. `{target_ns}/{backup_type}/`) before calling this method.
+ ///
+ /// The caller must hold either an exclusive namespace lock on the source namespace (as in
+ /// `move_namespace`) or both a shared namespace lock and an exclusive group lock (as in
+ /// `move_group`). This is required to prevent concurrent writers from adding objects between
+ /// the S3 copy sweep and the subsequent deletes.
+ pub(crate) fn move_to(
+ &self,
+ target_ns: &BackupNamespace,
+ backend: &DatastoreBackend,
+ ) -> Result<(), Error> {
+ let src_path = self.full_group_path();
+ let target_path = self.store.group_path(target_ns, &self.group);
+
+ log::info!("moving backup group {src_path:?} to {target_path:?}");
+
+ match backend {
+ DatastoreBackend::Filesystem => {
+ std::fs::rename(&src_path, &target_path).with_context(|| {
+ format!("failed to move group {src_path:?} to {target_path:?}")
+ })?;
+ // The caller's lock guard still holds the FD open and will harmlessly fail to
+ // remove this path when dropped.
+ let _ = std::fs::remove_file(self.lock_path());
+ }
+ DatastoreBackend::S3(s3_client) => {
+ // Build S3 key prefixes for source and target groups, e.g.:
+ // src: ".cnt/a/b/vm/100/"
+ // tgt: ".cnt/a/c/vm/100/"
+ let src_rel = self.relative_group_path();
+ let src_rel_str = src_rel
+ .to_str()
+ .ok_or_else(|| format_err!("invalid source group path"))?;
+ let src_prefix_str = format!("{S3_CONTENT_PREFIX}/{src_rel_str}/");
+
+ let mut tgt_rel = target_ns.path();
+ tgt_rel.push(self.group.ty.as_str());
+ tgt_rel.push(&self.group.id);
+ let tgt_rel_str = tgt_rel
+ .to_str()
+ .ok_or_else(|| format_err!("invalid target group path"))?;
+ let tgt_prefix_str = format!("{S3_CONTENT_PREFIX}/{tgt_rel_str}/");
+
+ // S3 list_objects returns full keys with the store name as the leading component,
+ // e.g. "mystore/.cnt/a/b/vm/100/2026-01-01T00:00:00Z/drive-scsi0.img.fidx".
+ // Strip "mystore/" to get the relative key used by copy_object.
+ let store_prefix = format!("{}/", self.store.name());
+
+ log::debug!(
+ "S3 move: listing prefix '{src_prefix_str}', store_prefix='{store_prefix}', tgt_prefix='{tgt_prefix_str}'"
+ );
+
+ // Copy all objects to the target prefix first. No source objects are deleted
+ // until all copies succeed, so a copy failure leaves the group intact at source.
+ let prefix = S3PathPrefix::Some(src_prefix_str.clone());
+ let mut token: Option<String> = None;
+ let mut src_keys = Vec::new();
+
+ loop {
+ let result = proxmox_async::runtime::block_on(
+ s3_client.list_objects_v2(&prefix, token.as_deref()),
+ )
+ .context("failed to list group objects on S3 backend")?;
+
+ log::debug!(
+ "S3 move: listed {} objects (truncated={})",
+ result.contents.len(),
+ result.is_truncated
+ );
+
+ for item in result.contents {
+ let full_key_str: &str = &item.key;
+ log::debug!("S3 move: processing key '{full_key_str}'");
+ let rel_key =
+ full_key_str.strip_prefix(&store_prefix).ok_or_else(|| {
+ format_err!("unexpected key prefix in '{full_key_str}'")
+ })?;
+ let src_key = S3ObjectKey::try_from(rel_key)?;
+
+ // Replace the source group prefix with the target prefix,
+ // keeping the snapshot dir and filename (suffix) intact.
+ let suffix = rel_key
+ .strip_prefix(&src_prefix_str)
+ .ok_or_else(|| format_err!("unexpected key format '{rel_key}'"))?;
+ let dst_key_str = format!("{tgt_prefix_str}{suffix}");
+ let dst_key = S3ObjectKey::try_from(dst_key_str.as_str())?;
+
+ log::debug!("S3 move: copy '{rel_key}' -> '{dst_key_str}'");
+ proxmox_async::runtime::block_on(
+ s3_client.copy_object(src_key.clone(), dst_key),
+ )
+ .with_context(|| format!("failed to copy S3 object '{rel_key}'"))?;
+ src_keys.push(src_key);
+ }
+
+ if result.is_truncated {
+ token = result.next_continuation_token;
+ } else {
+ break;
+ }
+ }
+
+ // All copies succeeded. Rename the local cache directory before deleting source
+ // objects so that the local cache reflects the target state as soon as possible.
+ std::fs::rename(&src_path, &target_path).with_context(|| {
+ format!("failed to move group {src_path:?} to {target_path:?}")
+ })?;
+ let _ = std::fs::remove_file(self.lock_path());
+
+ // Delete source objects. In case of a delete failure the group is already at the
+ // target (S3 copies + local cache). Treat delete failures as warnings so the
+ // caller does not misreport the group as "failed to move".
+ // Un-deleted sources must be removed manually.
+ log::debug!("S3 move: deleting {} source objects", src_keys.len());
+ for src_key in src_keys {
+ log::debug!("S3 move: delete '{src_key:?}'");
+ if let Err(err) =
+ proxmox_async::runtime::block_on(s3_client.delete_object(src_key.clone()))
+ {
+ log::warn!(
+ "S3 move: failed to delete source object '{src_key:?}' \
+ (group already at target, orphaned object requires manual removal): {err:#}"
+ );
+ }
+ }
+ log::info!("moved {}: {}", &self.group.ty, &self.group.id);
+ }
+ }
+
+ Ok(())
+ }
+
/// Helper function, assumes that no more snapshots are present in the group.
fn remove_group_dir(&self) -> Result<(), Error> {
let note_path = self.store.group_notes_path(&self.ns, &self.group);
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 564c44a1..51813acb 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1015,6 +1015,53 @@ impl DataStore {
backup_group.destroy(&self.backend()?)
}
+ /// Move a single backup group to a different namespace within the same datastore.
+ ///
+ /// Acquires shared namespace locks on both the source and target namespaces, and an exclusive
+ /// group lock on the source group to prevent concurrent writes to the same group.
+ pub fn move_group(
+ self: &Arc<Self>,
+ source_ns: &BackupNamespace,
+ group: &pbs_api_types::BackupGroup,
+ target_ns: &BackupNamespace,
+ ) -> Result<(), Error> {
+ if source_ns == target_ns {
+ bail!("source and target namespace must be different");
+ }
+
+ let source_group = self.backup_group(source_ns.clone(), group.clone());
+ let target_group = self.backup_group(target_ns.clone(), group.clone());
+
+ let _src_ns_lock = lock_namespace_shared(self.name(), source_ns)
+ .with_context(|| format!("failed to lock source namespace '{source_ns}'"))?;
+ let _tgt_ns_lock = lock_namespace_shared(self.name(), target_ns)
+ .with_context(|| format!("failed to lock target namespace '{target_ns}'"))?;
+
+ let _group_lock = source_group
+ .lock()
+ .with_context(|| format!("failed to lock group '{group}' for move"))?;
+
+ // Check existence under locks to avoid TOCTOU races with concurrent backups or
+ // namespace operations.
+ if !self.namespace_exists(target_ns) {
+ bail!("target namespace '{target_ns}' does not exist");
+ }
+ if !source_group.exists() {
+ bail!("group '{group}' does not exist in namespace '{source_ns}'");
+ }
+ if target_group.exists() {
+ bail!("group '{group}' already exists in target namespace '{target_ns}'");
+ }
+
+ let backend = self.backend()?;
+
+ std::fs::create_dir_all(self.type_path(target_ns, group.ty)).with_context(|| {
+ format!("failed to create type directory in '{target_ns}' for move")
+ })?;
+
+ source_group.move_to(target_ns, &backend)
+ }
+
/// Remove a backup directory including all content
pub fn remove_backup_dir(
self: &Arc<Self>,
--
2.47.3
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH proxmox-backup v4 2/7] datastore: add move_group
2026-03-11 15:13 ` [PATCH proxmox-backup v4 2/7] datastore: add move_group Hannes Laimer
@ 2026-03-12 16:08 ` Christian Ebner
2026-03-13 7:28 ` Hannes Laimer
0 siblings, 1 reply; 20+ messages in thread
From: Christian Ebner @ 2026-03-12 16:08 UTC (permalink / raw)
To: Hannes Laimer, pbs-devel
one comment inline.
On 3/11/26 4:12 PM, Hannes Laimer wrote:
> BackupGroup::move_to() performs the actual group relocation: for the
> filesystem backend a single rename(2) moves the group directory
> atomically. The orphaned group lock file is removed afterwards. For
> the S3 backend objects under the source group prefix are listed,
> copied to their destination keys, and then deleted.
>
> DataStore::move_group() is the public entry point. It acquires shared
> namespace locks on both source and target namespaces and an exclusive
> group lock, validates existence under those locks, ensures the target
> type directory exists, then calls BackupGroup::move_to().
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> pbs-datastore/src/backup_info.rs | 143 ++++++++++++++++++++++++++++++-
> pbs-datastore/src/datastore.rs | 47 ++++++++++
> 2 files changed, 189 insertions(+), 1 deletion(-)
>
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index 476daa61..fa984289 100644
> --- a/pbs-datastore/src/backup_info.rs
> +++ b/pbs-datastore/src/backup_info.rs
> @@ -9,7 +9,7 @@ use std::time::Duration;
> use anyhow::{bail, format_err, Context, Error};
> use const_format::concatcp;
>
> -use proxmox_s3_client::S3PathPrefix;
> +use proxmox_s3_client::{S3ObjectKey, S3PathPrefix};
> use proxmox_sys::fs::{lock_dir_noblock, lock_dir_noblock_shared, replace_file, CreateOptions};
> use proxmox_systemd::escape_unit;
>
> @@ -273,6 +273,147 @@ impl BackupGroup {
> Ok(delete_stats)
> }
>
> + /// Move this group to a new namespace.
> + ///
> + /// For the filesystem backend, uses `rename` to atomically relocate the group directory. For
> + /// the S3 backend, copies all objects to the destination prefix first, then renames the local
> + /// cache directory, then deletes the source objects. A copy failure returns an error with the
> + /// group intact at source. A delete failure is logged as a warning - any un-deleted source
> + /// objects are orphaned and must be removed manually.
> + ///
> + /// The caller must have created the target type directory
> + /// (e.g. `{target_ns}/{backup_type}/`) before calling this method.
> + ///
> + /// The caller must hold either an exclusive namespace lock on the source namespace (as in
> + /// `move_namespace`) or both a shared namespace lock and an exclusive group lock (as in
> + /// `move_group`). This is required to prevent concurrent writers from adding objects between
> + /// the S3 copy sweep and the subsequent deletes.
> + pub(crate) fn move_to(
> + &self,
> + target_ns: &BackupNamespace,
> + backend: &DatastoreBackend,
> + ) -> Result<(), Error> {
> + let src_path = self.full_group_path();
> + let target_path = self.store.group_path(target_ns, &self.group);
> +
> + log::info!("moving backup group {src_path:?} to {target_path:?}");
> +
> + match backend {
> + DatastoreBackend::Filesystem => {
> + std::fs::rename(&src_path, &target_path).with_context(|| {
> + format!("failed to move group {src_path:?} to {target_path:?}")
> + })?;
> + // The caller's lock guard still holds the FD open and will harmlessly fail to
> + // remove this path when dropped.
> + let _ = std::fs::remove_file(self.lock_path());
> + }
> + DatastoreBackend::S3(s3_client) => {
> + // Build S3 key prefixes for source and target groups, e.g.:
> + // src: ".cnt/a/b/vm/100/"
> + // tgt: ".cnt/a/c/vm/100/"
> + let src_rel = self.relative_group_path();
> + let src_rel_str = src_rel
> + .to_str()
> + .ok_or_else(|| format_err!("invalid source group path"))?;
> + let src_prefix_str = format!("{S3_CONTENT_PREFIX}/{src_rel_str}/");
> +
> + let mut tgt_rel = target_ns.path();
> + tgt_rel.push(self.group.ty.as_str());
> + tgt_rel.push(&self.group.id);
> + let tgt_rel_str = tgt_rel
> + .to_str()
> + .ok_or_else(|| format_err!("invalid target group path"))?;
> + let tgt_prefix_str = format!("{S3_CONTENT_PREFIX}/{tgt_rel_str}/");
> +
> + // S3 list_objects returns full keys with the store name as the leading component,
> + // e.g. "mystore/.cnt/a/b/vm/100/2026-01-01T00:00:00Z/drive-scsi0.img.fidx".
> + // Strip "mystore/" to get the relative key used by copy_object.
> + let store_prefix = format!("{}/", self.store.name());
> +
> + log::debug!(
> + "S3 move: listing prefix '{src_prefix_str}', store_prefix='{store_prefix}', tgt_prefix='{tgt_prefix_str}'"
> + );
> +
> + // Copy all objects to the target prefix first. No source objects are deleted
> + // until all copies succeed, so a copy failure leaves the group intact at source.
> + let prefix = S3PathPrefix::Some(src_prefix_str.clone());
> + let mut token: Option<String> = None;
> + let mut src_keys = Vec::new();
> +
> + loop {
> + let result = proxmox_async::runtime::block_on(
> + s3_client.list_objects_v2(&prefix, token.as_deref()),
> + )
> + .context("failed to list group objects on S3 backend")?;
> +
> + log::debug!(
> + "S3 move: listed {} objects (truncated={})",
> + result.contents.len(),
> + result.is_truncated
> + );
> +
> + for item in result.contents {
> + let full_key_str: &str = &item.key;
> + log::debug!("S3 move: processing key '{full_key_str}'");
> + let rel_key =
> + full_key_str.strip_prefix(&store_prefix).ok_or_else(|| {
> + format_err!("unexpected key prefix in '{full_key_str}'")
> + })?;
> + let src_key = S3ObjectKey::try_from(rel_key)?;
> +
> + // Replace the source group prefix with the target prefix,
> + // keeping the snapshot dir and filename (suffix) intact.
> + let suffix = rel_key
> + .strip_prefix(&src_prefix_str)
> + .ok_or_else(|| format_err!("unexpected key format '{rel_key}'"))?;
> + let dst_key_str = format!("{tgt_prefix_str}{suffix}");
> + let dst_key = S3ObjectKey::try_from(dst_key_str.as_str())?;
> +
> + log::debug!("S3 move: copy '{rel_key}' -> '{dst_key_str}'");
> + proxmox_async::runtime::block_on(
> + s3_client.copy_object(src_key.clone(), dst_key),
> + )
> + .with_context(|| format!("failed to copy S3 object '{rel_key}'"))?;
comment: If there is an error in one of the operations inside the whole
loop logic, we might end up with a lot of redundant objects in the
target prefix.
They could be tried to cleaned up by an
S3Client::delete_objects_by_prefix() call. Although not ideal since it
causes additional API requests and might itself fail.
An additional warning mentioning that there are now orphaned object in
the target prefix for the orphaned objects like below might however be
good in any case.
> + src_keys.push(src_key);
> + }
> +
> + if result.is_truncated {
> + token = result.next_continuation_token;
> + } else {
> + break;
> + }
> + }
> +
> + // All copies succeeded. Rename the local cache directory before deleting source
> + // objects so that the local cache reflects the target state as soon as possible.
> + std::fs::rename(&src_path, &target_path).with_context(|| {
> + format!("failed to move group {src_path:?} to {target_path:?}")
> + })?;
> + let _ = std::fs::remove_file(self.lock_path());
> +
> + // Delete source objects. In case of a delete failure the group is already at the
> + // target (S3 copies + local cache). Treat delete failures as warnings so the
> + // caller does not misreport the group as "failed to move".
> + // Un-deleted sources must be removed manually.
> + log::debug!("S3 move: deleting {} source objects", src_keys.len());
> + for src_key in src_keys {
> + log::debug!("S3 move: delete '{src_key:?}'");
> + if let Err(err) =
> + proxmox_async::runtime::block_on(s3_client.delete_object(src_key.clone()))
> + {
> + log::warn!(
> + "S3 move: failed to delete source object '{src_key:?}' \
> + (group already at target, orphaned object requires manual removal): {err:#}"
> + );
> + }
> + }
> + log::info!("moved {}: {}", &self.group.ty, &self.group.id);
> + }
> + }
> +
> + Ok(())
> + }
> +
> /// Helper function, assumes that no more snapshots are present in the group.
> fn remove_group_dir(&self) -> Result<(), Error> {
> let note_path = self.store.group_notes_path(&self.ns, &self.group);
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 564c44a1..51813acb 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1015,6 +1015,53 @@ impl DataStore {
> backup_group.destroy(&self.backend()?)
> }
>
> + /// Move a single backup group to a different namespace within the same datastore.
> + ///
> + /// Acquires shared namespace locks on both the source and target namespaces, and an exclusive
> + /// group lock on the source group to prevent concurrent writes to the same group.
> + pub fn move_group(
> + self: &Arc<Self>,
> + source_ns: &BackupNamespace,
> + group: &pbs_api_types::BackupGroup,
> + target_ns: &BackupNamespace,
> + ) -> Result<(), Error> {
> + if source_ns == target_ns {
> + bail!("source and target namespace must be different");
> + }
> +
> + let source_group = self.backup_group(source_ns.clone(), group.clone());
> + let target_group = self.backup_group(target_ns.clone(), group.clone());
> +
> + let _src_ns_lock = lock_namespace_shared(self.name(), source_ns)
> + .with_context(|| format!("failed to lock source namespace '{source_ns}'"))?;
> + let _tgt_ns_lock = lock_namespace_shared(self.name(), target_ns)
> + .with_context(|| format!("failed to lock target namespace '{target_ns}'"))?;
> +
> + let _group_lock = source_group
> + .lock()
> + .with_context(|| format!("failed to lock group '{group}' for move"))?;
> +
> + // Check existence under locks to avoid TOCTOU races with concurrent backups or
> + // namespace operations.
> + if !self.namespace_exists(target_ns) {
> + bail!("target namespace '{target_ns}' does not exist");
> + }
> + if !source_group.exists() {
> + bail!("group '{group}' does not exist in namespace '{source_ns}'");
> + }
> + if target_group.exists() {
> + bail!("group '{group}' already exists in target namespace '{target_ns}'");
> + }
> +
> + let backend = self.backend()?;
> +
> + std::fs::create_dir_all(self.type_path(target_ns, group.ty)).with_context(|| {
> + format!("failed to create type directory in '{target_ns}' for move")
> + })?;
> +
> + source_group.move_to(target_ns, &backend)
> + }
> +
> /// Remove a backup directory including all content
> pub fn remove_backup_dir(
> self: &Arc<Self>,
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH proxmox-backup v4 2/7] datastore: add move_group
2026-03-12 16:08 ` Christian Ebner
@ 2026-03-13 7:28 ` Hannes Laimer
2026-03-13 7:52 ` Christian Ebner
0 siblings, 1 reply; 20+ messages in thread
From: Hannes Laimer @ 2026-03-13 7:28 UTC (permalink / raw)
To: Christian Ebner, pbs-devel
On 2026-03-12 17:08, Christian Ebner wrote:
> one comment inline.
>
> On 3/11/26 4:12 PM, Hannes Laimer wrote:
>> BackupGroup::move_to() performs the actual group relocation: for the
>> filesystem backend a single rename(2) moves the group directory
>> atomically. The orphaned group lock file is removed afterwards. For
>> the S3 backend objects under the source group prefix are listed,
>> copied to their destination keys, and then deleted.
>>
>> DataStore::move_group() is the public entry point. It acquires shared
>> namespace locks on both source and target namespaces and an exclusive
>> group lock, validates existence under those locks, ensures the target
>> type directory exists, then calls BackupGroup::move_to().
>>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>> pbs-datastore/src/backup_info.rs | 143 ++++++++++++++++++++++++++++++-
>> pbs-datastore/src/datastore.rs | 47 ++++++++++
>> 2 files changed, 189 insertions(+), 1 deletion(-)
>>
>> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/
>> backup_info.rs
>> index 476daa61..fa984289 100644
>> --- a/pbs-datastore/src/backup_info.rs
>> +++ b/pbs-datastore/src/backup_info.rs
>> @@ -9,7 +9,7 @@ use std::time::Duration;
>> use anyhow::{bail, format_err, Context, Error};
>> use const_format::concatcp;
>> -use proxmox_s3_client::S3PathPrefix;
>> +use proxmox_s3_client::{S3ObjectKey, S3PathPrefix};
>> use proxmox_sys::fs::{lock_dir_noblock, lock_dir_noblock_shared,
>> replace_file, CreateOptions};
>> use proxmox_systemd::escape_unit;
>> @@ -273,6 +273,147 @@ impl BackupGroup {
>> Ok(delete_stats)
>> }
>> + /// Move this group to a new namespace.
>> + ///
>> + /// For the filesystem backend, uses `rename` to atomically
>> relocate the group directory. For
>> + /// the S3 backend, copies all objects to the destination prefix
>> first, then renames the local
>> + /// cache directory, then deletes the source objects. A copy
>> failure returns an error with the
>> + /// group intact at source. A delete failure is logged as a
>> warning - any un-deleted source
>> + /// objects are orphaned and must be removed manually.
>> + ///
>> + /// The caller must have created the target type directory
>> + /// (e.g. `{target_ns}/{backup_type}/`) before calling this method.
>> + ///
>> + /// The caller must hold either an exclusive namespace lock on
>> the source namespace (as in
>> + /// `move_namespace`) or both a shared namespace lock and an
>> exclusive group lock (as in
>> + /// `move_group`). This is required to prevent concurrent writers
>> from adding objects between
>> + /// the S3 copy sweep and the subsequent deletes.
>> + pub(crate) fn move_to(
>> + &self,
>> + target_ns: &BackupNamespace,
>> + backend: &DatastoreBackend,
>> + ) -> Result<(), Error> {
>> + let src_path = self.full_group_path();
>> + let target_path = self.store.group_path(target_ns, &self.group);
>> +
>> + log::info!("moving backup group {src_path:?} to
>> {target_path:?}");
>> +
>> + match backend {
>> + DatastoreBackend::Filesystem => {
>> + std::fs::rename(&src_path,
>> &target_path).with_context(|| {
>> + format!("failed to move group {src_path:?} to
>> {target_path:?}")
>> + })?;
>> + // The caller's lock guard still holds the FD open
>> and will harmlessly fail to
>> + // remove this path when dropped.
>> + let _ = std::fs::remove_file(self.lock_path());
>> + }
>> + DatastoreBackend::S3(s3_client) => {
>> + // Build S3 key prefixes for source and target
>> groups, e.g.:
>> + // src: ".cnt/a/b/vm/100/"
>> + // tgt: ".cnt/a/c/vm/100/"
>> + let src_rel = self.relative_group_path();
>> + let src_rel_str = src_rel
>> + .to_str()
>> + .ok_or_else(|| format_err!("invalid source group
>> path"))?;
>> + let src_prefix_str = format!("{S3_CONTENT_PREFIX}/
>> {src_rel_str}/");
>> +
>> + let mut tgt_rel = target_ns.path();
>> + tgt_rel.push(self.group.ty.as_str());
>> + tgt_rel.push(&self.group.id);
>> + let tgt_rel_str = tgt_rel
>> + .to_str()
>> + .ok_or_else(|| format_err!("invalid target group
>> path"))?;
>> + let tgt_prefix_str = format!("{S3_CONTENT_PREFIX}/
>> {tgt_rel_str}/");
>> +
>> + // S3 list_objects returns full keys with the store
>> name as the leading component,
>> + // e.g. "mystore/.cnt/a/b/
>> vm/100/2026-01-01T00:00:00Z/drive-scsi0.img.fidx".
>> + // Strip "mystore/" to get the relative key used by
>> copy_object.
>> + let store_prefix = format!("{}/", self.store.name());
>> +
>> + log::debug!(
>> + "S3 move: listing prefix '{src_prefix_str}',
>> store_prefix='{store_prefix}', tgt_prefix='{tgt_prefix_str}'"
>> + );
>> +
>> + // Copy all objects to the target prefix first. No
>> source objects are deleted
>> + // until all copies succeed, so a copy failure leaves
>> the group intact at source.
>> + let prefix = S3PathPrefix::Some(src_prefix_str.clone());
>> + let mut token: Option<String> = None;
>> + let mut src_keys = Vec::new();
>> +
>> + loop {
>> + let result = proxmox_async::runtime::block_on(
>> + s3_client.list_objects_v2(&prefix,
>> token.as_deref()),
>> + )
>> + .context("failed to list group objects on S3
>> backend")?;
>> +
>> + log::debug!(
>> + "S3 move: listed {} objects (truncated={})",
>> + result.contents.len(),
>> + result.is_truncated
>> + );
>> +
>> + for item in result.contents {
>> + let full_key_str: &str = &item.key;
>> + log::debug!("S3 move: processing key
>> '{full_key_str}'");
>> + let rel_key =
>> +
>> full_key_str.strip_prefix(&store_prefix).ok_or_else(|| {
>> + format_err!("unexpected key prefix in
>> '{full_key_str}'")
>> + })?;
>> + let src_key = S3ObjectKey::try_from(rel_key)?;
>> +
>> + // Replace the source group prefix with the
>> target prefix,
>> + // keeping the snapshot dir and filename
>> (suffix) intact.
>> + let suffix = rel_key
>> + .strip_prefix(&src_prefix_str)
>> + .ok_or_else(|| format_err!("unexpected
>> key format '{rel_key}'"))?;
>> + let dst_key_str = format!("{tgt_prefix_str}
>> {suffix}");
>> + let dst_key =
>> S3ObjectKey::try_from(dst_key_str.as_str())?;
>> +
>> + log::debug!("S3 move: copy '{rel_key}' ->
>> '{dst_key_str}'");
>> + proxmox_async::runtime::block_on(
>> + s3_client.copy_object(src_key.clone(),
>> dst_key),
>> + )
>> + .with_context(|| format!("failed to copy S3
>> object '{rel_key}'"))?;
>
> comment: If there is an error in one of the operations inside the whole
> loop logic, we might end up with a lot of redundant objects in the
> target prefix.
> They could be tried to cleaned up by an
> S3Client::delete_objects_by_prefix() call. Although not ideal since it
> causes additional API requests and might itself fail.
>
> An additional warning mentioning that there are now orphaned object in
> the target prefix for the orphaned objects like below might however be
> good in any case.
Yes, an explicit warning about the group would make sense, cleanup
should be somewhat simple cause the dir exists so just deleting it
through the UI will be enough.
As you said doing the cleanup here may fail as well, so I think stopping
to right right here is as good of a place as any other.
Would a s3-refresh after a move that contains at least one failed group
make sense? So we are sure the UI does show an up-to-date view, and
deleting left-overs of any failed copies is easy/possible.
>
>> + src_keys.push(src_key);
>> + }
>> +
>> + if result.is_truncated {
>> + token = result.next_continuation_token;
>> + } else {
>> + break;
>> + }
>> + }
>> +
>> + // All copies succeeded. Rename the local cache
>> directory before deleting source
>> + // objects so that the local cache reflects the
>> target state as soon as possible.
>> + std::fs::rename(&src_path,
>> &target_path).with_context(|| {
>> + format!("failed to move group {src_path:?} to
>> {target_path:?}")
>> + })?;
>> + let _ = std::fs::remove_file(self.lock_path());
>> +
>> + // Delete source objects. In case of a delete failure
>> the group is already at the
>> + // target (S3 copies + local cache). Treat delete
>> failures as warnings so the
>> + // caller does not misreport the group as "failed to
>> move".
>> + // Un-deleted sources must be removed manually.
>> + log::debug!("S3 move: deleting {} source objects",
>> src_keys.len());
>> + for src_key in src_keys {
>> + log::debug!("S3 move: delete '{src_key:?}'");
>> + if let Err(err) =
>> +
>> proxmox_async::runtime::block_on(s3_client.delete_object(src_key.clone()))
>> + {
>> + log::warn!(
>> + "S3 move: failed to delete source object
>> '{src_key:?}' \
>> + (group already at target, orphaned object
>> requires manual removal): {err:#}"
>> + );
>> + }
>> + }
>> + log::info!("moved {}: {}", &self.group.ty,
>> &self.group.id);
>> + }
>> + }
>> +
>> + Ok(())
>> + }
>> +
>> /// Helper function, assumes that no more snapshots are present
>> in the group.
>> fn remove_group_dir(&self) -> Result<(), Error> {
>> let note_path = self.store.group_notes_path(&self.ns,
>> &self.group);
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/
>> datastore.rs
>> index 564c44a1..51813acb 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -1015,6 +1015,53 @@ impl DataStore {
>> backup_group.destroy(&self.backend()?)
>> }
>> + /// Move a single backup group to a different namespace within
>> the same datastore.
>> + ///
>> + /// Acquires shared namespace locks on both the source and target
>> namespaces, and an exclusive
>> + /// group lock on the source group to prevent concurrent writes
>> to the same group.
>> + pub fn move_group(
>> + self: &Arc<Self>,
>> + source_ns: &BackupNamespace,
>> + group: &pbs_api_types::BackupGroup,
>> + target_ns: &BackupNamespace,
>> + ) -> Result<(), Error> {
>> + if source_ns == target_ns {
>> + bail!("source and target namespace must be different");
>> + }
>> +
>> + let source_group = self.backup_group(source_ns.clone(),
>> group.clone());
>> + let target_group = self.backup_group(target_ns.clone(),
>> group.clone());
>> +
>> + let _src_ns_lock = lock_namespace_shared(self.name(), source_ns)
>> + .with_context(|| format!("failed to lock source namespace
>> '{source_ns}'"))?;
>> + let _tgt_ns_lock = lock_namespace_shared(self.name(), target_ns)
>> + .with_context(|| format!("failed to lock target namespace
>> '{target_ns}'"))?;
>> +
>> + let _group_lock = source_group
>> + .lock()
>> + .with_context(|| format!("failed to lock group '{group}'
>> for move"))?;
>> +
>> + // Check existence under locks to avoid TOCTOU races with
>> concurrent backups or
>> + // namespace operations.
>> + if !self.namespace_exists(target_ns) {
>> + bail!("target namespace '{target_ns}' does not exist");
>> + }
>> + if !source_group.exists() {
>> + bail!("group '{group}' does not exist in namespace
>> '{source_ns}'");
>> + }
>> + if target_group.exists() {
>> + bail!("group '{group}' already exists in target namespace
>> '{target_ns}'");
>> + }
>> +
>> + let backend = self.backend()?;
>> +
>> + std::fs::create_dir_all(self.type_path(target_ns,
>> group.ty)).with_context(|| {
>> + format!("failed to create type directory in '{target_ns}'
>> for move")
>> + })?;
>> +
>> + source_group.move_to(target_ns, &backend)
>> + }
>> +
>> /// Remove a backup directory including all content
>> pub fn remove_backup_dir(
>> self: &Arc<Self>,
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH proxmox-backup v4 2/7] datastore: add move_group
2026-03-13 7:28 ` Hannes Laimer
@ 2026-03-13 7:52 ` Christian Ebner
0 siblings, 0 replies; 20+ messages in thread
From: Christian Ebner @ 2026-03-13 7:52 UTC (permalink / raw)
To: Hannes Laimer, pbs-devel
On 3/13/26 8:28 AM, Hannes Laimer wrote:
> On 2026-03-12 17:08, Christian Ebner wrote:
>> one comment inline.
>>
>> On 3/11/26 4:12 PM, Hannes Laimer wrote:
>>> BackupGroup::move_to() performs the actual group relocation: for the
>>> filesystem backend a single rename(2) moves the group directory
>>> atomically. The orphaned group lock file is removed afterwards. For
>>> the S3 backend objects under the source group prefix are listed,
>>> copied to their destination keys, and then deleted.
>>>
>>> DataStore::move_group() is the public entry point. It acquires shared
>>> namespace locks on both source and target namespaces and an exclusive
>>> group lock, validates existence under those locks, ensures the target
>>> type directory exists, then calls BackupGroup::move_to().
>>>
>>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>>> ---
>>> pbs-datastore/src/backup_info.rs | 143 ++++++++++++++++++++++++++++++-
>>> pbs-datastore/src/datastore.rs | 47 ++++++++++
>>> 2 files changed, 189 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/
>>> backup_info.rs
>>> index 476daa61..fa984289 100644
>>> --- a/pbs-datastore/src/backup_info.rs
>>> +++ b/pbs-datastore/src/backup_info.rs
>>> @@ -9,7 +9,7 @@ use std::time::Duration;
>>> use anyhow::{bail, format_err, Context, Error};
>>> use const_format::concatcp;
>>> -use proxmox_s3_client::S3PathPrefix;
>>> +use proxmox_s3_client::{S3ObjectKey, S3PathPrefix};
>>> use proxmox_sys::fs::{lock_dir_noblock, lock_dir_noblock_shared,
>>> replace_file, CreateOptions};
>>> use proxmox_systemd::escape_unit;
>>> @@ -273,6 +273,147 @@ impl BackupGroup {
>>> Ok(delete_stats)
>>> }
>>> + /// Move this group to a new namespace.
>>> + ///
>>> + /// For the filesystem backend, uses `rename` to atomically
>>> relocate the group directory. For
>>> + /// the S3 backend, copies all objects to the destination prefix
>>> first, then renames the local
>>> + /// cache directory, then deletes the source objects. A copy
>>> failure returns an error with the
>>> + /// group intact at source. A delete failure is logged as a
>>> warning - any un-deleted source
>>> + /// objects are orphaned and must be removed manually.
>>> + ///
>>> + /// The caller must have created the target type directory
>>> + /// (e.g. `{target_ns}/{backup_type}/`) before calling this method.
>>> + ///
>>> + /// The caller must hold either an exclusive namespace lock on
>>> the source namespace (as in
>>> + /// `move_namespace`) or both a shared namespace lock and an
>>> exclusive group lock (as in
>>> + /// `move_group`). This is required to prevent concurrent writers
>>> from adding objects between
>>> + /// the S3 copy sweep and the subsequent deletes.
>>> + pub(crate) fn move_to(
>>> + &self,
>>> + target_ns: &BackupNamespace,
>>> + backend: &DatastoreBackend,
>>> + ) -> Result<(), Error> {
>>> + let src_path = self.full_group_path();
>>> + let target_path = self.store.group_path(target_ns, &self.group);
>>> +
>>> + log::info!("moving backup group {src_path:?} to
>>> {target_path:?}");
>>> +
>>> + match backend {
>>> + DatastoreBackend::Filesystem => {
>>> + std::fs::rename(&src_path,
>>> &target_path).with_context(|| {
>>> + format!("failed to move group {src_path:?} to
>>> {target_path:?}")
>>> + })?;
>>> + // The caller's lock guard still holds the FD open
>>> and will harmlessly fail to
>>> + // remove this path when dropped.
>>> + let _ = std::fs::remove_file(self.lock_path());
>>> + }
>>> + DatastoreBackend::S3(s3_client) => {
>>> + // Build S3 key prefixes for source and target
>>> groups, e.g.:
>>> + // src: ".cnt/a/b/vm/100/"
>>> + // tgt: ".cnt/a/c/vm/100/"
>>> + let src_rel = self.relative_group_path();
>>> + let src_rel_str = src_rel
>>> + .to_str()
>>> + .ok_or_else(|| format_err!("invalid source group
>>> path"))?;
>>> + let src_prefix_str = format!("{S3_CONTENT_PREFIX}/
>>> {src_rel_str}/");
>>> +
>>> + let mut tgt_rel = target_ns.path();
>>> + tgt_rel.push(self.group.ty.as_str());
>>> + tgt_rel.push(&self.group.id);
>>> + let tgt_rel_str = tgt_rel
>>> + .to_str()
>>> + .ok_or_else(|| format_err!("invalid target group
>>> path"))?;
>>> + let tgt_prefix_str = format!("{S3_CONTENT_PREFIX}/
>>> {tgt_rel_str}/");
>>> +
>>> + // S3 list_objects returns full keys with the store
>>> name as the leading component,
>>> + // e.g. "mystore/.cnt/a/b/
>>> vm/100/2026-01-01T00:00:00Z/drive-scsi0.img.fidx".
>>> + // Strip "mystore/" to get the relative key used by
>>> copy_object.
>>> + let store_prefix = format!("{}/", self.store.name());
>>> +
>>> + log::debug!(
>>> + "S3 move: listing prefix '{src_prefix_str}',
>>> store_prefix='{store_prefix}', tgt_prefix='{tgt_prefix_str}'"
>>> + );
>>> +
>>> + // Copy all objects to the target prefix first. No
>>> source objects are deleted
>>> + // until all copies succeed, so a copy failure leaves
>>> the group intact at source.
>>> + let prefix = S3PathPrefix::Some(src_prefix_str.clone());
>>> + let mut token: Option<String> = None;
>>> + let mut src_keys = Vec::new();
>>> +
>>> + loop {
>>> + let result = proxmox_async::runtime::block_on(
>>> + s3_client.list_objects_v2(&prefix,
>>> token.as_deref()),
>>> + )
>>> + .context("failed to list group objects on S3
>>> backend")?;
>>> +
>>> + log::debug!(
>>> + "S3 move: listed {} objects (truncated={})",
>>> + result.contents.len(),
>>> + result.is_truncated
>>> + );
>>> +
>>> + for item in result.contents {
>>> + let full_key_str: &str = &item.key;
>>> + log::debug!("S3 move: processing key
>>> '{full_key_str}'");
>>> + let rel_key =
>>> +
>>> full_key_str.strip_prefix(&store_prefix).ok_or_else(|| {
>>> + format_err!("unexpected key prefix in
>>> '{full_key_str}'")
>>> + })?;
>>> + let src_key = S3ObjectKey::try_from(rel_key)?;
>>> +
>>> + // Replace the source group prefix with the
>>> target prefix,
>>> + // keeping the snapshot dir and filename
>>> (suffix) intact.
>>> + let suffix = rel_key
>>> + .strip_prefix(&src_prefix_str)
>>> + .ok_or_else(|| format_err!("unexpected
>>> key format '{rel_key}'"))?;
>>> + let dst_key_str = format!("{tgt_prefix_str}
>>> {suffix}");
>>> + let dst_key =
>>> S3ObjectKey::try_from(dst_key_str.as_str())?;
>>> +
>>> + log::debug!("S3 move: copy '{rel_key}' ->
>>> '{dst_key_str}'");
>>> + proxmox_async::runtime::block_on(
>>> + s3_client.copy_object(src_key.clone(),
>>> dst_key),
>>> + )
>>> + .with_context(|| format!("failed to copy S3
>>> object '{rel_key}'"))?;
>>
>> comment: If there is an error in one of the operations inside the whole
>> loop logic, we might end up with a lot of redundant objects in the
>> target prefix.
>> They could be tried to cleaned up by an
>> S3Client::delete_objects_by_prefix() call. Although not ideal since it
>> causes additional API requests and might itself fail.
>>
>> An additional warning mentioning that there are now orphaned object in
>> the target prefix for the orphaned objects like below might however be
>> good in any case.
>
> Yes, an explicit warning about the group would make sense, cleanup
> should be somewhat simple cause the dir exists so just deleting it
> through the UI will be enough.
>
> As you said doing the cleanup here may fail as well, so I think stopping
> to right right here is as good of a place as any other.
>
> Would a s3-refresh after a move that contains at least one failed group
> make sense? So we are sure the UI does show an up-to-date view, and
> deleting left-overs of any failed copies is easy/possible.
I do not think a full s3_refresh() should be done. That is very
expensive API request wise and needs to puts the datastore into a
maintenance mode. But one could create the (locked) backup group folder
in the local datastore cache already before starting the sync (so the
folder is visible in the UI) and only once the S3 move operation is
finished replace that with the final contents? By this it should be
possible to clean up on failure.
>
>>
>>> + src_keys.push(src_key);
>>> + }
>>> +
>>> + if result.is_truncated {
>>> + token = result.next_continuation_token;
>>> + } else {
>>> + break;
>>> + }
>>> + }
>>> +
>>> + // All copies succeeded. Rename the local cache
>>> directory before deleting source
>>> + // objects so that the local cache reflects the
>>> target state as soon as possible.
>>> + std::fs::rename(&src_path,
>>> &target_path).with_context(|| {
>>> + format!("failed to move group {src_path:?} to
>>> {target_path:?}")
>>> + })?;
>>> + let _ = std::fs::remove_file(self.lock_path());
>>> +
>>> + // Delete source objects. In case of a delete failure
>>> the group is already at the
>>> + // target (S3 copies + local cache). Treat delete
>>> failures as warnings so the
>>> + // caller does not misreport the group as "failed to
>>> move".
>>> + // Un-deleted sources must be removed manually.
>>> + log::debug!("S3 move: deleting {} source objects",
>>> src_keys.len());
>>> + for src_key in src_keys {
>>> + log::debug!("S3 move: delete '{src_key:?}'");
>>> + if let Err(err) =
>>> +
>>> proxmox_async::runtime::block_on(s3_client.delete_object(src_key.clone()))
>>> + {
>>> + log::warn!(
>>> + "S3 move: failed to delete source object
>>> '{src_key:?}' \
>>> + (group already at target, orphaned object
>>> requires manual removal): {err:#}"
>>> + );
>>> + }
>>> + }
>>> + log::info!("moved {}: {}", &self.group.ty,
>>> &self.group.id);
>>> + }
>>> + }
>>> +
>>> + Ok(())
>>> + }
>>> +
>>> /// Helper function, assumes that no more snapshots are present
>>> in the group.
>>> fn remove_group_dir(&self) -> Result<(), Error> {
>>> let note_path = self.store.group_notes_path(&self.ns,
>>> &self.group);
>>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/
>>> datastore.rs
>>> index 564c44a1..51813acb 100644
>>> --- a/pbs-datastore/src/datastore.rs
>>> +++ b/pbs-datastore/src/datastore.rs
>>> @@ -1015,6 +1015,53 @@ impl DataStore {
>>> backup_group.destroy(&self.backend()?)
>>> }
>>> + /// Move a single backup group to a different namespace within
>>> the same datastore.
>>> + ///
>>> + /// Acquires shared namespace locks on both the source and target
>>> namespaces, and an exclusive
>>> + /// group lock on the source group to prevent concurrent writes
>>> to the same group.
>>> + pub fn move_group(
>>> + self: &Arc<Self>,
>>> + source_ns: &BackupNamespace,
>>> + group: &pbs_api_types::BackupGroup,
>>> + target_ns: &BackupNamespace,
>>> + ) -> Result<(), Error> {
>>> + if source_ns == target_ns {
>>> + bail!("source and target namespace must be different");
>>> + }
>>> +
>>> + let source_group = self.backup_group(source_ns.clone(),
>>> group.clone());
>>> + let target_group = self.backup_group(target_ns.clone(),
>>> group.clone());
>>> +
>>> + let _src_ns_lock = lock_namespace_shared(self.name(), source_ns)
>>> + .with_context(|| format!("failed to lock source namespace
>>> '{source_ns}'"))?;
>>> + let _tgt_ns_lock = lock_namespace_shared(self.name(), target_ns)
>>> + .with_context(|| format!("failed to lock target namespace
>>> '{target_ns}'"))?;
>>> +
>>> + let _group_lock = source_group
>>> + .lock()
>>> + .with_context(|| format!("failed to lock group '{group}'
>>> for move"))?;
>>> +
>>> + // Check existence under locks to avoid TOCTOU races with
>>> concurrent backups or
>>> + // namespace operations.
>>> + if !self.namespace_exists(target_ns) {
>>> + bail!("target namespace '{target_ns}' does not exist");
>>> + }
>>> + if !source_group.exists() {
>>> + bail!("group '{group}' does not exist in namespace
>>> '{source_ns}'");
>>> + }
>>> + if target_group.exists() {
>>> + bail!("group '{group}' already exists in target namespace
>>> '{target_ns}'");
>>> + }
>>> +
>>> + let backend = self.backend()?;
>>> +
>>> + std::fs::create_dir_all(self.type_path(target_ns,
>>> group.ty)).with_context(|| {
>>> + format!("failed to create type directory in '{target_ns}'
>>> for move")
>>> + })?;
>>> +
>>> + source_group.move_to(target_ns, &backend)
>>> + }
>>> +
>>> /// Remove a backup directory including all content
>>> pub fn remove_backup_dir(
>>> self: &Arc<Self>,
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH proxmox-backup v4 3/7] datastore: add move_namespace
2026-03-11 15:13 [PATCH proxmox-backup v4 0/7] fixes #6195: add support for moving groups and namespaces Hannes Laimer
2026-03-11 15:13 ` [PATCH proxmox-backup v4 1/7] datastore: add namespace-level locking Hannes Laimer
2026-03-11 15:13 ` [PATCH proxmox-backup v4 2/7] datastore: add move_group Hannes Laimer
@ 2026-03-11 15:13 ` Hannes Laimer
2026-03-11 15:13 ` [PATCH proxmox-backup v4 4/7] api: add PUT endpoint for move_group Hannes Laimer
` (4 subsequent siblings)
7 siblings, 0 replies; 20+ messages in thread
From: Hannes Laimer @ 2026-03-11 15:13 UTC (permalink / raw)
To: pbs-devel
move_namespace relocates an entire namespace subtree (the given
namespace, all child namespaces, and their groups) to a new location
within the same datastore.
For the filesystem backend the entire subtree is relocated with a single
atomic rename. For the S3 backend groups are moved one at a time via
BackupGroup::move_to(). Groups that fail are left at the source and
listed as an error in the task log so they can be retried with
move_group individually. Source namespaces where all groups succeeded
have their S3 markers and local cache directories removed,
deepest-first.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
pbs-datastore/src/datastore.rs | 216 ++++++++++++++++++++++++++++++++-
1 file changed, 215 insertions(+), 1 deletion(-)
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 51813acb..81066faf 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -31,7 +31,7 @@ use pbs_api_types::{
ArchiveType, Authid, BackupGroupDeleteStats, BackupNamespace, BackupType, ChunkOrder,
DataStoreConfig, DatastoreBackendConfig, DatastoreBackendType, DatastoreFSyncLevel,
DatastoreTuning, GarbageCollectionCacheStats, GarbageCollectionStatus, MaintenanceMode,
- MaintenanceType, Operation, UPID,
+ MaintenanceType, Operation, MAX_NAMESPACE_DEPTH, UPID,
};
use pbs_config::s3::S3_CFG_TYPE_ID;
use pbs_config::{BackupLockGuard, ConfigVersionCache};
@@ -1001,6 +1001,220 @@ impl DataStore {
Ok((removed_all_requested, stats))
}
+ /// Move a backup namespace (including all child namespaces and groups) to a new location.
+ ///
+ /// The entire subtree rooted at `source_ns` is relocated to `target_ns`. Exclusive namespace
+ /// locks are held on both source and target namespaces for the duration to block concurrent
+ /// readers and writers.
+ ///
+ /// For the filesystem backend the rename is atomic. For the S3 backend groups are moved
+ /// one at a time. A group that fails to copy is left at the source and can be moved
+ /// individually with `move_group`. The operation returns an error listing any such groups.
+ ///
+ /// Fails if:
+ /// - `source_ns` is the root namespace
+ /// - `source_ns` == `target_ns`
+ /// - `source_ns` does not exist
+ /// - `target_ns` already exists (to prevent silent merging)
+ /// - `target_ns`'s parent does not exist
+ /// - `source_ns` is an ancestor of `target_ns`
+ /// - the move would exceed the maximum namespace depth
+ pub fn move_namespace(
+ self: &Arc<Self>,
+ source_ns: &BackupNamespace,
+ target_ns: &BackupNamespace,
+ ) -> Result<(), Error> {
+ if source_ns.is_root() {
+ bail!("cannot move root namespace");
+ }
+ if source_ns == target_ns {
+ bail!("source and target namespace must be different");
+ }
+
+ // lock_namespace also acquires shared locks on all ancestors, so a concurrent
+ // move_namespace on a child namespace (which also walks up to ancestors) will be blocked
+ // by our exclusive lock, and we will be blocked by any in-progress move's targeting one of
+ // our ancestors.
+ let _source_ns_guard = lock_namespace(self.name(), source_ns)
+ .with_context(|| format!("failed to lock source namespace '{source_ns}' for move"))?;
+ // Lock target_ns to prevent two concurrent moves from racing to create the same target.
+ let _target_ns_guard = lock_namespace(self.name(), target_ns)
+ .with_context(|| format!("failed to lock target namespace '{target_ns}' for move"))?;
+
+ if !self.namespace_exists(source_ns) {
+ bail!("source namespace '{source_ns}' does not exist");
+ }
+ if self.namespace_exists(target_ns) {
+ bail!("target namespace '{target_ns}' already exists");
+ }
+ let target_parent = target_ns.parent();
+ if !self.namespace_exists(&target_parent) {
+ bail!("target namespace parent '{target_parent}' does not exist");
+ }
+ if source_ns.contains(target_ns).is_some() {
+ bail!(
+ "cannot move namespace '{source_ns}' into its own subtree (target: '{target_ns}')"
+ );
+ }
+
+ let all_source_ns: Vec<BackupNamespace> = self
+ .recursive_iter_backup_ns(source_ns.clone())?
+ .collect::<Result<Vec<_>, Error>>()?;
+
+ let all_source_groups: Vec<BackupGroup> = all_source_ns
+ .iter()
+ .map(|ns| self.iter_backup_groups(ns.clone()))
+ .collect::<Result<Vec<_>, Error>>()?
+ .into_iter()
+ .flatten()
+ .collect::<Result<Vec<_>, Error>>()?;
+
+ let subtree_depth = all_source_ns
+ .iter()
+ .map(BackupNamespace::depth)
+ .max()
+ .map_or(0, |d| d - source_ns.depth());
+ if subtree_depth + target_ns.depth() > MAX_NAMESPACE_DEPTH {
+ bail!(
+ "move would exceed maximum namespace depth \
+ ({subtree_depth}+{} > {MAX_NAMESPACE_DEPTH})",
+ target_ns.depth(),
+ );
+ }
+
+ let backend = self.backend()?;
+
+ log::info!(
+ "moving namespace '{source_ns}' -> '{target_ns}': {} namespaces, {} groups",
+ all_source_ns.len(),
+ all_source_groups.len(),
+ );
+
+ match &backend {
+ DatastoreBackend::Filesystem => {
+ let src_path = self.namespace_path(source_ns);
+ let dst_path = self.namespace_path(target_ns);
+ if let Some(dst_parent) = dst_path.parent() {
+ std::fs::create_dir_all(dst_parent).with_context(|| {
+ format!("failed to create parent directory for namespace rename '{source_ns}' -> '{target_ns}'")
+ })?;
+ }
+ log::debug!("renaming namespace directory '{src_path:?}' -> '{dst_path:?}'");
+ std::fs::rename(&src_path, &dst_path).with_context(|| {
+ format!("failed to rename namespace directory '{source_ns}' -> '{target_ns}'")
+ })?;
+ }
+ DatastoreBackend::S3(s3_client) => {
+ // Create target local namespace directories upfront (covers empty namespaces).
+ for ns in &all_source_ns {
+ let target_child = ns.map_prefix(source_ns, target_ns)?;
+ std::fs::create_dir_all(self.namespace_path(&target_child)).with_context(
+ || {
+ format!(
+ "failed to create local dir for target namespace '{target_child}'"
+ )
+ },
+ )?;
+ }
+
+ // Create S3 namespace markers for all target namespaces.
+ for ns in &all_source_ns {
+ let target_child = ns.map_prefix(source_ns, target_ns)?;
+ let object_key = crate::s3::object_key_from_path(
+ &target_child.path(),
+ NAMESPACE_MARKER_FILENAME,
+ )
+ .context("invalid namespace marker object key")?;
+ log::debug!(
+ "creating S3 namespace marker for '{target_child}': {object_key:?}"
+ );
+ proxmox_async::runtime::block_on(
+ s3_client.upload_no_replace_with_retry(object_key, Bytes::from("")),
+ )
+ .context("failed to create namespace marker on S3 backend")?;
+ }
+
+ // Move each group. Failed groups are skipped and remain at the source in
+ // both S3 and local cache. Collect the namespaces of any failed groups so we
+ // know which source namespaces still have content after the loop.
+ let mut failed_groups: Vec<String> = Vec::new();
+ let mut failed_ns: HashSet<BackupNamespace> = HashSet::new();
+
+ for group in &all_source_groups {
+ let target_group_ns = group.backup_ns().map_prefix(source_ns, target_ns)?;
+
+ // Ensure the target type directory exists before move_to renames into it.
+ if let Err(err) =
+ std::fs::create_dir_all(self.type_path(&target_group_ns, group.group().ty))
+ {
+ warn!(
+ "move_namespace: failed to create type dir for '{}' in '{}': {err:#}",
+ group.group(),
+ target_group_ns
+ );
+ failed_groups.push(group.group().to_string());
+ failed_ns.insert(group.backup_ns().clone());
+ continue;
+ }
+
+ if let Err(err) = group.move_to(&target_group_ns, &backend) {
+ warn!(
+ "move_namespace: failed to move group '{}' from '{}' to '{}': {err:#}",
+ group.group(),
+ group.backup_ns(),
+ target_group_ns
+ );
+ failed_groups.push(group.group().to_string());
+ failed_ns.insert(group.backup_ns().clone());
+ }
+ }
+
+ // Clean up source namespaces that are now fully empty (all groups moved).
+ // Process deepest-first so parent directories are already empty when reached.
+ for ns in all_source_ns.iter().rev() {
+ // Skip if this namespace itself or any descendant still has groups.
+ let has_remaining = failed_ns
+ .iter()
+ .any(|fns| fns == ns || ns.contains(fns).is_some());
+ if has_remaining {
+ continue;
+ }
+
+ // Delete the source S3 namespace marker.
+ let object_key =
+ crate::s3::object_key_from_path(&ns.path(), NAMESPACE_MARKER_FILENAME)
+ .context("invalid namespace marker object key")?;
+ log::debug!("deleting source S3 namespace marker for '{ns}': {object_key:?}");
+ proxmox_async::runtime::block_on(s3_client.delete_object(object_key))
+ .context("failed to delete source namespace marker on S3 backend")?;
+
+ // Remove the source local cache directory. Try type subdirectories first
+ // (they should be empty after the per-group renames), then the namespace dir.
+ let ns_path = self.namespace_path(ns);
+ if let Ok(entries) = std::fs::read_dir(&ns_path) {
+ for entry in entries.flatten() {
+ let _ = std::fs::remove_dir(entry.path());
+ }
+ }
+ let _ = std::fs::remove_dir(&ns_path);
+ }
+
+ if !failed_groups.is_empty() {
+ bail!(
+ "namespace move partially completed; {} group(s) could not be moved \
+ and remain at source '{}': {}. \
+ Use move group to move them individually.",
+ failed_groups.len(),
+ source_ns,
+ failed_groups.join(", ")
+ );
+ }
+ }
+ }
+
+ Ok(())
+ }
+
/// Remove a complete backup group including all snapshots.
///
/// Returns `BackupGroupDeleteStats`, containing the number of deleted snapshots
--
2.47.3
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH proxmox-backup v4 4/7] api: add PUT endpoint for move_group
2026-03-11 15:13 [PATCH proxmox-backup v4 0/7] fixes #6195: add support for moving groups and namespaces Hannes Laimer
` (2 preceding siblings ...)
2026-03-11 15:13 ` [PATCH proxmox-backup v4 3/7] datastore: add move_namespace Hannes Laimer
@ 2026-03-11 15:13 ` Hannes Laimer
2026-03-12 16:17 ` Christian Ebner
2026-03-11 15:13 ` [PATCH proxmox-backup v4 5/7] api: add PUT endpoint for move_namespace Hannes Laimer
` (3 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Hannes Laimer @ 2026-03-11 15:13 UTC (permalink / raw)
To: pbs-devel
Requires DATASTORE_MODIFY on both the source and target namespaces.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
src/api2/admin/datastore.rs | 76 ++++++++++++++++++++++++++++++++++++-
1 file changed, 75 insertions(+), 1 deletion(-)
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index cca34055..77a88e51 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -69,7 +69,9 @@ use proxmox_rest_server::{formatter, worker_is_active, WorkerTask};
use crate::api2::backup::optional_ns_param;
use crate::api2::node::rrd::create_value_from_rrd;
-use crate::backup::{check_ns_privs_full, ListAccessibleBackupGroups, VerifyWorker, NS_PRIVS_OK};
+use crate::backup::{
+ check_ns_privs, check_ns_privs_full, ListAccessibleBackupGroups, VerifyWorker, NS_PRIVS_OK,
+};
use crate::server::jobstate::{compute_schedule_status, Job, JobState};
use crate::tools::{backup_info_to_snapshot_list_item, get_all_snapshot_files, read_backup_index};
@@ -278,6 +280,77 @@ pub async fn delete_group(
.await?
}
+#[api(
+ input: {
+ properties: {
+ store: { schema: DATASTORE_SCHEMA },
+ ns: {
+ type: BackupNamespace,
+ optional: true,
+ },
+ group: {
+ type: pbs_api_types::BackupGroup,
+ flatten: true,
+ },
+ "new-ns": {
+ type: BackupNamespace,
+ },
+ },
+ },
+ returns: {
+ schema: UPID_SCHEMA,
+ },
+ access: {
+ permission: &Permission::Anybody,
+ description: "Requires DATASTORE_MODIFY on both the source and target namespace.",
+ },
+)]
+/// Move a backup group to a different namespace within the same datastore.
+pub fn move_group(
+ store: String,
+ ns: Option<BackupNamespace>,
+ group: pbs_api_types::BackupGroup,
+ new_ns: BackupNamespace,
+ rpcenv: &mut dyn RpcEnvironment,
+) -> Result<Value, Error> {
+ let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+ let ns = ns.unwrap_or_default();
+
+ check_ns_privs(&store, &ns, &auth_id, PRIV_DATASTORE_MODIFY)?;
+ check_ns_privs(&store, &new_ns, &auth_id, PRIV_DATASTORE_MODIFY)?;
+
+ let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
+
+ // Best-effort pre-checks for a fast synchronous error before spawning a worker.
+ if ns == new_ns {
+ bail!("source and target namespace must be different");
+ }
+ if !datastore.namespace_exists(&new_ns) {
+ bail!("target namespace '{new_ns}' does not exist");
+ }
+ let source_group = datastore.backup_group(ns.clone(), group.clone());
+ if !source_group.exists() {
+ bail!("group '{group}' does not exist in namespace '{ns}'");
+ }
+ let target_group = datastore.backup_group(new_ns.clone(), group.clone());
+ if target_group.exists() {
+ bail!("group '{group}' already exists in target namespace '{new_ns}'");
+ }
+
+ let worker_id = format!("{store}:{ns}:{group}");
+ let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
+
+ let upid_str = WorkerTask::new_thread(
+ "move-group",
+ Some(worker_id),
+ auth_id.to_string(),
+ to_stdout,
+ move |_worker| datastore.move_group(&ns, &group, &new_ns),
+ )?;
+
+ Ok(json!(upid_str))
+}
+
#[api(
input: {
properties: {
@@ -2828,6 +2901,7 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
"groups",
&Router::new()
.get(&API_METHOD_LIST_GROUPS)
+ .put(&API_METHOD_MOVE_GROUP)
.delete(&API_METHOD_DELETE_GROUP),
),
("mount", &Router::new().post(&API_METHOD_MOUNT)),
--
2.47.3
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH proxmox-backup v4 4/7] api: add PUT endpoint for move_group
2026-03-11 15:13 ` [PATCH proxmox-backup v4 4/7] api: add PUT endpoint for move_group Hannes Laimer
@ 2026-03-12 16:17 ` Christian Ebner
0 siblings, 0 replies; 20+ messages in thread
From: Christian Ebner @ 2026-03-12 16:17 UTC (permalink / raw)
To: Hannes Laimer, pbs-devel
One note inline.
On 3/11/26 4:13 PM, Hannes Laimer wrote:
> Requires DATASTORE_MODIFY on both the source and target namespaces.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> src/api2/admin/datastore.rs | 76 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 75 insertions(+), 1 deletion(-)
>
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index cca34055..77a88e51 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -69,7 +69,9 @@ use proxmox_rest_server::{formatter, worker_is_active, WorkerTask};
>
> use crate::api2::backup::optional_ns_param;
> use crate::api2::node::rrd::create_value_from_rrd;
> -use crate::backup::{check_ns_privs_full, ListAccessibleBackupGroups, VerifyWorker, NS_PRIVS_OK};
> +use crate::backup::{
> + check_ns_privs, check_ns_privs_full, ListAccessibleBackupGroups, VerifyWorker, NS_PRIVS_OK,
> +};
> use crate::server::jobstate::{compute_schedule_status, Job, JobState};
> use crate::tools::{backup_info_to_snapshot_list_item, get_all_snapshot_files, read_backup_index};
>
> @@ -278,6 +280,77 @@ pub async fn delete_group(
> .await?
> }
>
> +#[api(
> + input: {
> + properties: {
> + store: { schema: DATASTORE_SCHEMA },
> + ns: {
> + type: BackupNamespace,
> + optional: true,
> + },
> + group: {
> + type: pbs_api_types::BackupGroup,
> + flatten: true,
> + },
> + "new-ns": {
> + type: BackupNamespace,
> + },
> + },
> + },
> + returns: {
> + schema: UPID_SCHEMA,
> + },
> + access: {
> + permission: &Permission::Anybody,
> + description: "Requires DATASTORE_MODIFY on both the source and target namespace.",
> + },
> +)]
> +/// Move a backup group to a different namespace within the same datastore.
> +pub fn move_group(
> + store: String,
> + ns: Option<BackupNamespace>,
> + group: pbs_api_types::BackupGroup,
> + new_ns: BackupNamespace,
> + rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<Value, Error> {
> + let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> + let ns = ns.unwrap_or_default();
> +
> + check_ns_privs(&store, &ns, &auth_id, PRIV_DATASTORE_MODIFY)?;
> + check_ns_privs(&store, &new_ns, &auth_id, PRIV_DATASTORE_MODIFY)?;
> +
> + let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
Please note that the operation parameter to
DataStore::lookup_datastore() has been made non-optional in a very
recent change [0] applied after you send the patch series. Please adapt
this accordingly for a new version of the patch series, thanks!
[0]
https://git.proxmox.com/?p=proxmox-backup.git;a=commit;h=c0a9b6513a065ae0a1dc92e4b8c22721db752023
> +
> + // Best-effort pre-checks for a fast synchronous error before spawning a worker.
> + if ns == new_ns {
> + bail!("source and target namespace must be different");
> + }
> + if !datastore.namespace_exists(&new_ns) {
> + bail!("target namespace '{new_ns}' does not exist");
> + }
> + let source_group = datastore.backup_group(ns.clone(), group.clone());
> + if !source_group.exists() {
> + bail!("group '{group}' does not exist in namespace '{ns}'");
> + }
> + let target_group = datastore.backup_group(new_ns.clone(), group.clone());
> + if target_group.exists() {
> + bail!("group '{group}' already exists in target namespace '{new_ns}'");
> + }
> +
> + let worker_id = format!("{store}:{ns}:{group}");
> + let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> +
> + let upid_str = WorkerTask::new_thread(
> + "move-group",
> + Some(worker_id),
> + auth_id.to_string(),
> + to_stdout,
> + move |_worker| datastore.move_group(&ns, &group, &new_ns),
> + )?;
> +
> + Ok(json!(upid_str))
> +}
> +
> #[api(
> input: {
> properties: {
> @@ -2828,6 +2901,7 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
> "groups",
> &Router::new()
> .get(&API_METHOD_LIST_GROUPS)
> + .put(&API_METHOD_MOVE_GROUP)
> .delete(&API_METHOD_DELETE_GROUP),
> ),
> ("mount", &Router::new().post(&API_METHOD_MOUNT)),
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH proxmox-backup v4 5/7] api: add PUT endpoint for move_namespace
2026-03-11 15:13 [PATCH proxmox-backup v4 0/7] fixes #6195: add support for moving groups and namespaces Hannes Laimer
` (3 preceding siblings ...)
2026-03-11 15:13 ` [PATCH proxmox-backup v4 4/7] api: add PUT endpoint for move_group Hannes Laimer
@ 2026-03-11 15:13 ` Hannes Laimer
2026-03-12 16:19 ` Christian Ebner
2026-03-11 15:13 ` [PATCH proxmox-backup v4 6/7] ui: add move group action Hannes Laimer
` (2 subsequent siblings)
7 siblings, 1 reply; 20+ messages in thread
From: Hannes Laimer @ 2026-03-11 15:13 UTC (permalink / raw)
To: pbs-devel
Requires DATASTORE_MODIFY on the parent of both the source and
destination namespaces, matching the permissions used by
create_namespace() and delete_namespace().
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
src/api2/admin/namespace.rs | 78 ++++++++++++++++++++++++++++++++++++-
1 file changed, 76 insertions(+), 2 deletions(-)
diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
index ec913001..522133b1 100644
--- a/src/api2/admin/namespace.rs
+++ b/src/api2/admin/namespace.rs
@@ -1,12 +1,16 @@
use anyhow::{bail, Context, Error};
use pbs_config::CachedUserInfo;
-use proxmox_router::{http_bail, ApiMethod, Permission, Router, RpcEnvironment};
+use proxmox_rest_server::WorkerTask;
+use proxmox_router::{
+ http_bail, ApiMethod, Permission, Router, RpcEnvironment, RpcEnvironmentType,
+};
use proxmox_schema::*;
+use serde_json::{json, Value};
use pbs_api_types::{
Authid, BackupGroupDeleteStats, BackupNamespace, NamespaceListItem, Operation,
- DATASTORE_SCHEMA, NS_MAX_DEPTH_SCHEMA, PROXMOX_SAFE_ID_FORMAT,
+ DATASTORE_SCHEMA, NS_MAX_DEPTH_SCHEMA, PROXMOX_SAFE_ID_FORMAT, UPID_SCHEMA,
};
use pbs_datastore::DataStore;
@@ -193,7 +197,77 @@ pub fn delete_namespace(
Ok(stats)
}
+#[api(
+ input: {
+ properties: {
+ store: { schema: DATASTORE_SCHEMA },
+ ns: {
+ type: BackupNamespace,
+ },
+ "new-ns": {
+ type: BackupNamespace,
+ },
+ },
+ },
+ returns: {
+ schema: UPID_SCHEMA,
+ },
+ access: {
+ permission: &Permission::Anybody,
+ description: "Requires DATASTORE_MODIFY on the parent of 'ns' and on the parent of 'new-ns'.",
+ },
+)]
+/// Move a backup namespace (including all child namespaces and groups) to a new location.
+pub fn move_namespace(
+ store: String,
+ ns: BackupNamespace,
+ new_ns: BackupNamespace,
+ rpcenv: &mut dyn RpcEnvironment,
+) -> Result<Value, Error> {
+ let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+
+ check_ns_modification_privs(&store, &ns, &auth_id)?;
+ check_ns_modification_privs(&store, &new_ns, &auth_id)?;
+
+ let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
+
+ // Best-effort pre-checks for a fast synchronous error before spawning a worker.
+ if ns.is_root() {
+ bail!("cannot move root namespace");
+ }
+ if ns == new_ns {
+ bail!("source and target namespace must be different");
+ }
+ if !datastore.namespace_exists(&ns) {
+ bail!("source namespace '{ns}' does not exist");
+ }
+ if datastore.namespace_exists(&new_ns) {
+ bail!("target namespace '{new_ns}' already exists");
+ }
+ let target_parent = new_ns.parent();
+ if !datastore.namespace_exists(&target_parent) {
+ bail!("target parent namespace '{target_parent}' does not exist");
+ }
+ if ns.contains(&new_ns).is_some() {
+ bail!("cannot move namespace '{ns}' into its own subtree (target: '{new_ns}')");
+ }
+
+ let worker_id = format!("{store}:{ns}");
+ let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
+
+ let upid_str = WorkerTask::new_thread(
+ "move-namespace",
+ Some(worker_id),
+ auth_id.to_string(),
+ to_stdout,
+ move |_worker| datastore.move_namespace(&ns, &new_ns),
+ )?;
+
+ Ok(json!(upid_str))
+}
+
pub const ROUTER: Router = Router::new()
.get(&API_METHOD_LIST_NAMESPACES)
.post(&API_METHOD_CREATE_NAMESPACE)
+ .put(&API_METHOD_MOVE_NAMESPACE)
.delete(&API_METHOD_DELETE_NAMESPACE);
--
2.47.3
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH proxmox-backup v4 5/7] api: add PUT endpoint for move_namespace
2026-03-11 15:13 ` [PATCH proxmox-backup v4 5/7] api: add PUT endpoint for move_namespace Hannes Laimer
@ 2026-03-12 16:19 ` Christian Ebner
0 siblings, 0 replies; 20+ messages in thread
From: Christian Ebner @ 2026-03-12 16:19 UTC (permalink / raw)
To: Hannes Laimer, pbs-devel
One note inline.
On 3/11/26 4:13 PM, Hannes Laimer wrote:
> Requires DATASTORE_MODIFY on the parent of both the source and
> destination namespaces, matching the permissions used by
> create_namespace() and delete_namespace().
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> src/api2/admin/namespace.rs | 78 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 76 insertions(+), 2 deletions(-)
>
> diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
> index ec913001..522133b1 100644
> --- a/src/api2/admin/namespace.rs
> +++ b/src/api2/admin/namespace.rs
> @@ -1,12 +1,16 @@
> use anyhow::{bail, Context, Error};
>
> use pbs_config::CachedUserInfo;
> -use proxmox_router::{http_bail, ApiMethod, Permission, Router, RpcEnvironment};
> +use proxmox_rest_server::WorkerTask;
> +use proxmox_router::{
> + http_bail, ApiMethod, Permission, Router, RpcEnvironment, RpcEnvironmentType,
> +};
> use proxmox_schema::*;
> +use serde_json::{json, Value};
>
> use pbs_api_types::{
> Authid, BackupGroupDeleteStats, BackupNamespace, NamespaceListItem, Operation,
> - DATASTORE_SCHEMA, NS_MAX_DEPTH_SCHEMA, PROXMOX_SAFE_ID_FORMAT,
> + DATASTORE_SCHEMA, NS_MAX_DEPTH_SCHEMA, PROXMOX_SAFE_ID_FORMAT, UPID_SCHEMA,
> };
>
> use pbs_datastore::DataStore;
> @@ -193,7 +197,77 @@ pub fn delete_namespace(
> Ok(stats)
> }
>
> +#[api(
> + input: {
> + properties: {
> + store: { schema: DATASTORE_SCHEMA },
> + ns: {
> + type: BackupNamespace,
> + },
> + "new-ns": {
> + type: BackupNamespace,
> + },
> + },
> + },
> + returns: {
> + schema: UPID_SCHEMA,
> + },
> + access: {
> + permission: &Permission::Anybody,
> + description: "Requires DATASTORE_MODIFY on the parent of 'ns' and on the parent of 'new-ns'.",
> + },
> +)]
> +/// Move a backup namespace (including all child namespaces and groups) to a new location.
> +pub fn move_namespace(
> + store: String,
> + ns: BackupNamespace,
> + new_ns: BackupNamespace,
> + rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<Value, Error> {
> + let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +
> + check_ns_modification_privs(&store, &ns, &auth_id)?;
> + check_ns_modification_privs(&store, &new_ns, &auth_id)?;
> +
> + let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
Same note as for the previous patch, the operation parameter is now
non-optional and needs adaption.
> +
> + // Best-effort pre-checks for a fast synchronous error before spawning a worker.
> + if ns.is_root() {
> + bail!("cannot move root namespace");
> + }
> + if ns == new_ns {
> + bail!("source and target namespace must be different");
> + }
> + if !datastore.namespace_exists(&ns) {
> + bail!("source namespace '{ns}' does not exist");
> + }
> + if datastore.namespace_exists(&new_ns) {
> + bail!("target namespace '{new_ns}' already exists");
> + }
> + let target_parent = new_ns.parent();
> + if !datastore.namespace_exists(&target_parent) {
> + bail!("target parent namespace '{target_parent}' does not exist");
> + }
> + if ns.contains(&new_ns).is_some() {
> + bail!("cannot move namespace '{ns}' into its own subtree (target: '{new_ns}')");
> + }
> +
> + let worker_id = format!("{store}:{ns}");
> + let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> +
> + let upid_str = WorkerTask::new_thread(
> + "move-namespace",
> + Some(worker_id),
> + auth_id.to_string(),
> + to_stdout,
> + move |_worker| datastore.move_namespace(&ns, &new_ns),
> + )?;
> +
> + Ok(json!(upid_str))
> +}
> +
> pub const ROUTER: Router = Router::new()
> .get(&API_METHOD_LIST_NAMESPACES)
> .post(&API_METHOD_CREATE_NAMESPACE)
> + .put(&API_METHOD_MOVE_NAMESPACE)
> .delete(&API_METHOD_DELETE_NAMESPACE);
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH proxmox-backup v4 6/7] ui: add move group action
2026-03-11 15:13 [PATCH proxmox-backup v4 0/7] fixes #6195: add support for moving groups and namespaces Hannes Laimer
` (4 preceding siblings ...)
2026-03-11 15:13 ` [PATCH proxmox-backup v4 5/7] api: add PUT endpoint for move_namespace Hannes Laimer
@ 2026-03-11 15:13 ` Hannes Laimer
2026-03-17 11:43 ` Christian Ebner
2026-03-11 15:13 ` [PATCH proxmox-backup v4 7/7] ui: add move namespace action Hannes Laimer
2026-03-12 16:21 ` [PATCH proxmox-backup v4 0/7] fixes #6195: add support for moving groups and namespaces Christian Ebner
7 siblings, 1 reply; 20+ messages in thread
From: Hannes Laimer @ 2026-03-11 15:13 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
www/Makefile | 1 +
www/datastore/Content.js | 61 ++++++++++++++++++++++++++++++++++++++++
www/window/GroupMove.js | 56 ++++++++++++++++++++++++++++++++++++
3 files changed, 118 insertions(+)
create mode 100644 www/window/GroupMove.js
diff --git a/www/Makefile b/www/Makefile
index 9ebf0445..db775b64 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -77,6 +77,7 @@ JSSRC= \
window/ACLEdit.js \
window/BackupGroupChangeOwner.js \
window/CreateDirectory.js \
+ window/GroupMove.js \
window/DataStoreEdit.js \
window/NamespaceEdit.js \
window/MaintenanceOptions.js \
diff --git a/www/datastore/Content.js b/www/datastore/Content.js
index a2aa1949..ccd48616 100644
--- a/www/datastore/Content.js
+++ b/www/datastore/Content.js
@@ -635,6 +635,50 @@ Ext.define('PBS.DataStoreContent', {
});
},
+ moveNS: function () {
+ let me = this;
+ let view = me.getView();
+ if (!view.namespace || view.namespace === '') {
+ return;
+ }
+ let win = Ext.create('PBS.window.NamespaceMove', {
+ datastore: view.datastore,
+ namespace: view.namespace,
+ taskDone: (success) => {
+ if (success) {
+ let newNs = win.getNewNamespace();
+ view.down('pbsNamespaceSelector').store?.load();
+ me.nsChange(null, newNs);
+ }
+ },
+ });
+ win.show();
+ },
+
+ moveGroup: function (data) {
+ let me = this;
+ let view = me.getView();
+ let group = `${data.backup_type}/${data.backup_id}`;
+ Ext.create('PBS.window.GroupMove', {
+ datastore: view.datastore,
+ namespace: view.namespace,
+ backupType: data.backup_type,
+ backupId: data.backup_id,
+ group,
+ taskDone: () => me.reload(),
+ }).show();
+ },
+
+ onMove: function (table, rI, cI, item, e, { data }) {
+ let me = this;
+ if (data.ty === 'group') {
+ me.moveGroup(data);
+ } else if (data.ty === 'ns') {
+ me.moveNS();
+ }
+ },
+
+
forgetGroup: function (data) {
let me = this;
let view = me.getView();
@@ -1080,6 +1124,23 @@ Ext.define('PBS.DataStoreContent', {
},
isActionDisabled: (v, r, c, i, rec) => rec.data.ty !== 'dir',
},
+ {
+ handler: 'onMove',
+ getTip: (v, m, { data }) => {
+ if (data.ty === 'group') {
+ return Ext.String.format(gettext("Move group '{0}'"), v);
+ }
+ return Ext.String.format(gettext("Move namespace '{0}'"), v);
+ },
+ getClass: (v, m, { data }) => {
+ if (data.ty === 'group') { return 'fa fa-arrows'; }
+ if (data.ty === 'ns' && !data.isRootNS && data.ns === undefined) {
+ return 'fa fa-arrows';
+ }
+ return 'pmx-hidden';
+ },
+ isActionDisabled: (v, r, c, i, { data }) => false,
+ },
{
handler: 'onForget',
getTip: (v, m, { data }) => {
diff --git a/www/window/GroupMove.js b/www/window/GroupMove.js
new file mode 100644
index 00000000..f663c606
--- /dev/null
+++ b/www/window/GroupMove.js
@@ -0,0 +1,56 @@
+Ext.define('PBS.window.GroupMove', {
+ extend: 'Proxmox.window.Edit',
+ alias: 'widget.pbsGroupMove',
+ mixins: ['Proxmox.Mixin.CBind'],
+
+ onlineHelp: 'storage-namespaces',
+
+ isCreate: true,
+ submitText: gettext('Move'),
+ showTaskViewer: true,
+
+ cbind: {
+ url: '/api2/extjs/admin/datastore/{datastore}/groups',
+ title: (get) => Ext.String.format(gettext("Move Backup Group '{0}'"), get('group')),
+ },
+ method: 'PUT',
+
+ width: 400,
+ fieldDefaults: {
+ labelWidth: 120,
+ },
+
+ items: {
+ xtype: 'inputpanel',
+ onGetValues: function (values) {
+ let win = this.up('window');
+ let result = {
+ 'backup-type': win.backupType,
+ 'backup-id': win.backupId,
+ 'new-ns': values['new-ns'] || '',
+ };
+ if (win.namespace && win.namespace !== '') {
+ result.ns = win.namespace;
+ }
+ return result;
+ },
+ items: [
+ {
+ xtype: 'displayfield',
+ fieldLabel: gettext('Group'),
+ cbind: {
+ value: '{group}',
+ },
+ },
+ {
+ xtype: 'pbsNamespaceSelector',
+ name: 'new-ns',
+ fieldLabel: gettext('Target Namespace'),
+ allowBlank: true,
+ cbind: {
+ datastore: '{datastore}',
+ },
+ },
+ ],
+ },
+});
--
2.47.3
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH proxmox-backup v4 6/7] ui: add move group action
2026-03-11 15:13 ` [PATCH proxmox-backup v4 6/7] ui: add move group action Hannes Laimer
@ 2026-03-17 11:43 ` Christian Ebner
2026-03-17 11:48 ` Hannes Laimer
0 siblings, 1 reply; 20+ messages in thread
From: Christian Ebner @ 2026-03-17 11:43 UTC (permalink / raw)
To: Hannes Laimer, pbs-devel
One comment inline.
On 3/11/26 4:13 PM, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> www/Makefile | 1 +
> www/datastore/Content.js | 61 ++++++++++++++++++++++++++++++++++++++++
> www/window/GroupMove.js | 56 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 118 insertions(+)
> create mode 100644 www/window/GroupMove.js
>
> diff --git a/www/Makefile b/www/Makefile
> index 9ebf0445..db775b64 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -77,6 +77,7 @@ JSSRC= \
> window/ACLEdit.js \
> window/BackupGroupChangeOwner.js \
> window/CreateDirectory.js \
> + window/GroupMove.js \
> window/DataStoreEdit.js \
> window/NamespaceEdit.js \
> window/MaintenanceOptions.js \
> diff --git a/www/datastore/Content.js b/www/datastore/Content.js
> index a2aa1949..ccd48616 100644
> --- a/www/datastore/Content.js
> +++ b/www/datastore/Content.js
> @@ -635,6 +635,50 @@ Ext.define('PBS.DataStoreContent', {
> });
> },
>
> + moveNS: function () {
> + let me = this;
> + let view = me.getView();
> + if (!view.namespace || view.namespace === '') {
> + return;
> + }
> + let win = Ext.create('PBS.window.NamespaceMove', {
> + datastore: view.datastore,
> + namespace: view.namespace,
> + taskDone: (success) => {
> + if (success) {
> + let newNs = win.getNewNamespace();
> + view.down('pbsNamespaceSelector').store?.load();
> + me.nsChange(null, newNs);
> + }
> + },
> + });
> + win.show();
> + },
> +
> + moveGroup: function (data) {
> + let me = this;
> + let view = me.getView();
> + let group = `${data.backup_type}/${data.backup_id}`;
> + Ext.create('PBS.window.GroupMove', {
> + datastore: view.datastore,
> + namespace: view.namespace,
> + backupType: data.backup_type,
> + backupId: data.backup_id,
> + group,
> + taskDone: () => me.reload(),
> + }).show();
> + },
> +
> + onMove: function (table, rI, cI, item, e, { data }) {
> + let me = this;
> + if (data.ty === 'group') {
> + me.moveGroup(data);
> + } else if (data.ty === 'ns') {
> + me.moveNS();
> + }
> + },
> +
> +
> forgetGroup: function (data) {
> let me = this;
> let view = me.getView();
> @@ -1080,6 +1124,23 @@ Ext.define('PBS.DataStoreContent', {
> },
> isActionDisabled: (v, r, c, i, rec) => rec.data.ty !== 'dir',
> },
> + {
> + handler: 'onMove',
> + getTip: (v, m, { data }) => {
> + if (data.ty === 'group') {
> + return Ext.String.format(gettext("Move group '{0}'"), v);
> + }
> + return Ext.String.format(gettext("Move namespace '{0}'"), v);
> + },
> + getClass: (v, m, { data }) => {
> + if (data.ty === 'group') { return 'fa fa-arrows'; }
> + if (data.ty === 'ns' && !data.isRootNS && data.ns === undefined) {
> + return 'fa fa-arrows';
> + }
> + return 'pmx-hidden';
> + },
> + isActionDisabled: (v, r, c, i, { data }) => false,
> + },
comment: placing this right before the prune icon is a bit dangerous
(although a prune will ask for confirmation).
Since the protected icons is not shown for groups and namespaces, it
should rather be placed before that, right after the verification icon.
Further, what I missed is having this also in the context menu opened on
right click of the group/namespace.
> {
> handler: 'onForget',
> getTip: (v, m, { data }) => {
> diff --git a/www/window/GroupMove.js b/www/window/GroupMove.js
> new file mode 100644
> index 00000000..f663c606
> --- /dev/null
> +++ b/www/window/GroupMove.js
> @@ -0,0 +1,56 @@
> +Ext.define('PBS.window.GroupMove', {
> + extend: 'Proxmox.window.Edit',
> + alias: 'widget.pbsGroupMove',
> + mixins: ['Proxmox.Mixin.CBind'],
> +
> + onlineHelp: 'storage-namespaces',
> +
> + isCreate: true,
> + submitText: gettext('Move'),
> + showTaskViewer: true,
> +
> + cbind: {
> + url: '/api2/extjs/admin/datastore/{datastore}/groups',
> + title: (get) => Ext.String.format(gettext("Move Backup Group '{0}'"), get('group')),
> + },
> + method: 'PUT',
> +
> + width: 400,
> + fieldDefaults: {
> + labelWidth: 120,
> + },
> +
> + items: {
> + xtype: 'inputpanel',
> + onGetValues: function (values) {
> + let win = this.up('window');
> + let result = {
> + 'backup-type': win.backupType,
> + 'backup-id': win.backupId,
> + 'new-ns': values['new-ns'] || '',
> + };
> + if (win.namespace && win.namespace !== '') {
> + result.ns = win.namespace;
> + }
> + return result;
> + },
> + items: [
> + {
> + xtype: 'displayfield',
> + fieldLabel: gettext('Group'),
> + cbind: {
> + value: '{group}',
> + },
> + },
> + {
> + xtype: 'pbsNamespaceSelector',
> + name: 'new-ns',
> + fieldLabel: gettext('Target Namespace'),
> + allowBlank: true,
> + cbind: {
> + datastore: '{datastore}',
> + },
> + },
> + ],
> + },
> +});
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH proxmox-backup v4 6/7] ui: add move group action
2026-03-17 11:43 ` Christian Ebner
@ 2026-03-17 11:48 ` Hannes Laimer
0 siblings, 0 replies; 20+ messages in thread
From: Hannes Laimer @ 2026-03-17 11:48 UTC (permalink / raw)
To: Christian Ebner, pbs-devel
On 2026-03-17 12:43, Christian Ebner wrote:
> One comment inline.
>
> On 3/11/26 4:13 PM, Hannes Laimer wrote:
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>> www/Makefile | 1 +
>> www/datastore/Content.js | 61 ++++++++++++++++++++++++++++++++++++++++
>> www/window/GroupMove.js | 56 ++++++++++++++++++++++++++++++++++++
>> 3 files changed, 118 insertions(+)
>> create mode 100644 www/window/GroupMove.js
>>
>> diff --git a/www/Makefile b/www/Makefile
>> index 9ebf0445..db775b64 100644
>> --- a/www/Makefile
>> +++ b/www/Makefile
>> @@ -77,6 +77,7 @@ JSSRC= \
>> window/ACLEdit.js \
>> window/BackupGroupChangeOwner.js \
>> window/CreateDirectory.js \
>> + window/GroupMove.js \
>> window/DataStoreEdit.js \
>> window/NamespaceEdit.js \
>> window/MaintenanceOptions.js \
>> diff --git a/www/datastore/Content.js b/www/datastore/Content.js
>> index a2aa1949..ccd48616 100644
>> --- a/www/datastore/Content.js
>> +++ b/www/datastore/Content.js
>> @@ -635,6 +635,50 @@ Ext.define('PBS.DataStoreContent', {
>> });
>> },
>> + moveNS: function () {
>> + let me = this;
>> + let view = me.getView();
>> + if (!view.namespace || view.namespace === '') {
>> + return;
>> + }
>> + let win = Ext.create('PBS.window.NamespaceMove', {
>> + datastore: view.datastore,
>> + namespace: view.namespace,
>> + taskDone: (success) => {
>> + if (success) {
>> + let newNs = win.getNewNamespace();
>> + view.down('pbsNamespaceSelector').store?.load();
>> + me.nsChange(null, newNs);
>> + }
>> + },
>> + });
>> + win.show();
>> + },
>> +
>> + moveGroup: function (data) {
>> + let me = this;
>> + let view = me.getView();
>> + let group = `${data.backup_type}/${data.backup_id}`;
>> + Ext.create('PBS.window.GroupMove', {
>> + datastore: view.datastore,
>> + namespace: view.namespace,
>> + backupType: data.backup_type,
>> + backupId: data.backup_id,
>> + group,
>> + taskDone: () => me.reload(),
>> + }).show();
>> + },
>> +
>> + onMove: function (table, rI, cI, item, e, { data }) {
>> + let me = this;
>> + if (data.ty === 'group') {
>> + me.moveGroup(data);
>> + } else if (data.ty === 'ns') {
>> + me.moveNS();
>> + }
>> + },
>> +
>> +
>> forgetGroup: function (data) {
>> let me = this;
>> let view = me.getView();
>> @@ -1080,6 +1124,23 @@ Ext.define('PBS.DataStoreContent', {
>> },
>> isActionDisabled: (v, r, c, i, rec) =>
>> rec.data.ty !== 'dir',
>> },
>> + {
>> + handler: 'onMove',
>> + getTip: (v, m, { data }) => {
>> + if (data.ty === 'group') {
>> + return Ext.String.format(gettext("Move
>> group '{0}'"), v);
>> + }
>> + return Ext.String.format(gettext("Move
>> namespace '{0}'"), v);
>> + },
>> + getClass: (v, m, { data }) => {
>> + if (data.ty === 'group') { return 'fa fa-
>> arrows'; }
>> + if (data.ty === 'ns' && !data.isRootNS &&
>> data.ns === undefined) {
>> + return 'fa fa-arrows';
>> + }
>> + return 'pmx-hidden';
>> + },
>> + isActionDisabled: (v, r, c, i, { data }) => false,
>> + },
>
> comment: placing this right before the prune icon is a bit dangerous
> (although a prune will ask for confirmation).
>
> Since the protected icons is not shown for groups and namespaces, it
> should rather be placed before that, right after the verification icon.
>
makes sense, will change in v5
> Further, what I missed is having this also in the context menu opened on
> right click of the group/namespace.
never right-clicked a group, didn't know we have a context menu there :)
will also add it there, thanks!
>
>> {
>> handler: 'onForget',
>> getTip: (v, m, { data }) => {
>> diff --git a/www/window/GroupMove.js b/www/window/GroupMove.js
>> new file mode 100644
>> index 00000000..f663c606
>> --- /dev/null
>> +++ b/www/window/GroupMove.js
>> @@ -0,0 +1,56 @@
>> +Ext.define('PBS.window.GroupMove', {
>> + extend: 'Proxmox.window.Edit',
>> + alias: 'widget.pbsGroupMove',
>> + mixins: ['Proxmox.Mixin.CBind'],
>> +
>> + onlineHelp: 'storage-namespaces',
>> +
>> + isCreate: true,
>> + submitText: gettext('Move'),
>> + showTaskViewer: true,
>> +
>> + cbind: {
>> + url: '/api2/extjs/admin/datastore/{datastore}/groups',
>> + title: (get) => Ext.String.format(gettext("Move Backup Group
>> '{0}'"), get('group')),
>> + },
>> + method: 'PUT',
>> +
>> + width: 400,
>> + fieldDefaults: {
>> + labelWidth: 120,
>> + },
>> +
>> + items: {
>> + xtype: 'inputpanel',
>> + onGetValues: function (values) {
>> + let win = this.up('window');
>> + let result = {
>> + 'backup-type': win.backupType,
>> + 'backup-id': win.backupId,
>> + 'new-ns': values['new-ns'] || '',
>> + };
>> + if (win.namespace && win.namespace !== '') {
>> + result.ns = win.namespace;
>> + }
>> + return result;
>> + },
>> + items: [
>> + {
>> + xtype: 'displayfield',
>> + fieldLabel: gettext('Group'),
>> + cbind: {
>> + value: '{group}',
>> + },
>> + },
>> + {
>> + xtype: 'pbsNamespaceSelector',
>> + name: 'new-ns',
>> + fieldLabel: gettext('Target Namespace'),
>> + allowBlank: true,
>> + cbind: {
>> + datastore: '{datastore}',
>> + },
>> + },
>> + ],
>> + },
>> +});
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH proxmox-backup v4 7/7] ui: add move namespace action
2026-03-11 15:13 [PATCH proxmox-backup v4 0/7] fixes #6195: add support for moving groups and namespaces Hannes Laimer
` (5 preceding siblings ...)
2026-03-11 15:13 ` [PATCH proxmox-backup v4 6/7] ui: add move group action Hannes Laimer
@ 2026-03-11 15:13 ` Hannes Laimer
2026-03-12 16:21 ` [PATCH proxmox-backup v4 0/7] fixes #6195: add support for moving groups and namespaces Christian Ebner
7 siblings, 0 replies; 20+ messages in thread
From: Hannes Laimer @ 2026-03-11 15:13 UTC (permalink / raw)
To: pbs-devel
When moving a namespace, exclude the source namespace and its subtree
from the 'New Parent' selector.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
www/Makefile | 1 +
www/datastore/Content.js | 1 -
www/form/NamespaceSelector.js | 11 +++++
www/window/NamespaceMove.js | 79 +++++++++++++++++++++++++++++++++++
4 files changed, 91 insertions(+), 1 deletion(-)
create mode 100644 www/window/NamespaceMove.js
diff --git a/www/Makefile b/www/Makefile
index db775b64..9452879a 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -80,6 +80,7 @@ JSSRC= \
window/GroupMove.js \
window/DataStoreEdit.js \
window/NamespaceEdit.js \
+ window/NamespaceMove.js \
window/MaintenanceOptions.js \
window/NotesEdit.js \
window/RemoteEdit.js \
diff --git a/www/datastore/Content.js b/www/datastore/Content.js
index ccd48616..fb0858e9 100644
--- a/www/datastore/Content.js
+++ b/www/datastore/Content.js
@@ -678,7 +678,6 @@ Ext.define('PBS.DataStoreContent', {
}
},
-
forgetGroup: function (data) {
let me = this;
let view = me.getView();
diff --git a/www/form/NamespaceSelector.js b/www/form/NamespaceSelector.js
index ddf68254..d349b568 100644
--- a/www/form/NamespaceSelector.js
+++ b/www/form/NamespaceSelector.js
@@ -90,6 +90,17 @@ Ext.define('PBS.form.NamespaceSelector', {
},
});
+ if (me.excludeNs) {
+ me.store.addFilter(
+ new Ext.util.Filter({
+ filterFn: (rec) => {
+ let ns = rec.data.ns;
+ return ns !== me.excludeNs && !ns.startsWith(`${me.excludeNs}/`);
+ },
+ }),
+ );
+ }
+
me.callParent();
},
});
diff --git a/www/window/NamespaceMove.js b/www/window/NamespaceMove.js
new file mode 100644
index 00000000..415d509d
--- /dev/null
+++ b/www/window/NamespaceMove.js
@@ -0,0 +1,79 @@
+Ext.define('PBS.window.NamespaceMove', {
+ extend: 'Proxmox.window.Edit',
+ alias: 'widget.pbsNamespaceMove',
+ mixins: ['Proxmox.Mixin.CBind'],
+
+ onlineHelp: 'storage-namespaces',
+
+ submitText: gettext('Move'),
+ isCreate: true,
+ showTaskViewer: true,
+
+ cbind: {
+ url: '/api2/extjs/admin/datastore/{datastore}/namespace',
+ title: (get) => Ext.String.format(gettext("Move Namespace '{0}'"), get('namespace')),
+ },
+ method: 'PUT',
+
+ width: 450,
+ fieldDefaults: {
+ labelWidth: 120,
+ },
+
+ cbindData: function (initialConfig) {
+ let ns = initialConfig.namespace ?? '';
+ let parts = ns.split('/');
+ return { nsName: parts[parts.length - 1] };
+ },
+
+ // Returns the new-ns path that was submitted, for use by the caller after success.
+ getNewNamespace: function () {
+ let me = this;
+ let parent = me.down('[name=parent]').getValue() || '';
+ let name = me.down('[name=name]').getValue();
+ return parent ? `${parent}/${name}` : name;
+ },
+
+ items: {
+ xtype: 'inputpanel',
+ onGetValues: function (values) {
+ let parent = values.parent || '';
+ let newNs = parent ? `${parent}/${values.name}` : values.name;
+ return {
+ ns: this.up('window').namespace,
+ 'new-ns': newNs,
+ };
+ },
+ items: [
+ {
+ xtype: 'displayfield',
+ fieldLabel: gettext('Namespace'),
+ cbind: {
+ value: '{namespace}',
+ },
+ },
+ {
+ xtype: 'pbsNamespaceSelector',
+ name: 'parent',
+ fieldLabel: gettext('New Parent'),
+ allowBlank: true,
+ cbind: {
+ datastore: '{datastore}',
+ excludeNs: '{namespace}',
+ },
+ },
+ {
+ xtype: 'proxmoxtextfield',
+ name: 'name',
+ fieldLabel: gettext('New Name'),
+ allowBlank: false,
+ maxLength: 31,
+ regex: PBS.Utils.SAFE_ID_RE,
+ regexText: gettext("Only alpha numerical, '_' and '-' (if not at start) allowed"),
+ cbind: {
+ value: '{nsName}',
+ },
+ },
+ ],
+ },
+});
--
2.47.3
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH proxmox-backup v4 0/7] fixes #6195: add support for moving groups and namespaces
2026-03-11 15:13 [PATCH proxmox-backup v4 0/7] fixes #6195: add support for moving groups and namespaces Hannes Laimer
` (6 preceding siblings ...)
2026-03-11 15:13 ` [PATCH proxmox-backup v4 7/7] ui: add move namespace action Hannes Laimer
@ 2026-03-12 16:21 ` Christian Ebner
7 siblings, 0 replies; 20+ messages in thread
From: Christian Ebner @ 2026-03-12 16:21 UTC (permalink / raw)
To: Hannes Laimer, pbs-devel
On 3/11/26 4:12 PM, Hannes Laimer wrote:
> Implements moving both backup groups and namespaces within a datastore.
> This also adds namespace locking, this allows moves to happen within a
> datastore that is otherwise in-use.
>
>
> # Namespace locking
>
> To make move_group and move_namespace safe against concurrent
> operations, a namespace-level locking scheme is introduced in the first
> patch.
>
> Lock files live at:
> /run/proxmox-backup/locks/<store>/<ns:colon:encoded>/.ns-lock
>
> Two lock modes are used, both non-blocking (timeout=0):
>
> - Shared: held by operations that read from or write into a namespace
> (backup, pull/push sync, verify per snapshot, prune per group).
> Acquiring a shared lock on ns also acquires shared locks on all
> non-root ancestors, so an exclusive lock on any ancestor blocks
> all active operations below it.
>
> - Exclusive: held by operations that structurally modify a namespace
> (move_namespace, move_group on the group lock).
> Acquiring an exclusive lock on ns also acquires shared locks on all
> non-root ancestors, mirroring the shared variant so that two
> concurrent structural operations on related namespaces contend
> correctly.
>
> Locking up the ancestor chain rather than down the subtree keeps the
> cost O(depth), bounded by MAX_NAMESPACE_DEPTH (8).
>
> Verify and prune skip gracefully when a namespace lock cannot be
> acquired, since a concurrent move is a transient condition.
> create_locked_backup_group now returns (owner, ns_guard, group_guard),
> all callers updated.
>
> # Moving
>
> ## Groups
> 1. lock source ns (shared), lock target ns (shared), lock group (exclusive)
> 2. create target type directory
> 3. FS: rename group directory
> S3: copy objects, rename cache, delete source objects
>
> ## Namespace
> 1. lock source ns (exclusive), lock target ns (exclusive)
> 2. FS: rename namespace directory
> S3: - create target ns dirs and markers, both on S3 and local cache
> - move groups one by one, if copying fails the group stays at
> the source and can be moved afterwards manually
> - clean up empty source ns dirs
Nice work! Took a quick glance and left some high level comments and
notes already. Plan to do some testing tomorrow.
^ permalink raw reply [flat|nested] 20+ messages in thread