[pbs-devel] [PATCH v2 proxmox{-backup, } 0/3] Add boot_mode, improve kernel version

[pbs-devel] [PATCH v2 proxmox{-backup, } 0/3] Add boot_mode, improve kernel version

[Gabriel Goller]

Ported the recent changes from the PVE NodeSummary (done by @Thomas) to
the PBS NodeDashboard.

It consists of:
* Adding the bootmode field, shows either Legacy BIOS, EFI, or EFI
    (Secure Boot)
* Declutter the kernel-version field and only show the release version
    and build-date.

Changes since v1:
* Moved boot_mode detection to proxmox-sys
* Added caching to boot_mode detection (lazy_static)
* Return legacy kernel-version as well


proxmox-backup:

Gabriel Goller (2):
  node: status: added bootmode
  node: status: declutter kernel-version

 pbs-api-types/src/node.rs | 67 +++++++++++++++++++++++++++++++++++++--
 src/api2/node/status.rs   | 29 ++++++++++++-----
 www/panel/NodeInfo.js     | 30 ++++++++++++++++--
 3 files changed, 113 insertions(+), 13 deletions(-)


proxmox:

Gabriel Goller (1):
  sys: add function to get boot_mode

 proxmox-sys/src/boot_mode.rs | 47 ++++++++++++++++++++++++++++++++++++
 proxmox-sys/src/lib.rs       |  1 +
 2 files changed, 48 insertions(+)
 create mode 100644 proxmox-sys/src/boot_mode.rs


Summary over all repositories:
  5 files changed, 161 insertions(+), 13 deletions(-)

-- 
murpp v0.4.0

[pbs-devel] [PATCH v2 proxmox 1/3] sys: add function to get boot_mode

[Gabriel Goller]

Helper that returns the current boot_mode. Either EFI, BIOS, or EFI
(Secure Boot).
Detection works the same as in pve, we use `/sys/firmware/efi` and
the `efivars/SecureBoot-xxx..` file.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-sys/src/boot_mode.rs | 47 ++++++++++++++++++++++++++++++++++++
 proxmox-sys/src/lib.rs       |  1 +
 2 files changed, 48 insertions(+)
 create mode 100644 proxmox-sys/src/boot_mode.rs

diff --git a/proxmox-sys/src/boot_mode.rs b/proxmox-sys/src/boot_mode.rs
new file mode 100644
index 0000000..570af8e
--- /dev/null
+++ b/proxmox-sys/src/boot_mode.rs
@@ -0,0 +1,47 @@
+use std::{io::Read, sync::Mutex};
+
+/// The possible BootModes
+#[derive(Clone, Copy)]
+pub enum BootModeInformation {
+    /// The BootMode is EFI/UEFI
+    Efi,
+    /// The BootMode is EFI/UEFI with Secure Boot enabled
+    EfiSecureBoot,
+    /// The BootMode is Legacy BIOS
+    Bios,
+}
+
+// Returns the current bootmode (BIOS, EFI, or EFI(Secure Boot))
+pub fn boot_mode() -> BootModeInformation {
+    lazy_static::lazy_static!(
+        static ref BOOT_MODE: Mutex<Option<BootModeInformation>> = Mutex::new(None);
+    );
+
+    let mut last = BOOT_MODE.lock().unwrap();
+    let value = last.or_else(|| {
+        if std::path::Path::new("/sys/firmware/efi").exists() {
+            // Check if SecureBoot is enabled
+            // Attention: this file is not seekable!
+            let efivar = std::fs::File::open(
+                "/sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c",
+            );
+            if let Ok(mut file) = efivar {
+                let mut buf = [0; 5];
+                let Ok(_) = file.read_exact(&mut buf) else {
+                    return Some(BootModeInformation::Efi);
+                };
+                if buf[4..] == [1] {
+                    Some(BootModeInformation::EfiSecureBoot)
+                } else {
+                    Some(BootModeInformation::Efi)
+                }
+            } else {
+                Some(BootModeInformation::Efi)
+            }
+        } else {
+            Some(BootModeInformation::Bios)
+        }
+    });
+    *last = value;
+    value.unwrap()
+}
diff --git a/proxmox-sys/src/lib.rs b/proxmox-sys/src/lib.rs
index 7e59058..8ea7073 100644
--- a/proxmox-sys/src/lib.rs
+++ b/proxmox-sys/src/lib.rs
@@ -1,5 +1,6 @@
 use std::os::unix::ffi::OsStrExt;
 
+pub mod boot_mode;
 pub mod command;
 #[cfg(feature = "crypt")]
 pub mod crypt;
-- 
2.39.2

Re: [pbs-devel] [PATCH v2 proxmox 1/3] sys: add function to get boot_mode

[Lukas Wagner]

On 11/24/23 13:02, Gabriel Goller wrote:
> Helper that returns the current boot_mode. Either EFI, BIOS, or EFI
> (Secure Boot).
> Detection works the same as in pve, we use `/sys/firmware/efi` and
> the `efivars/SecureBoot-xxx..` file.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>   proxmox-sys/src/boot_mode.rs | 47 ++++++++++++++++++++++++++++++++++++
>   proxmox-sys/src/lib.rs       |  1 +
>   2 files changed, 48 insertions(+)
>   create mode 100644 proxmox-sys/src/boot_mode.rs
> 
> diff --git a/proxmox-sys/src/boot_mode.rs b/proxmox-sys/src/boot_mode.rs
> new file mode 100644
> index 0000000..570af8e
> --- /dev/null
> +++ b/proxmox-sys/src/boot_mode.rs
> @@ -0,0 +1,47 @@
> +use std::{io::Read, sync::Mutex};
> +
> +/// The possible BootModes
> +#[derive(Clone, Copy)]
> +pub enum BootModeInformation {
> +    /// The BootMode is EFI/UEFI
> +    Efi,
> +    /// The BootMode is EFI/UEFI with Secure Boot enabled
> +    EfiSecureBoot,
> +    /// The BootMode is Legacy BIOS
> +    Bios,
> +}
> +
> +// Returns the current bootmode (BIOS, EFI, or EFI(Secure Boot))
> +pub fn boot_mode() -> BootModeInformation {
> +    lazy_static::lazy_static!(
> +        static ref BOOT_MODE: Mutex<Option<BootModeInformation>> = Mutex::new(None);
> +    );
> +
> +    let mut last = BOOT_MODE.lock().unwrap();
> +    let value = last.or_else(|| {
> +        if std::path::Path::new("/sys/firmware/efi").exists() {
> +            // Check if SecureBoot is enabled
> +            // Attention: this file is not seekable!
> +            let efivar = std::fs::File::open(
> +                "/sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c",
> +            );

Maybe add some comment what this magic EFI variable is, e.g. with a link 
to a spec?

-- 
- Lukas

[pbs-devel] [PATCH v2 proxmox-backup 2/3] node: status: added bootmode

[Gabriel Goller]

Added field that shows the bootmode of the node. The bootmode is either
Legacy Bios, EFI, or EFI (Secure Boot). To detect the mode we use the
exact same method as in pve: We check if the `/sys/firmware/efi` folder
exists, then check if the `SecureBoot-xx...` file in the `efivars`
directory has the SecureBoot flag enabled.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-api-types/src/node.rs | 20 ++++++++++++++++++--
 src/api2/node/status.rs   | 17 +++++++++++++++--
 www/panel/NodeInfo.js     | 17 +++++++++++++++++
 3 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/pbs-api-types/src/node.rs b/pbs-api-types/src/node.rs
index 704215bb..6d1fa7f0 100644
--- a/pbs-api-types/src/node.rs
+++ b/pbs-api-types/src/node.rs
@@ -1,9 +1,8 @@
-use serde::{Deserialize, Serialize};
 use proxmox_schema::*;
+use serde::{Deserialize, Serialize};
 
 use crate::StorageStatus;
 
-
 #[api]
 #[derive(Serialize, Deserialize, Default)]
 #[serde(rename_all = "kebab-case")]
@@ -39,6 +38,21 @@ pub struct NodeInformation {
     pub fingerprint: String,
 }
 
+
+#[api]
+#[derive(Serialize, Deserialize, Default)]
+#[serde(rename_all = "lowercase")]
+/// The possible BootModes
+pub enum BootModeInformation {
+    /// The BootMode is EFI/UEFI
+    Efi,
+    /// The BootMode is EFI/UEFI with Secure Boot enabled
+    EfiSecureBoot,
+    /// The BootMode is Legacy BIOS
+    #[default]
+    Bios,
+}
+
 #[api]
 #[derive(Serialize, Deserialize, Default)]
 #[serde(rename_all = "kebab-case")]
@@ -97,4 +111,6 @@ pub struct NodeStatus {
     pub wait: f64,
     pub cpuinfo: NodeCpuInformation,
     pub info: NodeInformation,
+    /// Current boot mode
+    pub boot_info: BootModeInformation,
 }
diff --git a/src/api2/node/status.rs b/src/api2/node/status.rs
index 639d7211..7cbf9cd6 100644
--- a/src/api2/node/status.rs
+++ b/src/api2/node/status.rs
@@ -1,16 +1,18 @@
-use std::os::unix::prelude::OsStrExt;
+use std::os::unix::ffi::OsStrExt;
 use std::process::Command;
 
 use anyhow::{bail, format_err, Error};
 use serde_json::Value;
 
+use proxmox_sys::boot_mode;
 use proxmox_sys::linux::procfs;
 
 use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment};
 use proxmox_schema::api;
 
 use pbs_api_types::{
-    NodePowerCommand, StorageStatus, NODE_SCHEMA, PRIV_SYS_AUDIT, PRIV_SYS_POWER_MANAGEMENT,
+    BootModeInformation, NodePowerCommand, StorageStatus, NODE_SCHEMA, PRIV_SYS_AUDIT,
+    PRIV_SYS_POWER_MANAGEMENT,
 };
 
 use pbs_api_types::{
@@ -25,6 +27,14 @@ fn procfs_to_node_cpu_info(info: procfs::ProcFsCPUInfo) -> NodeCpuInformation {
     }
 }
 
+fn boot_mode_to_info(bm: boot_mode::BootModeInformation) -> BootModeInformation {
+    match bm {
+        boot_mode::BootModeInformation::Efi => BootModeInformation::Efi,
+        boot_mode::BootModeInformation::EfiSecureBoot => BootModeInformation::EfiSecureBoot,
+        boot_mode::BootModeInformation::Bios => BootModeInformation::Bios,
+    }
+}
+
 #[api(
     input: {
         properties: {
@@ -79,6 +89,8 @@ async fn get_status(
 
     let disk = crate::tools::fs::fs_info_static(proxmox_lang::c_str!("/")).await?;
 
+    let boot_info = boot_mode_to_info(boot_mode::boot_mode());
+
     Ok(NodeStatus {
         memory,
         swap,
@@ -96,6 +108,7 @@ async fn get_status(
         info: NodeInformation {
             fingerprint: crate::cert_info()?.fingerprint()?,
         },
+        boot_info,
     })
 }
 
diff --git a/www/panel/NodeInfo.js b/www/panel/NodeInfo.js
index 2551c9a5..14f84a2e 100644
--- a/www/panel/NodeInfo.js
+++ b/www/panel/NodeInfo.js
@@ -147,6 +147,23 @@ Ext.define('PBS.NodeInfoPanel', {
 	    textField: 'kversion',
 	    value: '',
 	},
+	{
+	    colspan: 2,
+	    title: gettext('Boot Mode'),
+	    printBar: false,
+	    textField: 'boot-info',
+	    renderer: boot_mode => {
+	        if (boot_mode === 'bios') {
+	            return 'Legacy BIOS';
+	        } else if (boot_mode === 'efi') {
+	            return 'EFI';
+	        } else if (boot_mode === 'efisecureboot') {
+	            return 'EFI (Secure Boot)';
+	        }
+	        return Proxmox.Utils.unknownText;
+	    },
+	    value: '',
+	},
 	{
 	    xtype: 'pmxNodeInfoRepoStatus',
 	    itemId: 'repositoryStatus',
-- 
2.39.2

Re: [pbs-devel] [PATCH v2 proxmox-backup 2/3] node: status: added bootmode

[Lukas Wagner]

On 11/24/23 13:02, Gabriel Goller wrote:
>   
> +fn boot_mode_to_info(bm: boot_mode::BootModeInformation) -> BootModeInformation {
> +    match bm {
> +        boot_mode::BootModeInformation::Efi => BootModeInformation::Efi,
> +        boot_mode::BootModeInformation::EfiSecureBoot => BootModeInformation::EfiSecureBoot,
> +        boot_mode::BootModeInformation::Bios => BootModeInformation::Bios,
> +    }
> +}

Mini nit: Maybe a From<boot_mode::BootModeInformation> impl would be a 
bit more idiomatic?

> +
>   #[api(
>       input: {
>           properties: {
> @@ -79,6 +89,8 @@ async fn get_status(
>   
>       let disk = crate::tools::fs::fs_info_static(proxmox_lang::c_str!("/")).await?;
>   
> +    let boot_info = boot_mode_to_info(boot_mode::boot_mode());
> +
>       Ok(NodeStatus {
>           memory,
>           swap,
> @@ -96,6 +108,7 @@ async fn get_status(
>           info: NodeInformation {
>               fingerprint: crate::cert_info()?.fingerprint()?,
>           },
> +        boot_info,
>       })
>   }
>   
> diff --git a/www/panel/NodeInfo.js b/www/panel/NodeInfo.js
> index 2551c9a5..14f84a2e 100644
> --- a/www/panel/NodeInfo.js
> +++ b/www/panel/NodeInfo.js
> @@ -147,6 +147,23 @@ Ext.define('PBS.NodeInfoPanel', {
>   	    textField: 'kversion',
>   	    value: '',
>   	},
> +	{
> +	    colspan: 2,
> +	    title: gettext('Boot Mode'),
> +	    printBar: false,
> +	    textField: 'boot-info',
> +	    renderer: boot_mode => {
> +	        if (boot_mode === 'bios') {
> +	            return 'Legacy BIOS';
> +	        } else if (boot_mode === 'efi') {
> +	            return 'EFI';
> +	        } else if (boot_mode === 'efisecureboot') {
> +	            return 'EFI (Secure Boot)';
> +	        }
> +	        return Proxmox.Utils.unknownText;
> +	    },
> +	    value: '',
> +	},
>   	{
>   	    xtype: 'pmxNodeInfoRepoStatus',
>   	    itemId: 'repositoryStatus',

-- 
- Lukas

Re: [pbs-devel] [PATCH v2 proxmox-backup 2/3] node: status: added bootmode

[Gabriel Goller]

On 11/24/23 15:20, Lukas Wagner wrote:

>
>
> On 11/24/23 13:02, Gabriel Goller wrote:
>>   +fn boot_mode_to_info(bm: boot_mode::BootModeInformation) -> 
>> BootModeInformation {
>> +    match bm {
>> +        boot_mode::BootModeInformation::Efi => 
>> BootModeInformation::Efi,
>> +        boot_mode::BootModeInformation::EfiSecureBoot => 
>> BootModeInformation::EfiSecureBoot,
>> +        boot_mode::BootModeInformation::Bios => 
>> BootModeInformation::Bios,
>> +    }
>> +}
>
> Mini nit: Maybe a From<boot_mode::BootModeInformation> impl would be a 
> bit more idiomatic?
Thought about this as well, but sadly the orphan rule ruins the day :(
I also don't want to import `proxmox_sys` in `pbs-api-types` or viceversa.
>
>> +
>>   #[api(
>>       input: {
>>           properties: {
>> @@ -79,6 +89,8 @@ async fn get_status(
>>         let disk = 
>> crate::tools::fs::fs_info_static(proxmox_lang::c_str!("/")).await?;
>>   +    let boot_info = boot_mode_to_info(boot_mode::boot_mode());
>> +
>>       Ok(NodeStatus {
>>           memory,
>>           swap,
>> @@ -96,6 +108,7 @@ async fn get_status(
>>           info: NodeInformation {
>>               fingerprint: crate::cert_info()?.fingerprint()?,
>>           },
>> +        boot_info,
>>       })
>>   }
>>   diff --git a/www/panel/NodeInfo.js b/www/panel/NodeInfo.js
>> index 2551c9a5..14f84a2e 100644
>> --- a/www/panel/NodeInfo.js
>> +++ b/www/panel/NodeInfo.js
>> @@ -147,6 +147,23 @@ Ext.define('PBS.NodeInfoPanel', {
>>           textField: 'kversion',
>>           value: '',
>>       },
>> +    {
>> +        colspan: 2,
>> +        title: gettext('Boot Mode'),
>> +        printBar: false,
>> +        textField: 'boot-info',
>> +        renderer: boot_mode => {
>> +            if (boot_mode === 'bios') {
>> +                return 'Legacy BIOS';
>> +            } else if (boot_mode === 'efi') {
>> +                return 'EFI';
>> +            } else if (boot_mode === 'efisecureboot') {
>> +                return 'EFI (Secure Boot)';
>> +            }
>> +            return Proxmox.Utils.unknownText;
>> +        },
>> +        value: '',
>> +    },
>>       {
>>           xtype: 'pmxNodeInfoRepoStatus',
>>           itemId: 'repositoryStatus',
>

Re: [pbs-devel] [PATCH v2 proxmox-backup 2/3] node: status: added bootmode

[Lukas Wagner]

On 11/27/23 09:26, Gabriel Goller wrote:
> On 11/24/23 15:20, Lukas Wagner wrote:
> 
>>
>>
>> On 11/24/23 13:02, Gabriel Goller wrote:
>>>   +fn boot_mode_to_info(bm: boot_mode::BootModeInformation) -> 
>>> BootModeInformation {
>>> +    match bm {
>>> +        boot_mode::BootModeInformation::Efi => 
>>> BootModeInformation::Efi,
>>> +        boot_mode::BootModeInformation::EfiSecureBoot => 
>>> BootModeInformation::EfiSecureBoot,
>>> +        boot_mode::BootModeInformation::Bios => 
>>> BootModeInformation::Bios,
>>> +    }
>>> +}
>>
>> Mini nit: Maybe a From<boot_mode::BootModeInformation> impl would be a 
>> bit more idiomatic?
> Thought about this as well, but sadly the orphan rule ruins the day :(
> I also don't want to import `proxmox_sys` in `pbs-api-types` or viceversa.
>>
Ah yes, orphan rules... of course, didn't think about them during the 
review :D

-- 
- Lukas

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

[Gabriel Goller]

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',
+	    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})`;
+	    },
 	    value: '',
 	},
 	{
-- 
2.39.2

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

[Lukas Wagner]

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

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

[Gabriel Goller]

Thanks for the review!

On 11/24/23 15:19, Lukas Wagner wrote:
> Hi, some comments inline :)
>
> On 11/24/23 13:02, Gabriel Goller wrote:
>>
>> 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?
Not currently, as we return both the legacy *and* the new field. As we 
release the next
major version, we will remove the `multiField: true` and uncommment the 
`textField` property.
Same as it has been done in pve.
>
>> +        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
Right! Thanks for the heads up!
>
>> +        },
>>           value: '',
>>       },
>>       {
>

Re: [pbs-devel] [PATCH v2 proxmox{-backup, } 0/3] Add boot_mode, improve kernel version

[Lukas Wagner]

On 11/24/23 13:02, Gabriel Goller wrote:
> Ported the recent changes from the PVE NodeSummary (done by @Thomas) to
> the PBS NodeDashboard.
> 
> It consists of:
> * Adding the bootmode field, shows either Legacy BIOS, EFI, or EFI
>      (Secure Boot)
> * Declutter the kernel-version field and only show the release version
>      and build-date.
> 


Quickly tested this, seems to work fine.

Tested-by: Lukas Wagner <l.wagner@proxmox.com>


-- 
- Lukas

Re: [pbs-devel] [PATCH v2 proxmox{-backup, } 0/3] Add boot_mode, improve kernel version

[Thomas Lamprecht]

Am 24/11/2023 um 15:18 schrieb Lukas Wagner:
> On 11/24/23 13:02, Gabriel Goller wrote:
>> Ported the recent changes from the PVE NodeSummary (done by @Thomas) to
>> the PBS NodeDashboard.
>>
>> It consists of:
>> * Adding the bootmode field, shows either Legacy BIOS, EFI, or EFI
>>      (Secure Boot)
>> * Declutter the kernel-version field and only show the release version
>>      and build-date.
>>
> 
> 
> Quickly tested this, seems to work fine.
> 
> Tested-by: Lukas Wagner <l.wagner@proxmox.com>


Thanks for patch and review to both, but IMO this is still differing to much
from Proxmox VE's endpoint without any real justification?

There the "boot-info" object with the required key "mode" and the optional
"secureboot" entry, that explicitly de-couples the general mode from some
mode-specific detail.

The fitting rust struct (at least in sys) would be

pub enum BootModeInformation {
    /// The BootMode is EFI/UEFI, the boolean specifies if secure boot is enabled
    Efi(bool),
    /// The BootMode is Legacy BIOS
    Bios,
}


or if one wants to be overly specific then something like:

pub enum SecureBoot {
    Enabled,
    Disabled,
}


pub enum BootModeInformation {
    /// The BootMode is EFI/UEFI
    Efi(SecureBoot),
    /// The BootMode is Legacy BIOS
    Bios,
}

(but could be overkill)

It's not a hard must to keep this the same for pve-manager and pbs, but IMO
one should have very good reason for changing the format for relaying the
exact same information between two products, such inconsistencies make it
harder to interact with our API for any users, or external devs, and also
won't make it easier to reuse widgets for the (current or future) UIs..

Re: [pbs-devel] [PATCH v2 proxmox{-backup, } 0/3] Add boot_mode, improve kernel version

[Gabriel Goller]

Thanks for the review!

On 11/24/23 15:45, Thomas Lamprecht wrote:
> Thanks for patch and review to both, but IMO this is still differing to much
> from Proxmox VE's endpoint without any real justification?
>
> There the "boot-info" object with the required key "mode" and the optional
> "secureboot" entry, that explicitly de-couples the general mode from some
> mode-specific detail.
>
> The fitting rust struct (at least in sys) would be
>
> pub enum BootModeInformation {
>      /// The BootMode is EFI/UEFI, the boolean specifies if secure boot is enabled
>      Efi(bool),
>      /// The BootMode is Legacy BIOS
>      Bios,
> }
>
>
> or if one wants to be overly specific then something like:
>
> pub enum SecureBoot {
>      Enabled,
>      Disabled,
> }
>
>
> pub enum BootModeInformation {
>      /// The BootMode is EFI/UEFI
>      Efi(SecureBoot),
>      /// The BootMode is Legacy BIOS
>      Bios,
> }
>
> (but could be overkill)
>
> It's not a hard must to keep this the same for pve-manager and pbs, but IMO
> one should have very good reason for changing the format for relaying the
> exact same information between two products, such inconsistencies make it
> harder to interact with our API for any users, or external devs, and also
> won't make it easier to reuse widgets for the (current or future) UIs..
Ok, I will implement this in v2.
I think I'll choose the second one with the specific enum for 
`SecureBoot`. Will
be more clear what is means (without looking at comments) + won't use 
more memory
than the bool version.

Re: [pbs-devel] [PATCH v2 proxmox{-backup, } 0/3] Add boot_mode, improve kernel version

[Gabriel Goller]

On 11/27/23 09:41, Gabriel Goller wrote:

> Thanks for the review!
>
> On 11/24/23 15:45, Thomas Lamprecht wrote:
>> Thanks for patch and review to both, but IMO this is still differing 
>> to much
>> from Proxmox VE's endpoint without any real justification?
>>
>> There the "boot-info" object with the required key "mode" and the 
>> optional
>> "secureboot" entry, that explicitly de-couples the general mode from 
>> some
>> mode-specific detail.
>>
>> The fitting rust struct (at least in sys) would be
>>
>> pub enum BootModeInformation {
>>      /// The BootMode is EFI/UEFI, the boolean specifies if secure 
>> boot is enabled
>>      Efi(bool),
>>      /// The BootMode is Legacy BIOS
>>      Bios,
>> }
>>
>>
>> or if one wants to be overly specific then something like:
>>
>> pub enum SecureBoot {
>>      Enabled,
>>      Disabled,
>> }
>>
>>
>> pub enum BootModeInformation {
>>      /// The BootMode is EFI/UEFI
>>      Efi(SecureBoot),
>>      /// The BootMode is Legacy BIOS
>>      Bios,
>> }
>>
>> (but could be overkill)
>>
>> It's not a hard must to keep this the same for pve-manager and pbs, 
>> but IMO
>> one should have very good reason for changing the format for relaying 
>> the
>> exact same information between two products, such inconsistencies 
>> make it
>> harder to interact with our API for any users, or external devs, and 
>> also
>> won't make it easier to reuse widgets for the (current or future) UIs..
> Ok, I will implement this in v2.
> I think I'll choose the second one with the specific enum for 
> `SecureBoot`. Will
> be more clear what is means (without looking at comments) + won't use 
> more memory
> than the bool version.
>
On a second note, I don't like this.
We don't support enum variants (fields) with the `api` macro, so I would 
have to create
a struct containing `mode` and `secureboot`. That again implies that 
bios + secureboot is
possible, which is not.
But I get your argument about consistency between products, so if we'd 
go that
way, I'd send another patch with the requested changes...

Re: [pbs-devel] [PATCH v2 proxmox{-backup, } 0/3] Add boot_mode, improve kernel version

[Thomas Lamprecht]

On 27.11.23 09:56, Gabriel Goller wrote:
> On a second note, I don't like this.
> We don't support enum variants (fields) with the `api` macro, so I would 
> have to create
> a struct containing `mode` and `secureboot`. That again implies that 
> bios + secureboot is
> possible, which is not.

No that really does not imply that, especially if you use an option fro
secureboot.

> But I get your argument about consistency between products, so if we'd 
> go that
> way, I'd send another patch with the requested changes...
> 

Just to be fully sure: we will NOT take anything in here that isn't 1:1
compatible with what PVE produces on the line (i.e., serialized) what
it looks internal I do not bother (that much).