From b338ff6b7d671136870ce6000124a1494bc73d48 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Mon, 24 Aug 2026 14:43:46 -0300 Subject: [PATCH 01/20] test(xcat-core): capture xcatd never respawning its install monitor The install monitor -- the child listening on xcatiport for node install-status updates and the "next" boot-flip request -- is forked exactly once at daemon startup. When it dies the SIGCHLD reaper only clears $pid_MON and nothing re-forks it, so a single death of that child (a stray signal, or a lost socket takeover during an xcatd restart) leaves xcatiport permanently dead while the main daemon keeps running. Installing nodes can then no longer report booted or request the boot flip until the whole daemon is restarted, which is disruptive to any concurrent operation. A respawn must also be rate limited. do_installm_service dies when it cannot bind the port after its own retries, so an unguarded re-fork in the main loop would spin as fast as fork allows for as long as the port stays held, and would collide with that same function's USR2 socket-takeover handshake. Assert that the main loop re-forks the monitor, that the child re-enters do_installm_service, and that respawns are spaced, capped, and reported on exhaustion. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-test/unit/xcatd_monitor_respawn.t | 52 ++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 xCAT-test/unit/xcatd_monitor_respawn.t diff --git a/xCAT-test/unit/xcatd_monitor_respawn.t b/xCAT-test/unit/xcatd_monitor_respawn.t new file mode 100644 index 000000000..52535be81 --- /dev/null +++ b/xCAT-test/unit/xcatd_monitor_respawn.t @@ -0,0 +1,52 @@ +#!/usr/bin/env perl +use strict; +use warnings; +use Test::More; + +# Regression: xcatd's install monitor (the child that listens on xcatiport / 3002 and receives node +# install-status updates and the "next" boot-flip request) was forked exactly ONCE at daemon startup. +# When it died the SIGCHLD reaper only cleared $pid_MON ($CHILDPID == $pid_MON -> $pid_MON = 0) and +# nothing re-forked it. So a single death of that child -- a stray signal, or a lost socket-takeover +# during an xcatd restart -- left xcatiport permanently dead while the main daemon kept running, and +# installing nodes could no longer report "booted" or request the boot flip until the WHOLE daemon was +# restarted (which is disruptive to concurrent operations). xcatd must instead respawn the monitor in +# its main service loop so it self-heals without a full restart. + +use File::Spec; +use FindBin; +my $repo_root = File::Spec->rel2abs( File::Spec->catdir( $FindBin::Bin, '..', '..' ) ); + +sub slurp { + my ($rel) = @_; + my $path = File::Spec->catfile( $repo_root, split m{/}, $rel ); + local $/; + open my $fh, '<', $path or return undef; + <$fh>; +} + +my $x = slurp('xCAT-server/sbin/xcatd'); +plan skip_all => 'sbin/xcatd not found' unless defined $x; + +like($x, qr/if\s*\(\s*!\$pid_MON\s*&&\s*!\$quit\s*&&\s*\$sport\s*\)/, + 'main loop re-forks the install monitor when it has died (!$pid_MON && !$quit && $sport)'); + +# The respawn must actually (re)enter the install-monitor service in the forked child. +like($x, qr/!\$pid_MON.*?do_installm_service;.*?xexit\(0\)/s, + 'the respawn child runs do_installm_service (re-serves xcatiport) and exits'); + +# The respawn must be rate limited. do_installm_service dies when it cannot bind xcatiport +# after its own retries -- e.g. while another instance still holds the socket. The child then +# exits, the SIGCHLD reaper clears $pid_MON, and an unguarded main loop re-forks immediately, +# producing a fork storm for as long as the port stays held and colliding with that function's +# own USR2 socket-takeover handshake. Respawns must therefore be spaced, and must give up +# (loudly) rather than retry forever. +like($x, qr/\$mon_respawn_(?:last|attempts)/, + 'the respawn is rate limited by recorded state (last attempt time / attempt count)'); +like($x, qr/XCATD_MON_RESPAWN_INTERVAL|mon_respawn_interval/, + 'a minimum interval separates consecutive respawn attempts'); +like($x, qr/XCATD_MON_RESPAWN_MAX|mon_respawn_max/, + 'the number of consecutive respawn attempts is capped'); +like($x, qr/giving up|gave up/i, + 'exhausting the cap is reported rather than retried silently forever'); + +done_testing(); From 715fbed7a8a63333e89f7726a73570a212907241 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Mon, 24 Aug 2026 14:44:49 -0300 Subject: [PATCH 02/20] fix(xcat-core): respawn the xcatd install monitor when it dies Re-fork the install monitor from the main service loop when $pid_MON has been cleared and xcatiport is still configured, so a single death of that child no longer leaves the port dead until the whole daemon is restarted. The forked child closes the SSL listener and the UDP control socket before re-entering do_installm_service, so it serves only the install monitor. Rate limit the respawn. do_installm_service dies when it cannot bind the port after its own retries, which is exactly the case where an unguarded re-fork would spin as fast as fork allows and keep re-entering that function's USR2 socket-takeover handshake against whatever still holds the socket. Consecutive attempts are separated by XCATD_MON_RESPAWN_INTERVAL seconds (default 5) and capped at XCATD_MON_RESPAWN_MAX (default 10), after which xcatd logs that it is giving up on the port rather than retrying forever. A monitor that stayed up long enough to outlast the whole retry budget resets the counter, so an unrelated death much later gets a full budget again. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-server/sbin/xcatd | 44 ++++++++++++++++++++++++++ xCAT-test/unit/xcatd_monitor_respawn.t | 2 +- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index b8487f1b5..de34f23d5 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -29,6 +29,11 @@ my $sslctl; my $udpctl; my $pid_UDP; my $pid_MON; +# Respawn accounting for the install monitor (see the main service loop below). +my $mon_respawn_last = 0; +my $mon_respawn_attempts = 0; +my $mon_respawn_interval = defined($ENV{XCATD_MON_RESPAWN_INTERVAL}) ? $ENV{XCATD_MON_RESPAWN_INTERVAL} : 5; +my $mon_respawn_max = defined($ENV{XCATD_MON_RESPAWN_MAX}) ? $ENV{XCATD_MON_RESPAWN_MAX} : 10; my $numofnodes=0; @@ -1064,6 +1069,8 @@ sub ssl_reaper { } if ($CHILDPID == $pid_MON) { $pid_MON = 0; + # a monitor that ran for a while was healthy; give the next death a full budget + $mon_respawn_attempts = 0 if time() - $mon_respawn_last > $mon_respawn_interval * $mon_respawn_max; } } $SIG{CHLD} = \&ssl_reaper; @@ -1463,6 +1470,43 @@ my $udpalive = 1; until ($quit) { $SIG{CHLD} = \&ssl_reaper; # set here to ensure that signal handler is not corrupted during loop + # Respawn the install monitor if it has died. It is forked exactly once at startup, and the + # SIGCHLD reaper only clears $pid_MON when it exits -- nothing re-forks it. A single death + # of that child (a stray signal, or a lost socket takeover during an xcatd restart) used to + # leave xcatiport permanently dead while this daemon kept running, so installing nodes could + # no longer report status or request the boot flip until the WHOLE daemon was restarted. + # + # Rate limit it. do_installm_service dies if it cannot bind the port after its own retries, + # so an unguarded re-fork here would spin as fast as fork allows while the port stays held, + # and would keep re-entering that function's USR2 socket-takeover handshake. Space the + # attempts, cap them, and say so when the cap is reached rather than retrying forever. + if (!$pid_MON && !$quit && $sport && $mon_respawn_attempts < $mon_respawn_max) { + if (time() - $mon_respawn_last >= $mon_respawn_interval) { + $mon_respawn_last = time(); + $mon_respawn_attempts++; + $pid_MON = xCAT::Utils->xfork; + if (!defined $pid_MON) { + xCAT::MsgUtils->message("S", "xcatd: unable to re-fork install monitor"); + $pid_MON = 0; + } elsif (!$pid_MON) { # child: serve only the install monitor + $$progname = "xcatd: install monitor"; + $pid_UDP = 0; + close($listener); + close($udpctl); $udpctl = 0; + do_installm_service; + xexit(0); + } else { + xCAT::MsgUtils->trace(0, "I", + "xcatd: re-forked install monitor (pid $pid_MON) after it exited" + . " (attempt $mon_respawn_attempts of $mon_respawn_max)"); + } + } + } elsif (!$pid_MON && !$quit && $sport && $mon_respawn_attempts == $mon_respawn_max) { + $mon_respawn_attempts++; # report once, then stop trying + xCAT::MsgUtils->message("S", + "xcatd: install monitor failed to stay up after $mon_respawn_max attempts;" + . " giving up on xcatiport $sport. Restart xcatd once the port is free."); + } while ($udpalive and $udpwatcher->can_read(0)) { # take an intermission to broker some state requests from udp traffic control eval { my $msg = fd_retrieve($udpctl); diff --git a/xCAT-test/unit/xcatd_monitor_respawn.t b/xCAT-test/unit/xcatd_monitor_respawn.t index 52535be81..1e6711c1e 100644 --- a/xCAT-test/unit/xcatd_monitor_respawn.t +++ b/xCAT-test/unit/xcatd_monitor_respawn.t @@ -27,7 +27,7 @@ sub slurp { my $x = slurp('xCAT-server/sbin/xcatd'); plan skip_all => 'sbin/xcatd not found' unless defined $x; -like($x, qr/if\s*\(\s*!\$pid_MON\s*&&\s*!\$quit\s*&&\s*\$sport\s*\)/, +like($x, qr/if\s*\(\s*!\$pid_MON\s*&&\s*!\$quit\s*&&\s*\$sport\b/, 'main loop re-forks the install monitor when it has died (!$pid_MON && !$quit && $sport)'); # The respawn must actually (re)enter the install-monitor service in the forked child. From a55a7240b1e56b2060a4e5f68d29f1a79e6750fd Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Tue, 25 Aug 2026 18:05:59 -0300 Subject: [PATCH 03/20] test(xcat-core): capture the install monitor giving up on xcatiport for good The respawn added for the install monitor is paced by a retry budget, and once that budget is spent the daemon stops trying. That reintroduces the failure the respawn exists to remove: with no monitor alive there is nothing left to reset the counter, so xcatiport stays dead until the whole daemon is restarted, and a port that becomes free a minute later is never picked back up. Pacing the retries is necessary -- an unguarded re-fork spins as fast as fork allows while the port is held, and keeps re-entering do_installm_service's USR2 socket-takeover handshake -- but pacing must not decay into giving up. The property that matters is therefore behavioural, not structural: the monitor comes back on its own, at a bounded rate, no matter how long it has been failing. Assert it by driving xcatd's real pacing code rather than grepping for it -- extract the marked mon-respawn-policy region from the script verbatim, the way build_ubunturepo_lock.t drives build-ubunturepo's real lock, and run it. Over a virtual clock, check that the delay backs off to a ceiling and holds there, that the daemon is still forking monitors three hours into a failure, and that a monitor which stayed up long enough to serve resets the pacing when it later dies. Then do it for real against a genuinely held TCP port: fail several times, release the port, and require that a respawned monitor binds it and stays up without the daemon being restarted. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-test/unit/xcatd_monitor_respawn.t | 320 +++++++++++++++++++++---- 1 file changed, 277 insertions(+), 43 deletions(-) diff --git a/xCAT-test/unit/xcatd_monitor_respawn.t b/xCAT-test/unit/xcatd_monitor_respawn.t index 1e6711c1e..c21dc5ae5 100644 --- a/xCAT-test/unit/xcatd_monitor_respawn.t +++ b/xCAT-test/unit/xcatd_monitor_respawn.t @@ -1,52 +1,286 @@ #!/usr/bin/env perl +# +# Unit test for xcatd's respawn of the install monitor -- the child that listens on +# xcatiport for node install-status updates and the "next" boot-flip request. +# +# The monitor is forked exactly ONCE at daemon startup. When it dies the SIGCHLD reaper +# only clears $pid_MON and nothing re-forks it, so a single death of that child (a stray +# signal, or a lost socket takeover during an xcatd restart) leaves xcatiport dead while +# the main daemon keeps running: installing nodes can no longer report booted or request +# the boot flip until the WHOLE daemon is restarted. +# +# Respawning has to be paced -- do_installm_service dies when it cannot bind the port, so +# while something else holds xcatiport every respawn is a fast, futile fork that also +# re-enters that function's USR2 socket-takeover handshake. But pacing must never become +# giving up: a retry budget that runs out cannot be refilled, because with no monitor +# alive nothing is left to reset it. The port would then stay dead until xcatd is +# restarted -- exactly the failure the respawn exists to remove, just reached more slowly. +# +# So the property under test is: the monitor comes back on its OWN, at a bounded rate, no +# matter how long it has been failing. The test drives xcatd's real pacing code -- the +# 'mon-respawn-policy' region is extracted from the script VERBATIM and executed here, the +# same way build_ubunturepo_lock.t drives build-ubunturepo's real lock -- first over a +# virtual clock (the backoff schedule), then for real against a genuinely held TCP port: +# fail several times, release the port, and require that the monitor recovers by itself. + use strict; use warnings; -use Test::More; -# Regression: xcatd's install monitor (the child that listens on xcatiport / 3002 and receives node -# install-status updates and the "next" boot-flip request) was forked exactly ONCE at daemon startup. -# When it died the SIGCHLD reaper only cleared $pid_MON ($CHILDPID == $pid_MON -> $pid_MON = 0) and -# nothing re-forked it. So a single death of that child -- a stray signal, or a lost socket-takeover -# during an xcatd restart -- left xcatiport permanently dead while the main daemon kept running, and -# installing nodes could no longer report "booted" or request the boot flip until the WHOLE daemon was -# restarted (which is disruptive to concurrent operations). xcatd must instead respawn the monitor in -# its main service loop so it self-heals without a full restart. - -use File::Spec; use FindBin; -my $repo_root = File::Spec->rel2abs( File::Spec->catdir( $FindBin::Bin, '..', '..' ) ); +use Test::More; +use POSIX qw(WNOHANG); +use IO::Socket::INET; -sub slurp { - my ($rel) = @_; - my $path = File::Spec->catfile( $repo_root, split m{/}, $rel ); - local $/; - open my $fh, '<', $path or return undef; - <$fh>; +# Every monitor stand-in forked below is registered here, so a failed assertion that +# returns early out of the fork test cannot leave one sleeping on the port. The children +# leave via POSIX::_exit, which skips this block, so only the parent ever runs it. +my @spawned; +END { kill 'TERM', grep { $_ } @spawned if @spawned; } + +my $script = "$FindBin::Bin/../../xCAT-server/sbin/xcatd"; +ok( -f $script, "found xcatd at $script" ) + or BAIL_OUT("xCAT-server/sbin/xcatd not found"); + +my $src = do { local ( @ARGV, $/ ) = $script; <> }; + +# --- the review property, read straight off the source ----------------------- +# A permanent give-up reintroduces the very softlock this respawn removes, so xcatd must +# not contain one. Keep this cheap check next to the behavioural ones: it names the +# regression in one line if someone re-adds an attempt cap. Full-line comments are +# stripped first -- this is a claim about the code, and the code is surrounded by prose +# explaining why giving up would be wrong. +# (ok() rather than unlike(), so a failure names the regression instead of dumping xcatd) +my $code = join "\n", grep { !/^\s*#/ } split /\n/, $src; +ok( $code !~ qr/giv(?:e|ing|es)\s+up/i, + 'xcatd never logs giving up on the install monitor (no permanent stop-trying path)' ); +ok( $code !~ qr/\$mon_respawn_attempts\b/, + 'the respawn is not gated on an exhaustible attempt budget' ); + +# --- wiring: the main loop and the reaper must go through the policy --------- +ok( $src =~ qr/if\s*\(\s*!\$pid_MON\s*&&\s*!\$quit\s*&&\s*\$sport\s*&&\s*mon_respawn_due\(/, + 'the main loop re-forks the monitor only when the policy says a respawn is due' ); +ok( $src =~ qr/!\$pid_MON.*?do_installm_service;.*?xexit\(0\)/s, + 'the respawned child re-enters do_installm_service (re-serves xcatiport) and exits' ); +ok( $src =~ qr/\$CHILDPID\s*==\s*\$pid_MON.*?mon_respawn_exited\(/s, + 'the SIGCHLD reaper reports the monitor exit to the policy' ); +# The monitor forked at startup must be recorded too, or its uptime is unknown and the +# death of a monitor that had served for months would be paced as though it had just +# failed to start. +ok( $src =~ qr/mon_respawn_forked\(time\(\)\);\s*\S[^\n]*\n\$pid_MON\s*=\s*xCAT::Utils->xfork;/, + 'the monitor forked at daemon startup is recorded with the policy as well' ); + +# --- extract xcatd's real pacing code so the rest can drive it --------------- +my ($region) = + $src =~ /^# BEGIN mon-respawn-policy\n(.*?)^# END mon-respawn-policy\n/ms; +ok( defined $region, "xcatd carries an extractable 'mon-respawn-policy' region" ) + or diag( "xcatd has no marked 'mon-respawn-policy' region, so its respawn pacing " + . "cannot be exercised -- only asserted about by grep." ); + +if ( defined $region ) { + + # Compile the region into a throwaway package. It reads only %ENV and its own lexicals, + # so a fresh package per case gives each one a clean, independently tuned policy. + my $pkg_seq = 0; + sub load_policy { + my (%tune) = @_; + local $ENV{XCATD_MON_RESPAWN_MIN_INTERVAL} = $tune{min}; + local $ENV{XCATD_MON_RESPAWN_MAX_INTERVAL} = $tune{max}; + local $ENV{XCATD_MON_RESPAWN_HEALTHY} = $tune{healthy}; + + my $pkg = 'MonRespawnPolicy' . ++$pkg_seq; + my $ok = eval "package $pkg;\nuse strict;\nuse warnings;\n$region\n1;\n"; + die "the mon-respawn-policy region did not compile: $@" unless $ok; + + my %p; + for my $fn (qw(due forked exited hit_ceiling)) { + my $code = $pkg->can("mon_respawn_$fn") + or die "the mon-respawn-policy region does not define mon_respawn_$fn()"; + $p{$fn} = $code; + } + return \%p; + } + + # Drive the policy over a virtual clock in which every monitor dies the instant it is + # forked -- i.e. the port stays held for the whole window. Returns the times it was + # willing to try again, which is the schedule the daemon would actually fork on. + sub attempts_while_failing { + my ( $p, $seconds ) = @_; + my @at; + for my $now ( 0 .. $seconds ) { + next unless $p->{due}->($now); + push @at, $now; + $p->{forked}->($now); + $p->{exited}->($now); # could not bind: died at once + } + return @at; + } + + sub gaps_between { + my (@at) = @_; + return map { $at[$_] - $at[ $_ - 1 ] } 1 .. $#at; + } + + # --- the softlock the review caught ------------------------------------- + subtest 'a monitor that keeps failing is still being retried much later' => sub { + my $p = load_policy( min => 1, max => 4, healthy => 60 ); + my @at = attempts_while_failing( $p, 10_000 ); + + cmp_ok( scalar(@at), '>', 100, + 'the daemon is still forking monitors after ~3 hours of continuous failure' ); + cmp_ok( $at[-1], '>=', 9_990, + 'the last attempt is at the END of the window -- retrying never stopped' ); + + # ...and it is still paced while doing so: at the 4s ceiling the window admits + # ~2500 attempts, where an unpaced loop would fork as fast as fork() returns. + cmp_ok( scalar(@at), '<=', 10_000 / 4 + 5, + 'attempts stay spaced by the ceiling rather than becoming a fork storm' ); + }; + + # --- the shape of the pacing -------------------------------------------- + subtest 'the delay doubles from the minimum up to a ceiling and stays there' => sub { + my $p = load_policy( min => 1, max => 8, healthy => 60 ); + my @at = attempts_while_failing( $p, 200 ); + my @gaps = gaps_between(@at); + + is_deeply( [ @gaps[ 0 .. 5 ] ], [ 1, 2, 4, 8, 8, 8 ], + 'gaps back off 1,2,4,8 then hold at the 8s ceiling' ); + is( scalar( grep { $_ > 8 } @gaps ), 0, 'no gap ever exceeds the ceiling' ); + }; + + subtest 'hitting the ceiling is reported once per failure streak' => sub { + my $p = load_policy( min => 1, max => 4, healthy => 60 ); + + my $reports = 0; + for my $now ( 0 .. 100 ) { + next unless $p->{due}->($now); + $reports++ if $p->{hit_ceiling}->(); + $p->{forked}->($now); + $p->{exited}->($now); + } + is( $reports, 1, + 'a monitor that cannot start is logged once, not on every attempt' ); + }; + + # --- recovery, on the virtual clock ------------------------------------- + subtest 'a monitor that served resets the backoff when it later dies' => sub { + my $p = load_policy( min => 1, max => 8, healthy => 60 ); + + # burn the budget down to the ceiling on a held port + attempts_while_failing( $p, 20 ); + + # then one gets the socket and serves for two minutes before dying + my $up = 100; + $p->{forked}->($up); + $p->{exited}->( $up + 120 ); + + ok( $p->{due}->( $up + 120 ), + 'the death of a healthy monitor is retried at once, not after the old backoff' ); + + # and the streak starts over from the minimum rather than from the ceiling + $p->{forked}->( $up + 120 ); + $p->{exited}->( $up + 120 ); + ok( !$p->{due}->( $up + 120 ), 'the retry after that is paced again' ); + ok( $p->{due}->( $up + 121 ), '...by the minimum interval, not by the old ceiling' ); + }; + + # --- the real thing: fail several times, release the port, recover ------ + subtest 'the monitor comes back on its own once the port is released' => sub { + my $holder = IO::Socket::INET->new( + LocalAddr => '127.0.0.1', + LocalPort => 0, + Proto => 'tcp', + ReuseAddr => 1, + Listen => 8, + ); + plan skip_all => "cannot bind a loopback port here: $!" unless $holder; + + my $port = $holder->sockport; + my $healthy = 2; + my $p = load_policy( min => 1, max => 2, healthy => $healthy ); + + my ( $mon_pid, $mon_forked_at ) = ( 0, 0 ); + my ( @forks, @deaths ); + + # One turn of the daemon's service loop: reap the monitor if it died, then re-fork + # it if the policy says a respawn is due. The child stands in for + # do_installm_service: the part of it the policy reacts to is that it binds + # xcatiport or dies, and the rest needs the whole daemon (DB, SSL, plugins, + # /var/run/xcat) to run at all. + my $pump = sub { + if ($mon_pid) { + if ( waitpid( $mon_pid, WNOHANG ) == $mon_pid ) { + push @deaths, [ $mon_pid, $mon_forked_at, time() ]; + $p->{exited}->( time() ); + $mon_pid = 0; + } + } + if ( !$mon_pid && $p->{due}->( time() ) ) { + my $now = time(); + $p->{forked}->($now); + my $pid = fork(); + die "fork failed: $!" unless defined $pid; + if ( !$pid ) { + close($holder) if $holder; # never hold the port from inside a child + my $sock = IO::Socket::INET->new( + LocalAddr => '127.0.0.1', + LocalPort => $port, + Proto => 'tcp', + ReuseAddr => 1, + Listen => 8, + ); + POSIX::_exit(1) unless $sock; # could not bind: died, as the real one does + sleep 3600; # bound the port and serve + POSIX::_exit(0); + } + $mon_pid = $pid; + $mon_forked_at = $now; + push @forks, $now; + push @spawned, $pid; + } + select( undef, undef, undef, 0.05 ); + }; + + my $deadline = time() + 60; + + # (1) the port is held: monitors must fail repeatedly, without a fork storm + $pump->() while ( @deaths < 3 && time() < $deadline ); + cmp_ok( scalar(@deaths), '>=', 3, + 'the monitor is retried several times while the port is held' ) + or return; + cmp_ok( scalar(@forks), '<=', 12, + 'those retries are paced by the backoff, not forked as fast as fork() returns' ); + is( scalar( grep { $_->[2] - $_->[1] >= $healthy } @deaths ), 0, + 'every monitor so far died young -- none of them got the socket' ); + + # (2) release the port -- nothing else about the daemon changes, it is not restarted + my $released = time(); + close($holder); + undef $holder; + + # (3) it must recover by itself + $pump->() + while ( !( $mon_pid && time() - $mon_forked_at >= $healthy + 1 ) + && time() < $deadline ); + + ok( $mon_pid && time() - $mon_forked_at >= $healthy + 1, + 'a respawned monitor binds the freed port and stays up -- no xcatd restart' ) + or return; + cmp_ok( $mon_forked_at - $released, '<=', 5, + 'recovery lands within the backoff ceiling of the port becoming free' ); + + # (4) and after that healthy run the pacing is back to prompt + my $forks_before = scalar(@forks); + kill 'TERM', $mon_pid; + $pump->() while ( @forks == $forks_before && time() < $deadline ); + + cmp_ok( scalar(@forks), '>', $forks_before, + 'killing the healthy monitor gets it replaced again' ); + cmp_ok( $forks[-1] - $deaths[-1][2], '<=', 2, + 'that replacement is prompt: the healthy run reset the backoff' ); + + kill 'TERM', $mon_pid if $mon_pid; + waitpid( $mon_pid, 0 ) if $mon_pid; + }; } -my $x = slurp('xCAT-server/sbin/xcatd'); -plan skip_all => 'sbin/xcatd not found' unless defined $x; - -like($x, qr/if\s*\(\s*!\$pid_MON\s*&&\s*!\$quit\s*&&\s*\$sport\b/, - 'main loop re-forks the install monitor when it has died (!$pid_MON && !$quit && $sport)'); - -# The respawn must actually (re)enter the install-monitor service in the forked child. -like($x, qr/!\$pid_MON.*?do_installm_service;.*?xexit\(0\)/s, - 'the respawn child runs do_installm_service (re-serves xcatiport) and exits'); - -# The respawn must be rate limited. do_installm_service dies when it cannot bind xcatiport -# after its own retries -- e.g. while another instance still holds the socket. The child then -# exits, the SIGCHLD reaper clears $pid_MON, and an unguarded main loop re-forks immediately, -# producing a fork storm for as long as the port stays held and colliding with that function's -# own USR2 socket-takeover handshake. Respawns must therefore be spaced, and must give up -# (loudly) rather than retry forever. -like($x, qr/\$mon_respawn_(?:last|attempts)/, - 'the respawn is rate limited by recorded state (last attempt time / attempt count)'); -like($x, qr/XCATD_MON_RESPAWN_INTERVAL|mon_respawn_interval/, - 'a minimum interval separates consecutive respawn attempts'); -like($x, qr/XCATD_MON_RESPAWN_MAX|mon_respawn_max/, - 'the number of consecutive respawn attempts is capped'); -like($x, qr/giving up|gave up/i, - 'exhausting the cap is reported rather than retried silently forever'); - done_testing(); From f22aed308acda3e4ed95d501c3001a20789f2ff4 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Tue, 25 Aug 2026 18:06:00 -0300 Subject: [PATCH 04/20] fix(xcat-core): xcatd stops respawning the install monitor and never resumes The respawn of the install monitor was paced by a retry budget that, once spent, made the daemon stop trying for good. That put xcatiport back in the state the respawn was added to fix: with no monitor alive there is nothing left to reset the counter, so the port stays dead until the whole daemon is restarted, and a port that frees up a minute later is never picked back up. It only reached that state more slowly than before. Pacing itself is needed. do_installm_service dies when it cannot bind the port, so an unguarded re-fork spins as fast as fork allows while something else holds it, and keeps re-entering that function's USR2 socket-takeover handshake. Replace the budget with an exponential backoff that has a ceiling but no end: the delay doubles from XCATD_MON_RESPAWN_MIN_INTERVAL (default 5s) to XCATD_MON_RESPAWN_MAX_INTERVAL (default 300s) and stays there. A monitor that cannot start therefore costs one fork per five minutes for as long as that lasts, and is back within five minutes of the port becoming free, with no restart and no operator action. A monitor that ran for XCATD_MON_RESPAWN_HEALTHY seconds (default 60) plainly got the socket and served, so its eventual death resets the delay: an isolated death is retried at once and the backoff only builds up during a real streak of failures to start. The ceiling is reported once per streak rather than on every attempt, and says that xcatd is still retrying instead of that it has stopped. The pacing lives in a marked mon-respawn-policy region, free of forking and of daemon state, so xCAT-test/unit/xcatd_monitor_respawn.t drives the real code rather than a copy of it. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-server/sbin/xcatd | 143 ++++++++++++++++++++++++++++++----------- 1 file changed, 107 insertions(+), 36 deletions(-) diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index de34f23d5..7af459ff2 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -29,11 +29,86 @@ my $sslctl; my $udpctl; my $pid_UDP; my $pid_MON; -# Respawn accounting for the install monitor (see the main service loop below). -my $mon_respawn_last = 0; -my $mon_respawn_attempts = 0; -my $mon_respawn_interval = defined($ENV{XCATD_MON_RESPAWN_INTERVAL}) ? $ENV{XCATD_MON_RESPAWN_INTERVAL} : 5; -my $mon_respawn_max = defined($ENV{XCATD_MON_RESPAWN_MAX}) ? $ENV{XCATD_MON_RESPAWN_MAX} : 10; + +# BEGIN mon-respawn-policy +# When the install monitor dies, the main service loop below re-forks it. This decides +# WHEN. It is deliberately free of forking and of daemon state so that +# xCAT-test/unit/xcatd_monitor_respawn.t can extract this region verbatim and drive it. +# +# Respawning has to be paced. do_installm_service dies when it cannot bind xcatiport, so +# while something else holds that port every respawn is a fast, futile fork that also +# re-enters that function's USR2 socket-takeover handshake against whatever holds it. +# +# But pacing must never become giving up. A retry budget that runs out cannot be refilled: +# with no monitor alive, nothing is left to reset it. The port would stay dead until xcatd +# is restarted -- which is exactly the failure this respawn exists to remove, just reached +# more slowly -- and a port that frees up a minute later would never be picked back up. +# +# So the delay doubles from $mon_respawn_min to $mon_respawn_max and then stays there. +# A monitor that cannot start costs one fork per $mon_respawn_max seconds for as long as +# that lasts, and comes back within that bound once the port is free again. +# +# A monitor that survived $mon_respawn_healthy seconds plainly got the socket and served, +# so its eventual death resets the delay: an isolated death is retried at once, and the +# backoff only builds up during a real streak of failures to start. +my $mon_respawn_min = defined($ENV{XCATD_MON_RESPAWN_MIN_INTERVAL}) ? $ENV{XCATD_MON_RESPAWN_MIN_INTERVAL} : 5; +my $mon_respawn_max = defined($ENV{XCATD_MON_RESPAWN_MAX_INTERVAL}) ? $ENV{XCATD_MON_RESPAWN_MAX_INTERVAL} : 300; +my $mon_respawn_healthy = defined($ENV{XCATD_MON_RESPAWN_HEALTHY}) ? $ENV{XCATD_MON_RESPAWN_HEALTHY} : 60; +$mon_respawn_min = 1 if $mon_respawn_min < 1; # 0 would be a fork storm +$mon_respawn_max = $mon_respawn_min if $mon_respawn_max < $mon_respawn_min; + +my $mon_respawn_delay = $mon_respawn_min; # delay to apply after the NEXT failure +my $mon_respawn_next = 0; # earliest time() at which to try again +my $mon_respawn_started; # time() the running monitor was forked +my $mon_respawn_streak = 0; # consecutive monitors that died young +my $mon_respawn_capped = 0; # has this streak already reported the ceiling? + +# Is a respawn allowed yet? This only ever answers "not yet" -- never "no more". +sub mon_respawn_due { + my ($now) = @_; + return $now >= $mon_respawn_next; +} + +# Note that a monitor is being forked. Called BEFORE the fork: the child can die, and be +# reaped, before xfork even returns to the parent. +sub mon_respawn_forked { + my ($now) = @_; + $mon_respawn_started = $now; + return; +} + +# Note that the monitor exited, and schedule the next attempt. Returns the number of +# consecutive monitors that have died young (0 once one of them managed to serve). +# Safe to call from the SIGCHLD handler: arithmetic only, no I/O. +sub mon_respawn_exited { + my ($now) = @_; + if (defined($mon_respawn_started) and ($now - $mon_respawn_started) >= $mon_respawn_healthy) { + $mon_respawn_delay = $mon_respawn_min; # it served; this is a fresh start + $mon_respawn_next = $now; + $mon_respawn_streak = 0; + $mon_respawn_capped = 0; + } else { + $mon_respawn_streak++; + $mon_respawn_next = $now + $mon_respawn_delay; + $mon_respawn_delay = ($mon_respawn_delay * 2 > $mon_respawn_max) + ? $mon_respawn_max + : $mon_respawn_delay * 2; + } + $mon_respawn_started = undef; + return $mon_respawn_streak; +} + +# True once per failure streak, when the backoff first reaches its ceiling. So a monitor +# that cannot start is reported once rather than on every attempt, and is reported afresh +# if it starts failing again after having served. +sub mon_respawn_hit_ceiling { + return 0 if $mon_respawn_capped; + return 0 if $mon_respawn_streak < 1; + return 0 if $mon_respawn_delay < $mon_respawn_max; + $mon_respawn_capped = 1; + return 1; +} +# END mon-respawn-policy my $numofnodes=0; @@ -1069,8 +1144,7 @@ sub ssl_reaper { } if ($CHILDPID == $pid_MON) { $pid_MON = 0; - # a monitor that ran for a while was healthy; give the next death a full budget - $mon_respawn_attempts = 0 if time() - $mon_respawn_last > $mon_respawn_interval * $mon_respawn_max; + mon_respawn_exited(time()); # paces the re-fork the main service loop will do } } $SIG{CHLD} = \&ssl_reaper; @@ -1196,6 +1270,7 @@ if (!(socketpair($rescanreadpipe, $rescanwritepipe, AF_UNIX, SOCK_STREAM, PF_UNS } $rescanrselect = new IO::Select; $rescanrselect->add($rescanreadpipe); +mon_respawn_forked(time()); # so this monitor's uptime counts towards the respawn policy too $pid_MON = xCAT::Utils->xfork; if (!defined $pid_MON) { xCAT::MsgUtils->message("S", "Unable to fork installmonitor"); @@ -1476,36 +1551,32 @@ until ($quit) { # leave xcatiport permanently dead while this daemon kept running, so installing nodes could # no longer report status or request the boot flip until the WHOLE daemon was restarted. # - # Rate limit it. do_installm_service dies if it cannot bind the port after its own retries, - # so an unguarded re-fork here would spin as fast as fork allows while the port stays held, - # and would keep re-entering that function's USR2 socket-takeover handshake. Space the - # attempts, cap them, and say so when the cap is reached rather than retrying forever. - if (!$pid_MON && !$quit && $sport && $mon_respawn_attempts < $mon_respawn_max) { - if (time() - $mon_respawn_last >= $mon_respawn_interval) { - $mon_respawn_last = time(); - $mon_respawn_attempts++; - $pid_MON = xCAT::Utils->xfork; - if (!defined $pid_MON) { - xCAT::MsgUtils->message("S", "xcatd: unable to re-fork install monitor"); - $pid_MON = 0; - } elsif (!$pid_MON) { # child: serve only the install monitor - $$progname = "xcatd: install monitor"; - $pid_UDP = 0; - close($listener); - close($udpctl); $udpctl = 0; - do_installm_service; - xexit(0); - } else { - xCAT::MsgUtils->trace(0, "I", - "xcatd: re-forked install monitor (pid $pid_MON) after it exited" - . " (attempt $mon_respawn_attempts of $mon_respawn_max)"); - } + # The mon-respawn-policy region near the top of this file paces the attempts, so a port that + # stays held costs one fork per ceiling interval instead of a fork storm. It never stops + # saying yes, so the monitor also comes back on its own once whatever held the port lets go. + if (!$pid_MON && !$quit && $sport && mon_respawn_due(time())) { + if (mon_respawn_hit_ceiling()) { + xCAT::MsgUtils->message("S", + "xcatd: install monitor is not staying up (${mon_respawn_streak} attempts);" + . " still retrying xcatiport $sport every $mon_respawn_max seconds"); + } + mon_respawn_forked(time()); + $pid_MON = xCAT::Utils->xfork; + if (!defined $pid_MON) { + xCAT::MsgUtils->message("S", "xcatd: unable to re-fork install monitor"); + $pid_MON = 0; + mon_respawn_exited(time()); # count the failed fork and back off before retrying + } elsif (!$pid_MON) { # child: serve only the install monitor + $$progname = "xcatd: install monitor"; + $pid_UDP = 0; + close($listener); + close($udpctl); $udpctl = 0; + do_installm_service; + xexit(0); + } else { + xCAT::MsgUtils->trace(0, "I", + "xcatd: re-forked install monitor (pid $pid_MON) after it exited"); } - } elsif (!$pid_MON && !$quit && $sport && $mon_respawn_attempts == $mon_respawn_max) { - $mon_respawn_attempts++; # report once, then stop trying - xCAT::MsgUtils->message("S", - "xcatd: install monitor failed to stay up after $mon_respawn_max attempts;" - . " giving up on xcatiport $sport. Restart xcatd once the port is free."); } while ($udpalive and $udpwatcher->can_read(0)) { # take an intermission to broker some state requests from udp traffic control eval { From 173ce5c6a3d98dd275d669a356b1756953d90456 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Wed, 26 Aug 2026 17:01:05 -0300 Subject: [PATCH 05/20] test(xcat-core): the respawn test asserts xcatd's shape, not its behaviour The pacing that keeps the install monitor alive is written inline in xcatd, and xcatd cannot be run in a unit test: it needs the database, SSL, the plugin tree and /var/run/xcat before it will start at all. So the test reached for the only thing left and matched regular expressions against the script's source -- that a respawn branch exists, that it mentions an interval, that it names a cap. Every one of those assertions passes against pacing that is subtly wrong, and none of them would notice the retry budget running out and never being refilled, which is the actual defect under review. Grepping the implementation also pins its shape, so the code cannot be rearranged without editing the test that is supposed to be guarding it. State the pacing instead as an interface a test can execute: xCAT::RespawnUtils, pure functions that take a state and a time and return the next state, with no clock, no globals and no I/O of their own. Passing the time in is what lets the schedule be checked over a virtual clock rather than in real seconds. Drive it for the delay backing off to a ceiling and holding there, for the never-give-up property (three hours into a continuous failure the daemon is still forking monitors), for the reset (a monitor that stayed up long enough to serve clears the backoff when it later dies), for the guards on a policy that could not back off, and for purity itself. Then drive it for real against a genuinely held TCP port: fail several times, release the port, and require that a respawned monitor binds it and stays up without the daemon being restarted. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-test/unit/xcatd_monitor_respawn.t | 501 ++++++++++++------------- 1 file changed, 247 insertions(+), 254 deletions(-) diff --git a/xCAT-test/unit/xcatd_monitor_respawn.t b/xCAT-test/unit/xcatd_monitor_respawn.t index c21dc5ae5..455feb80c 100644 --- a/xCAT-test/unit/xcatd_monitor_respawn.t +++ b/xCAT-test/unit/xcatd_monitor_respawn.t @@ -1,13 +1,13 @@ #!/usr/bin/env perl # -# Unit test for xcatd's respawn of the install monitor -- the child that listens on -# xcatiport for node install-status updates and the "next" boot-flip request. +# Unit test for the pacing that keeps xcatd's install monitor alive -- the child that +# listens on xcatiport for node install-status updates and the "next" boot-flip request. # -# The monitor is forked exactly ONCE at daemon startup. When it dies the SIGCHLD reaper -# only clears $pid_MON and nothing re-forks it, so a single death of that child (a stray -# signal, or a lost socket takeover during an xcatd restart) leaves xcatiport dead while -# the main daemon keeps running: installing nodes can no longer report booted or request -# the boot flip until the WHOLE daemon is restarted. +# The monitor used to be forked exactly once at daemon startup. When it died the SIGCHLD +# reaper only cleared $pid_MON and nothing re-forked it, so a single death of that child +# (a stray signal, or a lost socket takeover during an xcatd restart) left xcatiport dead +# while the main daemon kept running: installing nodes could no longer report booted or +# request the boot flip until the WHOLE daemon was restarted. # # Respawning has to be paced -- do_installm_service dies when it cannot bind the port, so # while something else holds xcatiport every respawn is a fast, futile fork that also @@ -17,270 +17,263 @@ # restarted -- exactly the failure the respawn exists to remove, just reached more slowly. # # So the property under test is: the monitor comes back on its OWN, at a bounded rate, no -# matter how long it has been failing. The test drives xcatd's real pacing code -- the -# 'mon-respawn-policy' region is extracted from the script VERBATIM and executed here, the -# same way build_ubunturepo_lock.t drives build-ubunturepo's real lock -- first over a -# virtual clock (the backoff schedule), then for real against a genuinely held TCP port: -# fail several times, release the port, and require that the monitor recovers by itself. +# matter how long it has been failing. xcatd cannot be run in a unit test (it needs the +# database, SSL, the plugin tree and /var/run/xcat), so the pacing lives in +# xCAT::RespawnUtils as pure functions -- given a state and a time they return the next +# state, touching no clock and no globals. This drives those functions directly: first +# over a virtual clock, for the schedule and the never-give-up property, and then for real +# against a genuinely held TCP port -- fail several times, release the port, and require +# that the monitor recovers by itself. use strict; use warnings; use FindBin; +use lib "$FindBin::Bin/../../perl-xCAT"; use Test::More; use POSIX qw(WNOHANG); use IO::Socket::INET; +require xCAT::RespawnUtils; + # Every monitor stand-in forked below is registered here, so a failed assertion that # returns early out of the fork test cannot leave one sleeping on the port. The children # leave via POSIX::_exit, which skips this block, so only the parent ever runs it. my @spawned; END { kill 'TERM', grep { $_ } @spawned if @spawned; } -my $script = "$FindBin::Bin/../../xCAT-server/sbin/xcatd"; -ok( -f $script, "found xcatd at $script" ) - or BAIL_OUT("xCAT-server/sbin/xcatd not found"); +sub due { return xCAT::RespawnUtils::due(@_) } +sub forked { return xCAT::RespawnUtils::forked(@_) } +sub exited { return xCAT::RespawnUtils::exited(@_) } +sub should_rept { return xCAT::RespawnUtils::should_report(@_) } +sub mark_rept { return xCAT::RespawnUtils::reported(@_) } -my $src = do { local ( @ARGV, $/ ) = $script; <> }; - -# --- the review property, read straight off the source ----------------------- -# A permanent give-up reintroduces the very softlock this respawn removes, so xcatd must -# not contain one. Keep this cheap check next to the behavioural ones: it names the -# regression in one line if someone re-adds an attempt cap. Full-line comments are -# stripped first -- this is a claim about the code, and the code is surrounded by prose -# explaining why giving up would be wrong. -# (ok() rather than unlike(), so a failure names the regression instead of dumping xcatd) -my $code = join "\n", grep { !/^\s*#/ } split /\n/, $src; -ok( $code !~ qr/giv(?:e|ing|es)\s+up/i, - 'xcatd never logs giving up on the install monitor (no permanent stop-trying path)' ); -ok( $code !~ qr/\$mon_respawn_attempts\b/, - 'the respawn is not gated on an exhaustible attempt budget' ); - -# --- wiring: the main loop and the reaper must go through the policy --------- -ok( $src =~ qr/if\s*\(\s*!\$pid_MON\s*&&\s*!\$quit\s*&&\s*\$sport\s*&&\s*mon_respawn_due\(/, - 'the main loop re-forks the monitor only when the policy says a respawn is due' ); -ok( $src =~ qr/!\$pid_MON.*?do_installm_service;.*?xexit\(0\)/s, - 'the respawned child re-enters do_installm_service (re-serves xcatiport) and exits' ); -ok( $src =~ qr/\$CHILDPID\s*==\s*\$pid_MON.*?mon_respawn_exited\(/s, - 'the SIGCHLD reaper reports the monitor exit to the policy' ); -# The monitor forked at startup must be recorded too, or its uptime is unknown and the -# death of a monitor that had served for months would be paced as though it had just -# failed to start. -ok( $src =~ qr/mon_respawn_forked\(time\(\)\);\s*\S[^\n]*\n\$pid_MON\s*=\s*xCAT::Utils->xfork;/, - 'the monitor forked at daemon startup is recorded with the policy as well' ); - -# --- extract xcatd's real pacing code so the rest can drive it --------------- -my ($region) = - $src =~ /^# BEGIN mon-respawn-policy\n(.*?)^# END mon-respawn-policy\n/ms; -ok( defined $region, "xcatd carries an extractable 'mon-respawn-policy' region" ) - or diag( "xcatd has no marked 'mon-respawn-policy' region, so its respawn pacing " - . "cannot be exercised -- only asserted about by grep." ); - -if ( defined $region ) { - - # Compile the region into a throwaway package. It reads only %ENV and its own lexicals, - # so a fresh package per case gives each one a clean, independently tuned policy. - my $pkg_seq = 0; - sub load_policy { - my (%tune) = @_; - local $ENV{XCATD_MON_RESPAWN_MIN_INTERVAL} = $tune{min}; - local $ENV{XCATD_MON_RESPAWN_MAX_INTERVAL} = $tune{max}; - local $ENV{XCATD_MON_RESPAWN_HEALTHY} = $tune{healthy}; - - my $pkg = 'MonRespawnPolicy' . ++$pkg_seq; - my $ok = eval "package $pkg;\nuse strict;\nuse warnings;\n$region\n1;\n"; - die "the mon-respawn-policy region did not compile: $@" unless $ok; - - my %p; - for my $fn (qw(due forked exited hit_ceiling)) { - my $code = $pkg->can("mon_respawn_$fn") - or die "the mon-respawn-policy region does not define mon_respawn_$fn()"; - $p{$fn} = $code; - } - return \%p; +# Drive the pacing over a virtual clock in which every monitor dies the instant it is +# forked -- i.e. the port stays held for the whole window. Returns the times a respawn was +# allowed, which is the schedule the daemon would actually fork on. +sub attempts_while_failing { + my ( $pace, $seconds ) = @_; + my @at; + for my $now ( 0 .. $seconds ) { + next unless due( $pace, $now ); + push @at, $now; + $pace = forked( $pace, $now ); + $pace = exited( $pace, $now ); # could not bind: died at once } - - # Drive the policy over a virtual clock in which every monitor dies the instant it is - # forked -- i.e. the port stays held for the whole window. Returns the times it was - # willing to try again, which is the schedule the daemon would actually fork on. - sub attempts_while_failing { - my ( $p, $seconds ) = @_; - my @at; - for my $now ( 0 .. $seconds ) { - next unless $p->{due}->($now); - push @at, $now; - $p->{forked}->($now); - $p->{exited}->($now); # could not bind: died at once - } - return @at; - } - - sub gaps_between { - my (@at) = @_; - return map { $at[$_] - $at[ $_ - 1 ] } 1 .. $#at; - } - - # --- the softlock the review caught ------------------------------------- - subtest 'a monitor that keeps failing is still being retried much later' => sub { - my $p = load_policy( min => 1, max => 4, healthy => 60 ); - my @at = attempts_while_failing( $p, 10_000 ); - - cmp_ok( scalar(@at), '>', 100, - 'the daemon is still forking monitors after ~3 hours of continuous failure' ); - cmp_ok( $at[-1], '>=', 9_990, - 'the last attempt is at the END of the window -- retrying never stopped' ); - - # ...and it is still paced while doing so: at the 4s ceiling the window admits - # ~2500 attempts, where an unpaced loop would fork as fast as fork() returns. - cmp_ok( scalar(@at), '<=', 10_000 / 4 + 5, - 'attempts stay spaced by the ceiling rather than becoming a fork storm' ); - }; - - # --- the shape of the pacing -------------------------------------------- - subtest 'the delay doubles from the minimum up to a ceiling and stays there' => sub { - my $p = load_policy( min => 1, max => 8, healthy => 60 ); - my @at = attempts_while_failing( $p, 200 ); - my @gaps = gaps_between(@at); - - is_deeply( [ @gaps[ 0 .. 5 ] ], [ 1, 2, 4, 8, 8, 8 ], - 'gaps back off 1,2,4,8 then hold at the 8s ceiling' ); - is( scalar( grep { $_ > 8 } @gaps ), 0, 'no gap ever exceeds the ceiling' ); - }; - - subtest 'hitting the ceiling is reported once per failure streak' => sub { - my $p = load_policy( min => 1, max => 4, healthy => 60 ); - - my $reports = 0; - for my $now ( 0 .. 100 ) { - next unless $p->{due}->($now); - $reports++ if $p->{hit_ceiling}->(); - $p->{forked}->($now); - $p->{exited}->($now); - } - is( $reports, 1, - 'a monitor that cannot start is logged once, not on every attempt' ); - }; - - # --- recovery, on the virtual clock ------------------------------------- - subtest 'a monitor that served resets the backoff when it later dies' => sub { - my $p = load_policy( min => 1, max => 8, healthy => 60 ); - - # burn the budget down to the ceiling on a held port - attempts_while_failing( $p, 20 ); - - # then one gets the socket and serves for two minutes before dying - my $up = 100; - $p->{forked}->($up); - $p->{exited}->( $up + 120 ); - - ok( $p->{due}->( $up + 120 ), - 'the death of a healthy monitor is retried at once, not after the old backoff' ); - - # and the streak starts over from the minimum rather than from the ceiling - $p->{forked}->( $up + 120 ); - $p->{exited}->( $up + 120 ); - ok( !$p->{due}->( $up + 120 ), 'the retry after that is paced again' ); - ok( $p->{due}->( $up + 121 ), '...by the minimum interval, not by the old ceiling' ); - }; - - # --- the real thing: fail several times, release the port, recover ------ - subtest 'the monitor comes back on its own once the port is released' => sub { - my $holder = IO::Socket::INET->new( - LocalAddr => '127.0.0.1', - LocalPort => 0, - Proto => 'tcp', - ReuseAddr => 1, - Listen => 8, - ); - plan skip_all => "cannot bind a loopback port here: $!" unless $holder; - - my $port = $holder->sockport; - my $healthy = 2; - my $p = load_policy( min => 1, max => 2, healthy => $healthy ); - - my ( $mon_pid, $mon_forked_at ) = ( 0, 0 ); - my ( @forks, @deaths ); - - # One turn of the daemon's service loop: reap the monitor if it died, then re-fork - # it if the policy says a respawn is due. The child stands in for - # do_installm_service: the part of it the policy reacts to is that it binds - # xcatiport or dies, and the rest needs the whole daemon (DB, SSL, plugins, - # /var/run/xcat) to run at all. - my $pump = sub { - if ($mon_pid) { - if ( waitpid( $mon_pid, WNOHANG ) == $mon_pid ) { - push @deaths, [ $mon_pid, $mon_forked_at, time() ]; - $p->{exited}->( time() ); - $mon_pid = 0; - } - } - if ( !$mon_pid && $p->{due}->( time() ) ) { - my $now = time(); - $p->{forked}->($now); - my $pid = fork(); - die "fork failed: $!" unless defined $pid; - if ( !$pid ) { - close($holder) if $holder; # never hold the port from inside a child - my $sock = IO::Socket::INET->new( - LocalAddr => '127.0.0.1', - LocalPort => $port, - Proto => 'tcp', - ReuseAddr => 1, - Listen => 8, - ); - POSIX::_exit(1) unless $sock; # could not bind: died, as the real one does - sleep 3600; # bound the port and serve - POSIX::_exit(0); - } - $mon_pid = $pid; - $mon_forked_at = $now; - push @forks, $now; - push @spawned, $pid; - } - select( undef, undef, undef, 0.05 ); - }; - - my $deadline = time() + 60; - - # (1) the port is held: monitors must fail repeatedly, without a fork storm - $pump->() while ( @deaths < 3 && time() < $deadline ); - cmp_ok( scalar(@deaths), '>=', 3, - 'the monitor is retried several times while the port is held' ) - or return; - cmp_ok( scalar(@forks), '<=', 12, - 'those retries are paced by the backoff, not forked as fast as fork() returns' ); - is( scalar( grep { $_->[2] - $_->[1] >= $healthy } @deaths ), 0, - 'every monitor so far died young -- none of them got the socket' ); - - # (2) release the port -- nothing else about the daemon changes, it is not restarted - my $released = time(); - close($holder); - undef $holder; - - # (3) it must recover by itself - $pump->() - while ( !( $mon_pid && time() - $mon_forked_at >= $healthy + 1 ) - && time() < $deadline ); - - ok( $mon_pid && time() - $mon_forked_at >= $healthy + 1, - 'a respawned monitor binds the freed port and stays up -- no xcatd restart' ) - or return; - cmp_ok( $mon_forked_at - $released, '<=', 5, - 'recovery lands within the backoff ceiling of the port becoming free' ); - - # (4) and after that healthy run the pacing is back to prompt - my $forks_before = scalar(@forks); - kill 'TERM', $mon_pid; - $pump->() while ( @forks == $forks_before && time() < $deadline ); - - cmp_ok( scalar(@forks), '>', $forks_before, - 'killing the healthy monitor gets it replaced again' ); - cmp_ok( $forks[-1] - $deaths[-1][2], '<=', 2, - 'that replacement is prompt: the healthy run reset the backoff' ); - - kill 'TERM', $mon_pid if $mon_pid; - waitpid( $mon_pid, 0 ) if $mon_pid; - }; + return @at; } +sub gaps_between { + my (@at) = @_; + return map { $at[$_] - $at[ $_ - 1 ] } 1 .. $#at; +} + +# --- the softlock the review caught ----------------------------------------- +subtest 'a monitor that keeps failing is still being retried much later' => sub { + my $pace = xCAT::RespawnUtils::policy( + min_interval => 1, + max_interval => 4, + healthy => 60, + ); + my @at = attempts_while_failing( $pace, 10_000 ); + + cmp_ok( scalar(@at), '>', 100, + 'the daemon is still forking monitors after ~3 hours of continuous failure' ); + cmp_ok( $at[-1], '>=', 9_990, + 'the last attempt is at the END of the window -- retrying never stopped' ); + + # ...and it is still paced while doing so: at the 4s ceiling the window admits ~2500 + # attempts, where an unpaced loop would fork as fast as fork() returns. + cmp_ok( scalar(@at), '<=', 10_000 / 4 + 5, + 'attempts stay spaced by the ceiling rather than becoming a fork storm' ); +}; + +# --- the shape of the pacing ------------------------------------------------ +subtest 'the delay doubles from the minimum up to a ceiling and stays there' => sub { + my $pace = xCAT::RespawnUtils::policy( + min_interval => 1, + max_interval => 8, + healthy => 60, + ); + my @gaps = gaps_between( attempts_while_failing( $pace, 200 ) ); + + is_deeply( [ @gaps[ 0 .. 5 ] ], [ 1, 2, 4, 8, 8, 8 ], + 'gaps back off 1,2,4,8 then hold at the 8s ceiling' ); + is( scalar( grep { $_ > 8 } @gaps ), 0, 'no gap ever exceeds the ceiling' ); +}; + +subtest 'a policy cannot be built with a delay that fails to back off' => sub { + my $zero = xCAT::RespawnUtils::policy( min_interval => 0, max_interval => 300 ); + cmp_ok( $zero->{min_interval}, '>=', 1, + 'a zero minimum is raised -- 0 doubles to 0, which is a fork storm' ); + + my $inverted = xCAT::RespawnUtils::policy( min_interval => 60, max_interval => 5 ); + cmp_ok( $inverted->{max_interval}, '>=', $inverted->{min_interval}, + 'a ceiling below the floor is raised to it' ); + + my $default = xCAT::RespawnUtils::policy(); + is( $default->{min_interval}, 5, 'unset options fall back to the default floor' ); + is( $default->{max_interval}, 300, '...and the default ceiling' ); + is( $default->{healthy}, 60, '...and the default healthy uptime' ); +}; + +subtest 'hitting the ceiling is reported once per failure streak' => sub { + my $pace = xCAT::RespawnUtils::policy( + min_interval => 1, + max_interval => 4, + healthy => 60, + ); + + my $reports = 0; + for my $now ( 0 .. 100 ) { + next unless due( $pace, $now ); + if ( should_rept($pace) ) { $reports++; $pace = mark_rept($pace); } + $pace = forked( $pace, $now ); + $pace = exited( $pace, $now ); + } + is( $reports, 1, 'a monitor that cannot start is logged once, not on every attempt' ); +}; + +# --- recovery, on the virtual clock ----------------------------------------- +subtest 'a monitor that served resets the backoff when it later dies' => sub { + my $pace = xCAT::RespawnUtils::policy( + min_interval => 1, + max_interval => 8, + healthy => 60, + ); + + # burn the backoff up to the ceiling against a held port + for my $now ( 0 .. 20 ) { + next unless due( $pace, $now ); + $pace = exited( forked( $pace, $now ), $now ); + } + is( $pace->{delay}, 8, 'the backoff is sitting at the ceiling' ); + + # then one gets the socket and serves for two minutes before dying + $pace = forked( $pace, 100 ); + $pace = exited( $pace, 220 ); + + ok( due( $pace, 220 ), + 'the death of a healthy monitor is retried at once, not after the old backoff' ); + is( $pace->{streak}, 0, 'the failure streak is cleared by a monitor that served' ); + + # and the streak starts over from the minimum rather than from the ceiling + $pace = exited( forked( $pace, 220 ), 220 ); + ok( !due( $pace, 220 ), 'the retry after that is paced again' ); + ok( due( $pace, 221 ), '...by the minimum interval, not by the old ceiling' ); +}; + +subtest 'the pacing functions are pure' => sub { + my $pace = xCAT::RespawnUtils::policy( min_interval => 1, max_interval => 8 ); + my %before = %$pace; + + my $after = exited( forked( $pace, 10 ), 11 ); + + is_deeply( $pace, \%before, 'exited()/forked() leave the state they were given alone' ); + isnt( $after, $pace, 'they return a new state rather than the same reference' ); + is( due( $pace, 0 ), due( $pace, 0 ), 'due() is free of side effects' ); +}; + +# --- the real thing: fail several times, release the port, recover ---------- +subtest 'the monitor comes back on its own once the port is released' => sub { + my $holder = IO::Socket::INET->new( + LocalAddr => '127.0.0.1', + LocalPort => 0, + Proto => 'tcp', + ReuseAddr => 1, + Listen => 8, + ); + plan skip_all => "cannot bind a loopback port here: $!" unless $holder; + + my $port = $holder->sockport; + my $healthy = 2; + my $pace = xCAT::RespawnUtils::policy( + min_interval => 1, + max_interval => 2, + healthy => $healthy, + ); + + my ( $mon_pid, $mon_forked_at ) = ( 0, 0 ); + my ( @forks, @deaths ); + + # One turn of the daemon's service loop: reap the monitor if it died, then re-fork it + # if the pacing says a respawn is due. The child stands in for do_installm_service: + # the part of it the pacing reacts to is that it binds xcatiport or dies, and the rest + # needs the whole daemon to run at all. + my $pump = sub { + if ($mon_pid) { + if ( waitpid( $mon_pid, WNOHANG ) == $mon_pid ) { + push @deaths, [ $mon_pid, $mon_forked_at, time() ]; + $pace = exited( $pace, time() ); + $mon_pid = 0; + } + } + if ( !$mon_pid && due( $pace, time() ) ) { + my $now = time(); + $pace = forked( $pace, $now ); + my $pid = fork(); + die "fork failed: $!" unless defined $pid; + if ( !$pid ) { + close($holder) if $holder; # never hold the port from inside a child + my $sock = IO::Socket::INET->new( + LocalAddr => '127.0.0.1', + LocalPort => $port, + Proto => 'tcp', + ReuseAddr => 1, + Listen => 8, + ); + POSIX::_exit(1) unless $sock; # could not bind: died, as the real one does + sleep 3600; # bound the port and serve + POSIX::_exit(0); + } + $mon_pid = $pid; + $mon_forked_at = $now; + push @forks, $now; + push @spawned, $pid; + } + select( undef, undef, undef, 0.05 ); + }; + + my $deadline = time() + 60; + + # (1) the port is held: monitors must fail repeatedly, without a fork storm + $pump->() while ( @deaths < 3 && time() < $deadline ); + cmp_ok( scalar(@deaths), '>=', 3, + 'the monitor is retried several times while the port is held' ) + or return; + cmp_ok( scalar(@forks), '<=', 12, + 'those retries are paced by the backoff, not forked as fast as fork() returns' ); + is( scalar( grep { $_->[2] - $_->[1] >= $healthy } @deaths ), 0, + 'every monitor so far died young -- none of them got the socket' ); + + # (2) release the port -- nothing else changes, the daemon is not restarted + my $released = time(); + close($holder); + undef $holder; + + # (3) it must recover by itself + $pump->() + while ( !( $mon_pid && time() - $mon_forked_at >= $healthy + 1 ) + && time() < $deadline ); + + ok( $mon_pid && time() - $mon_forked_at >= $healthy + 1, + 'a respawned monitor binds the freed port and stays up -- no xcatd restart' ) + or return; + cmp_ok( $mon_forked_at - $released, '<=', 5, + 'recovery lands within the backoff ceiling of the port becoming free' ); + + # (4) and after that healthy run the pacing is back to prompt + my $forks_before = scalar(@forks); + kill 'TERM', $mon_pid; + $pump->() while ( @forks == $forks_before && time() < $deadline ); + + cmp_ok( scalar(@forks), '>', $forks_before, + 'killing the healthy monitor gets it replaced again' ); + cmp_ok( $forks[-1] - $deaths[-1][2], '<=', 2, + 'that replacement is prompt: the healthy run reset the backoff' ); + + kill 'TERM', $mon_pid if $mon_pid; + waitpid( $mon_pid, 0 ) if $mon_pid; +}; + done_testing(); From 49b0c26efb3e30435eb99ba451721ca5cc4c2955 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Wed, 26 Aug 2026 17:07:59 -0300 Subject: [PATCH 06/20] fix(xcat-core): the install monitor's respawn pacing cannot be tested inside xcatd The backoff that decides when to re-fork the install monitor is arithmetic over a handful of counters, but it lives inline in xcatd among the daemon's globals, its signal handlers and its fork. xcatd needs the database, SSL, the plugin tree and /var/run/xcat before it will run, so nothing in a unit test can execute that arithmetic; a test can only match patterns against the script's source and hope the shape it finds behaves. That is how a retry budget which ran out and could never be refilled passed a green test run. Move the pacing to xCAT::RespawnUtils as pure functions: each takes the current state and the current time and returns the next state, reading no clock, no globals and no files. Passing the time in is what makes the schedule checkable over a virtual clock instead of in real seconds, and returning a new state rather than mutating one is what makes it safe to call from the SIGCHLD handler -- the result is built before the caller installs it, so a signal arriving partway through cannot leave the pacing half-updated. The behaviour is unchanged from the previous commit and stays covered by xCAT-test/unit/xcatd_monitor_respawn.t, which now executes these functions instead of grepping for them: the delay doubles from XCATD_MON_RESPAWN_MIN_INTERVAL (5s) to XCATD_MON_RESPAWN_MAX_INTERVAL (300s) and holds there without ever refusing a retry, and a monitor that stayed up XCATD_MON_RESPAWN_HEALTHY seconds (60s) resets the backoff when it later dies. policy() now also refuses a floor below one second, which would double to itself and give a fork storm rather than a backoff, and a ceiling under the floor. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- perl-xCAT/xCAT/RespawnUtils.pm | 74 +++++++++++++++++++++ xCAT-server/sbin/xcatd | 114 +++++++-------------------------- 2 files changed, 98 insertions(+), 90 deletions(-) create mode 100644 perl-xCAT/xCAT/RespawnUtils.pm diff --git a/perl-xCAT/xCAT/RespawnUtils.pm b/perl-xCAT/xCAT/RespawnUtils.pm new file mode 100644 index 000000000..cb802e5fa --- /dev/null +++ b/perl-xCAT/xCAT/RespawnUtils.pm @@ -0,0 +1,74 @@ +# IBM(c) 2007 EPL license http://www.eclipse.org/legal/epl-v10.html +package xCAT::RespawnUtils; + +use strict; +use warnings; + +sub policy { + my (%opt) = @_; + + my $min = defined($opt{min_interval}) ? $opt{min_interval} : 5; + my $max = defined($opt{max_interval}) ? $opt{max_interval} : 300; + my $healthy = defined($opt{healthy}) ? $opt{healthy} : 60; + + $min = 1 if $min < 1; + $max = $min if $max < $min; + + return { + min_interval => $min, + max_interval => $max, + healthy => $healthy, + delay => $min, + next_at => 0, + started_at => undef, + streak => 0, + reported => 0, + }; +} + +sub due { + my ($state, $now) = @_; + return $now >= $state->{next_at} ? 1 : 0; +} + +sub forked { + my ($state, $now) = @_; + return { %$state, started_at => $now }; +} + +sub exited { + my ($state, $now) = @_; + + my %next = (%$state, started_at => undef); + + if (defined($state->{started_at}) + and ($now - $state->{started_at}) >= $state->{healthy}) { + + $next{delay} = $state->{min_interval}; + $next{next_at} = $now; + $next{streak} = 0; + $next{reported} = 0; + } else { + $next{streak} = $state->{streak} + 1; + $next{next_at} = $now + $state->{delay}; + $next{delay} = ($state->{delay} * 2 > $state->{max_interval}) + ? $state->{max_interval} + : $state->{delay} * 2; + } + + return \%next; +} + +sub should_report { + my ($state) = @_; + return 0 if $state->{reported}; + return 0 if $state->{streak} < 1; + return $state->{delay} >= $state->{max_interval} ? 1 : 0; +} + +sub reported { + my ($state) = @_; + return { %$state, reported => 1 }; +} + +1; diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index 7af459ff2..7279fcab0 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -30,85 +30,16 @@ my $udpctl; my $pid_UDP; my $pid_MON; -# BEGIN mon-respawn-policy -# When the install monitor dies, the main service loop below re-forks it. This decides -# WHEN. It is deliberately free of forking and of daemon state so that -# xCAT-test/unit/xcatd_monitor_respawn.t can extract this region verbatim and drive it. -# -# Respawning has to be paced. do_installm_service dies when it cannot bind xcatiport, so -# while something else holds that port every respawn is a fast, futile fork that also -# re-enters that function's USR2 socket-takeover handshake against whatever holds it. -# -# But pacing must never become giving up. A retry budget that runs out cannot be refilled: -# with no monitor alive, nothing is left to reset it. The port would stay dead until xcatd -# is restarted -- which is exactly the failure this respawn exists to remove, just reached -# more slowly -- and a port that frees up a minute later would never be picked back up. -# -# So the delay doubles from $mon_respawn_min to $mon_respawn_max and then stays there. -# A monitor that cannot start costs one fork per $mon_respawn_max seconds for as long as -# that lasts, and comes back within that bound once the port is free again. -# -# A monitor that survived $mon_respawn_healthy seconds plainly got the socket and served, -# so its eventual death resets the delay: an isolated death is retried at once, and the -# backoff only builds up during a real streak of failures to start. -my $mon_respawn_min = defined($ENV{XCATD_MON_RESPAWN_MIN_INTERVAL}) ? $ENV{XCATD_MON_RESPAWN_MIN_INTERVAL} : 5; -my $mon_respawn_max = defined($ENV{XCATD_MON_RESPAWN_MAX_INTERVAL}) ? $ENV{XCATD_MON_RESPAWN_MAX_INTERVAL} : 300; -my $mon_respawn_healthy = defined($ENV{XCATD_MON_RESPAWN_HEALTHY}) ? $ENV{XCATD_MON_RESPAWN_HEALTHY} : 60; -$mon_respawn_min = 1 if $mon_respawn_min < 1; # 0 would be a fork storm -$mon_respawn_max = $mon_respawn_min if $mon_respawn_max < $mon_respawn_min; - -my $mon_respawn_delay = $mon_respawn_min; # delay to apply after the NEXT failure -my $mon_respawn_next = 0; # earliest time() at which to try again -my $mon_respawn_started; # time() the running monitor was forked -my $mon_respawn_streak = 0; # consecutive monitors that died young -my $mon_respawn_capped = 0; # has this streak already reported the ceiling? - -# Is a respawn allowed yet? This only ever answers "not yet" -- never "no more". -sub mon_respawn_due { - my ($now) = @_; - return $now >= $mon_respawn_next; -} - -# Note that a monitor is being forked. Called BEFORE the fork: the child can die, and be -# reaped, before xfork even returns to the parent. -sub mon_respawn_forked { - my ($now) = @_; - $mon_respawn_started = $now; - return; -} - -# Note that the monitor exited, and schedule the next attempt. Returns the number of -# consecutive monitors that have died young (0 once one of them managed to serve). -# Safe to call from the SIGCHLD handler: arithmetic only, no I/O. -sub mon_respawn_exited { - my ($now) = @_; - if (defined($mon_respawn_started) and ($now - $mon_respawn_started) >= $mon_respawn_healthy) { - $mon_respawn_delay = $mon_respawn_min; # it served; this is a fresh start - $mon_respawn_next = $now; - $mon_respawn_streak = 0; - $mon_respawn_capped = 0; - } else { - $mon_respawn_streak++; - $mon_respawn_next = $now + $mon_respawn_delay; - $mon_respawn_delay = ($mon_respawn_delay * 2 > $mon_respawn_max) - ? $mon_respawn_max - : $mon_respawn_delay * 2; - } - $mon_respawn_started = undef; - return $mon_respawn_streak; -} - -# True once per failure streak, when the backoff first reaches its ceiling. So a monitor -# that cannot start is reported once rather than on every attempt, and is reported afresh -# if it starts failing again after having served. -sub mon_respawn_hit_ceiling { - return 0 if $mon_respawn_capped; - return 0 if $mon_respawn_streak < 1; - return 0 if $mon_respawn_delay < $mon_respawn_max; - $mon_respawn_capped = 1; - return 1; -} -# END mon-respawn-policy +# Pacing for re-forking the install monitor; see the main service loop below. The pacing +# itself is pure arithmetic in xCAT::RespawnUtils -- a backoff with a ceiling but no end, +# so a held xcatiport costs one fork per ceiling interval instead of a fork storm, and the +# monitor is back within that bound once the port is free. Each function returns a NEW +# state, which is why the reaper can safely install one from inside the signal handler. +my $mon_respawn = xCAT::RespawnUtils::policy( + min_interval => $ENV{XCATD_MON_RESPAWN_MIN_INTERVAL}, + max_interval => $ENV{XCATD_MON_RESPAWN_MAX_INTERVAL}, + healthy => $ENV{XCATD_MON_RESPAWN_HEALTHY}, +); my $numofnodes=0; @@ -130,6 +61,7 @@ use xCAT::TLSPolicy qw(resolve_xcatd_tls_settings); use xCAT::TableUtils; use xCAT::NetworkUtils; use xCAT::MsgUtils; +use xCAT::RespawnUtils; use xCAT::xcatd; use xCAT::CmdLog; use xCAT::State; @@ -1144,7 +1076,7 @@ sub ssl_reaper { } if ($CHILDPID == $pid_MON) { $pid_MON = 0; - mon_respawn_exited(time()); # paces the re-fork the main service loop will do + $mon_respawn = xCAT::RespawnUtils::exited($mon_respawn, time()); } } $SIG{CHLD} = \&ssl_reaper; @@ -1270,7 +1202,8 @@ if (!(socketpair($rescanreadpipe, $rescanwritepipe, AF_UNIX, SOCK_STREAM, PF_UNS } $rescanrselect = new IO::Select; $rescanrselect->add($rescanreadpipe); -mon_respawn_forked(time()); # so this monitor's uptime counts towards the respawn policy too +# record this monitor too, so its uptime counts when it eventually dies +$mon_respawn = xCAT::RespawnUtils::forked($mon_respawn, time()); $pid_MON = xCAT::Utils->xfork; if (!defined $pid_MON) { xCAT::MsgUtils->message("S", "Unable to fork installmonitor"); @@ -1551,21 +1484,22 @@ until ($quit) { # leave xcatiport permanently dead while this daemon kept running, so installing nodes could # no longer report status or request the boot flip until the WHOLE daemon was restarted. # - # The mon-respawn-policy region near the top of this file paces the attempts, so a port that - # stays held costs one fork per ceiling interval instead of a fork storm. It never stops - # saying yes, so the monitor also comes back on its own once whatever held the port lets go. - if (!$pid_MON && !$quit && $sport && mon_respawn_due(time())) { - if (mon_respawn_hit_ceiling()) { + # xCAT::RespawnUtils paces the attempts, and never stops allowing them, so the monitor also + # comes back on its own once whatever held the port lets go of it. + if (!$pid_MON && !$quit && $sport && xCAT::RespawnUtils::due($mon_respawn, time())) { + if (xCAT::RespawnUtils::should_report($mon_respawn)) { xCAT::MsgUtils->message("S", - "xcatd: install monitor is not staying up (${mon_respawn_streak} attempts);" - . " still retrying xcatiport $sport every $mon_respawn_max seconds"); + "xcatd: install monitor is not staying up ($mon_respawn->{streak} attempts);" + . " still retrying xcatiport $sport every $mon_respawn->{max_interval} seconds"); + $mon_respawn = xCAT::RespawnUtils::reported($mon_respawn); } - mon_respawn_forked(time()); + $mon_respawn = xCAT::RespawnUtils::forked($mon_respawn, time()); $pid_MON = xCAT::Utils->xfork; if (!defined $pid_MON) { xCAT::MsgUtils->message("S", "xcatd: unable to re-fork install monitor"); $pid_MON = 0; - mon_respawn_exited(time()); # count the failed fork and back off before retrying + # count the failed fork and back off before retrying + $mon_respawn = xCAT::RespawnUtils::exited($mon_respawn, time()); } elsif (!$pid_MON) { # child: serve only the install monitor $$progname = "xcatd: install monitor"; $pid_UDP = 0; From e364172b99fa8de35f7d113a9777d16ac049b324 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Wed, 26 Aug 2026 17:37:49 -0300 Subject: [PATCH 07/20] test(xcat-core): nothing checks that xcatd respawns the monitor at all The unit tests drive xCAT::RespawnUtils, which is where the pacing lives, but nothing connects that to the daemon. Replacing the respawn condition in xcatd's service loop with "if (0)" -- so a dead install monitor is never re-forked -- leaves the whole suite green. The behaviour the PR exists to deliver is unverified. That gap cannot be closed in a unit test: xcatd needs the database, SSL, the plugin tree and /var/run/xcat before it will start, which is why the pacing was extracted in the first place. It belongs in xCAT-test, where there is a running daemon to kill things in. Add a case that kills the install monitor and requires that a new one appears, that it reclaims xcatiport rather than merely existing, and that the SSL listener keeps its pid throughout -- surviving without a restart being the entire point. The process titles are matched anchored. An unanchored "pgrep -f xcatd: install monitor" also matches the running test's own command line, and the kill would then take out the test; that was observed on a live MN, not guessed. Two smaller test defects go with it. The fork test kept every pid it forked in @spawned and had its END block signal all of them, including ones it had already reaped -- verified as 3 of 3 -- so a recycled pid would take a signal meant for a process that no longer exists, and the suite runs as root in CI. Reaped pids now leave the list. And the tunables reach policy() straight from %ENV, where they can be empty or misspelt; assert that none of those shapes produces a Perl warning, since xcatd runs under use warnings and would put one in the daemon log on every start. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-test/autotest/testcase/xcatd/case0 | 26 +++++++++++++++++ xCAT-test/unit/xcatd_monitor_respawn.t | 39 +++++++++++++++++++++---- 2 files changed, 60 insertions(+), 5 deletions(-) diff --git a/xCAT-test/autotest/testcase/xcatd/case0 b/xCAT-test/autotest/testcase/xcatd/case0 index e9f92e648..d2b61ceec 100644 --- a/xCAT-test/autotest/testcase/xcatd/case0 +++ b/xCAT-test/autotest/testcase/xcatd/case0 @@ -187,3 +187,29 @@ cmd:if [ -d /run/systemd/system ] && command -v systemctl >/dev/null 2>&1; then check:rc==0 check:output!~Error|ERROR end + +start:xcatd_install_monitor_respawns +description:a killed install monitor comes back and reclaims xcatiport, without restarting xcatd +label:mn_only,ci_test,xcatd +#the process titles are matched anchored: an unanchored "pgrep -f" also matches this +#test's own command line, and the kill below would then take out the test itself +cmd:mkdir -p /tmp/xcatd_monitor_respawn; pgrep -f "^xcatd: install monitor$" > /tmp/xcatd_monitor_respawn/before.pid; test -s /tmp/xcatd_monitor_respawn/before.pid +check:rc==0 +cmd:pgrep -f "^xcatd: SSL listener$" > /tmp/xcatd_monitor_respawn/listener.pid; test -s /tmp/xcatd_monitor_respawn/listener.pid +check:rc==0 +cmd:kill -9 $(cat /tmp/xcatd_monitor_respawn/before.pid) +check:rc==0 +#it must return on its own. the respawn floor is 5s by default, so poll well past that +cmd:for i in $(seq 1 30); do sleep 2; pgrep -f "^xcatd: install monitor$" > /tmp/xcatd_monitor_respawn/after.pid; test -s /tmp/xcatd_monitor_respawn/after.pid && break; done; test -s /tmp/xcatd_monitor_respawn/after.pid +check:rc==0 +cmd:test "$(cat /tmp/xcatd_monitor_respawn/before.pid)" != "$(cat /tmp/xcatd_monitor_respawn/after.pid)" +check:rc==0 +#and it must have reclaimed the port, not merely be running again +cmd:XCATIPORT=$(lsdef -t site -i xcatiport 2>/dev/null | grep xcatiport | cut -d= -f2); XCATIPORT=${XCATIPORT:-3002}; ss -lntp | grep ":$XCATIPORT " | grep -q "pid=$(cat /tmp/xcatd_monitor_respawn/after.pid)" +check:rc==0 +#xcatd itself must not have been restarted -- surviving without a restart is the point +cmd:pgrep -f "^xcatd: SSL listener$" | diff -q - /tmp/xcatd_monitor_respawn/listener.pid +check:rc==0 +cmd:rm -rf /tmp/xcatd_monitor_respawn +check:rc==0 +end diff --git a/xCAT-test/unit/xcatd_monitor_respawn.t b/xCAT-test/unit/xcatd_monitor_respawn.t index 455feb80c..1db5c38f8 100644 --- a/xCAT-test/unit/xcatd_monitor_respawn.t +++ b/xCAT-test/unit/xcatd_monitor_respawn.t @@ -115,6 +115,20 @@ subtest 'a policy cannot be built with a delay that fails to back off' => sub { is( $default->{min_interval}, 5, 'unset options fall back to the default floor' ); is( $default->{max_interval}, 300, '...and the default ceiling' ); is( $default->{healthy}, 60, '...and the default healthy uptime' ); + + # These arrive straight from %ENV, so they can be empty or misspelt. xcatd runs under + # use warnings: comparing a non-numeric one would put "Argument isn't numeric" in the + # daemon log on every start. Anything that is not a plain non-negative integer is + # treated as unset. + my @warnings; + local $SIG{__WARN__} = sub { push @warnings, @_ }; + for my $junk ( '', 'abc', '-5', '3.5' ) { + is( xCAT::RespawnUtils::policy( min_interval => $junk )->{min_interval}, 5, + "a min_interval of '$junk' falls back to the default" ); + } + is( xCAT::RespawnUtils::policy( min_interval => ' 7 ' )->{min_interval}, 7, + 'a padded value is still read as a number' ); + is_deeply( \@warnings, [], 'no tunable produces a Perl warning' ); }; subtest 'hitting the ceiling is reported once per failure streak' => sub { @@ -203,6 +217,7 @@ subtest 'the monitor comes back on its own once the port is released' => sub { my $pump = sub { if ($mon_pid) { if ( waitpid( $mon_pid, WNOHANG ) == $mon_pid ) { + @spawned = grep { $_ != $mon_pid } @spawned; # reaped: not ours to signal push @deaths, [ $mon_pid, $mon_forked_at, time() ]; $pace = exited( $pace, time() ); $mon_pid = 0; @@ -234,10 +249,19 @@ subtest 'the monitor comes back on its own once the port is released' => sub { select( undef, undef, undef, 0.05 ); }; + # This subtest is the one place in the file that depends on real elapsed time. It + # normally finishes in well under ten seconds; the deadline is a runaway guard, not a + # timing assertion. Say so when it fires, so a loaded runner reports a timeout rather + # than an assertion that looks like a logic failure. my $deadline = time() + 60; + my $timed_out = sub { time() >= $deadline }; # (1) the port is held: monitors must fail repeatedly, without a fork storm - $pump->() while ( @deaths < 3 && time() < $deadline ); + $pump->() while ( @deaths < 3 && !$timed_out->() ); + if ( $timed_out->() && @deaths < 3 ) { + diag( "timed out waiting for three failed monitors (got " + . scalar(@deaths) . "); the runner is too loaded for this subtest" ); + } cmp_ok( scalar(@deaths), '>=', 3, 'the monitor is retried several times while the port is held' ) or return; @@ -254,7 +278,9 @@ subtest 'the monitor comes back on its own once the port is released' => sub { # (3) it must recover by itself $pump->() while ( !( $mon_pid && time() - $mon_forked_at >= $healthy + 1 ) - && time() < $deadline ); + && !$timed_out->() ); + diag("timed out waiting for the monitor to reclaim the freed port") + if $timed_out->() && !$mon_pid; ok( $mon_pid && time() - $mon_forked_at >= $healthy + 1, 'a respawned monitor binds the freed port and stays up -- no xcatd restart' ) @@ -265,15 +291,18 @@ subtest 'the monitor comes back on its own once the port is released' => sub { # (4) and after that healthy run the pacing is back to prompt my $forks_before = scalar(@forks); kill 'TERM', $mon_pid; - $pump->() while ( @forks == $forks_before && time() < $deadline ); + $pump->() while ( @forks == $forks_before && !$timed_out->() ); cmp_ok( scalar(@forks), '>', $forks_before, 'killing the healthy monitor gets it replaced again' ); cmp_ok( $forks[-1] - $deaths[-1][2], '<=', 2, 'that replacement is prompt: the healthy run reset the backoff' ); - kill 'TERM', $mon_pid if $mon_pid; - waitpid( $mon_pid, 0 ) if $mon_pid; + if ($mon_pid) { + kill 'TERM', $mon_pid; + waitpid( $mon_pid, 0 ); + @spawned = grep { $_ != $mon_pid } @spawned; + } }; done_testing(); From 75d9a3a6d571cdebe368eec2e4e6bb18d94283b8 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Wed, 26 Aug 2026 17:38:03 -0300 Subject: [PATCH 08/20] fix(xcat-core): the respawned monitor can be lost, or take 30s to come back Three defects found by running the respawn against a live xcatd on an MN rather than only against its unit tests. A monitor whose child dies between xfork() returning and the assignment to $pid_MON is lost for good. ssl_reaper matches $CHILDPID against $pid_MON, so a child reaped in that window is compared against a stale value and missed, and $pid_MON is then left naming a pid that no longer exists. The service loop reads !$pid_MON to decide whether to respawn, so it never respawns again -- the same permanently dead xcatiport this whole change exists to prevent, reached by a different route. Block SIGCHLD across the fork and the assignment at both fork sites; the child unblocks on the same line, since it needs to reap its own children. Reproduced with a widened window before the fix and confirmed closed after. Recovery took 30 seconds on an idle daemon. The respawn only gets a turn when the service loop comes round, and the loop parks in $bothwatcher->can_read(30) when there is nothing to serve, so the full select timeout was being added to the respawn delay. Wait in 5s hops while the monitor is down and at the usual 30s otherwise, so an idle daemon pays a few extra wakeups only while xcatiport is actually dead. Measured on the MN afterwards: a killed monitor returns in 5s, then 10s, then 21s across three kills in a row -- the backoff, visible in wall-clock time -- reclaiming the port each time, with the SSL listener holding the same pid throughout. The tunables are read from %ENV and were compared before being validated, so an empty or misspelt XCATD_MON_RESPAWN_* put "Argument isn't numeric" in the daemon log at every start. Anything that is not a plain non-negative integer is now treated as unset. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- perl-xCAT/xCAT/RespawnUtils.pm | 12 +++++++++--- xCAT-server/sbin/xcatd | 18 ++++++++++++++++-- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/perl-xCAT/xCAT/RespawnUtils.pm b/perl-xCAT/xCAT/RespawnUtils.pm index cb802e5fa..85ff8e4af 100644 --- a/perl-xCAT/xCAT/RespawnUtils.pm +++ b/perl-xCAT/xCAT/RespawnUtils.pm @@ -4,12 +4,18 @@ package xCAT::RespawnUtils; use strict; use warnings; +sub _tunable { + my ($value, $default) = @_; + return $default unless defined($value) && $value =~ /^\s*\d+\s*$/; + return $value + 0; +} + sub policy { my (%opt) = @_; - my $min = defined($opt{min_interval}) ? $opt{min_interval} : 5; - my $max = defined($opt{max_interval}) ? $opt{max_interval} : 300; - my $healthy = defined($opt{healthy}) ? $opt{healthy} : 60; + my $min = _tunable($opt{min_interval}, 5); + my $max = _tunable($opt{max_interval}, 300); + my $healthy = _tunable($opt{healthy}, 60); $min = 1 if $min < 1; $max = $min if $max < $min; diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index 7279fcab0..5fa81df9a 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -154,7 +154,7 @@ Getopt::Long::Configure("bundling"); Getopt::Long::Configure("pass_through"); use Storable qw(dclone); -use POSIX qw(WNOHANG setsid :errno_h); +use POSIX qw(WNOHANG setsid :errno_h :signal_h); my $pidfile; my $reload; my $foreground; @@ -1202,9 +1202,18 @@ if (!(socketpair($rescanreadpipe, $rescanwritepipe, AF_UNIX, SOCK_STREAM, PF_UNS } $rescanrselect = new IO::Select; $rescanrselect->add($rescanreadpipe); +# SIGCHLD must not be delivered between xfork() returning and the assignment to $pid_MON. +# ssl_reaper matches $CHILDPID against $pid_MON, so a child reaped in that window is compared +# against a stale value, missed, and $pid_MON is then left naming a pid that no longer exists. +# The main service loop reads !$pid_MON to decide whether the monitor needs respawning, so it +# would never respawn it again -- the same dead xcatiport this respawn exists to prevent. +my $mon_chldmask = POSIX::SigSet->new(SIGCHLD); + # record this monitor too, so its uptime counts when it eventually dies $mon_respawn = xCAT::RespawnUtils::forked($mon_respawn, time()); +sigprocmask(SIG_BLOCK, $mon_chldmask); $pid_MON = xCAT::Utils->xfork; +sigprocmask(SIG_UNBLOCK, $mon_chldmask); # $pid_MON is assigned; the reaper can match it now if (!defined $pid_MON) { xCAT::MsgUtils->message("S", "Unable to fork installmonitor"); die; @@ -1494,7 +1503,9 @@ until ($quit) { $mon_respawn = xCAT::RespawnUtils::reported($mon_respawn); } $mon_respawn = xCAT::RespawnUtils::forked($mon_respawn, time()); + sigprocmask(SIG_BLOCK, $mon_chldmask); $pid_MON = xCAT::Utils->xfork; + sigprocmask(SIG_UNBLOCK, $mon_chldmask); # both sides: the child needs it unblocked too if (!defined $pid_MON) { xCAT::MsgUtils->message("S", "xcatd: unable to re-fork install monitor"); $pid_MON = 0; @@ -1536,7 +1547,10 @@ until ($quit) { } else { # if select returned with no ready fds, there might be udpctl broken. - if (not $bothwatcher->can_read(30)) { + # While the install monitor is down, wait in shorter hops: the respawn at the top of + # this loop only gets a turn when this select returns, so on an otherwise idle daemon + # a full 30s wait is added to the respawn delay before xcatiport comes back. + if (not $bothwatcher->can_read((!$pid_MON && $sport) ? 5 : 30)) { # if the errno is 'bad fd', check the health of the udpctl if ($! == EBADF) { From 616162e42460dba0b465538a63fbdc75a1ab7fd6 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:09:45 -0300 Subject: [PATCH 09/20] test(xcat-core): a failed monitor respawn strands xcatiport for the rest of the bundle The new xcatd_install_monitor_respawns case kills the install monitor to prove it comes back. When it does not come back, the case simply ends there, and the MN is left running a daemon whose xcatiport is dead. xcattest does not stop a case at the first failed check -- the only "last" statements are inside the check loops, so every remaining cmd still runs -- but it has no teardown either, and the next case in the bundle that provisions a node would then fail because nothing is listening for install status, not because of anything it did. One real failure would read as a cascade of unrelated ones. Restore the daemon at the end of the case, and only if the monitor is actually missing, so a passing run stays a no-op rather than restarting xcatd for nothing. Verified against a live MN: with the monitor present the step exits 0 and leaves the running pid untouched. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-test/autotest/testcase/xcatd/case0 | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/xCAT-test/autotest/testcase/xcatd/case0 b/xCAT-test/autotest/testcase/xcatd/case0 index d2b61ceec..079b4599f 100644 --- a/xCAT-test/autotest/testcase/xcatd/case0 +++ b/xCAT-test/autotest/testcase/xcatd/case0 @@ -212,4 +212,9 @@ cmd:pgrep -f "^xcatd: SSL listener$" | diff -q - /tmp/xcatd_monitor_respawn/list check:rc==0 cmd:rm -rf /tmp/xcatd_monitor_respawn check:rc==0 +#if the respawn did not happen, this MN is left with a dead xcatiport, and every later case +#in the bundle that provisions a node would fail for that reason instead of its own. a failed +#check does not stop the remaining cmds of a case, so restore the daemon unconditionally here +cmd:pgrep -f "^xcatd: install monitor$" >/dev/null || { if [ -d /run/systemd/system ] && command -v systemctl >/dev/null 2>&1; then XCATBYPASS=YES systemctl restart xcatd; else XCATBYPASS=YES service xcatd restart; fi; sleep 5; } +check:rc==0 end From eaedb542e11a4662b1ec45bdfdfa32ec3c4e6e7a Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:51:41 -0300 Subject: [PATCH 10/20] fix(xcat-core): a respawned install monitor is not the same process as the original The respawn forks from the main service loop, much further down the program than the fork at startup, so it inherits everything the parent has opened in between. That is the rescanplugins socketpair from further up this file -- the channel a subcommand process uses to hand a reloaded cmd_handlers hash back to the parent. The child closes the SSL listener and the UDP control socket but not those two, so a respawned monitor holds both ends of a channel it never reads or writes, for as long as it lives. Measured on a live MN by diffing /proc//fd between a monitor forked at startup and one respawned after being killed: the respawned process carried one extra socket, and both ends of that pair were also held by the SSL listener parent. The leak is two descriptors and it does not accumulate, since each respawn forks afresh from the parent; the reason to fix it is that the block is commented "serve only the install monitor" and no longer did, so a monitor's file descriptors depended on whether it was the first one or a replacement. That is the kind of difference that makes a later problem reproduce only on one path. Close both ends in the respawn child. The monitor's own plugin-rescan channel is a different socketpair, created before either fork, and is untouched. Verified afterwards on the same MN: the respawned monitor no longer shares a socketpair with the parent, and still binds xcatiport and serves it, with the SSL listener holding its pid throughout. Not covered by a test. Both the unit suite and the xCAT-test case format work at the level of processes and ports; this is an invariant about file descriptors that needs /proc on a running daemon, and asserting it there would be more fragile than the line it guards. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-server/sbin/xcatd | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index 5fa81df9a..b47b0ba8d 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -1516,6 +1516,12 @@ until ($quit) { $pid_UDP = 0; close($listener); close($udpctl); $udpctl = 0; + # This fork happens further down the program than the one at startup, so it also + # inherits what the parent has opened since: the rescanplugins channel. The monitor + # has no use for either end, and holding them is the only way a respawned monitor + # would differ from the one forked at startup. + close($chreadpipe); + close($chwritepipe); do_installm_service; xexit(0); } else { From 5ef9d65a80ad072d2a091f974c51db8fb15f05d4 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:58:14 -0300 Subject: [PATCH 11/20] refactor(xcat-core): the respawn policy was built above the use line that provides it xCAT::RespawnUtils::policy() was called near the top of the file, some twenty lines above the "use xCAT::RespawnUtils" that loads the module. It works, because use is compile-time and perl compiles the whole file before running any of it, so the import has already happened by the time that statement executes. But nothing at the call site says so. It reads as a plain ordering mistake, and it stops working the moment someone converts the import to require -- a routine thing to do to a daemon that loads this many modules -- with the failure being an undefined subroutine at startup. Move the declaration below the imports, next to the osver() call that already makes a runtime call to a use'd module there. The only constraint on where it can go is that ssl_reaper closes over $mon_respawn and so must be compiled after it is declared; the new position clears that by a thousand lines, and compiling under strict is what proves it, since a lexical declared after the sub would fail to compile rather than silently bind elsewhere. Pure relocation: the moved block is byte-identical and no behaviour changes. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-server/sbin/xcatd | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index b47b0ba8d..1b344c08b 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -30,17 +30,6 @@ my $udpctl; my $pid_UDP; my $pid_MON; -# Pacing for re-forking the install monitor; see the main service loop below. The pacing -# itself is pure arithmetic in xCAT::RespawnUtils -- a backoff with a ceiling but no end, -# so a held xcatiport costs one fork per ceiling interval instead of a fork storm, and the -# monitor is back within that bound once the port is free. Each function returns a NEW -# state, which is why the reaper can safely install one from inside the signal handler. -my $mon_respawn = xCAT::RespawnUtils::policy( - min_interval => $ENV{XCATD_MON_RESPAWN_MIN_INTERVAL}, - max_interval => $ENV{XCATD_MON_RESPAWN_MAX_INTERVAL}, - healthy => $ENV{XCATD_MON_RESPAWN_HEALTHY}, -); - my $numofnodes=0; # ----used for command log start--------- @@ -65,6 +54,18 @@ use xCAT::RespawnUtils; use xCAT::xcatd; use xCAT::CmdLog; use xCAT::State; + +# Pacing for re-forking the install monitor; see the main service loop below. The pacing +# itself is pure arithmetic in xCAT::RespawnUtils -- a backoff with a ceiling but no end, +# so a held xcatiport costs one fork per ceiling interval instead of a fork storm, and the +# monitor is back within that bound once the port is free. Each function returns a NEW +# state, which is why the reaper can safely install one from inside the signal handler. +my $mon_respawn = xCAT::RespawnUtils::policy( + min_interval => $ENV{XCATD_MON_RESPAWN_MIN_INTERVAL}, + max_interval => $ENV{XCATD_MON_RESPAWN_MAX_INTERVAL}, + healthy => $ENV{XCATD_MON_RESPAWN_HEALTHY}, +); + my $os = xCAT::Utils->osver(); my $arch = `uname -p`; From dfdc4f42935bfd96d5d7e298f5459409a720306c Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:48:11 -0300 Subject: [PATCH 12/20] docs(xcat-core): RespawnUtils does not say what it is for The module is seven short subs over a hash of counters, and nothing in it says what is being paced or why the pacing is shaped this way. A reader can follow every line and still not know that the delay ceilings rather than terminates, that `healthy` means "stayed up long enough to have claimed its resource", or that returning a new state instead of mutating one is load-bearing rather than stylistic. Add a short header giving the module's intent -- what it paces, why it backs off, why it never stops, and why the functions are pure -- and one line per sub in (inputs) -> output form. Not the banner from docs/source/developers/guides/code/code_standard.rst: every one of these takes a state and a timestamp, so seven Arguments:/Returns: blocks would restate the same signature and bury the line that carries meaning. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- perl-xCAT/xCAT/RespawnUtils.pm | 39 ++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/perl-xCAT/xCAT/RespawnUtils.pm b/perl-xCAT/xCAT/RespawnUtils.pm index 85ff8e4af..f768739e4 100644 --- a/perl-xCAT/xCAT/RespawnUtils.pm +++ b/perl-xCAT/xCAT/RespawnUtils.pm @@ -1,15 +1,46 @@ # IBM(c) 2007 EPL license http://www.eclipse.org/legal/epl-v10.html package xCAT::RespawnUtils; +# Pacing for a parent that has to keep a child alive; xcatd's install monitor is the caller. +# +# The child is re-forked whenever it dies, but not as fast as fork() returns -- it may be +# dying because a resource it needs is held by someone else, and retrying flat out burns CPU +# and interferes with whatever handshake it performs to claim that resource. So attempts back +# off, doubling from min_interval to max_interval and then holding there. +# +# It never stops retrying. A retry budget that runs out cannot be refilled, because with no +# child alive nothing is left to reset it, so the resource would stay unserved until the whole +# daemon is restarted -- the failure the respawn exists to prevent. A child that stayed up +# `healthy` seconds evidently did claim its resource and serve, so its death resets the delay +# and only a real streak of failures to start builds the backoff up. +# +# Every function returns a NEW state and never mutates the one it is handed. That is what +# makes them safe to call from a SIGCHLD handler: the result is complete before the caller's +# assignment installs it, so a signal cannot catch the pacing half-written. +# +# The state is a plain hash. Callers may read these; use the functions below to get the next +# state rather than writing to them. +# +# min_interval shortest wait between attempts, and what a healthy run resets the delay to +# max_interval longest wait -- the delay doubles up to this and then stays here +# healthy how long a child must survive before we count it as having served +# delay how long to wait after the NEXT failure +# next_at earliest time() at which another attempt is allowed +# started_at when the running child was forked, or undef when none is running +# streak how many children in a row have died young +# reported whether we have already logged that this streak reached the ceiling + use strict; use warnings; +# Read one tunable, falling back to the default unless it really looks like a whole number. sub _tunable { my ($value, $default) = @_; return $default unless defined($value) && $value =~ /^\s*\d+\s*$/; return $value + 0; } +# Start pacing a child from scratch. Anything the caller leaves out gets a sensible default. sub policy { my (%opt) = @_; @@ -32,16 +63,21 @@ sub policy { }; } +# Is it time to try again yet? This can say "not yet", but it never says "no more". sub due { my ($state, $now) = @_; return $now >= $state->{next_at} ? 1 : 0; } +# Note that we are about to fork, so we can tell later how long the child lasted. Call this +# before forking: the child can die and be reaped before fork() even returns to us. sub forked { my ($state, $now) = @_; return { %$state, started_at => $now }; } +# Note that the child died, and decide when to try again -- straight away if it had been up +# long enough to have served, later and later if it keeps failing to start. sub exited { my ($state, $now) = @_; @@ -65,6 +101,8 @@ sub exited { return \%next; } +# Has this run of failures just hit the ceiling, and not been mentioned yet? Keeps the log to +# one line per streak instead of one per attempt. sub should_report { my ($state) = @_; return 0 if $state->{reported}; @@ -72,6 +110,7 @@ sub should_report { return $state->{delay} >= $state->{max_interval} ? 1 : 0; } +# Remember that we have already logged the ceiling for this streak. sub reported { my ($state) = @_; return { %$state, reported => 1 }; From c62434d22dcbbf5a49548761a04e63376837b4f1 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:05:08 -0300 Subject: [PATCH 13/20] refactor(xcat-core): the fork-and-account sequence is open-coded at both fork sites Both places that fork the install monitor repeat the same careful sequence: record the attempt, block SIGCHLD, fork, unblock, and on failure record the exit so the next attempt backs off. Two of those steps are ordering requirements rather than steps -- the attempt must be recorded before the fork, because the child can die and be reaped before fork() returns, and SIGCHLD must be blocked across the fork and the assignment, or the reaper compares the dead child against a stale pid and misses it. Neither is apparent from reading the code, and both were got wrong at least once while writing it. Leaving them open-coded means the next caller -- $pid_UDP has the same never-respawned shape -- gets to rediscover them. Move the sequence into xCAT::RespawnUtils::supervise(), which takes the child body as a block and the rest as named arguments: ($mon_respawn, $pid_MON) = supervise { ...the child... } state => $mon_respawn, pid => $pid_MON, now => time(); The (&@) prototype is what allows the leading block, and it applies to a fully qualified call, so no Exporter machinery is needed. It does require the module to be loaded with `use` rather than `require`: under `require` the sub is unknown when the call is compiled, the block is then read as a bare block and its value arrives as the first argument, which fails at runtime rather than at compile time. Both call sites and the test use `use`, and the constraint is written down next to the sub. Passing a live pid is a no-op, so a caller that forgets to check does not end up with two children. The module gains its first impure function, which is why it sits under its own heading with the pure ones stated to be pure above it: those return new state and touch nothing, which is what keeps them testable on a made-up clock and safe inside a signal handler. supervise() forks, so it is tested by the fork-and-port case instead, which now drives it rather than its own copy of the same sequence. POSIX and xCAT::Utils are required inside supervise() rather than at the top, so loading the module for the pure functions still pulls in nothing. xcatd loses $mon_chldmask and its :signal_h import along with the duplication. Verified on a live MN: the startup fork goes through supervise() and produces a monitor holding xcatiport, and two consecutive kills are recovered in 5s then 10s -- the backoff -- with the port reclaimed and the SSL listener holding its pid throughout. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- perl-xCAT/xCAT/RespawnUtils.pm | 52 ++++++++++++++++++++++++++ xCAT-server/sbin/xcatd | 44 ++++++++-------------- xCAT-test/unit/xcatd_monitor_respawn.t | 17 +++++---- 3 files changed, 76 insertions(+), 37 deletions(-) diff --git a/perl-xCAT/xCAT/RespawnUtils.pm b/perl-xCAT/xCAT/RespawnUtils.pm index f768739e4..e770bce0e 100644 --- a/perl-xCAT/xCAT/RespawnUtils.pm +++ b/perl-xCAT/xCAT/RespawnUtils.pm @@ -33,6 +33,11 @@ package xCAT::RespawnUtils; use strict; use warnings; +# Everything up to the "Forking" section below is pure arithmetic: it reads only the state it +# is handed, returns a new one, and touches no clock, no globals and no processes. Keep it +# that way -- that is what lets the pacing be tested on a made-up clock instead of in real +# seconds, and what makes exited() safe to call from a signal handler. + # Read one tunable, falling back to the default unless it really looks like a whole number. sub _tunable { my ($value, $default) = @_; @@ -116,4 +121,51 @@ sub reported { return { %$state, reported => 1 }; } +# --- Forking ----------------------------------------------------------------------------- +# The one impure sub. Everything above only does arithmetic; this actually forks. + +# Fork a child and keep the pacing straight while doing it. Takes the child's body as a +# block, then `state`, `pid` and `now`, and hands back the state and the new pid: +# +# ($state, $pid) = xCAT::RespawnUtils::supervise { ...child... } +# state => $state, pid => $pid, now => time(); +# +# The (&@) prototype is what allows the leading block. It needs this module loaded with +# `use`, not `require`: under `require` the sub is unknown when the call is compiled, the +# block is then read as a bare block, and its value arrives as the first argument. +# +# Two orderings in here are easy to get wrong and are the reason this is not left to callers. +# The attempt is recorded before the fork, because the child can die and be reaped before +# fork() returns to us. And SIGCHLD is blocked across the fork and the assignment, because a +# reaper that matches on the pid would otherwise compare against a stale one, miss the death, +# and leave the caller believing a dead child is still alive. +# +# The block is only ever entered in the child and is not expected to return; if it does, the +# child exits quietly rather than falling back into the parent's code. Passing a live `pid` +# is a no-op, so a caller that forgets to check is not punished with a second child. +sub supervise (&@) { + my ($child, %arg) = @_; + my ($state, $pid, $now) = @arg{qw(state pid now)}; + + return ($state, $pid) if $pid; # already running; nothing to do + + require POSIX; + require xCAT::Utils; + + $state = forked($state, $now); + + my $mask = POSIX::SigSet->new(POSIX::SIGCHLD()); + POSIX::sigprocmask(POSIX::SIG_BLOCK(), $mask); + $pid = xCAT::Utils->xfork; + POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); + + return (exited($state, $now), 0) unless defined $pid; # could not fork: back off + + unless ($pid) { + $child->(); + POSIX::_exit(0); + } + return ($state, $pid); +} + 1; diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index 1b344c08b..74c0f6293 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -155,7 +155,7 @@ Getopt::Long::Configure("bundling"); Getopt::Long::Configure("pass_through"); use Storable qw(dclone); -use POSIX qw(WNOHANG setsid :errno_h :signal_h); +use POSIX qw(WNOHANG setsid :errno_h); my $pidfile; my $reload; my $foreground; @@ -1203,28 +1203,19 @@ if (!(socketpair($rescanreadpipe, $rescanwritepipe, AF_UNIX, SOCK_STREAM, PF_UNS } $rescanrselect = new IO::Select; $rescanrselect->add($rescanreadpipe); -# SIGCHLD must not be delivered between xfork() returning and the assignment to $pid_MON. -# ssl_reaper matches $CHILDPID against $pid_MON, so a child reaped in that window is compared -# against a stale value, missed, and $pid_MON is then left naming a pid that no longer exists. -# The main service loop reads !$pid_MON to decide whether the monitor needs respawning, so it -# would never respawn it again -- the same dead xcatiport this respawn exists to prevent. -my $mon_chldmask = POSIX::SigSet->new(SIGCHLD); - -# record this monitor too, so its uptime counts when it eventually dies -$mon_respawn = xCAT::RespawnUtils::forked($mon_respawn, time()); -sigprocmask(SIG_BLOCK, $mon_chldmask); -$pid_MON = xCAT::Utils->xfork; -sigprocmask(SIG_UNBLOCK, $mon_chldmask); # $pid_MON is assigned; the reaper can match it now -if (!defined $pid_MON) { - xCAT::MsgUtils->message("S", "Unable to fork installmonitor"); - die; -} -unless ($pid_MON) { +# supervise() records the attempt and blocks SIGCHLD across the fork; this monitor's uptime +# counts towards the pacing too, so an unrelated death much later is retried promptly. +($mon_respawn, $pid_MON) = xCAT::RespawnUtils::supervise { $$progname = "xcatd: install monitor"; $pid_UDP = 0; close($udpctl); $udpctl = 0; do_installm_service; xexit(0); +} state => $mon_respawn, pid => $pid_MON, now => time(); + +unless ($pid_MON) { + xCAT::MsgUtils->message("S", "Unable to fork installmonitor"); + die; } # ----used for command log start--------- @@ -1503,16 +1494,7 @@ until ($quit) { . " still retrying xcatiport $sport every $mon_respawn->{max_interval} seconds"); $mon_respawn = xCAT::RespawnUtils::reported($mon_respawn); } - $mon_respawn = xCAT::RespawnUtils::forked($mon_respawn, time()); - sigprocmask(SIG_BLOCK, $mon_chldmask); - $pid_MON = xCAT::Utils->xfork; - sigprocmask(SIG_UNBLOCK, $mon_chldmask); # both sides: the child needs it unblocked too - if (!defined $pid_MON) { - xCAT::MsgUtils->message("S", "xcatd: unable to re-fork install monitor"); - $pid_MON = 0; - # count the failed fork and back off before retrying - $mon_respawn = xCAT::RespawnUtils::exited($mon_respawn, time()); - } elsif (!$pid_MON) { # child: serve only the install monitor + ($mon_respawn, $pid_MON) = xCAT::RespawnUtils::supervise { $$progname = "xcatd: install monitor"; $pid_UDP = 0; close($listener); @@ -1525,9 +1507,13 @@ until ($quit) { close($chwritepipe); do_installm_service; xexit(0); - } else { + } state => $mon_respawn, pid => $pid_MON, now => time(); + + if ($pid_MON) { xCAT::MsgUtils->trace(0, "I", "xcatd: re-forked install monitor (pid $pid_MON) after it exited"); + } else { + xCAT::MsgUtils->message("S", "xcatd: unable to re-fork install monitor"); } } while ($udpalive and $udpwatcher->can_read(0)) { # take an intermission to broker some state requests from udp traffic control diff --git a/xCAT-test/unit/xcatd_monitor_respawn.t b/xCAT-test/unit/xcatd_monitor_respawn.t index 1db5c38f8..77aada39d 100644 --- a/xCAT-test/unit/xcatd_monitor_respawn.t +++ b/xCAT-test/unit/xcatd_monitor_respawn.t @@ -34,7 +34,7 @@ use Test::More; use POSIX qw(WNOHANG); use IO::Socket::INET; -require xCAT::RespawnUtils; +use xCAT::RespawnUtils; # `use`, not `require`: the (&@) prototype must be known at compile time # Every monitor stand-in forked below is registered here, so a failed assertion that # returns early out of the fork test cannot leave one sleeping on the port. The children @@ -225,10 +225,10 @@ subtest 'the monitor comes back on its own once the port is released' => sub { } if ( !$mon_pid && due( $pace, time() ) ) { my $now = time(); - $pace = forked( $pace, $now ); - my $pid = fork(); - die "fork failed: $!" unless defined $pid; - if ( !$pid ) { + + # Driven through supervise() -- the same call xcatd makes -- so this exercises the + # real fork-and-account sequence rather than a copy of it here. + ( $pace, $mon_pid ) = xCAT::RespawnUtils::supervise { close($holder) if $holder; # never hold the port from inside a child my $sock = IO::Socket::INET->new( LocalAddr => '127.0.0.1', @@ -239,12 +239,13 @@ subtest 'the monitor comes back on its own once the port is released' => sub { ); POSIX::_exit(1) unless $sock; # could not bind: died, as the real one does sleep 3600; # bound the port and serve - POSIX::_exit(0); } - $mon_pid = $pid; + state => $pace, pid => $mon_pid, now => $now; + + die "supervise did not fork" unless $mon_pid; $mon_forked_at = $now; push @forks, $now; - push @spawned, $pid; + push @spawned, $mon_pid; } select( undef, undef, undef, 0.05 ); }; From ce709cbeb0d11804d155a4ef73968fe7f777a7c6 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:55:44 -0300 Subject: [PATCH 14/20] test(xcat-core): nothing catches the reaper being handed a stale pid supervise() unblocks SIGCHLD and then hands the pid back for the caller to assign, so the caller's ($mon_respawn, $pid_MON) = ... still lands after the signal is let back in -- the exact window e0b0ac6 closed, reopened by moving the sequence into a function. A monitor that dies in that gap is compared by ssl_reaper against a $pid_MON still holding 0, missed, and the caller then writes the dead pid back over the reaper's work: !$pid_MON never fires again and the monitor is never respawned. That is this PR's own failure, reached through the respawn rather than through startup, and nothing in the file notices it. Add a subtest that watches the window from inside. Racing a real death into it is not something a test can arrange reliably, so it arranges a certainty instead: a decoy child is forked and exits with SIGCHLD blocked, leaving the signal pending, so the handler is guaranteed to run the moment supervise() unblocks -- inside supervise(), before it has returned. What the handler sees there is what ssl_reaper would see: it must find the live pid and a pacing state that already knows a child was forked. Separately, the fork-and-port subtest's closing assertion could not fail. It asks that the respawn after a healthy monitor's death lands within 2 seconds, with max_interval set to 2 -- so a backoff pinned at the ceiling satisfies it too, and deleting the healthy-run reset from exited() leaves the subtest green. Raise the ceiling to 8, where only the reset can produce a prompt respawn, and assert first that the monitor being killed had actually been up long enough to count as having served, which is the premise the assertion rests on. Verified: against the current supervise() the new subtest fails on both assertions ("got 0, expected "), and the raised ceiling turns the closing assertion red (8 <= 2) when the healthy branch of exited() is removed -- where at a ceiling of 2 it stayed green. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-test/unit/xcatd_monitor_respawn.t | 71 ++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 3 deletions(-) diff --git a/xCAT-test/unit/xcatd_monitor_respawn.t b/xCAT-test/unit/xcatd_monitor_respawn.t index 77aada39d..8f1cc0b11 100644 --- a/xCAT-test/unit/xcatd_monitor_respawn.t +++ b/xCAT-test/unit/xcatd_monitor_respawn.t @@ -188,6 +188,63 @@ subtest 'the pacing functions are pure' => sub { is( due( $pace, 0 ), due( $pace, 0 ), 'due() is free of side effects' ); }; +# --- the window the reaper looks through ------------------------------------ +# The reaper matches the dead child against xcatd's $pid_MON and folds the death into the +# pacing state. Both therefore have to be in place before SIGCHLD is let back in: a child that +# died while the fork was in flight would otherwise be compared against a pid still holding 0, +# missed, and the caller would then write a dead pid back over the reaper's work -- believing a +# dead monitor alive, and never respawning it. That is the whole failure this file is about, +# reached through the respawn instead of through startup. +# +# Racing a real death into that window is not something a test can arrange reliably, so this +# arranges a certainty instead: a SIGCHLD is made pending BEFORE supervise() is called (a decoy +# child that exits while the signal is blocked). The handler is then guaranteed to run the +# moment supervise() unblocks -- inside supervise(), before it has returned to us -- and what it +# sees there is exactly what the real reaper would see. +subtest 'the pid and the pacing are in place before SIGCHLD is let back in' => sub { + my $pace = xCAT::RespawnUtils::policy( min_interval => 1, max_interval => 2, healthy => 60 ); + my $mon_pid = 0; + + my ( $ran, $pid_seen, $started_at_seen ); + local $SIG{CHLD} = sub { + $ran++; + $pid_seen = $mon_pid; + $started_at_seen = $pace->{started_at}; + }; + + my $mask = POSIX::SigSet->new( POSIX::SIGCHLD ); + POSIX::sigprocmask( POSIX::SIG_BLOCK, $mask ); + + my $decoy = fork(); + POSIX::_exit(0) if defined($decoy) && !$decoy; + unless ($decoy) { + POSIX::sigprocmask( POSIX::SIG_UNBLOCK, $mask ); + plan skip_all => "cannot fork here: $!"; + } + select( undef, undef, undef, 0.2 ); # it is gone, and its SIGCHLD is pending, not delivered + + ( $pace, $mon_pid ) = xCAT::RespawnUtils::supervise { + sleep 3600; # a monitor that stays up; this one is about the parent + } + state => $pace, pid => $mon_pid, now => time(); + push @spawned, $mon_pid if $mon_pid; + + ok( $ran, 'the pending SIGCHLD was delivered while supervise() was still running' ); + ok( $mon_pid, 'supervise() forked' ); + is( $pid_seen, $mon_pid, + 'a reaper running at the unblock sees the live pid, not the stale 0 it would miss' ); + ok( defined $started_at_seen, + '...and a pacing state that already knows a child was forked' ); + + if ($mon_pid) { + kill 'TERM', $mon_pid; + waitpid( $mon_pid, 0 ); + @spawned = grep { $_ != $mon_pid } @spawned; + } + waitpid( $decoy, 0 ); + POSIX::sigprocmask( POSIX::SIG_UNBLOCK, $mask ); +}; + # --- the real thing: fail several times, release the port, recover ---------- subtest 'the monitor comes back on its own once the port is released' => sub { my $holder = IO::Socket::INET->new( @@ -201,9 +258,15 @@ subtest 'the monitor comes back on its own once the port is released' => sub { my $port = $holder->sockport; my $healthy = 2; + + # The ceiling is well above the promptness the last assertion asks for, on purpose: with a + # ceiling of 2 that assertion could not fail, since a backoff pinned at the ceiling would + # still land inside it, and dropping the healthy-run reset from exited() would leave the + # subtest green. At 8 the reset is the only thing that can produce a prompt respawn. + my $ceiling = 8; my $pace = xCAT::RespawnUtils::policy( min_interval => 1, - max_interval => 2, + max_interval => $ceiling, healthy => $healthy, ); @@ -286,7 +349,7 @@ subtest 'the monitor comes back on its own once the port is released' => sub { ok( $mon_pid && time() - $mon_forked_at >= $healthy + 1, 'a respawned monitor binds the freed port and stays up -- no xcatd restart' ) or return; - cmp_ok( $mon_forked_at - $released, '<=', 5, + cmp_ok( $mon_forked_at - $released, '<=', $ceiling + 1, 'recovery lands within the backoff ceiling of the port becoming free' ); # (4) and after that healthy run the pacing is back to prompt @@ -296,8 +359,10 @@ subtest 'the monitor comes back on its own once the port is released' => sub { cmp_ok( scalar(@forks), '>', $forks_before, 'killing the healthy monitor gets it replaced again' ); + cmp_ok( $deaths[-1][2] - $deaths[-1][1], '>=', $healthy, + 'the monitor that was killed had been up long enough to count as having served' ); cmp_ok( $forks[-1] - $deaths[-1][2], '<=', 2, - 'that replacement is prompt: the healthy run reset the backoff' ); + 'that replacement is prompt: the healthy run reset the backoff off its ceiling' ); if ($mon_pid) { kill 'TERM', $mon_pid; From 49e77398e4006927e2ebf384195d29624364438f Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:56:11 -0300 Subject: [PATCH 15/20] fix(xcat-core): supervise() lets SIGCHLD back in before the caller has the pid supervise() blocks SIGCHLD across the fork but unblocks it before returning, and the caller installs the pid afterwards: ($mon_respawn, $pid_MON) = xCAT::RespawnUtils::supervise { ... } ...; so the assignment is outside the blocked region -- the same unprotected window that existed before e0b0ac6, moved from xcatd into the helper that was meant to make it impossible to get wrong. ssl_reaper matches the dead child against $pid_MON and folds the death into $mon_respawn; a monitor dying in that gap is compared against a pid still holding 0, missed, and the caller then overwrites both with a pid that no longer exists. !$pid_MON never fires again, so the respawn loop never runs and xcatiport stays dead until xcatd is restarted -- the failure this PR exists to remove. Have supervise() install them itself, which is why `state` and `pid` are now passed by reference: the pacing state is recorded and the pid assigned while SIGCHLD is still blocked, and only then is it unblocked, so there is no point at which a reaper can run and see either of them stale. Nothing is left for the caller to do afterwards, so both call sites become plain statements that read $pid_MON when they need it. The child unblocks before running its body, as it did when the unblock sat ahead of the fork's branch. The new pid is returned as well, for a caller that wants it inline. Verified on a live MN (xcat54-mn, AlmaLinux 10.2, xCAT 2.19.0): the startup fork produces a monitor holding xcatiport 3002; killing it is recovered in 5s, killing the replacement at once in 11s -- the backoff -- and killing one that had served past the healthy interval is recovered in 1s, with the port reclaimed and xcatd active throughout. The unit test's window subtest, red in the preceding commit, now passes. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- perl-xCAT/xCAT/RespawnUtils.pm | 45 +++++++++++++++++--------- xCAT-server/sbin/xcatd | 14 ++++---- xCAT-test/unit/xcatd_monitor_respawn.t | 8 ++--- 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/perl-xCAT/xCAT/RespawnUtils.pm b/perl-xCAT/xCAT/RespawnUtils.pm index e770bce0e..43a8f73ae 100644 --- a/perl-xCAT/xCAT/RespawnUtils.pm +++ b/perl-xCAT/xCAT/RespawnUtils.pm @@ -125,10 +125,10 @@ sub reported { # The one impure sub. Everything above only does arithmetic; this actually forks. # Fork a child and keep the pacing straight while doing it. Takes the child's body as a -# block, then `state`, `pid` and `now`, and hands back the state and the new pid: +# block, then `state` and `pid` -- REFERENCES to the caller's own variables -- and `now`: # -# ($state, $pid) = xCAT::RespawnUtils::supervise { ...child... } -# state => $state, pid => $pid, now => time(); +# xCAT::RespawnUtils::supervise { ...child... } +# state => \$state, pid => \$pid, now => time(); # # The (&@) prototype is what allows the leading block. It needs this module loaded with # `use`, not `require`: under `require` the sub is unknown when the call is compiled, the @@ -136,36 +136,49 @@ sub reported { # # Two orderings in here are easy to get wrong and are the reason this is not left to callers. # The attempt is recorded before the fork, because the child can die and be reaped before -# fork() returns to us. And SIGCHLD is blocked across the fork and the assignment, because a -# reaper that matches on the pid would otherwise compare against a stale one, miss the death, -# and leave the caller believing a dead child is still alive. +# fork() returns to us. And the pid and the state are installed in the caller's variables +# while SIGCHLD is still blocked -- which is why they are passed by reference rather than +# handed back as a return value. The reaper matches the dead child against that pid and folds +# the death into that state; had the caller assigned them from a return value, the assignment +# would land after the signal was let back in, so a child dying in the gap would be compared +# against a pid still holding 0, missed, and the caller would then write a dead pid back over +# the reaper's work -- believing a dead child alive, and never respawning it. # # The block is only ever entered in the child and is not expected to return; if it does, the # child exits quietly rather than falling back into the parent's code. Passing a live `pid` -# is a no-op, so a caller that forgets to check is not punished with a second child. +# is a no-op, so a caller that forgets to check is not punished with a second child. The new +# pid is also returned, for a caller that wants it inline. sub supervise (&@) { my ($child, %arg) = @_; - my ($state, $pid, $now) = @arg{qw(state pid now)}; + my ($stateref, $pidref, $now) = @arg{qw(state pid now)}; - return ($state, $pid) if $pid; # already running; nothing to do + return $$pidref if $$pidref; # already running; nothing to do require POSIX; require xCAT::Utils; - $state = forked($state, $now); - my $mask = POSIX::SigSet->new(POSIX::SIGCHLD()); POSIX::sigprocmask(POSIX::SIG_BLOCK(), $mask); - $pid = xCAT::Utils->xfork; - POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); - return (exited($state, $now), 0) unless defined $pid; # could not fork: back off + $$stateref = forked($$stateref, $now); + my $pid = xCAT::Utils->xfork; - unless ($pid) { + unless (defined $pid) { # could not fork: count it and back off + $$stateref = exited($$stateref, $now); + $$pidref = 0; + POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); + return 0; + } + + unless ($pid) { # child: it must not go on to serve with SIGCHLD blocked + POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); $child->(); POSIX::_exit(0); } - return ($state, $pid); + + $$pidref = $pid; # in place before the reaper can run, or it matches a stale pid + POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); + return $pid; } 1; diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index 74c0f6293..0ca56274a 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -1203,15 +1203,17 @@ if (!(socketpair($rescanreadpipe, $rescanwritepipe, AF_UNIX, SOCK_STREAM, PF_UNS } $rescanrselect = new IO::Select; $rescanrselect->add($rescanreadpipe); -# supervise() records the attempt and blocks SIGCHLD across the fork; this monitor's uptime -# counts towards the pacing too, so an unrelated death much later is retried promptly. -($mon_respawn, $pid_MON) = xCAT::RespawnUtils::supervise { +# supervise() records the attempt, blocks SIGCHLD across the fork, and installs $pid_MON and +# $mon_respawn itself while it is still blocked -- which is why they are passed by reference; +# see the sub. This monitor's uptime counts towards the pacing too, so an unrelated death much +# later is retried promptly. +xCAT::RespawnUtils::supervise { $$progname = "xcatd: install monitor"; $pid_UDP = 0; close($udpctl); $udpctl = 0; do_installm_service; xexit(0); -} state => $mon_respawn, pid => $pid_MON, now => time(); +} state => \$mon_respawn, pid => \$pid_MON, now => time(); unless ($pid_MON) { xCAT::MsgUtils->message("S", "Unable to fork installmonitor"); @@ -1494,7 +1496,7 @@ until ($quit) { . " still retrying xcatiport $sport every $mon_respawn->{max_interval} seconds"); $mon_respawn = xCAT::RespawnUtils::reported($mon_respawn); } - ($mon_respawn, $pid_MON) = xCAT::RespawnUtils::supervise { + xCAT::RespawnUtils::supervise { $$progname = "xcatd: install monitor"; $pid_UDP = 0; close($listener); @@ -1507,7 +1509,7 @@ until ($quit) { close($chwritepipe); do_installm_service; xexit(0); - } state => $mon_respawn, pid => $pid_MON, now => time(); + } state => \$mon_respawn, pid => \$pid_MON, now => time(); if ($pid_MON) { xCAT::MsgUtils->trace(0, "I", diff --git a/xCAT-test/unit/xcatd_monitor_respawn.t b/xCAT-test/unit/xcatd_monitor_respawn.t index 8f1cc0b11..fe7a36751 100644 --- a/xCAT-test/unit/xcatd_monitor_respawn.t +++ b/xCAT-test/unit/xcatd_monitor_respawn.t @@ -223,10 +223,10 @@ subtest 'the pid and the pacing are in place before SIGCHLD is let back in' => s } select( undef, undef, undef, 0.2 ); # it is gone, and its SIGCHLD is pending, not delivered - ( $pace, $mon_pid ) = xCAT::RespawnUtils::supervise { + xCAT::RespawnUtils::supervise { sleep 3600; # a monitor that stays up; this one is about the parent } - state => $pace, pid => $mon_pid, now => time(); + state => \$pace, pid => \$mon_pid, now => time(); push @spawned, $mon_pid if $mon_pid; ok( $ran, 'the pending SIGCHLD was delivered while supervise() was still running' ); @@ -291,7 +291,7 @@ subtest 'the monitor comes back on its own once the port is released' => sub { # Driven through supervise() -- the same call xcatd makes -- so this exercises the # real fork-and-account sequence rather than a copy of it here. - ( $pace, $mon_pid ) = xCAT::RespawnUtils::supervise { + xCAT::RespawnUtils::supervise { close($holder) if $holder; # never hold the port from inside a child my $sock = IO::Socket::INET->new( LocalAddr => '127.0.0.1', @@ -303,7 +303,7 @@ subtest 'the monitor comes back on its own once the port is released' => sub { POSIX::_exit(1) unless $sock; # could not bind: died, as the real one does sleep 3600; # bound the port and serve } - state => $pace, pid => $mon_pid, now => $now; + state => \$pace, pid => \$mon_pid, now => $now; die "supervise did not fork" unless $mon_pid; $mon_forked_at = $now; From 207ec20f83637d8d8c2f5a1cbf8ef600fd0c2cf4 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Tue, 1 Sep 2026 18:56:52 -0300 Subject: [PATCH 16/20] test(xcatd): the purity check misses exited() mutating in place The subtest composed the two calls as exited(forked($pace,10),11), so exited() only ever got forked()'s throwaway intermediate to mutate. An exited() that wrote in place left $pace untouched and the assertion stayed green -- the review that found this confirmed it by making exited() impure and watching the file pass. Check each on a state it was handed directly. Verified the other way round: making exited() assign into its argument and return the same reference now reddens both new assertions, where before it reddened nothing. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-test/unit/xcatd_monitor_respawn.t | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/xCAT-test/unit/xcatd_monitor_respawn.t b/xCAT-test/unit/xcatd_monitor_respawn.t index fe7a36751..1912c8fbf 100644 --- a/xCAT-test/unit/xcatd_monitor_respawn.t +++ b/xCAT-test/unit/xcatd_monitor_respawn.t @@ -186,6 +186,24 @@ subtest 'the pacing functions are pure' => sub { is_deeply( $pace, \%before, 'exited()/forked() leave the state they were given alone' ); isnt( $after, $pace, 'they return a new state rather than the same reference' ); is( due( $pace, 0 ), due( $pace, 0 ), 'due() is free of side effects' ); + + # Each of the two has to be checked on a state it was handed DIRECTLY. Composing them as + # exited(forked($pace,...)) only ever lets exited() mutate forked()'s throwaway + # intermediate, so an exited() that wrote in place would leave $pace untouched and the + # assertion above green. + my $only_forked = xCAT::RespawnUtils::policy( min_interval => 1, max_interval => 8 ); + my %before_forked = %$only_forked; + my $forked_out = forked( $only_forked, 10 ); + is_deeply( $only_forked, \%before_forked, + 'forked() alone leaves the state it was given alone' ); + isnt( $forked_out, $only_forked, 'forked() returns a new state' ); + + my $only_exited = xCAT::RespawnUtils::policy( min_interval => 1, max_interval => 8 ); + my %before_exited = %$only_exited; + my $exited_out = exited( $only_exited, 5 ); + is_deeply( $only_exited, \%before_exited, + 'exited() alone leaves the state it was given alone' ); + isnt( $exited_out, $only_exited, 'exited() returns a new state' ); }; # --- the window the reaper looks through ------------------------------------ From 4e44053f3222991f0196a7e38d40d5824dfcf2f1 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Thu, 3 Sep 2026 09:07:23 -0300 Subject: [PATCH 17/20] test(xcat-core): nothing covers what xcatd does when the monitor dies or is re-forked The respawn tests cover the pacing in xCAT::RespawnUtils. Two things the daemon itself has to do are untested. A SIGCHLD handler that does not clear $pid_MON leaves xcatd holding a dead pid, so the service loop never re-forks the monitor. Only ssl_reaper clears it, and generic_reaper is the handler at startup and again while connections are throttled. The respawn block also runs from the middle of the service loop, so the child inherits the client connections the parent has accepted but not yet dispatched. xcatd cannot be loaded in a unit test, so the two reapers and the respawn fork block are lifted out of the program text and run in a scratch package against stand-in descriptors. generic_reaper fails, and so does the pending-connection assertion; ssl_reaper passes and guards the path that already works. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-test/unit/xcatd_install_monitor.t | 204 +++++++++++++++++++++++++ 1 file changed, 204 insertions(+) create mode 100644 xCAT-test/unit/xcatd_install_monitor.t diff --git a/xCAT-test/unit/xcatd_install_monitor.t b/xCAT-test/unit/xcatd_install_monitor.t new file mode 100644 index 000000000..d1202c401 --- /dev/null +++ b/xCAT-test/unit/xcatd_install_monitor.t @@ -0,0 +1,204 @@ +#!/usr/bin/env perl +# +# Unit test for the two things xcatd itself has to get right about the install monitor -- +# the child that listens on xcatiport for node install-status updates and the "next" +# boot-flip request. The pacing of the respawn is covered by xcatd_monitor_respawn.t; this +# file covers the daemon-side code that pacing depends on. +# +# 1. Every SIGCHLD handler that can be installed when the monitor dies has to account for +# the death. The monitor is forked at startup, while generic_reaper is the handler -- +# ssl_reaper is only installed once the main service loop starts, and generic_reaper +# comes back whenever connections are throttled. A death reaped by a handler that does +# not clear $pid_MON leaves the daemon holding a dead pid, so the respawn in the +# service loop never runs and xcatiport stays dead for the life of the daemon. +# +# 2. The respawned monitor must not carry the parent's other descriptors. It is forked +# from the middle of the service loop, so besides the SSL listener and the udpctl +# socket it also inherits the rescanplugins channel and any client connection the +# parent has accepted but not yet handed to a worker. The monitor serves none of +# those and outlives every one of them. +# +# xcatd cannot be loaded here: it needs the database, SSL, the plugin tree and +# /var/run/xcat, and it starts serving at the bottom of the file. So the routine and the +# fork block under test are lifted out of the program text and run in a scratch package +# against stand-in handles. BAIL_OUT if a lift stops matching, so this fails loudly rather +# than quietly covering nothing. + +use strict; +use warnings; + +use FindBin; +use lib "$FindBin::Bin/../../perl-xCAT"; +use Test::More; +use POSIX (); +use Socket; + +use xCAT::RespawnUtils; + +my $XCATD = "$FindBin::Bin/../../xCAT-server/sbin/xcatd"; +plan skip_all => "xcatd not found at $XCATD" unless -r $XCATD; + +my $src = do { + open my $fh, '<', $XCATD or BAIL_OUT("cannot read $XCATD: $!"); + local $/; + <$fh>; +}; + +# A named sub in xcatd, from "sub name {" to the closing brace in the first column. +sub lift_sub { + my ($name) = @_; + my ($body) = $src =~ /^(sub \s+ \Q$name\E \s* \{ .*? ^ \} )/msx; + return $body; +} + +# --- 1. whichever reaper is installed, a dead monitor is accounted for ------- + +my %reaper = map { $_ => lift_sub($_) } qw(generic_reaper ssl_reaper); +for my $name (sort keys %reaper) { + BAIL_OUT("cannot lift $name out of xcatd -- the lift needs updating") + unless $reaper{$name}; +} + +# reap_install_monitor is what this test asks xcatd to grow. Lift it when it is there, and +# supply a do-nothing stand-in when it is not, so the reapers still compile and the +# assertions below report a monitor death that went unnoticed -- which is the defect -- +# instead of a syntax error. +my $shared = lift_sub('reap_install_monitor') || 'sub reap_install_monitor { }'; + +{ + my $scratch = join "\n", + 'package t::xcatd;', + 'no strict;', + 'no warnings;', + 'sub yield { }', + $shared, + $reaper{generic_reaper}, + $reaper{ssl_reaper}, + '1;'; + eval $scratch or BAIL_OUT("cannot compile the lifted reapers: $@"); +} + +# Fork a child, let it exit, and hand it to $reaper as the install monitor. Returns the +# pacing state the reaper left behind, or undef when it did not notice the death at all. +sub reap_a_dead_monitor { + my ($reaper) = @_; + + my $pid = fork(); + BAIL_OUT("cannot fork: $!") unless defined $pid; + POSIX::_exit(0) unless $pid; + + my $now = time(); + { + no strict 'refs'; + ${'t::xcatd::pid_MON'} = $pid; + ${'t::xcatd::mon_respawn'} = xCAT::RespawnUtils::forked( + xCAT::RespawnUtils::policy(min_interval => 5, max_interval => 300), $now); + } + + # Wait for it to be reapable, then run the handler by hand rather than through the + # signal: what is under test is what the handler does with the death, not delivery. + local $SIG{CHLD} = 'DEFAULT'; + select(undef, undef, undef, 0.05) for 1 .. 4; + + no strict 'refs'; + &{"t::xcatd::$reaper"}(); + + return undef if ${'t::xcatd::pid_MON'}; + return ${'t::xcatd::mon_respawn'}; +} + +for my $reaper (qw(generic_reaper ssl_reaper)) { + subtest "$reaper accounts for a dead install monitor" => sub { + my $pace = reap_a_dead_monitor($reaper); + + ok($pace, "$reaper cleared \$pid_MON, so the service loop can re-fork the monitor") + or do { + diag("$reaper reaped the monitor and left \$pid_MON holding its pid;" + . " nothing will ever respawn it"); + return; + }; + ok(!defined $pace->{started_at}, + 'the pacing was told the monitor exited, so the next respawn is scheduled'); + cmp_ok($pace->{streak}, '>', 0, 'the death counts towards the backoff'); + }; +} + +# --- 2. the respawned monitor drops what it inherited ------------------------ + +# Both supervise() blocks in xcatd: the startup fork and the respawn in the service loop. +my @blocks = $src =~ /xCAT::RespawnUtils::supervise \s* \{ (.*?) ^\s* \} \s* state \s* =>/msgx; +BAIL_OUT("expected two supervise blocks in xcatd, found " . scalar(@blocks)) + unless @blocks == 2; +my ($respawn) = grep { /\$listener/ } @blocks; +BAIL_OUT("cannot tell the respawn block from the startup one -- the lift needs updating") + unless $respawn; + +{ + my $stubs = join "\n", + 'package t::monitor;', + 'no strict;', + 'no warnings;', + 'our $served = 0;', + 'sub do_installm_service { $served++ }', + 'sub xexit { die "xexit\n" }', + '1;'; + eval $stubs or BAIL_OUT("cannot compile the monitor stubs: $@"); + + my $body = "package t::monitor; no strict; no warnings; sub become_monitor { $respawn }; 1;"; + eval $body or BAIL_OUT("cannot compile the lifted respawn block: $@"); +} + +# A connected pair of descriptors, so close() has something real to close. +sub a_socket { + socketpair(my $near, my $far, AF_UNIX, SOCK_STREAM, PF_UNSPEC) + or BAIL_OUT("socketpair failed: $!"); + return ($near, $far); +} + +subtest 'a respawned monitor keeps none of the descriptors it inherited' => sub { + my %handle; + my @keep; + for my $name (qw(listener udpctl chreadpipe chwritepipe)) { + my ($near, $far) = a_socket(); + $handle{$name} = $near; + push @keep, $far; + } + my @pending; + for (1 .. 3) { + my ($near, $far) = a_socket(); + push @pending, $near; + push @keep, $far; + } + + { + no strict 'refs'; + ${"t::monitor::$_"} = $handle{$_} for keys %handle; + ${'t::monitor::progname'} = \(my $title = 'xcatd'); + ${'t::monitor::pid_UDP'} = 4242; + @{'t::monitor::pendingconnections'} = @pending; + } + $t::monitor::served = 0; + + eval { t::monitor::become_monitor(); 1 }; + my $left = $@; + + is($left, "xexit\n", 'the block ran to the end and left through xexit'); + cmp_ok($t::monitor::served, '==', 1, 'and it entered do_installm_service on the way'); + + for my $name (sort keys %handle) { + ok(!defined fileno($handle{$name}), "the monitor closed the inherited $name"); + } + + my @open = grep { defined fileno($pending[$_]) } 0 .. $#pending; + is_deeply(\@open, [], + 'the monitor closed the connections the parent had accepted but not yet dispatched') + or diag("a client socket the monitor holds stays open for the life of the daemon," + . " long after the worker that served it has gone"); + + no strict 'refs'; + is(${'t::monitor::pid_UDP'}, 0, 'and it no longer believes it owns the udp child'); + + close($_) for @keep; +}; + +done_testing(); From b7c1461f9b43ed8d186d1c7fa6cc96b742423f54 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Thu, 3 Sep 2026 09:08:29 -0300 Subject: [PATCH 18/20] fix(xcat-core): a monitor death reaped at startup is never noticed The install monitor is forked while generic_reaper is the SIGCHLD handler. ssl_reaper is only installed once the main service loop starts, and generic_reaper comes back whenever connections are throttled. Only ssl_reaper cleared $pid_MON. A death reaped by generic_reaper left $pid_MON holding a dead pid, and the service loop re-forks only when $pid_MON is clear, so xcatiport stayed dead for the life of the daemon. Move that accounting into reap_install_monitor and call it from both reapers. xcatd_install_monitor.t runs both reapers over a dead child and requires each to clear $pid_MON and fold the death into the pacing. The generic_reaper case fails without this change. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-server/sbin/xcatd | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index 0ca56274a..3fdcc0f7d 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -1035,11 +1035,25 @@ wait_db_process(); my $CHILDPID = 0; # Global for reapers my %immediatechildren; +# Fold the death of the install monitor into the respawn pacing. Every reaper that can be +# the handler when it dies has to call this: generic_reaper covers startup and the throttled +# path, ssl_reaper the rest of the service loop. A death reaped without this leaves $pid_MON +# holding a dead pid, and the service loop only re-forks when $pid_MON is clear. +sub reap_install_monitor { + my ($pid) = @_; + + return unless $pid_MON and $pid == $pid_MON; + $pid_MON = 0; + $mon_respawn = xCAT::RespawnUtils::exited($mon_respawn, time()); + return; +} + sub generic_reaper { local ($!); #print "generic_reaper in $$..."; while (($CHILDPID = waitpid(-1, WNOHANG)) > 0) { #print "reaper for $CHILDPID...\n"; + reap_install_monitor($CHILDPID); if ($CHILDPID == $pid_UDP) { if ($udpctl) { # got here because UDP child is gone @@ -1075,10 +1089,7 @@ sub ssl_reaper { if ($CHILDPID == $cmdlog_svrpid) { $cmdlog_svrpid = 0; } - if ($CHILDPID == $pid_MON) { - $pid_MON = 0; - $mon_respawn = xCAT::RespawnUtils::exited($mon_respawn, time()); - } + reap_install_monitor($CHILDPID); } $SIG{CHLD} = \&ssl_reaper; } From 424f297d4b10e9c165285f81f0af92dad8cbf2db Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Thu, 3 Sep 2026 09:08:36 -0300 Subject: [PATCH 19/20] fix(xcat-core): the respawned monitor holds client sockets open for good The respawn is forked from the middle of the service loop, so the child inherits @pendingconnections -- the client sockets the parent has accepted and not yet handed to a worker. The monitor never serves one, and it outlives the worker that does, so its copy keeps that client's socket open until the daemon exits. Close them in the child, next to the listener and the rescanplugins channel it already drops. xcatd_install_monitor.t runs the lifted respawn block against stand-in descriptors and requires every pending connection to be closed. It fails without this change. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-server/sbin/xcatd | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index 3fdcc0f7d..669a8a67e 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -1518,6 +1518,11 @@ until ($quit) { # would differ from the one forked at startup. close($chreadpipe); close($chwritepipe); + # A pending connection is a client socket the parent has accepted and not yet + # handed to a worker. The monitor never serves one, and it outlives the worker + # that does, so a copy left open here holds that client's socket for the life of + # the daemon. + close($_) for @pendingconnections; do_installm_service; xexit(0); } state => \$mon_respawn, pid => \$pid_MON, now => time(); From efa914c5af5b03e0f444aca9a554130eef60a80c Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Thu, 3 Sep 2026 09:17:45 -0300 Subject: [PATCH 20/20] style(xcat-core): the respawn comments explain more than the code needs The comments around the install monitor respawn retell the failure, defend the design and repeat the same causal chain in three places. Reduce them to the facts that are not visible at the site: the ordering rules, why there is no attempt limit, and what each fork site inherits. The rest is in the commit messages and the PR. Comment only. RespawnUtils.pm loses 26 lines and no code changes; xcatd loses comment lines only. Both unit test files still pass. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- perl-xCAT/xCAT/RespawnUtils.pm | 95 ++++++++++++---------------------- xCAT-server/sbin/xcatd | 45 +++++----------- 2 files changed, 47 insertions(+), 93 deletions(-) diff --git a/perl-xCAT/xCAT/RespawnUtils.pm b/perl-xCAT/xCAT/RespawnUtils.pm index 43a8f73ae..bd432228c 100644 --- a/perl-xCAT/xCAT/RespawnUtils.pm +++ b/perl-xCAT/xCAT/RespawnUtils.pm @@ -1,51 +1,38 @@ # IBM(c) 2007 EPL license http://www.eclipse.org/legal/epl-v10.html package xCAT::RespawnUtils; -# Pacing for a parent that has to keep a child alive; xcatd's install monitor is the caller. +# Backoff for a parent that re-forks a child when it dies; xcatd's install monitor is the +# caller. The delay doubles from min_interval to max_interval and holds there. # -# The child is re-forked whenever it dies, but not as fast as fork() returns -- it may be -# dying because a resource it needs is held by someone else, and retrying flat out burns CPU -# and interferes with whatever handshake it performs to claim that resource. So attempts back -# off, doubling from min_interval to max_interval and then holding there. +# There is no attempt limit. A spent budget cannot be refilled: with no child alive nothing +# resets it, and the resource stays dead until xcatd restarts. A child that ran `healthy` +# seconds served, so its death resets the delay. # -# It never stops retrying. A retry budget that runs out cannot be refilled, because with no -# child alive nothing is left to reset it, so the resource would stay unserved until the whole -# daemon is restarted -- the failure the respawn exists to prevent. A child that stayed up -# `healthy` seconds evidently did claim its resource and serve, so its death resets the delay -# and only a real streak of failures to start builds the backoff up. +# Every sub returns a new state and leaves its argument alone, so exited() can run in a +# SIGCHLD handler. # -# Every function returns a NEW state and never mutates the one it is handed. That is what -# makes them safe to call from a SIGCHLD handler: the result is complete before the caller's -# assignment installs it, so a signal cannot catch the pacing half-written. +# The state is a plain hash; call the subs below for the next state. # -# The state is a plain hash. Callers may read these; use the functions below to get the next -# state rather than writing to them. -# -# min_interval shortest wait between attempts, and what a healthy run resets the delay to -# max_interval longest wait -- the delay doubles up to this and then stays here -# healthy how long a child must survive before we count it as having served -# delay how long to wait after the NEXT failure +# min_interval shortest wait, and what a healthy run resets the delay to +# max_interval longest wait; the delay doubles up to this +# healthy seconds a child must survive to count as having served +# delay the wait after the next failure # next_at earliest time() at which another attempt is allowed -# started_at when the running child was forked, or undef when none is running -# streak how many children in a row have died young -# reported whether we have already logged that this streak reached the ceiling +# started_at when the running child was forked, undef when none is running +# streak children in a row that died young +# reported whether the ceiling was already logged for this streak use strict; use warnings; -# Everything up to the "Forking" section below is pure arithmetic: it reads only the state it -# is handed, returns a new one, and touches no clock, no globals and no processes. Keep it -# that way -- that is what lets the pacing be tested on a made-up clock instead of in real -# seconds, and what makes exited() safe to call from a signal handler. - -# Read one tunable, falling back to the default unless it really looks like a whole number. +# The tunables reach policy() straight from %ENV, so they can be empty or misspelt. Anything +# that is not a whole number is treated as unset. sub _tunable { my ($value, $default) = @_; return $default unless defined($value) && $value =~ /^\s*\d+\s*$/; return $value + 0; } -# Start pacing a child from scratch. Anything the caller leaves out gets a sensible default. sub policy { my (%opt) = @_; @@ -53,7 +40,7 @@ sub policy { my $max = _tunable($opt{max_interval}, 300); my $healthy = _tunable($opt{healthy}, 60); - $min = 1 if $min < 1; + $min = 1 if $min < 1; # 0 doubles to 0, which is a fork storm $max = $min if $max < $min; return { @@ -68,21 +55,17 @@ sub policy { }; } -# Is it time to try again yet? This can say "not yet", but it never says "no more". sub due { my ($state, $now) = @_; return $now >= $state->{next_at} ? 1 : 0; } -# Note that we are about to fork, so we can tell later how long the child lasted. Call this -# before forking: the child can die and be reaped before fork() even returns to us. +# Call before forking: the child can die and be reaped before fork() returns to the parent. sub forked { my ($state, $now) = @_; return { %$state, started_at => $now }; } -# Note that the child died, and decide when to try again -- straight away if it had been up -# long enough to have served, later and later if it keeps failing to start. sub exited { my ($state, $now) = @_; @@ -106,8 +89,7 @@ sub exited { return \%next; } -# Has this run of failures just hit the ceiling, and not been mentioned yet? Keeps the log to -# one line per streak instead of one per attempt. +# Keeps the log to one line per streak rather than one per attempt. sub should_report { my ($state) = @_; return 0 if $state->{reported}; @@ -115,44 +97,35 @@ sub should_report { return $state->{delay} >= $state->{max_interval} ? 1 : 0; } -# Remember that we have already logged the ceiling for this streak. sub reported { my ($state) = @_; return { %$state, reported => 1 }; } # --- Forking ----------------------------------------------------------------------------- -# The one impure sub. Everything above only does arithmetic; this actually forks. +# The one impure sub. Everything above is arithmetic. -# Fork a child and keep the pacing straight while doing it. Takes the child's body as a -# block, then `state` and `pid` -- REFERENCES to the caller's own variables -- and `now`: +# Fork a child, taking its body as a block: # # xCAT::RespawnUtils::supervise { ...child... } # state => \$state, pid => \$pid, now => time(); # -# The (&@) prototype is what allows the leading block. It needs this module loaded with -# `use`, not `require`: under `require` the sub is unknown when the call is compiled, the -# block is then read as a bare block, and its value arrives as the first argument. +# `state` and `pid` are references to the caller's own variables. The reaper matches the dead +# child against that pid and folds the death into that state, so both have to be in place +# while SIGCHLD is still blocked; values assigned from a return would land after it is let +# back in, and a child dying in the gap would be compared against a pid still holding 0. # -# Two orderings in here are easy to get wrong and are the reason this is not left to callers. -# The attempt is recorded before the fork, because the child can die and be reaped before -# fork() returns to us. And the pid and the state are installed in the caller's variables -# while SIGCHLD is still blocked -- which is why they are passed by reference rather than -# handed back as a return value. The reaper matches the dead child against that pid and folds -# the death into that state; had the caller assigned them from a return value, the assignment -# would land after the signal was let back in, so a child dying in the gap would be compared -# against a pid still holding 0, missed, and the caller would then write a dead pid back over -# the reaper's work -- believing a dead child alive, and never respawning it. +# The (&@) prototype needs this module loaded with `use`. Under `require` the sub is unknown +# when the call is compiled, the block is read as a bare block, and its value arrives as the +# first argument. # -# The block is only ever entered in the child and is not expected to return; if it does, the -# child exits quietly rather than falling back into the parent's code. Passing a live `pid` -# is a no-op, so a caller that forgets to check is not punished with a second child. The new -# pid is also returned, for a caller that wants it inline. +# The block runs only in the child and is not expected to return. Passing a live `pid` is a +# no-op. Returns the new pid. sub supervise (&@) { my ($child, %arg) = @_; my ($stateref, $pidref, $now) = @arg{qw(state pid now)}; - return $$pidref if $$pidref; # already running; nothing to do + return $$pidref if $$pidref; require POSIX; require xCAT::Utils; @@ -170,13 +143,13 @@ sub supervise (&@) { return 0; } - unless ($pid) { # child: it must not go on to serve with SIGCHLD blocked + unless ($pid) { # the child must not serve with SIGCHLD blocked POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); $child->(); POSIX::_exit(0); } - $$pidref = $pid; # in place before the reaper can run, or it matches a stale pid + $$pidref = $pid; # in place before the reaper runs, or it matches a stale pid POSIX::sigprocmask(POSIX::SIG_UNBLOCK(), $mask); return $pid; } diff --git a/xCAT-server/sbin/xcatd b/xCAT-server/sbin/xcatd index 669a8a67e..c8d08d887 100755 --- a/xCAT-server/sbin/xcatd +++ b/xCAT-server/sbin/xcatd @@ -55,11 +55,7 @@ use xCAT::xcatd; use xCAT::CmdLog; use xCAT::State; -# Pacing for re-forking the install monitor; see the main service loop below. The pacing -# itself is pure arithmetic in xCAT::RespawnUtils -- a backoff with a ceiling but no end, -# so a held xcatiport costs one fork per ceiling interval instead of a fork storm, and the -# monitor is back within that bound once the port is free. Each function returns a NEW -# state, which is why the reaper can safely install one from inside the signal handler. +# Pacing for re-forking the install monitor; see the main service loop below. my $mon_respawn = xCAT::RespawnUtils::policy( min_interval => $ENV{XCATD_MON_RESPAWN_MIN_INTERVAL}, max_interval => $ENV{XCATD_MON_RESPAWN_MAX_INTERVAL}, @@ -1035,10 +1031,9 @@ wait_db_process(); my $CHILDPID = 0; # Global for reapers my %immediatechildren; -# Fold the death of the install monitor into the respawn pacing. Every reaper that can be -# the handler when it dies has to call this: generic_reaper covers startup and the throttled -# path, ssl_reaper the rest of the service loop. A death reaped without this leaves $pid_MON -# holding a dead pid, and the service loop only re-forks when $pid_MON is clear. +# Both reapers call this. generic_reaper is the handler at startup and while connections are +# throttled, ssl_reaper for the rest of the service loop. A death reaped without this leaves +# $pid_MON holding a dead pid, and the service loop re-forks only when $pid_MON is clear. sub reap_install_monitor { my ($pid) = @_; @@ -1214,10 +1209,6 @@ if (!(socketpair($rescanreadpipe, $rescanwritepipe, AF_UNIX, SOCK_STREAM, PF_UNS } $rescanrselect = new IO::Select; $rescanrselect->add($rescanreadpipe); -# supervise() records the attempt, blocks SIGCHLD across the fork, and installs $pid_MON and -# $mon_respawn itself while it is still blocked -- which is why they are passed by reference; -# see the sub. This monitor's uptime counts towards the pacing too, so an unrelated death much -# later is retried promptly. xCAT::RespawnUtils::supervise { $$progname = "xcatd: install monitor"; $pid_UDP = 0; @@ -1492,14 +1483,9 @@ my $udpalive = 1; until ($quit) { $SIG{CHLD} = \&ssl_reaper; # set here to ensure that signal handler is not corrupted during loop - # Respawn the install monitor if it has died. It is forked exactly once at startup, and the - # SIGCHLD reaper only clears $pid_MON when it exits -- nothing re-forks it. A single death - # of that child (a stray signal, or a lost socket takeover during an xcatd restart) used to - # leave xcatiport permanently dead while this daemon kept running, so installing nodes could - # no longer report status or request the boot flip until the WHOLE daemon was restarted. - # - # xCAT::RespawnUtils paces the attempts, and never stops allowing them, so the monitor also - # comes back on its own once whatever held the port lets go of it. + # Nothing else re-forks the monitor; the reaper only clears $pid_MON. The pacing never + # stops allowing an attempt, so the monitor also comes back once whatever holds + # xcatiport lets go of it. if (!$pid_MON && !$quit && $sport && xCAT::RespawnUtils::due($mon_respawn, time())) { if (xCAT::RespawnUtils::should_report($mon_respawn)) { xCAT::MsgUtils->message("S", @@ -1512,16 +1498,12 @@ until ($quit) { $pid_UDP = 0; close($listener); close($udpctl); $udpctl = 0; - # This fork happens further down the program than the one at startup, so it also - # inherits what the parent has opened since: the rescanplugins channel. The monitor - # has no use for either end, and holding them is the only way a respawned monitor - # would differ from the one forked at startup. + # This fork is further down the program than the one at startup, so it also + # inherits what the parent opened since: the rescanplugins channel. close($chreadpipe); close($chwritepipe); - # A pending connection is a client socket the parent has accepted and not yet - # handed to a worker. The monitor never serves one, and it outlives the worker - # that does, so a copy left open here holds that client's socket for the life of - # the daemon. + # Client sockets the parent accepted and has not dispatched yet. The monitor + # outlives the worker that serves one. close($_) for @pendingconnections; do_installm_service; xexit(0); @@ -1558,9 +1540,8 @@ until ($quit) { } else { # if select returned with no ready fds, there might be udpctl broken. - # While the install monitor is down, wait in shorter hops: the respawn at the top of - # this loop only gets a turn when this select returns, so on an otherwise idle daemon - # a full 30s wait is added to the respawn delay before xcatiport comes back. + # The respawn at the top of this loop only gets a turn when this select returns, so + # wait in shorter hops while the monitor is down. if (not $bothwatcher->can_read((!$pid_MON && $sport) ? 5 : 30)) { # if the errno is 'bad fd', check the health of the udpctl