modprobe: fix handling of modprobe.Context.bash() errors and expand testing
#6957
Conversation
This file contains hidden or 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
Problem: sched-simple uses flux_log_error() when logging an error message about a bad module argument, but errno is not valid here so this call is not appropriate. Switch to flux_log() with LOG_ERR.
Problem: The Context class bash() helper function doesn't catch nonzero exit codes from the subprocess, which could lead to an instance being started in an inconsistent state. Switch to subprocess.run() instead of Popen() and check the CompletedProcess returncode. Raise an appropriate RuntimError if the process exited with a non-zero status.
|
Thanks. I have a couple more improvements incoming here... |
Problem: `Modprobe.load()` tries to load target modules and all their dependencies, which results in unnecessary errors when modules are already loaded. Get the list of loaded modules and filter them from the set of target modules to load. If the set of modules to load is empty after this step, raise `FileExistsError` with an appropriate message that there was nothing to do.
Problem: `flux modprobe load` loads target modules and their dependencies when necessary, but it will exit with nonzero exit status if all modules are already loaded. Downgrade the error that there was nothing to do to an informational message and exit with success.
Problem: The description of `flux modprobe load` in flux-modprobe(1) is insufficient as it does not describe the new behavior of the command when modules are already loaded. Add a paragraph to `flux-modprobe(1)` describing this behavior.
Problem: The modprobe tests in t0100-modprobe.t do not ensure that some failures result in flux-modprobe (and thus rc1/rc3) failure, and that `flux modprobe load <module>` doesn't error if module is already loaded. Expand tests in `t0100-modprobe.t` to cover these cases.
|
Ok, sorry about that @garlick. Changes since your last review:
|
garlick
approved these changes
Aug 8, 2025
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6957 +/- ##
==========================================
- Coverage 84.04% 84.00% -0.04%
==========================================
Files 545 545
Lines 91811 91822 +11
==========================================
- Hits 77163 77139 -24
- Misses 14648 14683 +35
🚀 New features to boost your workflow:
|
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 PR fixes the
modprobe.Context.bash()helper, which currently is ignoring non-zero exit status from the invoked command(s).Additionally, a few more tests are added to the modprobe testsuite to check that the
bash()helper is working properly, and that module load failures (and other@taskfailures) causeflux modprobeto fail.