feat(attachmux): add the stdio multiplexing package for multi-session attach - #5168
Draft
ekalinin wants to merge 7 commits into
Draft
feat(attachmux): add the stdio multiplexing package for multi-session attach#5168ekalinin wants to merge 7 commits into
ekalinin wants to merge 7 commits into
Conversation
Signed-off-by: Eugene Kalinin <e.v.kalinin@gmail.com>
Signed-off-by: Eugene Kalinin <e.v.kalinin@gmail.com>
Signed-off-by: Eugene Kalinin <e.v.kalinin@gmail.com>
Signed-off-by: Eugene Kalinin <e.v.kalinin@gmail.com>
A failed write to the container's stdin returned from readLoop, whose deferred dropSession took the whole session with it. A container that closed its stdin and kept printing lost its terminal on the next keystroke, and one that had just exited was reported as a broken session instead of a clean exit. Also from the same review pass: - Write splits a chunk larger than one frame instead of dropping it, and refuses the control stream, which is the broker's own. - EncodeFrame moved out of b.mu. - Dial stops retrying a refused connection, which unlike a missing socket means nothing is listening. - Session.Close is no longer reported by Stream as a lost connection. - The stdin pump stops forwarding once Stream returns, so it no longer swallows the next keystroke. - Serve's context watchdog no longer leaks when Serve returns first. - Probe trims an over-long name instead of probing a longer path than a real socket. - ErrUnsupported wraps errors.ErrUnsupported. - SocketPath tests no longer sit behind a build tag, and the unsupported stubs are covered. - RemoveSocket moves here from the logging step: it is package API. Signed-off-by: Eugene Kalinin <e.v.kalinin@gmail.com>
…dropped Four decisions from the review that are cheapest to settle while the package still has no consumers. A session's queue is bounded by bytes rather than frames. A TTY echoes single keystrokes, so a frame count evicted a session that was a couple of kilobytes behind, while a frame can be up to maxPayload, so a frame count also put no real ceiling on memory in the logging process. An evicted session is told so with a control frame before it is disconnected. Otherwise falling behind is indistinguishable from the broker dying, and the client reports both as a lost connection. Listen removes the socket against its inode instead of its path. Two brokers can briefly overlap for one container while the restart monitor swaps tasks; the new one taking the path over is correct, but the old one exiting afterwards would unlink the live socket and leave the new broker on an inode nobody can reach. A client now refuses only a broker newer than itself. A broker lives as long as its container, so a client that is newer is the normal case after an upgrade, and it has nowhere to fall back to. Signed-off-by: Eugene Kalinin <e.v.kalinin@gmail.com>
The assertion spelled the separator out, so it failed on windows once the test moved out from behind the build tag. That move was the point: these cases never needed a socket, and windows is where the package is only the unsupported stubs. Signed-off-by: Eugene Kalinin <e.v.kalinin@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First step towards multi-session
nerdctl attach(#3570, #4374). Design discussion is in #3570 (comment).This PR adds the
pkg/attachmuxpackage and nothing else.Why
A container's stdio FIFOs cannot be shared: a FIFO has one queue, so two
attachsessions each receive a random subset of the container's output. The fix is to give each container a single owner of its stdio and make every CLI session a client of it over a unix socket. This package is both halves of that conversation.What is here
proto.gobroker.goclient.goattach,run -itandstart -asocket_supported.gosocket_unsupported.goErrUnsupportedelsewhereattachmux.goErrUnsupported,SocketPathThe invariants this package exists to guarantee
Each of these is a bug in
attachtoday, and each has a test:nerdctl runwith attached stdio stops draining container stdout at ~72 KiB, deadlocking the container (nerdctl 2.3.5, containerd 2.3.3, Kata runtime-rs) #5137 / Fix foreground stdio deadlock when the internal logging process stops consuming #5151.Close(exited bool)only claims an exit when the caller knows there was one. The owner's stdio reaching EOF is not evidence: the same happens when the logging process is shut down while the container keeps running, and a client treats the exit frame as proof and stops streaming.Closenever waits on a blocked stdin write. It takes the descriptor out under the state lock and closes it after the sessions have drained; a write blocked on a FIFO the container stopped reading is evicted by the runtime poller rather than holding up shutdown.Testing
go test -race ./pkg/attachmux/..., 23 tests. Run repeatedly (-count=5, several rounds) on linux/arm64 because a good half of them are about concurrency: slow-session eviction,Closeracing with a detaching session,SetStdinracing withClose.golangci-lintis clean for linux, windows, freebsd and darwin.Notes for review
linux || freebsdonly, matching where the owner process will be built in a later step.Probereturning success where no broker can exist would be a capability check that lies.SocketPathis a pure function of the data store, namespace and container ID, deliberately with nothing recorded on disk. The reasoning, including why it cannot live in$XDG_RUNTIME_DIR, is in the issue comment linked above.