Re: [RFC] add max-depth to the datastore snapshots API endpoint

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

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