[chore] RFC: OCB Plugins/Hooks - #15638
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #15638 +/- ##
=======================================
Coverage 91.05% 91.05%
=======================================
Files 729 729
Lines 48478 48478
=======================================
Hits 44143 44143
Misses 3008 3008
Partials 1327 1327 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| ## Abstract | ||
|
|
||
| This RFC proposes a new feature in OCB that I am calling "hooks". It introduces a plugin interface for OCB plugins managed via [Hashicorp's Go Plugin framework][Hashicorp Go Plugin] which can be configured as hooks during OCB's process for generating and compiling a distribution. As of now, there are 4 hooks available: Pre/Post Generate, and Pre/Post Compile. Each hook can be individually skipped, or both skipped when Generation/Compilation respectively are disabled. |
There was a problem hiding this comment.
You had mentioned that using the Go Plugin framework makes it easier on hook authors, but after looking at the POC code I'm not entirely convinced the extra dependency has much value here, since we're typically making a single call, passing in one of four "actions" + the raw YAML config section, and getting back a potential error. An alternative could be to have hooks be simple binaries that communicate through stdin/stderr/files, with the ocbplugin module containing utilities to abstract over the communication method. I may try to fork your POC to try this out if I have the time.
There was a problem hiding this comment.
I'll be interested to see what you have in mind. The feeling I got was that writing our own standard for how to communicate between OCB and a valid OCB plugin binary was going to essentially be writing our own communication framework from scratch. Maybe there would be a nice way to write a library such that most of the details are abstracted away from plugin authors, but currently the only thing plugin authors explicitly have to know related to the "plugin" framework is that they need a main() that calls our ocbplugin.Serve function. Otherwise they're just implementing an interface with normal Go code.
I'm open to the idea if you want to put something together.
There was a problem hiding this comment.
I pushed a quick-and-dirty fork of your prototype on my branch (comparison with your branch, comparison with main), to demonstrate both of my suggestions:
- It runs each plugin binary directly with
cmd/exec, passing as a command line argument the path to a temporary YAML file containing the action (pre-generate, etc.) and the hook's YAML config - The plugin source config is spliced into the hook config itself, and
mod/versionis replaced by a singlegomodconfig. Thepathconfig was renamedgopathto avoid interfering with thescriptplugin's own config, whether this is the right way to avoid this conflict can be bikeshed later I think.
Part of the +150 -537 diff was me just deleting tests that no longer compile, but despite that I think it overall simplifies things. I've tested the prototype with this basic config:
dist:
name: otelcol-custom
description: Custom Collector Distribution
output_path: ./build
receivers:
- gomod: go.opentelemetry.io/collector/receiver/otlpreceiver v0.156.0
exporters:
- gomod: go.opentelemetry.io/collector/exporter/nopexporter v0.156.0
hooks:
post_generate:
- gopath: ./scriptplugin
path: ./scriptplugin/example/post_generate.sh(I tried testing the downloading of remote plugins, but I couldn't get pseudoversions to work like I wanted for this unfortunately. I'm not sure if you necessarily need a main directory to define a new binary?)
| hooks: | ||
| plugins: | ||
| - name: scriptplugin | ||
| module: go.opentelemetry.io/cmd/builder/scriptplugin | ||
| version: v0.157.0 | ||
| pre_generate: | ||
| - plugin: scriptplugin | ||
| path: ./scripts/pre_generate.sh | ||
| args: | ||
| - "--verbose" |
There was a problem hiding this comment.
Since the goal of hooks is to make running OCB as streamlined as possible (without a wrapper shell script), I wonder if we could simplify the config a tad:
hooks:
pre_generate:
- gomod: go.opentelemetry.io/cmd/builder/scriptplugin v0.157.0
path: ./scripts/pre_generate.sh
args: ["--verbose"](This proposal and the above are assuming the "actions" are fairly independent and there isn't a strong use case for keeping data in memory between separate actions for the same hook.)
There was a problem hiding this comment.
The goals for the setup I chose was:
- Install and run plugin subprocesses only once per OCB run
- Be able to detect already installed plugins in case of multiple runs on the same machine
- Support purely local plugins
2 would not be much of a downside if we fully rely on Go's module cache in a scenario where we only leverage go install. But for the other two points, I'm not sure how to support it in a scenario where we only rely on gomod inline to each hook execution.
There was a problem hiding this comment.
I'm not sure I see how 2 and 3 would be an issue here. ocb would run go install . in the local path like you're currently doing, then run the produced binary once for each hook.
Regarding 1 however, if "running plugin subprocesses only once per OCB run" is an absolute requirement, then indeed neither of my suggestions are applicable. But my assumption is that using the same plugin in multiple steps is fairly rare, and even in those cases, the overhead of running the same binary a handful of times is nothing compared to the actual work being done in the hook (let alone the Go compilation).
There was a problem hiding this comment.
It'd be great to add these goals to the text of the RFC
|
|
||
| ## Abstract | ||
|
|
||
| This RFC proposes a new feature in OCB that I am calling "hooks". It introduces a plugin interface for OCB plugins managed via [Hashicorp's Go Plugin framework][Hashicorp Go Plugin] which can be configured as hooks during OCB's process for generating and compiling a distribution. As of now, there are 4 hooks available: Pre/Post Generate, and Pre/Post Compile. Each hook can be individually skipped, or both skipped when Generation/Compilation respectively are disabled. |
There was a problem hiding this comment.
Would it make sense to start with a smaller number of hook types for now? It would be interesting to see what use cases cover each and see if e.g. we can start only with the one(s) needed for the OBI use case
There was a problem hiding this comment.
Maybe good to compare with https://ftp.osuosl.org/pub/rpm/max-rpm/s1-rpm-inside-scripts.html
| plugins: | ||
| - name: scriptplugin | ||
| module: go.opentelemetry.io/cmd/builder/scriptplugin | ||
| version: v0.157.0 | ||
| pre_generate: | ||
| - plugin: scriptplugin | ||
| path: ./scripts/pre_generate.sh | ||
| args: | ||
| - "--verbose" |
There was a problem hiding this comment.
Similar to the previous comment: could we pick one of these for the initial plugin support? There's a lot of overlap between the two
| hooks: | ||
| plugins: | ||
| - name: scriptplugin | ||
| module: go.opentelemetry.io/cmd/builder/scriptplugin | ||
| version: v0.157.0 | ||
| pre_generate: | ||
| - plugin: scriptplugin | ||
| path: ./scripts/pre_generate.sh | ||
| args: | ||
| - "--verbose" |
There was a problem hiding this comment.
It'd be great to add these goals to the text of the RFC
Description
This PR adds a new RFC for OCB build hooks using an external "plugin" interface through the Hashicorp go-plugin framework. Included in the RFC is a link to a full proof of concept.
Link to tracking issue
Related to #15430
Testing
All testing was done by manually building a local copy of
cmd/builderwith my changes and ensuring the new hooks worked like I expect.Documentation
The feature will be fully documented if the RFC is accepted.
Authorship
AI Usage Disclosure
The RFC was written entirely by me, but the POC leveraged Google Antigravity for some code generation.