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] 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) {