Repository navigation
pm/hydra: clean up pending PMI2 requests on client exit - #8085
Conversation
6a4dded to
6ff8e86
Compare
|
I've sent my signed Individual Contributor License Agreement to the MPICH CLA email address. |
Yes, we received your CLA. Thank you! |
hzhou
left a comment
hzhou
left a comment
There was a problem hiding this comment.
The PR looks good. Just some comments on improving the error messages when the test fails.
2560702 to
4b0823d
Compare
3004702 to
1091a47
Compare
|
test:mpich/ch4/ofi |
|
test:mpich/ch4/most |
hzhou
left a comment
hzhou
left a comment
There was a problem hiding this comment.
Looks good. A couple small changes needed. In addition, squash the last commits and revase to the main branch.
| @@ -1,2 +1,4 @@ | |||
| test_pmi1 2 | |||
| test_pmi2 2 | |||
| test_pmi2_nodeattr 4 | |||
There was a problem hiding this comment.
Need the same mpiexecarg=-ppn mpiexecarg=4 option to force the 4 ranks on the same node for the test to work.
| @@ -0,0 +1,98 @@ | |||
| #define _POSIX_C_SOURCE 200809L | |||
There was a problem hiding this comment.
Drop this. It won't work with some testing setup which defines -D_POSIX_C_SOURCE=200112L via compiler flags. Simply declare -
int kill(pid_t pid, int sig);
should work.
Separately, The copyright line should always be at the top
1091a47 to
7356e29
Compare
|
Thanks @hzhou. I added I also squashed the commits and rebased on |
Oops, I didn't mean to squash all commits. I was only referring to the previous merge commits. Sorry about that. Could you at least split the tests into a separate commit? |
When a client exits during PMI2_Info_GetNodeAttr(wait=true), Hydra can keep a pending reply for its closed socket. A later PutNodeAttr may try to use that socket and disrupt the remaining processes. Remove only requests owned by the disconnected client and release the queued command state when each request completes.
Add a normal multiple-waiter test and a forced-exit regression test for deferred PMI2 node-attribute replies. Include useful failure diagnostics, keep the test ranks on one node, and avoid overriding the build POSIX feature-test macro.
7356e29 to
432862e
Compare
|
No problem, thanks for clarifying @hzhou. I have split it back into two commits now: one for the Hydra fix, and one for the PMI2 regression tests (including the diagnostics and review adjustments). Both are based on the latest Let me know if you need any revision on those. |
|
test:mpich/authorship |
Pull Request Description
If a process exits while waiting for a PMI2 node attribute, Hydra can leave the request queued. When another process sets that attribute, Hydra may try to reply through the closed socket and fail.
Remove pending requests when the client disconnects, without affecting other waiters. Added a PMI2 fault test for this case.
The build and targeted PMI tests passed in GitHub Actions.
ChatGPT helped investigate the issue and prepare the patch and tests. I reviewed the changes and validation results.
Author Checklist
Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
Commits are self-contained and do not do two things at once.
Commit message is of the form:
module: short descriptionCommit message explains what's in the commit.
Whitespace checker. Warnings test. Additional tests via comments.
For non-Argonne authors, check contribution agreement.
If necessary, request an explicit comment from your companies PR approval manager.