[nchaimov]

Spindle already had some code that was intended to time out after 60 seconds, but that did not actually work. These were:

• COBO connect limit: COBO defines a default time limit of 60 seconds

  Spindle/src/cobo/cobo.c

  Lines 54 to 56 in 594befc

  	#ifndef COBO_CONNECT_TIMELIMIT
  	#define COBO_CONNECT_TIMELIMIT (60) /* seconds -- wait this long before giving up for good */
  	#endif

  which is used in cobo_connect_hostname to give up after that many seconds and log an error, then returning -1 if the socket was not successfully opened:

  Spindle/src/cobo/cobo.c

  Line 494 in 594befc

  while (!connected && secs < cobo_connect_timelimit) {

  Spindle/src/cobo/cobo.c

  Lines 593 to 607 in 594befc

  	/* compute how many seconds we've spent trying to connect */
  	cobo_gettimeofday(&end);
  	secs = cobo_getsecs(&end, &start);
  	if (secs >= cobo_connect_timelimit) {
  	err_printf("Time limit to connect to rank %d on %s expired\n",
  	rank, hostname);
  	}
  	}
  	/* check that we successfully opened a socket */
  	if (s == -1) {
  	err_printf("Connecting socket to %s at %s failed\n",
  	hostname, inet_ntoa(saddr));
  	return s;
  	}

  However, the return value was thrown away: cobo_connect_hostname would return to cobo_open_tree which would pass along the -1, but cobo_open did not check the return value and proceeded into a barrier, which would then hang forever:

  Spindle/src/cobo/cobo.c

  Lines 1530 to 1537 in 594befc

  	/* open the tree */
  	cobo_open_tree();
  	/* need to check that tree opened successfully before returning, so do a barrier */
  	if (cobo_barrier() != COBO_SUCCESS) {
  	err_printf("Failed to open tree\n");
  	return -1;
  	}

  Additionally, the environment variable that was meant to override the default time limit was never read because the variable name had \n at the end of it:

  Spindle/src/cobo/cobo.c

  Lines 1513 to 1516 in 594befc

  	/* seconds */
  	if ((value = cobo_getenv("COBO_CONNECT_TIMELIMIT\n", ENV_OPTIONAL))) {
  	cobo_connect_timelimit = (double) atoi(value);
  	}

• Frontend startup timeout: spindle_fe.cc defines a startup timeout of 60 seconds:

  Spindle/src/fe/startup/spindle_fe.cc

  Line 45 in 594befc

  #define STARTUP_TIMEOUT 60

  This is passed to ldcs_audit_server_fe_md_waitfor_alive as the time to wait for the alive message in sendAndWaitForAlive():

  Spindle/src/fe/startup/spindle_fe.cc

  Lines 511 to 516 in 594befc

  	result = ldcs_audit_server_fe_md_waitfor_alive(STARTUP_TIMEOUT);
  	if (result == -1) {
  	debug_printf("Failure waiting for alive message\n");
  	return false;
  	}
  	return true;

  This would return false if the time ran out; however, the caller, spindleInitFE, did not check the return value of sendAndWaitForAlive() and returned 0 unconditionally:

  Spindle/src/fe/startup/spindle_fe.cc

  Lines 439 to 442 in 594befc

  	/* Wait for servers to indicate startup */
  	sendAndWaitForAlive();
  	return 0;

• Client ready wait: The client repeatedly tries to stat the ready file and waits 0.1 seconds between stats. There is a limit of 600 retries (so a 60 second limit). However, nothing checks whether the retries exceeded the limit, so in the event that the 600 retries are used, it falls through to continuing client startup and proceeds to try to create the incoming FIFO:

  Spindle/src/client/client_comlib/client_api_pipe.c

  Lines 208 to 223 in 594befc

  	/* wait for directory (at most one minute) */
  	stat_cnt = 0;
  	snprintf(ready, MAX_PATH_LEN, "%s/spindle_comm/ready", location);
  	memset(&st, 0, sizeof(st));
  	while (((stat(ready, &st) == -1) || ((st.st_mode & (S_IRUSR | S_IWUSR)) == 0)) &&
  	(stat_cnt<600)) {
  	if (stat_cnt % 10 == 0)
  	debug_printf3("waiting: location %s does not exists (after %d seconds)\n", ready, stat_cnt/10);
  	usleep(100000); /* .1 seconds */
  	stat_cnt++;
  	}
  	/* create incomming fifo */
  	sprintf(fifo, "%s/spindle_comm/fifo-%d-0", location, getpid());
  	result = ldcs_mkfifo(fifo);

  , which blocks indefinitely until the other end opens it, which may never happen.

This PR makes changes to Spindle startup so that startup either succeeds or fails within a bounded time limit. This is done by 1) fixing the time limits that were already in the code but that had no effect; 2) ensuring that error conditions during startup return non-zero and that these failures propagate back to the original initialization call; and 3) by using non-blocking I/O for sockets and FIFOs so that calls to accept() and FIFO opens can't hang indefinitely. The specific changes are:

• The functionality of COBO_CONNECT_TIMELIMIT is restored and applies to the startup process through the end of cachepath consensus. Failures in COBO tree setup are propagated to the caller instead of being ignored.
• spindleInitFE returns -1 when the tree cannot be formed, the settings cannot be sent, or ALIVE does not arrive within the time limit.
• FE broadcasts no longer raise SIGPIPE so that the FE can detect a dead root server instead of being killed by SIGPIPE.
• A new env var is added for controlling the time the client will wait for the ready file, SPINDLE_READY_TIMELIMIT. The functionality of the ready file time limit is restored.
• spindle_bootstrap exits nonzero if its rank info or cache path query fails; it previously silently continued.
• Servers no longer stop reading from their parent after a failed forward to a child, as this caused an indefinite hang.
• spindle --start-session exits nonzero when the session daemon exits before it finishes starting.
• The Flux and Slurm plugins now pass COBO_CONNECT_TIMELIMIT through to the servers.
• In cobo_open_tree the server's listen socket is now nonblocking and polls for its parent until the deadline; this prevents an indefinite hang in accept()
• In client_open_connection_pipe the client now opens its input FIFO nonblocking before creating the output FIFO and opening it nonblocking. This is retried on ENXIO until the server has opened the read end or the time limit expires.
• Unify duplicate copies of COBO fd helper function. There were two substantially identical copies of the helpers in different files that differed only in that a bug fix had been applied to one but not the other.

To test this, a new test suite is added. Fault injection points are added to Spindle controlled by environment variables:

• SPINDLE_TEST_BE_EXIT_HOSTS=host1,host2,...: BEs on these hosts exit as soon as they enter cobo_open, before listening for their parent.
• SPINDLE_TEST_BE_FAULT_PHASE=tree|ready: for the below variables, if tree, exit or stall during tree formation (after the hostlist has been read from the parent); if ready, exit or stall just after the ready file has been created.
• SPINDLE_TEST_BE_EXIT=1,2,...: BEs with these COBO ranks will exit at the fault phase.
• SPINDLE_TEST_BE_STALL=1,2,...: BEs with these COBO ranks will stall at the fault phase.
• SPINDLE_TEST_BE_STALL_SECS=N: BEs that stall will stall for the given number of seconds.

These modes are tested with the new startup failure test driver run_startup_fault_tests.sh. Because the tests can take as long to run as all the existing tests combined, they are currently not enabled as part of the standard CI. Instead, the new tests run only upon request. It is not possible to run them on request until the changes have been merged into the default branch. I have run it manually on my own branch by temporarily changing the default branch so that we can see the results of the startup tests before merging. For those results, see https://github.com/ParaToolsInc/Spindle/actions/runs/41743592538

Most of the tests actually complete relatively quickly, except for the SPANK plugin case, where the startup tests take 10m46sec in the above run. This is due to the SPANK plugin itself adding a 30 second delay while it tries to stop a frontend that never started. If we can get rid of that delay, then the slowest of the tests would be only 6 minutes.

I've also run the startup fault tests on Tuolumne and they pass there, as does the standard testsuite. If running the startup fault tests manually, note that they require a minimum of 3 nodes.

While failures are logged in the debug log, if enabled, there is not yet any attempt to determine which nodes failed and to present a list of them to the user. Error reporting will come in a future PR.

[nchaimov]

Restructured fault point to place an explicit check for environment variables up front and explicitly return early if none is set.

[mplegendre]

Can we make this a define, one set through autoconf. I'm worried we may need to shift the time around for large systems, and I don't want to have to go back to the code.

I see we got the environment variable, but if someone's doing a system install that's not going to be set for users. Config system would be ideal, but we obviously aren't setup to read configs yet.

[mplegendre]

It's not obvious to me that we'd need to make any of these autoconf, ala the above comment about the timeouts.

But if you think making the defaults for these autoconf would be something an HPC center would need to do, then we should do so.