job shell: add flux_shell_t and completion references #2240
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
Handle NULL passed to rcalc_destroy () for consistency with most other flux-core code.
Problem: As the job shell grows in complexity, functions and callbacks may need to access more than one of the shell internal datatatypes at a time, potentially including flux handle, reactor, shell job information etc. Currently there is no easy or common way for internal code to accomplish this without use of custom data types, adding globals, et.c To reduce this future effort and present a more common methodology, add a `struct flux_shell` data type as a container for other shell data types, and refactor the shell to pass this object to submodules, simplifying their implementation a bit. As part of this effort, fetching of jobspec and R is moved wholly into the info.c module, whether fetched from the command line or KVS. Command line options, such as jobid and the standalone and verbose flags, is moved up into the `struct flux_shell`.
Problem: Once the flux-shell starts adding reactor watchers other than those of libsubprocess, e.g. message handlers, signalfd watchers, etc., then exiting from the internal reactor when all tasks have completed will not be as straightforward. Instead of adding a single `is_complete()` function that needs to check for all tasks exited, all IO complete, etc, set up a system where shell components can take a "completion reference" on the shell. Once all existing completion refs are released, then the shell's reactor will be stopped manually with flux_reactor_stop(). The completion references are named to aid in future debugging. If a shell appears to be stuck, keys from the internal hash could be dumped to determine which components haven't completed.
Use flux_shell_add_completion_ref() to take a reference on the shell for each task executed. Drop the reference in the completion callback so that the shell's reactor is manually stopped after the final task exits. This doesn't change any functionality of the shell, but ensures the shell will still exit the reactor when new reactor references are attached to the shell in future commits (e.g. message watchers).
Codecov Report
@@ Coverage Diff @@
## master #2240 +/- ##
==========================================
+ Coverage 80.74% 80.77% +0.02%
==========================================
Files 209 209
Lines 32988 33037 +49
==========================================
+ Hits 26637 26685 +48
- Misses 6351 6352 +1
|
|
Excellent! |
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 addresses #2237.
I apologize in advance because this will be a difficult PR to review. I attempt to make a minor advance by adding a
struct flux_shellstructure to contain the independingioinfoandpmistructures used by the various components of the shell, but ended up making more changes than I wanted. Perhaps with the luxury of more time, this PR could be made cleaner, but hopefully after some rounds of feedback this approach could be acceptable.As noted above, this branch adds a new
struct flux_shellstructure as container for the most of the other structures used by the shell. This structure can then be passed to callbacks that may require access to multiple shell components and data, without requiring those callers to use custom structures or push multiple pointers into the reactor or handle aux hashes.The
struct flux_shellmay also serve as an opaque handle for plugins in the future (along with a set of accessors as yet unwritten), and thus aflux_shell_ttypedef is used as the start of a public interface. Later when we need this we can installflux/shell.hand move thestruct flux_shelldefinition internal.Once the
flux_shell_tstructure was defined, many of theinitfunctions were updated to take this function as an argument. This resulted in cascading changes that, unfortunately, come in one big commit. To help review, here's a summary of the changes:shell_parse_cmdline()flux_shell_tshell->jobidflux_shell_tintoshell_finalize()jobspecandRfully intoshell_info_create()(whether set on cmdline or fetched via kvs) (this is probably a majority of the code change, since some functions were moved fromshell.ctoinfo.c)Finally, to address #2237, a concept from
wrexecdwas borrowed to add a "completion reference" interface for the shell's reactor. Once all completion refs that are taken are dropped, the shell's reactor is manually stopped withflux_reactor_stop(). The references are also named as a future debugging aid. Initially, a reference is taken for each task, and released in the task completion handler.Fixes #2237.