flux-job: attach: support MPIR tool launch extension #5758

grondo · 2024-02-28T14:39:34Z

This PR adds the minimum support for the MPIR tool daemon launch extensions documented in The MPIR Process Acquisition Interface Version 1.1 , Section 7.1. It also splits the MPIR code in src/cmd/job/attach.c into its own source file so that the implementation can be tested with the "shell" mpir test utility in t/shell/mpir.c.

Currently the tool specified in MPIR_executable_path and MPIR_server_arguments is launched with libsubprocess. We depend on the disconnect handler to clean up stray tool processes if flux job attach is terminated or otherwise exits early.

This is currently a WIP because it adds a list of task/broker rank pairs to the shell proctable response since this was the simplest approach for now, but there's probably a more efficient way.

This PR is based on top of #5756

garlick · 2024-02-28T16:01:22Z

Mentioning @lee218llnl since he may be interested or have a perspective on this part of the MPIR spec.

grondo · 2024-02-28T19:09:45Z

I have an improvement for the MPIR rangelist representation that I think makes addition of the broker ranks to the proctable response reasonable. For example, this is the JSON representation of the proctable entries for 3072 processes running across 64 nodes on corona:

{"hosts":[["corona",[[187,-47],[13,-47],[1,-47,1],[2,-47],[1,-47,2],[6,-47],[1,-47,1],[2,-47],[1,-47,4],[2,-47],[1,-47,34],[2,-47],
[1,-47,9]]]],"executables":[["sleep",[[-1,-3071]]]],"ids":[[0,3071]],"pids":[[1535168,47],[-535966,47],[-228557,47],
[686167,47],[-622563,47],[-129439,47],[2866245,47],[-1739757,47],[220055,47],[-428264,47],[217894,47],[48174,47],
[-56983,47],[-157167,47],[-242878,47],[348002,47],[141991,47],[-1051847,47],[853639,47],[279800,47],[-315681,47],
[-1396323,47],[-1368,47],[944,47],[-24426,47],[139,47],[-112,47],[-133,47],[-264,47],[141,47],[-19694,47],[19383,47],
[-3497,47],[548,47],[-5766,47],[-171,47],[-344,47],[1131,47],[-3202,47],[-243,47],[104,47],[39,47],[-98,47],[224,47],
[-186,47],[53,47],[-393,47],[98,47],[-1593,47],[-378,47],[97,47],[-113,47],[101,47],[-69,47],[-53,47],[214,47],[-369,47],
[149,47],[-369,47],[-1869,47],[-84,47,1],[-62,47],[-133,47]],"ranks":[[0,-47],[1,-47,62]]}

The additional overhead is the "ranks":[[0,-47],[1,-47,62]] bit.

src/shell/mpir/rangelist.c

grondo · 2024-02-29T20:20:17Z

Just removed WIP. However, once #5605 goes in, possibly this PR should be updated to use the shell rexec service instead of the broker service. So while an early review would be welcome, that small change could be forthcoming.

garlick · 2024-02-29T21:04:21Z

Just a quick meta question - would it make sense to have a libmpir convenience library that concentrates this stuff and any unit tests?

grondo · 2024-02-29T21:15:37Z

Just a quick meta question - would it make sense to have a libmpir convenience library that concentrates this stuff and any unit tests?

There are already unit tests in src/shell/mpir. For the testing of the MPIR_proctable and other MPIR symbols, a unit test will not suffice since it requires a running job shell. Or were you wanting the mpir code moved out of the shell to src/common and combined with the MPIR support from flux-job somehow -- I'm not sure it makes sense to always have the MPIR_proctable and other MPIR symbols linked in to the shell, as this might confuse debuggers. This is the main reason I've shied away from that approach for now.

There is also the small libdebugged library that is used to give access to MPIR_being_debugged from Python... maybe they could all be combined into a single thing? I'm kind of afraid it might be more messy to do it that way -- perhaps save it for another day?

garlick · 2024-02-29T21:29:09Z

Sure, I was just asking. It sort of worked out with the PMI stuff but good point about the special symbols.

grondo · 2024-02-29T21:46:56Z

Yeah, we should revisit it perhaps in the future when not pressed for time. Maybe I'm overthinking the symbols...

garlick

Just a few minor typos and a question for clarification. This is great to have done!

garlick · 2024-02-29T23:09:32Z

src/shell/mpir/rangelist.c

@@ -25,16 +25,20 @@
 *   which is delta encoded from the previous entry (or 0 if this is the


In "shell: mpir: compress repeating ranges in rangelist" commit message: s/commoon/common/
Also is that space in one of the ranges supposed to be there?

garlick · 2024-02-29T23:19:57Z

src/shell/mpir/rangelist.c

+static int increment_range_repeat (json_t *range)
+{
+    if (json_array_size (range) == 3) {
+        json_int_t repeat = range_json_val (range, 2);
+        return json_array_set_new (range, 2, json_integer (repeat + 1));
+    }
+    return json_array_append_new (range, json_integer (1));
+}


If either of the _new() functions fails, the json_integer() value is leaked.

garlick · 2024-02-29T23:30:24Z

src/cmd/job/mpir.c

+    rank = idset_first (ranks);
+    while (rank != IDSET_INVALID_ID) {
+        flux_subprocess_t *p;
+        if (!(p = flux_rexec (h, rank, 0, cmd, &ops)))
+            log_err ("MPIR: failed to launch %s", tool_path);
+        rank = idset_next (ranks, rank);
+    }
+    flux_cmd_destroy (cmd);
+    return;


Are the subprocess objects leaked here purposefully because there is no way to return an error via the MPIR interface?

The subprocess objects are destroyed in completion_cb() during normal termination, and I notice there is a missing flux_subprocess_destroy() in the state_cb as well when the subprocess fails. And yeah, since there is no way or need to traverse the list of launched subprocesses at normal program exit, it didn't seem necessary to try to keep this list internally and then somehow do something at exit (maybe with an at_exit handler?).

On abnormal termination of this command, one of the log_*_exit() functions are usually called and everything is leaked.

garlick · 2024-02-29T23:32:39Z

t/shell/mpir.c

@@ -15,6 +15,7 @@
 #endif

 #include <flux/core.h>
+#include <flux/optparse.h>


In "testsuite: switch mpir test program to optparse " commit message, s/--leade-rank/leader-rank/

Problem: Processes like `flux job attach` that use MPIR_Procdesc information may need to know the broker rank information for tasks in addition to or instead of hostnames. This information could be provided by the shell mpir procdesc response so just add it there.

Problem: There are no unit tests for the new proctable 'rank' member. Add tests to the proctable unit test.

Problem: The MPIR Process Acquisition Interface[1] Version 1.1 describes an optional tool daemon launch extension in Section 7.1, but flux-job attach does not support this extension. Support optional launch of tool daemons via MPIR_executable_path and MPIR_server_arguments as described in the MPIR spec. 1. https://www.mpi-forum.org/docs/mpir-specification-03-01-2018.pdf

Problem: The t/shell/mpir.c test program uses free arguments to pass the leader shell rank and service name to the test, but this is not flexible enough for future use of this program. Add `-r, --leader-rank=N` and `-s, --service=NAME` options to the test program, and drop the use of free arguments. Update the affected sharness test.

Problem: The shell/mpir test program only tests the shell proctable RPC, but does not support MPIR tool launch extensions as is supported by flux-job attach. Link t/shell/mpir with flux-job attach mpir code to share MPIR_proctable generation and also new MPIR tool launch extensions. Add a new option `-t, --tool-launch` to the test program for exercising these extensions.

Problem: Use of tab is standard in sharness tests, but t2610-job-shell-mpir.t uses 4 spaces instead. Replace spaces with tab where appropriate.

Problem: There are no tests of flux-job attach support for the MPIR tool daemon launch extension. Utilize the support in t/shell/mpir to drive testing of the code that supports MPIR_executable_path and MPIR_server_arguments.

grondo · 2024-03-01T23:40:59Z

Rebased on master. Use shell rexec service to launch tool daemons.

grondo · 2024-03-04T18:06:40Z

This is approved, but I think there's an open question about how the subprocess objects are managed within the mpir module of flux job attach.

Also, while the code itself is tested via the testsuite, there is no real-world debugger or tool that uses MPIR_executable_path and MPIR_server_arguments against which the PR can be tested (DDT supposedly does, but I was unable to force it to work with flux job attach), so whether the current approach will actually work in practice is an unknown.

Therefore, I'm unsure whether this PR should be merged, or just closed and resurrected when there is a real need.

Since the code is only activated when a debugger or tool has attached to flux job attach (MPIR_being_debugged = 1), and the tool has written to MPIR_executable_path, the risk of merging the code is probably low.

garlick · 2024-03-04T18:32:52Z

It would be a shame to lose this work in case it is needed later (for example if DDT uses it). My vote is we merge it.

Later, we should perhaps find somewhere to document the specific "tool support" features we have in flux-core so future people wanting to port a tool have someplace to start.

grondo · 2024-03-04T18:40:46Z

Great idea! Maybe the debugging docs in flux-docs should be moved into flux-core and the MPIR support mechanism details added to that section.

Ok, I'll set MWP here since there's low probability of anything bad happening as a result.

codecov · 2024-03-04T18:41:47Z

Codecov Report

Merging #5758 (f4c55ea) into master (01a63f2) will increase coverage by 0.01%.
The diff coverage is 86.39%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5758      +/-   ##
==========================================
+ Coverage   83.50%   83.52%   +0.01%     
==========================================
  Files         509      509              
  Lines       83703    83842     +139     
==========================================
+ Hits        69897    70025     +128     
- Misses      13806    13817      +11

Files	Coverage Δ
src/shell/mpir/mpir.c	`68.88% <83.33%> (-0.43%)`	⬇️
src/shell/mpir/proctable.c	`84.21% <80.43%> (-1.95%)`	⬇️
src/cmd/job/mpir.c	`85.93% <89.47%> (+12.40%)`	⬆️

... and 3 files with indirect coverage changes

grondo force-pushed the mpir-tool-launch branch from b2532c9 to bc505ab Compare February 28, 2024 15:10

grondo force-pushed the mpir-tool-launch branch from bc505ab to ec13bda Compare February 29, 2024 16:00

github-advanced-security bot found potential problems Feb 29, 2024

View reviewed changes

src/shell/mpir/rangelist.c Fixed Show fixed Hide fixed

src/shell/mpir/rangelist.c Fixed Show fixed Hide fixed

grondo force-pushed the mpir-tool-launch branch 2 times, most recently from ce3292f to b135759 Compare February 29, 2024 18:30

grondo changed the title ~~WIP: flux-job: attach: support MPIR tool launch extension~~ flux-job: attach: support MPIR tool launch extension Feb 29, 2024

garlick approved these changes Feb 29, 2024

View reviewed changes

grondo force-pushed the mpir-tool-launch branch from b135759 to 8bbd9fc Compare March 1, 2024 01:00

grondo mentioned this pull request Mar 1, 2024

add flux job hostpids command #5765

Merged

grondo force-pushed the mpir-tool-launch branch from 8bbd9fc to 3adfe9e Compare March 1, 2024 23:06

grondo added 7 commits March 1, 2024 23:29

shell: mpir: add unit tests for proctable rank member

8137bb0

Problem: There are no unit tests for the new proctable 'rank' member. Add tests to the proctable unit test.

testsuite: use tabs in t2610-job-shell-mpir.t

8e8e231

Problem: Use of tab is standard in sharness tests, but t2610-job-shell-mpir.t uses 4 spaces instead. Replace spaces with tab where appropriate.

testsuite: test MPIR tool daemon launch

f4c55ea

Problem: There are no tests of flux-job attach support for the MPIR tool daemon launch extension. Utilize the support in t/shell/mpir to drive testing of the code that supports MPIR_executable_path and MPIR_server_arguments.

grondo force-pushed the mpir-tool-launch branch from 3adfe9e to f4c55ea Compare March 1, 2024 23:40

grondo added the merge-when-passing label Mar 4, 2024

mergify bot merged commit fac47c6 into flux-framework:master Mar 4, 2024
35 checks passed

grondo deleted the mpir-tool-launch branch March 4, 2024 18:42

grondo mentioned this pull request Mar 27, 2024

flux-job: support MPIR_executable_path and MPIR_server_arguments in attach #5727

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flux-job: attach: support MPIR tool launch extension #5758

flux-job: attach: support MPIR tool launch extension #5758

grondo commented Feb 28, 2024

garlick commented Feb 28, 2024

grondo commented Feb 28, 2024

grondo commented Feb 29, 2024

garlick commented Feb 29, 2024

grondo commented Feb 29, 2024 •

edited

Loading

garlick commented Feb 29, 2024

grondo commented Feb 29, 2024

garlick left a comment

garlick Feb 29, 2024

garlick Feb 29, 2024

garlick Feb 29, 2024

grondo Mar 1, 2024 •

edited

Loading

garlick Feb 29, 2024

grondo commented Mar 1, 2024

grondo commented Mar 4, 2024 •

edited

Loading

garlick commented Mar 4, 2024

grondo commented Mar 4, 2024

codecov bot commented Mar 4, 2024

		@@ -25,16 +25,20 @@
		* which is delta encoded from the previous entry (or 0 if this is the

flux-job: attach: support MPIR tool launch extension #5758

flux-job: attach: support MPIR tool launch extension #5758

Conversation

grondo commented Feb 28, 2024

garlick commented Feb 28, 2024

grondo commented Feb 28, 2024

grondo commented Feb 29, 2024

garlick commented Feb 29, 2024

grondo commented Feb 29, 2024 • edited Loading

garlick commented Feb 29, 2024

grondo commented Feb 29, 2024

garlick left a comment

Choose a reason for hiding this comment

garlick Feb 29, 2024

Choose a reason for hiding this comment

garlick Feb 29, 2024

Choose a reason for hiding this comment

garlick Feb 29, 2024

Choose a reason for hiding this comment

grondo Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

garlick Feb 29, 2024

Choose a reason for hiding this comment

grondo commented Mar 1, 2024

grondo commented Mar 4, 2024 • edited Loading

garlick commented Mar 4, 2024

grondo commented Mar 4, 2024

codecov bot commented Mar 4, 2024

Codecov Report

grondo commented Feb 29, 2024 •

edited

Loading

grondo Mar 1, 2024 •

edited

Loading

grondo commented Mar 4, 2024 •

edited

Loading