diff --git a/docs/lifecycle.md b/docs/lifecycle.md index 7354cbe..f837ac6 100644 --- a/docs/lifecycle.md +++ b/docs/lifecycle.md @@ -35,7 +35,11 @@ and the hibernate transition. it, same as the UpdatePod wake path. - **Fork-from** (`spec.ForkFrom`): snapshot the named source VM once (deduped via `ensureForkSnapshot`) and clone every fork off that - shared snapshot. + shared snapshot. The fork snapshot is a per-lineage baseline: every + fresh (non-restore) bring-up drops `fork-` before booting, so a + recreated same-name VM can never hand a dead incarnation's baseline + to new sub-agents; a hibernate restore keeps it, since a wake + continues the same lineage. - **`Managed=false`** (static / externally-managed VMs, e.g. Windows toolboxes on an external QEMU host): skip the runtime entirely and adopt the pre-assigned `VMID` / `IP` / `VNCPort` the operator @@ -107,7 +111,15 @@ and the hibernate transition. i.e. slot 0 of its CocoonSet). - `never`: skip snapshots entirely. 3. `Runtime.Remove(vmID)` to destroy the VM. -4. Forget the pod from the in-memory tables. +4. Drop the local snapshot and its fork snapshot, **unless** the pod carries + `vm.cocoonstack.io/keep-snapshot-on-delete`. The operator sets that flag + when the delete is a `hibernatePolicy: release` seat release: the VM state + stays claimable from the `:hibernate` tag, so the node-local snapshot is + kept as the warm-wake cache that lets a wake landing back on this node + skip the registry pull. `resolveWakeSource` still verifies any local copy + against the tag's `SnapshotID`, so keeping it cannot restore stale state. + A missing flag only costs a pull, never correctness. +5. Forget the pod from the in-memory tables. ## UpdatePod diff --git a/go.mod b/go.mod index 8eb758d..14d732d 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/cocoonstack/vk-cocoon go 1.26.5 require ( - github.com/cocoonstack/cocoon-common v0.2.9-0.20260728170736-b0a8b954c944 + github.com/cocoonstack/cocoon-common v0.2.9-0.20260731042413-9ca4f1c8fc0f github.com/google/go-containerregistry v0.21.7 github.com/projecteru2/core v0.0.0-20241016125006-ff909eefe04c github.com/prometheus/client_golang v1.23.2 diff --git a/go.sum b/go.sum index 83a9e12..31e9e6a 100644 --- a/go.sum +++ b/go.sum @@ -43,8 +43,8 @@ github.com/cockroachdb/logtags v0.0.0-20230118201751-21c54148d20b h1:r6VH0faHjZe github.com/cockroachdb/logtags v0.0.0-20230118201751-21c54148d20b/go.mod h1:Vz9DsVWQQhf3vs21MhPMZpMGSht7O/2vFW2xusFUVOs= github.com/cockroachdb/redact v1.1.3 h1:AKZds10rFSIj7qADf0g46UixK8NNLwWTNdCIGS5wfSQ= github.com/cockroachdb/redact v1.1.3/go.mod h1:BVNblN9mBWFyMyqK1k3AAiSxhvhfK2oOZZ2lK+dpvRg= -github.com/cocoonstack/cocoon-common v0.2.9-0.20260728170736-b0a8b954c944 h1:Jru/8LxzwHZKVjFklwClA/UU6oRV9wVVTIJxLSeMBlg= -github.com/cocoonstack/cocoon-common v0.2.9-0.20260728170736-b0a8b954c944/go.mod h1:VSfgYiWxoHRnWybzQNaxmK4kVoLT9ffiMKll5/i92CM= +github.com/cocoonstack/cocoon-common v0.2.9-0.20260731042413-9ca4f1c8fc0f h1:mcHAOv4VUN1cc3CSAeoWyKh/iwPQ4WithgB2sKAVi0Y= +github.com/cocoonstack/cocoon-common v0.2.9-0.20260731042413-9ca4f1c8fc0f/go.mod h1:VSfgYiWxoHRnWybzQNaxmK4kVoLT9ffiMKll5/i92CM= github.com/codegangsta/inject v0.0.0-20150114235600-33e0aa1cb7c0/go.mod h1:4Zcjuz89kmFXt9morQgcfYZAYZ5n8WHjt81YYWIwtTM= github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= github.com/coreos/go-etcd v2.0.0+incompatible/go.mod h1:Jez6KQU2B/sWsbdaef3ED8NzMklzPG4d5KIOhIy30Tk= diff --git a/metrics/metrics.go b/metrics/metrics.go index 7cca216..93510d5 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -21,7 +21,7 @@ var ( Name: "pod_lifecycle_total", Help: "Number of pod lifecycle operations by op, result, and reason.", }, - // result=ok|failed|skipped; reason=adopted|noop|missing_vmname|no_vm + // result=ok|failed|skipped; reason=adopted|noop|missing_vmname|no_vm|seat_release []string{"op", labelResult, "reason"}, ) diff --git a/provider/cocoon/create.go b/provider/cocoon/create.go index d2e9c1b..31ad7ba 100644 --- a/provider/cocoon/create.go +++ b/provider/cocoon/create.go @@ -194,8 +194,7 @@ func (p *Provider) hibernateEvidence(ctx context.Context, vmName string) (bool, return true, m.Annotations[manifest.AnnotationSnapshotBaseImage], nil } -// bringUpVM dispatches on mode: unmanaged, clone, run, or fork. The -// returned sourceImage feeds post-clone classification. +// bringUpVM boots the VM for its mode; the returned sourceImage feeds post-clone classification. func (p *Provider) bringUpVM(ctx context.Context, pod *corev1.Pod, spec meta.VMSpec) (*vm.VM, string, error) { if !spec.Managed { runtime := meta.ParseVMRuntime(pod) @@ -212,8 +211,7 @@ func (p *Provider) bringUpVM(ctx context.Context, pod *corev1.Pod, spec meta.VMS if err != nil { return nil, "", err } - switch { - case meta.ReadRestoreFromHibernate(pod): + if meta.ReadRestoreFromHibernate(pod) { sourceName, snapshot, err := p.resolveWakeSource(ctx, spec.VMName) if err != nil { return nil, "", err @@ -223,7 +221,15 @@ func (p *Provider) bringUpVM(ctx context.Context, pod *corev1.Pod, spec meta.VMS return nil, "", err } return v, "", nil + } + + // Invalidate the fork snapshot from a previous incarnation so sub-agents clone from current state. + forkName := forkSnapshotName(spec.VMName) + if err := p.Runtime.SnapshotRemoveIfExists(ctx, forkName); err != nil { + log.WithFunc("Provider.bringUpVM").Errorf(ctx, err, "invalidate fork snapshot %s", forkName) + } + switch { case fromDir != "": if mode == string(cocoonv1.AgentModeRun) { return nil, "", fmt.Errorf("annotation %s is incompatible with mode=run", meta.AnnotationCloneFromDir) @@ -284,12 +290,6 @@ func (p *Provider) bringUpVM(ctx context.Context, pod *corev1.Pod, spec meta.VMS if err != nil { return nil, "", fmt.Errorf("run vm %s: %w", spec.VMName, err) } - // Invalidate fork snapshot from a previous incarnation so later - // sub-agents clone from current state. - forkName := forkSnapshotName(spec.VMName) - if err := p.Runtime.SnapshotRemoveIfExists(ctx, forkName); err != nil { - log.WithFunc("Provider.bringUpVM").Errorf(ctx, err, "invalidate fork snapshot %s", forkName) - } return v, "", nil default: // clone is the default @@ -334,10 +334,9 @@ func (p *Provider) imagePresent(ctx context.Context, digest string) bool { return digest != "" && p.Runtime.Image(ctx, digest) == nil } -// ensureSnapshotBaseImage materializes a snapshot's OCI-ref base image before a -// clone or restore. cocoon's `vm clone --pull` only fetches http(s) bases, so an -// OCI-ref base must be imported here. Dedup by digest — the same bytes may be -// local under another name (epoch→AR ref migration). +// cocoon's `vm clone --pull` only fetches http(s) bases, so an OCI-ref base +// must be imported here. Dedup by digest — the same bytes may be local under +// another name (epoch→AR ref migration). func (p *Provider) ensureSnapshotBaseImage(ctx context.Context, snapshot *vm.Snapshot) error { if snapshot == nil || snapshot.Image == "" || isHTTPURL(snapshot.Image) || p.imagePresent(ctx, snapshot.ImageDigest) { return nil @@ -543,8 +542,6 @@ func awaitFlight[T any](ctx context.Context, ch <-chan singleflight.Result, zero } } -// parseCloneFromDirAnnotation returns the validated absolute, canonical -// path from the clone-from-dir annotation, or "" when absent. func parseCloneFromDirAnnotation(pod *corev1.Pod) (string, error) { raw := strings.TrimSpace(pod.Annotations[meta.AnnotationCloneFromDir]) if raw == "" { @@ -573,8 +570,6 @@ func isClonedBoot(pod *corev1.Pod, spec meta.VMSpec) bool { return hasExplicitCloneSource(pod, spec) || strings.ToLower(spec.Mode) != string(cocoonv1.AgentModeRun) } -// hasExplicitCloneSource reports whether the pod names its own clone source -// (clone-from-dir or fork-from). func hasExplicitCloneSource(pod *corev1.Pod, spec meta.VMSpec) bool { return strings.TrimSpace(pod.Annotations[meta.AnnotationCloneFromDir]) != "" || spec.ForkFrom != "" } @@ -593,7 +588,6 @@ func assertSnapshotBackend(snapshot *vm.Snapshot, targetBackend string) error { snapshot.Name, snapshot.Hypervisor, targetBackend) } -// isHTTPURL reports whether ref looks like an HTTP(S) cloud-image URL. func isHTTPURL(ref string) bool { return strings.HasPrefix(ref, "http://") || strings.HasPrefix(ref, "https://") } diff --git a/provider/cocoon/create_test.go b/provider/cocoon/create_test.go index 34e9da7..10c3fc8 100644 --- a/provider/cocoon/create_test.go +++ b/provider/cocoon/create_test.go @@ -215,24 +215,37 @@ func TestEnsureForkSnapshotDedupsConcurrentSaves(t *testing.T) { } } -func TestCreatePodRunModeInvalidatesForkSnapshot(t *testing.T) { - // A fresh main VM must drop the old fork snapshot so sub-agents cloned - // after a main recreate pick up current state, not the stale checkpoint. - rt := &fakeRuntime{runVM: &vm.VM{ID: "vmid-main", Name: "vk-ns-demo-0"}} - p := newTestProvider(t) - p.Runtime = rt - - pod := newPodWithSpec(meta.VMSpec{ - VMName: "vk-ns-demo-0", - Image: "registry.example/cocoon/ubuntu:24.04", - Mode: "run", - OS: "linux", - }) - if err := p.CreatePod(t.Context(), pod); err != nil { - t.Fatalf("create: %v", err) +// TestCreatePodInvalidatesForkSnapshot locks the fresh-boot invalidation: a recreated main must drop the old fork so sub-agents pick up current state, not the stale checkpoint. +func TestCreatePodInvalidatesForkSnapshot(t *testing.T) { + tests := []struct { + name string + rt *fakeRuntime + spec meta.VMSpec + }{ + { + name: "run mode", + rt: &fakeRuntime{runVM: &vm.VM{ID: "vmid-main", Name: "vk-ns-demo-0"}}, + spec: meta.VMSpec{VMName: "vk-ns-demo-0", Image: "registry.example/cocoon/ubuntu:24.04", Mode: "run", OS: "linux"}, + }, + { + name: "clone mode", + rt: &fakeRuntime{snapshots: map[string]*vm.Snapshot{"snapshot-repo": {Name: "snapshot-repo"}}}, + spec: meta.VMSpec{VMName: "vk-ns-demo-0", Image: "snapshot-repo:latest", Mode: "clone"}, + }, } - if len(rt.snapshotRemoveCalls) != 1 || rt.snapshotRemoveCalls[0] != "fork-vk-ns-demo-0" { - t.Fatalf("SnapshotRemoveIfExists calls = %v, want [fork-vk-ns-demo-0]", rt.snapshotRemoveCalls) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := newTestProvider(t) + p.Runtime = tt.rt + + pod := newPodWithSpec(tt.spec) + if err := p.CreatePod(t.Context(), pod); err != nil { + t.Fatalf("create: %v", err) + } + if len(tt.rt.snapshotRemoveCalls) != 1 || tt.rt.snapshotRemoveCalls[0] != "fork-vk-ns-demo-0" { + t.Fatalf("SnapshotRemoveIfExists calls = %v, want [fork-vk-ns-demo-0]", tt.rt.snapshotRemoveCalls) + } + }) } } @@ -1265,7 +1278,7 @@ func TestStartupReconcileOrphanDestroyRemovesUnmatchedVM(t *testing.T) { p := newTestProvider(t) p.NodeName = "cocoon-pool" p.Runtime = rt - p.Clientset = fake.NewSimpleClientset() // no pods + p.Clientset = fake.NewSimpleClientset() p.OrphanPolicy = provider.OrphanDestroy if err := p.StartupReconcile(t.Context()); err != nil { @@ -2026,7 +2039,6 @@ 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") diff --git a/provider/cocoon/delete.go b/provider/cocoon/delete.go index a6c9b9c..24dd20b 100644 --- a/provider/cocoon/delete.go +++ b/provider/cocoon/delete.go @@ -21,12 +21,19 @@ func (p *Provider) DeletePod(ctx context.Context, pod *corev1.Pod) error { return err } spec := meta.ParseVMSpec(pod) + // A seat release keeps the local snapshot as the same-node warm-wake cache; resolveWakeSource still gates it on the :hibernate tag. + keepSnapshots := meta.ReadKeepSnapshotOnDelete(pod) v := p.vmForPod(pod.Namespace, pod.Name) if v == nil { - p.removeLocalSnapshots(ctx, spec.VMName) + reason := "no_vm" + if keepSnapshots { + reason = "seat_release" + } else { + p.removeLocalSnapshots(ctx, spec.VMName) + } p.forgetPod(pod.Namespace, pod.Name) - metrics.PodLifecycleTotal.WithLabelValues("delete", "skipped", "no_vm").Inc() + metrics.PodLifecycleTotal.WithLabelValues("delete", "skipped", reason).Inc() return nil } @@ -39,7 +46,9 @@ func (p *Provider) DeletePod(ctx context.Context, pod *corev1.Pod) error { return fmt.Errorf("remove vm %s: %w", v.ID, err) } - p.removeLocalSnapshots(ctx, v.Name) + if !keepSnapshots { + p.removeLocalSnapshots(ctx, v.Name) + } p.forgetPod(pod.Namespace, pod.Name) pod.Status.Phase = corev1.PodSucceeded @@ -48,7 +57,6 @@ func (p *Provider) DeletePod(ctx context.Context, pod *corev1.Pod) error { return nil } -// removeLocalSnapshots drops the clone source and its fork snapshot so a later restore cannot prefer stale local state over the registry tag. func (p *Provider) removeLocalSnapshots(ctx context.Context, vmName string) { if vmName == "" { return diff --git a/provider/cocoon/delete_test.go b/provider/cocoon/delete_test.go index 2f2eb9c..b94ee0f 100644 --- a/provider/cocoon/delete_test.go +++ b/provider/cocoon/delete_test.go @@ -1,6 +1,7 @@ package cocoon import ( + "slices" "strings" "testing" @@ -9,31 +10,60 @@ import ( "github.com/cocoonstack/vk-cocoon/vm" ) -// TestDeletePodForgottenVMRemovesLocalSnapshots locks the GC fix: deleting a pod whose VM was already forgotten must still remove its local snapshots. -func TestDeletePodForgottenVMRemovesLocalSnapshots(t *testing.T) { - rt := &fakeRuntime{} - p := newTestProvider(t) - p.Runtime = rt +// TestDeletePodSnapshotRetention locks the delete GC table: a plain delete removes the local snapshots, a seat release keeps them as the warm-wake cache. +func TestDeletePodSnapshotRetention(t *testing.T) { + tests := []struct { + name string + track *vm.VM + keep bool + wantRemovedID string + wantSnapshots []string + }{ + { + name: "forgotten vm removes snapshots", + wantSnapshots: []string{"vk-ns-demo-0", forkSnapshotName("vk-ns-demo-0")}, + }, + { + name: "seat release keeps snapshots", + keep: true, + }, + { + name: "seat release with live vm removes only the vm", + track: &vm.VM{ID: "live-vmid", Name: "vk-ns-demo-0", State: vm.StateRunning}, + keep: true, + wantRemovedID: "live-vmid", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rt := &fakeRuntime{} + p := newTestProvider(t) + p.Runtime = rt - // Pod is never tracked → vmForPod returns nil (VM already forgotten). - pod := newPodWithSpec(meta.VMSpec{VMName: "vk-ns-demo-0"}) + pod := newPodWithSpec(meta.VMSpec{VMName: "vk-ns-demo-0", Mode: "clone"}) + if tt.keep { + meta.MarkKeepSnapshotOnDelete(pod) + } + if tt.track != nil { + p.trackPod(pod, tt.track) + } - if err := p.DeletePod(t.Context(), pod); err != nil { - t.Fatalf("DeletePod: %v", err) - } + if err := p.DeletePod(t.Context(), pod); err != nil { + t.Fatalf("DeletePod: %v", err) + } - removed := map[string]bool{} - for _, name := range rt.snapshotRemoveCalls { - removed[name] = true - } - if !removed["vk-ns-demo-0"] || !removed[forkSnapshotName("vk-ns-demo-0")] || len(removed) != 2 { - t.Errorf("snapshotRemoveCalls = %v, want exactly [vk-ns-demo-0 %s]", rt.snapshotRemoveCalls, forkSnapshotName("vk-ns-demo-0")) - } - if rt.removedID != "" { - t.Errorf("forgotten-VM delete must not call Runtime.Remove, got %q", rt.removedID) - } - if rt.snapshotSaveCount != 0 { - t.Errorf("forgotten-VM delete must not save a snapshot, got %d", rt.snapshotSaveCount) + if rt.removedID != tt.wantRemovedID { + t.Errorf("removedID = %q, want %q", rt.removedID, tt.wantRemovedID) + } + got := slices.Sorted(slices.Values(rt.snapshotRemoveCalls)) + want := slices.Sorted(slices.Values(tt.wantSnapshots)) + if !slices.Equal(got, want) { + t.Errorf("snapshotRemoveCalls = %v, want %v", rt.snapshotRemoveCalls, tt.wantSnapshots) + } + if rt.snapshotSaveCount != 0 { + t.Errorf("delete must not save a snapshot, got %d", rt.snapshotSaveCount) + } + }) } } diff --git a/provider/cocoon/restore_test.go b/provider/cocoon/restore_test.go index 2bd3728..419f3a6 100644 --- a/provider/cocoon/restore_test.go +++ b/provider/cocoon/restore_test.go @@ -56,6 +56,9 @@ func TestBringUpVMRestoreFromHibernate(t *testing.T) { if tc.wantNICs && (rt.cloned.NICs == nil || *rt.cloned.NICs != 1) { t.Errorf("CH+Windows restore must clone with --nics 1; got %v", rt.cloned.NICs) } + if len(rt.snapshotRemoveCalls) != 0 { + t.Errorf("restore must keep the fork snapshot, removed %v", rt.snapshotRemoveCalls) + } }) } }