Re: [pbs-devel] [PATCH v2 proxmox-backup 3/3] node: status: declutter kernel-version

[Lukas Wagner <l.wagner@proxmox.com>]

Hi, some comments inline :)

On 11/24/23 13:02, Gabriel Goller wrote:
> Return a struct with all the components of the kernel version like it
> has been done in pve. Extract and display the build version and kernel
> release nicely.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>   pbs-api-types/src/node.rs | 47 ++++++++++++++++++++++++++++++++++++++-
>   src/api2/node/status.rs   | 18 +++++++--------
>   www/panel/NodeInfo.js     | 13 +++++++++--
>   3 files changed, 66 insertions(+), 12 deletions(-)
> 
> diff --git a/pbs-api-types/src/node.rs b/pbs-api-types/src/node.rs
> index 6d1fa7f0..ad2be5e3 100644
> --- a/pbs-api-types/src/node.rs
> +++ b/pbs-api-types/src/node.rs
> @@ -1,3 +1,5 @@
> +use std::ffi::OsStr;
> +
>   use proxmox_schema::*;
>   use serde::{Deserialize, Serialize};
>   
> @@ -38,6 +40,47 @@ pub struct NodeInformation {
>       pub fingerprint: String,
>   }
>   
> +#[api]
> +#[derive(Serialize, Deserialize, Default)]
> +#[serde(rename_all = "lowercase")]
> +/// The current kernel version (output of `uname`)
> +pub struct KernelVersionInformation {
> +    /// The systemname/nodename
> +    pub sysname: String,
> +    /// The kernel release number
> +    pub release: String,
> +    /// The kernel version
> +    pub version: String,
> +    /// The machine architecture
> +    pub machine: String,
> +}
> +
> +impl KernelVersionInformation {
> +    pub fn from_ostr(sysname: &OsStr, release: &OsStr, version: &OsStr, machine: &OsStr) -> Self {
> +        KernelVersionInformation {
> +            sysname: sysname
> +                .to_os_string()
> +                .into_string()
> +                .unwrap_or("".to_string()),
> +            release: release
> +                .to_os_string()
> +                .into_string()
> +                .unwrap_or("".to_string()),
> +            version: version
> +                .to_os_string()
> +                .into_string()
> +                .unwrap_or("".to_string()),
> +            machine: machine
> +                .to_os_string()
> +                .into_string()
> +                .unwrap_or("".to_string()),
> +        }
> +    }
> +
> +    pub fn get_legacy(&self) -> String {
> +        format!("{} {} {}", self.sysname, self.release, self.version)
> +    }
> +}
>   
>   #[api]
>   #[derive(Serialize, Deserialize, Default)]
> @@ -103,7 +146,9 @@ pub struct NodeStatus {
>       pub uptime: u64,
>       /// Load for 1, 5 and 15 minutes.
>       pub loadavg: [f64; 3],
> -    /// The current kernel version.
> +    /// The current kernel version (NEW struct type).
> +    pub current_kernel: KernelVersionInformation,
> +    /// The current kernel version (LEGACY string type).
>       pub kversion: String,
>       /// Total CPU usage since last query.
>       pub cpu: f64,
> diff --git a/src/api2/node/status.rs b/src/api2/node/status.rs
> index 7cbf9cd6..92273515 100644
> --- a/src/api2/node/status.rs
> +++ b/src/api2/node/status.rs
> @@ -1,4 +1,3 @@
> -use std::os::unix::ffi::OsStrExt;
>   use std::process::Command;
>   
>   use anyhow::{bail, format_err, Error};
> @@ -11,8 +10,8 @@ use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment};
>   use proxmox_schema::api;
>   
>   use pbs_api_types::{
> -    BootModeInformation, NodePowerCommand, StorageStatus, NODE_SCHEMA, PRIV_SYS_AUDIT,
> -    PRIV_SYS_POWER_MANAGEMENT,
> +    BootModeInformation, KernelVersionInformation, NodePowerCommand, StorageStatus, NODE_SCHEMA,
> +    PRIV_SYS_AUDIT, PRIV_SYS_POWER_MANAGEMENT,
>   };
>   
>   use pbs_api_types::{
> @@ -80,11 +79,11 @@ async fn get_status(
>       let cpuinfo = procfs_to_node_cpu_info(cpuinfo);
>   
>       let uname = nix::sys::utsname::uname()?;
> -    let kversion = format!(
> -        "{} {} {}",
> -        std::str::from_utf8(uname.sysname().as_bytes())?,
> -        std::str::from_utf8(uname.release().as_bytes())?,
> -        std::str::from_utf8(uname.version().as_bytes())?
> +    let kernel_version = KernelVersionInformation::from_ostr(
> +        uname.sysname(),
> +        uname.release(),
> +        uname.version(),
> +        uname.machine(),
>       );
>   
>       let disk = crate::tools::fs::fs_info_static(proxmox_lang::c_str!("/")).await?;
> @@ -101,7 +100,8 @@ async fn get_status(
>           },
>           uptime: procfs::read_proc_uptime()?.0 as u64,
>           loadavg,
> -        kversion,
> +        kversion: kernel_version.get_legacy(),
> +        current_kernel: kernel_version,
>           cpuinfo,
>           cpu,
>           wait,
> diff --git a/www/panel/NodeInfo.js b/www/panel/NodeInfo.js
> index 14f84a2e..b1e4ece2 100644
> --- a/www/panel/NodeInfo.js
> +++ b/www/panel/NodeInfo.js
> @@ -140,11 +140,20 @@ Ext.define('PBS.NodeInfoPanel', {
>   	    value: '',
>   	},
>   	{
> -	    itemId: 'kversion',
>   	    colspan: 2,
>   	    title: gettext('Kernel Version'),
>   	    printBar: false,
> -	    textField: 'kversion',
> +	    // TODO: remove with next major and only use newish current-kernel textfield
> +	    multiField: true,
> +	    //textField: 'current-kernel',
I guess this is not needed?

> +	    renderer: ({ data }) => {
> +	        if (!data['current-kernel']) {
> +	            return data.kversion;
> +	        }
> +                let kernel = data['current-kernel'];
> +                let buildDate = kernel.version.match(/\((.+)\)\s*$/)[1] ?? 'unknown';
> +                return `${kernel.sysname} ${kernel.release} (${buildDate})`;

Identation is off here, spaces instead of tabs+spaces

> +	    },
>   	    value: '',
>   	},
>   	{

-- 
- Lukas