guest/stdio: keep stdio alive across LCOW live migration - #2793
guest/stdio: keep stdio alive across LCOW live migration#2793shreyanshjain7174 wants to merge 5 commits into
Conversation
| written := 0 | ||
| for written < len(p) { | ||
| n, err := w.Write(p[written:]) | ||
| written += n |
There was a problem hiding this comment.
Shouldnt you only increment written after the err check. If its an err you didnt write it and so shouldnt have incremented.
There was a problem hiding this comment.
This matches io.Copy: per the io.Writer contract n is the bytes actually written even when err is non-nil, so on a partial write those bytes already left. copyOut holds p[written:] and replays only that on the re-dialed conn, so counting the partial n is what stops it re-sending and duplicating those bytes. Moving the increment after the err check would replay the already-sent prefix; TestCopyOutHoldsPartialWriteRemainder covers it. Added a comment on that line in the latest push to make the intent explicit.
| }() | ||
| if settings.StdIn != nil { | ||
| c, err := tport.Dial(*settings.StdIn) | ||
| port := *settings.StdIn |
There was a problem hiding this comment.
nit: These are all cosmetic changes and do not impact any functionality. Please revert it to earlier state.
There was a problem hiding this comment.
Reverted these back to the original in the rebase (your 756c712). Thanks.
| } | ||
| if settings.StdOut != nil { | ||
| c, err := tport.Dial(*settings.StdOut) | ||
| port := *settings.StdOut |
There was a problem hiding this comment.
nit: These are all cosmetic changes and do not impact any functionality. Please revert it to earlier state.
| } | ||
| if settings.StdErr != nil { | ||
| c, err := tport.Dial(*settings.StdErr) | ||
| port := *settings.StdErr |
There was a problem hiding this comment.
nit: These are all cosmetic changes and do not impact any functionality. Please revert it to earlier state.
| bridgeOut = bridgeCon | ||
| } | ||
|
|
||
| // The bridge is up again; let the stdio relays resume any copiers that |
There was a problem hiding this comment.
We can simplify this logic about indicating the bridge being disrupted due to migration. We can make it generic so that If the connection was disrupted due to any reason, we would retry for 6 Seconds before giving up.
@jterry75 @shreyanshjain7174 What do you think?
See commit- 756c712
There was a problem hiding this comment.
Picked up your 756c712, thanks. The bridgeDown flag and the cmd/gcs wiring are gone, and the copiers now just retry on any disconnect for the redial window instead of keying off a migration flag.
| logrus.ErrorKey: err, | ||
| "file": name, | ||
| }).Error("opengcs::PipeRelay::copyAndCleanClose - error reading for clean close") | ||
| pr.mu.Lock() |
There was a problem hiding this comment.
The code in this file is too dense and hard to follow. There are way too many lock/unlock patterns. Please simplify the same.
Note: Specific to lock/unlock pattern- Do not snapshot under lock and then use it outside of lock as it defeats the purpose. There can be race condition wherein the value got swapped as soon as you snapshotted and released the lock.
Therefore, consider taking longer locks if needed.
There was a problem hiding this comment.
Simplified in 93e5130. run and runCopiers no longer take the lock at all. The swap and teardown now go through a small setConn helper, plus a teardown helper on the tty relay, and the snapshot-under-lock-then-use-outside reads are gone. The manager goroutine is the only writer of the set, so the lock is only there to keep the connection close from racing Wait's CloseRead. Race tested at 50 runs.
67496fa to
93e5130
Compare
d5627a6 to
e6d811d
Compare
A live migration pauses the UVM and drops the GCS vsock bridge, which then re-dials on the destination. Today the stdio relay tears the process down when that happens, and io.Copy throws away whatever it had buffered, so in-flight stdout/stderr is lost. Now the relay pauses instead. On a copy failure mid-migration a manager goroutine re-dials, swaps the conn, and resumes. A bridgeDown flag, set by cmd/gcs around the reconnect loop, is what tells a migration pause apart from a real process exit. The output copy drops io.Copy for a read/write-all loop that keeps the unwritten tail and replays it on the new conn, so the relay does not lose bytes. I tried two other designs first: a per-conn wrapper that parks Read/Write until reconnect (leaves the relays alone but wraps every read/write), and rewriting both relays into pump loops with a registry (touches the shared relay path every container and exec runs through). Went with pause-on-error because the conn is only swapped after the copiers stop, so nothing mutates a live conn under a running copy. Caveat: it is reactive. Whatever the kernel or socket already dropped at the blackout is gone; this only removes the relay-level drop. And if the host closes the conns just before bridgeDown flips, that stream still tears down. Tested under -race and on a two-node migration: the container keeps streaming across the move. Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
The io.Writer contract returns n as the bytes written even on a partial-write error, so counting it before the error check lets copyOut replay only the unwritten tail and never re-send those bytes. Addresses a review question. Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
Collapse the repeated connection swap and teardown lock blocks into a small setConn helper (plus a teardown helper for the tty relay), and drop the snapshot-under-lock-then-use-outside reads in run and runCopiers since the manager goroutine is the sole writer of the connection set. Same serialization of the conn close against the Wait CloseRead, fewer lock sites. Also drop a stale bridgeDown mention left in a comment. Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
Start relays only after runtime start succeeds and tear them down synchronously when start fails, and replace the pause-flag plumbing with explicit needsRedial/canRedial semantics. A relay round ends only when every copier returns, but a copier parked in a blocking read of an idle process pipe or pty yields neither data nor EOF while the container runs. On a live-migration destination the host connections are dead, so the copier that notices returns errNeedsRedial and then waits forever on its siblings: the round never ends, the redial never starts, the stdout pipe fills, and the workload freezes. Wake the parked copiers instead of waiting for them, by arming an already-elapsed read deadline on the relay-owned readers and shutting down the stdin connection. Deadlines are used for the pipe and pty reads because the vendored vsockConn.SetReadDeadline is a no-op stub and Close does not interrupt an in-flight read; CloseRead is used for stdin for the same reason Wait already used it. The container's own pipe ends are untouched, so the process keeps its stdio across the redial. The authoritative trigger is the GCS bridge reconnecting, since a copier write failure cannot fire for a stream with no traffic. That signal is generation counted rather than a bare closed channel, so an event landing between rounds or inside redialWithRetry is not lost, and each relay holds a durable subscription across rounds. The pty ioctls move off os.File.Fd, which reverts the descriptor to blocking mode and drops it from the runtime poller for good. After that SetReadDeadline returns nil and does nothing, which would have left TtyRelay silently unfixable; the fd received from runc is also set non-blocking so os.NewFile can register it. The redial budget is bounded by elapsed time rather than an attempt count, and sized against what one failed attempt costs: VsockTransport.Dial retries ETIMEDOUT ten times internally, so a Connect to a port nothing is listening on takes roughly 20s to fail. Giving up is the expensive direction, because a relay that keeps retrying costs one goroutine while one that stops leaves the container wedged on a full stdout pipe with no way back. Each failed attempt is logged, since a host silently not listening on a stdio port was otherwise only visible in shim ETW. Verified on a two-node LCOW live-migration rig with a per-second workload and with a container that stays completely silent across the migration. Shim ETW from the silent runs shows the guest re-dialing its stdio ports about 56 seconds before the workload produced its first byte, so the bridge notification, not a copier write failure, is what recovered that relay. Signed-off-by: Shreyansh Sancheti <shsancheti@microsoft.com>
e6d811d to
c7f7caf
Compare
|
@rawahars — while verifying this on the two-node rig I hit a second, independent failure that I think belongs to the host side rather than this PR. Flagging it here so it doesn't get lost. Symptom: after a successful migration the destination container keeps running but its stdout never reaches the host — CRI log frozen, Cause: the host's stdio IO channel listener is one-shot. func (c *ioChannel) accept() {
c.c, c.err = c.l.Accept()
c.l.Close() // listener destroyed after the first accept
close(c.ch)
}That is fine for a normal container — one connection per process lifetime. But the whole guest-side recovery in this PR is built on the guest re-dialing its stdio ports after resume. The host can satisfy exactly one redial; a second one has nothing to connect to, permanently. Evidence (shim ETW,
The failing run:
Ruled out that the host is merely slow: I raised the guest's redial budget from 60s to 5 minutes, rebuilt the guest, and re-ran. Still no recovery at 105s+ with the relay still retrying. Extending guest-side patience cannot help when nothing will ever bind. Why I think it's worth a separate fix rather than folding into this PR: The guest-side fix here is still necessary: without it the relay never re-dials at all, because a copier parked in a blocking read on an idle pipe keeps Happy to pick up the host-side change if you'd like, or leave it with you — let me know which you prefer. |
3 participants
A live migration pauses the UVM and drops the GCS vsock bridge, which then
re-dials on the destination. Today the stdio relay tears the process down when
that happens, and io.Copy throws away whatever it had buffered, so in-flight
stdout/stderr is lost.
Now the relay pauses instead. On a copy failure mid-migration a manager
goroutine re-dials, swaps the conn, and resumes. A bridgeDown flag, set by
cmd/gcs around the reconnect loop, is what tells a migration pause apart from a
real process exit. The output copy drops io.Copy for a read/write-all loop that
keeps the unwritten tail and replays it on the new conn, so the relay does not
lose bytes.
I tried two other designs first: a per-conn wrapper that parks Read/Write until
reconnect (leaves the relays alone but wraps every read/write), and rewriting
both relays into pump loops with a registry (touches the shared relay path
every container and exec runs through). Went with pause-on-error because the
conn is only swapped after the copiers stop, so nothing mutates a live conn
under a running copy.
Caveat: it is reactive. Whatever the kernel or socket already dropped at the
blackout is gone; this only removes the relay-level drop. And if the host
closes the conns just before bridgeDown flips, that stream still tears down.
Tested under -race and on a two-node migration: the container keeps streaming
across the move.