public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Hannes Laimer <h.laimer@proxmox.com>, pbs-devel@lists.proxmox.com
Subject: Re: [PATCH proxmox-backup v5 4/9] datastore: add move_namespace
Date: Wed, 25 Mar 2026 09:55:53 +0100	[thread overview]
Message-ID: <1774427682.fqtrys91x3.astroid@yuna.none> (raw)
In-Reply-To: <b3f8afdf-a993-40f8-b852-1e29f2283830@proxmox.com>

On March 25, 2026 7:32 am, Hannes Laimer wrote:
> On 2026-03-24 13:59, Fabian Grünbichler wrote:
>> On March 19, 2026 5:13 pm, Hannes Laimer wrote:
>>> 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 | 249 ++++++++++++++++++++++++++++++++-
>>>  1 file changed, 248 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>>> index d4c88452..3187cbc3 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};
>>> @@ -1003,6 +1003,253 @@ 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.
>> 
>> I think we want these semantics in both cases, since a move is only
>> atomic if you ignore a lot of the invariants you need to violate to make
>> it so, even in the filesystem case. and if we have such semantics, and
>> handle the invariants correctly, we only need the namespace locks if we
>> basically want to make the namespace lock a "per-NS-maintenance-mode" in
>> disguise.. which I don't think is what users actually want - they want
>> an option to bulk move groups from A to B without interrupting the
>> world..
>> 
> 
> yes, i see that use-case. but isn't a group lock also just a
> per-group-maintenance-mode :P

in a way. my point was the scope. locking a single group (and its
snapshot) at a time is much less invasive than locking a whole
namespace. the current approach pretty much requires(!) stopping all
readers and writers (which includes things like verification, prune job,
GC, ..) before I can move a namespace, and the move can then take
arbitrarily long with S3. this is rather cumbersome in practice, because
the person doing the moving and the person(s) controlling the rest of
the tasks are often not the same. that's why I noted the parallelism to
maintenance mode - taking an exclusive lock on a namespace forces a lot
of unrelated actions to be blocked, just like maintance mode does.
taking an exclusive lock on a group just prevents deleting it or adding
a new snapshot, it does not prevent verifying it, for example.

the other approach (recursively locking groups and moving them
one-by-one) has downsides:
- if a reader/writer conflicts with the move, that group either needs to
  be copied or skipped, and will remain in the source namespace
- it's more work in the file system case
- if aborted half-way through for whatever reason, you have a partial
  move

all of those points are already somewhat true with S3 anyway.

the big upside is that by reducing the granularity of the "unit of
work", the amount of conflicts goes down as well. so it meshes better
with how we handle other operations.

this is particularly important if you think of bulk moving of
*contents*, instead of moving of namespaces themselves. and I think that
is an important use case, in particular considering future extensions:
- move groups into an "archive" namespace based on certain criteria (no
  backups in the past N weeks?)
- move snapshots older than N into an archive namespace (creating groups
  if needed)

the current move namespace feature here is more akin to a rename
namespace feature, which is something fundamentally different than
moving groups. like I said, we can still have it, but it might be better
coupled with a special maintenance mode (which is an even bigger hammer,
but one we already have that doesn't otherwise affect the regular hot
path).

> you're point makes sense, i think the "extra" locking it introduces
> everywhere is the best reason against ns locks. as for scoping, i think
> locking a whole ns before moving that ns does make sense though.
> 
> there may be leftovers regardless of locks in case of s3 backends. but
> these would then have to be moved manually, and if that is more than a
> few it gets cumbersome rather fast. what we could is is just lock
> everything, then move..

the same is true when deleting a namespace though - it is best effort,
and if something remains at the end you need to retry or live with it ;)

I don't think locking all groups and their snapshots at the same time is
an option, but I also don't think partially moving is that bad.

we could even collect groups which have conflicting tasks (failure to
obtain exclusive lock), and then retry them at the end?

one question is what to do with groups where a snapshot in the middle is
not lockable - do we want to move everything up to that point, and leave
the rest? or do we want to lock all snapshots up-front, and only move
the group wholesale if possible, and not at all if not? when destroying
a group, we remove snapshots until the first one fails, so there is some
precedent for doing it one-by-one at that level as well..

> either way thanks for the feedback!
> not opposed to dropping ns locks, you do have very good points :)
> 
>> I hope my comments on the other patches make it clear what I mean - I am
>> open for arguments for the current approach with namespace locks, but at
>> the moment I am not yet convinced this is the better approach..
>> 
>> skipping the rest of the implementation below (for now)
>> 
>>> +    ///
>>> +    /// 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(
>> 
>> other than noting that this particular fn is *very* long ;)
>> 
>>> [..]
> 
> 




  reply	other threads:[~2026-03-25  8:55 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-19 16:13 [PATCH proxmox-backup v5 0/9] fixes #6195: add support for moving groups and namespaces Hannes Laimer
2026-03-19 16:13 ` [PATCH proxmox-backup v5 1/9] ui: show empty groups Hannes Laimer
2026-03-24 10:28   ` Dominik Csapak
2026-03-19 16:13 ` [PATCH proxmox-backup v5 2/9] datastore: add namespace-level locking Hannes Laimer
2026-03-24 13:00   ` Fabian Grünbichler
2026-03-19 16:13 ` [PATCH proxmox-backup v5 3/9] datastore: add move_group Hannes Laimer
2026-03-24 13:00   ` Fabian Grünbichler
2026-03-25  6:20     ` Hannes Laimer
2026-03-19 16:13 ` [PATCH proxmox-backup v5 4/9] datastore: add move_namespace Hannes Laimer
2026-03-24 13:00   ` Fabian Grünbichler
2026-03-25  6:32     ` Hannes Laimer
2026-03-25  8:55       ` Fabian Grünbichler [this message]
2026-03-19 16:13 ` [PATCH proxmox-backup v5 5/9] api: add PUT endpoint for move_group Hannes Laimer
2026-03-19 16:13 ` [PATCH proxmox-backup v5 6/9] api: add PUT endpoint for move_namespace Hannes Laimer
2026-03-19 16:13 ` [PATCH proxmox-backup v5 7/9] ui: add move group action Hannes Laimer
2026-03-24 10:41   ` Dominik Csapak
2026-03-19 16:13 ` [PATCH proxmox-backup v5 8/9] ui: add move namespace action Hannes Laimer
2026-03-24 10:56   ` Dominik Csapak
2026-03-19 16:13 ` [PATCH proxmox-backup v5 9/9] cli: add move-namespace and move-group commands Hannes Laimer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1774427682.fqtrys91x3.astroid@yuna.none \
    --to=f.gruenbichler@proxmox.com \
    --cc=h.laimer@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal