Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shell: add functions to get/set shell "options" #2392

Merged
merged 10 commits into from Sep 24, 2019

Conversation

@grondo
Copy link
Contributor

commented Sep 24, 2019

This PR adds a new set of functions for managing the shell "options" set in jobspec via the public shell api:

/*  Get shell option as JSON string as set in jobspec
 *   attributes.system.shell.options
 *
 *  Returns 1 on success, 0 if option "name" was not set, or -1 on failure.
 */
int flux_shell_getopt (flux_shell_t *shell, const char *name, char **json_str);

/*  Unpack shell option from attributes.system.shell.options.<name> using
 *   jansson style unpack arguments.
 *
 *  Returns 1 on success, 0 if option was not found, or -1 on error.
 */
int flux_shell_getopt_unpack (flux_shell_t *shell,
                              const char *name,
                              const char *fmt, ...);

/*  Set shell option as a JSON string. Future calls to getopt will return
 */
int flux_shell_setopt (flux_shell_t *shell,
                       const char *name,
                       const char *json_str);

/*  Set shell option using jansson style pack args
 */
int flux_shell_setopt_pack (flux_shell_t *shell,
                            const char *name,
                            const char *fmt, ...);

It may be surprising that there are functions for setting options, but it may be useful to allow plugins to set options for other plugins, or allow a system initrc.lua to set a default option (e.g. if not shell.options.mpi then shell.options.mpi = "spectrum" end)

As seen above, shell options are exposed in initrc.lua via a proxy shell.options table, which currently allows getting, setting, and un-setting options if necessary.

Finally, some errors were fixed in the current spectrum MPI plugin.

@grondo grondo force-pushed the grondo:shell-getopt branch from 45bb8ec to 1a9ddf5 Sep 24, 2019
grondo added 10 commits Sep 22, 2019
Store attributes.system.shell.options as jobspec->options for ease of
use by shell internal components.
Allow attributes.system.shell.options.{initrc,verbose} to be
used to set path to initrc and shell verbose flag.
Add a couple tests to ensure that job-shell initrc can be overridden
by jobspec in attributes.system.shell.initrc
Add a couple of public shell functions to get and set shell "options",
i.e. the options passed in jobspec via attributes.system.shell.options.
Add a special field to the global initrc "shell" object to allow
access to shell options. This table index simply allows getting
and setting shell options via index and newindex methods. There
is currently no method for iterating all currently set options.
Now that shell.options is a proxy table for jobspec
attributes.system.shell.options, there is no need to copy this table
into place for other plugins to use.
Fix a couple of obvious errors in the "spectrum" mpi plugin for the
shell. These were errors made during conversion from the old wreck
style plugins.

Additionally, shell.options is always set now, so remove the test
for nil.
Add a shell plugin used to verify operation of flux_shell_getopt*
and run it from the initrc test script.

Add invalid argument testing for this interface to the invalid-args
plugin.
Add a sanity check that spectrum plugin is disabled by default
and enabled with attributes.system.shell.options.mpi = "spectrum".
Ensure that options set in attributes.system.shell.options
can be read via the shell's shell.options index. Also, ensure
options can be set via this same table.
@grondo grondo force-pushed the grondo:shell-getopt branch from 1a9ddf5 to 6ebda8c Sep 24, 2019
@grondo grondo changed the title shell: add functions for getting and setting shell "options" shell: add functions to get/set shell "options" Sep 24, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

Just forced a push with a fix for a typo in shell/jobspec.c @garlick noticed.

Copy link
Member

left a comment

IMHO this is fine to go in.

@mergify mergify bot merged commit 84a9266 into flux-framework:master Sep 24, 2019
2 checks passed
2 checks passed
Summary 1 rule matches
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@codecov-io

This comment has been minimized.

Copy link

commented Sep 24, 2019

Codecov Report

Merging #2392 into master will increase coverage by 0.02%.
The diff coverage is 89.85%.

@@            Coverage Diff             @@
##           master    #2392      +/-   ##
==========================================
+ Coverage   81.05%   81.07%   +0.02%     
==========================================
  Files         223      223              
  Lines       35560    35627      +67     
==========================================
+ Hits        28823    28885      +62     
- Misses       6737     6742       +5
Impacted Files Coverage Δ
src/shell/jobspec.c 88.05% <ø> (ø) ⬆️
src/shell/shell.c 84.9% <89.74%> (+0.47%) ⬆️
src/shell/rc.c 87.13% <90%> (-0.33%) ⬇️
src/common/libflux/mrpc.c 87.79% <0%> (-1.19%) ⬇️
src/cmd/flux-job.c 85.03% <0%> (-0.29%) ⬇️
src/common/libsubprocess/local.c 80.13% <0%> (+0.34%) ⬆️
src/modules/connector-local/local.c 74.27% <0%> (+1.16%) ⬆️
@grondo grondo deleted the grondo:shell-getopt branch Sep 24, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.