From d86671dbd0c3fade6ecabba602254fe5cbb81b24 Mon Sep 17 00:00:00 2001 From: CMGS Date: Thu, 16 Jul 2026 14:21:45 +0800 Subject: [PATCH] fix(clone): replace removed --on-demand with --restore-mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cocoon#127 removed the --on-demand flag in favor of --restore-mode copy|ondemand|mmap, so every non-Windows CH clone (normal, fork, from-dir, hibernation wake) fails at argv parse against current cocoon. CloneOptions.OnDemand becomes a RestoreMode enum. The node-wide default comes from VK_RESTORE_MODE (invalid values abort startup) and defaults to ondemand — the same restore semantics shipped today, so the compatibility fix carries no behavior change. Windows stays on copy (lazy restore stalls DHCP boot) and firecracker never receives the flag. copy emits no flag at all, keeping the argv valid on cocoon builds predating --restore-mode. mmap is opt-in only: it needs a CH build with mmap restore support and fails clones on any other build. --- docs/configuration.md | 1 + main.go | 9 ++++ provider/cocoon/create.go | 47 +++++++++++---------- provider/cocoon/create_test.go | 4 +- provider/cocoon/provider.go | 2 + provider/cocoon/update.go | 14 +++---- provider/cocoon/update_test.go | 17 ++++---- vm/cocoon_cli.go | 7 ++-- vm/cocoon_cli_test.go | 77 ++++++++++++++++++++++++++++------ vm/runtime.go | 46 +++++++++++++------- 10 files changed, 154 insertions(+), 70 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 66d2aad..a9a9021 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -13,6 +13,7 @@ systemd unit reads them from `/etc/cocoon/vk-cocoon.env`. | `VK_LEASES_PATH` | `/var/lib/cocoon/net/leases.json` | cocoon-net JSON lease file. | | `VK_COCOON_BIN` | `/usr/local/bin/cocoon` | Path to the cocoon CLI binary. | | `VK_ORPHAN_POLICY` | `destroy` | `destroy` (auto-clean), `alert`, or `keep`. | +| `VK_RESTORE_MODE` | `ondemand` | Guest-memory restore mode for Cloud Hypervisor clones: `copy`, `ondemand`, or `mmap`. Windows VMs always use `copy` (lazy restore stalls DHCP boot); Firecracker has no restore mode. `mmap` shares page cache across clones of one snapshot — the fastest fan-out — but requires a Cloud Hypervisor build with mmap restore support (cocoonstack/cloud-hypervisor `dev`); on other CH builds clones fail, so it is opt-in. Invalid values abort startup. | | `VK_NODE_IP` | auto-detected | Override the virtual node's InternalIP address (first non-loopback IPv4 used otherwise). | | `VK_NODE_POOL` | `default` | Cocoon pool label stamped onto the registered node. | | `VK_PROVIDER_ID` | unset | Cloud-provider ProviderID for the virtual node (e.g. `gce:////`). Prevents cloud node lifecycle controllers from deleting the virtual node. | diff --git a/main.go b/main.go index 496428e..1905327 100644 --- a/main.go +++ b/main.go @@ -51,6 +51,7 @@ const ( defaultNodeName = "cocoon-pool" defaultMetricsAddr = ":9091" defaultOrphanPolicy = string(provider.OrphanDestroy) + defaultRestoreMode = string(vm.RestoreOnDemand) defaultTLSCert = "/etc/cocoon/vk/tls/vk-kubelet.crt" defaultTLSKey = "/etc/cocoon/vk/tls/vk-kubelet.key" @@ -77,6 +78,7 @@ func main() { leasesPath := commonk8s.EnvOrDefault("VK_LEASES_PATH", network.DefaultLeasesPath) cocoonBin := commonk8s.EnvOrDefault("VK_COCOON_BIN", "") orphanPolicy := commonk8s.EnvOrDefault("VK_ORPHAN_POLICY", defaultOrphanPolicy) + restoreMode := commonk8s.EnvOrDefault("VK_RESTORE_MODE", defaultRestoreMode) nodeIP := commonk8s.EnvOrDefault("VK_NODE_IP", "") nodePool := commonk8s.EnvOrDefault("VK_NODE_POOL", meta.DefaultNodePool) providerID := os.Getenv("VK_PROVIDER_ID") @@ -124,6 +126,7 @@ func main() { leasesPath: leasesPath, cocoonBin: cocoonBin, orphanPolicy: orphanPolicy, + restoreMode: restoreMode, clientset: clientset, recorder: recorder, }) @@ -219,6 +222,7 @@ type buildOpts struct { leasesPath string cocoonBin string orphanPolicy string + restoreMode string clientset kubernetes.Interface recorder record.EventRecorder } @@ -235,6 +239,10 @@ func buildRegistry(opts buildOpts) (oci.Registry, error) { func buildProvider(ctx context.Context, opts buildOpts) (*cocoon.Provider, error) { logger := log.WithFunc("buildProvider") + restoreMode, err := vm.ParseRestoreMode(opts.restoreMode) + if err != nil { + return nil, fmt.Errorf("parse VK_RESTORE_MODE: %w", err) + } registry, err := buildRegistry(opts) if err != nil { return nil, fmt.Errorf("construct registry client: %w", err) @@ -261,6 +269,7 @@ func buildProvider(ctx context.Context, opts buildOpts) (*cocoon.Provider, error p.GuestSAC = &sac.Dialer{} p.Probes = probes.NewManager(ctx) p.OrphanPolicy = provider.OrphanPolicy(strings.ToLower(opts.orphanPolicy)) + p.RestoreMode = restoreMode return p, nil } diff --git a/provider/cocoon/create.go b/provider/cocoon/create.go index 2de4845..7e44790 100644 --- a/provider/cocoon/create.go +++ b/provider/cocoon/create.go @@ -167,12 +167,12 @@ func (p *Provider) bringUpVM(ctx context.Context, pod *corev1.Pod, spec meta.VMS return nil, "", fmt.Errorf("annotation %s is incompatible with fork-from %q", meta.AnnotationCloneFromDir, spec.ForkFrom) } v, err := p.Runtime.Clone(ctx, vm.CloneOptions{ - FromDir: fromDir, - To: spec.VMName, - Network: spec.Network, - Backend: backend, - NoDirectIO: noDirectIO, - OnDemand: useOnDemandClone(spec.OS), + FromDir: fromDir, + To: spec.VMName, + Network: spec.Network, + Backend: backend, + NoDirectIO: noDirectIO, + RestoreMode: restoreModeFor(p.RestoreMode, spec.OS), }) if err != nil { metrics.CloneFromDirTotal.WithLabelValues("failed").Inc() @@ -187,12 +187,12 @@ func (p *Provider) bringUpVM(ctx context.Context, pod *corev1.Pod, spec meta.VMS return nil, "", err } v, err := p.Runtime.Clone(ctx, vm.CloneOptions{ - From: cloneFrom, - To: spec.VMName, - Network: spec.Network, - Backend: backend, - NoDirectIO: noDirectIO, - OnDemand: useOnDemandClone(spec.OS), + From: cloneFrom, + To: spec.VMName, + Network: spec.Network, + Backend: backend, + NoDirectIO: noDirectIO, + RestoreMode: restoreModeFor(p.RestoreMode, spec.OS), }) if err != nil { return nil, "", fmt.Errorf("clone vm %s from %s: %w", spec.VMName, cloneFrom, err) @@ -254,13 +254,13 @@ func (p *Provider) bringUpVM(ctx context.Context, pod *corev1.Pod, spec meta.VMS } v, err := p.Runtime.Clone(ctx, vm.CloneOptions{ - From: local, - To: spec.VMName, - Network: spec.Network, - Backend: backend, - NoDirectIO: noDirectIO, - Pull: srcImage != "", - OnDemand: useOnDemandClone(spec.OS), + From: local, + To: spec.VMName, + Network: spec.Network, + Backend: backend, + NoDirectIO: noDirectIO, + Pull: srcImage != "", + RestoreMode: restoreModeFor(p.RestoreMode, spec.OS), }) if err != nil { return nil, "", fmt.Errorf("clone vm %s from %s: %w", spec.VMName, local, err) @@ -444,9 +444,12 @@ func parseCloneFromDirAnnotation(pod *corev1.Pod) (string, error) { return raw, nil } -// useOnDemandClone is off for Windows: UFFD lazy paging stalls DHCP boot. -func useOnDemandClone(os string) bool { - return os != string(cocoonv1.OSWindows) +// Windows forces copy: lazy memory restore stalls DHCP boot. +func restoreModeFor(mode vm.RestoreMode, os string) vm.RestoreMode { + if os == string(cocoonv1.OSWindows) { + return vm.RestoreCopy + } + return mode } // isClonedBoot reports whether bringUpVM took a clone path. spec.Mode alone diff --git a/provider/cocoon/create_test.go b/provider/cocoon/create_test.go index da255ce..e53a4b3 100644 --- a/provider/cocoon/create_test.go +++ b/provider/cocoon/create_test.go @@ -318,8 +318,8 @@ func TestCreatePodCloneFromDirAnnotationDispatches(t *testing.T) { if rt.cloned.From != "" { t.Errorf("clone From = %q, want empty when FromDir is set", rt.cloned.From) } - if !rt.cloned.OnDemand { - t.Errorf("OnDemand should be true on clone-from-dir path") + if rt.cloned.RestoreMode != vm.RestoreOnDemand { + t.Errorf("RestoreMode = %q, want %q on clone-from-dir path", rt.cloned.RestoreMode, vm.RestoreOnDemand) } if rt.snapshotSaveCount != 0 || len(rt.ensuredImages) != 0 { t.Errorf("from-dir path should bypass snapshot/ensure: save=%d images=%v", diff --git a/provider/cocoon/provider.go b/provider/cocoon/provider.go index 62e26b8..1a92c62 100644 --- a/provider/cocoon/provider.go +++ b/provider/cocoon/provider.go @@ -80,6 +80,7 @@ type Provider struct { NodeName string OrphanPolicy provider.OrphanPolicy + RestoreMode vm.RestoreMode Clientset kubernetes.Interface Runtime vm.Runtime @@ -133,6 +134,7 @@ func NewProvider() *Provider { lifecycleCtx: lifecycleCtx, lifecycleStop: lifecycleStop, OrphanPolicy: provider.OrphanDestroy, + RestoreMode: vm.RestoreOnDemand, Pinger: network.NopPinger{}, pods: map[string]*corev1.Pod{}, vmsByPod: map[string]*vm.VM{}, diff --git a/provider/cocoon/update.go b/provider/cocoon/update.go index 9e5b91d..c57f88a 100644 --- a/provider/cocoon/update.go +++ b/provider/cocoon/update.go @@ -202,13 +202,13 @@ func (p *Provider) cloneFromHibernate(ctx context.Context, spec meta.VMSpec, sou } } opts := vm.CloneOptions{ - From: sourceName, - To: spec.VMName, - Network: spec.Network, - Backend: spec.Backend, - NoDirectIO: spec.NoDirectIO, - OnDemand: useOnDemandClone(spec.OS), - Pull: snapshot != nil && snapshot.Image != "", + From: sourceName, + To: spec.VMName, + Network: spec.Network, + Backend: spec.Backend, + NoDirectIO: spec.NoDirectIO, + RestoreMode: restoreModeFor(p.RestoreMode, spec.OS), + Pull: snapshot != nil && snapshot.Image != "", } if shouldDropNICBeforeHibernate(spec) { opts.NICs = ptr.To(1) diff --git a/provider/cocoon/update_test.go b/provider/cocoon/update_test.go index 7380d82..61b515c 100644 --- a/provider/cocoon/update_test.go +++ b/provider/cocoon/update_test.go @@ -15,21 +15,22 @@ import ( "github.com/cocoonstack/vk-cocoon/vm" ) -func TestUseOnDemandClone(t *testing.T) { +func TestRestoreModeFor(t *testing.T) { cases := []struct { name string os string - want bool + mode vm.RestoreMode + want vm.RestoreMode }{ - {"linux", string(cocoonv1.OSLinux), true}, - {"windows off", string(cocoonv1.OSWindows), false}, - {"android counts as non-windows", string(cocoonv1.OSAndroid), true}, - {"empty OS defaults to on", "", true}, + {"linux gets configured mode", string(cocoonv1.OSLinux), vm.RestoreMmap, vm.RestoreMmap}, + {"windows forced to copy", string(cocoonv1.OSWindows), vm.RestoreMmap, vm.RestoreCopy}, + {"android counts as non-windows", string(cocoonv1.OSAndroid), vm.RestoreOnDemand, vm.RestoreOnDemand}, + {"empty OS gets configured mode", "", vm.RestoreOnDemand, vm.RestoreOnDemand}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if got := useOnDemandClone(tc.os); got != tc.want { - t.Errorf("useOnDemandClone(%q) = %v, want %v", tc.os, got, tc.want) + if got := restoreModeFor(tc.mode, tc.os); got != tc.want { + t.Errorf("restoreModeFor(%q, %q) = %q, want %q", tc.mode, tc.os, got, tc.want) } }) } diff --git a/vm/cocoon_cli.go b/vm/cocoon_cli.go index 82ccf43..ee61dde 100644 --- a/vm/cocoon_cli.go +++ b/vm/cocoon_cli.go @@ -421,10 +421,9 @@ func buildCloneArgs(opts CloneOptions) []string { if opts.Pull || opts.FromDir != "" { args = append(args, "--pull") } - if opts.OnDemand && opts.Backend != BackendFirecracker { - // UFFD lazy memory restore is CH-only; skipping on FC keeps the - // same CloneOptions usable for both backends. - args = append(args, "--on-demand") + // copy emits no flag, keeping the argv valid on cocoon builds predating --restore-mode. + if opts.RestoreMode != "" && opts.RestoreMode != RestoreCopy && opts.Backend != BackendFirecracker { + args = append(args, "--restore-mode", string(opts.RestoreMode)) } if opts.NICs != nil && opts.Backend != BackendFirecracker { args = append(args, "--nics", strconv.Itoa(*opts.NICs)) diff --git a/vm/cocoon_cli_test.go b/vm/cocoon_cli_test.go index 392e450..b47ee3b 100644 --- a/vm/cocoon_cli_test.go +++ b/vm/cocoon_cli_test.go @@ -1,6 +1,7 @@ package vm import ( + "cmp" "errors" "reflect" "testing" @@ -72,8 +73,8 @@ func TestBuildCloneArgs(t *testing.T) { want: []string{"vm", "clone", "--output", "json", "--name", "vm-a", "snap-a"}, }, { - name: "firecracker clone strips on-demand", - opts: CloneOptions{From: "snap-a", To: "vm-b", Backend: "firecracker", OnDemand: true}, + name: "firecracker clone strips restore-mode", + opts: CloneOptions{From: "snap-a", To: "vm-b", Backend: "firecracker", RestoreMode: RestoreOnDemand}, want: []string{"vm", "clone", "--output", "json", "--name", "vm-b", "snap-a"}, }, { @@ -92,9 +93,24 @@ func TestBuildCloneArgs(t *testing.T) { want: []string{"vm", "clone", "--output", "json", "--name", "vm-e", "--pull", "snap-a"}, }, { - name: "on-demand appended on cloud-hypervisor", - opts: CloneOptions{From: "snap-a", To: "vm-od", Backend: "cloud-hypervisor", OnDemand: true}, - want: []string{"vm", "clone", "--output", "json", "--name", "vm-od", "--on-demand", "snap-a"}, + name: "restore-mode ondemand appended on cloud-hypervisor", + opts: CloneOptions{From: "snap-a", To: "vm-od", Backend: "cloud-hypervisor", RestoreMode: RestoreOnDemand}, + want: []string{"vm", "clone", "--output", "json", "--name", "vm-od", "--restore-mode", "ondemand", "snap-a"}, + }, + { + name: "restore-mode mmap appended on cloud-hypervisor", + opts: CloneOptions{From: "snap-a", To: "vm-mm", Backend: "cloud-hypervisor", RestoreMode: RestoreMmap}, + want: []string{"vm", "clone", "--output", "json", "--name", "vm-mm", "--restore-mode", "mmap", "snap-a"}, + }, + { + name: "restore-mode copy emits no flag", + opts: CloneOptions{From: "snap-a", To: "vm-cp", Backend: "cloud-hypervisor", RestoreMode: RestoreCopy}, + want: []string{"vm", "clone", "--output", "json", "--name", "vm-cp", "snap-a"}, + }, + { + name: "empty backend treated as cloud-hypervisor for restore-mode", + opts: CloneOptions{From: "snap-a", To: "vm-eb", RestoreMode: RestoreOnDemand}, + want: []string{"vm", "clone", "--output", "json", "--name", "vm-eb", "--restore-mode", "ondemand", "snap-a"}, }, { name: "from-dir replaces positional and forces --pull", @@ -102,13 +118,13 @@ func TestBuildCloneArgs(t *testing.T) { want: []string{"vm", "clone", "--output", "json", "--name", "vm-f", "--pull", "--from-dir", "/var/lib/cocoon/snaps/foo"}, }, { - name: "from-dir on cloud-hypervisor with on-demand", - opts: CloneOptions{To: "vm-g", FromDir: "/snaps/bar", Backend: "cloud-hypervisor", OnDemand: true}, - want: []string{"vm", "clone", "--output", "json", "--name", "vm-g", "--pull", "--on-demand", "--from-dir", "/snaps/bar"}, + name: "from-dir on cloud-hypervisor with restore-mode", + opts: CloneOptions{To: "vm-g", FromDir: "/snaps/bar", Backend: "cloud-hypervisor", RestoreMode: RestoreOnDemand}, + want: []string{"vm", "clone", "--output", "json", "--name", "vm-g", "--pull", "--restore-mode", "ondemand", "--from-dir", "/snaps/bar"}, }, { - name: "from-dir on firecracker skips on-demand", - opts: CloneOptions{To: "vm-h", FromDir: "/snaps/fc", Backend: "firecracker", OnDemand: true}, + name: "from-dir on firecracker skips restore-mode", + opts: CloneOptions{To: "vm-h", FromDir: "/snaps/fc", Backend: "firecracker", RestoreMode: RestoreOnDemand}, want: []string{"vm", "clone", "--output", "json", "--name", "vm-h", "--pull", "--from-dir", "/snaps/fc"}, }, { @@ -132,9 +148,9 @@ func TestBuildCloneArgs(t *testing.T) { want: []string{"vm", "clone", "--output", "json", "--name", "vm-l", "--nics", "0", "snap-a"}, }, { - name: "nics combines with on-demand on CH", - opts: CloneOptions{From: "snap-a", To: "vm-m", Backend: "cloud-hypervisor", OnDemand: true, NICs: ptr.To(2)}, - want: []string{"vm", "clone", "--output", "json", "--name", "vm-m", "--on-demand", "--nics", "2", "snap-a"}, + name: "nics combines with restore-mode on CH", + opts: CloneOptions{From: "snap-a", To: "vm-m", Backend: "cloud-hypervisor", RestoreMode: RestoreOnDemand, NICs: ptr.To(2)}, + want: []string{"vm", "clone", "--output", "json", "--name", "vm-m", "--restore-mode", "ondemand", "--nics", "2", "snap-a"}, }, { name: "firecracker clone strips --nics", @@ -152,6 +168,41 @@ func TestBuildCloneArgs(t *testing.T) { } } +func TestParseRestoreMode(t *testing.T) { + t.Parallel() + + cases := []struct { + in string + want RestoreMode + wantErr bool + }{ + {in: "copy", want: RestoreCopy}, + {in: "ondemand", want: RestoreOnDemand}, + {in: "mmap", want: RestoreMmap}, + {in: "MMAP", want: RestoreMmap}, + {in: " ondemand ", want: RestoreOnDemand}, + {in: "", wantErr: true}, + {in: "on-demand", wantErr: true}, + } + for _, tc := range cases { + t.Run(cmp.Or(tc.in, "empty"), func(t *testing.T) { + got, err := ParseRestoreMode(tc.in) + if tc.wantErr { + if err == nil { + t.Fatalf("ParseRestoreMode(%q) = %q, want error", tc.in, got) + } + return + } + if err != nil { + t.Fatalf("ParseRestoreMode(%q): %v", tc.in, err) + } + if got != tc.want { + t.Errorf("ParseRestoreMode(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + func TestBuildRunArgs(t *testing.T) { t.Parallel() diff --git a/vm/runtime.go b/vm/runtime.go index 8791930..b019179 100644 --- a/vm/runtime.go +++ b/vm/runtime.go @@ -3,11 +3,19 @@ package vm import ( "context" "errors" + "fmt" "io" + "strings" ) -// StateRunning is the state string cocoon reports for a live VM. -const StateRunning = "running" +const ( + // StateRunning is the state string cocoon reports for a live VM. + StateRunning = "running" + + RestoreCopy RestoreMode = "copy" + RestoreOnDemand RestoreMode = "ondemand" + RestoreMmap RestoreMode = "mmap" +) var ( // ErrVMNotFound signals the cocoon CLI has authoritatively reported the @@ -71,21 +79,31 @@ type Snapshot struct { Hypervisor string } +// RestoreMode maps to `cocoon vm clone --restore-mode`: how CH restores guest +// memory. RestoreMmap needs a CH build with mmap restore support. +type RestoreMode string + +// ParseRestoreMode validates a configured restore mode, normalizing case. +func ParseRestoreMode(s string) (RestoreMode, error) { + switch m := RestoreMode(strings.ToLower(strings.TrimSpace(s))); m { + case RestoreCopy, RestoreOnDemand, RestoreMmap: + return m, nil + default: + return "", fmt.Errorf("restore mode must be %s, %s or %s, got %q", + RestoreCopy, RestoreOnDemand, RestoreMmap, s) + } +} + // CloneOptions is the input to Runtime.Clone. Resource fields inherit // from the snapshot unless overridden. type CloneOptions struct { - From string - To string - Network string - Backend string - NoDirectIO bool - Pull bool - // OnDemand maps to `cocoon vm clone --on-demand`: CH loads guest - // memory lazily via userfaultfd instead of copying it upfront. - // Cuts clone wall time from ~500ms to ~50-100ms at the cost of - // per-page faults during the first seconds of guest execution. - // CH only; firecracker ignores the flag. - OnDemand bool + From string + To string + Network string + Backend string + NoDirectIO bool + Pull bool + RestoreMode RestoreMode // FromDir maps to `cocoon vm clone --from-dir`: when set, From is // ignored and --pull is forced (the dir holds snapshot data, not // base image layers).