public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Stefan Reiter <s.reiter@proxmox.com>
To: Proxmox Backup Server development discussion
	<pbs-devel@lists.proxmox.com>,
	Mira Limbeck <m.limbeck@proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup] add version API call
Date: Wed, 4 Nov 2020 14:08:27 +0100	[thread overview]
Message-ID: <01668350-c72f-87af-3601-0dca76de1758@proxmox.com> (raw)
In-Reply-To: <20201104114135.15463-1-m.limbeck@proxmox.com>

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 <m.limbeck@proxmox.com>
> ---
>   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<Value, Error> {
> +    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<PackageInfo> = 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<String>,
> +}
> +
>   #[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<Value, Error> {
>       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<Value, Error> {
> +    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<PackageInfo> = 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,
> 




      parent reply	other threads:[~2020-11-04 13:08 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-04 11:41 Mira Limbeck
2020-11-04 11:47 ` Mira Limbeck
2020-11-04 12:51   ` Mira Limbeck
2020-11-04 13:08 ` Stefan Reiter [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=01668350-c72f-87af-3601-0dca76de1758@proxmox.com \
    --to=s.reiter@proxmox.com \
    --cc=m.limbeck@proxmox.com \
    --cc=pbs-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