public inbox for pdm-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Lukas Wagner" <l.wagner@proxmox.com>
To: "Proxmox Datacenter Manager development discussion"
	<pdm-devel@lists.proxmox.com>
Cc: "pdm-devel" <pdm-devel-bounces@lists.proxmox.com>
Subject: Re: [pdm-devel] [RFC PATCH datacenter-manager 3/3] api/ui: pve: get cluster resources from cache if remote is unreachable
Date: Thu, 16 Oct 2025 11:09:24 +0200	[thread overview]
Message-ID: <DDJMNMFP4110.2P9D4T707RWH8@proxmox.com> (raw)
In-Reply-To: <20251013083806.1068917-4-d.csapak@proxmox.com>

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


      reply	other threads:[~2025-10-16  9:09 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-10-13  8:33 [pdm-devel] [PATCH datacenter-manager 0/3] refactor api caching & try to reuse the cache Dominik Csapak
2025-10-13  8:33 ` [pdm-devel] [PATCH datacenter-manager 1/3] server: api: resources: change use of remote to reference Dominik Csapak
2025-10-13  8:33 ` [pdm-devel] [PATCH datacenter-manager 2/3] server: introduce ApiCache abstraction Dominik Csapak
2025-10-16  9:09   ` Lukas Wagner
2025-10-13  8:33 ` [pdm-devel] [RFC PATCH datacenter-manager 3/3] api/ui: pve: get cluster resources from cache if remote is unreachable Dominik Csapak
2025-10-16  9:09   ` Lukas Wagner [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DDJMNMFP4110.2P9D4T707RWH8@proxmox.com \
    --to=l.wagner@proxmox.com \
    --cc=pdm-devel-bounces@lists.proxmox.com \
    --cc=pdm-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal