Close transient VZ control connections - #482
Conversation
Disable HTTP keep-alives for one-shot VZ control clients and honor dial cancellation. Bound idle control sessions in the shim, evict the pooled guest connection on terminal stop, and cover successful, error, cancellation, and repeated client paths with Unix ConnState regressions.
chruffins
left a comment
chruffins
left a comment
There was a problem hiding this comment.
lgtm but just needs a regression test in one spot!
| // the live peer, but the keyed gRPC ClientConn must not survive into a later | ||
| // start that reuses the same per-instance socket path. | ||
| if dialer, err := hypervisor.NewVsockDialer(inst.HypervisorType, inst.VsockSocket, inst.VsockCID); err == nil { | ||
| guest.CloseConn(dialer.Key()) |
There was a problem hiding this comment.
The existing VZ stop/restart test won't catch removal of this eviction: its first failed post-restart ExecIntoInstance attempt calls guest.CloseConn on retryable errors, and the surrounding test retries the exec. Please add a regression assertion that proves the stop path evicts the old pooled connection, or otherwise verifies the first post-restart RPC does not reuse it.
|
Regression test added, ready to re-review, thank you. |
|
Thank you Chris |
|
P.s. @chruffins Do you know anyone who would care about this that you could ping? kernel/neko#17 |
|
Hey, could you open a PR for that? I'd be happy to take a look / ask other engineers. |
|
Opened one for each part, def appreciate if you can get anyone to look at it! <3 #22: Fix XTest scroll fallback units |
2 participants
Each five-second VZ state refresh constructs a new client and transport. After a successful
/vm.inforesponse, that transport kept its Unix HTTP connection idle forever, so a continuously polled VM accumulated one API/shim socket pair per refresh.This change disables keep-alives for these one-shot VZ control transports, uses
DialContextfor cancellation-aware Unix dialing, gives the shim a 30-second idle timeout as defense in depth, and evicts the pooled guest-agent gRPC connection on terminal stop before the per-instance socket paths can be reused. Synthetic Unix HTTP tests track server connections throughConnStateand require them to return to zero after successful info reads, HTTP/JSON errors, cancellation, and 128 repeated client constructions.Validation:
go test -race -count=100 -tags containers_image_openpgp -run '^(TestRepeatedClientsReleaseSuccessfulControlConnections|TestControlConnectionsCloseAfterResponseErrors|TestControlConnectionClosesAfterCancellation)$' ./lib/hypervisor/vzgo vet -tags containers_image_openpgp ./lib/hypervisor/vz ./lib/instances ./cmd/vz-shimgo test -tags containers_image_openpgp -run '^$' ./lib/instances ./cmd/vz-shimmake build-darwinAs a regression control, temporarily re-enabling keep-alives makes the repeated-client test fail on its first iteration with two retained connections (constructor ping plus info read).
Note
Medium Risk
Touches instance stop cleanup and VZ HTTP transport behavior; changes are narrow and covered by new regression tests, but affect lifecycle and hypervisor control paths.
Overview
Fixes connection leaks from short-lived VZ control traffic and from guest-agent gRPC surviving instance stop.
The VZ hypervisor client now uses context-aware Unix dialing and
DisableKeepAlivesso each one-shot control client does not leave an idle HTTP connection on the shim aftervm.info/ lifecycle calls. The vz-shim controlhttp.Servergets a 30sIdleTimeoutas a backstop.On terminal stop, the instance manager calls
guest.CloseConnfor that VM’s vsock dialer key before reusing per-instance socket paths, so a pooledClientConnis not carried into the next start.New darwin tests assert the fake Unix control server returns to zero connections after success, HTTP/JSON errors, cancellation, and 128 repeated client constructions; a lifecycle noop test asserts stop evicts and shuts down the pooled guest connection.
Reviewed by Cursor Bugbot for commit 6c87311. Bugbot is set up for automated code reviews on this repo. Configure here.