From 7d254cfe0b59b4db60686c8e6524bc73cf57eabe Mon Sep 17 00:00:00 2001 From: tonic Date: Sat, 18 Jul 2026 10:54:48 +0800 Subject: [PATCH 1/8] feat(hibernate): release the DHCP lease pre-snapshot; renew-nudge stalled wakes in-window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hibernate snapshots freeze the guest with its DHCP lease still bound while the server-side lease (24h) keeps squatting under the old MAC. The restored clone re-DHCPs on a fresh NIC/MAC, so any wake within the lease lifetime REQUESTs the cached IP and the server must NAK it ("leased to another client" — the VM's own previous MAC). win11's DHCP client then wedges on APIPA ~3% of the time (event 1002, then no re-DISCOVER, no background retry — measured fleet-wide: 66 NAKs/64 recovered/2 wedged in one week) and the 45s lease wait times out into lifecycle=failed. Two changes, both CH+Windows-gated and best-effort, mirroring VMware Tools' default power scripts (release on suspend, renew on resume): - hibernate: exec `ipconfig /release` over the agent channel before the NetResize(0) NIC drop. Frees the server lease immediately (no 24h zombie pool slots) and freezes a guest with no cached lease, so restored clones DISCOVER instead of triggering the NAK path. rollbackHibernateNIC renews after re-adding the NIC since the lease was already released. - waitForFreshIP: one-shot `ipconfig /renew` nudge at 15s (tunable) still lease-less, inside the same 45s budget — the wake either completes in time or fails for real; the deadline stays the external contract. Unsticks the APIPA wedge (4/4 in incident response) and covers pre-existing snapshots that still carry cached leases. Never fires once a lease is observed, never fires for Linux guests. New metrics: hibernate_total gains phase=dhcp_release; new wake_renew_nudge_total{result}. docs/metrics.md updated. --- docs/metrics.md | 3 +- metrics/metrics.go | 11 ++ provider/cocoon/create_test.go | 5 + provider/cocoon/provider.go | 10 ++ provider/cocoon/update.go | 86 +++++++++++++++ provider/cocoon/update_test.go | 191 +++++++++++++++++++++++++++++++++ 6 files changed, 305 insertions(+), 1 deletion(-) diff --git a/docs/metrics.md b/docs/metrics.md index d417513..8940fca 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -40,9 +40,10 @@ Prometheus endpoint with vk-cocoon-specific metrics: | `cocoon_vk_pod_lifecycle_total{op,result,reason}` | Counter | Pod lifecycle operations (`result=ok\|failed\|skipped`, `reason` sub-classifies) | | `cocoon_vk_snapshot_pull_total{result}` / `save_total` / `push_total` | Counter | Snapshot pull/save/push counts | | `cocoon_vk_clone_from_dir_total{result}` | Counter | Annotation-driven `--from-dir` clone attempts | -| `cocoon_vk_hibernate_total{phase,result}` | Counter | Hibernate stage outcomes (`phase=netresize\|snapshot\|push\|remove`) | +| `cocoon_vk_hibernate_total{phase,result}` | Counter | Hibernate stage outcomes (`phase=dhcp_release\|netresize\|snapshot\|push\|remove`) | | `cocoon_vk_wake_total{result}` | Counter | Wake operation outcomes | | `cocoon_vk_wake_ip_wait_total{result}` | Counter | Post-clone and wake DHCP-lease-wait outcomes — both the CH+Windows dropNIC wake and every clone's post-clone IP wait (`result=ok\|timeout`) | +| `cocoon_vk_wake_renew_nudge_total{result}` | Counter | `ipconfig /renew` nudges sent to Windows guests still lease-less mid lease-wait (`result=ok\|failed`; failed means the exec didn't confirm — the in-guest renew may still have taken effect, the lease re-check decides) | | `cocoon_vk_postclone_total{kind,result}` | Counter | Post-clone fixup outcomes (`kind=linux_static\|linux_fc\|windows\|sac`) | | `cocoon_vk_postclone_retry_attempts{result}` | Histogram | Attempts consumed before post-clone exec succeeded or failed (`result=ok\|failed`) | | `cocoon_vk_vm_table_size` | Gauge | Tracked VM count | diff --git a/metrics/metrics.go b/metrics/metrics.go index 5f4116a..50ce6ad 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -190,6 +190,16 @@ var ( []string{"result"}, // result=ok|timeout ) + WakeRenewNudgeTotal = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Namespace: metricNamespace, + Subsystem: metricSubsystem, + Name: "wake_renew_nudge_total", + Help: "ipconfig /renew nudges sent to Windows guests still lease-less mid lease-wait.", + }, + []string{"result"}, // result=ok|failed + ) + PostCloneTotal = prometheus.NewCounterVec( prometheus.CounterOpts{ Namespace: metricNamespace, @@ -233,6 +243,7 @@ func Register(reg prometheus.Registerer) { HibernateTotal, WakeTotal, WakeIPWaitTotal, + WakeRenewNudgeTotal, PostCloneTotal, PostCloneRetryAttempts, ) diff --git a/provider/cocoon/create_test.go b/provider/cocoon/create_test.go index a9c0331..f324dd3 100644 --- a/provider/cocoon/create_test.go +++ b/provider/cocoon/create_test.go @@ -1678,6 +1678,8 @@ type fakeRuntime struct { // onRemove, when set, fires at Remove entry — for ordering / failure tests. onRemove func() + // onExec, when set, fires at Exec entry — lets tests block or mutate state mid-exec. + onExec func() // removeErr, when set, makes Remove fail with this error. removeErr error // snapshotSaveErr, when set, makes SnapshotSave fail with this error. @@ -1891,6 +1893,9 @@ type fakeExecCall struct { } func (f *fakeRuntime) Exec(_ context.Context, vmID string, argv []string, env map[string]string, stdin io.Reader, stdout, stderr io.Writer) error { + if f.onExec != nil { + f.onExec() + } call := fakeExecCall{vmID: vmID, argv: argv, env: env} if stdin != nil { buf, _ := io.ReadAll(stdin) diff --git a/provider/cocoon/provider.go b/provider/cocoon/provider.go index 2df3f8e..50ae6e5 100644 --- a/provider/cocoon/provider.go +++ b/provider/cocoon/provider.go @@ -20,6 +20,7 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/record" + cocoonv1 "github.com/cocoonstack/cocoon-common/apis/v1" commonk8s "github.com/cocoonstack/cocoon-common/k8s" "github.com/cocoonstack/cocoon-common/meta" "github.com/cocoonstack/cocoon-common/oci" @@ -114,6 +115,7 @@ type Provider struct { // dropNIC wake tunables; defaults live in update.go. wakeFreshIPBudget time.Duration wakeFreshIPInterval time.Duration + wakeRenewNudgeDelay time.Duration } // NewProvider constructs a Provider with empty tables. @@ -348,6 +350,14 @@ func (p *Provider) buildProbe(namespace, name string) probes.Probe { } } +// isWindowsGuest reports whether the tracked pod declares a Windows guest. +func (p *Provider) isWindowsGuest(namespace, name string) bool { + p.mu.RLock() + pod := p.pods[meta.PodKey(namespace, name)] + p.mu.RUnlock() + return pod != nil && meta.ParseVMSpec(pod).OS == string(cocoonv1.OSWindows) +} + func (p *Provider) probePort(ctx context.Context, namespace, name string) string { pod, _ := p.GetPod(ctx, namespace, name) if pod == nil { diff --git a/provider/cocoon/update.go b/provider/cocoon/update.go index e731ab7..e4aa682 100644 --- a/provider/cocoon/update.go +++ b/provider/cocoon/update.go @@ -1,10 +1,12 @@ package cocoon import ( + "bytes" "cmp" "context" "errors" "fmt" + "strings" "time" "github.com/projecteru2/core/log" @@ -26,6 +28,18 @@ const ( // the budget is generous. defaultWakeFreshIPBudget = 45 * time.Second defaultWakeFreshIPInterval = 200 * time.Millisecond + + // defaultWakeRenewNudgeDelay splits the 45s lease budget in two: natural + // DHCP gets the first 30s (slow UFFD restores legitimately take a while), + // then a `ipconfig /renew` nudge gets the rest. win11's DHCP client can + // wedge on APIPA after a NAK (event 1002, then no re-DISCOVER ever); a + // renew restarts DORA reliably and lands in ~1s, so 15s of post-nudge + // budget is ample. The nudge never fires once a lease is observed. + defaultWakeRenewNudgeDelay = 30 * time.Second + + // guestIpconfigTimeout bounds the vsock exec for release/renew so a sick + // guest (dead agent, stopped DHCP service) cannot stall hibernate or wake. + guestIpconfigTimeout = 20 * time.Second ) // UpdatePod handles hibernate/wake transitions; other spec changes are no-ops @@ -72,6 +86,17 @@ func (p *Provider) hibernate(ctx context.Context, pod *corev1.Pod, v *vm.VM) err p.markLifecycleState(ctx, pod, meta.LifecycleStateHibernating, "") dropNIC := shouldDropNICBeforeHibernate(spec) if dropNIC { + // Release the DHCP lease before yanking the NIC (VMware Tools' default + // suspend behavior): the server-side lease frees immediately instead of + // squatting its pool slot for 24h, and the snapshot freezes a guest with + // no cached lease, so the restored clone DISCOVERs instead of REQUESTing + // an IP the server must NAK. Best-effort: a sick guest still hibernates. + if err := p.execGuestIpconfig(ctx, v.ID, "release"); err != nil { + metrics.HibernateTotal.WithLabelValues("dhcp_release", "failed").Inc() + logger.Warnf(ctx, "dhcp release before hibernate %s: %v (proceeding)", v.Name, err) + } else { + metrics.HibernateTotal.WithLabelValues("dhcp_release", "ok").Inc() + } if err := p.Runtime.NetResize(ctx, v.ID, 0); err != nil { metrics.HibernateTotal.WithLabelValues("netresize", "failed").Inc() err = fmt.Errorf("drop NIC pre-hibernate %s: %w", v.Name, err) @@ -149,6 +174,11 @@ func (p *Provider) rollbackHibernateNIC(ctx context.Context, v *vm.VM, dropped b } if err := p.Runtime.NetResize(ctx, v.ID, 1); err != nil { log.WithFunc("Provider.rollbackHibernateNIC").Errorf(ctx, err, "re-add NIC after hibernate failure %s", v.Name) + return + } + // The pre-hibernate release left the guest unbound; nudge it to re-acquire. + if err := p.execGuestIpconfig(ctx, v.ID, "renew"); err != nil { + log.WithFunc("Provider.rollbackHibernateNIC").Warnf(ctx, "dhcp renew after hibernate rollback %s: %v", v.Name, err) } } @@ -285,6 +315,8 @@ func (p *Provider) waitForFreshIP(ctx context.Context, namespace, name string) b budget := cmp.Or(p.wakeFreshIPBudget, defaultWakeFreshIPBudget) interval := cmp.Or(p.wakeFreshIPInterval, defaultWakeFreshIPInterval) deadline := time.Now().Add(budget) + renewAt := time.Now().Add(cmp.Or(p.wakeRenewNudgeDelay, defaultWakeRenewNudgeDelay)) + renewed := false for { v := p.vmForPod(namespace, name) if v == nil { @@ -293,6 +325,30 @@ func (p *Provider) waitForFreshIP(ctx context.Context, namespace, name string) b if ip := p.resolveVMIP(namespace, name, v); ip != "" { return true } + // One-shot renew nudge: only while still lease-less, only for Windows + // guests (win11 wedges on APIPA after a NAK and never re-DISCOVERs). + // A guest that already holds a lease returns above and is never nudged. + if !renewed && !time.Now().Before(renewAt) { + renewed = true // non-Windows passes through once and never re-checks + if p.isWindowsGuest(namespace, name) { + // Cap the exec by the overall deadline too: a hung agent must not + // push the failure verdict past the 45s contract. + nudgeCtx, cancel := context.WithDeadline(ctx, deadline) + err := p.execGuestIpconfig(nudgeCtx, v.ID, "renew") + cancel() + if err != nil { + // failed = the exec didn't confirm; the in-guest renew may + // still have landed — the lease re-check decides, not this. + metrics.WakeRenewNudgeTotal.WithLabelValues("failed").Inc() + log.WithFunc("Provider.waitForFreshIP").Warnf(ctx, "renew nudge %s/%s: %v", namespace, name, err) + } else { + metrics.WakeRenewNudgeTotal.WithLabelValues("ok").Inc() + } + // The exec may have blocked long enough for the lease to land; + // re-check it before the deadline verdict. + continue + } + } if !time.Now().Before(deadline) { return false } @@ -302,6 +358,36 @@ func (p *Provider) waitForFreshIP(ctx context.Context, namespace, name string) b } } +// execGuestIpconfig runs `ipconfig /` inside a Windows guest over the +// cocoon-agent vsock channel — the same release/renew pair VMware Tools' +// default power scripts have shipped for decades. Network-independent: the +// exec path never touches the guest's IP. +func (p *Provider) execGuestIpconfig(ctx context.Context, vmID, verb string) error { + ctx, cancel := context.WithTimeout(ctx, guestIpconfigTimeout) + defer cancel() + var out bytes.Buffer + if err := p.Runtime.Exec(ctx, vmID, []string{"cmd", "/c", "ipconfig /" + verb}, nil, nil, &out, &out); err != nil { + // Surface the guest-side reason ("The RPC server is unavailable", ...). + if line := lastNonEmptyLine(out.String()); line != "" { + return fmt.Errorf("%w: %s", err, line) + } + return err + } + return nil +} + +// lastNonEmptyLine returns the last non-blank line of s, trimmed. ipconfig +// prints a banner first and the actual error last, so the tail is the signal. +func lastNonEmptyLine(s string) string { + last := "" + for line := range strings.SplitSeq(s, "\n") { + if trimmed := strings.TrimSpace(line); trimmed != "" { + last = trimmed + } + } + return last +} + // resolveWakeSource returns the clone source name and its snapshot metadata — // the local snapshot when present, else pulled from the registry. func (p *Provider) resolveWakeSource(ctx context.Context, vmName string) (string, *vm.Snapshot, error) { diff --git a/provider/cocoon/update_test.go b/provider/cocoon/update_test.go index 083203a..3e54e95 100644 --- a/provider/cocoon/update_test.go +++ b/provider/cocoon/update_test.go @@ -4,10 +4,12 @@ import ( "errors" "maps" "reflect" + "strings" "testing" "time" corev1 "k8s.io/api/core/v1" + "k8s.io/client-go/kubernetes/fake" cocoonv1 "github.com/cocoonstack/cocoon-common/apis/v1" "github.com/cocoonstack/cocoon-common/meta" @@ -412,3 +414,192 @@ func newDropNICWakeFixture(t *testing.T, budget, interval time.Duration) (*Provi p.markLifecycleState(t.Context(), pod, meta.LifecycleStateCreating, "") return p, pod, v } + +func execArgvs(rt *fakeRuntime) []string { + var out []string + for _, c := range rt.execCalls { + out = append(out, strings.Join(c.argv, " ")) + } + return out +} + +func TestHibernateReleasesLeaseBeforeNICDrop(t *testing.T) { + rt := &fakeRuntime{} + p := newTestProvider(t) + p.Runtime = rt + p.Probes = probes.NewManager(t.Context()) + + pod := newPodWithSpec(meta.VMSpec{ + VMName: "vk-ns-demo-0", + Backend: string(cocoonv1.BackendCloudHypervisor), + OS: string(cocoonv1.OSWindows), + }) + v := &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0"} + + if err := p.hibernate(t.Context(), pod, v); err != nil { + t.Fatalf("hibernate: %v", err) + } + if got := execArgvs(rt); len(got) != 1 || got[0] != "cmd /c ipconfig /release" { + t.Errorf("exec calls = %v, want exactly [cmd /c ipconfig /release]", got) + } + if len(rt.netResizeCalls) != 1 || rt.netResizeCalls[0].target != 0 { + t.Errorf("NetResize calls = %#v, want one drop-to-0", rt.netResizeCalls) + } +} + +func TestHibernateReleaseFailureDoesNotBlock(t *testing.T) { + rt := &fakeRuntime{execErr: errors.New("agent down")} + p := newTestProvider(t) + p.Runtime = rt + p.Probes = probes.NewManager(t.Context()) + + pod := newPodWithSpec(meta.VMSpec{ + VMName: "vk-ns-demo-0", + Backend: string(cocoonv1.BackendCloudHypervisor), + OS: string(cocoonv1.OSWindows), + }) + if err := p.hibernate(t.Context(), pod, &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0"}); err != nil { + t.Fatalf("hibernate must proceed past a failed release: %v", err) + } + if rt.removedID != "vmid-1" { + t.Errorf("hibernate did not complete: removedID=%q", rt.removedID) + } +} + +func TestHibernateSkipsReleaseOnNonDropNIC(t *testing.T) { + rt := &fakeRuntime{} + p := newTestProvider(t) + p.Runtime = rt + p.Probes = probes.NewManager(t.Context()) + + pod := newPodWithSpec(meta.VMSpec{ + VMName: "vk-ns-demo-0", + Backend: string(cocoonv1.BackendCloudHypervisor), + OS: string(cocoonv1.OSLinux), + }) + if err := p.hibernate(t.Context(), pod, &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0"}); err != nil { + t.Fatalf("hibernate: %v", err) + } + if len(rt.execCalls) != 0 { + t.Errorf("linux hibernate must not exec ipconfig, got %v", execArgvs(rt)) + } +} + +func TestHibernateRollbackRenews(t *testing.T) { + rt := &fakeRuntime{snapshotSaveErr: errors.New("save boom")} + p := newTestProvider(t) + p.Runtime = rt + p.Probes = probes.NewManager(t.Context()) + p.Clientset = fake.NewSimpleClientset() + + pod := newPodWithSpec(meta.VMSpec{ + VMName: "vk-ns-demo-0", + Backend: string(cocoonv1.BackendCloudHypervisor), + OS: string(cocoonv1.OSWindows), + }) + if err := p.hibernate(t.Context(), pod, &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0"}); err == nil { + t.Fatal("hibernate should fail on snapshot save error") + } + got := execArgvs(rt) + if len(got) != 2 || got[0] != "cmd /c ipconfig /release" || got[1] != "cmd /c ipconfig /renew" { + t.Errorf("exec calls = %v, want [release, renew-on-rollback]", got) + } + if len(rt.netResizeCalls) != 2 || rt.netResizeCalls[1].target != 1 { + t.Errorf("NetResize calls = %#v, want drop then rollback re-add", rt.netResizeCalls) + } +} + +func trackWindowsPodNoIP(t *testing.T, p *Provider, withIP string) *vm.VM { + t.Helper() + pod := newPodWithSpec(meta.VMSpec{ + VMName: "vk-ns-demo-0", + Backend: string(cocoonv1.BackendCloudHypervisor), + OS: string(cocoonv1.OSWindows), + }) + v := &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0", IP: withIP} + p.trackPod(pod, v) + return v +} + +func TestWaitForFreshIPRenewNudgeWhenLeaseMissing(t *testing.T) { + rt := &fakeRuntime{} + p := newTestProvider(t) + p.Runtime = rt + p.Probes = probes.NewManager(t.Context()) + p.wakeFreshIPBudget = 200 * time.Millisecond + p.wakeFreshIPInterval = 10 * time.Millisecond + p.wakeRenewNudgeDelay = 30 * time.Millisecond + trackWindowsPodNoIP(t, p, "") + + if p.waitForFreshIP(t.Context(), "ns", "demo-0") { + t.Fatal("no IP should time out") + } + got := execArgvs(rt) + if len(got) != 1 || got[0] != "cmd /c ipconfig /renew" { + t.Errorf("exec calls = %v, want exactly one renew nudge", got) + } +} + +func TestWaitForFreshIPNoRenewWhenIPPresent(t *testing.T) { + rt := &fakeRuntime{} + p := newTestProvider(t) + p.Runtime = rt + p.Probes = probes.NewManager(t.Context()) + p.wakeFreshIPBudget = 200 * time.Millisecond + p.wakeFreshIPInterval = 10 * time.Millisecond + p.wakeRenewNudgeDelay = time.Nanosecond // even an instant nudge window must not fire + trackWindowsPodNoIP(t, p, "10.0.0.9") + + if !p.waitForFreshIP(t.Context(), "ns", "demo-0") { + t.Fatal("IP present should return true") + } + if len(rt.execCalls) != 0 { + t.Errorf("guest already holds a lease; renew must not fire, got %v", execArgvs(rt)) + } +} + +func TestWaitForFreshIPNoRenewForLinux(t *testing.T) { + rt := &fakeRuntime{} + p := newTestProvider(t) + p.Runtime = rt + p.Probes = probes.NewManager(t.Context()) + p.wakeFreshIPBudget = 120 * time.Millisecond + p.wakeFreshIPInterval = 10 * time.Millisecond + p.wakeRenewNudgeDelay = 20 * time.Millisecond + + pod := newPodWithSpec(meta.VMSpec{ + VMName: "vk-ns-demo-0", + Backend: string(cocoonv1.BackendCloudHypervisor), + OS: string(cocoonv1.OSLinux), + }) + p.trackPod(pod, &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0"}) + + if p.waitForFreshIP(t.Context(), "ns", "demo-0") { + t.Fatal("no IP should time out") + } + if len(rt.execCalls) != 0 { + t.Errorf("linux guest must not get ipconfig, got %v", execArgvs(rt)) + } +} + +func TestWaitForFreshIPLeaseLandingDuringNudgeWins(t *testing.T) { + // Review finding: if the renew exec blocks past the deadline while the + // lease lands mid-exec, the verdict must be success, not timeout. + rt := &fakeRuntime{} + p := newTestProvider(t) + p.Runtime = rt + p.Probes = probes.NewManager(t.Context()) + p.wakeFreshIPBudget = 100 * time.Millisecond + p.wakeFreshIPInterval = 10 * time.Millisecond + p.wakeRenewNudgeDelay = 20 * time.Millisecond + trackWindowsPodNoIP(t, p, "") + + rt.onExec = func() { + time.Sleep(150 * time.Millisecond) // block past the 100ms deadline + p.setVMIP("ns", "demo-0", "10.0.0.9") + } + + if !p.waitForFreshIP(t.Context(), "ns", "demo-0") { + t.Fatal("lease landed during the nudge exec; verdict must be success") + } +} From e4dc5d09f16f79668fa7523d5825716ee1618b17 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 19 Jul 2026 12:23:25 +0800 Subject: [PATCH 2/8] fix(hibernate): re-acquire the released lease when the NIC drop fails The release-then-drop sequence compensated every later failure through rollbackHibernateNIC, except the drop itself failing: that branch returned with the guest lease-less and its NIC up, and Windows never re-DISCOVERs after an explicit release. Renew before surfacing the error; extracting dropNICForHibernate keeps the branch flat. --- provider/cocoon/update.go | 108 ++++++++++++++++----------------- provider/cocoon/update_test.go | 61 +++++++++---------- 2 files changed, 83 insertions(+), 86 deletions(-) diff --git a/provider/cocoon/update.go b/provider/cocoon/update.go index e4aa682..c1064bd 100644 --- a/provider/cocoon/update.go +++ b/provider/cocoon/update.go @@ -29,12 +29,8 @@ const ( defaultWakeFreshIPBudget = 45 * time.Second defaultWakeFreshIPInterval = 200 * time.Millisecond - // defaultWakeRenewNudgeDelay splits the 45s lease budget in two: natural - // DHCP gets the first 30s (slow UFFD restores legitimately take a while), - // then a `ipconfig /renew` nudge gets the rest. win11's DHCP client can - // wedge on APIPA after a NAK (event 1002, then no re-DISCOVER ever); a - // renew restarts DORA reliably and lands in ~1s, so 15s of post-nudge - // budget is ample. The nudge never fires once a lease is observed. + // defaultWakeRenewNudgeDelay leaves natural DHCP the first 30s of the lease + // budget; the one-shot renew after it restarts DORA and lands in ~1s. defaultWakeRenewNudgeDelay = 30 * time.Second // guestIpconfigTimeout bounds the vsock exec for release/renew so a sick @@ -86,24 +82,10 @@ func (p *Provider) hibernate(ctx context.Context, pod *corev1.Pod, v *vm.VM) err p.markLifecycleState(ctx, pod, meta.LifecycleStateHibernating, "") dropNIC := shouldDropNICBeforeHibernate(spec) if dropNIC { - // Release the DHCP lease before yanking the NIC (VMware Tools' default - // suspend behavior): the server-side lease frees immediately instead of - // squatting its pool slot for 24h, and the snapshot freezes a guest with - // no cached lease, so the restored clone DISCOVERs instead of REQUESTing - // an IP the server must NAK. Best-effort: a sick guest still hibernates. - if err := p.execGuestIpconfig(ctx, v.ID, "release"); err != nil { - metrics.HibernateTotal.WithLabelValues("dhcp_release", "failed").Inc() - logger.Warnf(ctx, "dhcp release before hibernate %s: %v (proceeding)", v.Name, err) - } else { - metrics.HibernateTotal.WithLabelValues("dhcp_release", "ok").Inc() - } - if err := p.Runtime.NetResize(ctx, v.ID, 0); err != nil { - metrics.HibernateTotal.WithLabelValues("netresize", "failed").Inc() - err = fmt.Errorf("drop NIC pre-hibernate %s: %w", v.Name, err) + if err := p.dropNICForHibernate(ctx, v); err != nil { p.failOp(ctx, pod, "HibernateNetResizeFailed", "update", err) return err } - metrics.HibernateTotal.WithLabelValues("netresize", "ok").Inc() } saveStart := time.Now() if err := p.Runtime.SnapshotSave(ctx, v.Name, v.ID); err != nil { @@ -167,18 +149,46 @@ func (p *Provider) hibernate(ctx context.Context, pod *corev1.Pod, v *vm.VM) err return nil } +// dropNICForHibernate releases the lease then detaches the NIC (VMware Tools' +// suspend default): the snapshot carries no cached lease, so restored clones +// DISCOVER instead of drawing a NAK. Best-effort: a sick guest still hibernates, +// and a failed detach re-acquires the lease it just released. +func (p *Provider) dropNICForHibernate(ctx context.Context, v *vm.VM) error { + logger := log.WithFunc("Provider.dropNICForHibernate") + released := false + if err := p.execGuestIpconfig(ctx, v.ID, "release"); err != nil { + metrics.HibernateTotal.WithLabelValues("dhcp_release", "failed").Inc() + logger.Warnf(ctx, "dhcp release before hibernate %s: %v (proceeding)", v.Name, err) + } else { + released = true + metrics.HibernateTotal.WithLabelValues("dhcp_release", "ok").Inc() + } + if err := p.Runtime.NetResize(ctx, v.ID, 0); err != nil { + metrics.HibernateTotal.WithLabelValues("netresize", "failed").Inc() + if released { + if renewErr := p.execGuestIpconfig(ctx, v.ID, "renew"); renewErr != nil { + logger.Warnf(ctx, "dhcp renew after failed NIC drop %s: %v", v.Name, renewErr) + } + } + return fmt.Errorf("drop NIC pre-hibernate %s: %w", v.Name, err) + } + metrics.HibernateTotal.WithLabelValues("netresize", "ok").Inc() + return nil +} + // rollbackHibernateNIC re-adds the NIC dropped pre-snapshot. func (p *Provider) rollbackHibernateNIC(ctx context.Context, v *vm.VM, dropped bool) { if !dropped { return } + logger := log.WithFunc("Provider.rollbackHibernateNIC") if err := p.Runtime.NetResize(ctx, v.ID, 1); err != nil { - log.WithFunc("Provider.rollbackHibernateNIC").Errorf(ctx, err, "re-add NIC after hibernate failure %s", v.Name) + logger.Errorf(ctx, err, "re-add NIC after hibernate failure %s", v.Name) return } // The pre-hibernate release left the guest unbound; nudge it to re-acquire. if err := p.execGuestIpconfig(ctx, v.ID, "renew"); err != nil { - log.WithFunc("Provider.rollbackHibernateNIC").Warnf(ctx, "dhcp renew after hibernate rollback %s: %v", v.Name, err) + logger.Warnf(ctx, "dhcp renew after hibernate rollback %s: %v", v.Name, err) } } @@ -259,7 +269,7 @@ func (p *Provider) dispatchHibernateRestore(pod *corev1.Pod, spec meta.VMSpec, v // finalizeDropNICWake holds Ready until the fresh NIC's lease lands. func (p *Provider) finalizeDropNICWake(ctx context.Context, pod *corev1.Pod, v *vm.VM) { - gotIP := p.waitForFreshIP(ctx, pod.Namespace, pod.Name) + gotIP := p.waitForFreshIP(ctx, pod) if ctx.Err() != nil { return } @@ -311,43 +321,35 @@ func (p *Provider) markLifecycleStateForWake(ctx context.Context, pod *corev1.Po // resumes contend, so the first lease can land many seconds after resume; // a short budget would misread that as lifecycle=failed and trigger an // operator rebuild. -func (p *Provider) waitForFreshIP(ctx context.Context, namespace, name string) bool { +func (p *Provider) waitForFreshIP(ctx context.Context, pod *corev1.Pod) bool { budget := cmp.Or(p.wakeFreshIPBudget, defaultWakeFreshIPBudget) interval := cmp.Or(p.wakeFreshIPInterval, defaultWakeFreshIPInterval) deadline := time.Now().Add(budget) renewAt := time.Now().Add(cmp.Or(p.wakeRenewNudgeDelay, defaultWakeRenewNudgeDelay)) - renewed := false + nudged := meta.ParseVMSpec(pod).OS != string(cocoonv1.OSWindows) for { - v := p.vmForPod(namespace, name) + v := p.vmForPod(pod.Namespace, pod.Name) if v == nil { return false } - if ip := p.resolveVMIP(namespace, name, v); ip != "" { + if ip := p.resolveVMIP(pod.Namespace, pod.Name, v); ip != "" { return true } - // One-shot renew nudge: only while still lease-less, only for Windows - // guests (win11 wedges on APIPA after a NAK and never re-DISCOVERs). - // A guest that already holds a lease returns above and is never nudged. - if !renewed && !time.Now().Before(renewAt) { - renewed = true // non-Windows passes through once and never re-checks - if p.isWindowsGuest(namespace, name) { - // Cap the exec by the overall deadline too: a hung agent must not - // push the failure verdict past the 45s contract. - nudgeCtx, cancel := context.WithDeadline(ctx, deadline) - err := p.execGuestIpconfig(nudgeCtx, v.ID, "renew") - cancel() - if err != nil { - // failed = the exec didn't confirm; the in-guest renew may - // still have landed — the lease re-check decides, not this. - metrics.WakeRenewNudgeTotal.WithLabelValues("failed").Inc() - log.WithFunc("Provider.waitForFreshIP").Warnf(ctx, "renew nudge %s/%s: %v", namespace, name, err) - } else { - metrics.WakeRenewNudgeTotal.WithLabelValues("ok").Inc() - } - // The exec may have blocked long enough for the lease to land; - // re-check it before the deadline verdict. - continue + // One-shot renew: win11 can wedge on APIPA after a NAK and never re-DISCOVER. + if !nudged && !time.Now().Before(renewAt) { + nudged = true + // Capped by the wake deadline so a hung agent cannot push the verdict past it. + nudgeCtx, cancel := context.WithDeadline(ctx, deadline) + err := p.execGuestIpconfig(nudgeCtx, v.ID, "renew") + cancel() + if err != nil { + metrics.WakeRenewNudgeTotal.WithLabelValues("failed").Inc() + log.WithFunc("Provider.waitForFreshIP").Warnf(ctx, "renew nudge %s/%s: %v", pod.Namespace, pod.Name, err) + } else { + metrics.WakeRenewNudgeTotal.WithLabelValues("ok").Inc() } + // The lease may have landed during the exec; re-check before the deadline verdict. + continue } if !time.Now().Before(deadline) { return false @@ -358,10 +360,8 @@ func (p *Provider) waitForFreshIP(ctx context.Context, namespace, name string) b } } -// execGuestIpconfig runs `ipconfig /` inside a Windows guest over the -// cocoon-agent vsock channel — the same release/renew pair VMware Tools' -// default power scripts have shipped for decades. Network-independent: the -// exec path never touches the guest's IP. +// execGuestIpconfig runs `ipconfig /` in the guest over vsock, so the +// exec path never depends on the IP it is repairing. func (p *Provider) execGuestIpconfig(ctx context.Context, vmID, verb string) error { ctx, cancel := context.WithTimeout(ctx, guestIpconfigTimeout) defer cancel() diff --git a/provider/cocoon/update_test.go b/provider/cocoon/update_test.go index 3e54e95..9ca4c73 100644 --- a/provider/cocoon/update_test.go +++ b/provider/cocoon/update_test.go @@ -509,29 +509,36 @@ func TestHibernateRollbackRenews(t *testing.T) { } } -func trackWindowsPodNoIP(t *testing.T, p *Provider, withIP string) *vm.VM { - t.Helper() +func TestHibernateRenewsWhenNICDropFails(t *testing.T) { + rt := &fakeRuntime{netResizeErr: errors.New("resize boom")} + p := newTestProvider(t) + p.Runtime = rt + p.Probes = probes.NewManager(t.Context()) + p.Clientset = fake.NewSimpleClientset() + pod := newPodWithSpec(meta.VMSpec{ VMName: "vk-ns-demo-0", Backend: string(cocoonv1.BackendCloudHypervisor), OS: string(cocoonv1.OSWindows), }) - v := &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0", IP: withIP} - p.trackPod(pod, v) - return v + if err := p.hibernate(t.Context(), pod, &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0"}); err == nil { + t.Fatal("hibernate should fail when the NIC drop fails") + } + got := execArgvs(rt) + if len(got) != 2 || got[0] != "cmd /c ipconfig /release" || got[1] != "cmd /c ipconfig /renew" { + t.Errorf("exec calls = %v, want [release, renew] around the failed drop", got) + } + if len(rt.netResizeCalls) != 1 { + t.Errorf("NetResize calls = %#v, want only the failed drop (NIC never dropped, no re-add)", rt.netResizeCalls) + } } func TestWaitForFreshIPRenewNudgeWhenLeaseMissing(t *testing.T) { - rt := &fakeRuntime{} - p := newTestProvider(t) - p.Runtime = rt - p.Probes = probes.NewManager(t.Context()) - p.wakeFreshIPBudget = 200 * time.Millisecond - p.wakeFreshIPInterval = 10 * time.Millisecond + p, pod, _ := newDropNICWakeFixture(t, 200*time.Millisecond, 10*time.Millisecond) + rt := p.Runtime.(*fakeRuntime) p.wakeRenewNudgeDelay = 30 * time.Millisecond - trackWindowsPodNoIP(t, p, "") - if p.waitForFreshIP(t.Context(), "ns", "demo-0") { + if p.waitForFreshIP(t.Context(), pod) { t.Fatal("no IP should time out") } got := execArgvs(rt) @@ -541,16 +548,12 @@ func TestWaitForFreshIPRenewNudgeWhenLeaseMissing(t *testing.T) { } func TestWaitForFreshIPNoRenewWhenIPPresent(t *testing.T) { - rt := &fakeRuntime{} - p := newTestProvider(t) - p.Runtime = rt - p.Probes = probes.NewManager(t.Context()) - p.wakeFreshIPBudget = 200 * time.Millisecond - p.wakeFreshIPInterval = 10 * time.Millisecond + p, pod, v := newDropNICWakeFixture(t, 200*time.Millisecond, 10*time.Millisecond) + rt := p.Runtime.(*fakeRuntime) p.wakeRenewNudgeDelay = time.Nanosecond // even an instant nudge window must not fire - trackWindowsPodNoIP(t, p, "10.0.0.9") + v.IP = "10.0.0.9" - if !p.waitForFreshIP(t.Context(), "ns", "demo-0") { + if !p.waitForFreshIP(t.Context(), pod) { t.Fatal("IP present should return true") } if len(rt.execCalls) != 0 { @@ -574,7 +577,7 @@ func TestWaitForFreshIPNoRenewForLinux(t *testing.T) { }) p.trackPod(pod, &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0"}) - if p.waitForFreshIP(t.Context(), "ns", "demo-0") { + if p.waitForFreshIP(t.Context(), pod) { t.Fatal("no IP should time out") } if len(rt.execCalls) != 0 { @@ -583,23 +586,17 @@ func TestWaitForFreshIPNoRenewForLinux(t *testing.T) { } func TestWaitForFreshIPLeaseLandingDuringNudgeWins(t *testing.T) { - // Review finding: if the renew exec blocks past the deadline while the - // lease lands mid-exec, the verdict must be success, not timeout. - rt := &fakeRuntime{} - p := newTestProvider(t) - p.Runtime = rt - p.Probes = probes.NewManager(t.Context()) - p.wakeFreshIPBudget = 100 * time.Millisecond - p.wakeFreshIPInterval = 10 * time.Millisecond + // A lease landing while the renew exec blocks past the deadline must win over timeout. + p, pod, _ := newDropNICWakeFixture(t, 100*time.Millisecond, 10*time.Millisecond) + rt := p.Runtime.(*fakeRuntime) p.wakeRenewNudgeDelay = 20 * time.Millisecond - trackWindowsPodNoIP(t, p, "") rt.onExec = func() { time.Sleep(150 * time.Millisecond) // block past the 100ms deadline p.setVMIP("ns", "demo-0", "10.0.0.9") } - if !p.waitForFreshIP(t.Context(), "ns", "demo-0") { + if !p.waitForFreshIP(t.Context(), pod) { t.Fatal("lease landed during the nudge exec; verdict must be success") } } From 1885db43ae41fa76fd7da70f69e154f4a6877149 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 19 Jul 2026 12:23:25 +0800 Subject: [PATCH 3/8] review: thread the pod into waitForFreshIP, dedup wake fixtures, tighten comments waitForFreshIP re-derived the guest OS through a lock and map lookup that could race pod tracking mid-wait; both callers already hold the pod, so take it as the parameter and drop isWindowsGuest. Fold the nudge tests onto newDropNICWakeFixture and trim multi-line comments to their one load-bearing constraint. --- provider/cocoon/postclone.go | 2 +- provider/cocoon/provider.go | 9 --------- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/provider/cocoon/postclone.go b/provider/cocoon/postclone.go index 8ee6bce..fc7c27a 100644 --- a/provider/cocoon/postclone.go +++ b/provider/cocoon/postclone.go @@ -123,7 +123,7 @@ func (p *Provider) runPostCloneSetup(ctx context.Context, pod *corev1.Pod, spec // ready then would promise an IP resolveVMIP can't yet return. On timeout mark // failed, never ready-without-IP. func (p *Provider) markReadyAfterIP(ctx context.Context, pod *corev1.Pod, v *vm.VM) { - gotIP := p.waitForFreshIP(ctx, pod.Namespace, pod.Name) + gotIP := p.waitForFreshIP(ctx, pod) if ctx.Err() != nil { return } diff --git a/provider/cocoon/provider.go b/provider/cocoon/provider.go index 50ae6e5..107d775 100644 --- a/provider/cocoon/provider.go +++ b/provider/cocoon/provider.go @@ -20,7 +20,6 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/record" - cocoonv1 "github.com/cocoonstack/cocoon-common/apis/v1" commonk8s "github.com/cocoonstack/cocoon-common/k8s" "github.com/cocoonstack/cocoon-common/meta" "github.com/cocoonstack/cocoon-common/oci" @@ -350,14 +349,6 @@ func (p *Provider) buildProbe(namespace, name string) probes.Probe { } } -// isWindowsGuest reports whether the tracked pod declares a Windows guest. -func (p *Provider) isWindowsGuest(namespace, name string) bool { - p.mu.RLock() - pod := p.pods[meta.PodKey(namespace, name)] - p.mu.RUnlock() - return pod != nil && meta.ParseVMSpec(pod).OS == string(cocoonv1.OSWindows) -} - func (p *Provider) probePort(ctx context.Context, namespace, name string) string { pod, _ := p.GetPod(ctx, namespace, name) if pod == nil { From 59e596db4680d84e6463c8a15ba6ce26a20f44cc Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 19 Jul 2026 14:10:33 +0800 Subject: [PATCH 4/8] fix(hibernate): renew unconditionally on a failed NIC drop; bind the IP waiter to its VMID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An exec error on the release does not prove the guest skipped it — a vsock timeout can land after the command ran — so gating the compensation renew on a confirmed release could leave a live VM lease-less. Renew regardless; renewing a still-bound adapter is benign. waitForFreshIP resolved the VM by pod key each round, so a same-name pod recreate mid-wait handed the waiter the successor VM: the renew nudge and the caller's refresh/notify all sit outside the VMID lifecycle guard. The waiter now carries the VMID it was armed for and bails on mismatch. --- provider/cocoon/postclone.go | 2 +- provider/cocoon/update.go | 20 +++++++--------- provider/cocoon/update_test.go | 43 ++++++++++++++++++++++++++++++---- 3 files changed, 49 insertions(+), 16 deletions(-) diff --git a/provider/cocoon/postclone.go b/provider/cocoon/postclone.go index fc7c27a..8a6f33d 100644 --- a/provider/cocoon/postclone.go +++ b/provider/cocoon/postclone.go @@ -123,7 +123,7 @@ func (p *Provider) runPostCloneSetup(ctx context.Context, pod *corev1.Pod, spec // ready then would promise an IP resolveVMIP can't yet return. On timeout mark // failed, never ready-without-IP. func (p *Provider) markReadyAfterIP(ctx context.Context, pod *corev1.Pod, v *vm.VM) { - gotIP := p.waitForFreshIP(ctx, pod) + gotIP := p.waitForFreshIP(ctx, pod, v.ID) if ctx.Err() != nil { return } diff --git a/provider/cocoon/update.go b/provider/cocoon/update.go index c1064bd..a348614 100644 --- a/provider/cocoon/update.go +++ b/provider/cocoon/update.go @@ -151,24 +151,21 @@ func (p *Provider) hibernate(ctx context.Context, pod *corev1.Pod, v *vm.VM) err // dropNICForHibernate releases the lease then detaches the NIC (VMware Tools' // suspend default): the snapshot carries no cached lease, so restored clones -// DISCOVER instead of drawing a NAK. Best-effort: a sick guest still hibernates, -// and a failed detach re-acquires the lease it just released. +// DISCOVER instead of drawing a NAK. Best-effort: a sick guest still hibernates. +// A failed detach renews unconditionally — an exec error does not prove the +// guest skipped the release, and renewing a still-bound adapter is benign. func (p *Provider) dropNICForHibernate(ctx context.Context, v *vm.VM) error { logger := log.WithFunc("Provider.dropNICForHibernate") - released := false if err := p.execGuestIpconfig(ctx, v.ID, "release"); err != nil { metrics.HibernateTotal.WithLabelValues("dhcp_release", "failed").Inc() logger.Warnf(ctx, "dhcp release before hibernate %s: %v (proceeding)", v.Name, err) } else { - released = true metrics.HibernateTotal.WithLabelValues("dhcp_release", "ok").Inc() } if err := p.Runtime.NetResize(ctx, v.ID, 0); err != nil { metrics.HibernateTotal.WithLabelValues("netresize", "failed").Inc() - if released { - if renewErr := p.execGuestIpconfig(ctx, v.ID, "renew"); renewErr != nil { - logger.Warnf(ctx, "dhcp renew after failed NIC drop %s: %v", v.Name, renewErr) - } + if renewErr := p.execGuestIpconfig(ctx, v.ID, "renew"); renewErr != nil { + logger.Warnf(ctx, "dhcp renew after failed NIC drop %s: %v", v.Name, renewErr) } return fmt.Errorf("drop NIC pre-hibernate %s: %w", v.Name, err) } @@ -269,7 +266,7 @@ func (p *Provider) dispatchHibernateRestore(pod *corev1.Pod, spec meta.VMSpec, v // finalizeDropNICWake holds Ready until the fresh NIC's lease lands. func (p *Provider) finalizeDropNICWake(ctx context.Context, pod *corev1.Pod, v *vm.VM) { - gotIP := p.waitForFreshIP(ctx, pod) + gotIP := p.waitForFreshIP(ctx, pod, v.ID) if ctx.Err() != nil { return } @@ -321,7 +318,7 @@ func (p *Provider) markLifecycleStateForWake(ctx context.Context, pod *corev1.Po // resumes contend, so the first lease can land many seconds after resume; // a short budget would misread that as lifecycle=failed and trigger an // operator rebuild. -func (p *Provider) waitForFreshIP(ctx context.Context, pod *corev1.Pod) bool { +func (p *Provider) waitForFreshIP(ctx context.Context, pod *corev1.Pod, vmID string) bool { budget := cmp.Or(p.wakeFreshIPBudget, defaultWakeFreshIPBudget) interval := cmp.Or(p.wakeFreshIPInterval, defaultWakeFreshIPInterval) deadline := time.Now().Add(budget) @@ -329,7 +326,8 @@ func (p *Provider) waitForFreshIP(ctx context.Context, pod *corev1.Pod) bool { nudged := meta.ParseVMSpec(pod).OS != string(cocoonv1.OSWindows) for { v := p.vmForPod(pod.Namespace, pod.Name) - if v == nil { + // A same-name recreate swaps the tracked VM; never touch the successor. + if v == nil || v.ID != vmID { return false } if ip := p.resolveVMIP(pod.Namespace, pod.Name, v); ip != "" { diff --git a/provider/cocoon/update_test.go b/provider/cocoon/update_test.go index 9ca4c73..f555e9c 100644 --- a/provider/cocoon/update_test.go +++ b/provider/cocoon/update_test.go @@ -533,12 +533,47 @@ func TestHibernateRenewsWhenNICDropFails(t *testing.T) { } } +func TestHibernateRenewsEvenWhenReleaseVerdictUnknown(t *testing.T) { + rt := &fakeRuntime{execErr: errors.New("vsock timeout"), netResizeErr: errors.New("resize boom")} + p := newTestProvider(t) + p.Runtime = rt + p.Probes = probes.NewManager(t.Context()) + p.Clientset = fake.NewSimpleClientset() + + pod := newPodWithSpec(meta.VMSpec{ + VMName: "vk-ns-demo-0", + Backend: string(cocoonv1.BackendCloudHypervisor), + OS: string(cocoonv1.OSWindows), + }) + if err := p.hibernate(t.Context(), pod, &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0"}); err == nil { + t.Fatal("hibernate should fail when the NIC drop fails") + } + got := execArgvs(rt) + if len(got) != 2 || got[1] != "cmd /c ipconfig /renew" { + t.Errorf("exec calls = %v, want a renew attempt even though the release verdict was an error", got) + } +} + +func TestWaitForFreshIPBailsWhenVMSwapped(t *testing.T) { + p, pod, _ := newDropNICWakeFixture(t, 500*time.Millisecond, 10*time.Millisecond) + rt := p.Runtime.(*fakeRuntime) + p.wakeRenewNudgeDelay = time.Nanosecond + p.trackPod(pod, &vm.VM{ID: "vmid-successor", Name: "vk-ns-demo-0", IP: "10.0.0.9"}) + + if p.waitForFreshIP(t.Context(), pod, "vmid-wake") { + t.Fatal("waiter armed for a replaced VM must fail, not adopt the successor") + } + if len(rt.execCalls) != 0 { + t.Errorf("waiter must not exec into the successor VM, got %v", execArgvs(rt)) + } +} + func TestWaitForFreshIPRenewNudgeWhenLeaseMissing(t *testing.T) { p, pod, _ := newDropNICWakeFixture(t, 200*time.Millisecond, 10*time.Millisecond) rt := p.Runtime.(*fakeRuntime) p.wakeRenewNudgeDelay = 30 * time.Millisecond - if p.waitForFreshIP(t.Context(), pod) { + if p.waitForFreshIP(t.Context(), pod, "vmid-wake") { t.Fatal("no IP should time out") } got := execArgvs(rt) @@ -553,7 +588,7 @@ func TestWaitForFreshIPNoRenewWhenIPPresent(t *testing.T) { p.wakeRenewNudgeDelay = time.Nanosecond // even an instant nudge window must not fire v.IP = "10.0.0.9" - if !p.waitForFreshIP(t.Context(), pod) { + if !p.waitForFreshIP(t.Context(), pod, "vmid-wake") { t.Fatal("IP present should return true") } if len(rt.execCalls) != 0 { @@ -577,7 +612,7 @@ func TestWaitForFreshIPNoRenewForLinux(t *testing.T) { }) p.trackPod(pod, &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0"}) - if p.waitForFreshIP(t.Context(), pod) { + if p.waitForFreshIP(t.Context(), pod, "vmid-1") { t.Fatal("no IP should time out") } if len(rt.execCalls) != 0 { @@ -596,7 +631,7 @@ func TestWaitForFreshIPLeaseLandingDuringNudgeWins(t *testing.T) { p.setVMIP("ns", "demo-0", "10.0.0.9") } - if !p.waitForFreshIP(t.Context(), pod) { + if !p.waitForFreshIP(t.Context(), pod, "vmid-wake") { t.Fatal("lease landed during the nudge exec; verdict must be success") } } From dec4e523e5f39724deffd6754ed5d4a969835a24 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 19 Jul 2026 14:26:06 +0800 Subject: [PATCH 5/8] fix(hibernate): detach the compensation renew from the request; guard the lease write-back by VMID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drop can fail precisely because ctx died, and the compensation ran on that same dead context — exec.CommandContext then refuses it outright. Detach it with context.WithoutCancel, bounded by the exec's own timeout; the fake runtime now honors ctx like the real one so the case is testable. resolveVMIP looked the lease up for v's MAC but setVMIP wrote it back by pod key unconditionally, so a same-name recreate landing inside the lookup put the old MAC's IP on the successor VM with nothing to ever correct it. setVMIP now writes only when the tracked VMID still matches, and the lease is returned only after the write lands. --- provider/cocoon/create_test.go | 5 +++- provider/cocoon/provider.go | 13 ++++++--- provider/cocoon/update.go | 4 ++- provider/cocoon/update_test.go | 53 ++++++++++++++++++++++++++++++++-- 4 files changed, 66 insertions(+), 9 deletions(-) diff --git a/provider/cocoon/create_test.go b/provider/cocoon/create_test.go index f324dd3..c359315 100644 --- a/provider/cocoon/create_test.go +++ b/provider/cocoon/create_test.go @@ -1892,7 +1892,10 @@ type fakeExecCall struct { stdin string } -func (f *fakeRuntime) Exec(_ context.Context, vmID string, argv []string, env map[string]string, stdin io.Reader, stdout, stderr io.Writer) error { +func (f *fakeRuntime) Exec(ctx context.Context, vmID string, argv []string, env map[string]string, stdin io.Reader, stdout, stderr io.Writer) error { + if err := ctx.Err(); err != nil { + return err + } if f.onExec != nil { f.onExec() } diff --git a/provider/cocoon/provider.go b/provider/cocoon/provider.go index 107d775..d58351c 100644 --- a/provider/cocoon/provider.go +++ b/provider/cocoon/provider.go @@ -296,13 +296,13 @@ func (p *Provider) vmForPod(namespace, name string) *vm.VM { } // setVMIP updates the tracked VM's IP (copy-on-write for concurrency safety). -func (p *Provider) setVMIP(namespace, name, ip string) { +func (p *Provider) setVMIP(namespace, name, vmID, ip string) bool { p.mu.Lock() defer p.mu.Unlock() key := meta.PodKey(namespace, name) v, ok := p.vmsByPod[key] - if !ok { - return + if !ok || v.ID != vmID { + return false } updated := *v updated.IP = ip @@ -310,6 +310,7 @@ func (p *Provider) setVMIP(namespace, name, ip string) { if updated.Name != "" { p.vmsByName[updated.Name] = &updated } + return true } // resolveVMIP returns the VM's IP, falling back to a cocoon-net lease @@ -323,7 +324,11 @@ func (p *Provider) resolveVMIP(namespace, name string, v *vm.VM) string { if err != nil { return "" } - p.setVMIP(namespace, name, lease.IP) + // The lease belongs to v's MAC; if a same-name recreate swapped the tracked + // VM during the lookup, the IP must not leak onto the successor. + if !p.setVMIP(namespace, name, v.ID, lease.IP) { + return "" + } return lease.IP } diff --git a/provider/cocoon/update.go b/provider/cocoon/update.go index a348614..967e683 100644 --- a/provider/cocoon/update.go +++ b/provider/cocoon/update.go @@ -164,7 +164,9 @@ func (p *Provider) dropNICForHibernate(ctx context.Context, v *vm.VM) error { } if err := p.Runtime.NetResize(ctx, v.ID, 0); err != nil { metrics.HibernateTotal.WithLabelValues("netresize", "failed").Inc() - if renewErr := p.execGuestIpconfig(ctx, v.ID, "renew"); renewErr != nil { + // Cancel-detached: the drop may have failed because ctx died, and the + // compensation must still run (bounded by execGuestIpconfig's timeout). + if renewErr := p.execGuestIpconfig(context.WithoutCancel(ctx), v.ID, "renew"); renewErr != nil { logger.Warnf(ctx, "dhcp renew after failed NIC drop %s: %v", v.Name, renewErr) } return fmt.Errorf("drop NIC pre-hibernate %s: %w", v.Name, err) diff --git a/provider/cocoon/update_test.go b/provider/cocoon/update_test.go index f555e9c..ee51a8b 100644 --- a/provider/cocoon/update_test.go +++ b/provider/cocoon/update_test.go @@ -1,8 +1,11 @@ package cocoon import ( + "context" "errors" "maps" + "os" + "path/filepath" "reflect" "strings" "testing" @@ -13,6 +16,7 @@ import ( cocoonv1 "github.com/cocoonstack/cocoon-common/apis/v1" "github.com/cocoonstack/cocoon-common/meta" + "github.com/cocoonstack/vk-cocoon/network" "github.com/cocoonstack/vk-cocoon/probes" "github.com/cocoonstack/vk-cocoon/vm" ) @@ -305,7 +309,7 @@ func TestFinalizeDropNICWakeMarksReadyWhenIPArrives(t *testing.T) { }() time.AfterFunc(10*time.Millisecond, func() { - p.setVMIP("ns", "demo-0", "172.20.1.228") + p.setVMIP("ns", "demo-0", v.ID, "172.20.1.228") }) select { @@ -366,7 +370,7 @@ func TestFinalizeDropNICWakeSkipsLifecycleWhenHibernateRequested(t *testing.T) { time.AfterFunc(10*time.Millisecond, func() { meta.HibernateState(true).Apply(pod) - p.setVMIP("ns", "demo-0", "172.20.1.228") + p.setVMIP("ns", "demo-0", v.ID, "172.20.1.228") }) select { @@ -554,6 +558,49 @@ func TestHibernateRenewsEvenWhenReleaseVerdictUnknown(t *testing.T) { } } +func TestHibernateRenewSurvivesCancelledContext(t *testing.T) { + rt := &fakeRuntime{netResizeErr: errors.New("resize boom")} + p := newTestProvider(t) + p.Runtime = rt + p.Probes = probes.NewManager(t.Context()) + p.Clientset = fake.NewSimpleClientset() + + ctx, cancel := context.WithCancel(t.Context()) + rt.onExec = func() { cancel() } // the request dies while the release is in flight + + pod := newPodWithSpec(meta.VMSpec{ + VMName: "vk-ns-demo-0", + Backend: string(cocoonv1.BackendCloudHypervisor), + OS: string(cocoonv1.OSWindows), + }) + if err := p.hibernate(ctx, pod, &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0"}); err == nil { + t.Fatal("hibernate should fail when the NIC drop fails") + } + got := execArgvs(rt) + if len(got) != 2 || got[1] != "cmd /c ipconfig /renew" { + t.Errorf("exec calls = %v, want the renew to outlive the cancelled request", got) + } +} + +func TestResolveVMIPRefusesWriteToSwappedVM(t *testing.T) { + p := newTestProvider(t) + leases := filepath.Join(t.TempDir(), "leases.json") + if err := os.WriteFile(leases, []byte(`[{"mac":"aa:bb:cc:dd:ee:ff","ip":"172.20.0.10","expiry":"2099-01-01T00:00:00Z"}]`), 0o644); err != nil { + t.Fatalf("write leases: %v", err) + } + p.LeaseParser = network.NewLeaseParser(leases) + pod := newPodWithSpec(meta.VMSpec{VMName: "vk-ns-demo-0"}) + p.trackPod(pod, &vm.VM{ID: "vmid-successor", Name: "vk-ns-demo-0"}) + + stale := &vm.VM{ID: "vmid-old", Name: "vk-ns-demo-0", MAC: "aa:bb:cc:dd:ee:ff"} + if ip := p.resolveVMIP("ns", "demo-0", stale); ip != "" { + t.Errorf("stale VM's lease resolved to %q, want refused", ip) + } + if got := p.vmForPod("ns", "demo-0").IP; got != "" { + t.Errorf("successor VM polluted with IP %q", got) + } +} + func TestWaitForFreshIPBailsWhenVMSwapped(t *testing.T) { p, pod, _ := newDropNICWakeFixture(t, 500*time.Millisecond, 10*time.Millisecond) rt := p.Runtime.(*fakeRuntime) @@ -628,7 +675,7 @@ func TestWaitForFreshIPLeaseLandingDuringNudgeWins(t *testing.T) { rt.onExec = func() { time.Sleep(150 * time.Millisecond) // block past the 100ms deadline - p.setVMIP("ns", "demo-0", "10.0.0.9") + p.setVMIP("ns", "demo-0", "vmid-wake", "10.0.0.9") } if !p.waitForFreshIP(t.Context(), pod, "vmid-wake") { From 25f0e0e61b055dbb674f24991eb93329d15330c2 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 19 Jul 2026 14:36:41 +0800 Subject: [PATCH 6/8] fix(hibernate): detach the NIC rollback from the request's cancellation SnapshotSave, push and remove can fail precisely because ctx died, and rollbackHibernateNIC then ran its NetResize(1) and renew on that dead context, leaving an online VM NIC-less. The whole rollback now runs under one cancel-detached, bounded lifetime; the fake runtime's NetResize honors ctx like the real CommandContext so the case stays pinned. --- provider/cocoon/create_test.go | 5 ++++- provider/cocoon/update.go | 7 ++++++- provider/cocoon/update_test.go | 24 ++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/provider/cocoon/create_test.go b/provider/cocoon/create_test.go index c359315..97dc869 100644 --- a/provider/cocoon/create_test.go +++ b/provider/cocoon/create_test.go @@ -1880,7 +1880,10 @@ type netResizeCall struct { target int } -func (f *fakeRuntime) NetResize(_ context.Context, vmID string, target int) error { +func (f *fakeRuntime) NetResize(ctx context.Context, vmID string, target int) error { + if err := ctx.Err(); err != nil { + return err + } f.netResizeCalls = append(f.netResizeCalls, netResizeCall{vmID: vmID, target: target}) return f.netResizeErr } diff --git a/provider/cocoon/update.go b/provider/cocoon/update.go index 967e683..81f062a 100644 --- a/provider/cocoon/update.go +++ b/provider/cocoon/update.go @@ -35,7 +35,8 @@ const ( // guestIpconfigTimeout bounds the vsock exec for release/renew so a sick // guest (dead agent, stopped DHCP service) cannot stall hibernate or wake. - guestIpconfigTimeout = 20 * time.Second + guestIpconfigTimeout = 20 * time.Second + hibernateRollbackTimeout = 30 * time.Second ) // UpdatePod handles hibernate/wake transitions; other spec changes are no-ops @@ -181,6 +182,10 @@ func (p *Provider) rollbackHibernateNIC(ctx context.Context, v *vm.VM, dropped b return } logger := log.WithFunc("Provider.rollbackHibernateNIC") + // Cancel-detached: the failure that triggered the rollback may be ctx dying, + // and an online VM must not be left NIC-less. + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), hibernateRollbackTimeout) + defer cancel() if err := p.Runtime.NetResize(ctx, v.ID, 1); err != nil { logger.Errorf(ctx, err, "re-add NIC after hibernate failure %s", v.Name) return diff --git a/provider/cocoon/update_test.go b/provider/cocoon/update_test.go index ee51a8b..12cbe7f 100644 --- a/provider/cocoon/update_test.go +++ b/provider/cocoon/update_test.go @@ -582,6 +582,30 @@ func TestHibernateRenewSurvivesCancelledContext(t *testing.T) { } } +func TestHibernateRollbackSurvivesCancelledContext(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + rt := &fakeRuntime{snapshotSaveErr: errors.New("save boom"), snapshotSaveHook: cancel} + p := newTestProvider(t) + p.Runtime = rt + p.Probes = probes.NewManager(t.Context()) + p.Clientset = fake.NewSimpleClientset() + + pod := newPodWithSpec(meta.VMSpec{ + VMName: "vk-ns-demo-0", + Backend: string(cocoonv1.BackendCloudHypervisor), + OS: string(cocoonv1.OSWindows), + }) + if err := p.hibernate(ctx, pod, &vm.VM{ID: "vmid-1", Name: "vk-ns-demo-0"}); err == nil { + t.Fatal("hibernate should fail on snapshot save error") + } + if len(rt.netResizeCalls) != 2 || rt.netResizeCalls[1].target != 1 { + t.Errorf("NetResize calls = %#v, want the NIC re-add to outlive the cancelled request", rt.netResizeCalls) + } + if got := execArgvs(rt); len(got) != 2 || got[1] != "cmd /c ipconfig /renew" { + t.Errorf("exec calls = %v, want the rollback renew to share the detached lifetime", got) + } +} + func TestResolveVMIPRefusesWriteToSwappedVM(t *testing.T) { p := newTestProvider(t) leases := filepath.Join(t.TempDir(), "leases.json") From 401d7bbb0ef58b8854b19c91eeaeb054ced99b87 Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 19 Jul 2026 14:46:13 +0800 Subject: [PATCH 7/8] fix: derive the provider lifecycle context from the caller Rooted in context.Background, lifecycle goroutines outlived SIGTERM whenever Close was bypassed; buildProvider already holds the signal context, so derive from it and let cancellation propagate. --- main.go | 2 +- provider/cocoon/create_test.go | 2 +- provider/cocoon/provider.go | 9 +++++---- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/main.go b/main.go index b93ddbf..17bc2fc 100644 --- a/main.go +++ b/main.go @@ -248,7 +248,7 @@ func buildProvider(ctx context.Context, opts buildOpts) (*cocoon.Provider, error } logger.Infof(ctx, "registry backend: OCI %s", opts.ociRegistry) runtime := vm.NewCocoonCLI(opts.cocoonBin) - p := cocoon.NewProvider() + p := cocoon.NewProvider(ctx) p.NodeName = opts.nodeName p.Clientset = opts.clientset p.Recorder = opts.recorder diff --git a/provider/cocoon/create_test.go b/provider/cocoon/create_test.go index 97dc869..610495b 100644 --- a/provider/cocoon/create_test.go +++ b/provider/cocoon/create_test.go @@ -1953,7 +1953,7 @@ func (nopWriteCloser) Close() error { return nil } // race with the next test on a recycled pod heap address. func newTestProvider(t *testing.T) *Provider { t.Helper() - p := NewProvider() + p := NewProvider(t.Context()) t.Cleanup(p.Close) return p } diff --git a/provider/cocoon/provider.go b/provider/cocoon/provider.go index d58351c..23fc1d8 100644 --- a/provider/cocoon/provider.go +++ b/provider/cocoon/provider.go @@ -117,10 +117,11 @@ type Provider struct { wakeRenewNudgeDelay time.Duration } -// NewProvider constructs a Provider with empty tables. -// Default Pinger is NopPinger so tests degrade gracefully. -func NewProvider() *Provider { - lifecycleCtx, lifecycleStop := context.WithCancel(context.Background()) +// NewProvider constructs a Provider with empty tables; background work stops +// when ctx is cancelled or Close is called. Default Pinger is NopPinger so +// tests degrade gracefully. +func NewProvider(ctx context.Context) *Provider { + lifecycleCtx, lifecycleStop := context.WithCancel(ctx) return &Provider{ startTime: time.Now(), lifecycleCtx: lifecycleCtx, From b1ff43b3a396ac9fecb7366781e7ceeed5741c4e Mon Sep 17 00:00:00 2001 From: CMGS Date: Sun, 19 Jul 2026 14:54:49 +0800 Subject: [PATCH 8/8] review: dedup lease fixtures, drop dead test fields, tighten the drop-NIC comments The unconditional-renew rationale lived twice (godoc and call site); it now lives once at the call it explains. newLeaseParser replaces the leases.json boilerplate duplicated across two tests. The swap test's nudge delay and the cancelled-ctx test's netResizeErr never influenced their runs. --- provider/cocoon/create_test.go | 18 ++++++++++++------ provider/cocoon/provider.go | 2 +- provider/cocoon/update.go | 6 ++---- provider/cocoon/update_test.go | 12 ++---------- 4 files changed, 17 insertions(+), 21 deletions(-) diff --git a/provider/cocoon/create_test.go b/provider/cocoon/create_test.go index 610495b..3eb1192 100644 --- a/provider/cocoon/create_test.go +++ b/provider/cocoon/create_test.go @@ -1607,12 +1607,7 @@ func TestGetPodStatusRefreshesIPFromLease(t *testing.T) { p.Probes = probes.NewManager(t.Context()) p.Probes.Set("ns/demo-0", probes.Result{Ready: true}) - leasePath := filepath.Join(t.TempDir(), "leases.json") - leases := `[{"mac":"aa:bb:cc:dd:ee:ff","ip":"172.20.0.88","expiry":"2099-01-01T00:00:00Z"}]` - if err := os.WriteFile(leasePath, []byte(leases), 0o644); err != nil { - t.Fatalf("write leases: %v", err) - } - p.LeaseParser = network.NewLeaseParser(leasePath) + p.LeaseParser = newLeaseParser(t, "aa:bb:cc:dd:ee:ff", "172.20.0.88") pod := newPodWithSpec(meta.VMSpec{VMName: "vk-ns-demo-0", Mode: "run"}) p.trackPod(pod, &vm.VM{ID: "vmid", Name: "vk-ns-demo-0", MAC: "aa:bb:cc:dd:ee:ff"}) @@ -1958,6 +1953,17 @@ func newTestProvider(t *testing.T) *Provider { return p } +// newLeaseParser writes a one-entry leases.json and returns a parser for it. +func newLeaseParser(t *testing.T, mac, ip string) *network.LeaseParser { + t.Helper() + path := filepath.Join(t.TempDir(), "leases.json") + entry := `[{"mac":"` + mac + `","ip":"` + ip + `","expiry":"2099-01-01T00:00:00Z"}]` + if err := os.WriteFile(path, []byte(entry), 0o644); err != nil { + t.Fatalf("write leases: %v", err) + } + return network.NewLeaseParser(path) +} + func newPodWithSpec(spec meta.VMSpec) *corev1.Pod { pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "demo-0", Namespace: "ns"}} spec.Managed = true diff --git a/provider/cocoon/provider.go b/provider/cocoon/provider.go index 23fc1d8..ca5e290 100644 --- a/provider/cocoon/provider.go +++ b/provider/cocoon/provider.go @@ -118,7 +118,7 @@ type Provider struct { } // NewProvider constructs a Provider with empty tables; background work stops -// when ctx is cancelled or Close is called. Default Pinger is NopPinger so +// when ctx is canceled or Close is called. Default Pinger is NopPinger so // tests degrade gracefully. func NewProvider(ctx context.Context) *Provider { lifecycleCtx, lifecycleStop := context.WithCancel(ctx) diff --git a/provider/cocoon/update.go b/provider/cocoon/update.go index 81f062a..f3cb3ea 100644 --- a/provider/cocoon/update.go +++ b/provider/cocoon/update.go @@ -153,8 +153,6 @@ func (p *Provider) hibernate(ctx context.Context, pod *corev1.Pod, v *vm.VM) err // dropNICForHibernate releases the lease then detaches the NIC (VMware Tools' // suspend default): the snapshot carries no cached lease, so restored clones // DISCOVER instead of drawing a NAK. Best-effort: a sick guest still hibernates. -// A failed detach renews unconditionally — an exec error does not prove the -// guest skipped the release, and renewing a still-bound adapter is benign. func (p *Provider) dropNICForHibernate(ctx context.Context, v *vm.VM) error { logger := log.WithFunc("Provider.dropNICForHibernate") if err := p.execGuestIpconfig(ctx, v.ID, "release"); err != nil { @@ -165,8 +163,8 @@ func (p *Provider) dropNICForHibernate(ctx context.Context, v *vm.VM) error { } if err := p.Runtime.NetResize(ctx, v.ID, 0); err != nil { metrics.HibernateTotal.WithLabelValues("netresize", "failed").Inc() - // Cancel-detached: the drop may have failed because ctx died, and the - // compensation must still run (bounded by execGuestIpconfig's timeout). + // Renew regardless, detached from ctx: an exec error does not prove the + // guest skipped the release, and the drop may have failed because ctx died. if renewErr := p.execGuestIpconfig(context.WithoutCancel(ctx), v.ID, "renew"); renewErr != nil { logger.Warnf(ctx, "dhcp renew after failed NIC drop %s: %v", v.Name, renewErr) } diff --git a/provider/cocoon/update_test.go b/provider/cocoon/update_test.go index 12cbe7f..384eb0f 100644 --- a/provider/cocoon/update_test.go +++ b/provider/cocoon/update_test.go @@ -4,8 +4,6 @@ import ( "context" "errors" "maps" - "os" - "path/filepath" "reflect" "strings" "testing" @@ -16,7 +14,6 @@ import ( cocoonv1 "github.com/cocoonstack/cocoon-common/apis/v1" "github.com/cocoonstack/cocoon-common/meta" - "github.com/cocoonstack/vk-cocoon/network" "github.com/cocoonstack/vk-cocoon/probes" "github.com/cocoonstack/vk-cocoon/vm" ) @@ -559,7 +556,7 @@ func TestHibernateRenewsEvenWhenReleaseVerdictUnknown(t *testing.T) { } func TestHibernateRenewSurvivesCancelledContext(t *testing.T) { - rt := &fakeRuntime{netResizeErr: errors.New("resize boom")} + rt := &fakeRuntime{} p := newTestProvider(t) p.Runtime = rt p.Probes = probes.NewManager(t.Context()) @@ -608,11 +605,7 @@ func TestHibernateRollbackSurvivesCancelledContext(t *testing.T) { func TestResolveVMIPRefusesWriteToSwappedVM(t *testing.T) { p := newTestProvider(t) - leases := filepath.Join(t.TempDir(), "leases.json") - if err := os.WriteFile(leases, []byte(`[{"mac":"aa:bb:cc:dd:ee:ff","ip":"172.20.0.10","expiry":"2099-01-01T00:00:00Z"}]`), 0o644); err != nil { - t.Fatalf("write leases: %v", err) - } - p.LeaseParser = network.NewLeaseParser(leases) + p.LeaseParser = newLeaseParser(t, "aa:bb:cc:dd:ee:ff", "172.20.0.10") pod := newPodWithSpec(meta.VMSpec{VMName: "vk-ns-demo-0"}) p.trackPod(pod, &vm.VM{ID: "vmid-successor", Name: "vk-ns-demo-0"}) @@ -628,7 +621,6 @@ func TestResolveVMIPRefusesWriteToSwappedVM(t *testing.T) { func TestWaitForFreshIPBailsWhenVMSwapped(t *testing.T) { p, pod, _ := newDropNICWakeFixture(t, 500*time.Millisecond, 10*time.Millisecond) rt := p.Runtime.(*fakeRuntime) - p.wakeRenewNudgeDelay = time.Nanosecond p.trackPod(pod, &vm.VM{ID: "vmid-successor", Name: "vk-ns-demo-0", IP: "10.0.0.9"}) if p.waitForFreshIP(t.Context(), pod, "vmid-wake") {