xCAT/xcat-core
2
0
Fork 0
mirror of https://github.com/xcat2/xcat-core.git synced 2026-09-05 04:27:55 +00:00
Code Issues Releases Wiki Activity

refactor(xcat-core): the fork-and-account sequence is open-coded at both fork sites

Browse Source 
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>
This commit is contained in:
Daniel Hilst
2026-08-27 17:05:08 -03:00
parent dfdc4f4293
commit c62434d22d
3 changed files with 76 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
perl-xCAT/xCAT/RespawnUtils.pm
+52
View File
@@ -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;
xCAT-server/sbin/xcatd
+15 -29
View File
@@ -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
xCAT-test/unit/xcatd_monitor_respawn.t
+9 -8
View File
@@ -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 );
};