diff --git a/Taskfile.yml b/Taskfile.yml index 330959c..cac28d2 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -219,6 +219,19 @@ tasks: set -eou pipefail echo "πŸ” Running golangci-lint with config: .golangci.yml" GOOS=linux golangci-lint run --config .golangci.yml ./... + # The tagged files, compiled. + # + # Neither the lint above nor `go test ./...` looks at them, so a rename + # that misses one is green here and fails in CI as `[build failed]`, + # after five minutes of setting up a VM β€” the slowest lane in the + # repository reporting the fastest kind of error there is. That happened. + # + # `go vet` and not golangci-lint with the tags: the break to catch is a + # build break, and turning the full linter loose on files it has never + # seen reports seven pre-existing style findings that have nothing to do + # with whoever is running this. + echo "πŸ” Type-checking the tagged tests" + GOOS=linux go vet -tags=integration,e2e ./... echo "βœ“ Linting passed" integration: @@ -253,10 +266,10 @@ tasks: trap 'rm -rf "${WORKDIR}"' EXIT echo "πŸ“¦ Verifying QEMU firmware in build output..." - if [ -d "{{.OUTPUT_DIR}}/share/spin-stack/qemu" ]; then - ls -la "{{.OUTPUT_DIR}}/share/spin-stack/qemu" + if [ -d "{{.OUTPUT_DIR}}/qemu" ]; then + ls -la "{{.OUTPUT_DIR}}/qemu" else - echo "πŸ‘Ή Error: {{.OUTPUT_DIR}}/share/spin-stack/qemu not found" + echo "πŸ‘Ή Error: {{.OUTPUT_DIR}}/qemu not found" exit 1 fi diff --git a/go.mod b/go.mod index 9b10dc2..3be742e 100644 --- a/go.mod +++ b/go.mod @@ -26,6 +26,7 @@ require ( github.com/opencontainers/image-spec v1.1.1 github.com/opencontainers/runc v1.5.1 github.com/opencontainers/runtime-spec v1.3.0 + github.com/spin-stack/spin-machine v0.0.0-20260908021448-4a006f3f2f33 github.com/stretchr/testify v1.11.1 github.com/vishvananda/netlink v1.3.1 github.com/vishvananda/netns v0.0.5 diff --git a/go.sum b/go.sum index 8ff5376..6d5fb27 100644 --- a/go.sum +++ b/go.sum @@ -173,6 +173,8 @@ github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0t github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= github.com/sirupsen/logrus v1.9.4 h1:TsZE7l11zFCLZnZ+teH4Umoq5BhEIfIzfRDZ1Uzql2w= github.com/sirupsen/logrus v1.9.4/go.mod h1:ftWc9WdOfJ0a92nsE2jF5u5ZwH8Bv2zdeOC42RjbV2g= +github.com/spin-stack/spin-machine v0.0.0-20260908021448-4a006f3f2f33 h1:7AZSlQGTyFxi6eoEW0CZ11JV+DX9azwya1W5wObOgxk= +github.com/spin-stack/spin-machine v0.0.0-20260908021448-4a006f3f2f33/go.mod h1:VbqCcwvAK1lcazIf6rSli6qLrgLU5f22CutMng1pRbc= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= diff --git a/hack/release b/hack/release index 61c1fb2..ecabdf6 100755 --- a/hack/release +++ b/hack/release @@ -44,7 +44,7 @@ mkdir -p "${RELEASE_DIR}/usr/share/spin-stack/config" echo "Copying artifacts to release structure..." artifacts=( - "${OUTPUT_DIR}/vmlinux:kernel:vmlinux" + "${OUTPUT_DIR}/kernel/vmlinux:kernel:vmlinux" "${OUTPUT_DIR}/spinbox-initrd:kernel:spinbox-initrd" "${OUTPUT_DIR}/containerd-shim-spinbox-v1:bin:containerd-shim-spinbox-v1" "${OUTPUT_DIR}/spinbox-commit:bin:spinbox-commit" @@ -77,16 +77,16 @@ for binary in qemu-system-x86_64 qemu-img; do fi done -if [ -d "${OUTPUT_DIR}/share/spin-stack/qemu" ]; then +if [ -d "${OUTPUT_DIR}/qemu" ]; then mkdir -p "${RELEASE_DIR}/usr/share/spin-stack/qemu" - cp -r "${OUTPUT_DIR}/share/spin-stack/qemu/"* "${RELEASE_DIR}/usr/share/spin-stack/qemu/" + cp -r "${OUTPUT_DIR}/qemu/"* "${RELEASE_DIR}/usr/share/spin-stack/qemu/" echo " ok: QEMU firmware files" else echo " warn: QEMU firmware files not found - skipping" fi # The firmware a q35 started with -nodefaults -nographic and a PVH kernel needs, which is -# what Dockerfile.qemu ships. vgabios-stdvga.bin is not on the list any more because no +# what a spin-machine release ships. vgabios-stdvga.bin is not on the list any more because no # display adapter is built or created; pvh.bin is, and it was missing from this check while # being the one file a direct kernel boot cannot start without β€” QEMU has no entry point # into a PVH ELF kernel otherwise, and it fails as a rom-open error that reads like a @@ -100,9 +100,9 @@ for firmware in bios.bin bios-256k.bin pvh.bin kvmvapic.bin efi-virtio.rom; do done if [ "${missing_qemu_firmware}" -ne 0 ]; then - if [ -d "${OUTPUT_DIR}/share/spin-stack/qemu" ]; then - echo "Contents of ${OUTPUT_DIR}/share/spin-stack/qemu:" - ls -la "${OUTPUT_DIR}/share/spin-stack/qemu" || true + if [ -d "${OUTPUT_DIR}/qemu" ]; then + echo "Contents of ${OUTPUT_DIR}/qemu:" + ls -la "${OUTPUT_DIR}/qemu" || true fi exit 1 fi diff --git a/hack/spin-machine b/hack/spin-machine index 68f0661..89c2f0d 100755 --- a/hack/spin-machine +++ b/hack/spin-machine @@ -51,9 +51,14 @@ want=( # place # -# Copies the wanted files into _output/ in the layout the rest of the build and -# hack/release expect: binaries in bin/, firmware in share/spin-stack/qemu/, the -# kernel at the top. +# Copies the wanted files into _output/ at the paths they already have. +# +# It used to rearrange them β€” binaries here, firmware under another prefix, the +# kernel moved to the top β€” which meant the same four files had one layout in the +# release, a second here, and a third wherever they were installed, with a +# translation between each pair and a discovery function on the Go side guessing +# which one it had been handed. The release layout is the layout now, everywhere, +# so this is a copy. place() { local root="$1" missing=0 for rel in "${want[@]}"; do @@ -63,11 +68,7 @@ place() { missing=1 continue fi - case "${rel}" in - bin/*) dest="${OUTPUT_DIR}/${rel}" ;; - qemu/*) dest="${OUTPUT_DIR}/share/spin-stack/${rel}" ;; - kernel/*) dest="${OUTPUT_DIR}/$(basename "${rel}")" ;; - esac + dest="${OUTPUT_DIR}/${rel}" mkdir -p "$(dirname "${dest}")" # Removed first: _output may hold files from when this repository built the # machine itself, and those were written by a container running as root. @@ -93,22 +94,15 @@ place() { # through to a download or, worse, reported success. It did both before this split. have_sibling() { local out="${SIBLING}/_output" - [ -f "${out}/bin/qemu-system-x86_64" ] && [ -f "${out}/vmlinux" ] + [ -f "${out}/bin/qemu-system-x86_64" ] && [ -f "${out}/kernel/vmlinux" ] } from_sibling() { - local out="${SIBLING}/_output" echo "Using the machine built in ${SIBLING}" - # A built tree, not an unpacked release: same files, different shape. - local staged - staged="$(mktemp -d)" - trap 'rm -rf "${staged}"' RETURN - mkdir -p "${staged}/bin" "${staged}/kernel" "${staged}/qemu" - cp "${out}/bin/qemu-system-x86_64" "${out}/bin/qemu-img" "${staged}/bin/" - cp "${out}/vmlinux" "${staged}/kernel/vmlinux" - cp "${out}/kernel-config" "${staged}/kernel/kernel-config" - cp "${out}"/share/spin-stack/qemu/* "${staged}/qemu/" - place "${staged}" + # A built tree and an unpacked release are now the same shape, so this is the + # same copy the release path does. It used to stage the files into a third + # layout first, because they were not. + place "${SIBLING}/_output" } from_release() { @@ -165,4 +159,4 @@ else from_release fi -echo "βœ“ machine ${VERSION}: qemu $("${OUTPUT_DIR}/bin/qemu-system-x86_64" --version | head -1 | awk '{print $4}'), kernel $(stat -c%s "${OUTPUT_DIR}/vmlinux") bytes" +echo "βœ“ machine ${VERSION}: qemu $("${OUTPUT_DIR}/bin/qemu-system-x86_64" --version | head -1 | awk '{print $4}'), kernel $(stat -c%s "${OUTPUT_DIR}/kernel/vmlinux") bytes" diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 4241a16..e358a17 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -43,15 +43,7 @@ func createTestConfigEnv(t *testing.T, baseDir string) testConfigEnv { t.Fatalf("failed to create log dir: %v", err) } - // Create dummy kernel and initrd files - kernelPath := filepath.Join(env.shareDir, "kernel", "spinbox-kernel-x86_64") - initrdPath := filepath.Join(env.shareDir, "kernel", "spinbox-initrd") - if err := os.WriteFile(kernelPath, []byte("dummy"), 0644); err != nil { - t.Fatalf("failed to create dummy kernel: %v", err) - } - if err := os.WriteFile(initrdPath, []byte("dummy"), 0644); err != nil { - t.Fatalf("failed to create dummy initrd: %v", err) - } + writeMachine(t, env.shareDir) // Create and write config cfg := DefaultConfig() @@ -159,15 +151,7 @@ func TestLoadFrom_ValidConfig(t *testing.T) { t.Fatal(err) } - // Create dummy kernel and initrd - kernelPath := filepath.Join(kernelDir, "spinbox-kernel-x86_64") - initrdPath := filepath.Join(kernelDir, "spinbox-initrd") - if err := os.WriteFile(kernelPath, []byte("dummy"), 0600); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(initrdPath, []byte("dummy"), 0600); err != nil { - t.Fatal(err) - } + writeMachine(t, shareDir) cfg := &Config{ Paths: PathsConfig{ @@ -333,15 +317,7 @@ func TestValidate_Comprehensive(t *testing.T) { t.Fatal(err) } - // Create dummy kernel and initrd - kernelPath := filepath.Join(kernelDir, "spinbox-kernel-x86_64") - initrdPath := filepath.Join(kernelDir, "spinbox-initrd") - if err := os.WriteFile(kernelPath, []byte("dummy"), 0600); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(initrdPath, []byte("dummy"), 0600); err != nil { - t.Fatal(err) - } + writeMachine(t, shareDir) cfg.Paths.ShareDir = shareDir cfg.Paths.StateDir = stateDir @@ -686,3 +662,30 @@ func TestReset(t *testing.T) { t.Fatal("test setup error: directories should be different") } } + +// writeMachine fills a share directory with a whole spin-machine release plus +// the initrd this repository builds, which together are what validatePaths +// requires. +// +// It writes all four release files and not just a kernel: validation opens the +// release rather than stating one path, because a host with three of the four is +// a host that cannot start a guest and should be told so once, by name, instead +// of finding out from whichever file something happened to ask for first. +func writeMachine(t *testing.T, shareDir string) { + t.Helper() + for _, f := range []string{ + "bin/qemu-system-x86_64", + "bin/qemu-img", + "kernel/vmlinux", + "kernel/spinbox-initrd", + "qemu/pvh.bin", + } { + p := filepath.Join(shareDir, f) + if err := os.MkdirAll(filepath.Dir(p), 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(f), 0o600); err != nil { + t.Fatal(err) + } + } +} diff --git a/internal/config/validation.go b/internal/config/validation.go index fdc7099..5ccee69 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -6,6 +6,7 @@ import ( "path/filepath" "time" + "github.com/spin-stack/spin-machine/machine" "golang.org/x/sys/unix" ) @@ -37,31 +38,22 @@ func (c *Config) validatePaths() error { return err } - // Check kernel and initrd exist. - // - // The two kernel names are repeated from paths.KernelPath rather than taken - // from it: that package imports this one for PathsConfig, so calling into it - // here is an import cycle. Two names in two places is a duplication with a - // reason, and it is the reason this comment exists β€” whoever adds a third - // name has to change both. - // - // "vmlinux" is what a spin-machine release installs; - // "spinbox-kernel-x86_64" is what installs from before the machine moved out - // of this repository carry. - kernelDir := filepath.Join(c.Paths.ShareDir, "kernel") - initrdPath := filepath.Join(kernelDir, "spinbox-initrd") - - kernelPath := "" - for _, name := range []string{"vmlinux", "spinbox-kernel-x86_64"} { - p := filepath.Join(kernelDir, name) - if _, err := os.Stat(p); err == nil { - kernelPath = p - break - } - } - if kernelPath == "" { - return fmt.Errorf("no guest kernel in %s (run 'task machine' to fetch the pinned one)", kernelDir) + // The machine β€” QEMU, the kernel and the firmware β€” checked as one thing, + // because that is what it is: a release with a part missing is not a machine + // with a gap in it, it is a host that cannot start a guest, and the failure + // is worth having here rather than at boot. + if _, err := machine.Open(c.Paths.ShareDir); err != nil { + return fmt.Errorf("%w (run 'task machine' to fetch the pinned release)", err) } + + // The initrd is not part of a release: what runs as PID 1 inside a guest is + // this repository's business. It is checked separately for the same reason it + // is built separately. + // The name is spelled here and in internal/paths rather than shared: that + // package imports this one for PathsConfig, so calling into it would be a + // cycle. One filename in two places, and this is the note for whoever changes + // it. + initrdPath := filepath.Join(c.Paths.ShareDir, "kernel", "spinbox-initrd") if _, err := os.Stat(initrdPath); err != nil { if os.IsNotExist(err) { return fmt.Errorf("initrd not found at %s (run 'task build:initrd')", initrdPath) diff --git a/internal/host/vm/qemu/devices.go b/internal/host/vm/qemu/devices.go index 631b2af..6472579 100644 --- a/internal/host/vm/qemu/devices.go +++ b/internal/host/vm/qemu/devices.go @@ -62,11 +62,17 @@ func (q *Instance) AddDisk(ctx context.Context, blockID, mountPath string, opts blockID = stableID } + format := mc.Format + if format == "" { + format = vm.DefaultDiskFormat + } + q.disks = append(q.disks, &DiskConfig{ Path: mountPath, Readonly: mc.Readonly, ID: blockID, Serial: mc.Serial, + Format: format, }) log.G(ctx).WithFields(log.Fields{ diff --git a/internal/host/vm/qemu/instance.go b/internal/host/vm/qemu/instance.go index 9474a6d..208fbfc 100644 --- a/internal/host/vm/qemu/instance.go +++ b/internal/host/vm/qemu/instance.go @@ -113,34 +113,36 @@ func validateResourceConfig(cfg *vm.VMResourceConfig) *vm.VMResourceConfig { return &result } +// findQemu and findKernel return the two files that come out of a spin-machine +// release. They open the release rather than stat a composed path: a release +// with a part missing should say which part, once, and not surface as whichever +// of the three files somebody happened to ask for first. func findQemu() (string, error) { cfg, err := config.Get() if err != nil { return "", fmt.Errorf("failed to get config: %w", err) } - - path := paths.QemuPath(cfg.Paths) - if _, err := os.Stat(path); err == nil { - return path, nil + rel, err := paths.Machine(cfg.Paths) + if err != nil { + return "", err } - return "", fmt.Errorf("qemu-system-x86_64 binary not found at %s", path) + return paths.QemuPath(cfg.Paths, rel), nil } -// findKernel returns the path to the kernel binary for QEMU func findKernel() (string, error) { cfg, err := config.Get() if err != nil { return "", fmt.Errorf("failed to get config: %w", err) } - - path := paths.KernelPath(cfg.Paths) - if _, err := os.Stat(path); err == nil { - return path, nil + rel, err := paths.Machine(cfg.Paths) + if err != nil { + return "", err } - return "", fmt.Errorf("kernel not found at %s (use SPINBOX_SHARE_DIR to override)", path) + return rel.Kernel(), nil } -// findInitrd returns the path to the initrd for QEMU +// findInitrd returns the path to the initrd for QEMU. It is not part of a +// release: what runs as PID 1 inside a guest is this repository's business. func findInitrd() (string, error) { cfg, err := config.Get() if err != nil { diff --git a/internal/host/vm/qemu/kernel_cmdline.go b/internal/host/vm/qemu/kernel_cmdline.go index 8071438..62847db 100644 --- a/internal/host/vm/qemu/kernel_cmdline.go +++ b/internal/host/vm/qemu/kernel_cmdline.go @@ -6,11 +6,27 @@ import ( "fmt" "strings" + "github.com/spin-stack/spin-machine/machine" + "github.com/spin-stack/spinbox/internal/host/vm" "github.com/spin-stack/spinbox/internal/vsock" ) -// KernelCmdlineConfig holds the configuration for building a kernel command line. +// The kernel command line, which is two things stuck together. +// +// Most of it is a statement about the hardware β€” that the PCI bus stops at 0, +// that the TSC can be trusted, that there is no timer to probe for β€” and none of +// that is this repository's to decide. It comes from machine.Cmdline, next to the +// kernel it was written for, and getting one of those wrong shows up as boot time +// rather than as an error. +// +// What is here is the rest: the contract with vminitd. Which vsock ports to +// answer on, how many disks to wait for, where the extras disk is, what address +// the guest has. The kernel ignores every one of them and passes them through to +// /proc/cmdline, which is exactly why they work β€” see system.FromCmdline. + +// KernelCmdlineConfig is what this repository adds to the machine's own command +// line. type KernelCmdlineConfig struct { // Console device (e.g., "ttyS0") Console string @@ -51,173 +67,59 @@ type KernelCmdlineConfig struct { // DefaultKernelCmdlineConfig returns a default configuration. func DefaultKernelCmdlineConfig() KernelCmdlineConfig { + d := machine.DefaultCmdline() return KernelCmdlineConfig{ - Console: "ttyS0", + Console: d.Console, VsockRPCPort: vsock.DefaultRPCPort, VsockStreamPort: vsock.DefaultStreamPort, - Quiet: true, - LogLevel: 3, + Quiet: d.Quiet, + LogLevel: d.LogLevel, } } // BuildKernelCmdline constructs the kernel command line from the configuration. func BuildKernelCmdline(cfg KernelCmdlineConfig) string { - var parts []string - - // Console. The profiling boot uses the same console as a production boot, and - // says nothing on it: the per-initcall lines are read from /dev/kmsg by vminitd - // (system.DumpKernelBootProfile), not scraped from the console, so there is no - // reason for them to be written twice. - // - // This used to route the console through virtio-console (hvc0) because the - // verbose stream over the emulated 8250 - a PIO VMEXIT per byte - inflated the - // timings being measured. It fixed the symptom and moved the cost: a console - // registers at the device_initcall phase, and registering it replays the whole - // printk ring into it, synchronously, inside that initcall. The profile then - // showed 24 ms in virtio_console_init, which was the measurement writing itself - // out - the largest single entry in a boot the same run reported as 137 ms, - // against the ~90 ms a normal boot takes. - // - // With loglevel=0 below, nothing reaches the console at all and the question - // does not arise; the ring buffer still records everything, which is all the - // profile needs. - console := cfg.Console - if console != "" { - parts = append(parts, fmt.Sprintf("console=%s", console)) + c := machine.Cmdline{ + Console: cfg.Console, + Quiet: cfg.Quiet, + LogLevel: cfg.LogLevel, + Init: "/sbin/vminitd", + InitArgs: buildInitArgs(cfg), + Extra: guestParams(cfg), } - - // Boot verbosity. Debug mode forces verbose output so initcall timings - // are visible; otherwise honor the configured quiet/loglevel. - quiet, loglevel := cfg.Quiet, cfg.LogLevel if cfg.Debug { - // Silent console, full ring buffer. loglevel is the *console* threshold; - // every message is still recorded for /dev/kmsg, which is where the - // profile is read from. - quiet, loglevel = false, 0 - } - if quiet { - parts = append(parts, "quiet") + // Silent console, full ring buffer, per-initcall timings and the initcall + // tracepoints. vminitd reads both afterwards; see + // system.DumpKernelBootProfile. + c = c.Profiling() + // The userspace half, and this one is ours: vminitd emits VMINITD_PROFILE + // lines for its own boot phases when it finds this marker. A separate + // token so the kernel ignores it and it still reaches /proc/cmdline. + c.Extra = append(c.Extra, "spin.profile") } - parts = append(parts, fmt.Sprintf("loglevel=%d", loglevel)) - - // Scan PCI bus 0 and stop. The mechanism is worth writing down because the first - // version of this comment guessed at it and was wrong. - // - // arch/x86/pci/mmconfig-shared.c takes the *last bus of the ECAM window* as the - // last bus worth probing: - // - // if (pcibios_last_bus < 0) - // list_for_each_entry(cfg, &pci_mmcfg_list, list) - // pcibios_last_bus = cfg->end_bus; - // - // and a q35 advertises `ECAM [mem 0xb0000000-0xbfffffff] for domain 0000 - // [bus 00-ff]`, so that is 255. pci_subsys_init then calls - // pcibios_fixup_peer_bridges(), which walks buses 0..255 and reads the vendor ID - // at 32 devfns on each looking for a peer host bridge: 8192 config reads, every - // one a VM exit, to discover that there is nothing behind a bus this VM never had. - // - // Setting it on the command line wins because the parser runs before MMCONFIG - // init, so the `< 0` test above no longer fires. Measured here, full device set, - // kernel-to-init (VMINITD_READY pid1-entry): 87.6 / 85.0 ms without, 51.1 / 51.8 - // with, and pci_subsys_init 30 ms -> 1. `pci=lastbus=255` behaves exactly like no - // flag at all, which is the check that this is the mechanism and not a - // coincidence. - // - // **It is a constraint, not just a flag.** A device behind a root port would be on - // bus 1 and would simply not exist for the guest - no error, no warning, an absent - // disk. Everything here is placed by hand at a fixed slot on bus 0 - // (qemu_command.go, 0x02 through 0x1e); whoever changes that has to change this. - // - // Open: the CI runner does not pay this at all (pci_subsys_init under 39 us there, - // against 30 ms here) and the flag buys nothing on it. Same kernel, same QEMU, so - // something about that host stops the ECAM window from setting last_bus - most - // likely pci_mmcfg_reject_broken() rejecting it over the E820 map. Unexplained, - // and harmless: where the loop does not run, this changes nothing. - // A VM that deals in templates carries a PCIe root port, and a root port's - // devices live on bus 1: stopping at bus 0 would hide the very slot the - // restore hot-plugs into. Scanning one extra bus costs about 40 config - // accesses, against the 8192 that scanning all 256 costs - and a template VM - // pays even that once, since the boot it pays it in is the one being frozen. - parts = append(parts, "pci=lastbus=0") + return c.String() +} - // Systemd options - parts = append(parts, +// guestParams is everything vminitd reads out of /proc/cmdline. +func guestParams(cfg KernelCmdlineConfig) []string { + // systemd is not PID 1 in this guest β€” vminitd is β€” so these two do nothing + // here. They are kept because a guest that does run systemd, which the base + // image can, should not print a boot status nobody reads onto the console the + // kernel log shares. + parts := []string{ "systemd.show_status=0", "systemd.log_level=warning", - ) - - // Panic behavior - parts = append(parts, "panic=1") - - // Network naming - parts = append(parts, "net.ifnames=0", "biosdevname=0") - - // Cgroup v2 - parts = append(parts, - "systemd.unified_cgroup_hierarchy=1", - "cgroup_no_v1=all", - ) - - // Disable tickless kernel (reduces overhead for short-lived VMs) - parts = append(parts, "nohz=off") - - // Boot-speed tuning for a KVM guest: - // no_timer_check - skip the boot-time timer IRQ delivery probe - // tsc=reliable - trust the TSC and skip the clocksource watchdog - // rcupdate.rcu_expedited=1 - expedite RCU grace periods during boot - // - // pci=lastbus is set once, above, because how far the scan goes depends on - // whether this VM has a root port; it used to be appended here as well, so - // every guest booted with the flag twice. - parts = append(parts, - "no_timer_check", - "tsc=reliable", - "rcupdate.rcu_expedited=1", - ) - - // Boot profiling: print per-initcall timings to the console log. - // - // log_buf_len enlarges the printk ring buffer for the profiling boot, and it is - // the one part of this that is not about the console: vminitd reads the ring - // after boot, so everything initcall_debug emits - two lines per initcall, plus - // the verbose ACPI/PCI dumps - has to still be in it. The default 256 KiB - // (CONFIG_LOG_BUF_SHIFT=18) overflows under that load and silently drops the - // earliest entries, which are exactly the early core/subsys initcalls the - // profile exists to see. 4 MiB holds the whole boot. - if cfg.Debug { - parts = append(parts, "initcall_debug", "printk.time=1", "log_buf_len=4M") - // The initcall tracepoints, which are the only source for where a level - // *boundary* falls: initcall_debug times each call and says nothing about - // the time between them, and that time is the larger half of boot. The - // events have been compiled in all along (CONFIG_EVENT_TRACING=y), so this - // costs a cmdline token and a ring; vminitd reads /sys/kernel/tracing/trace - // after boot, the same way it reads /dev/kmsg, and for the same reason - - // nothing is printed while the thing being measured is running. - parts = append(parts, "trace_event=initcall:*", "trace_buf_size=4M") - // Userspace companion to initcall_debug: vminitd emits VMINITD_PROFILE - // lines for its boot phases when this marker is present (see - // system.BootProfiler). Kept as a separate token so the kernel ignores - // it and it reaches /proc/cmdline for vminitd to read. - parts = append(parts, "spin.profile") } - // Network configuration if netParam := buildNetworkParam(cfg.Network); netParam != "" { parts = append(parts, netParam) } - // Extras disk index for guest to locate the extras block device parts = append(parts, fmt.Sprintf("spin.disks=%d", cfg.DiskCount)) - if cfg.ExtrasDiskIndex != nil { parts = append(parts, fmt.Sprintf("spin.extras_disk=%d", *cfg.ExtrasDiskIndex)) } - - // Init command with vsock args - initArgs := buildInitArgs(cfg) - parts = append(parts, fmt.Sprintf("init=/sbin/vminitd -- %s", formatInitArgs(initArgs))) - - return strings.Join(parts, " ") + return parts } // buildNetworkParam builds the ip= kernel parameter for network configuration. diff --git a/internal/host/vm/qemu/qemu_command.go b/internal/host/vm/qemu/qemu_command.go deleted file mode 100644 index 9308801..0000000 --- a/internal/host/vm/qemu/qemu_command.go +++ /dev/null @@ -1,402 +0,0 @@ -package qemu - -import ( - "fmt" - "strings" -) - -// Fixed PCI slot assignments on the q35 root complex (pcie.0). -// -// Every virtio device sits directly on bus 0 - there are no PCIe root ports - -// which is what lets the kernel cmdline carry pci=lastbus=0 and skip scanning -// buses 1-255 (see kernel_cmdline.go). Pinning each device to a slot instead of -// letting QEMU auto-assign makes guest enumeration order deterministic: it no -// longer depends on the order of builder calls, so reordering code here cannot -// silently renumber devices inside the guest. -// -// The q35 machine owns both ends of the bus: 0x00 is the host bridge and 0x1f -// the ICH9 LPC/SATA/SMBus function block. 0x01 is left free (q35 convention -// places VGA there; we run -nodefaults with no display). -// Every device is on bus 0, and that is load-bearing beyond tidiness: the kernel -// is booted with `pci=lastbus=0` (BuildKernelCmdline), which stops the PCI scan -// after bus 0 and saves about 34 ms of every boot. A device placed behind a root -// port would be on bus 1 and would not exist for the guest. -const ( - pciSlotVsock = 0x02 - pciSlotRNG = 0x03 - // 0x04 is free: it held the boot-profiling virtio-serial until the profile - // stopped needing a console at all. - - pciSlotDiskBase = 0x05 - pciSlotDiskMax = 0x0f - - pciSlotNICBase = 0x10 - pciSlotNICMax = 0x1e -) - -// maxDisks and maxNICs bound the fixed slot ranges above. Exceeding either is a -// configuration error, caught before the command line is built. -const ( - maxDisks = pciSlotDiskMax - pciSlotDiskBase + 1 - maxNICs = pciSlotNICMax - pciSlotNICBase + 1 -) - -// virtioModern forces virtio 1.0 (modern-only) on a PCI virtio device. -// -// disable-legacy=on drops the legacy I/O BAR and the transitional device ID, so -// the guest skips the legacy probe path entirely. Every kernel we boot is -// virtio 1.0 capable, so the transitional mode QEMU would otherwise negotiate -// buys nothing. Note this was measured neutral for boot time (time-to-PID1 is -// unchanged within run-to-run noise); the win is a smaller device surface, not -// speed. -const virtioModern = "disable-legacy=on" - -// memoryBackendID names the RAM object when guest memory is file-backed. The -// machine references it by id, and migration matches RAM blocks by name across -// save and restore, so it must be identical on both sides. -const memoryBackendID = "pc.ram" - -// qemuCommandBuilder constructs QEMU command-line arguments using a fluent builder pattern. -// This provides type safety, validation, and clearer intent compared to raw string building. -// -// Example usage: -// -// cmd := newQemuCommandBuilder(). -// setBIOSPath("/usr/share/qemu"). -// setMachine("q35", "accel=kvm", "kernel-irqchip=on"). -// setCPU("host", "migratable=on"). -// setSMP(2, 4). -// setMemory(512, 0, 0). -// setKernel("/boot/vmlinuz"). -// build() -type qemuCommandBuilder struct { - args []string -} - -// newQemuCommandBuilder creates a new QEMU command builder. -func newQemuCommandBuilder() *qemuCommandBuilder { - return &qemuCommandBuilder{ - args: make([]string, 0, 64), // Pre-allocate for typical command size - } -} - -// setBIOSPath sets the BIOS/firmware directory path (-L option). -func (b *qemuCommandBuilder) setBIOSPath(path string) *qemuCommandBuilder { - b.args = append(b.args, "-L", path) - return b -} - -// setNoDefaults disables all default devices (-nodefaults). -// This prevents QEMU from creating default NIC (e1000e), VGA, serial, etc. -// All required devices must be explicitly added. -func (b *qemuCommandBuilder) setNoDefaults() *qemuCommandBuilder { - b.args = append(b.args, "-nodefaults") - return b -} - -// setSandbox enables QEMU's seccomp sandbox (-sandbox option). -// -// The binary is built with --enable-seccomp, so all four restrictions apply: -// - obsolete=deny block obsolete syscalls -// - elevateprivileges=deny block setuid/setgid family; QEMU never drops into -// another user here (the shim starts it with the identity it keeps) -// - spawn=deny block fork/exec; nothing is spawned - TAP arrives -// as a file descriptor, and slirp (which uses helpers) is not compiled in -// - resourcecontrol=deny block sched_setaffinity and friends. vCPU pinning, -// if ever needed, must then be done from the host side rather than from -// inside QEMU. -func (b *qemuCommandBuilder) setSandbox() *qemuCommandBuilder { - b.args = append(b.args, - "-sandbox", "on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny") - return b -} - -// addGlobal sets a global device property (-global option). -// Example: addGlobal("ICH9-LPC.disable_s3=1") -func (b *qemuCommandBuilder) addGlobal(property string) *qemuCommandBuilder { - b.args = append(b.args, "-global", property) - return b -} - -// setMachine sets the machine type and options (-machine option). -// Example: setMachine("q35", "accel=kvm", "kernel-irqchip=on") -func (b *qemuCommandBuilder) setMachine(machineType string, options ...string) *qemuCommandBuilder { - // Empty options are dropped rather than joined, so a caller can pass one - // conditionally without building the string itself - QEMU rejects the - // trailing comma an empty element would leave behind. - kept := make([]string, 0, len(options)) - for _, o := range options { - if o != "" { - kept = append(kept, o) - } - } - - value := machineType - if len(kept) > 0 { - value = fmt.Sprintf("%s,%s", machineType, strings.Join(kept, ",")) - } - b.args = append(b.args, "-machine", value) - return b -} - -// setCPU sets the CPU model and features (-cpu option). -// Example: setCPU("host", "migratable=on") -func (b *qemuCommandBuilder) setCPU(model string, features ...string) *qemuCommandBuilder { - value := model - if len(features) > 0 { - value = fmt.Sprintf("%s,%s", model, strings.Join(features, ",")) - } - b.args = append(b.args, "-cpu", value) - return b -} - -// setSMP sets CPU topology (-smp option). -// -// Parameters: -// - bootCPUs: Initial number of vCPUs -// - maxCPUs: Maximum vCPUs for hotplug (0 means same as bootCPUs, no hotplug) -// -// Example: setSMP(2, 4) produces "-smp 2,maxcpus=4" -func (b *qemuCommandBuilder) setSMP(bootCPUs, maxCPUs int) *qemuCommandBuilder { - b.args = append(b.args, "-smp", smpArg(bootCPUs, maxCPUs)) - return b -} - -// smpArg formats the -smp value. Shared with MachineIdentity, which has to -// spell the machine the same way the command line does. -func smpArg(bootCPUs, maxCPUs int) string { - if maxCPUs > 0 && maxCPUs != bootCPUs { - return fmt.Sprintf("%d,maxcpus=%d", bootCPUs, maxCPUs) - } - return fmt.Sprintf("%d", bootCPUs) -} - -// setMemory sets memory configuration (-m option). -// -// Parameters: -// - memoryMB: Initial memory in megabytes -// - slots: Number of memory hotplug slots (0 means no hotplug) -// - maxMemoryMB: Maximum memory in megabytes (0 means same as memoryMB) -// -// Examples: -// - setMemory(512, 0, 0) produces "-m 512" -// - setMemory(512, 4, 2048) produces "-m 512,slots=4,maxmem=2048M" -func (b *qemuCommandBuilder) setMemory(memoryMB int, slots int, maxMemoryMB int) *qemuCommandBuilder { - b.args = append(b.args, "-m", memoryArg(memoryMB, slots, maxMemoryMB)) - return b -} - -// memoryArg formats the -m value. Shared with MachineIdentity; see smpArg. -func memoryArg(memoryMB, slots, maxMemoryMB int) string { - if slots > 0 && maxMemoryMB > memoryMB { - return fmt.Sprintf("%d,slots=%d,maxmem=%dM", memoryMB, slots, maxMemoryMB) - } - return fmt.Sprintf("%d", memoryMB) -} - -// setMemoryBackendFile backs guest RAM with a file rather than anonymous -// memory, and points the machine at it. -// -// share=on is what a template needs: the pages it dirties must land in the file -// the restores will read. A restoring VM passes share=off, which maps the same -// file MAP_PRIVATE - it sees the template's memory, and anything it writes stays -// private to it. That is the whole copy-on-write story, and it is why one -// template file can serve many VMs without being copied. -func (b *qemuCommandBuilder) setMemoryBackendFile(path string, memoryMB int, share bool) *qemuCommandBuilder { - shareVal := "off" - if share { - shareVal = "on" - } - b.args = append(b.args, "-object", - fmt.Sprintf("memory-backend-file,id=%s,size=%dM,mem-path=%s,share=%s", - memoryBackendID, memoryMB, path, shareVal)) - return b -} - -// machineMemoryBackend returns the machine option that points at the file-backed -// RAM object, or an empty string when guest memory is anonymous. setMachine -// drops empty options, so this composes without branching at the call site. -func machineMemoryBackend(memoryFilePath string) string { - if memoryFilePath == "" { - return "" - } - return "memory-backend=" + memoryBackendID -} - -// setIncomingDefer starts QEMU with no machine state, waiting to be told where -// to load it from (migrate-incoming). Without "defer" the URI has to be known at -// exec time, which would mean re-execing QEMU to change templates. -func (b *qemuCommandBuilder) setIncomingDefer() *qemuCommandBuilder { - b.args = append(b.args, "-incoming", "defer") - return b -} - -// setKernel sets the kernel image path (-kernel option). -func (b *qemuCommandBuilder) setKernel(path string) *qemuCommandBuilder { - b.args = append(b.args, "-kernel", path) - return b -} - -// setInitrd sets the initial ramdisk path (-initrd option). -func (b *qemuCommandBuilder) setInitrd(path string) *qemuCommandBuilder { - b.args = append(b.args, "-initrd", path) - return b -} - -// setKernelArgs sets kernel command line arguments (-append option). -func (b *qemuCommandBuilder) setKernelArgs(cmdline string) *qemuCommandBuilder { - b.args = append(b.args, "-append", cmdline) - return b -} - -// setNoGraphic disables graphical output (-nographic option). -func (b *qemuCommandBuilder) setNoGraphic() *qemuCommandBuilder { - b.args = append(b.args, "-nographic") - return b -} - -// setSerial sets serial port configuration (-serial option). -// Example: setSerial("file:/tmp/console.log") -func (b *qemuCommandBuilder) setSerial(config string) *qemuCommandBuilder { - b.args = append(b.args, "-serial", config) - return b -} - -// addDevice adds a device (-device option). -// Example: addDevice("virtio-rng-pci") -// Example: addDevice("vhost-vsock-pci,guest-cid=3") -func (b *qemuCommandBuilder) addDevice(device string) *qemuCommandBuilder { - b.args = append(b.args, "-device", device) - return b -} - -// addVsockDevice adds a vhost-vsock device for guest communication. -func (b *qemuCommandBuilder) addVsockDevice(guestCID int) *qemuCommandBuilder { - return b.addDevice(fmt.Sprintf("vhost-vsock-pci,guest-cid=%d,%s,addr=0x%x", - guestCID, virtioModern, pciSlotVsock)) -} - -// addVMGenID adds the VM Generation ID device, whose value QEMU randomises for -// every VM it starts. -// -// It exists for restores. Every VM restored from a template starts with the -// template's memory, which includes the state of the guest's random pool: two -// containers restored from the same template would otherwise produce the same -// "random" bytes until something reseeded them. The guest watches this device -// (CONFIG_VMGENID) and reseeds when the value it sees differs from the one in -// the memory it woke up with, which is exactly the case here. -func (b *qemuCommandBuilder) addVMGenID() *qemuCommandBuilder { - return b.addDevice("vmgenid,guid=auto") -} - -// addVirtioRNG adds a virtio-rng device for entropy. -func (b *qemuCommandBuilder) addVirtioRNG() *qemuCommandBuilder { - return b.addDevice(fmt.Sprintf("virtio-rng-pci,%s,addr=0x%x", virtioModern, pciSlotRNG)) -} - -// setQMP sets QMP socket configuration (-qmp option). -// Example: setQMP("unix:/tmp/qmp.sock,server=on,wait=off") -func (b *qemuCommandBuilder) setQMP(config string) *qemuCommandBuilder { - b.args = append(b.args, "-qmp", config) - return b -} - -// setQMPUnixSocket sets QMP to use a Unix socket. -func (b *qemuCommandBuilder) setQMPUnixSocket(socketPath string) *qemuCommandBuilder { - return b.setQMP(fmt.Sprintf("unix:%s,server=on,wait=off", socketPath)) -} - -// addDisk adds a disk drive with virtio-blk device. -// -// Parameters: -// - index: 0-based disk index, mapped to a fixed PCI slot (pciSlotDiskBase+index) -// - id: Drive identifier (e.g., "blk0") -// - disk: Disk configuration -// -// This generates both -drive and -device options: -// -// -drive file=,if=none,id=,format=[,readonly=on|,file.locking=on] -// -device virtio-blk-pci,drive=,disable-legacy=on,addr=0x -// -// Format is auto-detected from file extension: -// - .vmdk β†’ vmdk -// - .qcow2 β†’ qcow2 -// - default β†’ raw -// -// Writable drives (the rwlayer) pin file.locking=on so QEMU holds an image -// lock on the backing file. The snapshotter's commit gate takes an OFD F_WRLCK -// on rwlayer.img to detect a running container; that only works if QEMU locks -// the same inode. Setting it explicitly avoids depending on QEMU's -// locking=auto default, which a shared-storage setup might globally disable. -func (b *qemuCommandBuilder) addDisk(index int, id string, disk *DiskConfig) *qemuCommandBuilder { - // Detect format based on file extension - format := "raw" - if strings.HasSuffix(disk.Path, ".vmdk") { - format = "vmdk" - } else if strings.HasSuffix(disk.Path, ".qcow2") { - format = "qcow2" - } - - driveArgs := fmt.Sprintf("file=%s,if=none,id=%s,format=%s", disk.Path, id, format) - if disk.Readonly { - driveArgs += ",readonly=on" - } else { - driveArgs += ",file.locking=on" - } - b.args = append(b.args, "-drive", driveArgs) - - deviceArgs := fmt.Sprintf("virtio-blk-pci,drive=%s,%s,addr=0x%x", - id, virtioModern, pciSlotDiskBase+index) - // Expose a stable serial so the guest can resolve this device via - // /sys/block//serial instead of relying on PCI enumeration order. - if disk.Serial != "" { - deviceArgs += fmt.Sprintf(",serial=%s", disk.Serial) - } - b.args = append(b.args, "-device", deviceArgs) - return b -} - -// NICConfig represents a network interface configuration. -type NICConfig struct { - TapFD int // File descriptor number (3+ for ExtraFiles) - MAC string // MAC address -} - -// addNIC adds a network interface using TAP device via file descriptor. -// -// Parameters: -// - index: 0-based NIC index, mapped to a fixed PCI slot (pciSlotNICBase+index) -// - id: Network identifier (e.g., "net0") -// - nic: NIC configuration -// -// This generates both -netdev and -device options: -// -// -netdev tap,id=,fd= -// -device virtio-net-pci,netdev=,mac=,romfile=,disable-legacy=on,addr=0x -// -// Note: romfile= disables option ROM loading (e.g., efi-virtio.rom) to avoid firmware dependency. -func (b *qemuCommandBuilder) addNIC(index int, id string, nic NICConfig) *qemuCommandBuilder { - b.args = append(b.args, - "-netdev", fmt.Sprintf("tap,id=%s,fd=%d", id, nic.TapFD), - "-device", fmt.Sprintf("virtio-net-pci,netdev=%s,mac=%s,romfile=,%s,addr=0x%x", - id, nic.MAC, virtioModern, pciSlotNICBase+index), - ) - return b -} - -// build returns the complete command-line arguments. -func (b *qemuCommandBuilder) build() []string { - return b.args -} - -// setMachineShape applies the four arguments that decide the shape of the -// machine, already formatted by machineShape. -// -// It takes them ready-made rather than formatting them itself because -// MachineIdentity needs the same four strings before there is a command line to -// read them from, and the two must never disagree: a restore loads state into a -// machine that has to be the same shape, and nothing checks that at runtime. -func (b *qemuCommandBuilder) setMachineShape(machine, cpu, smp, memory string) *qemuCommandBuilder { - b.args = append(b.args, "-machine", machine, "-cpu", cpu, "-smp", smp, "-m", memory) - return b -} diff --git a/internal/host/vm/qemu/qemu_command_test.go b/internal/host/vm/qemu/qemu_command_test.go deleted file mode 100644 index c085b73..0000000 --- a/internal/host/vm/qemu/qemu_command_test.go +++ /dev/null @@ -1,621 +0,0 @@ -package qemu - -import ( - "testing" -) - -func TestNewQemuCommandBuilder(t *testing.T) { - b := newQemuCommandBuilder() - if b == nil { - t.Fatal("newQemuCommandBuilder() returned nil") - } - args := b.build() - if len(args) != 0 { - t.Errorf("new builder should have empty args, got %v", args) - } -} - -func TestSetBIOSPath(t *testing.T) { - args := newQemuCommandBuilder(). - setBIOSPath("/usr/share/qemu"). - build() - - want := []string{"-L", "/usr/share/qemu"} - assertArgs(t, args, want) -} - -func TestSetNoDefaults(t *testing.T) { - args := newQemuCommandBuilder(). - setNoDefaults(). - build() - - want := []string{"-nodefaults"} - assertArgs(t, args, want) -} - -func TestSetMachine(t *testing.T) { - tests := []struct { - name string - machineType string - options []string - want []string - }{ - { - name: "machine type only", - machineType: "q35", - options: nil, - want: []string{"-machine", "q35"}, - }, - { - name: "machine with one option", - machineType: "q35", - options: []string{"accel=kvm"}, - want: []string{"-machine", "q35,accel=kvm"}, - }, - { - name: "machine with multiple options", - machineType: "q35", - options: []string{"accel=kvm", "kernel-irqchip=on", "hpet=off"}, - want: []string{"-machine", "q35,accel=kvm,kernel-irqchip=on,hpet=off"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - args := newQemuCommandBuilder(). - setMachine(tt.machineType, tt.options...). - build() - assertArgs(t, args, tt.want) - }) - } -} - -func TestSetCPU(t *testing.T) { - tests := []struct { - name string - model string - features []string - want []string - }{ - { - name: "model only", - model: "host", - features: nil, - want: []string{"-cpu", "host"}, - }, - { - name: "model with one feature", - model: "host", - features: []string{"migratable=on"}, - want: []string{"-cpu", "host,migratable=on"}, - }, - { - name: "model with multiple features", - model: "host", - features: []string{"migratable=on", "+vmx", "-svm"}, - want: []string{"-cpu", "host,migratable=on,+vmx,-svm"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - args := newQemuCommandBuilder(). - setCPU(tt.model, tt.features...). - build() - assertArgs(t, args, tt.want) - }) - } -} - -func TestSetSMP(t *testing.T) { - tests := []struct { - name string - bootCPUs int - maxCPUs int - want []string - }{ - { - name: "single CPU no hotplug", - bootCPUs: 1, - maxCPUs: 0, - want: []string{"-smp", "1"}, - }, - { - name: "multiple CPUs no hotplug", - bootCPUs: 4, - maxCPUs: 0, - want: []string{"-smp", "4"}, - }, - { - name: "same boot and max (no hotplug)", - bootCPUs: 2, - maxCPUs: 2, - want: []string{"-smp", "2"}, - }, - { - name: "with CPU hotplug", - bootCPUs: 2, - maxCPUs: 8, - want: []string{"-smp", "2,maxcpus=8"}, - }, - { - name: "boot 1 max 4", - bootCPUs: 1, - maxCPUs: 4, - want: []string{"-smp", "1,maxcpus=4"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - args := newQemuCommandBuilder(). - setSMP(tt.bootCPUs, tt.maxCPUs). - build() - assertArgs(t, args, tt.want) - }) - } -} - -func TestSetMemory(t *testing.T) { - tests := []struct { - name string - memoryMB int - slots int - maxMemoryMB int - want []string - }{ - { - name: "simple memory no hotplug", - memoryMB: 512, - slots: 0, - maxMemoryMB: 0, - want: []string{"-m", "512"}, - }, - { - name: "large memory no hotplug", - memoryMB: 4096, - slots: 0, - maxMemoryMB: 0, - want: []string{"-m", "4096"}, - }, - { - name: "with memory hotplug", - memoryMB: 512, - slots: 4, - maxMemoryMB: 2048, - want: []string{"-m", "512,slots=4,maxmem=2048M"}, - }, - { - name: "max equals initial (no hotplug)", - memoryMB: 1024, - slots: 4, - maxMemoryMB: 1024, - want: []string{"-m", "1024"}, - }, - { - name: "slots but no max (no hotplug)", - memoryMB: 512, - slots: 4, - maxMemoryMB: 0, - want: []string{"-m", "512"}, - }, - { - name: "max less than initial (no hotplug)", - memoryMB: 1024, - slots: 4, - maxMemoryMB: 512, - want: []string{"-m", "1024"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - args := newQemuCommandBuilder(). - setMemory(tt.memoryMB, tt.slots, tt.maxMemoryMB). - build() - assertArgs(t, args, tt.want) - }) - } -} - -func TestSetKernel(t *testing.T) { - args := newQemuCommandBuilder(). - setKernel("/boot/vmlinuz"). - build() - - want := []string{"-kernel", "/boot/vmlinuz"} - assertArgs(t, args, want) -} - -func TestSetInitrd(t *testing.T) { - args := newQemuCommandBuilder(). - setInitrd("/boot/initrd.img"). - build() - - want := []string{"-initrd", "/boot/initrd.img"} - assertArgs(t, args, want) -} - -func TestSetKernelArgs(t *testing.T) { - args := newQemuCommandBuilder(). - setKernelArgs("console=ttyS0 quiet"). - build() - - want := []string{"-append", "console=ttyS0 quiet"} - assertArgs(t, args, want) -} - -func TestSetNoGraphic(t *testing.T) { - args := newQemuCommandBuilder(). - setNoGraphic(). - build() - - want := []string{"-nographic"} - assertArgs(t, args, want) -} - -func TestSetSerial(t *testing.T) { - tests := []struct { - name string - config string - want []string - }{ - { - name: "file output", - config: "file:/tmp/console.log", - want: []string{"-serial", "file:/tmp/console.log"}, - }, - { - name: "stdio", - config: "stdio", - want: []string{"-serial", "stdio"}, - }, - { - name: "null", - config: "null", - want: []string{"-serial", "null"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - args := newQemuCommandBuilder(). - setSerial(tt.config). - build() - assertArgs(t, args, tt.want) - }) - } -} - -func TestAddDevice(t *testing.T) { - args := newQemuCommandBuilder(). - addDevice("virtio-rng-pci"). - build() - - want := []string{"-device", "virtio-rng-pci"} - assertArgs(t, args, want) -} - -func TestAddVsockDevice(t *testing.T) { - tests := []struct { - name string - guestCID int - want []string - }{ - { - name: "standard CID 3", - guestCID: 3, - want: []string{"-device", "vhost-vsock-pci,guest-cid=3,disable-legacy=on,addr=0x2"}, - }, - { - name: "custom CID", - guestCID: 42, - want: []string{"-device", "vhost-vsock-pci,guest-cid=42,disable-legacy=on,addr=0x2"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - args := newQemuCommandBuilder(). - addVsockDevice(tt.guestCID). - build() - assertArgs(t, args, tt.want) - }) - } -} - -func TestAddVirtioRNG(t *testing.T) { - args := newQemuCommandBuilder(). - addVirtioRNG(). - build() - - want := []string{"-device", "virtio-rng-pci,disable-legacy=on,addr=0x3"} - assertArgs(t, args, want) -} - -func TestSetQMP(t *testing.T) { - args := newQemuCommandBuilder(). - setQMP("unix:/tmp/qmp.sock,server=on,wait=off"). - build() - - want := []string{"-qmp", "unix:/tmp/qmp.sock,server=on,wait=off"} - assertArgs(t, args, want) -} - -func TestSetQMPUnixSocket(t *testing.T) { - args := newQemuCommandBuilder(). - setQMPUnixSocket("/tmp/qmp.sock"). - build() - - want := []string{"-qmp", "unix:/tmp/qmp.sock,server=on,wait=off"} - assertArgs(t, args, want) -} - -func TestAddDisk(t *testing.T) { - tests := []struct { - name string - index int - id string - disk *DiskConfig - want []string - }{ - { - name: "raw disk", - index: 0, - id: "blk0", - disk: &DiskConfig{ - Path: "/var/lib/vm/disk.raw", - Readonly: false, - }, - want: []string{ - "-drive", "file=/var/lib/vm/disk.raw,if=none,id=blk0,format=raw,file.locking=on", - "-device", "virtio-blk-pci,drive=blk0,disable-legacy=on,addr=0x5", - }, - }, - { - name: "raw disk readonly", - index: 0, - id: "blk0", - disk: &DiskConfig{ - Path: "/var/lib/vm/disk.raw", - Readonly: true, - }, - want: []string{ - "-drive", "file=/var/lib/vm/disk.raw,if=none,id=blk0,format=raw,readonly=on", - "-device", "virtio-blk-pci,drive=blk0,disable-legacy=on,addr=0x5", - }, - }, - { - name: "vmdk disk", - index: 1, - id: "blk1", - disk: &DiskConfig{ - Path: "/var/lib/vm/rootfs.vmdk", - Readonly: true, - }, - want: []string{ - "-drive", "file=/var/lib/vm/rootfs.vmdk,if=none,id=blk1,format=vmdk,readonly=on", - "-device", "virtio-blk-pci,drive=blk1,disable-legacy=on,addr=0x6", - }, - }, - { - name: "qcow2 disk", - index: 2, - id: "data", - disk: &DiskConfig{ - Path: "/var/lib/vm/data.qcow2", - Readonly: false, - }, - want: []string{ - "-drive", "file=/var/lib/vm/data.qcow2,if=none,id=data,format=qcow2,file.locking=on", - "-device", "virtio-blk-pci,drive=data,disable-legacy=on,addr=0x7", - }, - }, - { - name: "no extension defaults to raw", - index: 0, - id: "blk0", - disk: &DiskConfig{ - Path: "/dev/sda", - Readonly: false, - }, - want: []string{ - "-drive", "file=/dev/sda,if=none,id=blk0,format=raw,file.locking=on", - "-device", "virtio-blk-pci,drive=blk0,disable-legacy=on,addr=0x5", - }, - }, - { - name: "disk with serial", - index: 3, - id: "blk2", - disk: &DiskConfig{ - Path: "/var/lib/vm/layer.erofs", - Readonly: true, - Serial: "sbxblk2", - }, - want: []string{ - "-drive", "file=/var/lib/vm/layer.erofs,if=none,id=blk2,format=raw,readonly=on", - "-device", "virtio-blk-pci,drive=blk2,disable-legacy=on,addr=0x8,serial=sbxblk2", - }, - }, - { - // The writable rwlayer must pin file.locking=on so the snapshotter's - // OFD lock on rwlayer.img reliably gates commit of a running container. - name: "writable rwlayer pins file.locking", - index: 4, - id: "blk3", - disk: &DiskConfig{ - Path: "/var/lib/spinbox/rwlayer.img", - Readonly: false, - }, - want: []string{ - "-drive", "file=/var/lib/spinbox/rwlayer.img,if=none,id=blk3,format=raw,file.locking=on", - "-device", "virtio-blk-pci,drive=blk3,disable-legacy=on,addr=0x9", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - args := newQemuCommandBuilder(). - addDisk(tt.index, tt.id, tt.disk). - build() - assertArgs(t, args, tt.want) - }) - } -} - -func TestAddNIC(t *testing.T) { - tests := []struct { - name string - index int - id string - nic NICConfig - want []string - }{ - { - name: "basic NIC", - index: 0, - id: "net0", - nic: NICConfig{ - TapFD: 3, - MAC: "52:54:00:12:34:56", - }, - want: []string{ - "-netdev", "tap,id=net0,fd=3", - "-device", "virtio-net-pci,netdev=net0,mac=52:54:00:12:34:56,romfile=,disable-legacy=on,addr=0x10", - }, - }, - { - name: "second NIC", - index: 1, - id: "net1", - nic: NICConfig{ - TapFD: 4, - MAC: "52:54:00:12:34:57", - }, - want: []string{ - "-netdev", "tap,id=net1,fd=4", - "-device", "virtio-net-pci,netdev=net1,mac=52:54:00:12:34:57,romfile=,disable-legacy=on,addr=0x11", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - args := newQemuCommandBuilder(). - addNIC(tt.index, tt.id, tt.nic). - build() - assertArgs(t, args, tt.want) - }) - } -} - -func TestBuilderChaining(t *testing.T) { - args := newQemuCommandBuilder(). - setBIOSPath("/usr/share/qemu"). - setMachine("q35", "accel=kvm", "kernel-irqchip=on"). - setCPU("host", "migratable=on"). - setSMP(2, 4). - setMemory(512, 4, 2048). - setKernel("/boot/vmlinuz"). - setInitrd("/boot/initrd.img"). - setKernelArgs("console=ttyS0 quiet"). - setNoGraphic(). - setSerial("file:/tmp/console.log"). - addVsockDevice(3). - addVirtioRNG(). - setQMPUnixSocket("/tmp/qmp.sock"). - addDisk(0, "blk0", &DiskConfig{Path: "/var/lib/vm/disk.raw", Readonly: true}). - addNIC(0, "net0", NICConfig{TapFD: 3, MAC: "52:54:00:12:34:56"}). - build() - - // Verify essential components are present - expectedPairs := map[string]string{ - "-L": "/usr/share/qemu", - "-machine": "q35,accel=kvm,kernel-irqchip=on", - "-cpu": "host,migratable=on", - "-smp": "2,maxcpus=4", - "-m": "512,slots=4,maxmem=2048M", - "-kernel": "/boot/vmlinuz", - "-initrd": "/boot/initrd.img", - "-append": "console=ttyS0 quiet", - "-serial": "file:/tmp/console.log", - "-qmp": "unix:/tmp/qmp.sock,server=on,wait=off", - } - - for key, value := range expectedPairs { - found := false - for i := range len(args) - 1 { - if args[i] == key && args[i+1] == value { - found = true - break - } - } - if !found { - t.Errorf("expected %s %s in args, got %v", key, value, args) - } - } - - // Check -nographic is present (no value) - hasNographic := false - for _, arg := range args { - if arg == "-nographic" { - hasNographic = true - break - } - } - if !hasNographic { - t.Error("expected -nographic in args") - } -} - -func TestMultipleDevices(t *testing.T) { - args := newQemuCommandBuilder(). - addDisk(0, "blk0", &DiskConfig{Path: "/disk1.raw"}). - addDisk(1, "blk1", &DiskConfig{Path: "/disk2.vmdk", Readonly: true}). - addNIC(0, "net0", NICConfig{TapFD: 3, MAC: "52:54:00:00:00:01"}). - addNIC(1, "net1", NICConfig{TapFD: 4, MAC: "52:54:00:00:00:02"}). - build() - - // Count -drive and -device occurrences - driveCount := 0 - deviceCount := 0 - netdevCount := 0 - for _, arg := range args { - switch arg { - case "-drive": - driveCount++ - case "-device": - deviceCount++ - case "-netdev": - netdevCount++ - } - } - - if driveCount != 2 { - t.Errorf("expected 2 -drive, got %d", driveCount) - } - if deviceCount != 4 { // 2 for disks + 2 for NICs - t.Errorf("expected 4 -device, got %d", deviceCount) - } - if netdevCount != 2 { - t.Errorf("expected 2 -netdev, got %d", netdevCount) - } -} - -// assertArgs checks that the actual args exactly match expected args -func assertArgs(t *testing.T, actual, expected []string) { - t.Helper() - if len(actual) != len(expected) { - t.Errorf("args length mismatch: got %d, want %d\ngot: %v\nwant: %v", - len(actual), len(expected), actual, expected) - return - } - for i := range actual { - if actual[i] != expected[i] { - t.Errorf("args[%d] mismatch: got %q, want %q\nfull args: %v", - i, actual[i], expected[i], actual) - } - } -} diff --git a/internal/host/vm/qemu/qmp_client.go b/internal/host/vm/qemu/qmp_client.go index 5ae3306..0bf39c5 100644 --- a/internal/host/vm/qemu/qmp_client.go +++ b/internal/host/vm/qemu/qmp_client.go @@ -288,6 +288,21 @@ func (q *qmpClient) ObjectAdd(ctx context.Context, qomType, objID string, args m return err } +// QOMSet writes one property of one device in the machine's object model. +// +// It is how a device that is already there is asked to change, as against +// device_add and device_del, which are how a device arrives and leaves. Memory +// growth is the first thing here that works the first way: the virtio-mem device +// is on the command line from the start and its size is a property of it. +func (q *qmpClient) QOMSet(ctx context.Context, path, property string, value any) error { + _, err := q.execute(ctx, "qom-set", map[string]any{ + "path": path, + "property": property, + "value": value, + }) + return err +} + // ObjectDel removes a QEMU object. func (q *qmpClient) ObjectDel(ctx context.Context, objID string) error { _, err := q.execute(ctx, "object-del", map[string]any{ diff --git a/internal/host/vm/qemu/qmp_memory.go b/internal/host/vm/qemu/qmp_memory.go index 1167eca..655e0e9 100644 --- a/internal/host/vm/qemu/qmp_memory.go +++ b/internal/host/vm/qemu/qmp_memory.go @@ -25,111 +25,57 @@ func (q *qmpClient) QueryMemorySizeSummary(ctx context.Context) (*MemorySizeSumm return qmpQuery[*MemorySizeSummary](q, ctx, "query-memory-size-summary") } -// HotplugMemory adds memory to the VM using pc-dimm. -// slotID: memory slot index (0-7 based on -m slots=8) -// sizeBytes: memory size in bytes (must be 128MB aligned) -func (q *qmpClient) HotplugMemory(ctx context.Context, slotID int, sizeBytes int64) error { - // Validate 128MB alignment - const alignmentMB = 128 - const alignmentBytes = alignmentMB * 1024 * 1024 - if sizeBytes%alignmentBytes != 0 { - return fmt.Errorf("memory size must be %dMB aligned, got %d bytes", alignmentMB, sizeBytes) - } - - backendID := fmt.Sprintf("mem%d", slotID) - dimmID := fmt.Sprintf("dimm%d", slotID) - - // Query current state before adding - beforeSummary, err := q.QueryMemorySizeSummary(ctx) - if err != nil { - log.G(ctx).WithError(err).Warn("qemu: failed to query memory before hotplug") - } - - // Step 1: Create memory backend object - backendArgs := map[string]any{ - "size": sizeBytes, +// SetPluggedMemory asks the machine's virtio-mem device for a total amount of +// memory beyond the boot size, in bytes, and reports what the device says it has +// after the request. +// +// One call, one number, in both directions. It replaces adding and removing +// pc-dimm devices in fixed slots, which cost this package a backend object and a +// device per step, a slot table to say which of the eight were in use, LIFO +// ordering so that unplug removed the newest, an online RPC into the guest after +// every add and an offline RPC before every remove, and a rollback path for each +// of the four ways that could half-fail. None of it exists here: the guest +// onlines what arrives by itself (memhp_default_state=online), and shrinking is +// the same call with a smaller number. +// +// **The answer is advisory and the request is not a promise.** virtio-mem is a +// negotiation with the guest: it plugs memory in blocks as the guest accepts +// them, and on the way down it can only take back what the guest has released. +// Asking for less than the guest is using is not an error and does not fail β€” it +// simply does not arrive, which is why the size afterwards is read back rather +// than assumed. +func (q *qmpClient) SetPluggedMemory(ctx context.Context, sizeBytes int64) (int64, error) { + if sizeBytes < 0 { + return 0, fmt.Errorf("negative memory request: %d bytes", sizeBytes) } log.G(ctx).WithFields(log.Fields{ - fieldSlotID: slotID, - "size_bytes": sizeBytes, - "size_mb": sizeBytes / (1024 * 1024), - "backend_id": backendID, - }).Debug("qemu: creating memory backend") + "requested_bytes": sizeBytes, + "requested_mb": sizeBytes / (1024 * 1024), + }).Debug("qemu: asking virtio-mem for a new size") - if err := q.ObjectAdd(ctx, "memory-backend-ram", backendID, backendArgs); err != nil { - return fmt.Errorf("failed to create memory backend: %w", err) + if err := q.QOMSet(ctx, virtioMemQOMPath, "requested-size", sizeBytes); err != nil { + return 0, fmt.Errorf("requesting %d bytes from virtio-mem: %w", sizeBytes, err) } - // Step 2: Hotplug pc-dimm device - dimmArgs := map[string]any{ - "id": dimmID, - "memdev": backendID, + summary, err := q.QueryMemorySizeSummary(ctx) + if err != nil { + // The request went through; only the confirmation did not. Reporting the + // requested size here would be inventing a measurement, so the caller is + // told it does not know rather than told a number. + return 0, fmt.Errorf("reading back the memory size after requesting %d bytes: %w", sizeBytes, err) } log.G(ctx).WithFields(log.Fields{ - fieldSlotID: slotID, - "dimm_id": dimmID, - }).Debug("qemu: hotplugging memory device") + "requested_mb": sizeBytes / (1024 * 1024), + "plugged_mb": summary.PluggedMemory / (1024 * 1024), + "total_mb": (summary.BaseMemory + summary.PluggedMemory) / (1024 * 1024), + }).Info("qemu: virtio-mem size set") - if err := q.DeviceAdd(ctx, "pc-dimm", dimmArgs); err != nil { - // Cleanup backend on failure - if delErr := q.ObjectDel(ctx, backendID); delErr != nil { - log.G(ctx).WithError(delErr).Warn("qemu: failed to cleanup memory backend after device_add failure") - } - return fmt.Errorf("failed to hotplug memory device: %w", err) - } - - // Verify memory was added - if beforeSummary != nil { - afterSummary, err := q.QueryMemorySizeSummary(ctx) - if err == nil { - if afterSummary.BaseMemory+afterSummary.PluggedMemory <= beforeSummary.BaseMemory+beforeSummary.PluggedMemory { - log.G(ctx).WithFields(log.Fields{ - fieldSlotID: slotID, - "before_total": beforeSummary.BaseMemory + beforeSummary.PluggedMemory, - "after_total": afterSummary.BaseMemory + afterSummary.PluggedMemory, - "expected_size": sizeBytes, - }).Warn("qemu: device_add did not increase memory size") - return fmt.Errorf("device_add did not increase memory size") - } - log.G(ctx).WithFields(log.Fields{ - fieldSlotID: slotID, - "added_mb": sizeBytes / (1024 * 1024), - "total_mb": (afterSummary.BaseMemory + afterSummary.PluggedMemory) / (1024 * 1024), - "plugged_mb": afterSummary.PluggedMemory / (1024 * 1024), - }).Info("qemu: memory hotplug successful") - } - } - - return nil + return summary.PluggedMemory, nil } -// UnplugMemory removes memory from the VM. -// slotID: memory slot to remove -// Note: Memory hot-unplug requires guest kernel support (CONFIG_MEMORY_HOTREMOVE=y) -// and the memory must be offline in the guest before removal. -func (q *qmpClient) UnplugMemory(ctx context.Context, slotID int) error { - dimmID := fmt.Sprintf("dimm%d", slotID) - backendID := fmt.Sprintf("mem%d", slotID) - - log.G(ctx).WithFields(log.Fields{ - fieldSlotID: slotID, - "dimm_id": dimmID, - }).Debug("qemu: unplugging memory device") - - // Step 1: Remove device - if err := q.DeviceDelete(ctx, dimmID); err != nil { - return fmt.Errorf("failed to unplug memory device: %w", err) - } - - // Step 2: Remove backend object - // Note: QEMU may need time to complete device removal before backend deletion - // We'll attempt to delete the backend, but it's not critical if it fails - if err := q.ObjectDel(ctx, backendID); err != nil { - log.G(ctx).WithError(err).WithField("backend_id", backendID). - Warn("qemu: failed to delete memory backend (non-fatal)") - } - - return nil -} +// virtioMemQOMPath is where the machine puts its virtio-mem device. The id comes +// from machine.Spec.Args, which names it vmem0; QEMU exposes anything with an id +// under /machine/peripheral. +const virtioMemQOMPath = "/machine/peripheral/vmem0" diff --git a/internal/host/vm/qemu/qmp_memory_test.go b/internal/host/vm/qemu/qmp_memory_test.go deleted file mode 100644 index bbd15db..0000000 --- a/internal/host/vm/qemu/qmp_memory_test.go +++ /dev/null @@ -1,340 +0,0 @@ -//go:build linux - -package qemu - -import ( - "context" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -const qmpTestSocketPath = "/tmp/test-qemu-qmp.sock" - -// TestMemoryAlignmentValidation tests the 128MB alignment requirement. -// This is a unit test that doesn't require QEMU. -func TestMemoryAlignmentValidation(t *testing.T) { - tests := []struct { - name string - sizeBytes int64 - wantErr bool - errMsg string - }{ - { - name: "128MB aligned", - sizeBytes: 128 * 1024 * 1024, - wantErr: false, - }, - { - name: "256MB aligned", - sizeBytes: 256 * 1024 * 1024, - wantErr: false, - }, - { - name: "512MB aligned", - sizeBytes: 512 * 1024 * 1024, - wantErr: false, - }, - { - name: "1GB aligned", - sizeBytes: 1024 * 1024 * 1024, - wantErr: false, - }, - { - name: "64MB unaligned", - sizeBytes: 64 * 1024 * 1024, - wantErr: true, - errMsg: "128MB aligned", - }, - { - name: "100MB unaligned", - sizeBytes: 100 * 1024 * 1024, - wantErr: true, - errMsg: "128MB aligned", - }, - { - name: "200MB unaligned", - sizeBytes: 200 * 1024 * 1024, - wantErr: true, - errMsg: "128MB aligned", - }, - { - name: "1 byte unaligned", - sizeBytes: 1, - wantErr: true, - errMsg: "128MB aligned", - }, - } - - // Create a mock client that will fail on any actual QMP operation - // The alignment check happens before any QMP call - client := &qmpClient{} - client.closed.Store(true) // Mark as closed so it fails fast if we reach QMP calls - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := client.HotplugMemory(context.Background(), 0, tt.sizeBytes) - - if tt.wantErr { - require.Error(t, err) - assert.Contains(t, err.Error(), tt.errMsg) - } else if err != nil { - // For aligned sizes, we expect to fail at the QMP call (client closed) - // not at alignment validation - assert.NotContains(t, err.Error(), "aligned") - } - }) - } -} - -// TestQMPMemoryHotplug tests the memory hotplug functionality via QMP -// This is an integration test that requires a running QEMU VM -func TestQMPMemoryHotplug(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test in short mode") - } - - // Note: This test requires manual setup of a QEMU VM with QMP socket - // For automated testing, use the full VM integration tests - t.Skip("manual integration test - requires running QEMU VM") - - ctx := context.Background() - - // Connect to QMP (socket path from running VM) - qmp, _, err := newQMPClient(ctx, qmpTestSocketPath) - if err != nil { - t.Fatalf("failed to connect to QMP: %v", err) - } - defer qmp.Close() - - // Query initial memory state - initialSummary, err := qmp.QueryMemorySizeSummary(ctx) - if err != nil { - t.Fatalf("failed to query memory summary: %v", err) - } - - initialTotalMB := (initialSummary.BaseMemory + initialSummary.PluggedMemory) / (1024 * 1024) - t.Logf("Initial memory: base=%dMB, plugged=%dMB, total=%dMB", - initialSummary.BaseMemory/(1024*1024), - initialSummary.PluggedMemory/(1024*1024), - initialTotalMB) - - // Hotplug 128MB memory - const memoryToAdd = 128 * 1024 * 1024 // 128MB - slotID := 0 - - if err := qmp.HotplugMemory(ctx, slotID, memoryToAdd); err != nil { - t.Fatalf("failed to hotplug memory: %v", err) - } - - t.Logf("Hotplugged 128MB to slot %d", slotID) - - // Verify memory was added - afterSummary, err := qmp.QueryMemorySizeSummary(ctx) - if err != nil { - t.Fatalf("failed to query memory summary after hotplug: %v", err) - } - - afterTotalMB := (afterSummary.BaseMemory + afterSummary.PluggedMemory) / (1024 * 1024) - expectedTotalMB := initialTotalMB + 128 - - t.Logf("After hotplug: base=%dMB, plugged=%dMB, total=%dMB", - afterSummary.BaseMemory/(1024*1024), - afterSummary.PluggedMemory/(1024*1024), - afterTotalMB) - - if afterTotalMB != expectedTotalMB { - t.Errorf("expected %dMB total memory after hotplug, got %dMB", expectedTotalMB, afterTotalMB) - } - - // Query memory devices - devices, err := qmp.QueryMemoryDevices(ctx) - if err != nil { - t.Fatalf("failed to query memory devices: %v", err) - } - - t.Logf("Memory devices: %d", len(devices)) - for i, dev := range devices { - t.Logf(" Device %d: type=%s, data=%v", i, dev.Type, dev.Data) - } - - // Try to unplug the memory (may not work if memory is in use) - if err := qmp.UnplugMemory(ctx, slotID); err != nil { - t.Logf("Memory unplug failed (expected if memory in use): %v", err) - // Don't fail the test - memory hot-unplug may fail if pages are in use - } else { - t.Logf("Successfully unplugged memory from slot %d", slotID) - - // Verify memory was removed - finalSummary, err := qmp.QueryMemorySizeSummary(ctx) - if err != nil { - t.Fatalf("failed to query memory summary after unplug: %v", err) - } - - finalTotalMB := (finalSummary.BaseMemory + finalSummary.PluggedMemory) / (1024 * 1024) - if finalTotalMB != initialTotalMB { - t.Logf("Warning: expected %dMB after unplug, got %dMB", initialTotalMB, finalTotalMB) - } - } -} - -// TestQueryMemorySizeSummary tests the query-memory-size-summary command -func TestQueryMemorySizeSummary(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test in short mode") - } - - t.Skip("manual integration test - requires running QEMU VM") - - ctx := context.Background() - - qmp, _, err := newQMPClient(ctx, qmpTestSocketPath) - if err != nil { - t.Fatalf("failed to connect to QMP: %v", err) - } - defer qmp.Close() - - summary, err := qmp.QueryMemorySizeSummary(ctx) - if err != nil { - t.Fatalf("failed to query memory summary: %v", err) - } - - t.Logf("Memory summary:") - t.Logf(" Base memory: %d bytes (%d MB)", summary.BaseMemory, summary.BaseMemory/(1024*1024)) - t.Logf(" Plugged memory: %d bytes (%d MB)", summary.PluggedMemory, summary.PluggedMemory/(1024*1024)) - t.Logf(" Total: %d bytes (%d MB)", - summary.BaseMemory+summary.PluggedMemory, - (summary.BaseMemory+summary.PluggedMemory)/(1024*1024)) - - if summary.BaseMemory <= 0 { - t.Errorf("expected positive base memory, got %d", summary.BaseMemory) - } -} - -// TestQueryMemoryDevices tests the query-memory-devices command -func TestQueryMemoryDevices(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test in short mode") - } - - t.Skip("manual integration test - requires running QEMU VM") - - ctx := context.Background() - - qmp, _, err := newQMPClient(ctx, qmpTestSocketPath) - if err != nil { - t.Fatalf("failed to connect to QMP: %v", err) - } - defer qmp.Close() - - devices, err := qmp.QueryMemoryDevices(ctx) - if err != nil { - t.Fatalf("failed to query memory devices: %v", err) - } - - t.Logf("Found %d memory devices:", len(devices)) - for i, dev := range devices { - t.Logf(" Device %d:", i) - t.Logf(" Type: %s", dev.Type) - t.Logf(" Data: %v", dev.Data) - } -} - -// TestMemoryHotplugAlignment tests that memory size validation works -func TestMemoryHotplugAlignment(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test in short mode") - } - - t.Skip("manual integration test - requires running QEMU VM") - - ctx := context.Background() - - qmp, _, err := newQMPClient(ctx, qmpTestSocketPath) - if err != nil { - t.Fatalf("failed to connect to QMP: %v", err) - } - defer qmp.Close() - - // Test cases for memory alignment - testCases := []struct { - name string - sizeBytes int64 - shouldErr bool - }{ - { - name: "128MB aligned - valid", - sizeBytes: 128 * 1024 * 1024, - shouldErr: false, - }, - { - name: "256MB aligned - valid", - sizeBytes: 256 * 1024 * 1024, - shouldErr: false, - }, - { - name: "100MB unaligned - invalid", - sizeBytes: 100 * 1024 * 1024, - shouldErr: true, - }, - { - name: "64MB unaligned - invalid", - sizeBytes: 64 * 1024 * 1024, - shouldErr: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - err := qmp.HotplugMemory(ctx, 0, tc.sizeBytes) - if tc.shouldErr { - if err == nil { - t.Errorf("expected error for %dB memory, got nil", tc.sizeBytes) - } else { - t.Logf("Got expected error: %v", err) - } - } else { - if err != nil { - t.Errorf("expected no error for %dB memory, got: %v", tc.sizeBytes, err) - } - } - }) - } -} - -// TestObjectAddDel tests QMP object-add and object-del commands -func TestObjectAddDel(t *testing.T) { - if testing.Short() { - t.Skip("skipping integration test in short mode") - } - - t.Skip("manual integration test - requires running QEMU VM") - - ctx := context.Background() - - qmp, _, err := newQMPClient(ctx, qmpTestSocketPath) - if err != nil { - t.Fatalf("failed to connect to QMP: %v", err) - } - defer qmp.Close() - - // Add a memory backend - backendID := "test-mem-backend" - args := map[string]any{ - "size": int64(128 * 1024 * 1024), // 128MB - } - - if err := qmp.ObjectAdd(ctx, "memory-backend-ram", backendID, args); err != nil { - t.Fatalf("failed to add memory backend: %v", err) - } - - t.Logf("Added memory backend: %s", backendID) - - // Delete the memory backend - if err := qmp.ObjectDel(ctx, backendID); err != nil { - t.Fatalf("failed to delete memory backend: %v", err) - } - - t.Logf("Deleted memory backend: %s", backendID) -} diff --git a/internal/host/vm/qemu/snapshot_test.go b/internal/host/vm/qemu/snapshot_test.go index baedab7..e902a03 100644 --- a/internal/host/vm/qemu/snapshot_test.go +++ b/internal/host/vm/qemu/snapshot_test.go @@ -3,27 +3,47 @@ package qemu import ( + "path/filepath" "strings" "testing" + + "github.com/spin-stack/spin-machine/machine" ) // The command line is where the snapshot design is either right or wrong: the // template and the restore have to agree on the RAM object's name and disagree // on how it is mapped, and a VM that does neither must come out unchanged. +// +// The three arguments are built by the machine package; what is under test here +// is the decision this repository makes, which is which of the three cases a VM +// is in. It is two booleans derived from two paths β€” see (*Instance).spec β€” and +// getting one backwards is not an error at start-up. A template that mapped its +// RAM privately would freeze a file full of nothing; a restore that shared it +// would write into the template every later VM reads. func TestSnapshotCommandLine(t *testing.T) { t.Parallel() + dir := t.TempDir() build := func(memFile, restore string) string { - b := newQemuCommandBuilder(). - setMachine("q35", "accel=kvm", machineMemoryBackend(memFile)). - setMemory(512, 0, 0) - if memFile != "" { - b.setMemoryBackendFile(memFile, 512, restore == "") + s := machine.Spec{ + QEMU: filepath.Join(dir, "qemu"), + Kernel: filepath.Join(dir, "kernel"), + Firmware: dir, + BootCPUs: 1, + Memory: machine.Memory{ + SizeMB: 512, + File: memFile, + // The line from spec(): a template writes into the file and must + // share it, a restore maps the same file privately. + Shared: memFile != "" && restore == "", + }, + IncomingDefer: restore != "", } - if restore != "" { - b.setIncomingDefer() + args, err := s.Args() + if err != nil { + t.Fatalf("Args: %v", err) } - return strings.Join(b.build(), " ") + return strings.Join(args, " ") } t.Run("plain VM is untouched", func(t *testing.T) { @@ -35,9 +55,6 @@ func TestSnapshotCommandLine(t *testing.T) { if strings.Contains(got, "-incoming") { t.Errorf("plain VM should not wait for incoming state: %s", got) } - if !strings.Contains(got, "-machine q35,accel=kvm ") { - t.Errorf("machine option list should have no trailing comma: %s", got) - } }) t.Run("template shares its RAM file", func(t *testing.T) { @@ -45,7 +62,7 @@ func TestSnapshotCommandLine(t *testing.T) { got := build("/tmp/ram.img", "") for _, want := range []string{ "memory-backend-file,id=pc.ram,size=512M,mem-path=/tmp/ram.img,share=on", - "-machine q35,accel=kvm,memory-backend=pc.ram", + "memory-backend=pc.ram", } { if !strings.Contains(got, want) { t.Errorf("missing %q in: %s", want, got) diff --git a/internal/host/vm/qemu/spec.go b/internal/host/vm/qemu/spec.go new file mode 100644 index 0000000..0dd8c5a --- /dev/null +++ b/internal/host/vm/qemu/spec.go @@ -0,0 +1,167 @@ +//go:build linux + +package qemu + +import ( + "fmt" + "os" + + "github.com/spin-stack/spin-machine/machine" + + "github.com/spin-stack/spinbox/internal/config" + "github.com/spin-stack/spinbox/internal/host/vm" + "github.com/spin-stack/spinbox/internal/paths" +) + +// The machine a guest sees is defined once, in spin-machine, and this file is +// the whole of what this repository says about it. +// +// It used to be defined twice: a command-line builder here, and a MachineIdentity +// beside it that spelled the same four arguments again so a template's +// fingerprint could be computed before a command line existed. The two were kept +// in step by a shared machineShape function and a comment asking the next person +// not to break it, because nothing checks a restore at run time β€” a template +// loaded into a machine of another shape is memory and device state going into +// hardware it did not come from, and it does not fail, it misbehaves. +// +// Now there is one machine.Spec, the command line is Args() of it and the +// fingerprint is Fingerprint() of it, and the two cannot disagree because there +// is nothing left to disagree. + +// baseSpec is the machine every VM on this host is, with nothing about any +// particular VM in it. +// +// It exists because a template's fingerprint has to be computable before there is +// a VM: the lookup that decides whether to restore or boot happens before an +// instance is created, and building a throwaway one to ask would allocate a vsock +// CID and a log directory for a question. +// +// **The placeholders below are load-bearing.** machine.Spec.Fingerprint hashes the +// device topology β€” which devices at which slots β€” and for the vsock and the +// serial console what it hashes is *presence*, not the CID or the chardev string. +// Every VM this host starts has both, so this must have both, or a VM would hash +// a machine with no console and never find the template it would itself produce. +// That failure is silent in the direction that costs: every VM boots, nothing +// errors, and the templates are simply never used again. +func baseSpec(qemuPath, kernelPath, initrdPath, firmwareDir string, r *vm.VMResourceConfig) machine.Spec { + return machine.Spec{ + QEMU: qemuPath, + Kernel: kernelPath, + Initrd: initrdPath, + Firmware: firmwareDir, + + BootCPUs: r.BootCPUs, + MaxCPUs: r.MaxCPUs, + Memory: machine.Memory{ + SizeMB: int(r.MemorySize / (1024 * 1024)), + MaxMB: int(r.MemoryHotplugSize / (1024 * 1024)), + }, + + // Presence, not value. See above. + VsockCID: placeholderCID, + Serial: placeholderSerial, + } +} + +const ( + // placeholderCID stands for "this machine has a vhost-vsock device". Every VM + // gets a real, unique CID; none is ever this one, and none needs to be β€” the + // context id is not in the migration stream, and a restored guest re-reads it + // when QEMU resets the transport. + placeholderCID = 3 + // placeholderSerial stands for "this machine has an ISA serial port". The real + // one is a FIFO in the VM's state directory, which is per-VM by construction. + placeholderSerial = "none" +) + +// specFor returns the machine this host would build for a container of this +// size, without creating one. +func specFor(resourceCfg *vm.VMResourceConfig) (machine.Spec, error) { + cfg, err := config.Get() + if err != nil { + return machine.Spec{}, fmt.Errorf("reading config: %w", err) + } + rel, err := paths.Machine(cfg.Paths) + if err != nil { + return machine.Spec{}, err + } + initrdPath := paths.InitrdPath(cfg.Paths) + if _, err := os.Stat(initrdPath); err != nil { + return machine.Spec{}, fmt.Errorf("initrd not found at %s (run 'task build:initrd'): %w", initrdPath, err) + } + return baseSpec( + paths.QemuPath(cfg.Paths, rel), + rel.Kernel(), + initrdPath, + paths.QemuSharePath(cfg.Paths, rel), + validateResourceConfig(resourceCfg), + ), nil +} + +// spec is the machine this instance runs, complete: the shared shape plus +// everything that belongs to this one VM. +// +// cmdline is empty for a restoring VM, and deliberately: it never executes the +// kernel β€” its memory arrives from the template already booted β€” so nothing +// parses -append, and the guest's own /proc/cmdline comes from that restored +// memory. Passing a container's address and disk layout here would write them +// into a process command line that nothing reads and `ps` shows to everyone. +func (q *Instance) spec(cmdline string) (machine.Spec, error) { + cfg, err := config.Get() + if err != nil { + return machine.Spec{}, fmt.Errorf("reading config: %w", err) + } + rel, err := paths.Machine(cfg.Paths) + if err != nil { + return machine.Spec{}, err + } + + s := baseSpec(q.binaryPath, q.kernelPath, q.initrdPath, + paths.QemuSharePath(cfg.Paths, rel), validateResourceConfig(q.resourceCfg)) + + s.VsockCID = int(q.guestCID) + // QEMU writes the console into a FIFO rather than the log file directly, so a + // slow disk cannot block the VM; a goroutine drains it. See setupConsoleFIFO. + s.Serial = "file:" + q.consoleFifoPath + s.QMPSocket = q.qmpSocketPath + if q.restoreStatePath == "" { + s.Cmdline = cmdline + } + + // A template writes into the memory file and must share it; a VM restoring + // from one maps the same file privately, so what it writes stays its own. + // That is the whole copy-on-write story, and it is why one template file can + // serve many VMs without being copied. + s.Memory.File = q.memoryFilePath + s.Memory.Shared = q.memoryFilePath != "" && q.restoreStatePath == "" + s.IncomingDefer = q.restoreStatePath != "" + + for _, d := range q.disks { + s.Disks = append(s.Disks, machine.Disk{ + Path: d.Path, + // Stated by whoever added the disk, never probed here β€” see + // vm.MountConfig.Format. Today they are the snapshotter's: a vmdk of + // the merged layers and a raw rwlayer. They become one qcow2 chain, a + // read-only base many VMs map at once and a read-write tip that is this + // VM's, when the disks stop coming from a snapshotter. + Format: d.Format, + Readonly: d.Readonly, + Serial: d.Serial, + // A writable image gets an explicit lock so something outside QEMU can + // find out whether a VM is running on it by trying to take the same one. + // The read-only base takes no lock: that is the whole point of it. + Locking: !d.Readonly, + }) + } + + for i, n := range q.nets { + if n.TapFile == nil { + return machine.Spec{}, fmt.Errorf("NIC %s has no TAP file descriptor (openTapFiles not called?)", n.TapName) + } + // The descriptor number inside the QEMU process: ExtraFiles starts at 3, + // and the order here is the order they are appended to it in Start. + s.NICs = append(s.NICs, machine.NIC{TapFD: 3 + i, MAC: n.MAC}) + } + + return s, nil +} diff --git a/internal/host/vm/qemu/start.go b/internal/host/vm/qemu/start.go index 1d965dc..090213f 100644 --- a/internal/host/vm/qemu/start.go +++ b/internal/host/vm/qemu/start.go @@ -15,9 +15,7 @@ import ( "github.com/containerd/log" "github.com/containerd/ttrpc" - "github.com/spin-stack/spinbox/internal/config" "github.com/spin-stack/spinbox/internal/host/vm" - "github.com/spin-stack/spinbox/internal/paths" ) func (q *Instance) setupConsoleFIFO(ctx context.Context) error { @@ -469,13 +467,14 @@ func (q *Instance) Start(ctx context.Context, opts ...vm.StartOpt) error { // Build kernel command line cmdlineArgs := q.buildKernelCommandLine(startOpts) - // Boot profiling routes the console through virtio-console (see - // buildKernelCommandLine / buildQemuCommandLine); compute the flag once so - // the cmdline and the QEMU device list stay in agreement. - debugBoot := startOpts.DebugBoot || bootDebugEnabled() - - // Build QEMU command line (now uses the renamed TAP names) - qemuArgs, err := q.buildQemuCommandLine(cmdlineArgs, debugBoot) + // The machine, and its command line. Both come from one machine.Spec, so the + // thing QEMU is given and the thing a template's fingerprint describes cannot + // be different machines. See spec.go. + spec, err := q.spec(cmdlineArgs) + if err != nil { + return err + } + qemuArgs, err := spec.Args() if err != nil { return err } @@ -580,123 +579,5 @@ func bootDebugEnabled() bool { } } -// buildQemuCommandLine constructs the QEMU command line arguments. -// When debug is set, the kernel is asked for initcall_debug and a printk ring big -// enough to hold the result; the VM itself is the same one a production boot gets, -// which is the point β€” a profile of a different machine measures a different boot. -func (q *Instance) buildQemuCommandLine(cmdlineArgs string, debug bool) ([]string, error) { - cfg, err := config.Get() - if err != nil { - return nil, fmt.Errorf("failed to get config: %w", err) - } - - // The four arguments that decide the shape of the machine come from - // machineShape, which MachineIdentity also uses. A restore requires the - // template and the VM to be the same shape and nothing checks it at runtime, - // so the two must be spelled in one place. See machineShape. - machineArg, cpuArg, smpArg, memoryArg := machineShape(q.resourceCfg, q.memoryFilePath != "") - memoryMB := int(q.resourceCfg.MemorySize / (1024 * 1024)) - - // Fixed PCI slots bound how many devices fit on the root complex; check - // before building so the failure names the limit instead of surfacing as a - // QEMU slot collision at exec time. - if len(q.disks) > maxDisks { - return nil, fmt.Errorf("too many disks: %d configured, %d PCI slots available", len(q.disks), maxDisks) - } - if len(q.nets) > maxNICs { - return nil, fmt.Errorf("too many NICs: %d configured, %d PCI slots available", len(q.nets), maxNICs) - } - - // Build QEMU command using fluent builder pattern - builder := newQemuCommandBuilder(). - setNoDefaults(). // Disable default devices (prevents e1000e NIC needing ROM files) - // Seccomp sandbox: cheap hardening around the VM isolation boundary. - setSandbox(). - setBIOSPath(paths.QemuSharePath(cfg.Paths)). - // Chipset, CPU model, vCPU count and memory - kernel IRQ chip on, HPET - // off. See machineShape, which a template's fingerprint also reads. - setMachineShape(machineArg, cpuArg, smpArg, memoryArg). - // Drop S3/S4 from the ACPI tables. A microVM never suspends or - // hibernates, and the guest skips the corresponding ACPI setup. - addGlobal("ICH9-LPC.disable_s3=1"). - addGlobal("ICH9-LPC.disable_s4=1"). - setKernel(q.kernelPath). - setInitrd(q.initrdPath). - setNoGraphic(). - // Serial console β†’ FIFO pipe (producer side) - // QEMU writes VM console output here; background goroutine reads and streams to log file - // See setupConsoleFIFO() for the producer-consumer pipeline details - setSerial(fmt.Sprintf("file:%s", q.consoleFifoPath)). - // QMP for VM control - setQMPUnixSocket(q.qmpSocketPath). - // RNG device for entropy - addVirtioRNG() - - // No virtio-console for profiling any more: the profile is read from /dev/kmsg - // and the console is silent (BuildKernelCmdline), so the device the debug boot - // used to add existed only to carry output nobody reads. Removing it also takes - // the 24 ms `virtio_console_init` out of the profile β€” which was the console - // registering and replaying the ring, not work a production boot does. - - // Vsock for guest communication (using vhost-vsock kernel module). It sits on - // the root complex, cold-plugged, on a restored VM exactly as on a booted one: - // a restore is handed its own CID on the command line and the guest picks it - // up by itself. See below. - builder.addVsockDevice(int(q.guestCID)) - - // A restored VM is given no kernel command line. - // - // It would be inert twice over. The VM never executes the kernel - its memory - // arrives from the template already booted - so nothing parses what is in - // -append; and the guest's own /proc/cmdline comes from that restored memory, - // which is the template's command line, not this one. Passing a container's - // address and disk layout here would write them into a process command line - // that nothing reads and `ps` shows to everyone. - // - // A VM that boots still takes its identity from here; that is what - // system.FromCmdline reads, and booting is the fallback whenever restoring is - // not possible. - if q.restoreStatePath == "" { - builder.setKernelArgs(cmdlineArgs) - } - - // Snapshot plumbing. All of it is skipped on a VM that neither builds a - // template nor restores from one, which is every VM today. - if q.memoryFilePath != "" { - // A restoring VM maps the template's RAM privately (copy-on-write); a - // template writes into it and must share. - builder.setMemoryBackendFile(q.memoryFilePath, memoryMB, q.restoreStatePath == ""). - // Every restore inherits the template's random pool along with its - // memory; this is how the guest learns it is a new VM and reseeds. - addVMGenID() - } - if q.restoreStatePath != "" { - builder.setIncomingDefer() - } - - // Add disks - for i, disk := range q.disks { - builder.addDisk(i, fmt.Sprintf("blk%d", i), disk) - } - - // Add NICs - for i, nic := range q.nets { - // Use Kata Containers approach: pass TAP via file descriptor - // FD will be passed via ExtraFiles, which start at FD 3 - // (FDs 0,1,2 are stdin/stdout/stderr) - if nic.TapFile == nil { - // This should never happen - TAP FD must be opened before Start() - return nil, fmt.Errorf("internal error: NIC %s has no TAP file descriptor (openTapFiles not called?)", nic.TapName) - } - fd := 3 + i - builder.addNIC(i, fmt.Sprintf("net%d", i), NICConfig{ - TapFD: fd, - MAC: nic.MAC, - }) - } - - return builder.build(), nil -} - // Client returns the long-lived TTRPC client for communicating with the guest. // This is used for the event stream and should not be shared for concurrent RPCs. diff --git a/internal/host/vm/qemu/template.go b/internal/host/vm/qemu/template.go index d3a76d4..9e0ae57 100644 --- a/internal/host/vm/qemu/template.go +++ b/internal/host/vm/qemu/template.go @@ -3,23 +3,19 @@ package qemu import ( - "crypto/sha256" - "encoding/hex" "errors" "fmt" - "io" "os" "path/filepath" - "runtime" "sort" "strings" "syscall" - "github.com/spin-stack/spinbox/internal/host/vm" + "github.com/spin-stack/spin-machine/machine" ) -// Templates: the frozen VM every later VM is restored from, and the identity -// that says which frozen VM a given machine may use. +// Templates: the frozen VM every later VM is restored from, and the store that +// says which frozen VM a given machine may use. // // Restoring is not a general-purpose import. The migration stream carries device // and CPU state for one exact machine, and loading it into a machine of another @@ -29,10 +25,9 @@ import ( // fingerprint of everything that has to match, and a machine that hashes // differently does not find one and boots instead. // -// What has to match is not obvious, and getting it wrong is silent. In -// particular -cpu host means the template carries the *host's* CPU model: a -// template is not portable to a machine with a different CPU, however identical -// the software. +// What has to match is machine.Spec.Fingerprint's business, not this file's. +// What is this file's business is the consequence: a template is a directory +// named by that hash, and nothing here ever compares two machines any other way. const ( // templateRAMName and templateStateName are the two files a template is. @@ -58,116 +53,27 @@ const ( // fingerprint. It is not a failure: it means this VM boots. var ErrNoTemplate = errors.New("no template for this machine") -// MachineIdentity is everything about a VM that a restore requires to be -// identical between the template and the VM restored from it. +// fingerprintOf reduces a machine to the name of the directory its template +// lives in. // -// It deliberately does not include what a restore is allowed to differ in, all -// of which is established elsewhere and measured: the vsock CID (not in the -// migration stream, and the guest re-reads it when QEMU sends the post-migration -// transport reset), the disks (cold-plugged onto the restored VM and found with -// a PCI rescan), and the network backend behind the NIC. -type MachineIdentity struct { - // QEMU is the emulator binary. Its contents, not its path: an upgrade in - // place must invalidate every template, and the version string alone does - // not distinguish two builds of the same release with different device - // configurations - which this project ships. - QEMU string - - // Kernel and Initrd are the guest images. A restored VM never executes - // them, but the memory it restores was produced by that exact pair. - Kernel string - Initrd string - - // Machine, CPU, SMP and Memory are the QEMU command-line arguments that - // decide the shape of the machine: which chipset and its options, the CPU - // model, the vCPU count and hotplug ceiling, and the memory size, slots and - // hotplug ceiling. A restored VM inherits all of them from the template, - // which is why boot-small-and-hotplug-up is the only way one template serves - // containers of different sizes. - Machine string - CPU string - SMP string - Memory string - - // HostCPU is the host's own CPU model. Under `-cpu host` the guest is shown - // the host's feature set, so a template made on one machine describes a CPU - // the next machine may not have. - HostCPU string -} - -// Fingerprint reduces the identity to the name of the directory its template -// lives in. Files are hashed by content; everything else by value. -func (m MachineIdentity) Fingerprint() (string, error) { - h := sha256.New() - - // Length-prefixed, so that no two different identities can produce the same - // byte stream by moving a delimiter into a value. - // - // The error is discarded because hash.Hash's Write never returns one; it is - // part of the interface's contract. - write := func(key, value string) { - _, _ = fmt.Fprintf(h, "%s=%d:%s\n", key, len(value), value) - } - - for _, f := range []struct{ key, path string }{ - {"qemu", m.QEMU}, - {"kernel", m.Kernel}, - {"initrd", m.Initrd}, - } { - sum, err := hashFile(f.path) - if err != nil { - return "", fmt.Errorf("fingerprinting %s: %w", f.key, err) - } - write(f.key, sum) - } - - write("machine", m.Machine) - write("cpu", m.CPU) - write("smp", m.SMP) - write("memory", m.Memory) - write("host-cpu", m.HostCPU) - write("arch", runtime.GOARCH) - - return hex.EncodeToString(h.Sum(nil))[:templateFingerprintLen], nil -} - -// hashFile returns the SHA-256 of a file's contents. -func hashFile(path string) (string, error) { - f, err := os.Open(path) +// The hash is machine.Spec.Fingerprint's β€” the QEMU binary, the kernel and the +// initrd by content, the four arguments that decide the machine's shape, the +// device topology, and the host's own CPU model when the guest is being shown it +// β€” truncated to a length that reads in a path. This repository no longer has an +// opinion about what belongs in it: it used to keep a MachineIdentity of its own +// that spelled the same fields, and the two disagreed in both directions. It +// hashed runtime.GOARCH, which cannot differ without the QEMU binary differing, +// and it hashed the host CPU unconditionally, which is right under `-cpu host` +// and wrong the moment a model is named β€” it would partition templates per +// machine exactly when the point of naming one is that they cross machines. It +// did not hash the device list at all, so adding the balloon left every existing +// template matching a machine it could no longer be restored into. +func fingerprintOf(spec machine.Spec) (string, error) { + full, err := spec.Fingerprint() if err != nil { return "", err } - defer f.Close() - - h := sha256.New() - if _, err := io.Copy(h, f); err != nil { - return "", fmt.Errorf("reading %s: %w", path, err) - } - return hex.EncodeToString(h.Sum(nil)), nil -} - -// HostCPUModel reads the host CPU's model name, which `-cpu host` makes part of -// what a template describes. -// -// It returns the model name from /proc/cpuinfo rather than the feature flags. -// The flags would be the exact thing, but they also move with microcode updates -// and kernel mitigations, which would invalidate every template on a machine -// that has not meaningfully changed. The model is the coarse identity that -// distinguishes one host's silicon from another's, which is what this is for. -func HostCPUModel() (string, error) { - b, err := os.ReadFile("/proc/cpuinfo") - if err != nil { - return "", fmt.Errorf("reading /proc/cpuinfo: %w", err) - } - for _, line := range strings.Split(string(b), "\n") { - if name, ok := strings.CutPrefix(line, "model name"); ok { - _, value, found := strings.Cut(name, ":") - if found { - return strings.TrimSpace(value), nil - } - } - } - return "", errors.New("no model name in /proc/cpuinfo") + return full[:templateFingerprintLen], nil } // TemplateStore holds the templates built on this host, one directory per @@ -205,8 +111,8 @@ type Template struct { // writes them in an order that never leaves a half-made template visible - see // Save - but a host that ran out of disk mid-build should boot rather than // restore from half a machine. -func (s *TemplateStore) Lookup(id MachineIdentity) (Template, error) { - fp, err := s.cache.fingerprint(id) +func (s *TemplateStore) Lookup(spec machine.Spec) (Template, error) { + fp, err := s.cache.fingerprint(spec) if err != nil { return Template{}, err } @@ -238,8 +144,8 @@ func (s *TemplateStore) at(fp string) Template { // over several seconds, and a VM that started restoring from a half-written // template would not fail cleanly - it would resume a guest whose memory is // part of one machine and part of nothing. -func (s *TemplateStore) Stage(id MachineIdentity) (Template, error) { - fp, err := s.cache.fingerprint(id) +func (s *TemplateStore) Stage(spec machine.Spec) (Template, error) { + fp, err := s.cache.fingerprint(spec) if err != nil { return Template{}, err } @@ -312,113 +218,6 @@ func (s *TemplateStore) Remove(fp string) error { return os.RemoveAll(filepath.Join(s.dir, fp)) } -// MachineIdentity describes the machine this instance would present to a guest, -// which is what decides whether it may restore from a given template. -func (q *Instance) MachineIdentity() (MachineIdentity, error) { - return machineIdentity(q.binaryPath, q.kernelPath, q.initrdPath, q.resourceCfg) -} - -// MachineIdentityFor returns the identity of the machine this host would build -// for a container of this size, without creating one. -// -// The lookup that decides whether a VM restores happens before there is an -// instance to ask, and building a throwaway one to ask it would allocate a vsock -// CID and a log directory for a question. -func MachineIdentityFor(resourceCfg *vm.VMResourceConfig) (MachineIdentity, error) { - qemuPath, err := findQemu() - if err != nil { - return MachineIdentity{}, err - } - kernelPath, err := findKernel() - if err != nil { - return MachineIdentity{}, err - } - initrdPath, err := findInitrd() - if err != nil { - return MachineIdentity{}, err - } - return machineIdentity(qemuPath, kernelPath, initrdPath, resourceCfg) -} - -// machineIdentity is the one place an identity is assembled. -// -// The four QEMU arguments come from machineShape, the same function the command -// line is built from, so the identity cannot describe a machine other than the -// one QEMU is given. The resource config goes through validateResourceConfig -// first for the same reason: an instance is created from the defaulted values, -// so an identity taken from the raw ones would hash a machine nobody builds - -// and the template a VM built would never be the template it later looked up. -func machineIdentity(qemuPath, kernelPath, initrdPath string, resourceCfg *vm.VMResourceConfig) (MachineIdentity, error) { - hostCPU, err := HostCPUModel() - if err != nil { - return MachineIdentity{}, err - } - - // Always file-backed: a template and every VM restored from one have their - // memory in a file, whatever the instance asking was configured with. - machine, cpu, smp, memory := machineShape(validateResourceConfig(resourceCfg), true) - - return MachineIdentity{ - QEMU: qemuPath, - Kernel: kernelPath, - Initrd: initrdPath, - Machine: machine, - CPU: cpu, - SMP: smp, - Memory: memory, - HostCPU: hostCPU, - }, nil -} - -// machineShape returns the four QEMU arguments that decide the shape of the -// machine a guest sees: the chipset and its options, the CPU model, the vCPU -// count with its hotplug ceiling, and the memory size with its slots and -// ceiling. -// -// It is one function because it has two callers that must never disagree: the -// command line QEMU is given, and the MachineIdentity that decides which -// template this machine may restore from. A restore loads device and CPU state -// into a machine that has to be the same shape, and nothing checks that at -// runtime - so if the identity stopped describing the command line, a VM would -// restore from a template of another machine and the failure would be silent. -// Spelling it once removes the possibility rather than testing for it. -// -// fileBackedRAM says whether guest memory comes from a memory-backend-file, -// which every template and every VM restored from one uses, and which changes -// the machine string. -func machineShape(r *vm.VMResourceConfig, fileBackedRAM bool) (machine, cpu, smp, memory string) { - backend := "" - if fileBackedRAM { - backend = machineMemoryBackend(memoryBackendID) - } - machine = strings.Join(nonEmpty( - "q35", "accel=kvm", "kernel-irqchip=on", "hpet=off", "acpi=on", backend, - ), ",") - - memoryMB := int(r.MemorySize / (1024 * 1024)) - memoryMaxMB := int(r.MemoryHotplugSize / (1024 * 1024)) - slots := defaultMemorySlots - if r.MemoryHotplugSize <= r.MemorySize { - slots = 0 - } - - return machine, "host,migratable=on", - smpArg(r.BootCPUs, r.MaxCPUs), - memoryArg(memoryMB, slots, memoryMaxMB) -} - -// nonEmpty drops the empty strings from a list, so a conditional option can be -// passed as "" without leaving the comma QEMU rejects. -func nonEmpty(values ...string) []string { - kept := make([]string, 0, len(values)) - for _, v := range values { - if v != "" { - kept = append(kept, v) - } - } - return kept -} - // buildsTemplate reports whether this VM is the one a template is made from: it // writes guest memory into a template's RAM file and loads no state of its own. // diff --git a/internal/host/vm/qemu/template_build.go b/internal/host/vm/qemu/template_build.go index d82a599..3c5e5a8 100644 --- a/internal/host/vm/qemu/template_build.go +++ b/internal/host/vm/qemu/template_build.go @@ -47,12 +47,12 @@ func BuildTemplate(ctx context.Context, store *TemplateStore, stateDir string, r ctx, cancel := context.WithTimeout(ctx, templateBuildTimeout) defer cancel() - id, err := MachineIdentityFor(resourceCfg) + spec, err := specFor(resourceCfg) if err != nil { return Template{}, fmt.Errorf("identifying this machine: %w", err) } - if existing, err := store.Lookup(id); err == nil { + if existing, err := store.Lookup(spec); err == nil { log.G(ctx).WithField("fingerprint", existing.Fingerprint). Debug("qemu: template already built for this machine") return existing, nil @@ -60,7 +60,7 @@ func BuildTemplate(ctx context.Context, store *TemplateStore, stateDir string, r return Template{}, err } - staged, err := store.Stage(id) + staged, err := store.Stage(spec) if err != nil { return Template{}, err } diff --git a/internal/host/vm/qemu/template_build_integration_test.go b/internal/host/vm/qemu/template_build_integration_test.go index 2472dd3..f75a0ab 100644 --- a/internal/host/vm/qemu/template_build_integration_test.go +++ b/internal/host/vm/qemu/template_build_integration_test.go @@ -68,9 +68,9 @@ func TestBuildTemplate(t *testing.T) { // A machine of a different shape must not find this template. Nothing checks // the shape at restore time, so this is the check. - other, err := MachineIdentityFor(&vm.VMResourceConfig{BootCPUs: 4, MaxCPUs: 4}) + other, err := specFor(&vm.VMResourceConfig{BootCPUs: 4, MaxCPUs: 4}) if err != nil { - t.Fatalf("MachineIdentityFor: %v", err) + t.Fatalf("specFor: %v", err) } if _, err := store.Lookup(other); !errors.Is(err, ErrNoTemplate) { t.Errorf("a machine with a different CPU count found this template (err %v)", err) diff --git a/internal/host/vm/qemu/template_cache.go b/internal/host/vm/qemu/template_cache.go index dd6653d..ba4bce4 100644 --- a/internal/host/vm/qemu/template_cache.go +++ b/internal/host/vm/qemu/template_cache.go @@ -11,6 +11,8 @@ import ( "strings" "sync" "syscall" + + "github.com/spin-stack/spin-machine/machine" ) // Memoising the fingerprint. @@ -53,20 +55,20 @@ func newFingerprintCache(dir string) *fingerprintCache { } } -// fingerprint returns the identity's fingerprint, reading the cache first. -func (c *fingerprintCache) fingerprint(id MachineIdentity) (string, error) { - key, err := statKey(id) +// fingerprint returns the machine's fingerprint, reading the cache first. +func (c *fingerprintCache) fingerprint(spec machine.Spec) (string, error) { + key, err := statKey(spec) if err != nil { - // The files cannot be stat'ed, so they cannot be hashed either; let - // Fingerprint produce the real error. - return id.Fingerprint() + // The files cannot be stat'ed, so they cannot be hashed either; let the + // fingerprint produce the real error. + return fingerprintOf(spec) } if fp, ok := c.lookup(key); ok { return fp, nil } - fp, err := id.Fingerprint() + fp, err := fingerprintOf(spec) if err != nil { return "", err } @@ -136,11 +138,17 @@ func (c *fingerprintCache) store(key, fp string) { } // statKey identifies the inputs of a fingerprint without reading them: each -// file's size, modification time and inode, plus the machine arguments, which -// are cheap enough to include verbatim. -func statKey(id MachineIdentity) (string, error) { +// file's size, modification time and inode, plus everything else the fingerprint +// hashes. +// +// That second part is machine.Spec.Identity and not a list spelled here. A list +// spelled here is a list that goes out of date the next time a device is added +// to the machine, and the way it fails is a cache hit returning the fingerprint +// of a machine this is not β€” which is a template restored into hardware it did +// not come from, silently. +func statKey(spec machine.Spec) (string, error) { h := sha256.New() - for _, path := range []string{id.QEMU, id.Kernel, id.Initrd} { + for _, path := range []string{spec.QEMU, spec.Kernel, spec.Initrd} { fi, err := os.Stat(path) if err != nil { return "", err @@ -152,6 +160,10 @@ func statKey(id MachineIdentity) (string, error) { // Discarded: hash.Hash's Write never returns an error, by contract. _, _ = fmt.Fprintf(h, "%s|%d|%d|%d\n", path, fi.Size(), fi.ModTime().UnixNano(), ino) } - _, _ = fmt.Fprintf(h, "%s|%s|%s|%s|%s\n", id.Machine, id.CPU, id.SMP, id.Memory, id.HostCPU) + ident, err := spec.Identity() + if err != nil { + return "", err + } + _, _ = fmt.Fprintf(h, "%s\n", ident) return hex.EncodeToString(h.Sum(nil)), nil } diff --git a/internal/host/vm/qemu/template_test.go b/internal/host/vm/qemu/template_test.go index 2d78d53..2bb3d22 100644 --- a/internal/host/vm/qemu/template_test.go +++ b/internal/host/vm/qemu/template_test.go @@ -6,15 +6,20 @@ import ( "errors" "os" "path/filepath" - "strings" "testing" - "github.com/spin-stack/spinbox/internal/host/vm" + "github.com/spin-stack/spin-machine/machine" ) -// testIdentity returns an identity whose three files exist, so Fingerprint can -// hash them. -func testIdentity(t *testing.T) MachineIdentity { +// The fingerprint itself is not tested here any more: it is machine.Spec's, and +// that package tests it against the properties that matter β€” every input moving +// it, the device topology being in it, what is behind a device not being, the +// host CPU folding in under one model and out under another. What is tested here +// is what this repository still owns: a store of directories named by that hash, +// and a memo so the hash is not recomputed for every container. + +// testSpec returns a machine whose three files exist, so it can be fingerprinted. +func testSpec(t *testing.T) machine.Spec { t.Helper() dir := t.TempDir() write := func(name, content string) string { @@ -24,138 +29,32 @@ func testIdentity(t *testing.T) MachineIdentity { } return p } - return MachineIdentity{ - QEMU: write("qemu", "qemu binary"), - Kernel: write("kernel", "kernel image"), - Initrd: write("initrd", "initrd image"), - Machine: "q35,accel=kvm,memory-backend=pc.ram", - CPU: "host,migratable=on", - SMP: "1,maxcpus=20", - Memory: "512,slots=8,maxmem=31360M", - HostCPU: "12th Gen Intel(R) Core(TM) i9-12900K", + return machine.Spec{ + QEMU: write("qemu", "qemu binary"), + Kernel: write("kernel", "kernel image"), + Initrd: write("initrd", "initrd image"), + Firmware: dir, + BootCPUs: 1, + MaxCPUs: 20, + Memory: machine.Memory{SizeMB: 512, MaxMB: 30720}, + VsockCID: placeholderCID, + Serial: placeholderSerial, } } -func fingerprint(t *testing.T, id MachineIdentity) string { +func fingerprint(t *testing.T, spec machine.Spec) string { t.Helper() - fp, err := id.Fingerprint() + fp, err := fingerprintOf(spec) if err != nil { - t.Fatalf("Fingerprint: %v", err) + t.Fatalf("fingerprint: %v", err) } return fp } -// TestFingerprintChangesWithEveryInput is the test this whole mechanism exists -// for. A restore loads device and CPU state into a machine that must be the -// same shape, and nothing checks that at runtime: if an input stops changing the -// fingerprint, a VM restores from a template of another machine and the failure -// is silent and arbitrary. -func TestFingerprintChangesWithEveryInput(t *testing.T) { - t.Parallel() - - base := testIdentity(t) - original := fingerprint(t, base) - - // Changing a file's *contents* must change the fingerprint, not just its - // path: QEMU and the kernel are upgraded in place. - for _, f := range []struct { - name string - path string - }{ - {"qemu binary", base.QEMU}, - {"kernel image", base.Kernel}, - {"initrd image", base.Initrd}, - } { - t.Run("contents of the "+f.name, func(t *testing.T) { - before, err := os.ReadFile(f.path) - if err != nil { - t.Fatalf("reading %s: %v", f.path, err) - } - t.Cleanup(func() { _ = os.WriteFile(f.path, before, 0600) }) - - if err := os.WriteFile(f.path, append(before, '!'), 0600); err != nil { - t.Fatalf("rewriting %s: %v", f.path, err) - } - if got := fingerprint(t, base); got == original { - t.Errorf("upgrading the %s in place left the fingerprint at %s, so a VM "+ - "would restore a template built by the previous one", f.name, got) - } - }) - } - - for _, c := range []struct { - name string - mutate func(*MachineIdentity) - }{ - {"machine type", func(m *MachineIdentity) { m.Machine = "q35,accel=kvm" }}, - {"cpu model", func(m *MachineIdentity) { m.CPU = "host" }}, - {"vcpu count", func(m *MachineIdentity) { m.SMP = "2,maxcpus=20" }}, - {"hotplug cpu ceiling", func(m *MachineIdentity) { m.SMP = "1,maxcpus=32" }}, - {"memory size", func(m *MachineIdentity) { m.Memory = "1024,slots=8,maxmem=31360M" }}, - {"hotplug memory ceiling", func(m *MachineIdentity) { m.Memory = "512,slots=8,maxmem=8192M" }}, - {"host cpu", func(m *MachineIdentity) { m.HostCPU = "AMD EPYC 9654" }}, - } { - t.Run(c.name, func(t *testing.T) { - t.Parallel() - changed := base - c.mutate(&changed) - if got := fingerprint(t, changed); got == original { - t.Errorf("changing the %s left the fingerprint at %s", c.name, got) - } - }) - } -} - -// TestFingerprintIsStable guards the other direction: the same machine must -// resolve to the same template every time, or every VM builds its own. -func TestFingerprintIsStable(t *testing.T) { - t.Parallel() - - id := testIdentity(t) - first := fingerprint(t, id) - for range 3 { - if got := fingerprint(t, id); got != first { - t.Fatalf("same machine hashed to %s and %s", first, got) - } - } - if len(first) != templateFingerprintLen { - t.Errorf("fingerprint %q is %d characters, want %d", first, len(first), templateFingerprintLen) - } -} - -// TestFingerprintDoesNotConfuseAdjacentFields checks that values cannot be -// shuffled across the boundary between two fields to produce the same hash. -func TestFingerprintDoesNotConfuseAdjacentFields(t *testing.T) { - t.Parallel() - - a := testIdentity(t) - a.SMP, a.Memory = "1", "2,slots=8,maxmem=31360M" - - b := a - b.SMP, b.Memory = "1,2", "slots=8,maxmem=31360M" - - if fingerprint(t, a) == fingerprint(t, b) { - t.Error("two different machines hashed the same; the fields are not delimited") - } -} - -func TestFingerprintReportsAMissingFile(t *testing.T) { - t.Parallel() - - id := testIdentity(t) - id.Kernel = filepath.Join(t.TempDir(), "absent") - - if _, err := id.Fingerprint(); err == nil { - t.Fatal("fingerprinting an identity with no kernel should fail") - } else if !strings.Contains(err.Error(), "kernel") { - t.Errorf("error should name the missing input, got: %v", err) - } -} - func TestTemplateStoreLookup(t *testing.T) { t.Parallel() - id := testIdentity(t) + id := testSpec(t) store, err := NewTemplateStore(filepath.Join(t.TempDir(), "templates")) if err != nil { t.Fatalf("NewTemplateStore: %v", err) @@ -216,7 +115,7 @@ func TestTemplateStoreLookup(t *testing.T) { func TestPublishKeepsTheTemplateAlreadyThere(t *testing.T) { t.Parallel() - id := testIdentity(t) + id := testSpec(t) store, err := NewTemplateStore(filepath.Join(t.TempDir(), "templates")) if err != nil { t.Fatalf("NewTemplateStore: %v", err) @@ -268,100 +167,12 @@ func TestRemoveRefusesAPath(t *testing.T) { } } -// TestMachineShape pins the four arguments that decide the shape of a machine. -// -// Both the QEMU command line and a template's fingerprint are built from this -// one function, so they cannot disagree - but if the values themselves change, -// every template on every host silently stops matching the machines that would -// restore from it. That is a deliberate act, and this is where it is recorded. -func TestMachineShape(t *testing.T) { - t.Parallel() - - for _, tc := range []struct { - name string - cfg vm.VMResourceConfig - fileBackedRAM bool - machine, cpu, smp, memory string - }{ - { - name: "hotplug headroom, file-backed", - cfg: vm.VMResourceConfig{BootCPUs: 1, MaxCPUs: 20, MemorySize: 512 << 20, MemoryHotplugSize: 30 << 30}, - fileBackedRAM: true, - machine: "q35,accel=kvm,kernel-irqchip=on,hpet=off,acpi=on,memory-backend=pc.ram", - cpu: "host,migratable=on", - smp: "1,maxcpus=20", - memory: "512,slots=8,maxmem=30720M", - }, - { - name: "no memory backend leaves no trailing comma", - cfg: vm.VMResourceConfig{BootCPUs: 1, MaxCPUs: 20, MemorySize: 512 << 20, MemoryHotplugSize: 30 << 30}, - fileBackedRAM: false, - machine: "q35,accel=kvm,kernel-irqchip=on,hpet=off,acpi=on", - cpu: "host,migratable=on", - smp: "1,maxcpus=20", - memory: "512,slots=8,maxmem=30720M", - }, - { - name: "no hotplug headroom drops slots and maxcpus", - cfg: vm.VMResourceConfig{BootCPUs: 4, MaxCPUs: 4, MemorySize: 2 << 30, MemoryHotplugSize: 2 << 30}, - fileBackedRAM: true, - machine: "q35,accel=kvm,kernel-irqchip=on,hpet=off,acpi=on,memory-backend=pc.ram", - cpu: "host,migratable=on", - smp: "4", - memory: "2048", - }, - } { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - machine, cpu, smp, memory := machineShape(&tc.cfg, tc.fileBackedRAM) - for _, f := range []struct{ what, got, want string }{ - {"-machine", machine, tc.machine}, - {"-cpu", cpu, tc.cpu}, - {"-smp", smp, tc.smp}, - {"-m", memory, tc.memory}, - } { - if f.got != f.want { - t.Errorf("%s is %q, want %q", f.what, f.got, f.want) - } - } - }) - } -} - -// TestMachineIdentityIsFileBacked checks the one place the identity deliberately -// differs from the instance it is taken from. -// -// A template and every VM restored from one have file-backed memory, so the -// identity always describes that machine - even when asked of an instance that -// has not been given a memory file yet, which is every instance at the moment -// the template lookup happens. -func TestMachineIdentityIsFileBacked(t *testing.T) { - t.Parallel() - - machineString := func(fileBackedRAM bool) string { - machine, _, _, _ := machineShape(&vm.VMResourceConfig{}, fileBackedRAM) //nolint:dogsled // only the machine string is under test here - return machine - } - withBackend := machineString(true) - without := machineString(false) - - if !strings.Contains(withBackend, "memory-backend=") { - t.Errorf("file-backed machine string has no backend: %q", withBackend) - } - if strings.Contains(without, "memory-backend=") { - t.Errorf("plain machine string should have no backend: %q", without) - } - if strings.HasSuffix(without, ",") { - t.Errorf("plain machine string ends in a comma, which QEMU rejects: %q", without) - } -} - // TestFingerprintCacheAvoidsRehashing checks the memo returns the same answer // the hash would, and stops returning it when a file changes. func TestFingerprintCacheAvoidsRehashing(t *testing.T) { t.Parallel() - id := testIdentity(t) + id := testSpec(t) dir := t.TempDir() cache := newFingerprintCache(dir) @@ -403,7 +214,7 @@ func TestFingerprintCacheAvoidsRehashing(t *testing.T) { func TestFingerprintCacheSurvivesGarbage(t *testing.T) { t.Parallel() - id := testIdentity(t) + id := testSpec(t) dir := t.TempDir() if err := os.WriteFile(filepath.Join(dir, fingerprintCacheName), []byte("not\x00a cache\n\n "), 0600); err != nil { t.Fatalf("writing a corrupt cache: %v", err) @@ -423,7 +234,7 @@ func TestFingerprintCacheSurvivesGarbage(t *testing.T) { func TestFingerprintCacheReportsAMissingFile(t *testing.T) { t.Parallel() - id := testIdentity(t) + id := testSpec(t) id.Initrd = filepath.Join(t.TempDir(), "absent") if _, err := newFingerprintCache(t.TempDir()).fingerprint(id); err == nil { diff --git a/internal/host/vm/qemu/types.go b/internal/host/vm/qemu/types.go index 55c22d9..69bb6ba 100644 --- a/internal/host/vm/qemu/types.go +++ b/internal/host/vm/qemu/types.go @@ -10,6 +10,9 @@ type DiskConfig struct { // Serial is the virtio-blk serial exposed to the guest (max 20 chars), // used by the guest to resolve the device independent of PCI order. Serial string + // Format is what QEMU is told the image is β€” see vm.MountConfig.Format for + // why it is stated rather than worked out. Never empty: AddDisk fills it in. + Format string } // NetConfig represents a virtio-net device configuration. diff --git a/internal/host/vm/vm.go b/internal/host/vm/vm.go index a1c86fa..ae7ec2e 100644 --- a/internal/host/vm/vm.go +++ b/internal/host/vm/vm.go @@ -89,7 +89,18 @@ func WithExtrasDisk(idx int) StartOpt { // MountConfig defines configuration for mounting disks into the VM. type MountConfig struct { Readonly bool - Vmdk bool + // Format is what QEMU is told the image is: raw, vmdk, qcow2. + // + // Stated by whoever adds the disk, because that is the only party that knows. + // It used to be worked out from the file extension while the command line was + // being built β€” a guess, in the one place nobody looks, made by code that had + // just been handed the answer and dropped it. A wrong format is not an error: + // it is a guest that boots and finds a disk full of nothing, and letting QEMU + // probe the format of a file the guest can write is how an image is talked + // into being read as another one. + // + // Empty means DefaultDiskFormat. + Format string // Serial is the virtio-blk serial exposed to the guest (max 20 chars). // The guest resolves the device by matching this serial, so the // layerβ†’device mapping does not depend on PCI enumeration order. @@ -106,10 +117,17 @@ func WithReadOnly() MountOpt { } } -// WithVmdk mounts the disk using VMDK format. -func WithVmdk() MountOpt { +// DefaultDiskFormat is what a disk is when nobody says otherwise. +// +// raw, because that is what the snapshotter's rwlayer and the extras disk are. +// It becomes qcow2 when the disks stop coming from a snapshotter and start +// coming from a layer chain, and this constant is the whole of that change. +const DefaultDiskFormat = "raw" + +// WithFormat states the image format QEMU is given for this disk. +func WithFormat(format string) MountOpt { return func(o *MountConfig) { - o.Vmdk = true + o.Format = format } } diff --git a/internal/paths/paths.go b/internal/paths/paths.go index ca9ac78..5e491eb 100644 --- a/internal/paths/paths.go +++ b/internal/paths/paths.go @@ -1,117 +1,62 @@ -// Package paths provides standard filesystem paths used by spinbox. -// These helpers take configuration as input to avoid global config coupling. -// QemuPath and QemuSharePath may probe the filesystem when auto-discovering paths. +// Package paths locates the parts of the machine this host runs guests on. +// +// There is almost nothing here, and that is the point. QEMU, the guest kernel +// and the firmware arrive together as one spin-machine release, in the layout +// that release defines, and `machine.Open` reads it. What used to be here was a +// discovery function per artefact, each with its own candidate list ending in +// /usr/bin β€” so a host with no release ran *a* QEMU, with different devices and +// a different fingerprint, and found out by way of a guest that would not start. +// +// The initrd is the exception and stays here: a release deliberately carries +// none, because what runs as PID 1 inside a guest is this repository's business +// and not the machine's. package paths import ( - "os" + "fmt" "path/filepath" + "github.com/spin-stack/spin-machine/machine" + "github.com/spin-stack/spinbox/internal/config" ) -// KernelPath returns the full path to the guest kernel. +// Machine opens the spin-machine release installed under the share directory, +// failing with the name of whatever part is missing. // -// It is "vmlinux", which is what the ELF is and what a spin-machine release -// installs. The older "spinbox-kernel-x86_64" is still looked for afterwards, so -// a host carrying an install from before the machine moved out of this repository -// keeps working until it is upgraded; there is nothing else to do for it, and -// finding out by way of a VM that will not start is worse. -func KernelPath(pathsCfg config.PathsConfig) string { - dir := filepath.Join(pathsCfg.ShareDir, "kernel") - for _, name := range []string{"vmlinux", "spinbox-kernel-x86_64"} { - if p := filepath.Join(dir, name); fileExists(p) { - return p - } +// The two explicit overrides are honoured because they are somebody saying what +// they mean, and are applied after the release is opened so that a host with an +// override still has to have a whole machine. +func Machine(pathsCfg config.PathsConfig) (*machine.Release, error) { + rel, err := machine.Open(pathsCfg.ShareDir) + if err != nil { + return nil, fmt.Errorf("%w (run 'task machine' to fetch the pinned release)", err) } - return filepath.Join(dir, "vmlinux") -} - -// InitrdPath returns the full path to the initrd binary based on the provided configuration -func InitrdPath(pathsCfg config.PathsConfig) string { - return filepath.Join(pathsCfg.ShareDir, "kernel", "spinbox-initrd") + return rel, nil } -// QemuPath returns the full path to the qemu-system-x86_64 binary based on the provided configuration -func QemuPath(pathsCfg config.PathsConfig) string { - // If explicitly configured, use that path +// QemuPath is the emulator to run, the override taking precedence. +func QemuPath(pathsCfg config.PathsConfig, rel *machine.Release) string { if pathsCfg.QEMUPath != "" { return pathsCfg.QEMUPath } - - // Otherwise perform auto-discovery - return discoverQemuPath(pathsCfg.ShareDir) + return rel.QEMU() } -// QemuSharePath returns the path to QEMU's share directory containing BIOS files based on the provided configuration -func QemuSharePath(pathsCfg config.PathsConfig) string { - // If explicitly configured, use that path +// QemuSharePath is the directory QEMU loads firmware from, the override taking +// precedence. +func QemuSharePath(pathsCfg config.PathsConfig, rel *machine.Release) string { if pathsCfg.QEMUSharePath != "" { return pathsCfg.QEMUSharePath } - - // Otherwise perform auto-discovery - return discoverQemuSharePath(pathsCfg.ShareDir) -} - -// discoverQemuPath attempts to find qemu-system-x86_64 binary -func discoverQemuPath(shareDir string) string { - // Check spinbox share directory first - candidates := []string{ - filepath.Join(shareDir, "bin", "qemu-system-x86_64"), - "/usr/bin/qemu-system-x86_64", - "/usr/local/bin/qemu-system-x86_64", - } - - for _, path := range candidates { - if fileExists(path) { - return path - } - } - - // Default fallback - return "/usr/bin/qemu-system-x86_64" -} - -// discoverQemuSharePath attempts to find QEMU share directory -func discoverQemuSharePath(shareDir string) string { - // Check spinbox share directory first - candidates := []string{ - filepath.Join(shareDir, "qemu"), - "/usr/share/qemu", - "/usr/local/share/qemu", - } - - for _, path := range candidates { - if dirExists(path) { - return path - } - } - - // Default fallback - return "/usr/share/qemu" + return rel.Firmware() } -// fileExists checks if a file exists, resolving symlinks to the real path. -// This surfaces the real target but does not prevent TOCTOU issues. -func fileExists(path string) bool { - // Resolve symlinks to get the real path - resolved, err := filepath.EvalSymlinks(path) - if err != nil { - return false - } - info, err := os.Stat(resolved) - return err == nil && !info.IsDir() -} - -// dirExists checks if a directory exists, resolving symlinks to the real path. -// This surfaces the real target but does not prevent TOCTOU issues. -func dirExists(path string) bool { - // Resolve symlinks to get the real path - resolved, err := filepath.EvalSymlinks(path) - if err != nil { - return false - } - info, err := os.Stat(resolved) - return err == nil && info.IsDir() +// InitrdPath is the initramfs this repository builds and a release does not +// carry. +func InitrdPath(pathsCfg config.PathsConfig) string { + // The name is spelled here and in internal/config's validation rather than + // shared: that package cannot import this one β€” this one imports it for + // PathsConfig β€” so a shared constant would be a cycle. + return filepath.Join(pathsCfg.ShareDir, "kernel", "spinbox-initrd") } diff --git a/internal/paths/paths_test.go b/internal/paths/paths_test.go index 3835cff..33eb54e 100644 --- a/internal/paths/paths_test.go +++ b/internal/paths/paths_test.go @@ -3,354 +3,86 @@ package paths import ( "os" "path/filepath" + "strings" "testing" "github.com/spin-stack/spinbox/internal/config" ) -func TestFileExists(t *testing.T) { - tests := []struct { - name string - setup func(t *testing.T, tmpDir string) string - want bool - }{ - { - name: "returns true for existing file", - setup: func(t *testing.T, tmpDir string) string { - path := filepath.Join(tmpDir, "file") - if err := os.WriteFile(path, []byte("test"), 0644); err != nil { - t.Fatal(err) - } - return path - }, - want: true, - }, - { - name: "returns true for symlink to existing file", - setup: func(t *testing.T, tmpDir string) string { - realFile := filepath.Join(tmpDir, "realfile") - if err := os.WriteFile(realFile, []byte("test"), 0644); err != nil { - t.Fatal(err) - } - symlinkPath := filepath.Join(tmpDir, "linkfile") - if err := os.Symlink(realFile, symlinkPath); err != nil { - t.Fatal(err) - } - return symlinkPath - }, - want: true, - }, - { - name: "returns false for broken symlink", - setup: func(t *testing.T, tmpDir string) string { - brokenLink := filepath.Join(tmpDir, "broken") - if err := os.Symlink("/nonexistent/target", brokenLink); err != nil { - t.Fatal(err) - } - return brokenLink - }, - want: false, - }, - { - name: "returns false for directory", - setup: func(t *testing.T, tmpDir string) string { - dirPath := filepath.Join(tmpDir, "testdir") - if err := os.MkdirAll(dirPath, 0750); err != nil { - t.Fatal(err) - } - return dirPath - }, - want: false, - }, - { - name: "returns false for non-existent path", - setup: func(t *testing.T, tmpDir string) string { - return filepath.Join(tmpDir, "nonexistent") - }, - want: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - tmpDir := t.TempDir() - path := tt.setup(t, tmpDir) - - if got := fileExists(path); got != tt.want { - t.Errorf("fileExists() = %v, want %v", got, tt.want) - } - }) +// release writes a share directory holding a whole spin-machine release. +func release(t *testing.T) string { + t.Helper() + dir := t.TempDir() + for _, f := range []string{ + "bin/qemu-system-x86_64", + "bin/qemu-img", + "kernel/vmlinux", + "qemu/pvh.bin", + } { + p := filepath.Join(dir, f) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(f), 0o755); err != nil { + t.Fatal(err) + } } + return dir } -func TestDirExists(t *testing.T) { - tests := []struct { - name string - setup func(t *testing.T, tmpDir string) string - want bool - }{ - { - name: "returns true for existing directory", - setup: func(t *testing.T, tmpDir string) string { - dirPath := filepath.Join(tmpDir, "testdir") - if err := os.MkdirAll(dirPath, 0750); err != nil { - t.Fatal(err) - } - return dirPath - }, - want: true, - }, - { - name: "returns true for symlink to existing directory", - setup: func(t *testing.T, tmpDir string) string { - realDir := filepath.Join(tmpDir, "realdir") - if err := os.MkdirAll(realDir, 0750); err != nil { - t.Fatal(err) - } - symlinkPath := filepath.Join(tmpDir, "linkdir") - if err := os.Symlink(realDir, symlinkPath); err != nil { - t.Fatal(err) - } - return symlinkPath - }, - want: true, - }, - { - name: "returns false for broken symlink", - setup: func(t *testing.T, tmpDir string) string { - brokenLink := filepath.Join(tmpDir, "broken") - if err := os.Symlink("/nonexistent/target", brokenLink); err != nil { - t.Fatal(err) - } - return brokenLink - }, - want: false, - }, - { - name: "returns false for file", - setup: func(t *testing.T, tmpDir string) string { - filePath := filepath.Join(tmpDir, "testfile") - if err := os.WriteFile(filePath, []byte("test"), 0644); err != nil { - t.Fatal(err) - } - return filePath - }, - want: false, - }, - { - name: "returns false for symlink to file", - setup: func(t *testing.T, tmpDir string) string { - realFile := filepath.Join(tmpDir, "realfile") - if err := os.WriteFile(realFile, []byte("test"), 0644); err != nil { - t.Fatal(err) - } - symlinkPath := filepath.Join(tmpDir, "fakedir") - if err := os.Symlink(realFile, symlinkPath); err != nil { - t.Fatal(err) - } - return symlinkPath - }, - want: false, - }, - { - name: "returns false for non-existent path", - setup: func(t *testing.T, tmpDir string) string { - return filepath.Join(tmpDir, "nonexistent") - }, - want: false, - }, +func TestMachineNamesWhatIsMissing(t *testing.T) { + // A share directory that is not a release is the case worth having a message + // for: it is what a host looks like before `task machine` has ever run, and + // what it looks like after a half-finished upgrade. + _, err := Machine(config.PathsConfig{ShareDir: t.TempDir()}) + if err == nil { + t.Fatal("Machine accepted a directory with no release in it") } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - tmpDir := t.TempDir() - path := tt.setup(t, tmpDir) - - if got := dirExists(path); got != tt.want { - t.Errorf("dirExists() = %v, want %v", got, tt.want) - } - }) + if !strings.Contains(err.Error(), "task machine") { + t.Errorf("the error does not say how to fix it: %v", err) } } -func TestPathFunctions(t *testing.T) { - tests := []struct { - name string - cfg config.PathsConfig - fn func(config.PathsConfig) string - want string - }{ - { - name: "KernelPath", - cfg: config.PathsConfig{ShareDir: "/usr/share/spin-stack"}, - fn: KernelPath, - want: "/usr/share/spin-stack/kernel/spinbox-kernel-x86_64", - }, - { - name: "InitrdPath", - cfg: config.PathsConfig{ShareDir: "/usr/share/spin-stack"}, - fn: InitrdPath, - want: "/usr/share/spin-stack/kernel/spinbox-initrd", - }, - { - name: "QemuPath with explicit config", - cfg: config.PathsConfig{ - ShareDir: "/usr/share/spin-stack", - QEMUPath: "/custom/path/qemu-system-x86_64", - }, - fn: QemuPath, - want: "/custom/path/qemu-system-x86_64", - }, - { - name: "QemuSharePath with explicit config", - cfg: config.PathsConfig{ - ShareDir: "/usr/share/spin-stack", - QEMUSharePath: "/custom/share/qemu", - }, - fn: QemuSharePath, - want: "/custom/share/qemu", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := tt.fn(tt.cfg); got != tt.want { - t.Errorf("%s() = %q, want %q", tt.name, got, tt.want) - } - }) - } -} +func TestMachinePaths(t *testing.T) { + dir := release(t) + cfg := config.PathsConfig{ShareDir: dir} -func TestDiscoverQemuPath(t *testing.T) { - tests := []struct { - name string - setup func(t *testing.T, shareDir string) string // returns expected path - }{ - { - name: "finds in share dir bin", - setup: func(t *testing.T, shareDir string) string { - binDir := filepath.Join(shareDir, "bin") - if err := os.MkdirAll(binDir, 0755); err != nil { - t.Fatal(err) - } - qemuPath := filepath.Join(binDir, "qemu-system-x86_64") - if err := os.WriteFile(qemuPath, []byte("#!/bin/sh\n"), 0755); err != nil { - t.Fatal(err) - } - return qemuPath - }, - }, - { - name: "finds symlink to qemu", - setup: func(t *testing.T, shareDir string) string { - // Create real binary elsewhere - realBinDir := filepath.Join(shareDir, "real-bin") - if err := os.MkdirAll(realBinDir, 0755); err != nil { - t.Fatal(err) - } - realQemuPath := filepath.Join(realBinDir, "qemu-system-x86_64") - if err := os.WriteFile(realQemuPath, []byte("#!/bin/sh\n"), 0755); err != nil { - t.Fatal(err) - } - // Create symlink in expected location - binDir := filepath.Join(shareDir, "bin") - if err := os.MkdirAll(binDir, 0755); err != nil { - t.Fatal(err) - } - symlinkPath := filepath.Join(binDir, "qemu-system-x86_64") - if err := os.Symlink(realQemuPath, symlinkPath); err != nil { - t.Fatal(err) - } - return symlinkPath - }, - }, - { - name: "falls back to default when not found", - setup: func(t *testing.T, shareDir string) string { - return "/usr/bin/qemu-system-x86_64" - }, - }, + rel, err := Machine(cfg) + if err != nil { + t.Fatal(err) } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - shareDir := t.TempDir() - want := tt.setup(t, shareDir) - - if got := discoverQemuPath(shareDir); got != want { - t.Errorf("discoverQemuPath() = %q, want %q", got, want) - } - }) + for _, c := range []struct{ name, got, want string }{ + {"QemuPath", QemuPath(cfg, rel), filepath.Join(dir, "bin/qemu-system-x86_64")}, + {"QemuSharePath", QemuSharePath(cfg, rel), filepath.Join(dir, "qemu")}, + {"Kernel", rel.Kernel(), filepath.Join(dir, "kernel/vmlinux")}, + {"InitrdPath", InitrdPath(cfg), filepath.Join(dir, "kernel/spinbox-initrd")}, + } { + if c.got != c.want { + t.Errorf("%s = %q, want %q", c.name, c.got, c.want) + } } } -func TestDiscoverQemuSharePath(t *testing.T) { - tests := []struct { - name string - setup func(t *testing.T, shareDir string) string // returns expected path - }{ - { - name: "finds in share dir", - setup: func(t *testing.T, shareDir string) string { - qemuShareDir := filepath.Join(shareDir, "qemu") - if err := os.MkdirAll(qemuShareDir, 0755); err != nil { - t.Fatal(err) - } - return qemuShareDir - }, - }, - { - name: "falls back to default when not found", - setup: func(t *testing.T, shareDir string) string { - return "/usr/share/qemu" - }, - }, +func TestExplicitPathsWin(t *testing.T) { + // Somebody who names a QEMU means it. The release still has to be whole β€” + // an override is not a way to run half a machine β€” which is why Machine is + // called first and these are applied to its answer. + dir := release(t) + cfg := config.PathsConfig{ + ShareDir: dir, + QEMUPath: "/opt/qemu/bin/qemu-system-x86_64", + QEMUSharePath: "/opt/qemu/share", } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - shareDir := t.TempDir() - want := tt.setup(t, shareDir) - - if got := discoverQemuSharePath(shareDir); got != want { - t.Errorf("discoverQemuSharePath() = %q, want %q", got, want) - } - }) - } -} - -func TestQemuPathDiscovery(t *testing.T) { - shareDir := t.TempDir() - - // Create qemu binary in share dir - binDir := filepath.Join(shareDir, "bin") - if err := os.MkdirAll(binDir, 0755); err != nil { + rel, err := Machine(cfg) + if err != nil { t.Fatal(err) } - qemuPath := filepath.Join(binDir, "qemu-system-x86_64") - if err := os.WriteFile(qemuPath, []byte("#!/bin/sh\n"), 0755); err != nil { - t.Fatal(err) + if got := QemuPath(cfg, rel); got != cfg.QEMUPath { + t.Errorf("QemuPath = %q, want the override %q", got, cfg.QEMUPath) } - - cfg := config.PathsConfig{ShareDir: shareDir} - - if got := QemuPath(cfg); got != qemuPath { - t.Errorf("QemuPath() with discovery = %q, want %q", got, qemuPath) - } -} - -func TestQemuSharePathDiscovery(t *testing.T) { - shareDir := t.TempDir() - - // Create qemu share directory - qemuShareDir := filepath.Join(shareDir, "qemu") - if err := os.MkdirAll(qemuShareDir, 0755); err != nil { - t.Fatal(err) - } - - cfg := config.PathsConfig{ShareDir: shareDir} - - if got := QemuSharePath(cfg); got != qemuShareDir { - t.Errorf("QemuSharePath() with discovery = %q, want %q", got, qemuShareDir) + if got := QemuSharePath(cfg, rel); got != cfg.QEMUSharePath { + t.Errorf("QemuSharePath = %q, want the override %q", got, cfg.QEMUSharePath) } } diff --git a/internal/shim/memhotplug/controller.go b/internal/shim/memhotplug/controller.go index 9a3367c..08579d4 100644 --- a/internal/shim/memhotplug/controller.go +++ b/internal/shim/memhotplug/controller.go @@ -13,32 +13,33 @@ import ( "github.com/spin-stack/spinbox/internal/shim/hotplug" ) -const ( - // defaultMemorySlots is the default number of memory hotplug slots. - // This should match the VMResourceConfig.MemorySlots value used when starting QEMU. - defaultMemorySlots = 8 -) - // fieldContainerID is the structured-logging field key for the container ID. const fieldContainerID = "container_id" // qmpMemoryClient defines the interface for QMP memory operations. // This interface exists to enable testing with mocks. +// +// One call in each direction, because the machine grows through a virtio-mem +// device and a virtio-mem device is one number. What this replaced was pc-dimm +// devices in eight fixed slots: a backend object and a device per step, a table +// saying which slots were in use, LIFO ordering so unplug took the newest, an +// RPC into the guest to online what arrived and another to offline what was +// leaving, and a rollback for each of the four ways that could half-fail. The +// guest onlines by itself now (memhp_default_state=online), and shrinking is the +// same call with a smaller number. type qmpMemoryClient interface { - HotplugMemory(ctx context.Context, slotID int, sizeBytes int64) error - UnplugMemory(ctx context.Context, slotID int) error + // SetPluggedMemory asks for a total amount above the boot size and returns + // what the device reports afterwards. The answer is what is used: virtio-mem + // negotiates with the guest and can only take back memory the guest has + // released, so a request is not a promise and asking is not the same as + // having. + SetPluggedMemory(ctx context.Context, sizeBytes int64) (int64, error) QueryMemorySizeSummary(ctx context.Context) (*qemu.MemorySizeSummary, error) } // StatsProvider returns cgroup memory usage in bytes type StatsProvider func(ctx context.Context) (usageBytes int64, err error) -// MemoryOffliner offlines a memory block in the guest before unplug -type MemoryOffliner func(ctx context.Context, memoryID int) error - -// MemoryOnliner onlines a memory block in the guest after hotplug -type MemoryOnliner func(ctx context.Context, memoryID int) error - // MemoryHotplugController defines the interface for memory hotplug management. type MemoryHotplugController interface { Start(ctx context.Context) @@ -70,11 +71,6 @@ type Config struct { // Enable/disable features EnableScaleDown bool - - // MaxSlots is the number of memory hotplug slots available. - // This must match the QEMU configuration (VMResourceConfig.MemorySlots). - // If zero, defaults to 8. - MaxSlots int } // DefaultConfig returns sensible defaults for memory hotplug @@ -86,11 +82,10 @@ func DefaultConfig() Config { ScaleUpThreshold: 85.0, // Add memory at 85% usage ScaleDownThreshold: 60.0, // Remove memory below 60% usage OOMSafetyMarginMB: 128, // Always keep 128MB free - IncrementSize: 128 * 1024 * 1024, // 128MB (DIMM slot size) + IncrementSize: 128 * 1024 * 1024, // 128MB ScaleUpStability: 3, // Need 3 consecutive high readings (30s) ScaleDownStability: 6, // Need 6 consecutive low readings (60s) EnableScaleDown: false, // Disabled by default (memory unplug is risky) - MaxSlots: defaultMemorySlots, } } @@ -103,20 +98,21 @@ func (n *noopMemoryController) Stop() {} // Controller manages dynamic memory allocation for a VM based on memory usage type Controller struct { - containerID string - qmpClient qmpMemoryClient - stats StatsProvider - offlineMemory MemoryOffliner - onlineMemory MemoryOnliner + containerID string + qmpClient qmpMemoryClient + stats StatsProvider // Resource limits bootMemory int64 // Minimum memory (never go below this) maxMemory int64 // Maximum memory (ceiling) // Current state (protected by mu) - mu sync.Mutex - currentMemory int64 // Current online memory in bytes - usedSlots map[int]bool // Track which memory slots are used + mu sync.Mutex + // currentMemory is what the VM actually has, read back from the device after + // every request and never assumed from what was asked for: virtio-mem plugs + // memory as the guest accepts it, and unplugs only what the guest has + // released, so the number that arrived is the only one worth keeping. + currentMemory int64 // Configuration config Config @@ -136,8 +132,6 @@ func NewController( containerID string, qmpClient qmpMemoryClient, stats StatsProvider, - offliner MemoryOffliner, - onliner MemoryOnliner, bootMemory, maxMemory int64, config Config, ) MemoryHotplugController { @@ -146,21 +140,13 @@ func NewController( return &noopMemoryController{} } - // Apply default for MaxSlots if not set - if config.MaxSlots < 1 { - config.MaxSlots = defaultMemorySlots - } - c := &Controller{ containerID: containerID, qmpClient: qmpClient, stats: stats, - offlineMemory: offliner, - onlineMemory: onliner, bootMemory: bootMemory, maxMemory: maxMemory, currentMemory: bootMemory, - usedSlots: make(map[int]bool), config: config, } @@ -266,113 +252,73 @@ func (c *Controller) EvaluateScaling(ctx context.Context) (hotplug.ScaleDirectio // ScaleUp implements hotplug.ResourceScaler func (c *Controller) ScaleUp(ctx context.Context) error { - c.mu.Lock() - defer c.mu.Unlock() - - // Find available slot - slotID := c.findFreeSlot() - if slotID < 0 { - log.G(ctx).WithField(fieldContainerID, c.containerID). - Warn("memory-hotplug: no free memory slots available") - return fmt.Errorf("no free memory slots available") - } - - targetMemory := c.currentMemory + c.config.IncrementSize - if targetMemory > c.maxMemory { - targetMemory = c.maxMemory - } - amountToAdd := targetMemory - c.currentMemory - - usagePct := float64(c.lastMemoryUsage) / float64(c.currentMemory) * 100.0 - freeMemory := c.currentMemory - c.lastMemoryUsage + return c.resize(ctx, c.clamp(c.currentMemory+c.config.IncrementSize), "up") +} - log.G(ctx).WithFields(log.Fields{ - fieldContainerID: c.containerID, - "current_memory_mb": c.currentMemory / (1024 * 1024), - "target_memory_mb": targetMemory / (1024 * 1024), - "add_mb": amountToAdd / (1024 * 1024), - "slot_id": slotID, - "usage_pct": fmt.Sprintf("%.2f", usagePct), - "free_mb": freeMemory / (1024 * 1024), - }).Info("memory-hotplug: scaling up memory") +// ScaleDown implements hotplug.ResourceScaler +func (c *Controller) ScaleDown(ctx context.Context) error { + return c.resize(ctx, c.clamp(c.currentMemory-c.config.IncrementSize), "down") +} - // Hotplug memory via QMP - if err := c.qmpClient.HotplugMemory(ctx, slotID, amountToAdd); err != nil { - return fmt.Errorf("failed to hotplug memory: %w", err) +// clamp keeps a target inside the boot size and the ceiling. Both are hard: the +// boot size is the memory a template was frozen with, and the ceiling is what the +// virtio-mem device was created able to hand out. +func (c *Controller) clamp(target int64) int64 { + if target > c.maxMemory { + return c.maxMemory } - - // Mark slot as used - c.usedSlots[slotID] = true - - // Online memory in guest - required for memory to be usable - if err := c.onlineMemory(ctx, slotID); err != nil { - log.G(ctx).WithError(err).WithField("slot_id", slotID). - Error("memory-hotplug: failed to online memory in guest") - // Memory was allocated via QMP but is not usable by guest - // Try to unplug it to avoid wasting resources - if unplugErr := c.qmpClient.UnplugMemory(ctx, slotID); unplugErr != nil { - log.G(ctx).WithError(unplugErr).WithField("slot_id", slotID). - Warn("memory-hotplug: failed to unplug unusable memory") - } - delete(c.usedSlots, slotID) - return fmt.Errorf("memory allocated but failed to online in guest: %w", err) + if target < c.bootMemory { + return c.bootMemory } - - c.currentMemory = targetMemory - return nil + return target } -// ScaleDown implements hotplug.ResourceScaler -func (c *Controller) ScaleDown(ctx context.Context) error { +// resize asks the machine for a total size and records what arrived. +// +// Growing and shrinking are the same call, which is the point of virtio-mem, and +// it is why there is one function here where there were two: the device is on the +// command line from the start, its size is a property, and setting that property +// is the whole operation in both directions. +// +// What arrived is read back rather than assumed. A request is a negotiation: the +// guest accepts memory in blocks, and on the way down the device can only take +// back what the guest has released, so asking for less than the guest is using +// is not an error and simply does not happen. Recording the request instead +// would leave this believing in memory the VM does not have β€” and the next +// decision is made against that number. +func (c *Controller) resize(ctx context.Context, target int64, direction string) error { c.mu.Lock() defer c.mu.Unlock() - // Find used slot to remove (last added, LIFO) - slotID := c.findUsedSlot() - if slotID < 0 { - return fmt.Errorf("no used memory slots to remove") - } - - targetMemory := c.currentMemory - c.config.IncrementSize - if targetMemory < c.bootMemory { - targetMemory = c.bootMemory + if target == c.currentMemory { + return nil } - amountToRemove := c.currentMemory - targetMemory usagePct := float64(c.lastMemoryUsage) / float64(c.currentMemory) * 100.0 - projectedFree := targetMemory - c.lastMemoryUsage - log.G(ctx).WithFields(log.Fields{ fieldContainerID: c.containerID, "current_memory_mb": c.currentMemory / (1024 * 1024), - "target_memory_mb": targetMemory / (1024 * 1024), - "remove_mb": amountToRemove / (1024 * 1024), - "slot_id": slotID, + "target_memory_mb": target / (1024 * 1024), "usage_pct": fmt.Sprintf("%.2f", usagePct), - "projected_free_mb": projectedFree / (1024 * 1024), - }).Info("memory-hotplug: scaling down memory") - - // Offline memory in guest first - if err := c.offlineMemory(ctx, slotID); err != nil { - log.G(ctx).WithError(err).WithField("slot_id", slotID). - Warn("memory-hotplug: failed to offline memory in guest") - return fmt.Errorf("failed to offline memory: %w", err) - } + "free_mb": (c.currentMemory - c.lastMemoryUsage) / (1024 * 1024), + }).Info("memory-hotplug: scaling memory " + direction) - // Unplug memory via QMP - if err := c.qmpClient.UnplugMemory(ctx, slotID); err != nil { - // Try to bring memory back online if unplug failed - if onlineErr := c.onlineMemory(ctx, slotID); onlineErr != nil { - log.G(ctx).WithError(onlineErr).WithField("slot_id", slotID). - Error("memory-hotplug: CRITICAL - failed to re-online memory after unplug failure, guest may have offline memory") - } - return fmt.Errorf("failed to unplug memory: %w", err) + plugged, err := c.qmpClient.SetPluggedMemory(ctx, target-c.bootMemory) + if err != nil { + return fmt.Errorf("resizing memory to %d bytes: %w", target, err) } - // Mark slot as free - delete(c.usedSlots, slotID) - c.currentMemory = targetMemory - + c.currentMemory = c.bootMemory + plugged + if c.currentMemory != target { + // Not a failure. The guest has not released what was asked for yet, or has + // not taken all of what was offered; the next cycle asks again with the + // same thresholds against the size that actually exists. + log.G(ctx).WithFields(log.Fields{ + fieldContainerID: c.containerID, + "target_memory_mb": target / (1024 * 1024), + "actual_memory_mb": c.currentMemory / (1024 * 1024), + }).Debug("memory-hotplug: the guest has not settled on the requested size") + } return nil } @@ -404,23 +350,3 @@ func (c *Controller) sampleMemory(ctx context.Context) (float64, bool, error) { return usagePct, true, nil } - -// findFreeSlot finds the first available memory slot -func (c *Controller) findFreeSlot() int { - for i := range c.config.MaxSlots { - if !c.usedSlots[i] { - return i - } - } - return -1 -} - -// findUsedSlot finds a used memory slot (LIFO - last added first) -func (c *Controller) findUsedSlot() int { - for i := c.config.MaxSlots - 1; i >= 0; i-- { - if c.usedSlots[i] { - return i - } - } - return -1 -} diff --git a/internal/shim/memhotplug/controller_test.go b/internal/shim/memhotplug/controller_test.go index d3c03e2..72f0e96 100644 --- a/internal/shim/memhotplug/controller_test.go +++ b/internal/shim/memhotplug/controller_test.go @@ -12,40 +12,32 @@ import ( // mockQMPClient simulates QEMU QMP client for testing type mockQMPClient struct { - mu sync.Mutex - baseMemory int64 - pluggedMemory int64 - hotplugErr error - unplugErr error - querySummaryErr error - hotplugCallCount int - unplugCallCount int + mu sync.Mutex + baseMemory int64 + pluggedMemory int64 + resizeErr error + querySummaryErr error + resizeCallCount int + // grantedFloor is the least the guest will give back, in bytes above the boot + // size. A virtio-mem device can only unplug what the guest has released, so a + // request below this settles here instead of where it was aimed β€” which is + // the case the controller has to survive without believing the number it + // asked for. + grantedFloor int64 } -func (m *mockQMPClient) HotplugMemory(ctx context.Context, slotID int, sizeBytes int64) error { +func (m *mockQMPClient) SetPluggedMemory(ctx context.Context, sizeBytes int64) (int64, error) { m.mu.Lock() defer m.mu.Unlock() - m.hotplugCallCount++ - if m.hotplugErr != nil { - return m.hotplugErr + m.resizeCallCount++ + if m.resizeErr != nil { + return 0, m.resizeErr } - m.pluggedMemory += sizeBytes - return nil -} - -func (m *mockQMPClient) UnplugMemory(ctx context.Context, slotID int) error { - m.mu.Lock() - defer m.mu.Unlock() - m.unplugCallCount++ - if m.unplugErr != nil { - return m.unplugErr - } - // Assume each slot is 128MB - m.pluggedMemory -= 128 * 1024 * 1024 - if m.pluggedMemory < 0 { - m.pluggedMemory = 0 + if sizeBytes < m.grantedFloor { + sizeBytes = m.grantedFloor } - return nil + m.pluggedMemory = sizeBytes + return m.pluggedMemory, nil } func (m *mockQMPClient) QueryMemorySizeSummary(ctx context.Context) (*qemu.MemorySizeSummary, error) { @@ -76,33 +68,6 @@ func (m *mockStatsProvider) getStats(ctx context.Context) (int64, error) { return m.usageBytes, nil } -// mockMemoryManager simulates guest memory online/offline -type mockMemoryManager struct { - mu sync.Mutex - offlineErr error - onlineErr error - offlineCalls int - onlineCalls int - offlineIDs []int - onlineIDs []int -} - -func (m *mockMemoryManager) offline(ctx context.Context, memoryID int) error { - m.mu.Lock() - defer m.mu.Unlock() - m.offlineCalls++ - m.offlineIDs = append(m.offlineIDs, memoryID) - return m.offlineErr -} - -func (m *mockMemoryManager) online(ctx context.Context, memoryID int) error { - m.mu.Lock() - defer m.mu.Unlock() - m.onlineCalls++ - m.onlineIDs = append(m.onlineIDs, memoryID) - return m.onlineErr -} - func TestDefaultConfig(t *testing.T) { config := DefaultConfig() @@ -133,7 +98,6 @@ func TestNewController(t *testing.T) { baseMemory: 512 * 1024 * 1024, } mockStats := &mockStatsProvider{} - mockMem := &mockMemoryManager{} config := DefaultConfig() config.MonitorInterval = 100 * time.Millisecond // Fast for testing @@ -142,8 +106,6 @@ func TestNewController(t *testing.T) { "test-container", mockQMP, mockStats.getStats, - mockMem.offline, - mockMem.online, 512*1024*1024, // boot memory 1024*1024*1024, // max memory config, @@ -174,7 +136,6 @@ func TestNewControllerNoopWhenNoHotplug(t *testing.T) { baseMemory: 512 * 1024 * 1024, } mockStats := &mockStatsProvider{} - mockMem := &mockMemoryManager{} config := DefaultConfig() @@ -183,8 +144,6 @@ func TestNewControllerNoopWhenNoHotplug(t *testing.T) { "test-container", mockQMP, mockStats.getStats, - mockMem.offline, - mockMem.online, 512*1024*1024, // boot memory 512*1024*1024, // max memory (same as boot) config, @@ -215,7 +174,6 @@ func TestControllerScaleUp(t *testing.T) { mockStats := &mockStatsProvider{ usageBytes: 450 * 1024 * 1024, // 450MB of 512MB = 87.9% usage } - mockMem := &mockMemoryManager{} config := DefaultConfig() config.MonitorInterval = 50 * time.Millisecond @@ -226,8 +184,6 @@ func TestControllerScaleUp(t *testing.T) { "test-container", mockQMP, mockStats.getStats, - mockMem.offline, - mockMem.online, 512*1024*1024, // boot memory 1024*1024*1024, // max memory config, @@ -244,20 +200,12 @@ func TestControllerScaleUp(t *testing.T) { controller.Stop() mockQMP.mu.Lock() - hotplugCalls := mockQMP.hotplugCallCount + hotplugCalls := mockQMP.resizeCallCount mockQMP.mu.Unlock() if hotplugCalls == 0 { t.Error("expected at least one hotplug call due to high memory usage") } - - mockMem.mu.Lock() - onlineCalls := mockMem.onlineCalls - mockMem.mu.Unlock() - - if onlineCalls == 0 { - t.Error("expected at least one online call after hotplug") - } } func TestControllerNoScaleUpBelowThreshold(t *testing.T) { @@ -267,7 +215,6 @@ func TestControllerNoScaleUpBelowThreshold(t *testing.T) { mockStats := &mockStatsProvider{ usageBytes: 300 * 1024 * 1024, // 300MB of 512MB = 58.6% usage (below 85%) } - mockMem := &mockMemoryManager{} config := DefaultConfig() config.MonitorInterval = 50 * time.Millisecond @@ -276,8 +223,6 @@ func TestControllerNoScaleUpBelowThreshold(t *testing.T) { "test-container", mockQMP, mockStats.getStats, - mockMem.offline, - mockMem.online, 512*1024*1024, // boot memory 1024*1024*1024, // max memory config, @@ -291,7 +236,7 @@ func TestControllerNoScaleUpBelowThreshold(t *testing.T) { controller.Stop() mockQMP.mu.Lock() - hotplugCalls := mockQMP.hotplugCallCount + hotplugCalls := mockQMP.resizeCallCount mockQMP.mu.Unlock() if hotplugCalls > 0 { @@ -307,7 +252,6 @@ func TestControllerScaleDown(t *testing.T) { mockStats := &mockStatsProvider{ usageBytes: 200 * 1024 * 1024, // 200MB of 640MB = 31.25% usage (below 60%) } - mockMem := &mockMemoryManager{} config := DefaultConfig() config.MonitorInterval = 50 * time.Millisecond @@ -319,8 +263,6 @@ func TestControllerScaleDown(t *testing.T) { "test-container", mockQMP, mockStats.getStats, - mockMem.offline, - mockMem.online, 512*1024*1024, // boot memory 768*1024*1024, // max memory config, @@ -332,7 +274,6 @@ func TestControllerScaleDown(t *testing.T) { t.Fatal("NewController returned non-Controller implementation") } ctrl.currentMemory = 640 * 1024 * 1024 // Set current memory to include plugged - ctrl.usedSlots[0] = true // Mark slot 0 as used ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel() @@ -345,20 +286,12 @@ func TestControllerScaleDown(t *testing.T) { controller.Stop() mockQMP.mu.Lock() - unplugCalls := mockQMP.unplugCallCount + unplugCalls := mockQMP.resizeCallCount mockQMP.mu.Unlock() if unplugCalls == 0 { t.Error("expected at least one unplug call due to low memory usage") } - - mockMem.mu.Lock() - offlineCalls := mockMem.offlineCalls - mockMem.mu.Unlock() - - if offlineCalls == 0 { - t.Error("expected at least one offline call before unplug") - } } func TestControllerScaleDownDisabled(t *testing.T) { @@ -369,7 +302,6 @@ func TestControllerScaleDownDisabled(t *testing.T) { mockStats := &mockStatsProvider{ usageBytes: 200 * 1024 * 1024, // Low usage } - mockMem := &mockMemoryManager{} config := DefaultConfig() config.MonitorInterval = 50 * time.Millisecond @@ -379,8 +311,6 @@ func TestControllerScaleDownDisabled(t *testing.T) { "test-container", mockQMP, mockStats.getStats, - mockMem.offline, - mockMem.online, 512*1024*1024, 768*1024*1024, config, @@ -394,7 +324,7 @@ func TestControllerScaleDownDisabled(t *testing.T) { controller.Stop() mockQMP.mu.Lock() - unplugCalls := mockQMP.unplugCallCount + unplugCalls := mockQMP.resizeCallCount mockQMP.mu.Unlock() if unplugCalls > 0 { @@ -410,7 +340,6 @@ func TestControllerOOMSafetyMargin(t *testing.T) { mockStats := &mockStatsProvider{ usageBytes: 450 * 1024 * 1024, } - mockMem := &mockMemoryManager{} config := DefaultConfig() config.MonitorInterval = 50 * time.Millisecond @@ -421,8 +350,6 @@ func TestControllerOOMSafetyMargin(t *testing.T) { "test-container", mockQMP, mockStats.getStats, - mockMem.offline, - mockMem.online, 512*1024*1024, 1024*1024*1024, config, @@ -436,7 +363,7 @@ func TestControllerOOMSafetyMargin(t *testing.T) { controller.Stop() mockQMP.mu.Lock() - hotplugCalls := mockQMP.hotplugCallCount + hotplugCalls := mockQMP.resizeCallCount mockQMP.mu.Unlock() if hotplugCalls == 0 { @@ -451,7 +378,6 @@ func TestControllerMaxMemoryLimit(t *testing.T) { mockStats := &mockStatsProvider{ usageBytes: 500 * 1024 * 1024, // Very high usage } - mockMem := &mockMemoryManager{} config := DefaultConfig() config.MonitorInterval = 50 * time.Millisecond @@ -462,8 +388,6 @@ func TestControllerMaxMemoryLimit(t *testing.T) { "test-container", mockQMP, mockStats.getStats, - mockMem.offline, - mockMem.online, 512*1024*1024, // boot memory 512*1024*1024, // max memory (same as boot, no hotplug possible) config, @@ -477,7 +401,7 @@ func TestControllerMaxMemoryLimit(t *testing.T) { controller.Stop() mockQMP.mu.Lock() - hotplugCalls := mockQMP.hotplugCallCount + hotplugCalls := mockQMP.resizeCallCount mockQMP.mu.Unlock() if hotplugCalls > 0 { @@ -488,12 +412,11 @@ func TestControllerMaxMemoryLimit(t *testing.T) { func TestControllerErrorHandling(t *testing.T) { mockQMP := &mockQMPClient{ baseMemory: 512 * 1024 * 1024, - hotplugErr: errors.New("simulated hotplug error"), + resizeErr: errors.New("simulated resize error"), } mockStats := &mockStatsProvider{ usageBytes: 450 * 1024 * 1024, // High usage to trigger scale-up } - mockMem := &mockMemoryManager{} config := DefaultConfig() config.MonitorInterval = 50 * time.Millisecond @@ -503,8 +426,6 @@ func TestControllerErrorHandling(t *testing.T) { "test-container", mockQMP, mockStats.getStats, - mockMem.offline, - mockMem.online, 512*1024*1024, 1024*1024*1024, config, @@ -518,87 +439,39 @@ func TestControllerErrorHandling(t *testing.T) { time.Sleep(300 * time.Millisecond) controller.Stop() - // Test should pass if controller doesn't crash + // Test should pass if controller does not crash } -func TestFindFreeSlot(t *testing.T) { - tests := []struct { - name string - usedSlots map[int]bool - want int - }{ - { - name: "all slots free", - usedSlots: map[int]bool{}, - want: 0, - }, - { - name: "first slot used", - usedSlots: map[int]bool{0: true}, - want: 1, - }, - { - name: "first two slots used", - usedSlots: map[int]bool{0: true, 1: true}, - want: 2, - }, - { - name: "all slots used", - usedSlots: map[int]bool{0: true, 1: true, 2: true, 3: true, 4: true, 5: true, 6: true, 7: true}, - want: -1, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - controller := &Controller{ - usedSlots: tt.usedSlots, - config: Config{MaxSlots: 8}, - } - if got := controller.findFreeSlot(); got != tt.want { - t.Errorf("findFreeSlot() = %d, want %d", got, tt.want) - } - }) - } -} - -func TestFindUsedSlot(t *testing.T) { - tests := []struct { - name string - usedSlots map[int]bool - want int - }{ - { - name: "no slots used", - usedSlots: map[int]bool{}, - want: -1, - }, - { - name: "single slot used", - usedSlots: map[int]bool{3: true}, - want: 3, - }, - { - name: "multiple slots used returns highest", - usedSlots: map[int]bool{2: true, 5: true}, - want: 5, - }, - { - name: "first slot only", - usedSlots: map[int]bool{0: true}, - want: 0, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - controller := &Controller{ - usedSlots: tt.usedSlots, - config: Config{MaxSlots: 8}, - } - if got := controller.findUsedSlot(); got != tt.want { - t.Errorf("findUsedSlot() = %d, want %d", got, tt.want) - } - }) +// TestResizeBelievesTheDeviceAndNotTheRequest is the test the virtio-mem switch +// exists for. +// +// A request is a negotiation: the device plugs memory as the guest accepts it, +// and unplugs only what the guest has already released. So asking for a size and +// recording it is wrong in a way nothing reports β€” the controller would go on +// believing in memory the VM does not have, and every threshold after that is +// computed against a number that is not real. +func TestResizeBelievesTheDeviceAndNotTheRequest(t *testing.T) { + const boot = 512 * 1024 * 1024 + const granted = 256 * 1024 * 1024 + + // The guest will not give back below 256 MB of the 512 that were plugged. + mockQMP := &mockQMPClient{baseMemory: boot, pluggedMemory: 512 * 1024 * 1024, grantedFloor: granted} + c := &Controller{ + containerID: "test-container", + qmpClient: mockQMP, + bootMemory: boot, + maxMemory: 4 * boot, + currentMemory: boot + 512*1024*1024, + config: DefaultConfig(), + } + + // Aim all the way back at the boot size, which the guest will not allow. + if err := c.resize(context.Background(), boot, "down"); err != nil { + t.Fatalf("resize: %v", err) + } + + if want := int64(boot + granted); c.currentMemory != want { + t.Errorf("currentMemory = %d, want %d β€” the controller recorded what it asked for, not what it got", + c.currentMemory, want) } } diff --git a/internal/shim/platform/mounts/linux.go b/internal/shim/platform/mounts/linux.go index 8b4d588..e57850b 100644 --- a/internal/shim/platform/mounts/linux.go +++ b/internal/shim/platform/mounts/linux.go @@ -142,7 +142,7 @@ type diskOptions struct { source string serial string readOnly bool - vmdk bool + format string } // blockSerial returns the virtio-blk serial for the disk at the given slot @@ -246,12 +246,16 @@ func (m *linuxManager) handleEROFS(_ context.Context, id string, disks *byte, mn } serial := blockSerial(*disks) + format := vm.DefaultDiskFormat + if isVMDK { + format = "vmdk" + } addDisks := []diskOptions{{ name: disk, source: source, serial: serial, readOnly: true, - vmdk: isVMDK, + format: format, }} // When using VMDK, the guest doesn't need device= options - it's a single device. @@ -298,7 +302,7 @@ func (m *linuxManager) handleExt4(id string, disks *byte, mnt *types.Mount) ([]* source: mnt.Source, serial: serial, readOnly: readOnly, - vmdk: false, + format: vm.DefaultDiskFormat, }} return []*types.Mount{out}, addDisks, nil } @@ -435,8 +439,8 @@ func (m *linuxManager) addDisksToVM(ctx context.Context, vmi vm.Instance, disks if do.readOnly { opts = append(opts, vm.WithReadOnly()) } - if do.vmdk { - opts = append(opts, vm.WithVmdk()) + if do.format != "" { + opts = append(opts, vm.WithFormat(do.format)) } if do.serial != "" { opts = append(opts, vm.WithSerial(do.serial)) diff --git a/internal/shim/platform/mounts/linux_test.go b/internal/shim/platform/mounts/linux_test.go index d1f478a..c45c8b3 100644 --- a/internal/shim/platform/mounts/linux_test.go +++ b/internal/shim/platform/mounts/linux_test.go @@ -14,6 +14,8 @@ import ( "github.com/containerd/errdefs" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/spin-stack/spinbox/internal/host/vm" ) func TestTransformMounts_ExceedsDiskCap(t *testing.T) { @@ -429,7 +431,7 @@ func TestHandleEROFS(t *testing.T) { assert.Equal(t, "/path/to/image.erofs", diskOpts[0].source) assert.True(t, diskOpts[0].readOnly) - assert.False(t, diskOpts[0].vmdk) + assert.Equal(t, vm.DefaultDiskFormat, diskOpts[0].format) assert.Equal(t, byte('b'), disks) // The disk serial must match the serial encoded in the guest mount Source // so the guest can resolve the device via /sys/block//serial. @@ -449,7 +451,7 @@ func TestHandleEROFS(t *testing.T) { require.NoError(t, err) require.Len(t, diskOpts, 1) - assert.True(t, diskOpts[0].vmdk) + assert.Equal(t, "vmdk", diskOpts[0].format) }) t.Run("filters device options", func(t *testing.T) { diff --git a/internal/shim/resources/hotplug.go b/internal/shim/resources/hotplug.go index 793897b..e3c684b 100644 --- a/internal/shim/resources/hotplug.go +++ b/internal/shim/resources/hotplug.go @@ -23,8 +23,6 @@ type HotplugCallbacks struct { OfflineCPU func(ctx context.Context, cpuID int) error OnlineCPU func(ctx context.Context, cpuID int) error GetMemoryStats func(ctx context.Context, containerID string) (int64, error) - OfflineMemory func(ctx context.Context, memoryID int) error - OnlineMemory func(ctx context.Context, memoryID int) error } // StartCPUHotplug starts the CPU hotplug controller for QEMU VMs. @@ -218,8 +216,6 @@ func StartMemoryHotplug( func(ctx context.Context) (int64, error) { return callbacks.GetMemoryStats(ctx, containerID) }, - callbacks.OfflineMemory, - callbacks.OnlineMemory, resourceCfg.MemorySize, resourceCfg.MemoryHotplugSize, memConfig, @@ -257,12 +253,6 @@ func CreateVMClientCallbacks(dialClient func(context.Context) (*ttrpc.Client, er GetMemoryStats: func(ctx context.Context, containerID string) (int64, error) { return getMemoryStats(ctx, dialClient, containerID) }, - OfflineMemory: func(ctx context.Context, memoryID int) error { - return offlineMemory(ctx, dialClient, memoryID) - }, - OnlineMemory: func(ctx context.Context, memoryID int) error { - return onlineMemory(ctx, dialClient, memoryID) - }, } } diff --git a/internal/shim/resources/vmclient.go b/internal/shim/resources/vmclient.go index 941f078..9cb1d4f 100644 --- a/internal/shim/resources/vmclient.go +++ b/internal/shim/resources/vmclient.go @@ -103,30 +103,12 @@ func getMemoryStats(ctx context.Context, dialClient func(context.Context) (*ttrp return int64(mem.GetUsage()), nil } -// offlineMemory takes memory offline in the guest VM. +// The two calls that used to be here β€” offlineMemory and onlineMemory β€” are +// gone with the DIMM slots they served. virtio-mem hands memory to a guest that +// onlines it itself (memhp_default_state=online) and takes back only what the +// guest has already released, so there is nothing for the host to ask the guest +// to do in either direction. // -// The dialClient function should return a managed TTRPC client. The caller -// (ConnectionManager) owns the client lifecycle. -func offlineMemory(ctx context.Context, dialClient func(context.Context) (*ttrpc.Client, error), memoryID int) error { - vmc, err := dialClient(ctx) - if err != nil { - return err - } - client := systemAPI.NewTTRPCSystemClient(vmc) - _, err = client.OfflineMemory(ctx, &systemAPI.OfflineMemoryRequest{MemoryID: uint32(memoryID)}) - return err -} - -// onlineMemory brings memory online in the guest VM. -// -// The dialClient function should return a managed TTRPC client. The caller -// (ConnectionManager) owns the client lifecycle. -func onlineMemory(ctx context.Context, dialClient func(context.Context) (*ttrpc.Client, error), memoryID int) error { - vmc, err := dialClient(ctx) - if err != nil { - return err - } - client := systemAPI.NewTTRPCSystemClient(vmc) - _, err = client.OnlineMemory(ctx, &systemAPI.OnlineMemoryRequest{MemoryID: uint32(memoryID)}) - return err -} +// The System service still carries the two RPCs and the guest still implements +// them. Removing them is a proto change and belongs in its own commit; nothing +// on this side calls them.