From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 2046A65A2E for ; Wed, 4 Nov 2020 14:08:31 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 13695DE7A for ; Wed, 4 Nov 2020 14:08:31 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 7ED73DE6A for ; Wed, 4 Nov 2020 14:08:29 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 3E49D45B45 for ; Wed, 4 Nov 2020 14:08:29 +0100 (CET) To: Proxmox Backup Server development discussion , Mira Limbeck References: <20201104114135.15463-1-m.limbeck@proxmox.com> From: Stefan Reiter Message-ID: <01668350-c72f-87af-3601-0dca76de1758@proxmox.com> Date: Wed, 4 Nov 2020 14:08:27 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <20201104114135.15463-1-m.limbeck@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.037 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [apt.rs, proxmox-backup-manager.rs, mod.rs, node.rs] Subject: Re: [pbs-devel] [PATCH proxmox-backup] add version API call X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 Nov 2020 13:08:31 -0000 In general, you should only call list_installed_apt_packages once, since it has to iterate the entire installed package list every time - which is slow, putting '--verbose' takes noticeable longer than without for no good reason. I would add 'proxmox-backup' and 'proxmox-backup-server' to PACKAGES as well, then run list_apt_packages once, something like: let pbs_packages = tools::apt::list_installed_apt_packages( |fd| ( // only package versions that are actually installed fd.installed_version.is_some() && fd.active_version == fd.installed_version.unwrap() ) && ( fd.package.starts_with("pve-kernel") || PACKAGES.contains(&fd.package) ), None, ); (completely off the top of my head :) ) Then only operate on that list, which should contain everything. Some other stuff inline too. On 11/4/20 12:41 PM, Mira Limbeck wrote: > Adds an API call that returns all relevant packages, same as the version > API call in PVE. > > In addition extens proxmox-backup-manager with the 'version' command > that prints the packages in a format similar to pveversion. > > Signed-off-by: Mira Limbeck > --- > src/api2/node.rs | 148 +++++++++++++++++++++++++++++- > src/api2/types/mod.rs | 13 +++ > src/bin/proxmox-backup-manager.rs | 40 +++++++- > src/tools/apt.rs | 3 + > 4 files changed, 202 insertions(+), 2 deletions(-) > > diff --git a/src/api2/node.rs b/src/api2/node.rs > index a19bea7e..8b0b439a 100644 > --- a/src/api2/node.rs > +++ b/src/api2/node.rs > @@ -19,7 +19,7 @@ use proxmox::tools::websocket::WebSocket; > use proxmox::{identity, sortable}; > > use crate::api2::types::*; > -use crate::config::acl::PRIV_SYS_CONSOLE; > +use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_CONSOLE}; > use crate::server::WorkerTask; > use crate::tools; > use crate::tools::ticket::{self, Empty, Ticket}; > @@ -305,6 +305,151 @@ fn upgrade_to_websocket( > .boxed() > } > > +#[api( > + input: { > + properties: { > + node: { > + schema: NODE_SCHEMA, > + }, > + verbose: { > + description: "Enable verbose output.", > + type: Boolean, > + optional: true, > + default: false, > + }, > + }, > + }, > + returns: { > + description: "List of packages and their installed version.", > + type: Array, > + items: { > + type: PackageInfo, > + }, > + }, > + access: { > + permission: &Permission::Privilege(&[], PRIV_SYS_AUDIT, false), > + }, > +)] > +/// Get package information for important Proxmox packages. > +pub fn get_version(verbose: bool) -> Result { > + const PACKAGES: [&str; 9] = [ > + "ifupdown2", > + "libjs-extjs", > + "proxmox-backup-docs", > + "proxmox-backup-client", > + "proxmox-mini-journalreader", > + "proxmox-widget-toolkit", > + "pve-xtermjs", > + "smartmontools", > + "zfsutils-linux", > + ]; > + > + let running_kernel = nix::sys::utsname::uname().release().to_owned(); > + let mut packages: Vec = Vec::new(); > + let proxmox_backup = > + tools::apt::list_installed_apt_packages(|fd| fd.package == "proxmox-backup", None); > + packages.push( > + proxmox_backup > + .iter() > + .map(|pkg| PackageInfo { > + name: pkg.package.clone(), > + version: pkg.old_version.clone(), > + comment: Some(format!("running kernel: {}", &running_kernel)), > + }) > + .next() > + .unwrap_or(PackageInfo { > + name: "proxmox-backup".to_owned(), > + version: "not correctly installed".to_owned(), > + comment: Some(format!("running kernel: {}", &running_kernel)), > + }), > + ); > + > + if !verbose { > + return Ok(json!(packages)); > + } > + > + let proxmox_backup_server = > + tools::apt::list_installed_apt_packages(|fd| fd.package == "proxmox-backup-server", None); > + packages.push( > + proxmox_backup_server > + .iter() > + .map(|pkg| PackageInfo { > + name: pkg.package.clone(), > + version: pkg.old_version.clone(), > + comment: Some(format!("running version: {}", &pkg.version)), 'pkg.version' refers to the one that would be installed on upgrade. The one running is always the one installed, so old_version is enough. > + }) > + .next() > + .unwrap_or(PackageInfo { > + name: "proxmox-backup-server".to_owned(), > + version: "not correctly installed".to_owned(), > + comment: None, > + }), > + ); > + > + let mut pve_kernel_pkgs = tools::apt::list_installed_apt_packages( > + |fd| { > + if fd.package.starts_with("pve-kernel") { > + return true; > + } > + false > + }, > + None, > + ); > + let cache = apt_pkg_native::Cache::get_singleton(); Just a side note because it bit me before: Cache::get_singleton() locks a mutex, so this only works because Rust is smart enough to drop 'cache' before the next call to list_installed_apt_packages. > + pve_kernel_pkgs.sort_by(|left, right| { > + cache.compare_versions(&left.old_version, &right.old_version).reverse() > + }); > + > + for pkg in pve_kernel_pkgs.iter().map(|pkg| PackageInfo { > + name: pkg.package.clone(), > + version: if pkg.old_version.is_empty() { > + "not correctly installed".to_owned() This would currently only trigger for new kernel packages that are released but not yet installed. It should not sound like an error IMO. > + } else { > + pkg.old_version.clone() > + }, > + comment: None, > + }) { > + packages.push(pkg); > + } > + > + let mut pbs_pkgs = tools::apt::list_installed_apt_packages( > + |fd| { > + if PACKAGES.contains(&fd.package) { > + return true; > + } > + false > + }, > + None, > + ); > + pbs_pkgs.sort_by(|left, right| left.package.cmp(&right.package)); > + > + // add packages we're interested in, but are not installed > + // and the installed ones returned by list_installed_apt_packages > + for pkg in PACKAGES.iter() { > + let apt_pkg = pbs_pkgs.iter().find(|item| &item.package == pkg); > + if apt_pkg.is_some() { > + let apt_pkg = apt_pkg.unwrap(); > + packages.push(PackageInfo { > + name: apt_pkg.package.clone(), > + version: if apt_pkg.old_version.is_empty() { > + "not correctly installed".to_owned() I'd put a different message here, to distinguish from the 'else' case below. Though both this and the kernel case above cannot happen if you use the filter Fn from my proposal, since it only returns installed package versions. > + } else { > + apt_pkg.old_version.clone() > + }, > + comment: None, > + }); > + } else { > + packages.push(PackageInfo { > + name: pkg.to_string(), > + version: "not correctly installed".to_owned(), > + comment: None, > + }); > + } > + } > + > + Ok(json!(packages)) > +} > + > pub const SUBDIRS: SubdirMap = &[ > ("apt", &apt::ROUTER), > ("disks", &disks::ROUTER), > @@ -320,6 +465,7 @@ pub const SUBDIRS: SubdirMap = &[ > ("tasks", &tasks::ROUTER), > ("termproxy", &Router::new().post(&API_METHOD_TERMPROXY)), > ("time", &time::ROUTER), > + ("version", &Router::new().get(&API_METHOD_GET_VERSION)), > ( > "vncwebsocket", > &Router::new().upgrade(&API_METHOD_WEBSOCKET), > diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs > index 7ee89f57..f52b1d37 100644 > --- a/src/api2/types/mod.rs > +++ b/src/api2/types/mod.rs > @@ -1155,6 +1155,19 @@ pub struct APTUpdateInfo { > pub change_log_url: String, > } > > +#[api()] > +#[derive(Serialize, Deserialize)] > +#[serde(rename_all = "PascalCase")] > +/// Pair of package name and version with optional comment. > +pub struct PackageInfo { > + /// Package name > + pub name: String, > + /// Package version > + pub version: String, > + /// Optional comment > + pub comment: Option, > +} > + > #[api()] > #[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)] > #[serde(rename_all = "lowercase")] > diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs > index 7499446b..bdfdfdfa 100644 > --- a/src/bin/proxmox-backup-manager.rs > +++ b/src/bin/proxmox-backup-manager.rs > @@ -363,6 +363,42 @@ async fn report() -> Result { > Ok(Value::Null) > } > > +#[api( > + input: { > + properties: { > + verbose: { > + description: "Enable verbose output.", > + type: Boolean, > + optional: true, > + default: false, > + }, > + }, > + }, > +)] > +/// Get package information for important Proxmox packages. > +async fn get_version(param: Value) -> Result { > + let client = connect()?; > + > + let node = proxmox::tools::nodename(); > + > + let path = format!("api2/json/nodes/{}/version", node); > + > + let mut result = client.get(&path, Some(param)).await?; > + > + let data = result["data"].take(); > + let packages: Vec = serde_json::from_value(data)?; > + > + for pkg in packages { > + if pkg.comment.is_some() { > + println!("{}: {} ({})", pkg.name, pkg.version, pkg.comment.unwrap()); > + } else { > + println!("{}: {}", pkg.name, pkg.version); > + } > + } > + > + Ok(Value::Null) > +} > + > fn main() { > > proxmox_backup::tools::setup_safe_path_env(); > @@ -396,7 +432,9 @@ fn main() { > ) > .insert("report", > CliCommand::new(&API_METHOD_REPORT) > - ); > + ) > + .insert("version", > + CliCommand::new(&API_METHOD_GET_VERSION)); > > > > diff --git a/src/tools/apt.rs b/src/tools/apt.rs > index 5800e0a2..fab8f998 100644 > --- a/src/tools/apt.rs > +++ b/src/tools/apt.rs > @@ -136,6 +136,8 @@ fn get_changelog_url( > } > > pub struct FilterData<'a> { > + // package name > + pub package: &'a str, > // this is version info returned by APT > pub installed_version: Option<&'a str>, > pub candidate_version: &'a str, > @@ -270,6 +272,7 @@ where > let mut long_desc = "".to_owned(); > > let fd = FilterData { > + package: package.as_str(), > installed_version: current_version.as_deref(), > candidate_version: &candidate_version, > active_version: &version, >