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] 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 ); };