Prototype plugin architecture #1208
Conversation
|
Easy to fix CI issue, @dominiklohmann: |
dominiklohmann
force-pushed
the
story/ch21022/prototype-plugin-architecture
branch
from
19bdd66
to
e3b39d8
Compare
dominiklohmann
force-pushed
the
story/ch21022/prototype-plugin-architecture
branch
from
e3b39d8
to
c5ac0bf
Compare
|
@dominiklohmann I'm a bit clueless why CI started failing. Do you have an idea? |
Looks unrelated to me. Does it reproduce after you re-run the CI? |
|
Seems like the GCC check now passes. But the Docker issue still persists. Unfortunately I'm a bit clueless how to go after this one. My hunch was that the schema definitions are not available with the example configuration, but they are copied as well. 🤷♂️ |
There was a problem hiding this comment.
From the user API this looks pretty good to me, so the comments all concern the implemtation side.
There also seem to be two empty README.md files left over that I can't comment on directly, these should probably be removed of filled with example documentation.
| // Load plugins. | ||
| auto& plugins = plugins::get(); | ||
| auto plugin_dirs = detail::get_plugin_dirs(cfg); | ||
| for (const auto& dir : plugin_dirs) { |
There was a problem hiding this comment.
I think it would be good to require a more explicit opt-in for plugin loading, this is executing arbitrary code after all and everyone with access to the current users home directory can inject it.
Not completely sure what the best solution would be, but some ideas:
- a build-time switch to turn off plugin support completely
- a run-time --enable-plugins switch
- a list of specific dynamic plugins to load, intead of directories, like
--enable-plugin=/usr/lib/vast/plugins/matcher.so
There was a problem hiding this comment.
We'll postpone this to a separate story so we can discuss this prperly.
There was a problem hiding this comment.
Can we at least turn it off by default then until it is discussed? It may be an experimental feature, but it will also be present on production systems.
|
|
||
| /// Checks if a version meets the plugin version requirements. | ||
| /// @param version The version to compare against the requirements. | ||
| bool has_required_version(const plugin_version& version) noexcept; |
There was a problem hiding this comment.
Although in the medium- to long-term we certainly need a separate plugin versioning scheme (as soon as we have binary third-party plugins), for now we could probably save ourselves a lot of headaches and lingering compatibility bugs by requiring that all plugins must be built from the same commit as vast itself.
There was a problem hiding this comment.
We're keeping this as is, doesn't hurt to have this now already. We can always make a breaking change while it's labeled experimental.
There was a problem hiding this comment.
I was more thinking about the scenario where we unintentionally break ABI but forget to bump the major version of the plugin API, leading to strange memory corruption.
Actually, while writing this I noticed that the implementation of this function also seems off, at the least the major version should match exactly as opposed to being <=.
This came up in a review: While I prefer to generally always use `auto&&` or `const auto&`, and never `auto&`, because the former always work, we already use the latter for other calls to the same function and we should do so consistently.
Co-authored-by: Matthias Vallentin <matthias@tenzir.com>
|
#1261 confirmed that the Docker workflow failure is unrelated. Let's merge regardless of it and fix that separately. |
📔 Description
A working prototype to load plugins as shared objects with hooks in vast.yaml to pass configuration options to plugins. Provides an example plugin that hooks into the table slice stream of the importer.
📝 Checklist
🎯 Review Instructions
File-by-file feels best.