[pdm-devel] [PATCH datacenter-manager 0/3] refactor api caching & try to reuse the cache

[pdm-devel] [PATCH datacenter-manager 0/3] refactor api caching & try to reuse the cache

[Dominik Csapak]

This series introduces a small refactored struct to access the api cache
(like we used for resources + subscription) so that we don't have to
repeat the same way for every bit of data we want to cache.

It also shows how we can add a mechanism to live update this cache
for e.g. pve resources api call and utilize it when the
remote is not reachable

Dominik Csapak (3):
  server: api: resources: change use of remote to reference
  server: introduce ApiCache abstraction
  api/ui: pve: get cluster resources from cache if remote is unreachable

 lib/pdm-client/src/lib.rs                    |  10 +-
 server/src/api/pve/mod.rs                    |  48 ++++--
 server/src/api/resources.rs                  | 165 +++++--------------
 server/src/api_cache.rs                      |  87 ++++++++++
 server/src/lib.rs                            |   1 +
 server/src/metric_collection/top_entities.rs |   2 +-
 ui/src/pve/mod.rs                            |  14 +-
 7 files changed, 179 insertions(+), 148 deletions(-)
 create mode 100644 server/src/api_cache.rs

-- 
2.47.3



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

[pdm-devel] [PATCH datacenter-manager 1/3] server: api: resources: change use of remote to reference

[Dominik Csapak]

we don't need to take ownership here, and it makes the code easier
later.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 server/src/api/resources.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
index f4f56bcf..39a400c3 100644
--- a/server/src/api/resources.rs
+++ b/server/src/api/resources.rs
@@ -255,7 +255,7 @@ pub(crate) async fn get_resources_impl(
         }
         let filter = filters.clone();
         let handle = tokio::spawn(async move {
-            let (mut resources, error) = match get_resources_for_remote(remote, max_age).await {
+            let (mut resources, error) = match get_resources_for_remote(&remote, max_age).await {
                 Ok(resources) => (resources, None),
                 Err(error) => {
                     tracing::debug!("failed to get resources from remote - {error:?}");
@@ -700,7 +700,7 @@ static CACHE: LazyLock<RwLock<HashMap<String, CachedResources>>> =
 ///
 /// If recent enough cached data is available, it is returned
 /// instead of calling out to the remote.
-async fn get_resources_for_remote(remote: Remote, max_age: u64) -> Result<Vec<Resource>, Error> {
+async fn get_resources_for_remote(remote: &Remote, max_age: u64) -> Result<Vec<Resource>, Error> {
     let remote_name = remote.id.to_owned();
     if let Some(cached_resource) = get_cached_resources(&remote_name, max_age) {
         Ok(cached_resource.resources)
@@ -756,13 +756,13 @@ fn update_cached_resources(remote: &str, resources: &[Resource], now: i64) {
 }
 
 /// Fetch remote resources and map to pdm-native data types.
-async fn fetch_remote_resource(remote: Remote) -> Result<Vec<Resource>, Error> {
+async fn fetch_remote_resource(remote: &Remote) -> Result<Vec<Resource>, Error> {
     let mut resources = Vec::new();
     let remote_name = remote.id.to_owned();
 
     match remote.ty {
         RemoteType::Pve => {
-            let client = connection::make_pve_client(&remote)?;
+            let client = connection::make_pve_client(remote)?;
 
             let cluster_resources = client.cluster_resources(None).await?;
 
@@ -773,7 +773,7 @@ async fn fetch_remote_resource(remote: Remote) -> Result<Vec<Resource>, Error> {
             }
         }
         RemoteType::Pbs => {
-            let client = connection::make_pbs_client(&remote)?;
+            let client = connection::make_pbs_client(remote)?;
 
             let status = client.node_status().await?;
             resources.push(map_pbs_node_status(&remote_name, status));
-- 
2.47.3



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

[pdm-devel] [PATCH datacenter-manager 2/3] server: introduce ApiCache abstraction

[Dominik Csapak]

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 <d.csapak@proxmox.com>
---
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<String, Option<NodeSubscriptionInfo>>,
-    timestamp: i64,
-}
-
-static SUBSCRIPTION_CACHE: LazyLock<RwLock<HashMap<String, CachedSubscriptionState>>> =
-    LazyLock::new(|| RwLock::new(HashMap::new()));
+static SUBSCRIPTION_CACHE: LazyLock<ApiCache<HashMap<String, Option<NodeSubscriptionInfo>>>> =
+    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<HashMap<String, Option<NodeSubscriptionInfo>>, 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<CachedSubscriptionState> {
-    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<String, Option<NodeSubscriptionInfo>>,
-    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<Resource>,
-    pub timestamp: i64,
-}
-
-static CACHE: LazyLock<RwLock<HashMap<String, CachedResources>>> =
-    LazyLock::new(|| RwLock::new(HashMap::new()));
+static CACHE: LazyLock<ApiCache<Vec<Resource>>> = LazyLock::new(ApiCache::new);
 
 /// Get resources for a given remote.
 ///
@@ -702,57 +638,16 @@ static CACHE: LazyLock<RwLock<HashMap<String, CachedResources>>> =
 /// instead of calling out to the remote.
 async fn get_resources_for_remote(remote: &Remote, max_age: u64) -> Result<Vec<Resource>, 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<CachedResources> {
-    // 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<Vec<Resource>> {
+    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<CacheType> {
+    data: RwLock<HashMap<String, (i64, CacheType)>>,
+}
+
+impl<CacheType> ApiCache<CacheType> {
+    /// 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<CacheType> Default for ApiCache<CacheType> {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
+impl<CacheType> ApiCache<CacheType>
+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<CacheType> {
+        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<CacheType> ApiCache<CacheType>
+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<S, F, R>(
+        &self,
+        remote: &str,
+        max_age: i64,
+        update_func: F,
+    ) -> Result<CacheType, anyhow::Error>
+    where
+        S: Into<CacheType>,
+        F: Fn(&str) -> R,
+        R: Future<Output = Result<S, anyhow::Error>>,
+    {
+        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

[pdm-devel] [RFC PATCH datacenter-manager 3/3] api/ui: pve: get cluster resources from cache if remote is unreachable

[Dominik Csapak]

so that the last cached data can be shown instead of an error.
This now means that we'll always use the full api call here and filter
on the PDM side to get a fresh version in the cache.

The 'kind' parameter is not sent to the pve backend anymore but it's
filtered on the pdm side.

To show a notice in the ui that it was loaded from the cache, we need to
add a marker property in the result, and need to extract that in the
pdm-client so the gui has access to that.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
sending as rfc because
* it changes how we handle the 'kind' parameter
* we may want to opt in to that behaviour?
* not sure if a result side channel is the best way to convey the
  'from-cache' state.

 lib/pdm-client/src/lib.rs   | 10 ++++++--
 server/src/api/pve/mod.rs   | 48 ++++++++++++++++++++++++-------------
 server/src/api/resources.rs | 20 ++++++++++++++++
 ui/src/pve/mod.rs           | 14 +++++++----
 4 files changed, 70 insertions(+), 22 deletions(-)

diff --git a/lib/pdm-client/src/lib.rs b/lib/pdm-client/src/lib.rs
index 2f36fab1..3ebb1c35 100644
--- a/lib/pdm-client/src/lib.rs
+++ b/lib/pdm-client/src/lib.rs
@@ -408,11 +408,17 @@ impl<T: HttpApiClient> PdmClient<T> {
         &self,
         remote: &str,
         kind: Option<pve_api_types::ClusterResourceKind>,
-    ) -> Result<Vec<PveResource>, Error> {
+    ) -> Result<(bool, Vec<PveResource>), Error> {
         let query = ApiPathBuilder::new(format!("/api2/extjs/pve/remotes/{remote}/resources"))
             .maybe_arg("kind", &kind)
             .build();
-        Ok(self.0.get(&query).await?.expect_json()?.data)
+        let api_response_data = self.0.get(&query).await?.expect_json()?;
+        let from_cache = api_response_data
+            .attribs
+            .get("from-cache")
+            .and_then(|val| val.as_bool())
+            .unwrap_or(false);
+        Ok((from_cache, api_response_data.data))
     }
 
     pub async fn pve_cluster_status(
diff --git a/server/src/api/pve/mod.rs b/server/src/api/pve/mod.rs
index 0de82323..c16d6de5 100644
--- a/server/src/api/pve/mod.rs
+++ b/server/src/api/pve/mod.rs
@@ -16,19 +16,18 @@ use proxmox_sortable_macro::sortable;
 use pdm_api_types::remotes::{
     NodeUrl, Remote, RemoteListEntry, RemoteType, TlsProbeOutcome, REMOTE_ID_SCHEMA,
 };
-use pdm_api_types::resource::PveResource;
+use pdm_api_types::resource::{PveResource, Resource};
 use pdm_api_types::{
     Authid, RemoteUpid, HOST_OPTIONAL_PORT_FORMAT, PRIV_RESOURCE_AUDIT, PRIV_RESOURCE_DELETE,
     PRIV_SYS_MODIFY,
 };
 
 use pve_api_types::ClusterNodeStatus;
+use pve_api_types::ClusterResourceKind;
 use pve_api_types::ListRealm;
 use pve_api_types::PveUpid;
-use pve_api_types::{ClusterResourceKind, ClusterResourceType};
-
-use super::resources::{map_pve_lxc, map_pve_node, map_pve_qemu, map_pve_storage};
 
+use crate::api::resources::get_updated_resources_or_cache;
 use crate::connection::PveClient;
 use crate::connection::{self, probe_tls_connection};
 use crate::remote_tasks;
@@ -202,13 +201,29 @@ pub async fn cluster_resources(
         http_bail!(UNAUTHORIZED, "user has no access to resource list");
     }
 
-    let cluster_resources = connect_to_remote(&remotes, &remote)?
-        .cluster_resources(kind)
-        .await?
+    let remote = get_remote(&remotes, &remote)?;
+
+    let (from_cache, cluster_resources) = get_updated_resources_or_cache(remote).await?;
+
+    rpcenv.result_attrib_mut()["from-cache"] = from_cache.into();
+
+    let cluster_resources = cluster_resources
         .into_iter()
-        .filter_map(|r| map_pve_resource(&remote, r));
+        .filter(|res| match kind {
+            Some(kind) => matches!(
+                (kind, res),
+                (ClusterResourceKind::Vm, Resource::PveQemu(_))
+                    | (ClusterResourceKind::Vm, Resource::PveLxc(_))
+                    | (ClusterResourceKind::Storage, Resource::PveStorage(_))
+                    | (ClusterResourceKind::Node, Resource::PveNode(_))
+                    | (ClusterResourceKind::Sdn, Resource::PveSdn(_))
+            ),
+            None => true,
+        })
+        .filter_map(map_pve_resource)
+        .collect();
 
-    Ok(cluster_resources.collect())
+    Ok(cluster_resources)
 }
 
 #[api(
@@ -245,13 +260,14 @@ pub async fn cluster_status(
     Ok(status)
 }
 
-fn map_pve_resource(remote: &str, resource: pve_api_types::ClusterResource) -> Option<PveResource> {
-    match resource.ty {
-        ClusterResourceType::Node => map_pve_node(remote, resource).map(PveResource::Node),
-        ClusterResourceType::Lxc => map_pve_lxc(remote, resource).map(PveResource::Lxc),
-        ClusterResourceType::Qemu => map_pve_qemu(remote, resource).map(PveResource::Qemu),
-        ClusterResourceType::Storage => map_pve_storage(remote, resource).map(PveResource::Storage),
-        _ => None,
+fn map_pve_resource(resource: Resource) -> Option<PveResource> {
+    match resource {
+        Resource::PveStorage(storage) => Some(PveResource::Storage(storage)),
+        Resource::PveQemu(qemu) => Some(PveResource::Qemu(qemu)),
+        Resource::PveLxc(lxc) => Some(PveResource::Lxc(lxc)),
+        Resource::PveNode(node) => Some(PveResource::Node(node)),
+        Resource::PveSdn(sdn) => Some(PveResource::Sdn(sdn)),
+        Resource::PbsNode(_) | Resource::PbsDatastore(_) => None,
     }
 }
 
diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
index 5b7ac524..5b876998 100644
--- a/server/src/api/resources.rs
+++ b/server/src/api/resources.rs
@@ -645,6 +645,26 @@ async fn get_resources_for_remote(remote: &Remote, max_age: u64) -> Result<Vec<R
         .await
 }
 
+/// Tries to update the resources and returns data from the cache if it was not
+/// possible to reach the remote. This is indacted in the return value when
+/// `(true, data)` is returned.
+pub async fn get_updated_resources_or_cache(
+    remote: &Remote,
+) -> Result<(bool, Vec<Resource>), Error> {
+    match CACHE
+        .get_updated_value(&remote.id, 0, |_| async move {
+            fetch_remote_resource(remote).await
+        })
+        .await
+    {
+        Ok(data) => Ok((false, data)),
+        Err(err) => match CACHE.get_cached_value(&remote.id, i64::MAX) {
+            Some(data) => Ok((true, data)),
+            None => Err(err.context("cache was empty and could not update")),
+        },
+    }
+}
+
 /// Read cached resource data from the cache
 pub fn get_cached_resources(remote: &str, max_age: u64) -> Option<Vec<Resource>> {
     CACHE.get_cached_value(remote, max_age as i64)
diff --git a/ui/src/pve/mod.rs b/ui/src/pve/mod.rs
index 058424a9..2cdc99f7 100644
--- a/ui/src/pve/mod.rs
+++ b/ui/src/pve/mod.rs
@@ -11,7 +11,7 @@ use yew::{
 use pwt::{
     css::AlignItems,
     state::NavigationContainer,
-    widget::{Button, Container, Fa},
+    widget::{error_message, Button, Container, Fa},
 };
 use pwt::{
     css::FlexFit,
@@ -122,13 +122,14 @@ impl GuestInfo {
 
 pub enum Msg {
     SelectedView(tree::PveTreeNode),
-    ResourcesList(Result<Vec<PveResource>, Error>),
+    ResourcesList(Result<(bool, Vec<PveResource>), Error>),
 }
 
 pub struct PveRemoteComp {
     view: tree::PveTreeNode,
     resources: Rc<Vec<PveResource>>,
     last_error: Option<String>,
+    from_cache: bool,
 }
 
 impl LoadableComponent for PveRemoteComp {
@@ -142,6 +143,7 @@ impl LoadableComponent for PveRemoteComp {
             view: PveTreeNode::Root,
             resources: Rc::new(Vec::new()),
             last_error: None,
+            from_cache: false,
         }
     }
 
@@ -151,8 +153,9 @@ impl LoadableComponent for PveRemoteComp {
                 self.view = node;
             }
             Msg::ResourcesList(res) => match res {
-                Ok(res) => {
+                Ok((from_cache, res)) => {
                     self.last_error = None;
+                    self.from_cache = from_cache;
                     self.resources = Rc::new(res);
                 }
                 Err(err) => {
@@ -245,7 +248,10 @@ impl LoadableComponent for PveRemoteComp {
                                     let link = link.clone();
                                     move |_| link.send_reload()
                                 },
-                            )),
+                            ))
+                            .with_optional_child(self.from_cache.then_some(error_message(&tr!(
+                                "Could not reach remote, data shown from cache."
+                            )))),
                     ),
             )
             .with_child(
-- 
2.47.3



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

Re: [pdm-devel] [RFC PATCH datacenter-manager 3/3] api/ui: pve: get cluster resources from cache if remote is unreachable

[Lukas Wagner]

On Mon Oct 13, 2025 at 10:33 AM CEST, Dominik Csapak wrote:
> so that the last cached data can be shown instead of an error.
> This now means that we'll always use the full api call here and filter
> on the PDM side to get a fresh version in the cache.
>
> The 'kind' parameter is not sent to the pve backend anymore but it's
> filtered on the pdm side.
>
> To show a notice in the ui that it was loaded from the cache, we need to
> add a marker property in the result, and need to extract that in the
> pdm-client so the gui has access to that.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> sending as rfc because
> * it changes how we handle the 'kind' parameter
> * we may want to opt in to that behaviour?
> * not sure if a result side channel is the best way to convey the
>   'from-cache' state.

I think think this should be part of the API response type, the
'attribs' mechanic feels a bit too magic for me.

So instead of return a Vec<PveResource>, maybe a PveResourceList

#[api]
struct PveResourceList {
    resources: Vec<PveResource>,
    cached: bool,
    cache_timestamp: ... // Could be useful as well
}

Recently I've started to avoid returning a bare Vec<...> from API
endpoints in general, basically exactly for cases like these, where we
want to augment the endpoint with additional data later on.

>
>  lib/pdm-client/src/lib.rs   | 10 ++++++--
>  server/src/api/pve/mod.rs   | 48 ++++++++++++++++++++++++-------------
>  server/src/api/resources.rs | 20 ++++++++++++++++
>  ui/src/pve/mod.rs           | 14 +++++++----
>  4 files changed, 70 insertions(+), 22 deletions(-)
>
> diff --git a/lib/pdm-client/src/lib.rs b/lib/pdm-client/src/lib.rs
> index 2f36fab1..3ebb1c35 100644
> --- a/lib/pdm-client/src/lib.rs
> +++ b/lib/pdm-client/src/lib.rs
> @@ -408,11 +408,17 @@ impl<T: HttpApiClient> PdmClient<T> {
>          &self,
>          remote: &str,
>          kind: Option<pve_api_types::ClusterResourceKind>,
> -    ) -> Result<Vec<PveResource>, Error> {
> +    ) -> Result<(bool, Vec<PveResource>), Error> {
>          let query = ApiPathBuilder::new(format!("/api2/extjs/pve/remotes/{remote}/resources"))
>              .maybe_arg("kind", &kind)
>              .build();
> -        Ok(self.0.get(&query).await?.expect_json()?.data)
> +        let api_response_data = self.0.get(&query).await?.expect_json()?;
> +        let from_cache = api_response_data
> +            .attribs
> +            .get("from-cache")
> +            .and_then(|val| val.as_bool())
> +            .unwrap_or(false);
> +        Ok((from_cache, api_response_data.data))
>      }
>  
>      pub async fn pve_cluster_status(
> diff --git a/server/src/api/pve/mod.rs b/server/src/api/pve/mod.rs
> index 0de82323..c16d6de5 100644
> --- a/server/src/api/pve/mod.rs
> +++ b/server/src/api/pve/mod.rs
> @@ -16,19 +16,18 @@ use proxmox_sortable_macro::sortable;
>  use pdm_api_types::remotes::{
>      NodeUrl, Remote, RemoteListEntry, RemoteType, TlsProbeOutcome, REMOTE_ID_SCHEMA,
>  };
> -use pdm_api_types::resource::PveResource;
> +use pdm_api_types::resource::{PveResource, Resource};
>  use pdm_api_types::{
>      Authid, RemoteUpid, HOST_OPTIONAL_PORT_FORMAT, PRIV_RESOURCE_AUDIT, PRIV_RESOURCE_DELETE,
>      PRIV_SYS_MODIFY,
>  };
>  
>  use pve_api_types::ClusterNodeStatus;
> +use pve_api_types::ClusterResourceKind;
>  use pve_api_types::ListRealm;
>  use pve_api_types::PveUpid;
> -use pve_api_types::{ClusterResourceKind, ClusterResourceType};
> -
> -use super::resources::{map_pve_lxc, map_pve_node, map_pve_qemu, map_pve_storage};
>  
> +use crate::api::resources::get_updated_resources_or_cache;
>  use crate::connection::PveClient;
>  use crate::connection::{self, probe_tls_connection};
>  use crate::remote_tasks;
> @@ -202,13 +201,29 @@ pub async fn cluster_resources(
>          http_bail!(UNAUTHORIZED, "user has no access to resource list");
>      }
>  
> -    let cluster_resources = connect_to_remote(&remotes, &remote)?
> -        .cluster_resources(kind)
> -        .await?
> +    let remote = get_remote(&remotes, &remote)?;
> +
> +    let (from_cache, cluster_resources) = get_updated_resources_or_cache(remote).await?;
> +
> +    rpcenv.result_attrib_mut()["from-cache"] = from_cache.into();
> +
> +    let cluster_resources = cluster_resources
>          .into_iter()
> -        .filter_map(|r| map_pve_resource(&remote, r));
> +        .filter(|res| match kind {
> +            Some(kind) => matches!(
> +                (kind, res),
> +                (ClusterResourceKind::Vm, Resource::PveQemu(_))
> +                    | (ClusterResourceKind::Vm, Resource::PveLxc(_))
> +                    | (ClusterResourceKind::Storage, Resource::PveStorage(_))
> +                    | (ClusterResourceKind::Node, Resource::PveNode(_))
> +                    | (ClusterResourceKind::Sdn, Resource::PveSdn(_))
> +            ),
> +            None => true,
> +        })
> +        .filter_map(map_pve_resource)
> +        .collect();
>  
> -    Ok(cluster_resources.collect())
> +    Ok(cluster_resources)
>  }
>  
>  #[api(
> @@ -245,13 +260,14 @@ pub async fn cluster_status(
>      Ok(status)
>  }
>  
> -fn map_pve_resource(remote: &str, resource: pve_api_types::ClusterResource) -> Option<PveResource> {
> -    match resource.ty {
> -        ClusterResourceType::Node => map_pve_node(remote, resource).map(PveResource::Node),
> -        ClusterResourceType::Lxc => map_pve_lxc(remote, resource).map(PveResource::Lxc),
> -        ClusterResourceType::Qemu => map_pve_qemu(remote, resource).map(PveResource::Qemu),
> -        ClusterResourceType::Storage => map_pve_storage(remote, resource).map(PveResource::Storage),
> -        _ => None,
> +fn map_pve_resource(resource: Resource) -> Option<PveResource> {
> +    match resource {
> +        Resource::PveStorage(storage) => Some(PveResource::Storage(storage)),
> +        Resource::PveQemu(qemu) => Some(PveResource::Qemu(qemu)),
> +        Resource::PveLxc(lxc) => Some(PveResource::Lxc(lxc)),
> +        Resource::PveNode(node) => Some(PveResource::Node(node)),
> +        Resource::PveSdn(sdn) => Some(PveResource::Sdn(sdn)),
> +        Resource::PbsNode(_) | Resource::PbsDatastore(_) => None,
>      }
>  }
>  
> diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
> index 5b7ac524..5b876998 100644
> --- a/server/src/api/resources.rs
> +++ b/server/src/api/resources.rs
> @@ -645,6 +645,26 @@ async fn get_resources_for_remote(remote: &Remote, max_age: u64) -> Result<Vec<R
>          .await
>  }
>  
> +/// Tries to update the resources and returns data from the cache if it was not
> +/// possible to reach the remote. This is indacted in the return value when
> +/// `(true, data)` is returned.
> +pub async fn get_updated_resources_or_cache(
> +    remote: &Remote,
> +) -> Result<(bool, Vec<Resource>), Error> {
> +    match CACHE
> +        .get_updated_value(&remote.id, 0, |_| async move {
> +            fetch_remote_resource(remote).await
> +        })
> +        .await
> +    {
> +        Ok(data) => Ok((false, data)),
> +        Err(err) => match CACHE.get_cached_value(&remote.id, i64::MAX) {
> +            Some(data) => Ok((true, data)),
> +            None => Err(err.context("cache was empty and could not update")),
> +        },
> +    }
> +}
> +
>  /// Read cached resource data from the cache
>  pub fn get_cached_resources(remote: &str, max_age: u64) -> Option<Vec<Resource>> {
>      CACHE.get_cached_value(remote, max_age as i64)
> diff --git a/ui/src/pve/mod.rs b/ui/src/pve/mod.rs
> index 058424a9..2cdc99f7 100644
> --- a/ui/src/pve/mod.rs
> +++ b/ui/src/pve/mod.rs
> @@ -11,7 +11,7 @@ use yew::{
>  use pwt::{
>      css::AlignItems,
>      state::NavigationContainer,
> -    widget::{Button, Container, Fa},
> +    widget::{error_message, Button, Container, Fa},
>  };
>  use pwt::{
>      css::FlexFit,
> @@ -122,13 +122,14 @@ impl GuestInfo {
>  
>  pub enum Msg {
>      SelectedView(tree::PveTreeNode),
> -    ResourcesList(Result<Vec<PveResource>, Error>),
> +    ResourcesList(Result<(bool, Vec<PveResource>), Error>),
>  }
>  
>  pub struct PveRemoteComp {
>      view: tree::PveTreeNode,
>      resources: Rc<Vec<PveResource>>,
>      last_error: Option<String>,
> +    from_cache: bool,
>  }
>  
>  impl LoadableComponent for PveRemoteComp {
> @@ -142,6 +143,7 @@ impl LoadableComponent for PveRemoteComp {
>              view: PveTreeNode::Root,
>              resources: Rc::new(Vec::new()),
>              last_error: None,
> +            from_cache: false,
>          }
>      }
>  
> @@ -151,8 +153,9 @@ impl LoadableComponent for PveRemoteComp {
>                  self.view = node;
>              }
>              Msg::ResourcesList(res) => match res {
> -                Ok(res) => {
> +                Ok((from_cache, res)) => {
>                      self.last_error = None;
> +                    self.from_cache = from_cache;
>                      self.resources = Rc::new(res);
>                  }
>                  Err(err) => {
> @@ -245,7 +248,10 @@ impl LoadableComponent for PveRemoteComp {
>                                      let link = link.clone();
>                                      move |_| link.send_reload()
>                                  },
> -                            )),
> +                            ))
> +                            .with_optional_child(self.from_cache.then_some(error_message(&tr!(
> +                                "Could not reach remote, data shown from cache."
> +                            )))),
>                      ),
>              )
>              .with_child(



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel

Re: [pdm-devel] [PATCH datacenter-manager 2/3] server: introduce ApiCache abstraction

[Lukas Wagner]

A couple of smaller comments inline.

In general: We should have unit tests for new or refactored code like this.
Should be pretty simple for ApiCache, the only 'problem' I see are the calls to
`proxmox_time::epoch_i64`, maybe ApiCache could have a test-only
`set_time` function to inject a timestamp for test cases. Maybe there
are better solutions for this though.

On Mon Oct 13, 2025 at 10:33 AM CEST, Dominik Csapak wrote:
> 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 <d.csapak@proxmox.com>
> ---
> 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<String, Option<NodeSubscriptionInfo>>,
> -    timestamp: i64,
> -}
> -
> -static SUBSCRIPTION_CACHE: LazyLock<RwLock<HashMap<String, CachedSubscriptionState>>> =
> -    LazyLock::new(|| RwLock::new(HashMap::new()));
> +static SUBSCRIPTION_CACHE: LazyLock<ApiCache<HashMap<String, Option<NodeSubscriptionInfo>>>> =
> +    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<HashMap<String, Option<NodeSubscriptionInfo>>, 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<CachedSubscriptionState> {
> -    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<String, Option<NodeSubscriptionInfo>>,
> -    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<Resource>,
> -    pub timestamp: i64,
> -}
> -
> -static CACHE: LazyLock<RwLock<HashMap<String, CachedResources>>> =
> -    LazyLock::new(|| RwLock::new(HashMap::new()));
> +static CACHE: LazyLock<ApiCache<Vec<Resource>>> = LazyLock::new(ApiCache::new);
>  
>  /// Get resources for a given remote.
>  ///
> @@ -702,57 +638,16 @@ static CACHE: LazyLock<RwLock<HashMap<String, CachedResources>>> =
>  /// instead of calling out to the remote.
>  async fn get_resources_for_remote(remote: &Remote, max_age: u64) -> Result<Vec<Resource>, 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<CachedResources> {
> -    // 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<Vec<Resource>> {
> +    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<CacheType> {
> +    data: RwLock<HashMap<String, (i64, CacheType)>>,
> +}
> +
> +impl<CacheType> ApiCache<CacheType> {
> +    /// 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<CacheType> Default for ApiCache<CacheType> {
> +    fn default() -> Self {
> +        Self::new()
> +    }
> +}
> +
> +impl<CacheType> ApiCache<CacheType>
> +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<CacheType> {

tiny nit:

I think since the whole thing itself is nothing more than a cache, the
'cached' part is already implied, so maybe just call it 'get_value'?

> +        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
> +        }
> +    }
> +}
> +

v ^ You can merge these two impl blocks.

> +impl<CacheType> ApiCache<CacheType>
> +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<S, F, R>(

tiny nit: How about `get_value_or_update`? Since you only update if it is too old
or does not exist?

> +        &self,
> +        remote: &str,
> +        max_age: i64,
> +        update_func: F,
> +    ) -> Result<CacheType, anyhow::Error>
> +    where
> +        S: Into<CacheType>,
> +        F: Fn(&str) -> R,
> +        R: Future<Output = Result<S, anyhow::Error>>,
> +    {
> +        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 {



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel