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>
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 <pid>"), 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>
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>
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>
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>
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/<pid>/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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Cover the order that names the install device: installnic, then primarynic,
then mac.mac. Either attribute may name an interface or carry an address, and
the keyword mac returns to mac.mac.
Cover the device the kickstart names for each of those inputs. A node that
sets neither attribute keeps the address it has today. Cover the defect the
change closes, where a mac.mac entry that holds several untagged addresses
resolves to the last of them.
Cover that Ubuntu keeps its own pair of a name and an address over the same
resolution, and that the unique local address still comes from the hardware
address.
noderes.installnic names the adapter that deploys the operating system. The
kickstart network line ignored it and named the adapter from mac.mac alone, so
a node that sets installnic got a kernel command line that obeys installnic and
a kickstart that configures a different adapter. On a node with more than one
adapter the installer then brings up the wrong one and cannot reach the
repository.
Name the device from the shared resolution, which gives the interface name when
installnic or primarynic names one, and the address otherwise. A node that sets
neither attribute keeps the address it has today.
The unique local address still comes from the hardware address, because
autoulaaddress builds the address from it.
The order that names the install device is noderes.installnic, then
noderes.primarynic, then mac.mac. gen_net_boot_params owns that order for the
netboot kernel parameters. Only the Ubuntu template reused it. Every other
install template reads mac.mac on its own.
Move the reuse into install_device_params so that any install template can
share it. subiquity_install_netcfg keeps its own name and its own return
value, because netplan needs the pair of a name and an address. Behaviour
does not change.
Five comments in the discovery path explained a consequence chain or
justified the code to a reviewer rather than stating what is not visible
locally: the payload protocol the code below already shows, a rationale
for the missing boot loader message, the two Genesis compression
variants, the HTTP and TFTP entry pair, and the :noboot divergence from
the PXELINUX files. Say the non-obvious part once.
Comments only; no executable line changes.
Firmware configured for UEFI HTTP boot sends client architecture 28
(0x001c) and only accepts an offer whose boot file is a URL and whose
reply carries the HTTPClient vendor class; it ignores the PXE offer that
answers architecture 27. riscv64 nodes therefore could not boot at all
from firmware set up that way.
Answer them with the same grub2 image over HTTP. The URL has to name the
management node address of the network the request came in on, so the
class belongs to the subnet, like the POWER OPAL and xNBA network classes
next to it, and it is only offered while the boot loader is actually
published. The ISC backend renders the matching subnet branch.
This covers every client without a reservation, which is the discovery
case. A node that nodeset has configured keeps its per-node boot loader
over TFTP, as on the other architectures.
The option flags of a client class -- always-send and its siblings -- were
passed through as whatever the caller set, so a plain Perl 1 reached the
configuration as the number 1 and Kea refuses to parse that. The class
flag next to it is already normalised; do the same for the option data, so
callers can stay free of JSON.
The grub2 discovery configuration loaded the Genesis kernel and initramfs
over TFTP, a lockstep protocol that acknowledges every block and runs one
server process per client. Fetching the same 79 MiB Genesis image from a
node on the lab network took 61.5 s over TFTP and 1.2 s over HTTP, and a
whole cluster discovering at once queues on the TFTP server.
Write two entries instead. The default one sets root to the HTTP server
of the management node on that network and loads the same files from
below the TFTP root, the way nodeset does for netboot=grub2-http; the
second keeps the TFTP paths for a management node that does not serve the
TFTP root over HTTP, and "set fallback=1" moves to it when GRUB cannot
fetch the payload over HTTP. site.httpport is honoured.
riscv64 nodes boot through UEFI and grub2 only, and xCAT builds no boot
loader: /tftpboot/boot/grub2/grub2.riscv64 has to come from the xcat-dep
grub2-xcat package or be copied by hand, which is a step an admin only
finds out about when a node times out in firmware.
The EL riscv64 media carry exactly that image as EFI/BOOT/grubriscv64.efi,
so copycd publishes it after a successful media copy, and says so. An
image the management node already has is never replaced, and the media of
every other architecture is untouched.
Explain why the riscv64 templates disable the installer's kdump add-on,
how to reserve memory for crash dumps on an installed node, and what
diskless images reserve by default.
Daniel Hilst
Vinícius Ferrão