Introduce support for CLI plugin hooks #4376
Conversation
laurazard
changed the title
laurazard
force-pushed
the
plugin-hooks
branch
2 times, most recently
from
bee2b62
to
06a6617
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4376 +/- ##
===========================================
+ Coverage 0 61.53% +61.53%
===========================================
Files 0 292 +292
Lines 0 20378 +20378
===========================================
+ Hits 0 12539 +12539
- Misses 0 6945 +6945
- Partials 0 894 +894 |
laurazard
force-pushed
the
plugin-hooks
branch
2 times, most recently
from
8950476
to
ac19075
Compare
laurazard
changed the title
laurazard
force-pushed
the
plugin-hooks
branch
5 times, most recently
from
f70b8e7
to
10e3444
Compare
cli-plugins/manager/hooks.go
Outdated
|
|
||
| fmt.Fprintln(out, aec.LightBlueF.Apply("\nNext steps:")) | ||
| for _, m := range messages { | ||
| fmt.Fprintf(out, " - %s\n", m) |
There was a problem hiding this comment.
Looks like "list" layout is enforced here. Should we also make sure that each message is 1-line only?
There was a problem hiding this comment.
Unsure about this one. I think it would be fine to have a multiline list type of hints, e.g.:
Next steps:
- Keep in mind that lorem ipsum dolor sit amet.
(also, pellentesque ac eleifend tellus, ac malesuada dolor)
- Nullam luctus rutrum fringilla
What does everyone think?
There was a problem hiding this comment.
Maybe we could format each line of a message (after the first) so they at least start with the same indentaton? (a small terminal would wrap the lines, so in that case it'd look ugly anyway)
There was a problem hiding this comment.
Maybe we could format each line of a message (after the first) so they at least start with the same indentaton?
Next steps:
- Keep in mind that lorem ipsum dolor sit amet.
(also, pellentesque ac eleifend tellus, ac malesuada dolor)
- Nullam luctus rutrum fringilla
Like this? Not sure if this is clearer or not for me.
There was a problem hiding this comment.
Exactly! I think it looks much better (but that might just be a strong bias on my side 😅)
There was a problem hiding this comment.
Today in docker scout we don't display them as list items, even if there's multiple items:
There was a problem hiding this comment.
I'll take a jab at it – I like the idea, but slightly concerned about totally breaking formatting with the indentation in smaller terminals.
laurazard
force-pushed
the
plugin-hooks
branch
2 times, most recently
from
fb02226
to
943d2f5
Compare
This is from codespaces and can be replicated there. Just make sure to install desktop to get the plugins bundle as only compose&buildx are installed by default. 8 seconds is indeed unexpected and it doesn't fit linear scaling from plugins count(considering rc2 baseline takes 300ms on same mode), but I think any double-digit percentage (=measurable) performance regression should be avoided. I added some debug on that drop_cache run. Clearly still coming from that function. I think previously I had one extra plugin leftover and that causes the 8s -> 6s difference. |
cli-plugins/hooks/printer.go
Outdated
| fmt.Fprintln(out, aec.LightBlueF.Apply("\nNext steps:")) | ||
| for _, n := range messages { | ||
| _, _ = fmt.Fprintf(out, " - %s\n", n) | ||
| } |
There was a problem hiding this comment.
Current implementation of hooks looks like this:
And with multiple messages:
Basically two main differences:
Next steps:vsWhat's Next?and colors-in front of each lines
I can change the scout CLI to add - on each line, so we ensure it's done the same way everywhere.
About the title I wonder if What's next? isn't a bit better. But I'll defer to @jalonsogo
|
Replying here to provide a coherent response to the concerns brought up by @tonistiigi: To properly test performance, I made the following changes to diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go
index 431b1e027..ca9142151 100644
--- a/cmd/docker/docker.go
+++ b/cmd/docker/docker.go
@@ -306,7 +306,7 @@ func runDocker(dockerCli *command.DockerCli) error {
}
}
- var subCommand *cobra.Command
+ // var subCommand *cobra.Command
if len(args) > 0 {
ccmd, _, err := cmd.Find(args)
if err != nil || pluginmanager.IsPluginCommand(ccmd) {
@@ -318,7 +318,7 @@ func runDocker(dockerCli *command.DockerCli) error {
// cmd.Execute() which deals with reporting
// "command not found" in a consistent way.
} else {
- subCommand = ccmd
+ // subCommand = ccmd
}
}
@@ -332,9 +332,9 @@ func runDocker(dockerCli *command.DockerCli) error {
// If the command is being executed in an interactive terminal,
// run the plugin hooks (but don't throw an error if something misbehaves)
- if dockerCli.Out().IsTerminal() && subCommand != nil {
- _ = pluginmanager.RunPluginHooks(dockerCli, cmd, subCommand)
- }
+ // if dockerCli.Out().IsTerminal() && subCommand != nil {
_ = pluginmanager.RunPluginHooks(dockerCli, cmd, subCommand)
+ // }
return nil
}And built a version, with all the same code, with and without hooks. After that, I benchmarked with: hyperfine --prepare 'sudo sh -c "sync; echo 1 > /proc/sys/vm/drop_caches"' './build/docker version' './build/docker-with-hooks version'So, lets test this:
We'll test with a number of different plugins: buildx: Docker Buildx (Docker Inc.)
compose: Docker Compose (Docker Inc.)
feedback: Provide feedback, right in your terminal! (Docker Inc.)
hints: Docker Hints (Docker Inc.)
long: Docker Long Running Example Plugin (Docker Inc.)
Benchmark 1: ./build/docker version
Time (mean ± σ): 28.9 ms ± 7.2 ms [User: 3.5 ms, System: 11.9 ms]
Range (min … max): 25.5 ms … 61.1 ms 23 runs
Benchmark 2: ./build/docker-with-hooks version
Time (mean ± σ): 66.8 ms ± 3.6 ms [User: 32.8 ms, System: 65.8 ms]
Range (min … max): 64.1 ms … 78.1 ms 28 runs
Summary
./build/docker version ran
2.31 ± 0.59 times faster than ./build/docker-with-hooks versionSo that's ~40ms difference. With more plugins: buildx: Docker Buildx (Docker Inc.)
compose: Docker Compose (Docker Inc.)
dev: Docker Dev Environments (Docker Inc.)
feedback: Provide feedback, right in your terminal! (Docker Inc.)
hints: Docker Hints (Docker Inc.)
init: Creates Docker-related starter files for your project (Docker Inc.)
long: Docker Long Running Example Plugin (Docker Inc.)
Benchmark 1: ./build/docker version
Time (mean ± σ): 30.0 ms ± 5.8 ms [User: 4.4 ms, System: 11.7 ms]
Range (min … max): 24.8 ms … 52.3 ms 21 runs
Benchmark 2: ./build/docker-with-hooks version
Time (mean ± σ): 71.9 ms ± 8.1 ms [User: 38.3 ms, System: 83.8 ms]
Range (min … max): 66.6 ms … 103.4 ms 22 runs
Warning: The first benchmarking run for this command was significantly slower than the rest (103.4 ms). This could be caused by (filesystem) caches that were not filled until after the first run. You are already using the '--prepare' option which can be used to clear caches. If you did not use a cache-clearing command with '--prepare', you can either try that or consider using the '--warmup' option to fill those caches before the actual benchmark.
Summary
./build/docker version ran
2.40 ± 0.54 times faster than ./build/docker-with-hooks versionHuh that's about 40ms too. Lets try more: buildx: Docker Buildx (Docker Inc.)
compose: Docker Compose (Docker Inc.)
dev: Docker Dev Environments (Docker Inc.)
feedback: Provide feedback, right in your terminal! (Docker Inc.)
hints: Docker Hints (Docker Inc.)
init: Creates Docker-related starter files for your project (Docker Inc.)
long: Docker Long Running Example Plugin (Docker Inc.)
sbom: View the packaged-based Software Bill Of Materials (SBOM) for an image (Anchore Inc.)
Benchmark 1: ./build/docker version
Time (mean ± σ): 27.1 ms ± 1.8 ms [User: 4.1 ms, System: 10.9 ms]
Range (min … max): 25.2 ms … 35.7 ms 30 runs
Benchmark 2: ./build/docker-with-hooks version
Time (mean ± σ): 71.9 ms ± 4.2 ms [User: 45.8 ms, System: 90.9 ms]
Range (min … max): 66.4 ms … 85.0 ms 31 runs
Summary
./build/docker version ran
2.66 ± 0.24 times faster than ./build/docker-with-hooks versionIt's still about the same. It seems like the performance penalty does not depend on the number of plugins, as we thought (from the original PR text):
Adding scout: That's 60ms, 20ms slower than without. I guess that's because whereas other plugins are a good bit faster This checks out with what we expect. Currently, the performance penalty is as we expect, barring any strange edge-cases. As called out in the PR description, a mitigation (that we're planning on working on after merging this) for this issue is to cache plugin metadata so that we don't always have to execute every plugin to see if they support hooks. I'll also be adding a very naive timeout to the plugin scanning logic, to make sure we cap that out and never let it go longer than 80 or 100ms |
You can see in your output how user/system gradually grows with every added plugin, spinning the CPU more each time. In the last example, the resource usage is 14x of baseline, 9x before scout is added. As these plugins all run in their own goroutine, the real time depends on cpu count, OS's exec implementation(including malware detection mechanisms), prewarmed cache, system load, and other factors. Docker is run by millions of users, with various os/arch/perf configurations. Afaik Desktop does not come with a free 10+ core machine. If you compare absolute numbers then obviously users with low perf and low cpu count would see the impact more (and CLI is released to some really low-powered machines) than machines with a lot of resources to spare. To compare the absolute numbers, you would need a big test matrix of all the OS(maybe even kernels & infra providers), different ranges of cpu counts and perf generations, memory pressure, invocation frequency etc. But we don't need to compare the absolute numbers, just that previously we needed resources to run 1 binary and now we need resources to run 10 (and more with any future update or extra plugin the user has added). Unless you can defy some laws of physics, the latter will be slower for the users.
Scout binary is more than 4x bigger than dev/debug/init/feedback etc small plugins. Minimally, the extra resources need to be spent to load these binaries into memory, but it is not impossible that there is some
That's another position where we differ. To me, any measurable performance regression from spinning the CPU needlessly should be avoided. Comparing this to other historic regressions I've either fixed or reported in the past, I think this is the biggest one yet (at least if the high multi-core machines are excluded). |
|
This (current) performance discussion is moot as we'll be making some implementation changes that allow us to not have to scan/execute all the plugins, but for discussion's sake:
Yes, obviously cpu time scales with # plugins. I'd expected that we were discussing real time before, esp. since we're talking about the Desktop plugin bundles that most people would be running on their main development computers, and not in CI/etc. Otherwise if we're discussing CI environments why are you testing with more than 2 plugins? We can discuss either case, but discussing the worst case scenario of "extremely restricted environment but for some reason with Docker Desktop + all the plugins bundled installed" doesn't seem very productive.
Yes, it is. That still does not mean that we cannot incur a performance hit ever, especially when this is done as a step in the direction of overall improving performance.
Maybe I wasn't clear in my previous reply, but this sentence –
I agree. Which is why we're implementing this – so we can remove the wrapper and the "needless cpu spinning" that it does. Regardless, I'll amend the PR soon – I was discussing with @thaJeztah and we were thinking about instead storing whether a plugin supports hooks in the CLI's config file, which would negate the need to cycle over/execute all the plugins to check for support. |
|
Pushed a change reworking the plugin hook listing+scanning logic – instead of scanning the plugin folders and executing every candidate to check their metadata for hook support, we now expect plugins with hooks to be configured in the CLI's {
"auths": {},
"plugins": {
"hints": {
"hooks": "pull,build"
}
}
}The new logic looks only for plugins configured this way, which significantly reduces execution time (and makes it so that performance does not get worse with the amount of plugins bundled). Applying the above patch (executes plugins even if not attached to a tty): Benchmark 1: ./build/docker version
Time (mean ± σ): 9.3 ms ± 1.0 ms [User: 3.7 ms, System: 3.1 ms]
Range (min … max): 6.8 ms … 13.7 ms 211 runs
Benchmark 2: ./build/docker-without-hooks version
Time (mean ± σ): 9.4 ms ± 2.0 ms [User: 3.6 ms, System: 3.2 ms]
Range (min … max): 6.8 ms … 40.6 ms 288 runs
Summary
./build/docker version ran
1.01 ± 0.24 times faster than ./build/docker-without-hooks versionThat is for ./build/docker pull alpine
Using default tag: latest
latest: Pulling from library/alpine
Digest: sha256:c5b1261d6d3e43071626931fc004f70149baeba2c8ec672bd4f27761f8e1ad6b
Status: Image is up to date for alpine:latest
docker.io/library/alpine:latest
Next steps:
- Run this image with `docker run alpine`
RunPluginHooks took 12.409638ms
...
RunPluginHooks took 8.642533ms
...
RunPluginHooks took 14.853688msfrom end to end, including calling the hook plugin, which is a lot more reasonable. Still want to do a quick run through of some codebases to check no plugin is going to break if we do this. |
laurazard
force-pushed
the
plugin-hooks
branch
6 times, most recently
from
6138fe1
to
39b6c40
Compare
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
- What I did
Introduce a "hooks" mechanism for CLI plugins, which enables CLI plugins adding useful, contextual messages/actions after a command has been executed.
This works by defining a special plugin command (such as the metadata command) which the CLI calls with some information about the just-executed command (currently, command and flag names). The plugin then prints a json-encoded to it's stdout with a template for the message it would like the CLI to print.
Using such a system, a plugin to help new docker users could, for example, declare intent to hook into
docker build, and return a templated message instructing the user how to run their newly-built image.A templating approach was chosen to allow plugin messages to include information from the original command execution (such as the tag used for the image in
build), so that messages can provide more useful information (such as printing "Run your image withdocker run my-image" after adocker build -t my-image .Helpers are provided in the form of
manager.TemplateReplaceFlagValue("tag")for this case, and use Go Templates under the hood.The templating approach chosen also keeps sensitive data from leaving the CLI process.
This PR also introduces a "features" map into the CLI
config.json, to allow for global enabling/disabling of this (and future) features.Example Plugin
Which results in:
Performance Callouts
Edited: for current approach + benchmarks see #4376 (comment)
Security Callouts
The current approach works by letting plugins declare a list of strings which are the CLI commands (such as
build,run, etc.) whose execution the plugin wants to be hooked into. The CLI then only invokes each plugin for the commands the plugin has declared support for.Since a plugin might want to be invoked for more than one command, some information must be passed into the plugin when it is invoked so that it can decide what information it wants to add – which for right now, is codified into the
HookPluginDatatype and contains only a string representing the command being executed. Richer output is made possible by employing a templating approach, wherein the plugin can return a template string with special values which the CLI then processes – e.g.This allows for plugins to provide useful/contextual messages, without passing any potentially sensitive information to any other binary.
Possible future developments to make this more secure are things such as always being explicit when a plugin is invoked, so that the user is aware, or prompting the user whether they are okay with a plugin hooking into a command execution on first execution.
- A picture of a cute animal (not mandatory but encouraged)
Release Notes: