Quote user arguments passed to slurm launchers #26450
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Vassily Litvinov <[email protected]>
bradcray
added a commit
to bradcray/chapel
that referenced
this pull request
Mar 1, 2025
…Flag tests This pair of tests checks that `-v` / `--verbose` work as expected in Chapel multilocale programs. When the tests were added, the only thing verbose mode did was to print out a little roll call of the nodes as they started running, which made it very portable and worth testing. Since then, verbose mode has also caused aspects of the runtime, like Qthreads and GASNet or libfabric, to also print out additional information about their startup, along with the launcher commands being executed. As a result, the logic around these tests to keep them working (either screening for such information or passing over it) has similarly grown, and grown far more complex. Most recently, we've seen failures in slurm-srun testing due to Vass's bug-fix PR that quotes additional arguments in: chapel-lang#26450 and then failures in GASNet testing due to the new capability to print out the segment size/type in: chapel-lang#26791 and chapel-lang#26811 Here, I'm skipping over these configurations for the time being as a potential stepping stone toward retiring these tests altogether, or only testing them in CHPL_COMM=none, CHPL_LAUNCHER=none mode. Another approach would be to preserve the rollcall behavior, either by: * introducing multiple verbosity levels * introducing a `--rollcall` flag * having the runtime startup information be controlled by a separate flag like `--debug`, `--devel` or possibly just CHPL_DEVELOPER The downsides to some of these approaches are that there's the potential for flags like these to interfere with configs/flags that the user wants to support, which makes the "CHPL_DEVELOPER" approach most attractive to me, and the "multiple verbosity levels" next most attractive. In any case, rather than taking a stance on this design, this PR is intended mostly to quiet the failing cases for the time being and to pave the way for further reducing the maintenance burden going forward that these tests have unfortunately, unintentionally, and gradually added over time. Resolves Cray/chapel-private#7010 --- Signed-off-by: Brad Chamberlain <[email protected]>
bradcray
added a commit
that referenced
this pull request
Mar 1, 2025
…Flag tests (#26812) [trivial, not reviewed, but pre-approved in concept by @jhh67] This pair of tests checks that `-v` / `--verbose` work as expected in Chapel multilocale programs. When the tests were added, the only thing verbose mode did was to print out a little roll call of the nodes as they started running, which made it very portable and worth testing. Since then, verbose mode has also caused aspects of the runtime, like Qthreads and GASNet or libfabric, to also print out additional information about their startup, along with the launcher commands being executed. As a result, the logic around these tests to keep them working (either screening for such information or passing over it) has similarly grown, and grown far more complex. Most recently, we've seen failures in slurm-srun testing due to Vass's bug-fix PR that quotes additional arguments in: #26450 and then failures in GASNet testing due to the new capability to print out the segment size/type in #26791 and #26811 Here, I'm skipping over these configurations for the time being as a potential stepping stone toward retiring these tests altogether, or only testing them in CHPL_COMM=none, CHPL_LAUNCHER=none mode. Another approach would be to preserve the rollcall behavior, either by: * introducing multiple verbosity levels * introducing a `--rollcall` flag * having the runtime startup information be controlled by a separate flag like `--debug`, `--devel` or possibly just CHPL_DEVELOPER The downsides to some of these approaches are that there's the potential for flags like these to interfere with configs/flags that the user wants to support, which makes the "CHPL_DEVELOPER" approach most attractive to me, and the "multiple verbosity levels" next most attractive. In any case, rather than taking a stance on this design, this PR is intended mostly to quiet the failing cases for the time being and to pave the way for further reducing the maintenance burden going forward that these tests have unfortunately, unintentionally, and gradually added over time. Resolves Cray/chapel-private#7010
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This safeguards spaces and most other symbols in the arguments passed to a compiled Chapel program that uses a slurm launcher, where such a safeguard was missing. For example:
used to behave as if it were
./myProgram --myString=Hello, world!, i.e.,world!would be passed as a separate argument tomyProgram_realwhen launched with slurm.This is a stopgap solution to ensure that the test augmented in #26439 passes in slurm configurations. It is not bullet-proof because user arguments containing single quotes and/or backslashes will not be passed through properly.
Next steps: #26381 eliminates static limits on the size of user arguments. In the long run we want to switch from launching using
system(), when quoting may be necessary, to usingexecvpto avoid the need to quote.Discussed with Brad and Anna.