* [pve-devel] [PATCH installer 0/4] fix #4872: properly timeout `traceroute` command in coutry detection @ 2024-02-09 10:55 Christoph Heiss 2024-02-09 10:55 ` [pve-devel] [PATCH installer 1/4] low-level: initialize UI backend for 'dump-env' subcommand too Christoph Heiss ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Christoph Heiss @ 2024-02-09 10:55 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH installer 1/4] low-level: initialize UI backend for 'dump-env' subcommand too 2024-02-09 10:55 [pve-devel] [PATCH installer 0/4] fix #4872: properly timeout `traceroute` command in coutry detection Christoph Heiss @ 2024-02-09 10:55 ` Christoph Heiss 2024-02-09 12:36 ` Aaron Lauterer 2024-02-09 10:55 ` [pve-devel] [PATCH installer 2/4] sys: command: allow terminating the process early from log subroutine Christoph Heiss ` (2 subsequent siblings) 3 siblings, 1 reply; 9+ messages in thread From: Christoph Heiss @ 2024-02-09 10:55 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH installer 1/4] low-level: initialize UI backend for 'dump-env' subcommand too 2024-02-09 10:55 ` [pve-devel] [PATCH installer 1/4] low-level: initialize UI backend for 'dump-env' subcommand too Christoph Heiss @ 2024-02-09 12:36 ` Aaron Lauterer 2024-02-12 10:15 ` Christoph Heiss 0 siblings, 1 reply; 9+ messages in thread From: Aaron Lauterer @ 2024-02-09 12:36 UTC (permalink / raw) To: Proxmox VE development discussion, Christoph Heiss 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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH installer 1/4] low-level: initialize UI backend for 'dump-env' subcommand too 2024-02-09 12:36 ` Aaron Lauterer @ 2024-02-12 10:15 ` Christoph Heiss 0 siblings, 0 replies; 9+ messages in thread From: Christoph Heiss @ 2024-02-12 10:15 UTC (permalink / raw) To: Aaron Lauterer; +Cc: Proxmox VE development discussion 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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH installer 2/4] sys: command: allow terminating the process early from log subroutine 2024-02-09 10:55 [pve-devel] [PATCH installer 0/4] fix #4872: properly timeout `traceroute` command in coutry detection Christoph Heiss 2024-02-09 10:55 ` [pve-devel] [PATCH installer 1/4] low-level: initialize UI backend for 'dump-env' subcommand too Christoph Heiss @ 2024-02-09 10:55 ` Christoph Heiss 2024-02-12 10:58 ` Thomas Lamprecht 2024-02-09 10:56 ` [pve-devel] [PATCH installer 3/4] sys: command: add option to not print process output to stdout Christoph Heiss 2024-02-09 10:56 ` [pve-devel] [PATCH installer 4/4] fix #4872: run env: use run_command() for country detection Christoph Heiss 3 siblings, 1 reply; 9+ messages in thread From: Christoph Heiss @ 2024-02-09 10:55 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH installer 2/4] sys: command: allow terminating the process early from log subroutine 2024-02-09 10:55 ` [pve-devel] [PATCH installer 2/4] sys: command: allow terminating the process early from log subroutine Christoph Heiss @ 2024-02-12 10:58 ` Thomas Lamprecht 2024-02-12 12:06 ` Christoph Heiss 0 siblings, 1 reply; 9+ messages in thread From: Thomas Lamprecht @ 2024-02-12 10:58 UTC (permalink / raw) To: Proxmox VE development discussion, Christoph Heiss 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) { ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH installer 2/4] sys: command: allow terminating the process early from log subroutine 2024-02-12 10:58 ` Thomas Lamprecht @ 2024-02-12 12:06 ` Christoph Heiss 0 siblings, 0 replies; 9+ messages in thread From: Christoph Heiss @ 2024-02-12 12:06 UTC (permalink / raw) To: Thomas Lamprecht; +Cc: Proxmox VE development discussion 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) { > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH installer 3/4] sys: command: add option to not print process output to stdout 2024-02-09 10:55 [pve-devel] [PATCH installer 0/4] fix #4872: properly timeout `traceroute` command in coutry detection Christoph Heiss 2024-02-09 10:55 ` [pve-devel] [PATCH installer 1/4] low-level: initialize UI backend for 'dump-env' subcommand too Christoph Heiss 2024-02-09 10:55 ` [pve-devel] [PATCH installer 2/4] sys: command: allow terminating the process early from log subroutine Christoph Heiss @ 2024-02-09 10:56 ` Christoph Heiss 2024-02-09 10:56 ` [pve-devel] [PATCH installer 4/4] fix #4872: run env: use run_command() for country detection Christoph Heiss 3 siblings, 0 replies; 9+ messages in thread From: Christoph Heiss @ 2024-02-09 10:56 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [pve-devel] [PATCH installer 4/4] fix #4872: run env: use run_command() for country detection 2024-02-09 10:55 [pve-devel] [PATCH installer 0/4] fix #4872: properly timeout `traceroute` command in coutry detection Christoph Heiss ` (2 preceding siblings ...) 2024-02-09 10:56 ` [pve-devel] [PATCH installer 3/4] sys: command: add option to not print process output to stdout Christoph Heiss @ 2024-02-09 10:56 ` Christoph Heiss 3 siblings, 0 replies; 9+ messages in thread From: Christoph Heiss @ 2024-02-09 10:56 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-02-12 12:06 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-09 10:55 [pve-devel] [PATCH installer 0/4] fix #4872: properly timeout `traceroute` command in coutry detection Christoph Heiss 2024-02-09 10:55 ` [pve-devel] [PATCH installer 1/4] low-level: initialize UI backend for 'dump-env' subcommand too Christoph Heiss 2024-02-09 12:36 ` Aaron Lauterer 2024-02-12 10:15 ` Christoph Heiss 2024-02-09 10:55 ` [pve-devel] [PATCH installer 2/4] sys: command: allow terminating the process early from log subroutine Christoph Heiss 2024-02-12 10:58 ` Thomas Lamprecht 2024-02-12 12:06 ` Christoph Heiss 2024-02-09 10:56 ` [pve-devel] [PATCH installer 3/4] sys: command: add option to not print process output to stdout Christoph Heiss 2024-02-09 10:56 ` [pve-devel] [PATCH installer 4/4] fix #4872: run env: use run_command() for country detection Christoph Heiss
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox