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 7523E1FF15E for ; Mon, 13 Oct 2025 10:38:24 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 540A3DD6B; Mon, 13 Oct 2025 10:38:41 +0200 (CEST) From: Dominik Csapak To: pdm-devel@lists.proxmox.com Date: Mon, 13 Oct 2025 10:33:33 +0200 Message-ID: <20251013083806.1068917-3-d.csapak@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20251013083806.1068917-1-d.csapak@proxmox.com> References: <20251013083806.1068917-1-d.csapak@proxmox.com> MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.973 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 KAM_MAILER 2 Automated Mailer Tag Left in Email SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pdm-devel] [PATCH datacenter-manager 2/3] server: introduce ApiCache abstraction X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" Abstract our resources/subscription cache mechanism away into a struct that takes care of the max-age logic and fetching/inserting. This can be useful to have in general, and we don't need to copy it every time. We still have to use a LazyLock at the usage site and give the correct Type, but we can deduplicate most of the access/update logic. Signed-off-by: Dominik Csapak --- note: there may be better ways to abstract that away, i just went with the most obvious way to not duplicate the methods cache/max-age handling we had. server/src/api/resources.rs | 139 +++---------------- server/src/api_cache.rs | 87 ++++++++++++ server/src/lib.rs | 1 + server/src/metric_collection/top_entities.rs | 2 +- 4 files changed, 106 insertions(+), 123 deletions(-) create mode 100644 server/src/api_cache.rs diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs index 39a400c3..5b7ac524 100644 --- a/server/src/api/resources.rs +++ b/server/src/api/resources.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; use std::str::FromStr; -use std::sync::{LazyLock, RwLock}; +use std::sync::LazyLock; use anyhow::{bail, format_err, Error}; use futures::future::join_all; @@ -28,6 +28,7 @@ use proxmox_sortable_macro::sortable; use proxmox_subscription::SubscriptionStatus; use pve_api_types::{ClusterResource, ClusterResourceType}; +use crate::api_cache::ApiCache; use crate::connection; use crate::metric_collection::top_entities; @@ -521,14 +522,8 @@ async fn get_top_entities( Ok(res) } -#[derive(Clone)] -struct CachedSubscriptionState { - node_info: HashMap>, - timestamp: i64, -} - -static SUBSCRIPTION_CACHE: LazyLock>> = - LazyLock::new(|| RwLock::new(HashMap::new())); +static SUBSCRIPTION_CACHE: LazyLock>>> = + LazyLock::new(ApiCache::new); /// Get the subscription state for a given remote. /// @@ -538,63 +533,11 @@ async fn get_subscription_info_for_remote( remote: &Remote, max_age: u64, ) -> Result>, Error> { - if let Some(cached_subscription) = get_cached_subscription_info(&remote.id, max_age) { - Ok(cached_subscription.node_info) - } else { - let node_info = fetch_remote_subscription_info(remote).await?; - let now = proxmox_time::epoch_i64(); - update_cached_subscription_info(&remote.id, &node_info, now); - Ok(node_info) - } -} - -fn get_cached_subscription_info(remote: &str, max_age: u64) -> Option { - let cache = SUBSCRIPTION_CACHE - .read() - .expect("subscription mutex poisoned"); - - if let Some(cached_subscription) = cache.get(remote) { - let now = proxmox_time::epoch_i64(); - let diff = now - cached_subscription.timestamp; - - if diff > max_age as i64 || diff < 0 { - // value is too old or from the future - None - } else { - Some(cached_subscription.clone()) - } - } else { - None - } -} - -/// Update cached subscription data. -/// -/// If the cache already contains more recent data we don't insert the passed resources. -fn update_cached_subscription_info( - remote: &str, - node_info: &HashMap>, - now: i64, -) { - // there is no good way to recover from this, so panicking should be fine - let mut cache = SUBSCRIPTION_CACHE - .write() - .expect("subscription mutex poisoned"); - - if let Some(cached_resource) = cache.get(remote) { - // skip updating if the data is new enough - if cached_resource.timestamp >= now { - return; - } - } - - cache.insert( - remote.into(), - CachedSubscriptionState { - node_info: node_info.clone(), - timestamp: now, - }, - ); + SUBSCRIPTION_CACHE + .get_updated_value(&remote.id, max_age as i64, |_| async move { + fetch_remote_subscription_info(remote).await + }) + .await } /// Maps a list of node subscription infos into a single [`RemoteSubscriptionState`] @@ -687,14 +630,7 @@ async fn fetch_remote_subscription_info( Ok(list) } -#[derive(Clone)] -pub struct CachedResources { - pub resources: Vec, - pub timestamp: i64, -} - -static CACHE: LazyLock>> = - LazyLock::new(|| RwLock::new(HashMap::new())); +static CACHE: LazyLock>> = LazyLock::new(ApiCache::new); /// Get resources for a given remote. /// @@ -702,57 +638,16 @@ static CACHE: LazyLock>> = /// instead of calling out to the remote. async fn get_resources_for_remote(remote: &Remote, max_age: u64) -> Result, Error> { let remote_name = remote.id.to_owned(); - if let Some(cached_resource) = get_cached_resources(&remote_name, max_age) { - Ok(cached_resource.resources) - } else { - let resources = fetch_remote_resource(remote).await?; - let now = proxmox_time::epoch_i64(); - update_cached_resources(&remote_name, &resources, now); - Ok(resources) - } + CACHE + .get_updated_value(&remote_name, max_age as i64, |_| async move { + fetch_remote_resource(remote).await + }) + .await } /// Read cached resource data from the cache -pub fn get_cached_resources(remote: &str, max_age: u64) -> Option { - // there is no good way to recover from this, so panicking should be fine - let cache = CACHE.read().expect("mutex poisoned"); - - if let Some(cached_resource) = cache.get(remote) { - let now = proxmox_time::epoch_i64(); - let diff = now - cached_resource.timestamp; - - if diff > max_age as i64 || diff < 0 { - // value is too old or from the future - None - } else { - Some(cached_resource.clone()) - } - } else { - None - } -} - -/// Update cached resource data. -/// -/// If the cache already contains more recent data we don't insert the passed resources. -fn update_cached_resources(remote: &str, resources: &[Resource], now: i64) { - // there is no good way to recover from this, so panicking should be fine - let mut cache = CACHE.write().expect("mutex poisoned"); - - if let Some(cached_resource) = cache.get(remote) { - // skip updating if existing data is newer - if cached_resource.timestamp >= now { - return; - } - } - - cache.insert( - remote.into(), - CachedResources { - timestamp: now, - resources: resources.into(), - }, - ); +pub fn get_cached_resources(remote: &str, max_age: u64) -> Option> { + CACHE.get_cached_value(remote, max_age as i64) } /// Fetch remote resources and map to pdm-native data types. diff --git a/server/src/api_cache.rs b/server/src/api_cache.rs new file mode 100644 index 00000000..1339fe64 --- /dev/null +++ b/server/src/api_cache.rs @@ -0,0 +1,87 @@ +use std::{collections::HashMap, future::Future, sync::RwLock}; + +/// Holds the given data per remote with a timestamp indicating the last update +pub struct ApiCache { + data: RwLock>, +} + +impl ApiCache { + /// Creates a new instance of `ApiCache` + pub fn new() -> Self { + Self { + data: RwLock::new(HashMap::new()), + } + } + + fn update_cache(&self, remote: &str, data: CacheType, now: i64) { + // there is no good way to recover from this, so panicking should be fine + let mut cache = self.data.write().expect("cache mutex poisoned"); + + if let Some((timestamp, _)) = cache.get(remote) { + // skip updating if existing data is newer + if *timestamp >= now { + return; + } + } + + cache.insert(remote.into(), (now, data)); + } +} + +impl Default for ApiCache { + fn default() -> Self { + Self::new() + } +} + +impl ApiCache +where + CacheType: Clone, +{ + /// Returns the cached value for the given remote if it's not older than `max_age` + pub fn get_cached_value(&self, remote: &str, max_age: i64) -> Option { + let cache = self.data.read().expect("cache mutex poisoned"); + if let Some((timestamp, cached_data)) = cache.get(remote) { + let now = proxmox_time::epoch_i64(); + let diff = now - timestamp; + if diff > max_age || diff < 0 { + // value is too old or from the future + None + } else { + Some(cached_data.clone()) + } + } else { + None + } + } +} + +impl ApiCache +where + CacheType: Clone, +{ + /// Returns data for the given remote that is not older than `max_age` either + /// from the cache or updated via the given `update_func` + pub async fn get_updated_value( + &self, + remote: &str, + max_age: i64, + update_func: F, + ) -> Result + where + S: Into, + F: Fn(&str) -> R, + R: Future>, + { + if let Some(cached_data) = self.get_cached_value(remote, max_age) { + Ok(cached_data) + } else { + let data = update_func(remote).await?; + let now = proxmox_time::epoch_i64(); + + let data: CacheType = data.into(); + self.update_cache(remote, data.clone(), now); + Ok(data) + } + } +} diff --git a/server/src/lib.rs b/server/src/lib.rs index 964807eb..1f8508c9 100644 --- a/server/src/lib.rs +++ b/server/src/lib.rs @@ -2,6 +2,7 @@ pub mod acl; pub mod api; +pub mod api_cache; pub mod auth; pub mod context; pub mod env; diff --git a/server/src/metric_collection/top_entities.rs b/server/src/metric_collection/top_entities.rs index ea121eef..715d0eeb 100644 --- a/server/src/metric_collection/top_entities.rs +++ b/server/src/metric_collection/top_entities.rs @@ -50,7 +50,7 @@ pub fn calculate_top( if let Some(data) = crate::api::resources::get_cached_resources(remote_name, i64::MAX as u64) { - for res in data.resources { + for res in data { let id = res.id().to_string(); let name = format!("pve/{remote_name}/{id}"); match &res { -- 2.47.3 _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel