[pbs-devel] [PATCH v2 proxmox-backup 1/2] add extra_info field to APTUpdateInfo

[pbs-devel] [PATCH v2 proxmox-backup 1/2] add extra_info field to APTUpdateInfo

[Mira Limbeck]

Add an optional string field to APTUpdateInfo which can be used for
extra information.

This is used for passing running kernel and running version information
in the versions API call together with proxmox-backup and
proxmox-backup-server.

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
v2:
    - renamed additional_info to extra_info to make it compatible with
      GUI code
    - changed the running_kernel and running_version info to contain the
      text 'running kernel: ' and 'running version: ' which removes the
      need for special casing in proxmox-backup-manager versions and
      makes it again compatible with the GUI

 src/api2/node/apt.rs  | 35 ++++++++++++++++++++++++++++-------
 src/api2/types/mod.rs |  2 ++
 src/tools/apt.rs      |  1 +
 3 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/src/api2/node/apt.rs b/src/api2/node/apt.rs
index aa41d6a5..98d02318 100644
--- a/src/api2/node/apt.rs
+++ b/src/api2/node/apt.rs
@@ -276,7 +276,7 @@ pub fn get_versions() -> Result<Value, Error> {
         "zfsutils-linux",
     ];
 
-    fn unknown_package(package: String) -> APTUpdateInfo {
+    fn unknown_package(package: String, extra_info: Option<String>) -> APTUpdateInfo {
         APTUpdateInfo {
             package,
             title: "unknown".into(),
@@ -288,6 +288,7 @@ pub fn get_versions() -> Result<Value, Error> {
             priority: "unknown".into(),
             section: "unknown".into(),
             change_log_url: "unknown".into(),
+            extra_info,
         }
     }
 
@@ -301,14 +302,34 @@ pub fn get_versions() -> Result<Value, Error> {
         },
         None,
     );
-    if let Some(proxmox_backup) = pbs_packages.iter().find(|pkg| pkg.package == "proxmox-backup") {
-        packages.push(proxmox_backup.to_owned());
+
+    let running_kernel = nix::sys::utsname::uname().release().to_owned();
+    if let Some(proxmox_backup) = pbs_packages
+        .iter()
+        .find(|pkg| pkg.package == "proxmox-backup")
+    {
+        let mut proxmox_backup = proxmox_backup.clone();
+        proxmox_backup.extra_info = Some(format!("running kernel: {}", running_kernel));
+        packages.push(proxmox_backup);
     } else {
-        packages.push(unknown_package("proxmox-backup".into()));
+        packages.push(unknown_package(
+            "proxmox-backup".into(),
+            Some(running_kernel),
+        ));
     }
 
-    if let Some(pkg) = pbs_packages.iter().find(|pkg| pkg.package == "proxmox-backup-server") {
-        packages.push(pkg.to_owned());
+    if let Some(pkg) = pbs_packages
+        .iter()
+        .find(|pkg| pkg.package == "proxmox-backup-server")
+    {
+        let running_version = format!(
+            "running version: {}.{}",
+            crate::api2::version::PROXMOX_PKG_VERSION,
+            crate::api2::version::PROXMOX_PKG_RELEASE
+        );
+        let mut pkg = pkg.clone();
+        pkg.extra_info = Some(running_version);
+        packages.push(pkg);
     }
 
     let mut kernel_pkgs: Vec<APTUpdateInfo> = pbs_packages
@@ -334,7 +355,7 @@ pub fn get_versions() -> Result<Value, Error> {
         }
         match pbs_packages.iter().find(|item| &item.package == pkg) {
             Some(apt_pkg) => packages.push(apt_pkg.to_owned()),
-            None => packages.push(unknown_package(pkg.to_string())),
+            None => packages.push(unknown_package(pkg.to_string(), None)),
         }
     }
 
diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
index ba9d2da9..f240fb64 100644
--- a/src/api2/types/mod.rs
+++ b/src/api2/types/mod.rs
@@ -1177,6 +1177,8 @@ pub struct APTUpdateInfo {
     pub section: String,
     /// URL under which the package's changelog can be retrieved
     pub change_log_url: String,
+    /// Additional package information
+    pub extra_info: Option<String>,
 }
 
 #[api()]
diff --git a/src/tools/apt.rs b/src/tools/apt.rs
index 841b7447..af8f51f1 100644
--- a/src/tools/apt.rs
+++ b/src/tools/apt.rs
@@ -361,6 +361,7 @@ where
                 },
                 priority: priority_res,
                 section: section_res,
+                extra_info: None,
             });
         }
     }
-- 
2.20.1

[pbs-devel] [PATCH v2 proxmox-backup 2/2] add versions command to proxmox-backup-manager

[Mira Limbeck]

Add the versions command to proxmox-backup-manager with a similar output
to pveversion [-v]. It prints the packages line by line with only the
package name, followed by the version and, for proxmox-backup and
proxmox-backup-server, some additional information (running kernel,
running version).

In addition it supports the optional output-format parameter which can
be used to print the complete data in either json, json-pretty or text
format. If output-format is specified, the --verbose parameter is
ignored and the detailed list of packages is printed.

With the addition of the versions command, the report is extended as
well.

Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
---
v2:
    - removed the special casing for running kernel and running version
      as the get_versions result already contains the complete string
    - fixed version string to print 'not correctly installed' instead of
      'unknown' if no old_version is available

 src/bin/proxmox-backup-manager.rs | 78 +++++++++++++++++++++++++++++++
 src/server/report.rs              |  1 +
 2 files changed, 79 insertions(+)

diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
index e52c2f76..60af205f 100644
--- a/src/bin/proxmox-backup-manager.rs
+++ b/src/bin/proxmox-backup-manager.rs
@@ -363,6 +363,81 @@ async fn report() -> Result<Value, Error> {
     Ok(Value::Null)
 }
 
+#[api(
+    input: {
+        properties: {
+            verbose: {
+                type: Boolean,
+                optional: true,
+                default: false,
+                description: "Output verbose package information. It is ignored if output-format is specified.",
+            },
+            "output-format": {
+                schema: OUTPUT_FORMAT,
+                optional: true,
+            }
+        }
+    }
+)]
+/// List package versions for important Proxmox Backup Server packages.
+async fn get_versions(verbose: bool, param: Value) -> Result<Value, Error> {
+    let output_format = param.get("output-format");
+
+    if !verbose && output_format.is_none() {
+        let pkg_version = format!(
+            "{}.{}",
+            crate::api2::version::PROXMOX_PKG_VERSION,
+            crate::api2::version::PROXMOX_PKG_RELEASE
+        );
+        let running_kernel = nix::sys::utsname::uname().release().to_owned();
+
+        println!(
+            "proxmox-backup-server/{} (running kernel: {})",
+            pkg_version, running_kernel
+        );
+        return Ok(Value::Null);
+    }
+
+    let mut packages = crate::api2::node::apt::get_versions()?;
+    if let Some(output_format) = output_format {
+        if let Some(output_format) = output_format.as_str() {
+            let options = TableFormatOptions::default();
+            format_and_print_result_full(
+                &mut packages,
+                &crate::api2::node::apt::API_RETURN_SCHEMA_GET_VERSIONS,
+                output_format,
+                &options,
+            );
+        }
+    } else {
+        // pveversion style print
+        let packages: Vec<APTUpdateInfo> = serde_json::from_value(packages)?;
+        for pkg in packages {
+            let mut version = "not correctly installed";
+            if !pkg.old_version.is_empty() && &pkg.old_version != "unknown" {
+                version = &pkg.old_version;
+            }
+
+            if let Some(extra_info) = pkg.extra_info {
+                println!(
+                    "{}: {} ({})",
+                    pkg.package,
+                    version,
+                    extra_info
+                );
+            } else {
+                println!(
+                    "{}: {}",
+                    pkg.package,
+                    version,
+                );
+            }
+        }
+    }
+
+    Ok(Value::Null)
+}
+
 fn main() {
 
     proxmox_backup::tools::setup_safe_path_env();
@@ -396,6 +471,9 @@ fn main() {
         )
         .insert("report",
             CliCommand::new(&API_METHOD_REPORT)
+        )
+        .insert("versions",
+            CliCommand::new(&API_METHOD_GET_VERSIONS)
         );
 
 
diff --git a/src/server/report.rs b/src/server/report.rs
index 9c6e2406..22e16a14 100644
--- a/src/server/report.rs
+++ b/src/server/report.rs
@@ -20,6 +20,7 @@ fn files() -> Vec<&'static str> {
 fn commands() -> Vec<(&'static str, Vec<&'static str>)> {
     vec![
     //  ("<command>", vec![<arg [, arg]>])
+        ("proxmox-backup-manager", vec!["versions", "--verbose"]),
         ("df", vec!["-h"]),
         ("lsblk", vec!["--ascii"]),
         ("zpool", vec!["status"]),
-- 
2.20.1

[pbs-devel] applied: [PATCH v2 proxmox-backup 2/2] add versions command to proxmox-backup-manager

[Thomas Lamprecht]

On 10.11.20 14:22, Mira Limbeck wrote:
> Add the versions command to proxmox-backup-manager with a similar output
> to pveversion [-v]. It prints the packages line by line with only the
> package name, followed by the version and, for proxmox-backup and
> proxmox-backup-server, some additional information (running kernel,
> running version).
> 
> In addition it supports the optional output-format parameter which can
> be used to print the complete data in either json, json-pretty or text
> format. If output-format is specified, the --verbose parameter is
> ignored and the detailed list of packages is printed.
> 
> With the addition of the versions command, the report is extended as
> well.
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> v2:
>     - removed the special casing for running kernel and running version
>       as the get_versions result already contains the complete string
>     - fixed version string to print 'not correctly installed' instead of
>       'unknown' if no old_version is available
> 
>  src/bin/proxmox-backup-manager.rs | 78 +++++++++++++++++++++++++++++++
>  src/server/report.rs              |  1 +
>  2 files changed, 79 insertions(+)
> 
>

applied, thanks! I did a followup reducing this to a more unified code, chipping
of ~ 30 lines, single thing I'm unhappy with is the de-structuring and rebuilding
for slicing out the first package info in the non-verbose case, so if one has
better ideas/nicer code there...

What I found a bit weird that the verbose flag was effectively also triggered
indirectly, by setting an output-format.

One thing I noticed, the CLI argument parser does not recognizes the short option
here (-v) - just noting it.

[pbs-devel] [PATCH proxmox-backup] apt: use typed response for get_versions

[Stefan Reiter]

...and cleanup get_versions for manager CLI.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

No need to return an untyped Value, especially if we want to further work with
it, so how about this for a cleanup?

 src/api2/node/apt.rs              |  4 ++--
 src/bin/proxmox-backup-manager.rs | 10 ++--------
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/src/api2/node/apt.rs b/src/api2/node/apt.rs
index 500b2309..765ea525 100644
--- a/src/api2/node/apt.rs
+++ b/src/api2/node/apt.rs
@@ -261,7 +261,7 @@ fn apt_get_changelog(
     },
 )]
 /// Get package information for important Proxmox Backup Server packages.
-pub fn get_versions() -> Result<Value, Error> {
+pub fn get_versions() -> Result<Vec<APTUpdateInfo>, Error> {
     const PACKAGES: &[&str] = &[
         "ifupdown2",
         "libjs-extjs",
@@ -350,7 +350,7 @@ pub fn get_versions() -> Result<Value, Error> {
         }
     }
 
-    Ok(json!(packages))
+    Ok(packages)
 }
 
 const SUBDIRS: SubdirMap = &[
diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
index 84ec0148..80e699f1 100644
--- a/src/bin/proxmox-backup-manager.rs
+++ b/src/bin/proxmox-backup-manager.rs
@@ -383,14 +383,8 @@ async fn report() -> Result<Value, Error> {
 async fn get_versions(verbose: bool, param: Value) -> Result<Value, Error> {
     let output_format = get_output_format(&param);
 
-    let mut packages = if verbose {
-        crate::api2::node::apt::get_versions()?
-    } else {
-        // TODO: slice first element out in a nicer way?
-        let packages = crate::api2::node::apt::get_versions()?;
-        let packages = packages.as_array().unwrap();
-        Value::Array(vec![packages[0].to_owned()])
-    };
+    let packages = crate::api2::node::apt::get_versions()?;
+    let mut packages = json!(if verbose { &packages[..] } else { &packages[0..1] });
 
     let options = default_table_format_options()
         .disable_sort()
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup] apt: use typed response for get_versions

[Thomas Lamprecht]

On 12.11.20 10:12, Stefan Reiter wrote:
> ...and cleanup get_versions for manager CLI.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> No need to return an untyped Value, especially if we want to further work with
> it, so how about this for a cleanup?
> 
>  src/api2/node/apt.rs              |  4 ++--
>  src/bin/proxmox-backup-manager.rs | 10 ++--------
>  2 files changed, 4 insertions(+), 10 deletions(-)
> 
>

much nicer, and the obvious solution; I'd guess it was a bit to late for me yesterday ^^ 

applied, thanks!

Re: [pbs-devel] applied: [PATCH v2 proxmox-backup 2/2] add versions command to proxmox-backup-manager

[Thomas Lamprecht]

FYI: Including the mailing list again in CC.

On 12.11.20 10:11, Mira Limbeck wrote:
> So we print proxmox-backup with the kernel version instead of proxmox-backup-server in the non-verbose case?

no, the slice should be selecting the second package, fixed.

> 
> This is at least different from pveversion as that prints pve-manager and not proxmox-ve.
> 
> And with this change the output-format also changes the amount of information (json and json-pretty -> full output, text (default) -> only a subset)

yes, that's intended and what an user interface, be it CLI or web, is for.
Just dumping the whole state does not makes sense here for the user, same
as the web interface shouldn't just dump the raw state it gets from an
API response, different presentation require selecting different data shown
to point at the thing one actually want to present to the user.

If we just could always dump the whole state we could just auto generate
the whole user interfaces, but those tend to be pretty confusing and unusable
for most.

Re: [pbs-devel] [PATCH v2 proxmox-backup 1/2] add extra_info field to APTUpdateInfo

[Mira Limbeck]

the 'running kernel: ' string should be used in both the found and 
notfound cases. will fix this in the next version.

On 11/10/20 2:22 PM, Mira Limbeck wrote:
> Add an optional string field to APTUpdateInfo which can be used for
> extra information.
>
> This is used for passing running kernel and running version information
> in the versions API call together with proxmox-backup and
> proxmox-backup-server.
>
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> v2:
>      - renamed additional_info to extra_info to make it compatible with
>        GUI code
>      - changed the running_kernel and running_version info to contain the
>        text 'running kernel: ' and 'running version: ' which removes the
>        need for special casing in proxmox-backup-manager versions and
>        makes it again compatible with the GUI
>
>   src/api2/node/apt.rs  | 35 ++++++++++++++++++++++++++++-------
>   src/api2/types/mod.rs |  2 ++
>   src/tools/apt.rs      |  1 +
>   3 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/src/api2/node/apt.rs b/src/api2/node/apt.rs
> index aa41d6a5..98d02318 100644
> --- a/src/api2/node/apt.rs
> +++ b/src/api2/node/apt.rs
> @@ -276,7 +276,7 @@ pub fn get_versions() -> Result<Value, Error> {
>           "zfsutils-linux",
>       ];
>   
> -    fn unknown_package(package: String) -> APTUpdateInfo {
> +    fn unknown_package(package: String, extra_info: Option<String>) -> APTUpdateInfo {
>           APTUpdateInfo {
>               package,
>               title: "unknown".into(),
> @@ -288,6 +288,7 @@ pub fn get_versions() -> Result<Value, Error> {
>               priority: "unknown".into(),
>               section: "unknown".into(),
>               change_log_url: "unknown".into(),
> +            extra_info,
>           }
>       }
>   
> @@ -301,14 +302,34 @@ pub fn get_versions() -> Result<Value, Error> {
>           },
>           None,
>       );
> -    if let Some(proxmox_backup) = pbs_packages.iter().find(|pkg| pkg.package == "proxmox-backup") {
> -        packages.push(proxmox_backup.to_owned());
> +
> +    let running_kernel = nix::sys::utsname::uname().release().to_owned();
> +    if let Some(proxmox_backup) = pbs_packages
> +        .iter()
> +        .find(|pkg| pkg.package == "proxmox-backup")
> +    {
> +        let mut proxmox_backup = proxmox_backup.clone();
> +        proxmox_backup.extra_info = Some(format!("running kernel: {}", running_kernel));
> +        packages.push(proxmox_backup);
>       } else {
> -        packages.push(unknown_package("proxmox-backup".into()));
> +        packages.push(unknown_package(
> +            "proxmox-backup".into(),
> +            Some(running_kernel),
^ should also contain 'running kernel: <kernelversion>' instead of just 
'<kernelversion>'
> +        ));
>       }
>   
> -    if let Some(pkg) = pbs_packages.iter().find(|pkg| pkg.package == "proxmox-backup-server") {
> -        packages.push(pkg.to_owned());
> +    if let Some(pkg) = pbs_packages
> +        .iter()
> +        .find(|pkg| pkg.package == "proxmox-backup-server")
> +    {
> +        let running_version = format!(
> +            "running version: {}.{}",
> +            crate::api2::version::PROXMOX_PKG_VERSION,
> +            crate::api2::version::PROXMOX_PKG_RELEASE
> +        );
> +        let mut pkg = pkg.clone();
> +        pkg.extra_info = Some(running_version);
> +        packages.push(pkg);
>       }
>   
>       let mut kernel_pkgs: Vec<APTUpdateInfo> = pbs_packages
> @@ -334,7 +355,7 @@ pub fn get_versions() -> Result<Value, Error> {
>           }
>           match pbs_packages.iter().find(|item| &item.package == pkg) {
>               Some(apt_pkg) => packages.push(apt_pkg.to_owned()),
> -            None => packages.push(unknown_package(pkg.to_string())),
> +            None => packages.push(unknown_package(pkg.to_string(), None)),
>           }
>       }
>   
> diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
> index ba9d2da9..f240fb64 100644
> --- a/src/api2/types/mod.rs
> +++ b/src/api2/types/mod.rs
> @@ -1177,6 +1177,8 @@ pub struct APTUpdateInfo {
>       pub section: String,
>       /// URL under which the package's changelog can be retrieved
>       pub change_log_url: String,
> +    /// Additional package information
> +    pub extra_info: Option<String>,
>   }
>   
>   #[api()]
> diff --git a/src/tools/apt.rs b/src/tools/apt.rs
> index 841b7447..af8f51f1 100644
> --- a/src/tools/apt.rs
> +++ b/src/tools/apt.rs
> @@ -361,6 +361,7 @@ where
>                   },
>                   priority: priority_res,
>                   section: section_res,
> +                extra_info: None,
>               });
>           }
>       }

[pbs-devel] applied: [PATCH v2 proxmox-backup 1/2] add extra_info field to APTUpdateInfo

[Thomas Lamprecht]

On 10.11.20 14:22, Mira Limbeck wrote:
> Add an optional string field to APTUpdateInfo which can be used for
> extra information.
> 
> This is used for passing running kernel and running version information
> in the versions API call together with proxmox-backup and
> proxmox-backup-server.
> 
> Signed-off-by: Mira Limbeck <m.limbeck@proxmox.com>
> ---
> v2:
>     - renamed additional_info to extra_info to make it compatible with
>       GUI code
>     - changed the running_kernel and running_version info to contain the
>       text 'running kernel: ' and 'running version: ' which removes the
>       need for special casing in proxmox-backup-manager versions and
>       makes it again compatible with the GUI
> 
>  src/api2/node/apt.rs  | 35 ++++++++++++++++++++++++++++-------
>  src/api2/types/mod.rs |  2 ++
>  src/tools/apt.rs      |  1 +
>  3 files changed, 31 insertions(+), 7 deletions(-)
> 
>

applied, thanks! I did some followups restoring the format to the previous state,
line lengths up to 99 are fine for us, and reducing some lines can help read the
code faster as more is visible and thus in the mental state, at least it feels
less bloated to me :-)

Can you please address your comment on this patch as followup?