From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 711021FF133 for ; Mon, 11 May 2026 14:37:40 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 570041571C; Mon, 11 May 2026 14:37:39 +0200 (CEST) Message-ID: <11d83bb3-d8e7-430f-8af8-64181d78154c@proxmox.com> Date: Mon, 11 May 2026 14:37:22 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [RFC datacenter-manager 0/4] add generic, per-remote (and global) cache for remote API responses To: Lukas Wagner , pdm-devel@lists.proxmox.com References: <20260508150330.363622-1-l.wagner@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260508150330.363622-1-l.wagner@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1778502939628 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.050 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [lib.rs,proxmox-datacenter-privileged-api.rs,resources.rs] Message-ID-Hash: 46KA2GLCQD3FGF2GDZJMVYAQGSXNGF26 X-Message-ID-Hash: 46KA2GLCQD3FGF2GDZJMVYAQGSXNGF26 X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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(-) >