update FLUX_CLI_PLUGINPATH to support a colon-delimited path (multiple paths)

[wihobbs]

Problem: Currently CLI plugins are only loaded from a single path (FLUX_CLI_PLUGINPATH) which overrides the default path if set.

Support a colon-delimited list for the FLUX_CLI_PLUGINPATH and load plugins from all paths provided.

Fixes #6980

It's perhaps surprising, with this addition, that a command can fail if a plugin that collides with its option name has already been loaded. If we're going to follow the lead of other paths like PATH, perhaps we should just ignore that error and refer to the first option in FLUX_CLI_PLUGINPATH?

[grondo]

It's perhaps surprising, with this addition, that a command can fail if a plugin that collides with its option name has already been loaded. If we're going to follow the lead of other paths like PATH, perhaps we should just ignore that error and refer to the first option in FLUX_CLI_PLUGINPATH?

Great point. I agree the first plugin with a given name found in FLUX_CLI_PLUGINPATH should be used. This allows one to override a system-provided plugin (e.g. as might be done in a subproject) by prepending to FLUX_CLI_PLUGINPATH

[wihobbs]

Cool. Perhaps hold off on a review until I can fix that then :)

[grondo]

Another thing to consider: Now that a real search path is supported, we might want to think about standardizing the search path to include non-/etc paths for plugins which might eventually be provided by packages (e.g. in this case probably under /usr/libexec/flux) See e.g. #7039 for rationale and example

[wihobbs]

Ok @grondo, at long last this is ready for a real human to review it. :)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.55%. Comparing base (65e573a) to head (0a31429).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7244      +/-   ##
==========================================
- Coverage   83.56%   83.55%   -0.01%     
==========================================
  Files         562      562              
  Lines       93950    93967      +17     
==========================================
+ Hits        78509    78518       +9     
- Misses      15441    15449       +8     

Files with missing lines	Coverage Δ
src/bindings/python/flux/cli/base.py	95.74% <100.00%> (+0.02%)	⬆️
src/bindings/python/flux/cli/plugin.py	98.68% <100.00%> (+1.85%)	⬆️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[grondo]

Restarted a builder that hit #7406

[grondo]

Ok, leaving some comments after a first pass.

This appears to be working, excellent! The main thing I'd like to resolve before merging is how a user would prepend their plugin to the system supplied set.

[grondo]

So FLUX_CLI_PLUGINPATH overrides the system paths. I guess that preserves existing behavior, so was a good call here.

However, since CLI plugins are relatively new, I wonder if we could make a breaking change here for the better. It seems like FLUX_CLI_PLUGINPATH should be prepended to the builtin paths. This allows users to easily add CLI plugins without dropping access to system-supplied versions.

Of course, then there is no easy way to completely disable all system-supplied plugins, as might be useful for testing purposes.

The other alternative is a FLUX_CLI_PLUGINPATH_PREPEND variable. This matches existing practice of the FLUX_EXEC_PATH_PREPEND and FLUX_MODULE_PATH_PREPEND environment variables, e.g. here's the documentation for FLUX_MODULE_PATH_PREPEND:

       FLUX_MODULE_PATH_PREPEND
              FLUX_MODULE_PATH is set in the environment of the broker so that
              broker  modules  can  be  found  and  loaded  when  requested by
              flux-module(1):
                 $FLUX_MODULE_PATH_PREPEND : install-path : $FLUX_MODULE_PATH

What do you think?

[wihobbs]

Moreso than preserving existing behavior, I was thinking of the use case where a user doesn't want any of the system provided plugins.

I assumed anyone who wanted to both prepend to the FLUX_CLI_PLUGINPATH and maintain system plugins would know where those were and just include them in their environment setting. However, if we're expecting users to do that, that could go wrong very quick -- the user could end up inheriting their own environment in a subinstance and loading plugins from /etc that aren't relevant (or guaranteed to work in that context).

FLUX_CLI_PLUGINPATH_PREPEND is a long thing to type for what most users will want for default behavior (just guessing). How would you feel about this behavior @grondo:

• by default, load plugins from sysconfdir/cli/plugins:builtindir/cli/plugins
• if FLUX_CLI_PLUGINPATH is set, prepend that to the above
• allow users to set FLUX_CLI_PLUGINPATH_IGNORE_BUILTINS to completely override the system and libexec provided plugins.

[grondo]

Side note: I wonder if syscondir/cli/plugins should come before libexecdir/cli/plugins in the search path?

[wihobbs]

Huh, interesting point. I guess I was thinking that /etc/cli/plugins would contain Flux "core" plugins (if any) along with sysadmin-developed plugins, and libexecdir/cli/plugins would contain Flux "project" plugins that might want to override the core plugins.

In hindsight, it seems like my mental map violates the Filesystem Hierarchy Standard, and any Flux "core" plugins should not go in /etc, that should all be up to the sysadmins. In which case, I agree with you. Will fix.

[grondo]

I don't feel too strongly on this one. My guess is that administrators aren't going to need or want to override installed cli plugins, but the question did pop up in my head while looking at it. so I wanted to bring it up for discussion.

We'll need to find somewhere to document this behavior. Given the existence of flux-jobtap-plugins(7) and flux-shell-plugins(7), does it make sense to have flux-cli-plugins(7)?

[grondo]

I'm not sure I understand this comment. What is the problem being solved here?

It might be cleaner and clearer to first create the plugin instance, set its path, then append to the plugins list, e.g.:

    plugin = entry(program)
    plugin.path = path
    self.plugins.append(plugin)

[wihobbs]

What is the problem being solved here?

The comment is perhaps misleading; your solution would work fine and is cleaner. What I initially tried that would break existing plugins was to change the abstract base class to include an initializer:

class CLIPlugin(ABC):  # pragma no cover
    """Base class for a CLI submission plugin

    A plugin should derive from this class and implement one or more
    base methods (described below)

    Attributes:
        prog (str): command-line subcommand for which the plugin is active,
            e.g. "submit", "run", "alloc", "batch", "bulksubmit"
        prefix (str): By default, ``--ex-`` is added as a prefix to ensure
            plugin-provided options are namespaced. The prefix may be
            overridden with a site or project name.

    """

    def __init__(self, prog, path=None, prefix="ex", version=None):

This means all of the existing plugins would have to do work to make sure self.path = path was set in their initializers (as subclasses), or the later print_plugins() method would just print a bunch of Nones. Perhaps I should have thought of that with the version argument as well...

[grondo]

Since a plugin could be installed anywhere, setting the path from the loader makes sense to me.

I didn't check how version is handled, but if plugins have versions, it makes sense they capture their own version, perhaps as a class property/attribute that overrides the base class.

[grondo]

I wonder if there's a way we could move this check to load time, and just skip loading plugins with conflicts?

If instead of just appending plugins to the self.plugins list, we instead had a register(plugin, path) method, this would allow the path to be more naturally captured, and the dests dictionary could be built up as plugins are added. Then another check_conflict() method could be added that checks if a plugin has any conflicts, and could be used like:

   if not self.check_conflicts(plugin):
        self.register(plugin, path)

This would be cleaner IMO, and would speed up plugin loaded. However, it isn't a showstopper since the current code works. If you keep the current code, a comment explaining what's happening would be good (e.g. we iterate a copy of the list because it is being modified, etc)