Skip to content

Fix SIGPIPE-related failures in testsuite #207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mplegendre merged 2 commits into from
Aug 24, 2026
Merged

Fix SIGPIPE-related failures in testsuite #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e25642c
Send exit message with MSG_NOSIGNAL; ensure log daemon exits between …
nchaimov Aug 20, 2026
88fa5a4
Remove logd_kill_test; relocate logd check
nchaimov Aug 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions src/cobo/cobo_comm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,3 +241,61 @@ int write_msg(int fd, ldcs_message_t *msg)

return 0;
}

/* ll_send_nosignal is a variation on ll_write which uses
* send(..., MSG_NOSIGNAL) to avoid SIGPIPE when writing to
* a broken socket.
*/
int ll_send_nosignal(int fd, void *buf, size_t count)
{
int error;
ssize_t result;
size_t pos = 0;
unsigned char *cbuf = (unsigned char *) buf;

debug_printf3("Have %lu bytes at %p to write to network\n", count, buf);

while (pos < count) {
result = send(fd, cbuf + pos, count - pos, MSG_NOSIGNAL);
debug_printf3("Wrote %d bytes at %p to network: 0x%x 0x%x 0x%x 0x%x 0x%x 0x%x 0x%x 0x%x...\n", (int) result, cbuf + pos,
result > 0 ? ((int) cbuf[pos+0]) : 0,
result > 1 ? ((int) cbuf[pos+1]) : 0,
result > 2 ? ((int) cbuf[pos+2]) : 0,
result > 3 ? ((int) cbuf[pos+3]) : 0,
result > 4 ? ((int) cbuf[pos+4]) : 0,
result > 5 ? ((int) cbuf[pos+5]) : 0,
result > 6 ? ((int) cbuf[pos+6]) : 0,
result > 7 ? ((int) cbuf[pos+7]) : 0);
if (result == -1 && (errno == EINTR || errno == EAGAIN))
continue;
if (result <= 0) {
error = errno;
err_printf("Error writing to cobo FD %d: %s\n", fd, strerror(error));
return -1;
}
pos += result;
}
debug_printf3("Sent %lu bytes to fd %d\n", count, fd);
return 0;
}

/* write_msg_nosignal is a variation on write_msg
* which uses ll_send_nosignal instead of ll_send
* to avoid SIGPIPE when writing to a broken socket.
*/
int write_msg_nosignal(int fd, ldcs_message_t *msg)
{
int result = ll_send_nosignal(fd, msg, sizeof(*msg));
if (result == -1) {
return -1;
}

if (msg->header.len && msg->data) {
result = ll_send_nosignal(fd, msg->data, msg->header.len);
if (result == -1) {
return -1;
}
}

return 0;
}
2 changes: 2 additions & 0 deletions src/cobo/cobo_comm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ Place, Suite 330, Boston, MA 02111-1307 USA
int ldcs_cobo_read_fd(int fd, void* buf, int size);
int ldcs_cobo_write_fd(int fd, void* buf, int size);
int ll_write(int fd, void *buf, size_t count);
int ll_send_nosignal(int fd, void *buf, size_t count);
int ll_read(int fd, void *buf, size_t count);
int write_msg(int fd, ldcs_message_t *msg);
int write_msg_nosignal(int fd, ldcs_message_t *msg);

#endif /* _COBO_COMM_H */
4 changes: 3 additions & 1 deletion src/fe/comlib/cobo_fe_comm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,9 @@ int ldcs_audit_server_fe_md_close ( void *data ) {
out_msg.data = NULL;

cobo_server_get_root_socket(&root_fd);
write_msg(root_fd, &out_msg);
/* We use write_msg_nosignal to avoid SIGPIPE if the socket is broken.
* We're exiting here, so ignore if the server already exited. */
write_msg_nosignal(root_fd, &out_msg);
return cobo_server_close();
}

Expand Down
34 changes: 34 additions & 0 deletions testsuite/run_driver_template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,40 @@ export SPINDLE_TEST=1
export SPINDLE=SPINDLE_EXEC
export PATH=$PATH:.

SESSION_ACTIVE=false
if [ "x$SESSION_ID" != "x" ] || [ "x$SPANK_SPINDLE_USE_SESSION" != "x" ] ; then
SESSION_ACTIVE=true
fi

# If we're not running in a session, wait, on every node in the job, for
# the log daemon to exit before proceeding to the actual test; after a
# delay, forcibly kill it. This ensures that each test connects to a
# fresh daemon.
if [ $SESSION_ACTIVE == false ] && [ "x$1" != "x--end-session" ] ; then
# This is the command run on each node.
LOGD_WAIT_CMD='
SECONDS=0
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

@mplegendre mplegendre Aug 21, 2026

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.

@nchaimov nchaimov Aug 21, 2026

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?

@mplegendre mplegendre Aug 21, 2026

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.

@nchaimov nchaimov Aug 21, 2026

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.

@nchaimov nchaimov Aug 24, 2026

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.

I moved this to the place where it is used. With the removal of the logd_kill_test test, there was only one place where wait_for_logd_exit was called, so it's no longer a separate function, and is instead directly in the body of the if that checks whether a session is used

if [ $SECONDS -ge $TIMEOUT ]; then
echo "WARNING: $(hostname): spindle_logd (pid $PID) still running after $TIMEOUT sec; killing it" >&2

@mplegendre mplegendre Aug 21, 2026

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.

@nchaimov nchaimov Aug 24, 2026

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.

I removed the test, so this warning no longer occurs on every run.

kill -9 "$PID" 2>/dev/null
break
fi
sleep 0.1
done
rm -f "$TMP/spindle_log" "$TMP/spindle_test" "$TMP/spindle_log_lock" "$TMP/spindle_log_reset"
'
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"

@mplegendre mplegendre Aug 21, 2026

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.

elif [ "x$TEST_RM" == "xflux" ] ; then
flux exec -r all bash -c "$LOGD_WAIT_CMD"
fi
fi

if [ $1 == --start-session ] ; then
# With SPANK plugin, sessions are started by argument to salloc/sbatch instead
if [ "x$TEST_RM" == "xslurm-plugin" ] ; then
Expand Down