Re: [PATCH proxmox-backup v4 2/7] datastore: add move_group

[Christian Ebner <c.ebner@proxmox.com>]

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>,
>>
>