From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id EF4711FF15C for ; Fri, 17 Oct 2025 14:03:32 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 91B22E79; Fri, 17 Oct 2025 14:03:54 +0200 (CEST) From: Dominik Csapak To: pdm-devel@lists.proxmox.com Date: Fri, 17 Oct 2025 14:00:50 +0200 Message-ID: <20251017120315.2723235-3-d.csapak@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20251017120315.2723235-1-d.csapak@proxmox.com> References: <20251017120315.2723235-1-d.csapak@proxmox.com> MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.974 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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 v2 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 --- changes from v1: * add new CacheUpdateResult struct for the 'update_or_get_cache' method * rename like Lukas suggested * combine impl blocks server/src/api/resources.rs | 142 +++---------------- server/src/api_cache.rs | 128 +++++++++++++++++ server/src/lib.rs | 1 + server/src/metric_collection/top_entities.rs | 2 +- 4 files changed, 150 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 c640e4a2..0df9c781 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,12 @@ 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, - }, - ); + let (data, _) = SUBSCRIPTION_CACHE + .get_value_or_update(&remote.id, max_age as i64, |_| async move { + fetch_remote_subscription_info(remote).await + }) + .await?; + Ok(data) } /// Maps a list of node subscription infos into a single [`RemoteSubscriptionState`] @@ -687,14 +631,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 +639,18 @@ 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) - } + let (data, _) = CACHE + .get_value_or_update(&remote_name, max_age as i64, |_| async move { + fetch_remote_resource(remote).await + }) + .await?; + Ok(data) } /// 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> { + let (data, _) = CACHE.get_value(remote, max_age as i64)?; + Some(data) } /// 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..64b4ec04 --- /dev/null +++ b/server/src/api_cache.rs @@ -0,0 +1,128 @@ +use std::{collections::HashMap, future::Future, sync::RwLock}; + +use anyhow::Error; + +pub struct CacheUpdateResult { + pub data: T, + pub cache: bool, + pub cached_timestamp: i64, + pub error: Option, +} + +/// 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 and the cached timestamp for the given remote if it's not older than `max_age` + pub fn get_value(&self, remote: &str, max_age: i64) -> Option<(CacheType, i64)> { + 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(), *timestamp)) + } + } else { + None + } + } + + /// 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_value_or_update( + &self, + remote: &str, + max_age: i64, + update_func: F, + ) -> Result<(CacheType, i64), Error> + where + S: Into, + F: Fn(&str) -> R, + R: Future>, + { + if let Some(cached_data) = self.get_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, now)) + } + } + + /// Returns data for the given remote. Tries to update the value (and cache). + /// If that's not possible tries to load any cached value. + pub async fn update_or_get_cache( + &self, + remote: &str, + update_func: F, + ) -> Result, Error> + where + S: Into, + F: Fn(&str) -> R, + R: Future>, + { + match update_func(remote).await { + Ok(data) => { + let data: CacheType = data.into(); + let now = proxmox_time::epoch_i64(); + + self.update_cache(remote, data.clone(), now); + Ok(CacheUpdateResult { + data, + cache: false, + cached_timestamp: now, + error: None, + }) + } + Err(err) => match self.get_value(remote, i64::MAX) { + Some((data, timestamp)) => Ok(CacheUpdateResult { + data, + cache: true, + cached_timestamp: timestamp, + error: Some(err), + }), + None => Err(err.context("cache was empty and could not update")), + }, + } + } +} 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