Commpath by rountree · Pull Request #182 · llnl/Spindle

rountree · 2026-05-22T07:59:10Z

Passed all tests on my clone. Let's see what it does over here.

Previously, chosen_realized_cachepath was copied into set_intercept_readlink_cachepath() chosen_realized_cachepath and chosen_parsed_cachepath were copied into set_should_intercept_cachepath() This PR removes both setter functions and makes the original pointers global.

Removes chosen_cachepath and cachepath_bitindex from spindle_launch.h Updates initialization of matching variables in ldcs_process_data. determineValidCachePaths() moved from spindle_be.cc to ldcs_audit_server_process.c to get ldcs_process_data visibility. Added #include "parseloc.h" to ldcs_audit_server_process.c to get declaration of determineValidCachePaths(). Relocated "parseloc.h" to src/util so ldcs_audit_server_process.c could find it. Trued up signedness of types caused my making "parseloc.h" more visible, e.g., cachepath_bitidx is now uint64_t everywhere.

The three-message-reply response is now a single message with two strings. The symbolic version of the cachepath is no longer communicated as it was not being used.

New name is ldcs_audit_server_md_allreduce_AND(). If we get to the point where we're using other allreduce operations we can solve the problem of duplicating the op list in md-land and cobo-land. For now, we're only using one op in md-land, so the op can go into the function name.

Unlikely it would ever make a difference, but this is much more correct.

src/client/beboot/spindle_bootstrap.c Moved orig_location from static global to local Renamed symbolic_location to symbolic_commpath Renamed orig_location to orig_commpath Renamed location to commpath Renamed LDCS_LOCATION to LDCS_COMMPATH src/client/client/client.c Renamed LDCS_LOCATION to LDCS_COMMPATH Renamed location to commpath

src/client/client_comlib/client_api.c Added setenv("LDCS_CHOSEN_PARSED_CACHEPATH", local_cpc); testsuite/test_driver.c Replaced LDCS_LOCATION and LDCS_ORIG_LOCATION checks for cachepath with LDCS_CHOSEN_PARSED_CACHEPATH Replaced spindle_loc with cachepath

All tests pass with both distinct and identical commpaths/cachepaths.

Additionally populates /etc/environment just in case ssh is used to set up the servers.

The option is marked as obsolete in configure --help and will cause an error in configure if it is specified. As updates the CI configure scripts to use --with-cachepaths and --with-commpath instead of --with-localstorage.

Replaces args->location with args->commpath.

Additional integration for commpath + spank-plugin.

TMPDIR left out of a docker script, and --cachepath instead of --cachepaths.

This commit fixes a race condition where eager client processes can submit a LDCS_MSG_CHOSEN_CACHEPATH_REQUEST before the servers have come to a consensus. When that is the case, the server now responds with LDCS_MSG_NO_CACHEPATH_CONSENSUS_YET and the client sleeps for one second (max 10 retries) before sending the message again. 1) include/ldcs_api.h Added LDCS_MSG_NO_CACHEPATH_CONSENSUS_YET 2) server/comlib/ldcs_api_util.c Added STR_CASE entry for that message. 3) server/auditserver/ldcs_audit_server_handlers.c Added global "static bool cachepath_consensus_reached" above handle_cachepath_consensus() Set that variable to true inside handle_cachepath_consensus() In handle_chosen_cachepath_request(), made msg.header.type conditional on cachepath_consensus_reached 4) client/client_comlib/client_api.c This is the only place LDCS_MSG_CHOSEN_CACHEPATH_REQUEST is used. Will sleep for 1 second after each LDCS_MSG_NO_CACHEPATH_CONSENSUS_YET message.

The theory being that eager clients are using an uninitialized cachepath variable. By delaying the consensus, the failure should happen more often.

"sending message of type: request_location_path" is now "sending message of type: CHOSEN_CACHEPATH_REQUEST"

Known to affect the symbolic form of candidate cachepaths. Not sure that's ever being used, but it's fixed now.

_message_type_to_str() can now be used in cobo_fe_comm.c. ldcs_audit_server_fe_broadcast() now reports message type. Only two messages are expected to be routed through there, but it's the correct way to report it.

Cleanup now takes both commpath and cachepath and prefixes for removing files created by Spindle.

The original LDCS_LOCATION_MOD checked to see if there were multiple servers running on a node and, if so, modified the location string so that each server had its own location. The code did not handle the case where the directory above the requested directory was not writeable, e.g., if the user passed in --location=/tmp, the code would try to create a directory /tmp-00 for the first server. That fails. With commpath and cachepath replacing location, and with new initialization paths, the existing code would modify only commpath after the commpath directory had been created. If the multiple-server case needs to be supported, commpath- and cachepath-specific code needs to be added back in.

That configure parameter is no longer supported. Replaced with --with-cachepaths=/tmp/commpath/cachepath --with-commpath=/tmp/commpath

mplegendre · 2026-05-27T16:58:25Z


+   char *chosen_parsed_cachepath = NULL;
+   send_cachepath_query( ldcsid , NULL, &chosen_parsed_cachepath);
+   assert( chosen_parsed_cachepath );


Don't assert. Instead do an err_printf and return -1.

mplegendre · 2026-05-27T17:01:26Z

   }
-
+   send_cachepath_query( ldcsid, &chosen_realized_cachepath, &chosen_parsed_cachepath );
+   assert( chosen_realized_cachepath );


Again, don't assert in the client. err_printf and return -1.

mplegendre · 2026-05-27T17:01:38Z

   char *orig_file_name = (char *) name;
   if (is_in_spindle_cache(name)) {
-      debug_printf2("Library %s is in spindle cache (%s). Translating request\n", name, location);
+      assert( chosen_realized_cachepath );


Don't assert

mplegendre · 2026-05-27T17:03:21Z

+   extern char *chosen_realized_cachepath;

-   location_len = strlen(location);   
+   assert( chosen_realized_cachepath );


Remove assert

mplegendre · 2026-05-27T17:04:05Z

+   static int cachepath_size = 0;
+   static int orig_cachepath_size = 0;
+   extern char *chosen_realized_cachepath, *chosen_parsed_cachepath;
+   assert( chosen_realized_cachepath );


Don't assert

mplegendre · 2026-05-27T17:09:31Z

+      return -1;
+   }
+   char *local_crc = strdup( buffer );
+   char *local_cpc = strdup( &buffer[ strlen(local_crc) + 1 ] );


spindle_strdup instead of strdup on the above two lines

mplegendre · 2026-05-27T17:11:57Z

                   case HSHAKE_INTERNAL_ERROR:
                      err_printf("Internal error doing handshake: %s", spindle_handshake_last_error_str());
-                      exit(-1);
+                      return -1;


Revert this back to exit(-1).

The handshake is security checks. If things go wrong here, we just want to exit out and stop. We don't know if an attacker might be able to trigger the HSHAKE_INTERNAL_ERROR, and if they do we're better off just stopping the process.

mplegendre · 2026-05-27T17:12:23Z

           case HSHAKE_INTERNAL_ERROR:
              err_printf("Internal error doing handshake: %s", spindle_handshake_last_error_str());
-              exit(-1);
+              return -1;


Revert back to exit(-1)

mplegendre · 2026-05-27T17:27:37Z

+
+    int num_children = ldcs_audit_server_md_get_num_children(procdata);
+
+    debug_printf( "Processing REQUEST_CACHEPATH_CONSENSUS.\n" );


These debug_printfs are all at level1, but should be level 2-3.
level 1 should just be a single statement for the high-level operations. One or two "running consensus on ..." or "consensus reached on ..." would be appropriate.
level 2 should be the major operations within the high-level opertions. Many of these printfs should be level 2.
level 3 should be for the gross details.

mplegendre · 2026-05-27T17:30:48Z

+   errno=0;
+   while( stat( origpath, buf ) == -1 ){
+      local_errno = errno;
+      debug_printf("Failed to stat '%s' (%s).\n", origpath, strerror(local_errno));


Should probably be a debug level 3 printf

rountree-alt added 30 commits May 21, 2026 22:17

Cachepath: parse_loc utilities.

1411d6d

Cachepath: remove/rename [orig_]location.

75a8fc2

Cachepath: Configure-time support.

8aeec38

Cachepath: Internal messaging for path resolution

52ffab9

Cachepath: Adds cobo_allreduce()

41ddcf4

Cachepath: Adds parameters to config_mgr

093f0f6

Cachepath: adds flux parameter support

6ed3ac2

Cachepath: Adds logging support.

57b480c

Cachepath: Removes out-of-root cleanup checks.

15164a5

Cachepath: Set of small, miscellaneous patches.

091407f

Fixes per Matt's comments.

b2a8c53

Comments the cachepath variables.

9183447

Client cachepath message now uses single response.

0edeac9

The three-message-reply response is now a single message with two strings. The symbolic version of the cachepath is no longer communicated as it was not being used.

Removes assert(0) in network error paths.

c9594a5

Adds explicit enum values to CmdlineShortOptions.

fed846b

Return instead of exit on network errors.

c6ae16b

Apply rename to configuration and parameters.

05e78b5

Renaming location variables/fields to commpath.

8c1067e

Renames enums.

90fa1ec

Use strdup() for commpath instead of stack var.

c271b22

Unlikely it would ever make a difference, but this is much more correct.

Adds LDCS_COMMPATH

82d6118

Restores checkLinkForLeak() to test_driver.c

76c4ce1

Continues location rename.

41b4467

src/client/client_comlib/client_api.c Added setenv("LDCS_CHOSEN_PARSED_CACHEPATH", local_cpc); testsuite/test_driver.c Replaced LDCS_LOCATION and LDCS_ORIG_LOCATION checks for cachepath with LDCS_CHOSEN_PARSED_CACHEPATH Replaced spindle_loc with cachepath

Fixes -Wsign-compare warning in new code.

871e7c2

LDCS_CHOSEN_PARSED_CACHEPATH set in bootstrap.

9762ca6

Updates test_driver.c to ignore FIFO files.

6b0e13d

All tests pass with both distinct and identical commpaths/cachepaths.

rountree-alt added 23 commits May 21, 2026 22:17

Sets TMPDIR=/tmp in each Dockerfile

291d129

Additionally populates /etc/environment just in case ssh is used to set up the servers.

Restores --with-localstorage to generate error.

31e1deb

The option is marked as obsolete in configure --help and will cause an error in configure if it is specified. As updates the CI configure scripts to use --with-cachepaths and --with-commpath instead of --with-localstorage.

Updates spank plugin to use commpath.

e11f36b

Replaces args->location with args->commpath.

Updates a configure script to use commpath.

be289da

Rebasing commpath on devel

825e947

Additional integration for commpath + spank-plugin.

Fixes two silly bugs.

c97e946

TMPDIR left out of a docker script, and --cachepath instead of --cachepaths.

Testing non-overlapping cache/commpath directories.

a2351a1

Testing commpath as subdirectory of cachepath.

f7c739b

Testing cachepath as a subdirectory of commpath.

8137cc0

ci.yml Forcing commpath to use current devel ver.

5903772

Changes bool/true to int/1

8baee82

Adds delay to flush out consensus race condition.

5585081

The theory being that eager clients are using an uninitialized cachepath variable. By delaying the consensus, the failure should happen more often.

Adds lots of asserts around cachepath variables.

e1e707b

FIX: Update debug3 msg text

3712be1

"sending message of type: request_location_path" is now "sending message of type: CHOSEN_CACHEPATH_REQUEST"

FIX: Fixes use-after-free error

caa057b

Known to affect the symbolic form of candidate cachepaths. Not sure that's ever being used, but it's fixed now.

FIX: Greater message detail in FE code

3b4503c

_message_type_to_str() can now be used in cobo_fe_comm.c. ldcs_audit_server_fe_broadcast() now reports message type. Only two messages are expected to be routed through there, but it's the correct way to report it.

Fixed missing carriage return.

b01b3a0

FIX: Don't send server 4k of uninitialized data.

0e5c4b6

FIX: Removes 4-second delay in cachepath process

5f713a4

Reworks cleanup_created_dirs() and CleanupProc.

1a6fc91

Cleanup now takes both commpath and cachepath and prefixes for removing files created by Spindle.

Removes --with-localstorage from slurm testcase

b5970ec

That configure parameter is no longer supported. Replaced with --with-cachepaths=/tmp/commpath/cachepath --with-commpath=/tmp/commpath

rountree temporarily deployed to Spindle CI May 22, 2026 07:59 — with GitHub Actions Inactive

rountree deployed to Spindle CI May 22, 2026 07:59 — with GitHub Actions Active

mplegendre requested changes May 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Commpath#182

Commpath#182
rountree wants to merge 53 commits into
llnl:develfrom
rountree:commpath

rountree commented May 22, 2026

Uh oh!

mplegendre May 27, 2026

Uh oh!

mplegendre May 27, 2026

Uh oh!

mplegendre May 27, 2026

Uh oh!

mplegendre May 27, 2026

Uh oh!

mplegendre May 27, 2026

Uh oh!

mplegendre May 27, 2026

Uh oh!

mplegendre May 27, 2026

Uh oh!

mplegendre May 27, 2026

Uh oh!

mplegendre May 27, 2026

Uh oh!

mplegendre May 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		int num_children = ldcs_audit_server_md_get_num_children(procdata);

		debug_printf( "Processing REQUEST_CACHEPATH_CONSENSUS.\n" );

Conversation

rountree commented May 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants