Fix SIGPIPE-related failures in testsuite - #207
Conversation
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Should we kill the log daemon only when we're not in a session?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
This PR fixes two kinds of failures we were encountering in the testsuite:
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 withsend(..., MSG_NOSIGNAL)to avoid SIGPIPE if the socket is broken.srun --overlaporflux execare 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.