From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 63EA11FF144 for ; Tue, 24 Mar 2026 14:00:04 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B4EC7106E5; Tue, 24 Mar 2026 14:00:24 +0100 (CET) Date: Tue, 24 Mar 2026 14:00:17 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH proxmox-backup v5 3/9] datastore: add move_group To: Hannes Laimer , pbs-devel@lists.proxmox.com References: <20260319161325.206846-1-h.laimer@proxmox.com> <20260319161325.206846-4-h.laimer@proxmox.com> In-Reply-To: <20260319161325.206846-4-h.laimer@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1774354673.7zvzlsmevd.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774357173843 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: I72SSEY2IFSDDBMV22C2IWWWQAWLHNIP X-Message-ID-Hash: I72SSEY2IFSDDBMV22C2IWWWQAWLHNIP X-MailFrom: f.gruenbichler@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On March 19, 2026 5:13 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. >=20 > 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(). >=20 > Signed-off-by: Hannes Laimer > --- > pbs-datastore/src/backup_info.rs | 149 ++++++++++++++++++++++++++++++- > pbs-datastore/src/datastore.rs | 49 ++++++++++ > 2 files changed, 197 insertions(+), 1 deletion(-) >=20 > diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_= info.rs > index 57e0448f..5931b7b5 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; > =20 > -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; > =20 > @@ -273,6 +273,153 @@ impl BackupGroup { > Ok(delete_stats) > } > =20 > + /// 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 fir= st, 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 s= ource namespace (as in > + /// `move_namespace`) or both a shared namespace lock and an exclusi= ve group lock (as in > + /// `move_group`). This is required to prevent concurrent writers fr= om 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 =3D self.full_group_path(); > + let target_path =3D self.store.group_path(target_ns, &self.group= ); > + > + log::info!("moving backup group {src_path:?} to {target_path:?}"= ); > + > + match backend { > + DatastoreBackend::Filesystem =3D> { > + std::fs::rename(&src_path, &target_path).with_context(||= { > + format!("failed to move group {src_path:?} to {targe= t_path:?}") > + })?; > + // The caller's lock guard still holds the FD open and w= ill harmlessly fail to > + // remove this path when dropped. > + let _ =3D std::fs::remove_file(self.lock_path()); just concentrating on this variant for now.. > + } > + DatastoreBackend::S3(s3_client) =3D> { > [.. thus snipping this ..] > + } > + } > + > + Ok(()) > + } > + > /// Helper function, assumes that no more snapshots are present in t= he group. > fn remove_group_dir(&self) -> Result<(), Error> { > let note_path =3D self.store.group_notes_path(&self.ns, &self.gr= oup); > diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore= .rs > index 18712074..d4c88452 100644 > --- a/pbs-datastore/src/datastore.rs > +++ b/pbs-datastore/src/datastore.rs > @@ -1017,6 +1017,55 @@ impl DataStore { > backup_group.destroy(&self.backend()?) > } > =20 > + /// Move a single backup group to a different namespace within the s= ame datastore. > + /// > + /// Acquires shared namespace locks on both the source and target na= mespaces, and an exclusive > + /// group lock on the source group to prevent concurrent writes to t= he same group. > + pub fn move_group( > + self: &Arc, > + source_ns: &BackupNamespace, > + group: &pbs_api_types::BackupGroup, > + target_ns: &BackupNamespace, > + ) -> Result<(), Error> { > + if source_ns =3D=3D target_ns { > + bail!("source and target namespace must be different"); > + } > + > + let source_group =3D self.backup_group(source_ns.clone(), group.= clone()); > + let target_group =3D self.backup_group(target_ns.clone(), group.= clone()); > + > + let _src_ns_lock =3D > + lock_namespace_shared(self.name(), source_ns, Some(NS_SHARED= _LOCK_TIMEOUT)) > + .with_context(|| format!("failed to lock source namespac= e '{source_ns}'"))?; > + let _tgt_ns_lock =3D > + lock_namespace_shared(self.name(), target_ns, Some(NS_SHARED= _LOCK_TIMEOUT)) > + .with_context(|| format!("failed to lock target namespac= e '{target_ns}'"))?; > + > + let _group_lock =3D source_group > + .lock() > + .with_context(|| format!("failed to lock group '{group}' for= move"))?; this breaks concurrently running readers, verification tasks, .. that have already started working on a snapshot within that particular group.. compare this to deleting a group, which will: - obtain an exclusive lock on the group (preventing concurrent snapshot additions/removals) - obtain an exclusive lock on each snapshot in turn (which will fail if there is already a reader/.. task operating on it) I don't think we can take such shortcuts for this feature, even if it means making it a bit more expensive also for the file system case. but if we can't take such shortcuts.. we don't really benefit from namespace locks anymore, since we need to recursively lock and move best effort anyway.. we could still implement a fast version that is coupled with a maintenance mode (that in turn forbids tasks other than moving), if we really want to? > + > + // Check existence under locks to avoid TOCTOU races with concur= rent 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 =3D self.backend()?; > + > + std::fs::create_dir_all(self.type_path(target_ns, group.ty)).wit= h_context(|| { > + format!("failed to create type directory in '{target_ns}' fo= r move") > + })?; > + > + source_group.move_to(target_ns, &backend) > + } > + > /// Remove a backup directory including all content > pub fn remove_backup_dir( > self: &Arc, > --=20 > 2.47.3 >=20 >=20 >=20 >=20 >=20 >=20