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

libflux: add simple plugin interface #2357

Merged
merged 3 commits into from Sep 16, 2019

Conversation

@grondo
Copy link
Contributor

commented Sep 9, 2019

This PR isn't meant for merging, but just to put up some code for discussion.

After trying several different approaches this simple flux_plugin_t class was factored out of the work on shell plugins. While working on that PR I ended up with a flux_shell_plugin_t class that seemed like it might be useful as a more generic interface, so I moved the code under libflux and propose it here before moving my other shell work on top of it.

I had started by resurrecting the "flux_extensor_t" work, but unfortunately didn't quite have the mental capacity at this point to get a universal solution like that going in the short time I needed to get the shell plugin work done.

There's a full description of this plugin interface design in 0e49922. At a high level, a flux_plugin_t object is a simple function dispatch service which, similar to flux message dispatcher, allows a plugin to "subscribe" to host program calls via a set of topic string globs dynamically registered by plugins or another entity on their behalf.

This approach simplifies a lot of things, including creation of statically built plugins, allowing plugins to load other plugins, and allowing plugins to call other plugins.

Identified tradeoffs include

  • extra overhead for every call
  • one size fits all callback signature means "arguments" have to be encapsulated

These two main caveats is why I'm sharing this code now. I don't want to convert shell plugins into this interface if it isn't acceptable for libflux. I also went back and forth on how to propagate callback "arguments", and ended up with the least clean, but most flexible method, a flux_plugin_args_t type (just a wrapper for an struct aux_item).

Another iteration I toyed with included no separate flux_plugin_args_t type, instead caller pushes args into a plugin before using flux_plugin_call() and removes them after the call. Plugins then use void *flux_plugin_arg_get (flux_plugin_t *p, const char *name) to get "arguments" directly.

Of course, there is also the original varargs-based implementation, which seemed kind of kludgy to me, but had a much cleaner interface for plugins themselves.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

One overarching issue, and the main cause for my failure in resurrecting the extensor work, is I'm still not sure how to unify flux_module_t and flux_plugin_t -- really they should be the same thing.

In fact flux_module_t is already very close in concept to the plugin interface proposed here -- in that there is a single point of entry where the plugin/module sets up all its work (in the case of message actor modules, set up message handlers, in the case of plugins set up "call" handlers (wish I had a better name for that)).

However, the required mod_main symbol for flux_module_t doesn't quite work for the plugin case, and there is also the required static symbol mod_name which wouldn't apply to plugins. Perhaps a transition plan could involve embedding a flux_plugin_t in flux_module_t and if flux_plugin_init is found, a module could use the flux_plugin_register interface to register a main symbol and, dynamically, a name. I think this might be enough to allow a python/lua/exec loaders for modules, though like before the flux_modfind etc interfaces would need to be cleaned up.

On the bright side though, the internal cmb.* services could easily be converted to "static plugins" I think if there is any benefit there. The broker could eventually be extended to load other plugins for internal services, much like we plan to do with the shell (and using the same flux_plugin_t interface)

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

Another idea for plugin arguments would be to follow the msg_handler analogy until it hurts and pass arguments to callbacks in a const flux_msg_t *msg! 😉 (or another type that that is a wrapper for json_t and offers pack/unpack)

For arguments that would need to be read/write, or are themselves objects (e.g. flux_shell_t) these could be handled by creating wrappers for flux_plugin_aux_get, e.g. all shell plugins would have access to flux_shell_t *flux_plugin_get_shell (flux_plugin_t *p).

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Intriguing! Sorry, I'm out of time right now but I'll try to get my head around this ASAP!

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

On pack/unpack vs passing void pointers through an aux container or whatever - is the fact that it would be impossible to pass a pointer to a struct a tangible benefit of the former? I'm not sure how much to make of that possibility honestly.

@grondo grondo force-pushed the grondo:simplugins branch from c8f545f to ea54068 Sep 10, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

On pack/unpack vs passing void pointers through an aux container or whatever - is the fact that it would be impossible to pass a pointer to a struct a tangible benefit of the former? I'm not sure how much to make of that possibility honestly.

Oh yeah, this is exactly why I started with the aux container/hash approach. If using pack/unpack style then any pointer to a struct or void * would have to go someplace else (e.g. in a flux_plugin_get()), which is probably a bit confusing...

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

So no concern about needing to recompile plugins when structs change? That's the part I wasn't sure we needed to worry about.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

Only if the structs were exposed and not opaque. We've been careful with flux_shell_t, for example, to only expose an abstract type and have proposed an ABI which is likely to only require additions and not changes or deletions, in the hopes of preventing the need for unnecessary plugin rebuilds.

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Only if the structs were exposed and not opaque. We've been careful with flux_shell_t, for example, to only expose an abstract type and have proposed an ABI which is likely to only require additions and not changes or deletions, in the hopes of preventing the need for unnecessary plugin rebuilds.

Got it, thank you!

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

One random comment while digesting the above:

and there is also the required static symbol mod_name which wouldn't apply to plugins

See #663 - @trws identified this as a problem for go and later rust bindings, so that shouldn't hold us back.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

On pack/unpack vs aux container -- one idiom we could adopt, at least it would work well for shell plugins, is that the "host" context of the plugin could always export a handle through the flux_plugin_t * directly by offering a getter, e.g. in this case

flux_shell_t *flux_plugin_get_shell (flux_plugin_t *p)

but all other arguments to callbacks are passed with an unpack-style interface. If a plugin is operating as a filter, or the caller expects a return value, results could be pushed into a result with a pack() interface. (Much like sending a message and getting a response)

One use case might be to place the output encoding for the shell into plugins. The current output plugin could load its own encoding plugin(s), and for each chunk of output data it would call output.encode, passing the read data in "args". The selected encoding plugin (or a stack of them?) would read in the data from args, do the encoding and pack() the result via whatever interface. E.g. the "builtin" could be base64, but this could be swapped out for raw utf-8, and/or a plugin pushed on top of either to do compression/signing...

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

I do like the pack/unpack style interface over the "hash of void *" one. Although it's got its own gotchas, it has several things going for it:

  • direct mapping of multiple keys to values of different types in one call
  • a framework for optional keys e.g. s?:s (useful as interfaces evolve?)
  • error handling if the format string doesn't match what's provided

It seems like a "host handle" is also a nice way to go.

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

BTW, the term "subscribe" still evokes pub/sub in my mind. I am not sure if that's just me, or if it will lead users to incorrect conclusions about how many subscribers there can be, etc..

Would "register" be apropos here?

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

BTW, the term "subscribe" still evokes pub/sub in my mind. I am not sure if that's just me, or if it will lead users to incorrect conclusions about how many subscribers there can be, etc..
Would "register" be apropos here?

Sorry if I slipped into saying "subscribe" above, I don't think it appears in the code?

For the shell plugin case, it really is a lot like pub/sub, since all plugins that register a handler for a given name have their callbacks invoked when that name is "called". The difference is that the callbacks are called synchronously and sequentially.

Anyway, in the code it is basically add_handler, or "register" as you suggest is as good a name as any.

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Sorry, I made that comment without looking at the code.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

I do like the pack/unpack style interface over the "hash of void *" one. Although it's got its own gotchas, it has several things going for it:

I agree with everything you stated. So maybe the flux_plugin_arg_t * becomes just a thin wrapper around json_t *.

typedef flux_plugin_arg flux_plugin_arg_t;
flux_plugin_arg_t *flux_plugin_arg_create (void);
void flux_plugin_arg_destroy (flux_plugin_arg_t *arg);
// Get last error for arg
const char *flux_plugin_arg_strerror (flux_plugin_arg_t *arg);
int flux_plugin_arg_pack (flux_plugin_arg_t *arg, const char *fmt, ...);
int flux_plugin_arg_unpack (flux_plugin_arg_t *arg, const char *fmt, ...);

Not exactly sure the best way to allow plugin callback to set a "result", though. There could be another flux_plugin_arg_t **outarg argument to all plugin functions (not ideal), or a set of flux_plugin_result_pack (flux_plugin_arg_t *arg, const char *fmt, ...)
and flux_plugin_result_unpack (flux_plugin_arg_t *arg, const char *fmt, ...), which I prefer. Maybe even flux_plugin_pack/unpack could just take a flag, to have one set of interfaces

enum {
    FLUX_PLUGIN_ARG_RESULT
};
int flux_plugin_arg_pack (flux_plugin_arg_t *arg, int flags, const char *fmt, ...);
int flux_plugin_arg_unpack (flux_plugin_arg_t *arg, int flags, const char *fmt, ...);

Also, since packing and unpacking arguments might end up being useful elsewhere, maybe this type could be renamed flux_arg_t and put it its own module under libflux and unit tested alone, etc.

Sorry if I'm getting off track on minutia here.

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Looking at code now. This looks very well designed.

The two points you raised: 1) call overhead, and 2) arg encapsulation do not seem like show stoppers to me at all. To the contrary, it seems like the extra indirection has some nice benefits.

I haven't quite gotten my head around the issues you pointed out with wrapping the "comms module" stuff, but I strongly support unifying the plugin interface across the board. Will look at that some more, but I'm comfortable with your direction if you'd like to press on ahead using this for the shell plugins...

We should circle back and get something coherent into RFC 5 at some point.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

I haven't quite gotten my head around the issues you pointed out with wrapping the "comms module" stuff, but I strongly support unifying the plugin interface across the board. Will look at that some more, but I'm comfortable with your direction if you'd like to press on ahead using this for the shell plugins...

Yeah, I think it would take a couple weeks at least to attempt that unification effort, and we don't have time for that right now. The extraction of flux_plugin_t into its own module was a baby step in that direction (as you can guess by the tiny bit of code represented here, most of the time was spent on iterating on the required interface).

I was mainly worried about the argument passing style, for which I've gotten good feedback, and whether or hot the plugin.h interface should have stayed under src/shell or if it was ok to publish in the public libflux API.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

We should circle back and get something coherent into RFC 5 at some point.

Yeah, I'll open an issue. That reminds me that in the extensor work I'd done before, I'd called the "base" type a flux_extension_t, of which "modules" and "plugins" were both special types. That naming convention seems to fit better with the already established convention (though unfortunately it is more characters).

Do you think the proposed flux_plugin_t should be renamed flux_extension_t?

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

On pack/unpack interfaces, if "inargs" are only read in the plugin and written by the host, and the opposite for "outargs", then could flux_plugin_arg_pack() and flux_plugin_arg_unpack() just be context sensitive? E.g. internally keep the two objects separate?

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Do you think the proposed flux_plugin_t should be renamed flux_extension_t?

Maybe keep flux_plugin_t until we generalize it? Up to you really! We likely won't have a proliferation of out of tree plugins over the next year so changing later won't be hard.

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Also, since packing and unpacking arguments might end up being useful elsewhere, maybe this type could be renamed flux_arg_t and put it its own module under libflux and unit tested alone, etc.

My vote would be to hold off on that one.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

could flux_plugin_arg_pack() and flux_plugin_arg_unpack() just be context sensitive? E.g. internally keep the two objects separate?

That seems reasonable, but how would it be implemented? The caller would have to set some flag to change the context?

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

That seems reasonable, but how would it be implemented? The caller would have to set some flag to change the context?

Oh, Hmm, maybe that was not a great idea after all.

@grondo grondo force-pushed the grondo:simplugins branch from 518ac51 to aa85643 Sep 11, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

Just pushed an update with the change to flux_plugin_arg_t to use the pack/unpack interface as agreed, with tests. I do think this improved the interface.

/*  Create/destroy containers for marshalling read-only arguments
 *   and results between caller and plugin.
 */
flux_plugin_arg_t *flux_plugin_arg_create ();
void flux_plugin_arg_destroy (flux_plugin_arg_t *args);

const char *flux_plugin_arg_strerror (flux_plugin_arg_t *args);

/*  Flags for flux_plugin_arg_get/set/pack/unpack
 */
enum {
    FLUX_PLUGIN_ARG_IN =  0, /* Operate on input args  */
    FLUX_PLUGIN_ARG_OUT = 1  /* Operate on output args */
};

/*  Get/set arguments in plugin arg object using JSON encoded strings
 */
int flux_plugin_arg_set (flux_plugin_arg_t *args,
                         int flags,
                         const char *json_str);
int flux_plugin_arg_get (flux_plugin_arg_t *args,
                         int flags,
                         char **json_str);

/*  Pack/unpack arguments into plugin arg object using jansson pack style args
 */
int flux_plugin_arg_pack (flux_plugin_arg_t *args, int flags,
                          const char *fmt, ...);
int flux_plugin_arg_vpack (flux_plugin_arg_t *args, int flags,
                           const char *fmt, va_list ap);

int flux_plugin_arg_unpack (flux_plugin_arg_t *args, int flags,
                            const char *fmt, ...);
int flux_plugin_arg_vunpack (flux_plugin_arg_t *args, int flags,
                             const char *fmt, va_list ap);
@grondo grondo force-pushed the grondo:simplugins branch from a2f40df to e852851 Sep 11, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

I squashed and rebased the current iteration, since it isn't really the incremental development that is important here, but the final interface that is currently being proposed.

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

This looks great. The flux_plugin_strerror() is a nice touch that neatly encapsulates dlopen()'s somewhat odd error handling. I'm fine with merging if you are ready.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

It may make sense to merge this as a nice encapsulated unit, however, let me get further using this for the shell plugin implementation to make sure there are no missing (or just mis-) features.

Copy link
Member

left a comment

Looking good to me.

Feel free to set merge-when-passing and remove WIP prefix when you feel it's OK to go in.

@trws

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

The main one I'm thinking of is how they handle closures

That is really cool! I was actually heading somewhere like that but I don't think I was smart enough to accomplish it. It would be really awesome if we could eventually do something like that, but I fear I'm out of time to push this any farther right now! Sorry for the fail.

@trws

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@grondo grondo force-pushed the grondo:simplugins branch from e852851 to 9a528dd Sep 15, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

While working on shell plugins, I found that the addition of a per-callback void * data argument would greatly simplify things in some places, so I've added that here and force-pushed the branch.

The main difference is that the struct flux_plugin_handler has a void *data member, the flux_plugin_f function signature includes the same void *data argument, and of course flux_plugin_add_handler() allows the void * data to be passed along as the final arg.

@grondo grondo force-pushed the grondo:simplugins branch from 9a528dd to 6e147d6 Sep 15, 2019
@grondo grondo changed the title WIP: RFC: simple plugin interface for libflux libflux: add simple plugin interface Sep 15, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

This probably needs just another quick look with the addition mentioned above. Otherwise, since #2376 is based on this implementation, I have more confidence it won't need any more breaking changes in the near future.

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

I'm not seeing the void *data arg in flux_plugin_f or struct flux_plugin_handler. I've refreshed my working copy so I don't think I'm seeing a stale page. LMK if I'm just missing it somehow!

grondo added 3 commits Sep 9, 2019
Add a simple implementation of a flux_plugin_t class for libflux.

The interface boils down to a simple function dispatch service,
similar to the flux message dispatcher. Plugins can register (or have
registered on their behalf), handlers which should be invoked
when a given call "topic string" matches a glob associated with
the plugin's handler.

In addition to dynamic registration of callbacks, this interface
also supports dynamic registration of a plugin name. That is,
instead of requiring a global symbol for a plugin name, the
plugin itself (or an enitity on its behalf) can set the the
name with the flux_plugin_set_name() method.

This design has a few distict advantages:

 - statically loaded plugins are more easily implemented
 - plugins can load other plugins and set names/callbacks accordingly
 - plugins can call other plugins by invoking a custom topic string

There is only one function signature for all plugin callbacks:

 typedef int (*flux_plugin_f) (flux_plugin_t *p,
                               const char *topic,
                               flux_plugin_arg_t *args,
                               void *data);

which is meant to be general enough for all plugin use cases.
Instead of passing varargs or void * arguments to callbacks, plugin
arguments are placed into a plugin args object which supports
set/get and pack/unpack style interfaces for both input args and
output args (for returning results). The 'void *data' argument
is a per-callback context which can be set by the plugin registration
functions.

Depending on the host program, a plugin may also fetch relevant data
objects directly out the plugin handle with `flux_plugin_aux_get()` or
a provided wrapper.

Currently, the flux_plugin_t api supports only one method for dynamically
loading plugins: flux_plugin_load_dso (p, ...). This function opens
the plugin with dlopen(3), then calls flux_plugin_init() to allow
the plugin to register itself through flux_plugin_set_name(),
flux_plugin_add_handler(), or flux_plugin_register().

An interface for setting and retrieving plugin load-time configuration
is also provided as an alternative to (int argc, char **argv) style
arguments.
Add simple unit sanity tests for flux_plugin_t interface.
Expose flux plugin interface to <flux/core.h> by adding to flux.h
along with other libflux includes.
@grondo grondo force-pushed the grondo:simplugins branch from 6e147d6 to 750eb66 Sep 15, 2019
@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

Thanks for checking. I pushed from the wrong working copy. Just force-pushed the correct branch.

@garlick

This comment has been minimized.

Copy link
Member

commented Sep 15, 2019

Ah there it is. LGTM! I'll go ahead and set merge-when-passing.

@mergify mergify bot merged commit a6d362b into flux-framework:master Sep 16, 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 16, 2019

Codecov Report

Merging #2357 into master will increase coverage by 0.07%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #2357      +/-   ##
==========================================
+ Coverage   80.84%   80.92%   +0.07%     
==========================================
  Files         220      221       +1     
  Lines       34594    34819     +225     
==========================================
+ Hits        27969    28176     +207     
- Misses       6625     6643      +18
Impacted Files Coverage Δ
src/common/libflux/plugin.c 93.33% <93.33%> (ø)
src/common/libflux/mrpc.c 87.79% <0%> (-1.19%) ⬇️
src/broker/module.c 74.94% <0%> (-0.24%) ⬇️
src/common/libflux/message.c 80.5% <0%> (ø) ⬆️
src/broker/modservice.c 71.42% <0%> (+0.75%) ⬆️
@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
4 participants
You can’t perform that action at this time.