guest/stdio: keep stdio alive across LCOW live migration#2793
guest/stdio: keep stdio alive across LCOW live migration#2793shreyanshjain7174 wants to merge 4 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.
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>
67496fa to
93e5130
Compare
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.