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