Hi!

On 8/22/26 2:30 AM, thomas moore wrote:
> Hi all
>
> This is a proposal for discussion, not a patch. The
> developer documentation asks that plans be discussed before
> starting work, so I'd like to check whether this would be
> acceptable before writing it.
>
> GET /api2/json/admin/datastore/{store}/snapshots returns only
> the snapshots in the requested namespace, so a client wanting
> a datastore-wide view has to call
> /admin/datastore/{store}/namespace first and then issue one
> snapshots request per namespace. I ran into this writing a
> third-party monitoring client.
>
> Tested against PBS 4.2:
>
> /admin/datastore/pbs/snapshots -> 485 snapshots (root
> only)
>
> /admin/datastore/pbs/snapshots?ns=test -> 1 snapshot
>
> /admin/datastore/pbs/snapshots?max-depth=7 -> 400
> parameter verification failed - 'max-depth': schema does not
> allow additional properties
>
> Proposal: accept an optional max-depth using the
> existing NS_MAX_DEPTH_SCHEMA, defaulting to 0 so behaviour is
> unchanged for current callers.
>
> This looks anticipated in the code. As of
> 5d95fc20e, src/api2/admin/datastore.rs lines 571, 575 and 583
> each carry a "// FIXME: Recursion" on the group
> construction in list_snapshots_blocking, with a further one at
> 565 about filtering by owner before collecting.
> ListAccessibleBackupGroups already takes max_depth and applies
> the per-namespace privilege and owner checks during iteration,
> and get_snapshots_count uses it in roughly the shape list_snapshots
> would need.
>
> I'd send it as two patches: the refactor
> onto ListAccessibleBackupGroups with no behavioural change, then
> max-depth on top. I can test on a 4.2 instance including with a
> token holding DatastoreAudit on the root namespace only, to
> confirm namespaces the token cannot see stay absent from a
> recursive listing.
>
>
>
>
>
> Questions for the list
>
> 1. Is adding max-depth to the snapshots endpoint acceptable in
> principle?

Yes, although there has already been some work on this [0], implementing
a streaming API which should already cover your case as well. This
however is not applied yet, but might be picked up again.

Nevertheless, may I ask what your goal is and what limitation which is
not already covered by the current API you are running into? Does
performing the additional requests cause to much latency for your usecase?

> 2. Is ListAccessibleBackupGroups the right vehicle here, or is
> there a reason list_snapshots     does not already use it?

list_snapshots_blocking() is rather heavy weight and performance
critical, especially for datastores with lots of groups/snapshots on
slow (spin rust) storage. So performance and memory analysis is a must
here. Further, current behavior must be retained also with respect to
group listing by type and backup id only, respectively. But again, I
think patch [0] already would cover what you are asking for.

> 3. Snapshot entries carry no namespace field, so a recursive
> listing is ambiguous unless the     client tracks it per request. Adding
> one would mean a new field on SnapshotListItem, which     is also
> decoded from remote instances in server/sync.rs and server/push.rs

While not that critical, adding the additional field does have
implications for the response payload size, especially for deeply nested
datastores with lots of snapshots.

> If you'd rather this were solved another way or not at all I'm
> happy to hear it before writing anything.
>
> Regards Thomas Moore

[0]
https://lore.proxmox.com/pbs-devel/20251008134344.3512958-8-d.csapak@proxmox.com/

Best regards,
Chris