Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 37 additions & 38 deletions internal/controller/vm/save_lcow.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,47 +221,46 @@ func (c *Controller) Resume(ctx context.Context, rebuildBridge bool) error {
return fmt.Errorf("cannot resume from migration: VM is in state %s: %w", c.vmState, errdefs.ErrFailedPrecondition)
}

// On the destination, the log connection was never established.
// On source, the blackout dropped the source's GCS log connection, which tore
// down its listener and closed logOutputDone. Install a fresh signal and
// re-arm the listener so the resumed guest's reconnect-mode vsockexec can
// reconnect and host-side logs resume.
c.logOutputDone = make(chan struct{})
// We expect the reconnect to complete within the GCS connection timeout,
// otherwise we want to fail.
ctx, cancel := context.WithTimeout(ctx, timeout.GCSConnectionTimeout)
log.G(ctx).Debugf("using gcs connection timeout: %s\n", timeout.GCSConnectionTimeout)

g, gctx := errgroup.WithContext(ctx)
defer func() {
_ = g.Wait()
}()
defer cancel()

if err := c.setupLoggingListener(gctx, g); err != nil {
return fmt.Errorf("arm logging listener on resume: %w", err)
}

if rebuildBridge {
// Source rollback: arm the host GCS listener now, then accept the guest's
// post-blackout re-dial and swap it into the running bridge.
if err := c.guest.PrepareConnection(winio.VsockServiceID(prot.LinuxGcsVsockPort)); err != nil {
return fmt.Errorf("prepare source resume listener: %w", err)
}
if err := c.guest.ResumeConnection(ctx); err != nil {
return fmt.Errorf("resume source guest connection: %w", err)
// A source rollback before blackout never dropped the guest connection, so the
// live bridge and its log stream are reused instead of re-accepted.
reuseLiveConn := rebuildBridge && c.guest.IsBridgeConnected()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a potential race: connected is read here as a snapshot, but it is not synchronized with the recvLoopRoutine transition. If recvLoop() returns immediately before connected.Store(false) executes, Resume() can observe connected == true, set reuseLiveConn, and skip rebuilding the bridge. It can then clear migrating, after which recvLoopRoutine stores connected = false, observes migrating == false, and calls kill(err). This can leave the resumed session using a bridge that is subsequently terminated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In short, this workflow would be similar to how a pod/container is handled if the GCS connection was dropped randomly at any point today. Hence it should be safe, without introducing any backward incompatibility.

Long version:
So you are right that IsBridgeConnected is a point in time snapshot of the bridge state. Right now we are using it for Resume API which is a high level API from Controller. We can run into this API from 2 points-

  • Destination Resume: rebuildBridge is false and hence we would go into the loop. There is no prior bridge connection and therefore, we do that inside if.
  • Source Rollback:
    • After blackout: VM was paused and hence listeners dropped and bridge would have collapsed during Transfer API causing connected to be false. Therefore, it would always return false here and lead us to go inside the if.
    • Before blackout:
      • Bridge is active at the time of Resume: connected is true and we skip the if.

      • As soon as the c.guest.IsBridgeConnected() is read inside Resume, the VM terminates or connection drops for some reason.
        This is the race scenario but it's safe and follows the existing workflow. If this happens then we would mark the bridge as not migrating and finish the finalize. Since the GCS connection is not present, the bridge collapses on migrating == false and all the processes/containers signal their EXIT. containerd gets the notification and trigger delete. The caller sees the container as EXITED and pod as NOTREADY. Caller can then shut the shim.

        This would be similar to the scenario where no migration is going on and the GCS connection dropped mid-way which causes the bridge to collapse and all processes to trigger their EXIT.


// Reuse skips the work below. Otherwise re-arm host logging and (re)establish the
// bridge, bounded by the GCS connection timeout; the shared tail keeps the caller ctx.
if !reuseLiveConn {
timeoutCtx, cancel := context.WithTimeout(ctx, timeout.GCSConnectionTimeout)
log.G(timeoutCtx).Debugf("using gcs connection timeout: %s\n", timeout.GCSConnectionTimeout)
g, gctx := errgroup.WithContext(timeoutCtx)
defer func() {
_ = g.Wait()
}()
defer cancel()

// Logs were dropped by the source blackout and never established back.
c.logOutputDone = make(chan struct{})
if err := c.setupLoggingListener(gctx, g); err != nil {
return fmt.Errorf("arm logging listener on resume: %w", err)
}
} else {
// Destination: reuse the connection already armed at start.
if err := c.guest.CreateConnection(ctx, false); err != nil {
return fmt.Errorf("resume destination guest connection: %w", err)

if rebuildBridge {
// Source rollback after blackout: accept the guest's re-dial into the bridge.
if err := c.guest.PrepareConnection(winio.VsockServiceID(prot.LinuxGcsVsockPort)); err != nil {
return fmt.Errorf("prepare source resume listener: %w", err)
}
if err := c.guest.ResumeConnection(timeoutCtx); err != nil {
return fmt.Errorf("resume source guest connection: %w", err)
}
} else {
// Destination: reuse the connection already armed at start.
if err := c.guest.CreateConnection(timeoutCtx, false); err != nil {
return fmt.Errorf("resume destination guest connection: %w", err)
}
}
}

// Collect any errors from establishing the log connection.
// If the connection is not established then we need to error out.
if err := g.Wait(); err != nil {
return err
// Fail if the log connection could not be established.
if err := g.Wait(); err != nil {
return err
}
}

// Clear migrating flag only now that the new transport is in place.
Expand Down
24 changes: 16 additions & 8 deletions internal/gcs/bridge.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,21 @@ type bridge struct {
rpcs map[int64]*rpc
// conn is the transport carrying messages to and from the guest.
// Held atomically because the send path reads it while a migration swaps it.
conn atomic.Value
rpcCh chan *rpc
notify notifyFunc
closed bool
log *logrus.Entry
brdgErr error
waitCh chan struct{}
conn atomic.Value
rpcCh chan *rpc
notify notifyFunc
closed bool
log *logrus.Entry
brdgErr error
waitCh chan struct{}

// Migration related fields
// migrating tolerates transport drops during a live-migration window.
migrating atomic.Bool
resumeCh chan struct{}
// resumeCh wakes the parked recv loop when a new transport is swapped in.
resumeCh chan struct{}
// connected is true while a live transport is present and being read.
connected atomic.Bool
}

var ErrBridgeClosed = fmt.Errorf("bridge closed: %w", net.ErrClosed)
Expand Down Expand Up @@ -305,7 +311,9 @@ func (brdg *bridge) RPC(ctx context.Context, proc prot.RPCProc, req requestMessa

func (brdg *bridge) recvLoopRoutine() {
for {
brdg.connected.Store(true)
err := brdg.recvLoop()
brdg.connected.Store(false)

if !brdg.migrating.Load() {
brdg.kill(err)
Expand Down
8 changes: 8 additions & 0 deletions internal/gcs/guestconnection.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,14 @@ func (gc *GuestConnection) SetMigrating(migrating bool) {
gc.brdg.SetMigrating(migrating)
}

// IsBridgeConnected reports whether a live bridge transport is currently installed.
func (gc *GuestConnection) IsBridgeConnected() bool {
if gc.brdg == nil {
return false
}
return gc.brdg.connected.Load()
}

// ResumeOnConn resumes the bridge after swaping the bridge
// transport without dropping outstanding RPCs.
func (gc *GuestConnection) ResumeOnConn(ctx context.Context, conn io.ReadWriteCloser) error {
Expand Down
11 changes: 11 additions & 0 deletions internal/vm/guestmanager/guest.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,17 @@ func (gm *Guest) SetMigrating(migrating bool) {
gm.gc.SetMigrating(migrating)
}

// IsBridgeConnected reports whether a live bridge transport is currently installed.
func (gm *Guest) IsBridgeConnected() bool {
gm.mu.RLock()
defer gm.mu.RUnlock()

if gm.gc == nil {
return false
}
return gm.gc.IsBridgeConnected()
}

// ResumeConnection accepts a fresh hvsock on the prepared listener and
// swaps it into the existing GCS bridge, preserving in-flight RPCs.
func (gm *Guest) ResumeConnection(ctx context.Context) error {
Expand Down
Loading