From 3d53bb2c514f6571b7f7001d78848b987f6f1a00 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Mon, 7 Sep 2026 23:10:39 -0300 Subject: [PATCH 1/4] One definition of the machine, imported instead of spelled again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit spinbox kept its own copy of what a VM is: a command-line builder, a kernel command line, and a MachineIdentity beside them that spelled the same four arguments a second time so a template's fingerprint could be computed before there was a command line to read. 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. There is one machine.Spec now. The command line is Args() of it and the fingerprint is Fingerprint() of it, so the two cannot disagree because there is nothing left to disagree. The two definitions had already drifted, in both directions. Ours hashed runtime.GOARCH, which cannot differ without the QEMU binary differing. 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. And 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. **Memory grows through virtio-mem.** The `-m` line has no slots any more, so the pc-dimm path went with them: a backend object and a device per step, a table of which of the eight 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. One qom-set replaces it. The guest onlines by itself (memhp_default_state=online) and shrinking is the same call with a smaller number. What the device reports is read back rather than assumed: a request is a negotiation, and recording the number we asked for would leave the controller believing in memory the VM does not have. **Every disk is a qcow2.** The format was worked out from the file extension while the command line was being built β€” a guess in the one place nobody looks. There is one format because there is one thing that produces them: a chain with a read-only base many VMs map at once and a read-write tip that is this VM's. **One layout.** The release tree is the layout everywhere now, so hack/spin-machine copies instead of rearranging, and internal/paths stops guessing: no candidate lists ending in /usr/bin, no second kernel name. A host with no release used to run *a* QEMU, with different devices and a different fingerprint, and find out by way of a guest that would not start. Deleted rather than adapted: qemu_command_test.go and the fingerprint and machineShape tests, which now test machine.Spec upstream and better; and qmp_memory_test.go, whose every test was either a 128 MB alignment rule that no longer exists or a t.Skip("manual integration test") that has never run. Verified: task lint, go test -race ./..., and hack/spin-machine against a sibling checkout. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019GHRRFvbnZi5q3KSTvyW5a --- Taskfile.yml | 6 +- go.mod | 1 + go.sum | 6 + hack/release | 14 +- hack/spin-machine | 36 +- internal/config/config_test.go | 57 +- internal/config/validation.go | 40 +- internal/host/vm/qemu/instance.go | 26 +- internal/host/vm/qemu/kernel_cmdline.go | 190 ++---- internal/host/vm/qemu/qemu_command.go | 402 ------------- internal/host/vm/qemu/qemu_command_test.go | 621 -------------------- internal/host/vm/qemu/qmp_client.go | 15 + internal/host/vm/qemu/qmp_memory.go | 138 ++--- internal/host/vm/qemu/qmp_memory_test.go | 340 ----------- internal/host/vm/qemu/snapshot_test.go | 41 +- internal/host/vm/qemu/spec.go | 173 ++++++ internal/host/vm/qemu/start.go | 135 +---- internal/host/vm/qemu/template.go | 255 +------- internal/host/vm/qemu/template_build.go | 6 +- internal/host/vm/qemu/template_cache.go | 36 +- internal/host/vm/qemu/template_test.go | 245 +------- internal/paths/paths.go | 133 ++--- internal/paths/paths_test.go | 384 ++---------- internal/shim/memhotplug/controller.go | 220 +++---- internal/shim/memhotplug/controller_test.go | 245 ++------ internal/shim/resources/hotplug.go | 10 - internal/shim/resources/vmclient.go | 34 +- 27 files changed, 724 insertions(+), 3085 deletions(-) delete mode 100644 internal/host/vm/qemu/qemu_command.go delete mode 100644 internal/host/vm/qemu/qemu_command_test.go delete mode 100644 internal/host/vm/qemu/qmp_memory_test.go create mode 100644 internal/host/vm/qemu/spec.go diff --git a/Taskfile.yml b/Taskfile.yml index 330959c..2d10804 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -253,10 +253,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..8909fdf 100644 --- a/go.mod +++ b/go.mod @@ -65,6 +65,7 @@ require ( github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/sirupsen/logrus v1.9.4 // indirect + github.com/spin-stack/spin-machine v0.0.0-20260908015926-f9a7f19e4cf4 // indirect go.opencensus.io v0.24.0 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.68.0 // indirect diff --git a/go.sum b/go.sum index 8ff5376..62dbb3e 100644 --- a/go.sum +++ b/go.sum @@ -173,6 +173,12 @@ 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-20260908013732-8f43fedeb2ed h1:xJ89vnviFV0pMKYASEqFkkP55jPhnJ6Mm41Ni5OeOqo= +github.com/spin-stack/spin-machine v0.0.0-20260908013732-8f43fedeb2ed/go.mod h1:VbqCcwvAK1lcazIf6rSli6qLrgLU5f22CutMng1pRbc= +github.com/spin-stack/spin-machine v0.0.0-20260908015733-8e2ca54f55d6 h1:X7S4i2VWBA26q6VwFycIvQFfH48c/3wvLCNMbMc0SKU= +github.com/spin-stack/spin-machine v0.0.0-20260908015733-8e2ca54f55d6/go.mod h1:VbqCcwvAK1lcazIf6rSli6qLrgLU5f22CutMng1pRbc= +github.com/spin-stack/spin-machine v0.0.0-20260908015926-f9a7f19e4cf4 h1:rgHq/6Abl/HeHGLfQeW80uXBG3V2KNU6BbKNSj+KAqc= +github.com/spin-stack/spin-machine v0.0.0-20260908015926-f9a7f19e4cf4/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/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..59eaf1e --- /dev/null +++ b/internal/host/vm/qemu/spec.go @@ -0,0 +1,173 @@ +//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, + // qcow2, always, and stated rather than worked out from the filename. + // + // Every disk a guest gets here is one layer of a qcow2 chain: a + // read-only base that many VMs map at once, and a read-write tip that + // is this VM's. There is one format because there is one thing that + // produces them. + // + // It is spelled and not probed. Letting QEMU work out the format of a + // file the guest can write is how an image is talked into being read as + // another one, and a wrong answer is not an error β€” it is a guest that + // boots and finds a disk full of nothing. + Format: "qcow2", + 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_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/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/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. From f5f8da607d08ccee70d078aeaadd6b2cfc948420 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Mon, 7 Sep 2026 23:16:10 -0300 Subject: [PATCH 2/4] Pin the machine at the commit CI proved, and make the dependency direct go.mod carried spin-machine as indirect while three files import it; `go mod tidy` says what is true. The pin is a branch commit again and not a tag, for the reason the last one was: spin-stack/spin-machine#2 carries Spec.Identity, which this repository's template cache keys on, and which the merge of #1 did not take. It moves to a release once that lands. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019GHRRFvbnZi5q3KSTvyW5a --- go.mod | 2 +- go.sum | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index 8909fdf..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 @@ -65,7 +66,6 @@ require ( github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/sirupsen/logrus v1.9.4 // indirect - github.com/spin-stack/spin-machine v0.0.0-20260908015926-f9a7f19e4cf4 // indirect go.opencensus.io v0.24.0 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.68.0 // indirect diff --git a/go.sum b/go.sum index 62dbb3e..6d5fb27 100644 --- a/go.sum +++ b/go.sum @@ -173,12 +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-20260908013732-8f43fedeb2ed h1:xJ89vnviFV0pMKYASEqFkkP55jPhnJ6Mm41Ni5OeOqo= -github.com/spin-stack/spin-machine v0.0.0-20260908013732-8f43fedeb2ed/go.mod h1:VbqCcwvAK1lcazIf6rSli6qLrgLU5f22CutMng1pRbc= -github.com/spin-stack/spin-machine v0.0.0-20260908015733-8e2ca54f55d6 h1:X7S4i2VWBA26q6VwFycIvQFfH48c/3wvLCNMbMc0SKU= -github.com/spin-stack/spin-machine v0.0.0-20260908015733-8e2ca54f55d6/go.mod h1:VbqCcwvAK1lcazIf6rSli6qLrgLU5f22CutMng1pRbc= -github.com/spin-stack/spin-machine v0.0.0-20260908015926-f9a7f19e4cf4 h1:rgHq/6Abl/HeHGLfQeW80uXBG3V2KNU6BbKNSj+KAqc= -github.com/spin-stack/spin-machine v0.0.0-20260908015926-f9a7f19e4cf4/go.mod h1:VbqCcwvAK1lcazIf6rSli6qLrgLU5f22CutMng1pRbc= +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= From dcade02cf6bba545170ffa6de0d0286f8b2091d4 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Mon, 7 Sep 2026 23:26:17 -0300 Subject: [PATCH 3/4] The disk format is stated by whoever has the disk, not guessed and not assumed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hard-coding qcow2 was wrong today. It is where the disks are going β€” one chain, a read-only base many VMs map and a read-write tip that is this VM's β€” and it is not where they come from: containerd's snapshotter hands this shim a merged.vmdk and a raw rwlayer.img, and QEMU refused the first of them outright. qemu-system-x86_64: -drive file=…/merged.vmdk,…,format=qcow2: Image is not in qcow2 format The guess it replaced was no better β€” the format was worked out from the file extension while the command line was being built, by code that had been handed the answer and dropped it: `MountConfig.Vmdk` was already set by the one party that knows, and `addDisk` ignored it. So the option says the format instead of saying a format's name: WithFormat replaces WithVmdk, DiskConfig carries it, and vm.DefaultDiskFormat is what a disk is when nobody says otherwise. That constant is raw, and turning it into qcow2 is the whole of the change when the disks stop coming from a snapshotter. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019GHRRFvbnZi5q3KSTvyW5a --- internal/host/vm/qemu/devices.go | 6 +++++ internal/host/vm/qemu/spec.go | 18 +++++--------- internal/host/vm/qemu/types.go | 3 +++ internal/host/vm/vm.go | 26 +++++++++++++++++---- internal/shim/platform/mounts/linux.go | 14 +++++++---- internal/shim/platform/mounts/linux_test.go | 6 +++-- 6 files changed, 50 insertions(+), 23 deletions(-) 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/spec.go b/internal/host/vm/qemu/spec.go index 59eaf1e..0dd8c5a 100644 --- a/internal/host/vm/qemu/spec.go +++ b/internal/host/vm/qemu/spec.go @@ -139,18 +139,12 @@ func (q *Instance) spec(cmdline string) (machine.Spec, error) { for _, d := range q.disks { s.Disks = append(s.Disks, machine.Disk{ Path: d.Path, - // qcow2, always, and stated rather than worked out from the filename. - // - // Every disk a guest gets here is one layer of a qcow2 chain: a - // read-only base that many VMs map at once, and a read-write tip that - // is this VM's. There is one format because there is one thing that - // produces them. - // - // It is spelled and not probed. Letting QEMU work out the format of a - // file the guest can write is how an image is talked into being read as - // another one, and a wrong answer is not an error β€” it is a guest that - // boots and finds a disk full of nothing. - Format: "qcow2", + // 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 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/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) { From 486c468119582100aef1cb886c4488bea2dce2ec Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Mon, 7 Sep 2026 23:30:25 -0300 Subject: [PATCH 4/4] Type-check the tagged tests, which nothing was doing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rename left template_build_integration_test.go calling a function that no longer existed. `task lint` was green and `go test ./...` was green, because neither compiles what is behind a build tag; the break surfaced 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. `go vet -tags=integration,e2e` beside the linter. 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 belong to nobody running this today. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019GHRRFvbnZi5q3KSTvyW5a --- Taskfile.yml | 13 +++++++++++++ .../host/vm/qemu/template_build_integration_test.go | 4 ++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/Taskfile.yml b/Taskfile.yml index 2d10804..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: 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)