Re: [PATCH proxmox-backup v7 2/9] datastore: add move-group

[Hannes Laimer <h.laimer@proxmox.com>]

On 2026-04-20 16:48, Fabian Grünbichler wrote:
> On April 16, 2026 7:18 pm, Hannes Laimer wrote:
>> Add support for moving a single backup group to a different namespace
>> within the same datastore.
>>
>> For the filesystem backend each snapshot directory is renamed
>> individually. For S3 all objects are copied to the target prefix
>> before deleting the source, per snapshot.
>>
>> Exclusive locks on the group and all its snapshots are acquired
>> before the move to ensure no concurrent operations are active.
>> Snapshots are locked and moved in batches to avoid exhausting file
>> descriptors on groups with many snapshots.
>>
>> If the target group already exists and merging is enabled, the
>> individual snapshots are moved into the existing target group
>> provided both groups have the same owner and the source snapshots
>> are strictly newer than the target snapshots.
>>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---

[..]

>> +    /// Lock and move a single group. Acquires exclusive locks on both source and target
>> +    /// group paths, then moves snapshots in batches - each batch is locked, moved, and
>> +    /// released before the next batch starts. This ensures no concurrent readers (e.g.
>> +    /// verify) can operate on snapshots being moved, without exhausting file descriptors.
>> +    ///
>> +    /// Returns a `MoveGroupError` on failure, indicating whether the failure was a
>> +    /// transient lock conflict (retryable) or a hard error.
>> +    pub(crate) fn lock_and_move_group(
> 
> this can be private
> 
>> +        self: &Arc<Self>,
>> +        group: &BackupGroup,
>> +        source_ns: &BackupNamespace,
>> +        target_ns: &BackupNamespace,
> 
> the order of parameters is different than above..
> 
>> +        backend: &DatastoreBackend,
>> +        merge_groups: bool,
>> +    ) -> Result<(), MoveGroupError> {
>> +        let target_group_ns = group
>> +            .backup_ns()
>> +            .map_prefix(source_ns, target_ns)
>> +            .map_err(MoveGroupError::Hard)?;
>> +
>> +        let target_group = self.backup_group(target_group_ns.clone(), group.group().clone());
>> +
>> +        // Acquire exclusive group locks - source prevents new snapshot additions/removals,
>> +        // target prevents concurrent creation at the destination path.
>> +        let _group_lock = group.lock().map_err(MoveGroupError::Soft)?;
>> +        let _target_group_lock = target_group.lock().map_err(MoveGroupError::Soft)?;
> 
> this (continued below)
> 
>> +        let merge = target_group.exists();
>> +
>> +        if merge && !merge_groups {
>> +            return Err(MoveGroupError::Hard(format_err!(
>> +                "group '{}' already exists in target namespace '{target_group_ns}' \
>> +                and merging is disabled",
>> +                group.group(),
>> +            )));
>> +        }
>> +
>> +        if merge {
>> +            group
>> +                .check_merge_invariants(&target_group_ns)
>> +                .map_err(MoveGroupError::Hard)?;
>> +        }
>> +
>> +        let mut snapshots: Vec<_> = group
>> +            .iter_snapshots()
>> +            .map_err(MoveGroupError::Hard)?
>> +            .collect::<Result<Vec<_>, _>>()
>> +            .map_err(MoveGroupError::Hard)?;
>> +        // Sort by time so that a partial failure (e.g. batch N succeeds but N+1
>> +        // fails) leaves a clean time-ordered split: all moved snapshots are older
>> +        // than all remaining ones. This allows a merge retry to succeed since the
>> +        // invariant (source oldest > target newest) still holds.
>> +        snapshots.sort_by_key(|s| s.backup_time());
> 
> if we need the full list of sorted backups, why not use
> group.list_backups() and BackupInfo::sort_list ?
> 

list_backups() does do extra IO (`list_backup_files()`), without us
actually needing it


thanks for taking a look! will address the rest in a v8 :)

>> +
>> +        // Ensure the target type and group directories exist.
>> +        std::fs::create_dir_all(self.type_path(&target_group_ns, group.group().ty))
>> +            .map_err(|err| MoveGroupError::Hard(err.into()))?;
>> +        let target_group_path = self.group_path(&target_group_ns, group.group());
>> +        if !merge {
>> +            std::fs::create_dir_all(&target_group_path)
>> +                .map_err(|err| MoveGroupError::Hard(err.into()))?;
>> +            // Copy owner so the group is visible in the UI during the move (especially
>> +            // relevant for S3 where the copy can take a while).
>> +            if let Ok(owner) = group.get_owner() {
>> +                if let Err(err) = target_group.set_owner(&owner, true) {
>> +                    warn!(
>> +                        "failed to set owner for target group '{}' in '{target_group_ns}': {err:#}",
>> +                        group.group(),
>> +                    );
>> +                }
>> +            }
>> +        }
> 
> (continued from above) and this could be replaced with a call to
> `create_locked_backup_group`. downside - differentiating between
> "currently locked" and "other error" is harder, but the upside is that
> that code already handles a lot of the logic for us, and is how backup,
> tape restore and pull jobs create new groups or add snapshots to
> existing groups..
> 
> maybe it would be worth it to refactor that implementation, and a add
> new non-pub variant/wrapper that
> - returns whether it created the group or not
> - allows us to differentiate locking from other errors?
> 
>> +
>> +        log::info!(
>> +            "{} group '{}/{}' from '{}' to '{target_group_ns}'",
>> +            if merge { "merging" } else { "moving" },
>> +            group.group().ty,
>> +            group.group().id,
>> +            group.backup_ns(),
>> +        );
>> +
>> +        // Lock and move snapshots in batches. Each batch is locked, moved, and released
>> +        // before the next batch starts. This ensures no concurrent reader (e.g. verify,
>> +        // which only takes a snapshot-level shared lock) can operate on a snapshot while
>> +        // it is being moved.
>> +        for chunk in snapshots.chunks(MAX_SNAPSHOT_LOCKS) {
>> +            let _batch_locks: Vec<_> = chunk
>> +                .iter()
>> +                .map(|snap| snap.lock().map_err(MoveGroupError::Soft))
>> +                .collect::<Result<Vec<_>, _>>()?;
> 
> this is funky, but I guess it does the job ;) I'd feel a bit more
> comfortable if the Vec<_> were explicit though..
> 
>> +
>> +            for snap in chunk {
>> +                group
>> +                    .move_snapshot(snap, &target_group_path, backend)
>> +                    .map_err(MoveGroupError::Hard)?;
> 
> see above, I think we can leverage the type system a bit more here..
> 
>> +            }
>> +            // batch locks released here
>> +        }
>> +
>> +        // Copy group notes to the target (unless merging into a group that already has notes).
>> +        let src_notes_path = self.group_notes_path(group.backup_ns(), group.group());
>> +        let dst_notes_path = self.group_notes_path(&target_group_ns, group.group());
>> +        if src_notes_path.exists() && !dst_notes_path.exists() {
>> +            if let Err(err) = std::fs::copy(&src_notes_path, &dst_notes_path) {
> 
> should this be a rename? should we warn if merging and the notes have
> diverged? what if this is on S3?
> 
>> +                warn!(
>> +                    "failed to copy group notes from {src_notes_path:?} to {dst_notes_path:?}: {err}"
>> +                );
>> +            }
>> +        }
>> +

[..]