[pve-devel] [PATCH installer 0/6] small, overall install progress improvements

[pve-devel] [PATCH installer 0/6] small, overall install progress improvements

[Christoph Heiss]

This series tries to improve upon some small things around the
installation progress reporting, esp. in the auto-installer, as well as
some small fixes & code-deduplication.

Christoph Heiss (6):
  test: ui2-stdio: fix multi-process testing
  auto, tui: move low-level installer message struct to common crate
  auto: log non-json low-level messages into separate file
  low-level: stdio: fix: make progress text properly optional
  low-level: add proper message to 100% progress ratio update
  auto: avoid printing unnecessary progress update lines

 Proxmox/Install.pm                            |  2 +-
 Proxmox/UI/StdIO.pm                           |  8 ++-
 .../src/bin/proxmox-auto-installer.rs         | 35 ++++++++--
 proxmox-auto-installer/src/utils.rs           | 23 -------
 proxmox-installer-common/src/setup.rs         | 23 +++++++
 .../src/views/install_progress.rs             | 67 +++++++------------
 test/ui2-stdio.pl                             | 10 +--
 7 files changed, 88 insertions(+), 80 deletions(-)

-- 
2.44.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 1/6] test: ui2-stdio: fix multi-process testing

[Christoph Heiss]

Previously, if something failed in the child, the overall test would
still be successful and exit with `0`.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 test/ui2-stdio.pl | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/test/ui2-stdio.pl b/test/ui2-stdio.pl
index ae29b79..01f323d 100755
--- a/test/ui2-stdio.pl
+++ b/test/ui2-stdio.pl
@@ -4,7 +4,6 @@ use strict;
 use warnings;
 
 use JSON qw(from_json);
-use Test::More;
 
 use Proxmox::Log;
 use Proxmox::UI;
@@ -16,10 +15,11 @@ $parent_writer->autoflush(1);
 $child_writer->autoflush(1);
 $child_reader->autoflush(1);
 
-
 my $child_pid = fork() // die "fork failed - $!\n";
 
 if ($child_pid) {
+    eval 'use Test::More tests => 3;';
+
     # parent, the hypothetical low-level installer
     close($parent_reader);
     close($parent_writer);
@@ -45,8 +45,11 @@ if ($child_pid) {
     Proxmox::UI::progress(1, 0, 1, 'done');
 
     waitpid($child_pid, 0);
+    is($?, 0); # check child exit status
     done_testing();
 } else {
+    eval 'use Test::More tests => 10;';
+
     # child, e.g. the TUI
     close($child_reader);
     close($child_writer);
@@ -90,6 +93,5 @@ if ($child_pid) {
 	'should get 100% done progress message');
 
     done_testing();
-    exit(0);
 }
 
-- 
2.44.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH installer 1/6] test: ui2-stdio: fix multi-process testing

[Thomas Lamprecht]

Am 16.05.24 um 15:39 schrieb Christoph Heiss:
> Previously, if something failed in the child, the overall test would
> still be successful and exit with `0`.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  test/ui2-stdio.pl | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
>

applied this one, thanks!

most of the remaining patches need some rebasing or are ordered such, that
I'm not sure if I can simply apply them  without the previous one without
an in-depth look, but as some rebasing is required anyway I'm just go for
the lazy^W efficient variant and skip looking into that ;P


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 2/6] auto, tui: move low-level installer message struct to common crate

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 .../src/bin/proxmox-auto-installer.rs         |  8 ++--
 proxmox-auto-installer/src/utils.rs           | 23 ---------
 proxmox-installer-common/src/setup.rs         | 23 +++++++++
 .../src/views/install_progress.rs             | 48 +++++--------------
 4 files changed, 38 insertions(+), 64 deletions(-)

diff --git a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
index 9fcec1e..e607a9c 100644
--- a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
+++ b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
@@ -8,14 +8,12 @@ use std::{
 };
 
 use proxmox_installer_common::setup::{
-    installer_setup, read_json, spawn_low_level_installer, LocaleInfo, RuntimeInfo, SetupInfo,
+    installer_setup, read_json, spawn_low_level_installer, LocaleInfo, LowLevelMessage,
+    RuntimeInfo, SetupInfo,
 };
 
 use proxmox_auto_installer::{
-    answer::Answer,
-    log::AutoInstLogger,
-    udevinfo::UdevInfo,
-    utils::{parse_answer, LowLevelMessage},
+    answer::Answer, log::AutoInstLogger, udevinfo::UdevInfo, utils::parse_answer,
 };
 
 static LOGGER: AutoInstLogger = AutoInstLogger;
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 202ad41..7f3688d 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -380,26 +380,3 @@ pub fn parse_answer(
     }
     Ok(config)
 }
-
-#[derive(Clone, Debug, Deserialize, PartialEq)]
-#[serde(tag = "type", rename_all = "lowercase")]
-pub enum LowLevelMessage {
-    #[serde(rename = "message")]
-    Info {
-        message: String,
-    },
-    Error {
-        message: String,
-    },
-    Prompt {
-        query: String,
-    },
-    Finished {
-        state: String,
-        message: String,
-    },
-    Progress {
-        ratio: f32,
-        text: String,
-    },
-}
diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs
index 64d05af..0c62bc3 100644
--- a/proxmox-installer-common/src/setup.rs
+++ b/proxmox-installer-common/src/setup.rs
@@ -558,3 +558,26 @@ where
         _ => Err(de::Error::custom("could not find file system: {de_fs}")),
     }
 }
+
+#[derive(Clone, Debug, Deserialize, PartialEq)]
+#[serde(tag = "type", rename_all = "lowercase")]
+pub enum LowLevelMessage {
+    #[serde(rename = "message")]
+    Info {
+        message: String,
+    },
+    Error {
+        message: String,
+    },
+    Prompt {
+        query: String,
+    },
+    Finished {
+        state: String,
+        message: String,
+    },
+    Progress {
+        ratio: f32,
+        text: String,
+    },
+}
diff --git a/proxmox-tui-installer/src/views/install_progress.rs b/proxmox-tui-installer/src/views/install_progress.rs
index 6453426..11b12f0 100644
--- a/proxmox-tui-installer/src/views/install_progress.rs
+++ b/proxmox-tui-installer/src/views/install_progress.rs
@@ -4,7 +4,6 @@ use cursive::{
     views::{Dialog, DummyView, LinearLayout, PaddedView, ProgressBar, TextView},
     CbSink, Cursive,
 };
-use serde::Deserialize;
 use std::{
     fs::File,
     io::{BufRead, BufReader, Write},
@@ -14,7 +13,7 @@ use std::{
 };
 
 use crate::{abort_install_button, prompt_dialog, InstallerState};
-use proxmox_installer_common::setup::{spawn_low_level_installer, InstallConfig};
+use proxmox_installer_common::setup::{spawn_low_level_installer, InstallConfig, LowLevelMessage};
 
 pub struct InstallProgressView {
     view: PaddedView<LinearLayout>,
@@ -105,7 +104,7 @@ impl InstallProgressView {
                     continue;
                 }
 
-                let msg = match serde_json::from_str::<UiMessage>(&line) {
+                let msg = match serde_json::from_str::<LowLevelMessage>(&line) {
                     Ok(msg) => msg,
                     Err(err) => {
                         // Not a fatal error, so don't abort the installation by returning
@@ -116,17 +115,17 @@ impl InstallProgressView {
                 };
 
                 let result = match msg.clone() {
-                    UiMessage::Info { message } => cb_sink.send(Box::new(|siv| {
+                    LowLevelMessage::Info { message } => cb_sink.send(Box::new(|siv| {
                         siv.add_layer(Dialog::info(message).title("Information"));
                     })),
-                    UiMessage::Error { message } => cb_sink.send(Box::new(|siv| {
+                    LowLevelMessage::Error { message } => cb_sink.send(Box::new(|siv| {
                         siv.add_layer(Dialog::info(message).title("Error"));
                     })),
-                    UiMessage::Prompt { query } => cb_sink.send({
+                    LowLevelMessage::Prompt { query } => cb_sink.send({
                         let writer = writer.clone();
                         Box::new(move |siv| Self::show_prompt(siv, &query, writer))
                     }),
-                    UiMessage::Progress { ratio, text } => {
+                    LowLevelMessage::Progress { ratio, text } => {
                         counter.set((ratio * 100.).floor() as usize);
                         cb_sink.send(Box::new(move |siv| {
                             siv.call_on_name(Self::PROGRESS_TEXT_VIEW_ID, |v: &mut TextView| {
@@ -134,7 +133,7 @@ impl InstallProgressView {
                             });
                         }))
                     }
-                    UiMessage::Finished { state, message } => {
+                    LowLevelMessage::Finished { state, message } => {
                         counter.set(100);
                         cb_sink.send(Box::new(move |siv| {
                             siv.call_on_name(Self::PROGRESS_TEXT_VIEW_ID, |v: &mut TextView| {
@@ -241,29 +240,6 @@ impl ViewWrapper for InstallProgressView {
     cursive::wrap_impl!(self.view: PaddedView<LinearLayout>);
 }
 
-#[derive(Clone, Debug, Deserialize, PartialEq)]
-#[serde(tag = "type", rename_all = "lowercase")]
-enum UiMessage {
-    #[serde(rename = "message")]
-    Info {
-        message: String,
-    },
-    Error {
-        message: String,
-    },
-    Prompt {
-        query: String,
-    },
-    Finished {
-        state: String,
-        message: String,
-    },
-    Progress {
-        ratio: f32,
-        text: String,
-    },
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -292,7 +268,7 @@ mod tests {
             let mut line = String::new();
             reader.read_line(&mut line).expect("a line");
 
-            match serde_json::from_str::<UiMessage>(&line) {
+            match serde_json::from_str::<LowLevelMessage>(&line) {
                 Ok(msg) => Some(msg),
                 Err(err) => panic!("unexpected error: '{err}'"),
             }
@@ -300,7 +276,7 @@ mod tests {
 
         assert_eq!(
             next_msg(),
-            Some(UiMessage::Prompt {
+            Some(LowLevelMessage::Prompt {
                 query: "Reply anything?".to_owned()
             }),
         );
@@ -314,7 +290,7 @@ mod tests {
 
         assert_eq!(
             next_msg(),
-            Some(UiMessage::Info {
+            Some(LowLevelMessage::Info {
                 message: "Test Message - got ok".to_owned()
             }),
         );
@@ -322,7 +298,7 @@ mod tests {
         for i in (1..=1000).step_by(3) {
             assert_eq!(
                 next_msg(),
-                Some(UiMessage::Progress {
+                Some(LowLevelMessage::Progress {
                     ratio: (i as f32) / 1000.,
                     text: format!("foo {i}"),
                 }),
@@ -331,7 +307,7 @@ mod tests {
 
         assert_eq!(
             next_msg(),
-            Some(UiMessage::Finished {
+            Some(LowLevelMessage::Finished {
                 state: "ok".to_owned(),
                 message: "Installation finished - reboot now?".to_owned(),
             }),
-- 
2.44.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 3/6] auto: log non-json low-level messages into separate file

[Christoph Heiss]

Same as the TUI does, as introduced in [0].

[0] 22de6e5 ("tui: install_progress: write low-level non-JSON messages to separate file")

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 .../src/bin/proxmox-auto-installer.rs           | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
index e607a9c..9fc328c 100644
--- a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
+++ b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
@@ -2,6 +2,7 @@ use anyhow::{bail, format_err, Result};
 use log::{error, info, LevelFilter};
 use std::{
     env,
+    fs::File,
     io::{BufRead, BufReader, Write},
     path::PathBuf,
     process::ExitCode,
@@ -136,15 +137,29 @@ fn run_installation(
             .map_err(|err| format_err!("failed to serialize install config: {err}"))?;
         writeln!(writer).map_err(|err| format_err!("failed to write install config: {err}"))?;
 
+        let mut lowlevel_log = File::create("/tmp/install-low-level.log")
+            .map_err(|err| format_err!("failed to open low-level installer logfile: {err}"))?;
+
         for line in reader.lines() {
             let line = match line {
                 Ok(line) => line,
                 Err(_) => break,
             };
+
+            // The low-level installer also spews the output of any command it runs on its
+            // stdout. Use a very simple heuricstic to determine whether it is actually JSON
+            // or not.
+            if !line.starts_with('{') || !line.ends_with('}') {
+                let _ = writeln!(lowlevel_log, "{}", line);
+                continue;
+            }
+
             let msg = match serde_json::from_str::<LowLevelMessage>(&line) {
                 Ok(msg) => msg,
-                Err(_) => {
+                Err(err) => {
                     // Not a fatal error, so don't abort the installation by returning
+                    eprintln!("low-level installer: error while parsing message: '{err}'");
+                    eprintln!("    original message was: '{line}'");
                     continue;
                 }
             };
-- 
2.44.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 4/6] low-level: stdio: fix: make progress text properly optional

[Christoph Heiss]

.. such that receivers can differentiate between these two cases more
clearly.

Sometimes, the `progress` sub does not get passed a text (on purpose!),
just updating the progress ratio. This would cause log messages to be
written out which could indicate missing text and look rather weird.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/UI/StdIO.pm                           |  8 +++++---
 .../src/bin/proxmox-auto-installer.rs         |  6 +++++-
 proxmox-installer-common/src/setup.rs         |  2 +-
 .../src/views/install_progress.rs             | 19 +++++++++++++------
 test/ui2-stdio.pl                             |  2 +-
 5 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/Proxmox/UI/StdIO.pm b/Proxmox/UI/StdIO.pm
index 2838fcb..ec9eac9 100644
--- a/Proxmox/UI/StdIO.pm
+++ b/Proxmox/UI/StdIO.pm
@@ -70,9 +70,11 @@ sub display_html {
 sub progress {
     my ($self, $ratio, $text) = @_;
 
-    $text = '' if !defined($text);
-
-    send_msg('progress', ratio => $ratio, text => $text);
+    if (defined($text)) {
+	send_msg('progress', ratio => $ratio, text => $text);
+    } else {
+	send_msg('progress', ratio => $ratio);
+    }
 }
 
 sub process_events {
diff --git a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
index 9fc328c..96a48bd 100644
--- a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
+++ b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
@@ -172,7 +172,11 @@ fn run_installation(
                 }
                 LowLevelMessage::Progress { ratio, text } => {
                     let percentage = ratio * 100.;
-                    info!("progress {percentage:>5.1} % - {text}");
+                    if let Some(text) = text {
+                        info!("progress {percentage:>5.1} % - {text}");
+                    } else {
+                        info!("progress {percentage:>5.1} %");
+                    }
                 }
                 LowLevelMessage::Finished { state, message } => {
                     if state == "err" {
diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs
index 0c62bc3..d145fc5 100644
--- a/proxmox-installer-common/src/setup.rs
+++ b/proxmox-installer-common/src/setup.rs
@@ -578,6 +578,6 @@ pub enum LowLevelMessage {
     },
     Progress {
         ratio: f32,
-        text: String,
+        text: Option<String>,
     },
 }
diff --git a/proxmox-tui-installer/src/views/install_progress.rs b/proxmox-tui-installer/src/views/install_progress.rs
index 11b12f0..629472a 100644
--- a/proxmox-tui-installer/src/views/install_progress.rs
+++ b/proxmox-tui-installer/src/views/install_progress.rs
@@ -127,11 +127,18 @@ impl InstallProgressView {
                     }),
                     LowLevelMessage::Progress { ratio, text } => {
                         counter.set((ratio * 100.).floor() as usize);
-                        cb_sink.send(Box::new(move |siv| {
-                            siv.call_on_name(Self::PROGRESS_TEXT_VIEW_ID, |v: &mut TextView| {
-                                v.set_content(text);
-                            });
-                        }))
+                        if let Some(text) = text {
+                            cb_sink.send(Box::new(move |siv| {
+                                siv.call_on_name(
+                                    Self::PROGRESS_TEXT_VIEW_ID,
+                                    |v: &mut TextView| {
+                                        v.set_content(text);
+                                    },
+                                );
+                            }))
+                        } else {
+                            Ok(())
+                        }
                     }
                     LowLevelMessage::Finished { state, message } => {
                         counter.set(100);
@@ -300,7 +307,7 @@ mod tests {
                 next_msg(),
                 Some(LowLevelMessage::Progress {
                     ratio: (i as f32) / 1000.,
-                    text: format!("foo {i}"),
+                    text: Some(format!("foo {i}")),
                 }),
             );
         }
diff --git a/test/ui2-stdio.pl b/test/ui2-stdio.pl
index 01f323d..eebb5c9 100755
--- a/test/ui2-stdio.pl
+++ b/test/ui2-stdio.pl
@@ -81,7 +81,7 @@ if ($child_pid) {
 	'should get 20% done progress message');
 
     is_deeply(
-	$next_msg->(), { type => 'progress', ratio => 0.2, text => '' },
+	$next_msg->(), { type => 'progress', ratio => 0.2, },
 	'should get progress continuation message');
 
     is_deeply(
-- 
2.44.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 5/6] low-level: add proper message to 100% progress ratio update

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Install.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Proxmox/Install.pm b/Proxmox/Install.pm
index c0f8955..af857ca 100644
--- a/Proxmox/Install.pm
+++ b/Proxmox/Install.pm
@@ -1331,7 +1331,7 @@ _EOD
 
     my $err = $@;
 
-    update_progress(1, 0, 1, "");
+    update_progress(1, 0, 1, "installation finished");
 
     print STDERR $err if $err && $err ne "\n";
 
-- 
2.44.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 6/6] auto: avoid printing unnecessary progress update lines

[Christoph Heiss]

In case the progress message did not contain any text and the percentage
did not change, don't print another (useless) line.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-auto-installer/src/bin/proxmox-auto-installer.rs | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
index 96a48bd..a72181f 100644
--- a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
+++ b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
@@ -140,6 +140,8 @@ fn run_installation(
         let mut lowlevel_log = File::create("/tmp/install-low-level.log")
             .map_err(|err| format_err!("failed to open low-level installer logfile: {err}"))?;
 
+        let mut last_progress_percentage = 0.;
+
         for line in reader.lines() {
             let line = match line {
                 Ok(line) => line,
@@ -174,8 +176,10 @@ fn run_installation(
                     let percentage = ratio * 100.;
                     if let Some(text) = text {
                         info!("progress {percentage:>5.1} % - {text}");
-                    } else {
+                        last_progress_percentage = percentage;
+                    } else if (percentage - last_progress_percentage) > 0.1 {
                         info!("progress {percentage:>5.1} %");
+                        last_progress_percentage = percentage;
                     }
                 }
                 LowLevelMessage::Finished { state, message } => {
-- 
2.44.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH installer 0/6] small, overall install progress improvements

[Christoph Heiss]

Ping, still applies cleanly to current master as of today (23-07-2024).

Did a quick test round of Auto/GUI/TUI too, just to confirm everything.

On Thu, May 16, 2024 at 03:39:30PM GMT, Christoph Heiss wrote:
> This series tries to improve upon some small things around the
> installation progress reporting, esp. in the auto-installer, as well as
> some small fixes & code-deduplication.
>
> Christoph Heiss (6):
>   test: ui2-stdio: fix multi-process testing
>   auto, tui: move low-level installer message struct to common crate
>   auto: log non-json low-level messages into separate file
>   low-level: stdio: fix: make progress text properly optional
>   low-level: add proper message to 100% progress ratio update
>   auto: avoid printing unnecessary progress update lines
>
>  Proxmox/Install.pm                            |  2 +-
>  Proxmox/UI/StdIO.pm                           |  8 ++-
>  .../src/bin/proxmox-auto-installer.rs         | 35 ++++++++--
>  proxmox-auto-installer/src/utils.rs           | 23 -------
>  proxmox-installer-common/src/setup.rs         | 23 +++++++
>  .../src/views/install_progress.rs             | 67 +++++++------------
>  test/ui2-stdio.pl                             | 10 +--
>  7 files changed, 88 insertions(+), 80 deletions(-)
>
> --
> 2.44.0
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel