Re: [RFC datacenter-manager 0/4] add generic, per-remote (and global) cache for remote API responses

[Dominik Csapak <d.csapak@proxmox.com>]

hi,

IMO the code looks mostly good to me, as well as the approach, a
few comments regardless (all here, as the patches themselves are
not that big ;) )

* using the filesystem can be good, it can reduce the memory footprint
   since we're not automatically store everything in memory

   this can also be a downside though, since now the IOPS requirements
   rise, and for frequent update cycles it could have a negative impact
   on the disks (ssd wearout)

   this could be mitigated by using e.g. /var/run instead of /var/cache
   but then it's back to using memory all the time, with less
   clear accounting.

   I'm actually not sure which is better, maybe having that configurable
   could make sense? (as in, whats the cache path? this way users can
   decide if they want it on disk or in memory)

* currently you expire values if the timestamp is in the future,
   could it be better to return an error here? IMO there had to be one
   at some point, either the time was wrong in the past, or the time
   is wrong now (neither of which is good?)

* not sure if the pdm_cache::instance() abstraction is gaining us
   anything here, we could simply have freestanding functions?

* for the subscription info you chose to implement an async wrapper
   maybe we could/should have that directly in the interface
   (of pdm_cache for example) ? otherwise people might use it
   without a 'spawn_blocking'. having both an async and sync interface
   could be beneficial

On 5/8/26 5:02 PM, Lukas Wagner wrote:
> The main intention is to avoid a sprawl of different caching approaches by
> establishing a simple, easy to use cache implementation that can be used to
> persistently cache API responses from remotes (and derived aggregations).
> 
> Open questions:
>    - is the per-namespace lock too coarse? Should we rather lock
>      per key? Anyways, one should not hold the lock during longer periods of time
>      (e.g. when doing API requests), so the namespace-level lock seemed fine to me.
>      A per-namespace (so, per-remote) lock is nicer when one wants to update several
>      keys in one go.
> 
>    - Base directory for cache, currently it is
>      /var/cache/proxmox-datacenter-manager/cache
>      But this seems both redundant and generic to me, so maybe
>      'api-cache'?
> 
>    - Went with a max_age param on `get` instead of a `set` with an expiry time,
>      I think it's quite common to have cache readers with different requirements
>      to value freshness, so this might be a better fit.
>      Also, we use the max-age mechanism in the API already, so this
>      is a seamless fit then. Does this make sense? Or this we rather have
>      redis-style `set` with expiry time/TTL?
> 
> 
> The `namespaced_cache` module is pretty generic and can be moved to proxmox.git
> (maybe in proxmox-shared-cache) once it has sufficiently stabilized.
> 
> 
> proxmox-datacenter-manager:
> 
> Lukas Wagner (4):
>    add persistent, generic, namespaced key-value cache implementation
>    add pdm_cache cache as a specialized wrapper around the namespaced
>      cache
>    api: resources: subscriptions: switch over to pdm_cache
>    remote-updates: switch over to pdm_cache
> 
>   Cargo.toml                                    |   1 +
>   server/Cargo.toml                             |   2 +
>   server/src/api/resources.rs                   |  82 ++---
>   .../bin/proxmox-datacenter-privileged-api.rs  |   7 +
>   server/src/lib.rs                             |   2 +
>   server/src/namespaced_cache.rs                | 324 ++++++++++++++++++
>   server/src/pdm_cache.rs                       |  69 ++++
>   server/src/remote_updates.rs                  |  52 +--
>   8 files changed, 452 insertions(+), 87 deletions(-)
>   create mode 100644 server/src/namespaced_cache.rs
>   create mode 100644 server/src/pdm_cache.rs
> 
> 
> Summary over all repositories:
>    8 files changed, 452 insertions(+), 87 deletions(-)
>