Skip to content

Fix SIGPIPE-related failures in testsuite - #207

Open
nchaimov wants to merge 1 commit into
llnl:develfrom
ParaToolsInc:sigpipe-fixes
Open

Fix SIGPIPE-related failures in testsuite#207
nchaimov wants to merge 1 commit into
llnl:develfrom
ParaToolsInc:sigpipe-fixes

Conversation

@nchaimov

Copy link
Copy Markdown
Collaborator

This PR fixes two kinds of failures we were encountering in the testsuite:

  • With the srun wrapper launcher, where the servers are launched via srun, when the job exits, Slurm kills any still-running steps. This could race between the frontend and servers, such that the root server had already exited by the time the frontend sent it a message informing it to exit. This is fixed by sending the exit message with send(..., MSG_NOSIGNAL) to avoid SIGPIPE if the socket is broken.
  • When tests were running, a race condition exists where one test could exit, its log daemon could commit to exiting, and another test could start, and connect to the log daemon, before it completed exiting. The daemon would then exit while the next test was running. This is fixed by having the test driver check for running log daemons before running a test, and killing the daemons itself if they don't exit within 15 seconds. The test driver checks for the daemon on the node where the script is running, and srun --overlap or flux exec are used to perform the check on all compute nodes of the allocation. A new test is added which starts a log daemon and verifies that the check successfully kills it.

TMP="${TMPDIR:-${TEMPDIR:-/tmp}}"
LOCK="$TMP/spindle_log_lock"
TIMEOUT="${SPINDLE_LOGD_SHUTDOWN_TIMEOUT:-15}"
while PID=$(cat "$LOCK" 2>/dev/null) && kill -0 "$PID" 2>/dev/null ; do

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.

I'm waiting in a queue to test, so I haven't verified this. But I think this code will break debug logging for spindle session tests. We used to get the debug logs integrated for all session tests (which is probably how you want to debug session problems), and this will break that into multiple logs.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Should we kill the log daemon only when we're not in a session?

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.

Yes. Though since writing the above comment I got a node to test with, and it's not doing that....

And now I see more clearly that I'd mis-read the quoting on LOGD_WAIT_CMD='...'. And all of that LOGD_WAIT_CMD code will only run if the "if [ $SESSION_ACTIVE == false ] && [ "x$1" != "x--end-session" ] ;" condition triggers. So that's all correct.

I will say that it's a little easy to mis-read and think that LOGD_WAIT_CMD contents will run with every invocation of run_driver. What about moving the setting of the LOGD_WAIT_CMD variable to inside the wait_for_logd_exit() function? Just to make it a bit clearer at a glance when that code runs.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I see. Yes, I'll move it into the function. It's stored in a variable to begin with because we pass it to bash -c invocations on the compute nodes.

TIMEOUT="${SPINDLE_LOGD_SHUTDOWN_TIMEOUT:-15}"
while PID=$(cat "$LOCK" 2>/dev/null) && kill -0 "$PID" 2>/dev/null ; do
if [ $SECONDS -ge $TIMEOUT ]; then
echo "WARNING: $(hostname): spindle_logd (pid $PID) still running after $TIMEOUT sec; killing it" >&2

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.

I'm testing serially, and getting this warning with every test run:

Running: ./logd_kill_test
WARNING: tuolumne2149: spindle_logd (pid 225509) still running after 2 sec; killing it
./logd_kill_test: line 33: 225509 Killed                  "$LOGD_BIN" "$LOGD_TMP" -test spindle_test
PASSED

From glancing at the code, I think this will trigger with every test suite run. Suggest cleaning up the warning prints, or clean up the way the test runs so it doesn't go down this error path.

if test "x$SKIP_NONSESSION" != "xtrue"; then

./logd_kill_test
CHECK_RETCODE

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.

I'm not convinced this needed a dedicated test. It's testing test infrastructure--if this fails we presumably will see other test failures. I'm also not overly concerned with a 'kill' command failing to kill the logger.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This was mostly to convince myself that the kill code actually works. We can get rid of it from the PR, in which case we'd also no longer produce the warning when this test runs

wait_for_logd_exit() {
bash -c "$LOGD_WAIT_CMD"
if [ "x$TEST_RM" == "xslurm" -o "x$TEST_RM" == "xslurm-plugin" ] && [ "x$SLURM_NNODES" != "x" ] ; then
srun --overlap -N $SLURM_NNODES -n $SLURM_NNODES bash -c "$LOGD_WAIT_CMD"

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.

This PR didn't introduce TEST_RM to the test scripts, but the expanded usage here is revealing the weaknesses around our system for selecting what RM to use in tests.

It used to be that TEST_RM was a only build-time flag that just determined which test_driver_${TEST_RM} file got copied to test_driver_rm installation location. And we could switch RMs (I'd usually switch between serial and the system RM) by manually copying a different file to the installation location. Now we have to switch RMs with two steps that have to agree: Update TEST_RM in runTests and copy the desired file.

Let's fix this in another PR. But let's get back to needing to change only one thing to switch RMs. My suggestion is to:

  • Stop using TEST_RM at build time to determine which test_driver_${TEST_RM} file to install. Instead install all files.
  • Start using TEST_RM at test runtime to select which of the many test_driver_{TEST_RM} to execute.
  • Add a flag to runTests letting the user pick which RM to use.
  • Add a config setting the default value TEST_RM. Perhaps we could grep that out of the config file, since the testsuite doesn't have access to the normal config parser.

I'll put this into a new issue. No need to do anything about this in this PR.

# run_driver will kill it

cd TEST_RUN_DIR
export SPINDLE=SPINDLE_EXEC

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.

The environment variable $SPINDLE is already used as an public-interface indicator to applications of whether spindle is enabled in their process. While this testsuite usage doesn't seem to be causing problems (spindle probably just overwrites this value), I'd still suggest moving to another variable name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants