Plugin loader #4654
Plugin loader #4654
Conversation
| for k, config := range plugins { | ||
| // Create an instance | ||
| raw := config.Factory(l.logger) | ||
| base, ok := raw.(base.BasePlugin) |
There was a problem hiding this comment.
Why can't Factory return a BasePlugin and avoid this assertion?
plugins/shared/loader/init.go
Outdated
| info.baseInfo = i | ||
|
|
||
| // Parse and set the plugin version | ||
| if v, err := version.NewVersion(i.PluginVersion); err != nil { |
There was a problem hiding this comment.
[nit] if info.version, err = version.New...
plugins/shared/loader/init.go
Outdated
| if v, err := version.NewVersion(i.PluginVersion); err != nil { | ||
| multierror.Append(&mErr, fmt.Errorf("failed to parse version %q for internal plugin %s: %v", i.PluginVersion, k, err)) | ||
| continue | ||
| } else { |
There was a problem hiding this comment.
No need for an explicit else
plugins/shared/loader/init.go
Outdated
|
|
||
| var plugins []os.FileInfo | ||
| for _, f := range files { | ||
| if f.IsDir() { |
There was a problem hiding this comment.
Should we filter out files without the execute bit set? Thinking people might want to drop a README.txt or something else in the dir.
There was a problem hiding this comment.
Need to test if f is a symlink and if so resolve it and test if its a directory.
(Or use Readdirnames and Stat which follows symlinks for you.)
| info, err := l.fingerprintPlugin(p, c) | ||
| if err != nil { | ||
| l.logger.Error("failed to fingerprint plugin", "plugin", name) | ||
| multierror.Append(&mErr, err) |
There was a problem hiding this comment.
So will Nomad fail to start if a plugin fails to fingerprint?
I think this is the correct behavior as it seems really bad to allow Nomad to run without a driver plugin that may be necessary to communicate with running tasks. An operator can always fix these errors by removing the offending file (or removing the execute bit if you make that change).
Just wanted to verify as this seems like an important decision.
There was a problem hiding this comment.
Yes the New method to retrieve the factory which will happen in agent.go will return an error.
plugins/shared/loader/init.go
Outdated
| if v, err := version.NewVersion(i.PluginVersion); err != nil { | ||
| return nil, fmt.Errorf("failed to parse plugin %q (%v) version %q: %v", | ||
| i.Name, info.exePath, i.PluginVersion, err) | ||
| } else { |
plugins/shared/loader/init.go
Outdated
| if extPlugin.version.LessThan(internal.version) { | ||
| l.logger.Info("preferring external version of plugin", | ||
| "plugin", extPlugin.baseInfo.Name, "internal_version", internal.version.String(), | ||
| "external_version", extPlugin.version.String()) |
There was a problem hiding this comment.
No need to explicitly call String(). Anything that implements String() will have it called automatically.
plugins/shared/loader/loader.go
Outdated
| return nil, fmt.Errorf("invalid plugin loader configuration passed: %v", err) | ||
| } | ||
|
|
||
| logger := config.Logger.Named("plugin-loader").With("plugin-dir", config.PluginDir) |
There was a problem hiding this comment.
Hm, I've been using underscores as a separator in log names/keys so it looks like an identifier and matches hcl params (when applicable): https://gist.github.com/schmichael/da63914230b5127fb26486b01446f5a5
I don't mind switching, but we should probably standardize.
plugins/shared/loader/loader_test.go
Outdated
| } | ||
|
|
||
| // newHarness returns a harness and copies our test binary to the temp directory | ||
| // with teh passed plugin names. |
| logger := log.Default() | ||
| logger.SetLevel(log.Trace) | ||
| lconfig := &PluginLoaderConfig{ | ||
| Logger: logger, // XXX Use testlog package |
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR provides a plugin loader that scans a plugin directory for plugins and provides a unified interface for launching and reattaching to plugins whether they are internal or external.
The loader will configure the plugin before handing the instance back to the caller. The unit tests are slightly complicated, but the way it works is that there is a plugin main which allows switching to running the binary as a mock plugin. The plugin currently only can be a device plugin but can be extended later.
The test harness creates a plugin directory and then copies itself to the temp folder. This provides binaries that can be fingerprinted and run as plugins.