Skip to content

fix(xcat-core): one node's request to the install monitor blocks every other node - #7853

Open
dhilst wants to merge 4 commits into
xcat2:masterfrom
VersatusHPC:fix/installmon-per-node-fork
Open

dhilst wants to merge 4 commits into
xcat2:masterfrom
VersatusHPC:fix/installmon-per-node-fork

Conversation

@dhilst

@dhilst dhilst commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

The xcatd install monitor serves every installing node from one accept loop. It accepts a
connection, resolves the peer to a node and dispatches the request in line, so nothing else is
accepted until that request returns. On a flat el9 cluster, with one node holding three
connections that send nothing, a second node waited 5.7 seconds for the monitor's greeting.
The wait scales with the load: 1.7 s for one held connection, 9.7 s for five.

do_installm_service in xCAT-server/sbin/xcatd calls plugin_command directly in every branch --
next, installstatus, getpostscript, syncfiles, hpcbootstatus, locktftpdir. The per-request fork
that once stood there is commented out with the note that the node must be blocked, because
'nodeset next' and 'installstatus' for one node write the same chain row.

The monitor now gives each connection its own child and keeps that ordering per node: the child
for a node reads a pipe left by the previous child for the same node, and starts when that pipe
reaches end of file. Live children are capped at 64 and the rest wait in the listen backlog. The
parent reaps them and releases the gate, and a child that dies no longer takes the monitor with
it.

xCAT-test/unit/xcatd_install_monitor_concurrency.t lifts do_installm_service out of the program
and drives it with real clients on a port of its own. Without this change a second node waits
5.7 seconds for its greeting and the monitor does not survive a request that kills its handler;
with it the second node waits 0.004 s. Removing the pipe gate turns the ordering assertion red,
and raising the limit turns the backlog assertion red. Measured on the lab management node with
the patched binary installed, a second node waits 0.008 s against 5.716 s on the stock binary,
and a real getpostscript from the compute node is served by a forked handler

fix #7852

…ry other node

The xcatd install monitor accepts a connection, resolves the peer to a node and
dispatches the request in line. Nothing else is accepted until that request
returns, so a node whose 'nodeset next' takes three seconds costs every other
installing node three seconds. A request that kills the process serving it takes
the whole monitor down with it.

do_installm_service in xCAT-server/sbin/xcatd is one process with one accept
loop. Every branch calls plugin_command directly. The per-request fork that once
stood there is commented out, because two requests for one node write the same
chain row and must not overlap.

This commit adds the test only. xcatd_install_monitor_concurrency.t lifts
do_installm_service out of the program, runs it on a port of its own against
stand-in plugins, and drives it with real clients: one node holds a three-second
request, a second node times its greeting, two requests for one node are checked
for overlap, and one request kills the process that serves it.

The test fails on this tree. The second node waits 5.7 seconds for its greeting,
and the monitor does not survive a request that kills its handler.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
…y other node

The xcatd install monitor serves every installing node from one accept loop. It
accepts a connection, resolves the peer to a node and dispatches the request in
line, so nothing else is accepted until that request returns. On a management
node holding three connections that send nothing, a second node waited 5.7
seconds for the monitor's greeting.

do_installm_service in xCAT-server/sbin/xcatd calls plugin_command directly in
every branch. The per-request fork that once stood there is commented out with
the note that the node must be blocked, because 'nodeset next' and
'installstatus' for one node write the same chain row.

The monitor now gives each connection its own child and keeps that ordering per
node: the child for a node reads a pipe left by the previous child for the same
node, and starts when that pipe reaches end of file. Live children are capped at
64 and the rest wait in the listen backlog, the parent reaps them, and a child
that dies no longer takes the monitor with it. A per-node lock file was rejected
because it needs a new directory and gives no arrival order; letting the parent
wait for the busy node was rejected because it blocks the accept loop again.

xcatd_install_monitor_concurrency.t lifts do_installm_service out of the program
and drives it with real clients. Without this change the second node waits 5.7
seconds and the monitor does not survive a request that kills its handler.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst dhilst self-assigned this Sep 15, 2026
@dhilst
dhilst marked this pull request as draft September 15, 2026 16:29
@dhilst
dhilst marked this pull request as ready for review September 15, 2026 16:33
@dhilst dhilst added this to the 2.19 milestone Sep 15, 2026
…ile it reads is missing

xcatd_install_monitor_concurrency.t called plan skip_all when xCAT-server/sbin/xcatd was absent, so a checkout that lost
the file reported 0 tests and exit 0. A test that cannot fail measures nothing.

Die instead, which is what makentp_ntp_deps.t already does for setupntp.

With xCAT-server/sbin/xcatd moved aside the file now exits 2 and prints "xcatd not found at <path>";
before this change it exited 0 and printed "1..0 # SKIP xcatd not found at <path>". With the file
present the test passes either way.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>

@viniciusferrao viniciusferrao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Daniel,

  1. Could we preserve per-node ordering when a waiting child dies?
    With A running and B and C queued for the same node, killing B
    closes its pipe and lets C run before A finishes. The fork-failure
    fallback also closes the predecessor without waiting. Both paths
    can reintroduce the chain-row race. Please keep later requests
    behind the earlier work, with regression tests for these cases.

  2. Could the concurrency test use a scratch PID file instead of
    /var/run/xcat/installservice.pid? Running it as root overwrites
    the live monitor’s PID file. The capped-monitor case also leaves
    its dead PID behind, since the restore happens before that case.
    Please redirect every PID-file access in the lifted service,
    rather than saving and restoring the host’s file.

Thanks!

…hildren

The ordering between two requests for one node was a chain of pipes: each child
held the write end and the next child for that node read the previous one to end
of file. End of file there means the previous process is gone, not that its work
finished, so a child that died released the one behind it -- and released the
wrong one, because each child waits on its immediate predecessor rather than on
the request actually in flight. The fork-failure path closed the predecessor and
served the request in line without waiting at all.

The parent now owns the order. %installm_busy names the handler serving a node,
%installm_queue holds the connections accepted for that node meanwhile, and the
next one is forked when the handler ahead of it is reaped. A handler that dies
cannot release the one behind it, and the fork-failure path has nothing to fall
back over, because it is reached only when the node has no handler.

Three other changes the same design makes possible or necessary:

  - The answer to a destiny advance now follows the advance. Every other request
    is still answered before it runs, because its result does not change what
    the node does next. Holding one node costs no other node anything now, and
    it lets the node retry an advance whose handler died -- which the old order
    could not, because "done" was already on the wire.
  - The monitor drains its handlers before it exits. Without this a restart
    orphans them into the systemd service cgroup, where anything still running
    at TimeoutStopSec is killed after its answer was already sent.
  - SIGCHLD is caught, so a handler exiting interrupts accept and the parent
    comes back to look for a connection queued for that node.

The pid file path is a variable, so the test can point the lifted routine at a
scratch file instead of the one a restarting xcatd reads to tell the running
monitor to let go of the port.

xcatd_install_monitor_concurrency.t grew the cases for all of it. Four
mutations, each caught by one assertion: forking every connection at once turns
the ordering case red; answering a destiny advance before the plugin turns the
release case red; removing the drain turns the stand-down case red; and leaving
the emptied queue entry behind turns the leak case red. The full unit suite is
194 files, 5662 tests, green.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>
@dhilst dhilst modified the milestones: 2.19, 2.19.1 Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants