plugins: cleanup sockets when done #5146
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5146 +/- ##
==========================================
- Coverage 61.82% 61.67% -0.16%
==========================================
Files 298 295 -3
Lines 20730 20778 +48
==========================================
- Hits 12817 12815 -2
- Misses 7000 7043 +43
- Partials 913 920 +7 |
laurazard
force-pushed
the
close-plugin-server
branch
from
936c490
to
3c7af39
Compare
cli-plugins/socket/socket.go
Outdated
| @@ -92,6 +94,7 @@ func (pl *PluginServer) Addr() net.Addr { | |||
| // | |||
| // The error value is that of the underlying [net.Listner.Close] call. | |||
| func (pl *PluginServer) Close() error { | |||
| logrus.Debug("Closing plugin server") | |||
There was a problem hiding this comment.
Would you still rather I remove this line @tonistiigi? I don't really mind either way, but I expect debug logs to be quite verbose.
There was a problem hiding this comment.
Currently, we don't have many debug logs for commands, and for example in buildx where we use --debug for extra verbosity for build errors, this can be confusing to users who have no context for "plugin server".
There was a problem hiding this comment.
Yeah the tricky bit with the CLI (unlike the daemon, where it usually ends up in a log file) is that logging in the CLI is.. printing in the user's terminal.
Would be nice if we had a location to store logs, but .. we don't, so we should not use it except for exceptional cases.
We could use "trace" level logging (one level more verbose than debug), but not sure if we pass that through currently (and not sure if containerd logs had it as option)
There was a problem hiding this comment.
Oh, that works!
logrus.Trace("Closing plugin server")./build/docker -l trace compose version
TRAC[0000] Plugin server listening on /var/folders/zj/khjjyd1n0zjd9xc44815szxc0000gn/T/docker_cli_9e516c003cda3c20ab79b88afc18434b
Docker Compose version v2.27.1-desktop.1
TRAC[0000] Closing plugin server
./build/docker -D compose version
Docker Compose version v2.27.1-desktop.1
There was a problem hiding this comment.
@tonistiigi this work for you?
There was a problem hiding this comment.
I think it should be fine; we can remove in a follow-up if it's a problem (but we need to look a bit at logging overall; I also did a quick attempt to see what it'd could look like if we switched over to containerd/log, as we did on the engine side; #5149
Since 509123f, we've been leaking sockets in the filesystem on platforms where abstract sockets aren't supported. That change relied on Go to cleanup our sockets for us, which Go will happily do as long as we make sure to close the listener, which we weren't previously doing unless to signal the plugin to terminate. This change adds a deferred call to `PluginServer.Close()`, which makes sure we close the plugin server at the end of the plugin execution, so that we never exit without cleaning up. Signed-off-by: Laura Brehm <laurabrehm@hey.com>
laurazard
force-pushed
the
close-plugin-server
branch
from
3c7af39
to
3dcc653
Compare
|
@laurazard how difficult would it be to write a test-case for this? (could be in a follow-up if we want to get this in) |
|
Had a quick chat; let's bring this one in, and do a follow-up for the test and any other changes we may need. |
- What I did
Since 509123f, we've been leaking sockets in the filesystem on platforms where abstract sockets aren't supported.
That change relied on Go to cleanup our sockets for us, which Go will happily do as long as we make sure to close the listener, which we weren't previously doing unless to signal the plugin to terminate.
- How I did it
This change adds a deferred call to
PluginServer.Close(), which makes sure we close the plugin server at the end of the plugin execution, so that we never exit without cleaning up.- How to verify it
Execute a plugin command with debug logging enabled (e.g.
docker -D compose convert) and look for the added debug log. Also check$TMPDIRfor leaked sockets.- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)