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: abstract some shell internals into "builtin plugins" #2335

Merged
merged 16 commits into from Aug 29, 2019

Conversation

@grondo
Copy link
Contributor

commented Aug 25, 2019

I hesitated to post this PR, because I belatedly realized the plugin interface developed here was a bit of a misstep, but perhaps it is enough of a step forward for a review of the general direction, or a mercy merge for other reasons (see below)

This PR includes:

  • abstraction and export of some of the shell API for plugins in an installed flux/shell.h header. The shell struct is split into an internal.h header.
  • a first-cut of a plugin stack service which allows plugins with dynamic or static symbols. Plugins are called in the order loaded, except if an optional plugin name collides with an existing name, then the last loaded plugin wins. (This allows a dynamically loaded "pmi" plugin to override a builtin "pmi" plugin)
  • Convert pmi.c, kill.c, signals.c into "builtin" plugins.
  • Convert io.c into a builtin plugin, also rename the module to output.c and load it under the name "output" to allow the potential for different "input" and "output" plugins.
  • There is a bit of cheating going on with the "builtin" plugins. They still have access to shell internals so they aren't using the public flux_shell_t interface from shell.h exclusively. This might actually be a good thing since it will allow more efficient operation of shell builtins that won't have to curry JSON strings back and forth in the default case.
  • Though machinery is added to the shell to allow loading of both builtin and dynamic plugins, no dynamic plugins are actually being loaded at this time.

The simplistic plugstack.c plugin loading service will need a redo. The current version will not support non-dlopen based plugins very well, nor plugins that load other plugins (e.g. a python.so plugin that supports loading *.py scripts).

However, this PR does abstract io.c into output.c in a plugin-like thing, which means if we did end up merging this PR as a tiny step forward, there might be a better chance of keeping changes in the builtin shell output implementation from conflicting with internal shell upheaval in the future.

Copy link
Member

left a comment

I found a few unchecked mallocs probably left over from when you were prototyping this, and possible errno clobbering (sorry if that's not intended to be used - I was reviewing the diff which doesn't always give the full picture).

In general, I really like how this abstracts the builtins further from the core shell code!

My vote is to get this in and decide what additional work needs to be done to complete it. It looks to me like a good step forward. I apologize for not being too helpful in the design due to lack of experience in this sort of thing. You have my ringing endorsement FWIW!

src/Makefile.am Show resolved Hide resolved
src/shell/plugstack.c Show resolved Hide resolved
src/shell/plugstack.c Show resolved Hide resolved
src/shell/plugstack.c Outdated Show resolved Hide resolved
src/shell/plugstack.c Outdated Show resolved Hide resolved
src/shell/plugstack.c Outdated Show resolved Hide resolved
src/shell/plugstack.c Show resolved Hide resolved
src/shell/plugstack.c Outdated Show resolved Hide resolved
src/shell/shell.h Outdated Show resolved Hide resolved
@@ -171,21 +161,6 @@ int shell_task_io_enable (struct shell_task *task,
return 0;
}

int shell_task_pmi_enable (struct shell_task *task,

This comment has been minimized.

Copy link
@garlick

garlick Aug 26, 2019

Member

I am loving that this code is contained in the new pmi plugin abstraction. Really nice cleanup!

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

found a few unchecked mallocs probably left over from when you were prototyping this, and possible errno clobbering (sorry if that's not intended to be used - I was reviewing the diff which doesn't always give the full picture).

Yeah, your comments are spot on. The existing "plugstack" thing is really just a prototype (I haven't decided how to deal with errors at all yet), but I realized after the fact that the approach won't work in its current form, so it needs to be rewritten or replaced. (Thanks for taking a look though)

I wasn't sure if people would want an intermediate step like this merged while I try something else, or if we'd rather wait a few days to get the final working thing in. If we want to get this merged I'll go back and address your comments asap to get the current code acceptable at least (I do feel like the abstraction of the "builtin" plugins was a win, so that, at least, is a small step forward)

The main features we'll keep from the "plugstack" prototype I think are the ability to create compiled-in plugins by assigning arbitrary functions to plugin "symbols", and the ability to register plugins with names that override previously loaded plugins.

@garlick

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

I wasn't sure if people would want an intermediate step like this merged while I try something else, or if we'd rather wait a few days to get the final working thing in. If we want to get this merged I'll go back and address your comments asap to get the current code acceptable at least (I do feel like the abstraction of the "builtin" plugins was a win, so that, at least, is a small step forward)

I vote for merging the intermediate step.

@grondo grondo force-pushed the grondo:shplugins branch from deae75f to b0e9be5 Aug 27, 2019
@grondo grondo changed the title wip: shell: add experimental shell plugin interface wip: shell: abstract some shell internals to "builtin plugins" Aug 27, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

Ok, I've pushed a change to remove the dso loading support from plugstack.c since it isn't used here, and therefore change the title of this PR accordingly.

I've also attempted to address the remaining review comments by augmenting the plugstack prototype with return value checking, use of saved_errno in free functions, etc.

If the current state of the code is acceptable as a very minor step forward. We could merge this and I can work on redoing the dynamic loading of plugins next.

@grondo grondo changed the title wip: shell: abstract some shell internals to "builtin plugins" shell: abstract some shell internals into "builtin plugins" Aug 27, 2019
@grondo grondo requested a review from garlick Aug 27, 2019
Copy link
Member

left a comment

LGTM! I just called out a couple of minor things.

@@ -16,6 +16,11 @@
#include <flux/optparse.h>

typedef struct flux_shell flux_shell_t;
typedef struct shell_task flux_shell_task_t;

This comment has been minimized.

Copy link
@garlick

garlick Aug 28, 2019

Member

Since this is to be an installed header, should it be wrapped in:

#ifdef __cplusplus
extern "C" {
#endif

#ifdef __cplusplus
}

I'm not sure of the ramifications of a shell plugin written in C++, but we've done this for all other public headers.

This comment has been minimized.

Copy link
@grondo

grondo Aug 28, 2019

Author Contributor

Oh yeah, duh!

@@ -0,0 +1,14 @@
/************************************************************\
* Copyright 2014 Lawrence Livermore National Security, LLC

This comment has been minimized.

Copy link
@garlick

garlick Aug 28, 2019

Member

2019?

/* Allow in-tree programs to #include <flux/shell.h> like out-of-tree would.
*/

#include "src/shell/shell.h"

This comment has been minimized.

Copy link
@garlick

garlick Aug 28, 2019

Member

The czmq.h include should be dropped from the public header.

Edit: this comment was meant to be about the installed shell.h

return -1;
cw->cb = cb;
cw->arg = arg;
zhashx_update (task->subscribers, name, cw);

This comment has been minimized.

Copy link
@garlick

garlick Aug 28, 2019

Member

Suggestion: use zhash_insert() instead of zhash_lookup() + zhash_update().

This comment has been minimized.

Copy link
@grondo

grondo Aug 29, 2019

Author Contributor

Good suggestion, but that would require an unnecessary malloc/free on EEXIST. Still warranted you think?

This comment has been minimized.

Copy link
@garlick

garlick Aug 29, 2019

Member

Not important if you don't want to mess with working code.

My opinion: double hash lookup on every call vs extra malloc on a programming error that's probably fatal, I'd go with the latter.

Edit: I mean the former!

This comment has been minimized.

Copy link
@grondo

grondo Aug 29, 2019

Author Contributor

Ok, there's sense in that, I'll make the change. (Though I wouldn't
say that flux_shell_task_channel_subscribe() against an already subscribed channel is a "programming" error. Nor would it be fatal, to the shell anyway)

Perhaps instead of flux_shell_task_channel_subscribe() there should be a call that combines flux_cmd_add_channel and flux_task_channel_subscribe, something like

int flux_shell_task_add_channel (flux_shell_task_t *task, const char *name, flux_shell_task_io_f cb, void *arg);

Furthermore, perhaps shell.h probably shouldn't be installed as part of this PR, since dynamically loaded plugins aren't yet supported. Maybe that will make the current PR more palatable, so we don't have to worry (as much) about "public" interfaces that might change anyway on the next iteration?

This comment has been minimized.

Copy link
@garlick

garlick Aug 29, 2019

Member

Up to you, but I think the risk of fallout from publishing shell.h and then changing it is low at this point.

This comment has been minimized.

Copy link
@grondo

grondo Aug 29, 2019

Author Contributor

I'm tempted since it is the "right" thing to do. I was missing the correct flags to export dynamic symbols from flux-shell anyway. Probably best to perform both of these steps once shell plugins are ready.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

Great comments! Thanks. Will address.

@grondo grondo force-pushed the grondo:shplugins branch from 342cc4d to 5fcf56f Aug 29, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Ok, I pushed, rebased onto current master and (hopefully) addressed comments.

@garlick

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

LGTM! Squash?

Didn't mean to make a big deal about double hash thing. It was really minor IMHO, sorry about that.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

I will squash after #2323 is merged since there will be some conflicts.

grondo added 15 commits Aug 19, 2019
Prepare to make a shell plugin interface public as <flux/shell.h>.

Move struct flux_shell definition into an "internal.h" header and
keep existing public flux_shell_* functions in shell.h.

Add shell.h to src/include/flux so that existing components may
include it as <flux/shell.h>.

Update current users. No interfaces have been added or removed.
To allow builtin shell modules to "#include <flux/shell.h>" like
out-of-tree plugins would. Add src/include/shell.h as a wrapper for
src/shell/shell.h.
Add a simple plugin stack interface for use by the flux-shell.

The plugstack interface supports loading DSOs with arbitrary callback
symbols and an optional plugin name. Callbacks are called in the order
in which plugins are loaded, except in the case of a name collision,
in which case the last plugin loaded will override the original.

This interface alone doesn't support a config file or other method
for configuring how and when plugins are loaded. That is left for a
higher level.
Add basic plugstack tests of static interfaces (no dlopen loading yet)
Add aux_item support to the flux_shell_t object.
Add a pointer to the currently starting task in the shell.
This will be required for plugin interfaces that want to fetch
the currently "active" task.
Add a method for shell plugins to fetch the current shell task,
if valid, as well as accessors for the task's flux_cmd_t and
flux_subprocess_t members.

Additionally, prepare for task plugins to be able to "subscribe"
to channel output via flux_shell_task_channel_subscribe(), though
the method doesn't do anything yet besides track subscribers.
Add plugin callback sites to the shell in the following places:

 - flux_shell_init       - called during shell intialization
 - flux_shell_exit       - called just before shell exit
 - flux_shell_task_init  - called for each task before fork/exec
 - flux_shell_task_fork  - called from parent after task is forked
 - flux_shell_task_exec  - called from child before exec(2)

No plugins are registered at this time so all these calls do nothing.
Add support for loading shell internal plugins.

Shell 'builtins' are loaded onto the plugstack, but the provided
plugin functions are statically linked into the program. Besides
a cleaner abstraction, the main benefit of shell plugins is that
they are loaded into the plugin stack under a well-known name,
and thus may be overridden later by alternate, dynamically loaded
plugins.

Currently, no builtin plugins are loaded.
Untangle the PMI implementation from the rest of the shell and turn
it into a shell builtin plugin under the name "pmi".

This allows, in theory, the default PMI implementation to be
overridden by a dynamically loaded plugin.

There is a bit of cheating here since the pmi plugin actually has
access to shell internals, however in order to simplify development
we allow this violation of the shell plugin abstraction for builtins.
Export flux_shell_service_register() and flux_shell_rpc_pack()
to plugins via shell.h.
Untangle the io implementation from the shell internals and
abstract the module into a builtin shell plugin.
Problem: The output plugin is contained in a file called "io.c" and
includes a struct and set of functions that use "io" in the name,
when this module really only deals with output. This could cause
confusion when an eventual "input" plugin is added to the shell.

Rename everything that uses "io" in the name to use "output" instead.
Problem: The shell output plugin is held in the file "io.c".

Rename it to output.c.
Abstract shell "kill" event handler into a shell builtin plugin.
Abstract shell signal handler into a builtin plugin.
@grondo grondo force-pushed the grondo:shplugins branch from 5fcf56f to e247fa4 Aug 29, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Ok, rebased, conflicts resolved, and squashed up

@mergify mergify bot merged commit 5f0ecaa into flux-framework:master Aug 29, 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 Aug 29, 2019

Codecov Report

Merging #2335 into master will decrease coverage by 0.05%.
The diff coverage is 81.22%.

@@            Coverage Diff             @@
##           master    #2335      +/-   ##
==========================================
- Coverage   80.87%   80.82%   -0.06%     
==========================================
  Files         215      217       +2     
  Lines       34333    34494     +161     
==========================================
+ Hits        27768    27880     +112     
- Misses       6565     6614      +49
Impacted Files Coverage Δ
src/shell/info.c 80.85% <ø> (ø) ⬆️
src/shell/kill.c 100% <100%> (ø) ⬆️
src/shell/signals.c 93.33% <100%> (ø) ⬆️
src/shell/svc.c 73.33% <50%> (-7.71%) ⬇️
src/shell/task.c 79.16% <68.42%> (-10.58%) ⬇️
src/shell/shell.c 80.18% <77.27%> (-0.34%) ⬇️
src/shell/plugstack.c 79.78% <79.78%> (ø)
src/shell/pmi.c 76.53% <85%> (+0.83%) ⬆️
src/shell/output.c 75.97% <88.88%> (ø)
src/shell/builtins.c 94.73% <94.73%> (ø)
... and 8 more
@garlick garlick referenced this pull request Sep 30, 2019
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.