[pve-devel] [PATCH installer 0/4] fix #4872: properly timeout `traceroute` command in coutry detection

[pve-devel] [PATCH installer 0/4] fix #4872: properly timeout `traceroute` command in coutry detection

[Christoph Heiss]

For all the details, see patch #4.

TL;DR: SIGALRM does not interrupt line reading using <>, causing the
installer to hang on country detection. Fix it by using
Proxmox::Sys::Command::run_command(), which properly interacts with
SIGALRM.

Patch #1 is a rather mundane fix for some niche cases, #2 and #3 are
simply preparatory and to not alter existing behaviour.

Christoph Heiss (4):
  low-level: initialize UI backend for 'dump-env' subcommand too
  sys: command: allow terminating the process early from log subroutine
  sys: command: add option to not print process output to stdout
  fix #4872: run env: use run_command() for country detection

 Proxmox/Install/RunEnv.pm   | 23 ++++++++++++-----------
 Proxmox/Sys/Command.pm      | 13 ++++++++++---
 proxmox-low-level-installer |  1 +
 3 files changed, 23 insertions(+), 14 deletions(-)

--
2.43.0

[pve-devel] [PATCH installer 1/4] low-level: initialize UI backend for 'dump-env' subcommand too

[Christoph Heiss]

Some detection routines might try to log things and call some
Proxmox::Ui functions all the way down, so just initialize it with the
stdio backend to avoid errors.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-low-level-installer | 1 +
 1 file changed, 1 insertion(+)

diff --git a/proxmox-low-level-installer b/proxmox-low-level-installer
index d127a40..2848295 100755
--- a/proxmox-low-level-installer
+++ b/proxmox-low-level-installer
@@ -91,6 +91,7 @@ Proxmox::Log::init("/tmp/install-low-level-${cmd}.log");
 
 my $env = Proxmox::Install::ISOEnv::get();
 if ($cmd eq 'dump-env') {
+    Proxmox::UI::init_stdio({}, $env);
 
     my $out_dir = $env->{locations}->{run};
     make_path($out_dir);
-- 
2.43.0

[pve-devel] [PATCH installer 2/4] sys: command: allow terminating the process early from log subroutine

[Christoph Heiss]

This is done in a entirely backwards-compatible way, i.e. existing
usages don't need any modification.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Sys/Command.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Proxmox/Sys/Command.pm b/Proxmox/Sys/Command.pm
index c3e24b3..bf67b27 100644
--- a/Proxmox/Sys/Command.pm
+++ b/Proxmox/Sys/Command.pm
@@ -114,7 +114,14 @@ sub run_command {
 		$logout .= $buf;
 		while ($logout =~ s/^([^\010\r\n]*)(\r|\n|(\010)+|\r\n)//s) {
 		    my $line = $1;
-		    $func->($line) if $func;
+		    if ($func) {
+			my $ret = $func->($line);
+			if (defined($ret) && $ret == 1) {
+			    kill('KILL', $pid);
+			    waitpid($pid, 0);
+			    return $ostream;
+			}
+		    };
 		}
 
 	    } elsif ($h eq $error) {
-- 
2.43.0

[pve-devel] [PATCH installer 3/4] sys: command: add option to not print process output to stdout

[Christoph Heiss]

Fully backwards-compatible again, only takes effect if the new argument
is actually specified.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Sys/Command.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Proxmox/Sys/Command.pm b/Proxmox/Sys/Command.pm
index bf67b27..a5129fd 100644
--- a/Proxmox/Sys/Command.pm
+++ b/Proxmox/Sys/Command.pm
@@ -40,7 +40,7 @@ sub syscmd {
 }
 
 sub run_command {
-    my ($cmd, $func, $input, $noout) = @_;
+    my ($cmd, $func, $input, $noout, $noprint) = @_;
 
     my $cmdstr;
     if (!ref($cmd)) {
@@ -127,7 +127,7 @@ sub run_command {
 	    } elsif ($h eq $error) {
 		$ostream .= $buf if !($noout || $func);
 	    }
-	    print $buf;
+	    print $buf if !$noprint;
 	    STDOUT->flush();
 	    log_info($buf);
 	}
-- 
2.43.0

[pve-devel] [PATCH installer 4/4] fix #4872: run env: use run_command() for country detection

[Christoph Heiss]

This fixes a rather longstanding issue [0][1] with the country
detection, in that it might get completely stuck and thus hangs the
installation.

This is due how Perl, signals and line reading interacts.

A minimal reproducer, how the installer currently works, looks like
this:
```
    #!/usr/bin/env perl

    use strict;
    use warnings;

    open (my $fh, '-|', 'sleep', '1000') or die;

    my $prev = alarm(2);
    eval {
	local $SIG{ALRM} = sub { "timed out!\n" };

	my $line;
	while (defined ($line = <$fh>)) {
	    print "line: $line";
	}
    };

    alarm($prev);
    close($fh);
```

One might expect that this times out after 2 seconds, as specified in
`alarm(2)`. The thruth is that `$line = <$fh>` apparently prevents the
signal to go through. This then causes the installer to hang there
indefinitely, if `traceroute` never progresses - which seems to happen
on lots of (weird) networks, as evidently can be seen in the forum [1].

Proxmox::Sys::Command::run_command() handles of these weird cases, takes
care of the nitty-gritty details and - most importantly - interacts
properly with SIGALRM, so just use that instead.

This _should_ really fix that issue, but reproducing it 1:1 as part of
the installation process is _very_ hard, basically pure luck. But
rewriting the reproducer using run_command (in the exact same way that
this patch rewrites detect_country_tracing_to()) fixes the issue there,
so it's the best we can probably do.

NB: This causes that the traceroute command is now printed to the log
(as run_command() logs that by default), which we could also hide e.g.
through another parameter if wanted.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=4872
[1] https://forum.proxmox.com/threads/proxmox-installation-trying-to-detect-country.134301/

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Install/RunEnv.pm | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index c393f67..ee6b8bc 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -7,6 +7,7 @@ use Carp;
 use JSON qw(from_json to_json);

 use Proxmox::Log;
+use Proxmox::Sys::Command qw(run_command);
 use Proxmox::Sys::File qw(file_read_firstline);
 use Proxmox::Sys::Block;
 use Proxmox::Sys::Net;
@@ -188,34 +189,34 @@ my sub detect_country_tracing_to : prototype($$) {
     my ($ipver, $destination) = @_;

     print STDERR "trying to detect country...\n";
-    open(my $TRACEROUTE_FH, '-|', 'traceroute', "-$ipver", '-N', '1', '-q', '1', '-n', $destination)
-	or return undef;

+    my $traceroute_cmd = ['traceroute', "-$ipver", '-N', '1', '-q', '1', '-n', $destination];
     my $geoip_bin = ($ipver == 6) ? 'geoiplookup6' : 'geoiplookup';

     my $country;
-
-    my $previous_alarm = alarm (10);
     eval {
 	local $SIG{ALRM} = sub { die "timed out!\n" };
-	my $line;
-	while (defined ($line = <$TRACEROUTE_FH>)) {
+	my $previous_alarm = alarm (10);
+
+	run_command($traceroute_cmd, sub {
+	    my $line = shift;
+
 	    log_debug("DC TRACEROUTE: $line");
 	    if ($line =~ m/^\s*\d+\s+(\S+)\s/) {
 		my $geoip = qx/$geoip_bin $1/;
 		log_debug("DC GEOIP: $geoip");
+
 		if ($geoip =~ m/GeoIP Country Edition:\s*([A-Z]+),/) {
 		    $country = lc ($1);
 		    log_info("DC FOUND: $country\n");
-		    last;
+		    return 1;
 		}
 	    }
-	}
+	}, undef, undef, 1);
+
+	alarm ($previous_alarm);
     };
     my $err = $@;
-    alarm ($previous_alarm);
-
-    close($TRACEROUTE_FH);

     if ($err) {
 	die "unable to detect country - $err\n";
--
2.43.0

Re: [pve-devel] [PATCH installer 1/4] low-level: initialize UI backend for 'dump-env' subcommand too

[Aaron Lauterer]

Is this something I should consider for the new dump-udev subcommand as well?

https://lists.proxmox.com/pipermail/pve-devel/2024-January/061433.html

On 2/9/24 11:55, Christoph Heiss wrote:
> Some detection routines might try to log things and call some
> Proxmox::Ui functions all the way down, so just initialize it with the
> stdio backend to avoid errors.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>   proxmox-low-level-installer | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/proxmox-low-level-installer b/proxmox-low-level-installer
> index d127a40..2848295 100755
> --- a/proxmox-low-level-installer
> +++ b/proxmox-low-level-installer
> @@ -91,6 +91,7 @@ Proxmox::Log::init("/tmp/install-low-level-${cmd}.log");
>   
>   my $env = Proxmox::Install::ISOEnv::get();
>   if ($cmd eq 'dump-env') {
> +    Proxmox::UI::init_stdio({}, $env);
>   
>       my $out_dir = $env->{locations}->{run};
>       make_path($out_dir);

Re: [pve-devel] [PATCH installer 1/4] low-level: initialize UI backend for 'dump-env' subcommand too

[Christoph Heiss]

On Fri, Feb 09, 2024 at 01:36:12PM +0100, Aaron Lauterer wrote:
> Is this something I should consider for the new dump-udev subcommand as well?
>
> https://lists.proxmox.com/pipermail/pve-devel/2024-January/061433.html

As it also calls deeper into the installer code, I'd say yes. Although
nothing ever calls UI code on that chain, better future-proof it now :^)

>
> On 2/9/24 11:55, Christoph Heiss wrote:
> > Some detection routines might try to log things and call some
> > Proxmox::Ui functions all the way down, so just initialize it with the
> > stdio backend to avoid errors.
> >
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > ---
> >   proxmox-low-level-installer | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/proxmox-low-level-installer b/proxmox-low-level-installer
> > index d127a40..2848295 100755
> > --- a/proxmox-low-level-installer
> > +++ b/proxmox-low-level-installer
> > @@ -91,6 +91,7 @@ Proxmox::Log::init("/tmp/install-low-level-${cmd}.log");
> >   my $env = Proxmox::Install::ISOEnv::get();
> >   if ($cmd eq 'dump-env') {
> > +    Proxmox::UI::init_stdio({}, $env);
> >       my $out_dir = $env->{locations}->{run};
> >       make_path($out_dir);

Re: [pve-devel] [PATCH installer 2/4] sys: command: allow terminating the process early from log subroutine

[Thomas Lamprecht]

Am 09/02/2024 um 11:55 schrieb Christoph Heiss:
> This is done in a entirely backwards-compatible way, i.e. existing
> usages don't need any modification.

can you actually describe here that you do so by checking the return
value of the stdout parser closure, can be done in a short sentence
and surely doesn't hurt.

> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  Proxmox/Sys/Command.pm | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Proxmox/Sys/Command.pm b/Proxmox/Sys/Command.pm
> index c3e24b3..bf67b27 100644
> --- a/Proxmox/Sys/Command.pm
> +++ b/Proxmox/Sys/Command.pm
> @@ -114,7 +114,14 @@ sub run_command {
>  		$logout .= $buf;
>  		while ($logout =~ s/^([^\010\r\n]*)(\r|\n|(\010)+|\r\n)//s) {
>  		    my $line = $1;
> -		    $func->($line) if $func;
> +		    if ($func) {
> +			my $ret = $func->($line);
> +			if (defined($ret) && $ret == 1) {

maybe we can define a constant for this, so that we have 
descriptive name that better conveys the meaning of this special
return value, e.g., something like:

use constant RET_TIMED_OUT => 1;

(no hard feelings on that name, just used the first thing that came to
my mind)


> +			    kill('KILL', $pid);
> +			    waitpid($pid, 0);

FWIW, this can block forever too though?

How about factoring out the kill+waiting in it's own sub (could be
also one for waitpid with timeout and combined kill+waitpid that reuses
the first one), that does a TERM first, checks with WNOHANG if the
target PID exited and if not, sends the KILL and loops until a timeout
(say, /throws dice/, 5s) expired.

That way no D-state, or whatever else can block the process' death,
is hanging up the installer.

> +			    return $ostream;
> +			}
> +		    };
>  		}
>  
>  	    } elsif ($h eq $error) {

Re: [pve-devel] [PATCH installer 2/4] sys: command: allow terminating the process early from log subroutine

[Christoph Heiss]

Thanks for the review!

On Mon, Feb 12, 2024 at 11:58:27AM +0100, Thomas Lamprecht wrote:
> Am 09/02/2024 um 11:55 schrieb Christoph Heiss:
> > This is done in a entirely backwards-compatible way, i.e. existing
> > usages don't need any modification.
>
> can you actually describe here that you do so by checking the return
> value of the stdout parser closure, can be done in a short sentence
> and surely doesn't hurt.

I'll add some more documentation for this in v2.

>
> >
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > ---
> >  Proxmox/Sys/Command.pm | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/Proxmox/Sys/Command.pm b/Proxmox/Sys/Command.pm
> > index c3e24b3..bf67b27 100644
> > --- a/Proxmox/Sys/Command.pm
> > +++ b/Proxmox/Sys/Command.pm
> > @@ -114,7 +114,14 @@ sub run_command {
> >  		$logout .= $buf;
> >  		while ($logout =~ s/^([^\010\r\n]*)(\r|\n|(\010)+|\r\n)//s) {
> >  		    my $line = $1;
> > -		    $func->($line) if $func;
> > +		    if ($func) {
> > +			my $ret = $func->($line);
> > +			if (defined($ret) && $ret == 1) {
>
> maybe we can define a constant for this, so that we have
> descriptive name that better conveys the meaning of this special
> return value, e.g., something like:

Sounds good, I will add that.

>
> use constant RET_TIMED_OUT => 1;
>
> (no hard feelings on that name, just used the first thing that came to
> my mind)
>
>
> > +			    kill('KILL', $pid);
> > +			    waitpid($pid, 0);
>
> FWIW, this can block forever too though?

It /could/ probably happen if the process e.g. somehow enters D-state
after reading the previous line of output, good catch.

>
> How about factoring out the kill+waiting in it's own sub (could be
> also one for waitpid with timeout and combined kill+waitpid that reuses
> the first one), that does a TERM first, checks with WNOHANG if the
> target PID exited and if not, sends the KILL and loops until a timeout
> (say, /throws dice/, 5s) expired.
>
> That way no D-state, or whatever else can block the process' death,
> is hanging up the installer.

That seems like the /really/ bulletproof way.

This was more a proof-of-concept-fix anyway in the beginning, so a bit
of refactoring on that thing is warranted after all, I guess.

>
> > +			    return $ostream;
> > +			}
> > +		    };
> >  		}
> >
> >  	    } elsif ($h eq $error) {
>