From 2896969c867fb84b7328e214d90500176bbeae82 Mon Sep 17 00:00:00 2001 From: tonic Date: Wed, 29 Jul 2026 15:59:27 +0800 Subject: [PATCH 1/2] cloudhypervisor: converge a VM left paused by a killed capture A `snapshot save` or `hibernate` killed inside its pause window leaves the guest frozen with nobody left to resume it: WithPausedVM's resume runs on the way out, and a SIGKILLed process never gets there. Every later device op then failed on that stale pause, because ensureNotPaused refused outright: Error: vm is paused (snapshot or hibernate in flight); retry after it completes For vk-cocoon that was terminal. Its resumed hibernate re-runs `vm net --nics 0` first, which was already a no-op (the NIC had been dropped before the save), and the refusal happened before the target==current early return -- so the wedge was reachable through the ordinary `systemctl restart vk-cocoon` path and needed a hand-written vm.resume on the API socket to clear. Paused observed by these callers has no owner, and that is provable rather than assumed: every pause window in cocoon runs under the VM ops lock end to end (prepareSnapshot, prepareRestore), clone resumes a record still in creating which runningVMClientWithRecord rejects, and all three ensureNotPaused callers already hold that same lock. So a caller holding the lock cannot be racing a live capture. convergeOrphanedPause resumes and returns a re-read vm.info, which callers need since they classify devices off it. resumeVM was already idempotent, and pauseVM already carried the mirror-image fix ("swallows CH's Paused->Paused 500 so a stuck-paused VM recovers") -- the capture path anticipated this, the device path did not. Verified on internal-cocoon-node-7: stop vk 3s into a save, then restart. The resumed hibernate now reports hibernate_total{netresize,ok} and runs through snapshot/push/remove to Suspended, where the old build logged netresize failures until the pod was abandoned. The guest marker written before the hibernate was present after the wake. --- hypervisor/cloudhypervisor/extend.go | 29 +++++--- hypervisor/cloudhypervisor/netresize.go | 2 +- hypervisor/cloudhypervisor/netresize_test.go | 69 +++++++++++++++++--- 3 files changed, 83 insertions(+), 17 deletions(-) diff --git a/hypervisor/cloudhypervisor/extend.go b/hypervisor/cloudhypervisor/extend.go index d0932bc2..4c0cd84a 100644 --- a/hypervisor/cloudhypervisor/extend.go +++ b/hypervisor/cloudhypervisor/extend.go @@ -11,6 +11,8 @@ import ( "slices" "strings" + "github.com/projecteru2/core/log" + "github.com/cocoonstack/cocoon/extend/disk" "github.com/cocoonstack/cocoon/extend/fs" "github.com/cocoonstack/cocoon/extend/vfio" @@ -238,7 +240,7 @@ func (ch *CloudHypervisor) attachWith( return "", err } defer unlock() - if err = ensureNotPaused(info); err != nil { + if info, err = convergeOrphanedPause(ctx, hc, info); err != nil { return "", err } if checkErr := preCheck(info); checkErr != nil { @@ -276,7 +278,7 @@ func (ch *CloudHypervisor) detachWith( return err } defer unlock() - if err = ensureNotPaused(info); err != nil { + if info, err = convergeOrphanedPause(ctx, hc, info); err != nil { return err } deviceID, err := findID(info) @@ -313,12 +315,23 @@ func (ch *CloudHypervisor) runningVMClientWithRecord(ctx context.Context, vmRef return utils.NewSocketHTTPClient(sockPath), vmID, rec, nil } -// ensureNotPaused refuses device-set mutations while a capture window is open — mutating mid-capture would desync config and memory. -func ensureNotPaused(info *chVMInfoResponse) error { - if info.State == chStatePaused { - return fmt.Errorf("vm is paused (snapshot or hibernate in flight); retry after it completes") - } - return nil +// convergeOrphanedPause resumes a VM left paused by a capture whose process died, +// and returns a refreshed vm.info. Every caller holds the VM ops lock, and every +// pause window in cocoon runs under that same lock end to end (prepareSnapshot, +// prepareRestore; clone resumes a record still in creating, which +// runningVMClientWithRecord rejects) — so Paused observed here has no owner and +// nothing can be mid-capture. Without this a killed `snapshot save` wedges the VM: +// the guest stays frozen and every later device op fails on the stale pause. +func convergeOrphanedPause(ctx context.Context, hc *http.Client, info *chVMInfoResponse) (*chVMInfoResponse, error) { + if info.State != chStatePaused { + return info, nil + } + log.WithFunc("cloudhypervisor.convergeOrphanedPause"). + Warnf(ctx, "vm is paused with no capture in flight (interrupted snapshot or hibernate), resuming") + if err := resumeVM(ctx, hc); err != nil { + return nil, fmt.Errorf("resume orphaned pause: %w", err) + } + return getVMInfo(ctx, hc) } // listWith returns nil (not error) for stopped VMs so inspect can omit the field. diff --git a/hypervisor/cloudhypervisor/netresize.go b/hypervisor/cloudhypervisor/netresize.go index 52ef1f3c..28de91cf 100644 --- a/hypervisor/cloudhypervisor/netresize.go +++ b/hypervisor/cloudhypervisor/netresize.go @@ -43,7 +43,7 @@ func (ch *CloudHypervisor) NetResize(ctx context.Context, vmRef string, spec net if err != nil { return netresize.Result{}, err } - if err = ensureNotPaused(info); err != nil { + if info, err = convergeOrphanedPause(ctx, hc, info); err != nil { return netresize.Result{}, err } current := len(rec.NetworkConfigs) diff --git a/hypervisor/cloudhypervisor/netresize_test.go b/hypervisor/cloudhypervisor/netresize_test.go index fa40b5cc..59f774c9 100644 --- a/hypervisor/cloudhypervisor/netresize_test.go +++ b/hypervisor/cloudhypervisor/netresize_test.go @@ -212,6 +212,66 @@ func newTestCH(t *testing.T) *CloudHypervisor { // newCHStubClient serves vm.info and vm.remove-device over an httptest server; // removed() snapshots the eject calls. stickyIDs stay in the device tree after // removal, simulating a guest that never acks B0EJ. +// TestConvergeOrphanedPause pins the interrupted-capture wedge: a snapshot save +// killed inside its pause window leaves CH Paused with nobody left to resume it, +// and every later device op then fails on that stale pause. Callers hold the ops +// lock, which proves no capture is in flight, so the pause is converged. +func TestConvergeOrphanedPause(t *testing.T) { + var ( + mu sync.Mutex + resumes int + state = chStatePaused + ) + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/vm.resume", func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + resumes++ + state = "Running" + mu.Unlock() + w.WriteHeader(http.StatusNoContent) + }) + mux.HandleFunc("/api/v1/vm.info", func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + defer mu.Unlock() + _ = json.NewEncoder(w).Encode(chVMInfoResponse{State: state}) + }) + hc := newStubHTTPClient(t, mux) + + info, err := getVMInfo(t.Context(), hc) + if err != nil { + t.Fatalf("vm.info: %v", err) + } + fresh, err := convergeOrphanedPause(t.Context(), hc, info) + if err != nil { + t.Fatalf("convergeOrphanedPause: %v", err) + } + if resumes != 1 { + t.Fatalf("resumes = %d, want the orphaned pause resumed once", resumes) + } + // The returned info must be re-read: callers classify devices off it. + if fresh.State != "Running" { + t.Errorf("returned state = %q, want the refreshed Running", fresh.State) + } + if _, err = convergeOrphanedPause(t.Context(), hc, fresh); err != nil { + t.Fatalf("convergeOrphanedPause on a running VM: %v", err) + } + if resumes != 1 { + t.Errorf("resumes = %d, want a running VM left untouched", resumes) + } +} + +func newStubHTTPClient(t *testing.T, mux *http.ServeMux) *http.Client { + t.Helper() + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + addr := strings.TrimPrefix(srv.URL, "http://") + return &http.Client{Transport: &http.Transport{ + DialContext: func(_ context.Context, _, _ string) (net.Conn, error) { + return net.Dial("tcp", addr) + }, + }} +} + func newCHStubClient(t *testing.T, nets []chNet, stickyIDs ...string) (*http.Client, func() []string) { t.Helper() var mu sync.Mutex @@ -247,14 +307,7 @@ func newCHStubClient(t *testing.T, nets []chNet, stickyIDs ...string) (*http.Cli } _ = json.NewEncoder(w).Encode(chVMInfoResponse{Config: chVMInfoConfig{Nets: live}, DeviceTree: tree}) }) - srv := httptest.NewServer(mux) - t.Cleanup(srv.Close) - addr := strings.TrimPrefix(srv.URL, "http://") - hc := &http.Client{Transport: &http.Transport{ - DialContext: func(_ context.Context, _, _ string) (net.Conn, error) { - return net.Dial("tcp", addr) - }, - }} + hc := newStubHTTPClient(t, mux) return hc, func() []string { mu.Lock() defer mu.Unlock() From d8817abafd09782ad40ef3e1fa1610ff7e8834b1 Mon Sep 17 00:00:00 2001 From: CMGS Date: Wed, 29 Jul 2026 17:10:31 +0800 Subject: [PATCH 2/2] review: test placement, vm id in the converge log, compress the ownerless proof MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestConvergeOrphanedPause sat below the fixture helpers (asl testorder violation) and wedged newCHStubClient's doc comment away from its func. The converge warn now names the VM it resumes — every call site holds the resolved record. The convergeOrphanedPause comment states the complete proof in two lines: the ops lock covers every capture window end to end, and the Running-record gate in runningVMClientWithRecord is what keeps the restore and clone pause windows (record Stopped/creating) unreachable. --- hypervisor/cloudhypervisor/extend.go | 19 ++-- hypervisor/cloudhypervisor/netresize.go | 2 +- hypervisor/cloudhypervisor/netresize_test.go | 100 +++++++++---------- 3 files changed, 57 insertions(+), 64 deletions(-) diff --git a/hypervisor/cloudhypervisor/extend.go b/hypervisor/cloudhypervisor/extend.go index 4c0cd84a..baada1f0 100644 --- a/hypervisor/cloudhypervisor/extend.go +++ b/hypervisor/cloudhypervisor/extend.go @@ -240,7 +240,7 @@ func (ch *CloudHypervisor) attachWith( return "", err } defer unlock() - if info, err = convergeOrphanedPause(ctx, hc, info); err != nil { + if info, err = convergeOrphanedPause(ctx, hc, rec.ID, info); err != nil { return "", err } if checkErr := preCheck(info); checkErr != nil { @@ -273,12 +273,12 @@ func (ch *CloudHypervisor) detachWith( ctx context.Context, vmRef string, findID func(*chVMInfoResponse) (string, error), ) error { - hc, _, info, unlock, err := ch.lockedDeviceOp(ctx, vmRef) + hc, rec, info, unlock, err := ch.lockedDeviceOp(ctx, vmRef) if err != nil { return err } defer unlock() - if info, err = convergeOrphanedPause(ctx, hc, info); err != nil { + if info, err = convergeOrphanedPause(ctx, hc, rec.ID, info); err != nil { return err } deviceID, err := findID(info) @@ -315,19 +315,14 @@ func (ch *CloudHypervisor) runningVMClientWithRecord(ctx context.Context, vmRef return utils.NewSocketHTTPClient(sockPath), vmID, rec, nil } -// convergeOrphanedPause resumes a VM left paused by a capture whose process died, -// and returns a refreshed vm.info. Every caller holds the VM ops lock, and every -// pause window in cocoon runs under that same lock end to end (prepareSnapshot, -// prepareRestore; clone resumes a record still in creating, which -// runningVMClientWithRecord rejects) — so Paused observed here has no owner and -// nothing can be mid-capture. Without this a killed `snapshot save` wedges the VM: -// the guest stays frozen and every later device op fails on the stale pause. -func convergeOrphanedPause(ctx context.Context, hc *http.Client, info *chVMInfoResponse) (*chVMInfoResponse, error) { +// convergeOrphanedPause resumes a VM left paused by a capture whose process died, returning a refreshed vm.info for device classification. +// The pause is provably ownerless: callers hold the VM ops lock every capture window holds end to end, and the Running-record gate in runningVMClientWithRecord keeps restore/clone pause windows (record Stopped/creating) out of reach. +func convergeOrphanedPause(ctx context.Context, hc *http.Client, vmID string, info *chVMInfoResponse) (*chVMInfoResponse, error) { if info.State != chStatePaused { return info, nil } log.WithFunc("cloudhypervisor.convergeOrphanedPause"). - Warnf(ctx, "vm is paused with no capture in flight (interrupted snapshot or hibernate), resuming") + Warnf(ctx, "vm %s is paused with no capture in flight (interrupted snapshot or hibernate), resuming", vmID) if err := resumeVM(ctx, hc); err != nil { return nil, fmt.Errorf("resume orphaned pause: %w", err) } diff --git a/hypervisor/cloudhypervisor/netresize.go b/hypervisor/cloudhypervisor/netresize.go index 28de91cf..f062a272 100644 --- a/hypervisor/cloudhypervisor/netresize.go +++ b/hypervisor/cloudhypervisor/netresize.go @@ -43,7 +43,7 @@ func (ch *CloudHypervisor) NetResize(ctx context.Context, vmRef string, spec net if err != nil { return netresize.Result{}, err } - if info, err = convergeOrphanedPause(ctx, hc, info); err != nil { + if info, err = convergeOrphanedPause(ctx, hc, vmID, info); err != nil { return netresize.Result{}, err } current := len(rec.NetworkConfigs) diff --git a/hypervisor/cloudhypervisor/netresize_test.go b/hypervisor/cloudhypervisor/netresize_test.go index 59f774c9..59536344 100644 --- a/hypervisor/cloudhypervisor/netresize_test.go +++ b/hypervisor/cloudhypervisor/netresize_test.go @@ -155,6 +155,52 @@ func TestNICPersisted(t *testing.T) { } } +// TestConvergeOrphanedPause pins the interrupted-capture wedge: a save killed +// inside its pause window leaves CH Paused with nobody left to resume it. +func TestConvergeOrphanedPause(t *testing.T) { + var ( + mu sync.Mutex + resumes int + state = chStatePaused + ) + mux := http.NewServeMux() + mux.HandleFunc("/api/v1/vm.resume", func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + resumes++ + state = "Running" + mu.Unlock() + w.WriteHeader(http.StatusNoContent) + }) + mux.HandleFunc("/api/v1/vm.info", func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + defer mu.Unlock() + _ = json.NewEncoder(w).Encode(chVMInfoResponse{State: state}) + }) + hc := newStubHTTPClient(t, mux) + + info, err := getVMInfo(t.Context(), hc) + if err != nil { + t.Fatalf("vm.info: %v", err) + } + fresh, err := convergeOrphanedPause(t.Context(), hc, "vm1", info) + if err != nil { + t.Fatalf("convergeOrphanedPause: %v", err) + } + if resumes != 1 { + t.Fatalf("resumes = %d, want the orphaned pause resumed once", resumes) + } + // The returned info must be re-read: callers classify devices off it. + if fresh.State != "Running" { + t.Errorf("returned state = %q, want the refreshed Running", fresh.State) + } + if _, err = convergeOrphanedPause(t.Context(), hc, "vm1", fresh); err != nil { + t.Fatalf("convergeOrphanedPause on a running VM: %v", err) + } + if resumes != 1 { + t.Errorf("resumes = %d, want a running VM left untouched", resumes) + } +} + type stubPlumbing struct { removed []int } @@ -209,57 +255,6 @@ func newTestCH(t *testing.T) *CloudHypervisor { return &CloudHypervisor{Backend: backend, conf: cfg} } -// newCHStubClient serves vm.info and vm.remove-device over an httptest server; -// removed() snapshots the eject calls. stickyIDs stay in the device tree after -// removal, simulating a guest that never acks B0EJ. -// TestConvergeOrphanedPause pins the interrupted-capture wedge: a snapshot save -// killed inside its pause window leaves CH Paused with nobody left to resume it, -// and every later device op then fails on that stale pause. Callers hold the ops -// lock, which proves no capture is in flight, so the pause is converged. -func TestConvergeOrphanedPause(t *testing.T) { - var ( - mu sync.Mutex - resumes int - state = chStatePaused - ) - mux := http.NewServeMux() - mux.HandleFunc("/api/v1/vm.resume", func(w http.ResponseWriter, _ *http.Request) { - mu.Lock() - resumes++ - state = "Running" - mu.Unlock() - w.WriteHeader(http.StatusNoContent) - }) - mux.HandleFunc("/api/v1/vm.info", func(w http.ResponseWriter, _ *http.Request) { - mu.Lock() - defer mu.Unlock() - _ = json.NewEncoder(w).Encode(chVMInfoResponse{State: state}) - }) - hc := newStubHTTPClient(t, mux) - - info, err := getVMInfo(t.Context(), hc) - if err != nil { - t.Fatalf("vm.info: %v", err) - } - fresh, err := convergeOrphanedPause(t.Context(), hc, info) - if err != nil { - t.Fatalf("convergeOrphanedPause: %v", err) - } - if resumes != 1 { - t.Fatalf("resumes = %d, want the orphaned pause resumed once", resumes) - } - // The returned info must be re-read: callers classify devices off it. - if fresh.State != "Running" { - t.Errorf("returned state = %q, want the refreshed Running", fresh.State) - } - if _, err = convergeOrphanedPause(t.Context(), hc, fresh); err != nil { - t.Fatalf("convergeOrphanedPause on a running VM: %v", err) - } - if resumes != 1 { - t.Errorf("resumes = %d, want a running VM left untouched", resumes) - } -} - func newStubHTTPClient(t *testing.T, mux *http.ServeMux) *http.Client { t.Helper() srv := httptest.NewServer(mux) @@ -272,6 +267,9 @@ func newStubHTTPClient(t *testing.T, mux *http.ServeMux) *http.Client { }} } +// newCHStubClient serves vm.info and vm.remove-device over an httptest server; +// removed() snapshots the eject calls. stickyIDs stay in the device tree after +// removal, simulating a guest that never acks B0EJ. func newCHStubClient(t *testing.T, nets []chNet, stickyIDs ...string) (*http.Client, func() []string) { t.Helper() var mu sync.Mutex