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 53CA71FF17C for ; Wed, 3 Sep 2025 11:23:59 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BC52E2F48D; Wed, 3 Sep 2025 11:24:13 +0200 (CEST) Mime-Version: 1.0 Date: Wed, 03 Sep 2025 11:23:38 +0200 Message-Id: From: "Lukas Wagner" To: "Stefan Hanreich" , "Proxmox Datacenter Manager development discussion" X-Mailer: aerc 0.20.1-0-g2ecb8770224a References: <20250902151427.425017-1-l.wagner@proxmox.com> <20250902151427.425017-8-l.wagner@proxmox.com> <3b096098-d5c9-4108-8a92-717af8cf3978@proxmox.com> In-Reply-To: <3b096098-d5c9-4108-8a92-717af8cf3978@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1756891403471 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.027 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 RCVD_IN_MSPIKE_H2 0.001 Average reputation (+2) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pdm-devel] [PATCH proxmox-datacenter-manager 1/4] server: add api for getting available updates/changelogs for remote nodes 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" On Wed Sep 3, 2025 at 11:02 AM CEST, Stefan Hanreich wrote: > On 9/2/25 5:14 PM, Lukas Wagner wrote: >> This adds new APIs for update management: >> >> GET /pve/remotes/{remote}/nodes/{node}/apt/changelog >> -> get package changelog >> GET /pve/remotes/{remote}/nodes/{node}/apt/update >> -> get list of updatable packages >> POST /pve/remotes/{remote}/nodes/{node}/apt/update >> -> refresh APT database >> >> At this time these just pass the call through to PVE with no caching >> involved on the PDM side. This should be fine for this API, but once >> we have an API for 'give me a view of ALL available remote updates', >> we need to introduce a cache that is periodically refreshed. >> >> Signed-off-by: Lukas Wagner >> --- >> server/src/api/pve/apt.rs | 119 +++++++++++++++++++++++++++++++++++ >> server/src/api/pve/mod.rs | 3 +- >> server/src/api/pve/node.rs | 1 + >> server/src/lib.rs | 1 + >> server/src/remote_updates.rs | 96 ++++++++++++++++++++++++++++ >> 5 files changed, 219 insertions(+), 1 deletion(-) >> create mode 100644 server/src/api/pve/apt.rs >> create mode 100644 server/src/remote_updates.rs >> >> diff --git a/server/src/api/pve/apt.rs b/server/src/api/pve/apt.rs >> new file mode 100644 >> index 00000000..f5027fb8 >> --- /dev/null >> +++ b/server/src/api/pve/apt.rs >> @@ -0,0 +1,119 @@ >> +use anyhow::Error; >> + >> +use proxmox_apt_api_types::{APTGetChangelogOptions, APTUpdateInfo}; >> +use proxmox_router::{list_subdirs_api_method, Permission, Router, SubdirMap}; >> +use proxmox_schema::api; >> +use proxmox_schema::api_types::NODE_SCHEMA; >> + >> +use pdm_api_types::{remotes::REMOTE_ID_SCHEMA, RemoteUpid, PRIV_RESOURCE_MODIFY}; >> + >> +use crate::{api::remotes::get_remote, remote_updates}; >> + >> +#[api( >> + input: { >> + properties: { >> + remote: { >> + schema: REMOTE_ID_SCHEMA, >> + }, >> + node: { >> + schema: NODE_SCHEMA, >> + }, >> + }, >> + }, >> + returns: { >> + description: "A list of packages with available updates.", >> + type: Array, >> + items: { >> + type: APTUpdateInfo >> + }, >> + }, >> + access: { >> + permission: &Permission::Privilege(&["resource", "{remote}", "node", "{node}", "system"], PRIV_RESOURCE_MODIFY, false), >> + }, >> +)] >> +/// List available APT updates for a remote PVE node. >> +async fn apt_update_available(remote: String, node: String) -> Result, Error> { > > potentially fine to take a ref here for both params as well? Using a &str here leads to a compiler error error: implementation of `Deserialize` is not general enough > >> + let (config, _digest) = pdm_config::remotes::config()?; >> + let remote = get_remote(&config, &remote)?; >> + >> + let updates = remote_updates::list_available_updates(remote.clone(), &node).await?; >> + >> + Ok(updates) >> +} >> + >> +#[api( >> + input: { >> + properties: { >> + remote: { >> + schema: REMOTE_ID_SCHEMA, >> + }, >> + node: { >> + schema: NODE_SCHEMA, >> + }, >> + }, >> + }, >> + access: { >> + permission: &Permission::Privilege(&["resource", "{remote}", "node", "{node}", "system"], PRIV_RESOURCE_MODIFY, false), >> + }, >> +)] >> +/// Update the APT database of a remote PVE node. >> +pub async fn apt_update_database(remote: String, node: String) -> Result { > > here too then Same here. >> + let (config, _digest) = pdm_config::remotes::config()?; >> + let remote = get_remote(&config, &remote)?; >> + >> + let upid = remote_updates::update_apt_database(remote, &node).await?; >> + >> + Ok(upid) >> +} >> + >> +#[api( >> + input: { >> + properties: { >> + remote: { >> + schema: REMOTE_ID_SCHEMA, >> + }, >> + node: { >> + schema: NODE_SCHEMA, >> + }, >> + options: { >> + type: APTGetChangelogOptions, >> + flatten: true, >> + }, >> + }, >> + }, >> + returns: { >> + description: "The Package changelog.", >> + type: String, >> + }, >> + access: { >> + permission: &Permission::Privilege(&["resource", "{remote}", "node", "{node}", "system"], PRIV_RESOURCE_MODIFY, false), >> + }, >> +)] >> +/// Retrieve the changelog of the specified package for a remote PVE node. >> +async fn apt_get_changelog( >> + remote: String, >> + node: String, >> + options: APTGetChangelogOptions, > > here too then > Same here. >> +) -> Result { >> + let (config, _digest) = pdm_config::remotes::config()?; >> + let remote = get_remote(&config, &remote)?; >> + >> + remote_updates::get_changelog(remote.clone(), &node, options.name).await >> +} >> + >> +const SUBDIRS: SubdirMap = &[ >> + ( >> + "changelog", >> + &Router::new().get(&API_METHOD_APT_GET_CHANGELOG), >> + ), >> + ( >> + "update", >> + &Router::new() >> + .get(&API_METHOD_APT_UPDATE_AVAILABLE) >> + .post(&API_METHOD_APT_UPDATE_DATABASE), >> + ), >> +]; >> + >> +pub const ROUTER: Router = Router::new() >> + .get(&list_subdirs_api_method!(SUBDIRS)) >> + .subdirs(SUBDIRS); >> diff --git a/server/src/api/pve/mod.rs b/server/src/api/pve/mod.rs >> index 2cfdc5b7..0768083d 100644 >> --- a/server/src/api/pve/mod.rs >> +++ b/server/src/api/pve/mod.rs >> @@ -31,6 +31,7 @@ use crate::connection::PveClient; >> use crate::connection::{self, probe_tls_connection}; >> use crate::remote_tasks; >> >> +mod apt; >> mod lxc; >> mod node; >> mod qemu; >> @@ -77,7 +78,7 @@ const RESOURCES_ROUTER: Router = Router::new().get(&API_METHOD_CLUSTER_RESOURCES >> const STATUS_ROUTER: Router = Router::new().get(&API_METHOD_CLUSTER_STATUS); >> >> // converts a remote + PveUpid into a RemoteUpid and starts tracking it >> -async fn new_remote_upid(remote: String, upid: PveUpid) -> Result { >> +pub async fn new_remote_upid(remote: String, upid: PveUpid) -> Result { >> let remote_upid: RemoteUpid = (remote, upid.to_string()).try_into()?; >> remote_tasks::track_running_task(remote_upid.clone()).await?; >> Ok(remote_upid) >> diff --git a/server/src/api/pve/node.rs b/server/src/api/pve/node.rs >> index df96a1c3..99539d1c 100644 >> --- a/server/src/api/pve/node.rs >> +++ b/server/src/api/pve/node.rs >> @@ -13,6 +13,7 @@ pub const ROUTER: Router = Router::new() >> >> #[sortable] >> const SUBDIRS: SubdirMap = &sorted!([ >> + ("apt", &super::apt::ROUTER), >> ("rrddata", &super::rrddata::NODE_RRD_ROUTER), >> ("network", &Router::new().get(&API_METHOD_GET_NETWORK)), >> ("storage", &Router::new().get(&API_METHOD_GET_STORAGES)), >> diff --git a/server/src/lib.rs b/server/src/lib.rs >> index 3f8b7708..a58190d8 100644 >> --- a/server/src/lib.rs >> +++ b/server/src/lib.rs >> @@ -9,6 +9,7 @@ pub mod metric_collection; >> pub mod parallel_fetcher; >> pub mod remote_cache; >> pub mod remote_tasks; >> +pub mod remote_updates; >> pub mod resource_cache; >> pub mod task_utils; >> >> diff --git a/server/src/remote_updates.rs b/server/src/remote_updates.rs >> new file mode 100644 >> index 00000000..809e5de1 >> --- /dev/null >> +++ b/server/src/remote_updates.rs >> @@ -0,0 +1,96 @@ >> +use anyhow::Error; >> +use pdm_api_types::RemoteUpid; >> + >> +use proxmox_apt_api_types::APTUpdateInfo; >> + >> +use pdm_api_types::remotes::{Remote, RemoteType}; >> + >> +use crate::api::pve::new_remote_upid; >> +use crate::connection; >> + >> +/// Return a list of available updates for a given remote node. >> +pub async fn list_available_updates( >> + remote: Remote, >> + node: &str, >> +) -> Result, Error> { >> + let updates = fetch_available_updates(remote, node.to_string()).await?; >> + Ok(updates) >> +} >> + > > Is there a reason for having this as an additional function and not just > calling fetch_available_updates at its callsites (after changing its > visibility)? > > At the very least it could return the result of the await directly? Or > do you want to reserve the option of changing the public API? In that > case we could still introduce a private helper later. > This is basically a left-over from some other patches I was already working on but couldn't get ready in time before my vacation. In the future there will be some periodic task that will fetch available updates from all nodes and put the results in a cache. The fetch_available_udpates was the function used by ParallelFetcher::for_all_remote_nodes to do the fetching. I'd leave it for now if you don't mind. >> +/// Trigger `apt update` on a remote node. >> +/// >> +/// The function returns a `[RemoteUpid]` for the started update task. >> +pub async fn update_apt_database(remote: &Remote, node: &str) -> Result { >> + match remote.ty { >> + RemoteType::Pve => { >> + let client = connection::make_pve_client(remote)?; >> + >> + let params = pve_api_types::AptUpdateParams { >> + notify: Some(false), >> + quiet: Some(false), >> + }; >> + let upid = client.update_apt_database(node, params).await?; >> + >> + new_remote_upid(remote.id.clone(), upid).await >> + } >> + RemoteType::Pbs => todo!(), > > return an Error here? > Will do, as noted in my other mail! >> + } >> +} >> + >> +/// Get the changelog for a given package. >> +pub async fn get_changelog(remote: Remote, node: &str, package: String) -> Result { > > Interestingly the generated binding takes a String even though it > wouldn't need to? Maybe something we should check in the generation > logic later? > >> + match remote.ty { >> + RemoteType::Pve => { >> + let client = connection::make_pve_client(&remote)?; >> + >> + client >> + .get_package_changelog(node, package, None) >> + .await >> + .map_err(Into::into) >> + } >> + RemoteType::Pbs => Ok("TODO: Return PBS package changelog".into()), >> + } >> +} >> + >> +async fn fetch_available_updates( >> + remote: Remote, >> + node: String, > > take a ref here too? > As explained above, this one will be used by ParallelFetcher later, which requires the owned params. Sorry, maybe I should've put that in a comment; I was kind of in a hurry yesterday to get these patches out in time. >> +) -> Result, Error> { >> + match remote.ty { >> + RemoteType::Pve => { >> + let client = connection::make_pve_client(&remote)?; >> + >> + let updates = client >> + .list_available_updates(&node) >> + .await? >> + .into_iter() >> + .map(map_pve_update_info) >> + .collect(); >> + >> + Ok(updates) >> + } >> + RemoteType::Pbs => { >> + let _client = connection::make_pbs_client(&remote)?; >> + >> + // TODO: Fetch available updates >> + // >> + >> + Ok(Vec::new()) >> + } >> + } >> +} >> + >> +fn map_pve_update_info(info: pve_api_types::AptUpdateInfo) -> APTUpdateInfo { >> + APTUpdateInfo { >> + package: info.package, >> + title: info.title, >> + arch: info.arch, >> + description: info.description, >> + version: info.version, >> + old_version: info.old_version.unwrap_or_default(), >> + origin: info.origin, >> + priority: info.priority, >> + section: info.section, >> + extra_info: None, >> + } >> +} _______________________________________________ pdm-devel mailing list pdm-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel