[pbs-devel] [PATCH proxmox-backup 5/6] api: admin: datastore: optimize `groups` api call

[Thomas Lamprecht]

Am 03.10.25 um 10:50 schrieb Dominik Csapak:
> Currently we always touch all files for each snapshot in a group when
> listing them, even though we don't need all that info.
> 
> We're only interested in getting either the last finished snapshot
> information, or the last unfinished one (which must the only one in
> normal use, we can't have multiple unfinished snapshots usually)
> 
> Instead of getting all the information upgfront, use the snapshot

s/upgfront/upfront/

> iterator of the group to get only the id, sort them by time, and
> use the first we're interested in, getting the snapshot specific info
> only for those we want to check.
> 
> In my (admittedly extreme) setup with ~600 groups with ~1000 snapshots
> each, this changes the time this api call needs from ~40s to <1s.
> (on a relatively fast disk).

nice! In the UI we mainly (only?) uses this for gathering the notes of
a group, FWICT?

> 
> While at it, lift the restriction of only returning groups with
> snapshots in them, now returning also empty ones.
> 
> To keep api compatibility, use a timestamp of 0 for those.
> (no valid backup could have been made at that time anyway)

lazy question: Is this dependent on the previous patches? From a quick
check, it looks like it might be rather independent and one could apply
this one already.

> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  src/api2/admin/datastore.rs | 45 +++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 0b133d166..2252dcfa4 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -166,36 +166,47 @@ fn backup_group_to_group_list_item(
>          return None;
>      }
>  
> -    let snapshots = match group.list_backups() {
> -        Ok(snapshots) => snapshots,
> +    let mut snapshots: Vec<_> = match group.iter_snapshots() {
> +        Ok(snapshots) => snapshots.collect::<Result<Vec<_>, Error>>().ok()?,
>          Err(_) => return None,
>      };
>  
>      let backup_count: u64 = snapshots.len() as u64;
> -    if backup_count == 0 {
> -        return None;
> -    }
> +    let last = if backup_count == 1 {
> +        // we may have only one unfinished snapshot
> +        snapshots.pop().and_then(|dir| BackupInfo::new(dir).ok())
> +    } else {
> +        // we either have no snapshots, or at least one finished one, since we cannot have
> +        // multiple unfinished ones
> +        snapshots.sort_by_key(|b| std::cmp::Reverse(b.backup_time()));
> +        snapshots
> +            .iter()
> +            .filter_map(|backup| match BackupInfo::new(backup.clone()) {
> +                Ok(info) => {
> +                    if info.is_finished() {
> +                        Some(info)
> +                    } else {
> +                        None
> +                    }
> +                }
> +                Err(_) => None,

nit: Could be written as a bit more concise way:

snapshots
    .iter()
    .filter_map(|backup| BackupInfo::new(backup.clone()).ok())
    .filter(|info| info.is_finished())
    .next()

With splitting the conversion to BackupInfo and filtering for finished
backups into two makes it a bit easier (or rather quicker) to follow to me.

> +            })
> +            .next()
> +    };
>  
> -    let last_backup = snapshots
> -        .iter()
> -        .fold(&snapshots[0], |a, b| {
> -            if a.is_finished() && a.backup_dir.backup_time() > b.backup_dir.backup_time() {
> -                a
> -            } else {
> -                b
> -            }
> -        })
> -        .to_owned();
> +    let (last_backup, files) = last
> +        .map(|info| (info.backup_dir.backup_time(), info.files))
> +        .unwrap_or((0, Vec::new()));
>  
>      let notes_path = datastore.group_notes_path(ns, group.as_ref());
>      let comment = file_read_firstline(notes_path).ok();
>  
>      let item = GroupListItem {
>          backup: group.into(),
> -        last_backup: last_backup.backup_dir.backup_time(),
> +        last_backup,
>          owner: Some(owner),
>          backup_count,
> -        files: last_backup.files,
> +        files,
>          comment,
>      };
>