Repository navigation
review: 5 fixes, docs from hardware, and tests that prove what they name - #302
Merged
Merged
Conversation
CMGS
changed the title
With the meta backend, a SIGTERM during a commit canceled the context that Emit then used, so the committed delete's stop events were never written and nothing could write them later. The file backend ignores the context.
…gree A vm net that timed out on the guest's eject, or died between adding a NIC and recording it, leaves them out of step. A hibernate from there could not wake: restore refused the NIC count on every retry and vm net needs a running VM. The capture now refuses before pausing, while rerunning vm net still converges.
A vm net that commits while a restore waits on the ops lock passed the prefix check, leaving a record with more NICs than the guest and no error. The count now has to match, as on Cloud Hypervisor. A sidecar without MACs still skips it.
… their regressions Drive the GC refusal rows through a seam on the real errno, read the CPU count from a path a test owns, run the start-all row through StartSequence with a live VMM, and stop calling t.Fatal off the test goroutine.
The sh fixture forks and its child shares the cmdline, so under load the scan saw two pids or none: 22 of 60 runs failed with the old fixture under 48 CPU spinners, 0 of 60 with this one.
Vary the fields an unchanged pass must ignore, assert the converge timestamp, take one flock handle per holder so the mutual exclusion is real, and remove three sparse-copy duplicates.
…parse the FAT image DetachedValues never committed on the json model, a worker that acked nothing passed the kill storm, TestCorruptBothGenerations tore only one generation, and the cidata tests searched raw bytes.
…ors in snapshot fixtures uuidV5 uses the URL namespace and start.sh the DNS bytes, so neither matches the other or the comment that said so; nothing depends on them matching.
…without MACs omitempty dropped an empty nic_macs, so a zero-NIC snapshot read back as nil and skipped the count check: a vm net add that committed while the restore waited on the ops lock left a record with one NIC and a guest with none, and exited 0. omitzero writes the empty list. Sidecars from before v0.7.6 have no key and still skip it.
A data disk formatted by a newer host mke2fs carries features an older guest e2fsck refuses. With pass number 2 the guest's next boot failed the fsck and systemd-fsck started emergency.target, so the VM came up with no network and no sshd despite nofail. Seen with a Debian 13 host and an Ubuntu 22.04 cloud image on every second boot of a VM with a mounted data disk, the default on cloud images. Pass number 0 leaves the disk to ext4 journal replay at mount.
…g entries and the fsck recovery Each statement comes from a hardware run: when a clone-added disk mounts, how long Windows maintenance holds a shutdown, what a SIGKILL before an emit leaves in the ledger, and how an existing VM recovers from the data disk fsck.
CMGS
force-pushed
the
review/test-and-flow-audit-2026-11-04
branch
from
November 4, 2026 10:57
dcd7451 to
e936738
Compare
CMGS
added a commit
that referenced
this pull request
Nov 5, 2026
#312) * test(meta): exercise the watch-error branch on a synthetic watcher The json engine test sent on fsnotify's own Errors channel, which kqueue closes behind a pipe the race detector cannot see, so go test -race failed on darwin. The branch belongs to Broadcaster; test it there and drop the json notifier's test-only watcher field. * fix(cloudhypervisor): a detach or NIC removal retry re-requests a pending eject vm.remove-device drops the device from the VM config at once but keeps it in device_tree until the guest ejects. A vm net retry after an eject timeout saw no live NIC, deleted the TAP and truncated the record without asking the guest again; fs and device detach retries failed with "not attached". LiveNICs now lists pending NIC ejects unplaced so reconcile re-requests and waits, and fs/device detach find the id in device_tree as disk detach does. * fix(firecracker): capture refuses a live NIC count off its record A vm net add killed between the device PUT and the record append left FC with an unrecorded NIC that a snapshot then captured bound to the source TAP name, so every clone of it failed or reused the source TAP. Cloud Hypervisor refuses this since #302; the FC --pci pause check now does too. * fix(cloudhypervisor): fsync cidata before the create commits its record FinalizeCreate commits the record durably while cidata.img sat in the page cache. A host reboot in that window left an empty or zeroed cidata the next start still attached, so cloud-init silently got no user or network config and nothing regenerates it. Cost: one fsync of a 1 MiB file per cloudimg create or clone. * fix(vm): logs -f no longer reprints a short log head as it grows The truncation check compared the first 64 bytes for equality, so a log shorter than that at open looked truncated on its next append and was copied again from offset 0. A head that still starts with the old one is growth, not truncation. * fix(daemon): shutdown ends open events streams at once Request contexts did not derive from the daemon context, so Shutdown waited the full 2 s timeout whenever a /v1/events subscriber (sandboxd always is) was connected, and the stream handler outlived it. * fix(vm): console exits on signal with a direct-boot PTY f.Fd() switched the CH PTY to blocking mode, so closing it on SIGINT or SIGTERM no longer woke the pending read: the command hung until the guest printed and left the terminal raw. Take the fd through SyscallConn instead. * fix(daemon): api-socket-mode help shows the default in octal The help printed (default 432), the decimal form of 0660, so a user copying the style typed 660 and got mode 0o1224. The flag now defaults to 0, which the daemon already maps to 0660, and the help names the octal form. * fix(snapshot): import stops reading its archive on cancel Every other streaming verb closes its source when the context is canceled; import did not, so SIGINT or SIGTERM waited out the whole extraction. * fix(images): rm reports what an earlier backend deleted before a later one fails With refs owned by both backends, a failure in the second returned before the log loop, so refs the first had already removed were never reported. * fix(vm): status --event aligns its header with the rows The header went straight to stdout, outside the tabwriter that pads the rows, so its columns never lined up. * fix(cni): a failed re-plumb no longer frees recorded NICs' IPs The rollback ran CNI DEL on every NIC it tried, including recorded NICs a start or restore had just rebuilt with their saved IP. That released the IP in IPAM while the records kept it, so a create elsewhere could take it and every later start failed with "requested IP not available". A recorded NIC now only loses its TAP; the next start re-plumbs it. * fix(firecracker): keep the VMM's stderr in its process log Firecracker's stderr went to /dev/null, so an error it prints before its logger starts (an argument it rejects, a bad config) left only the API socket timeout and an empty vm logs. Cloud Hypervisor already writes stderr to the process log. * cut(cmd): drop the hand-written completion command for cobra's default Cobra adds the same completion command when none is defined, with bash completion V2 and --no-descriptions; cocoon completion <shell> keeps working as docs/install.md shows. * review: comments and messages that contradict the code - restore marker: start clears a stale marker since #311, so the FinalizeRestore warning, the restoreDirtyName comment and the apply quarantine comments described a refusal that no longer happens - create, gc, netresize, localfile Delete, types.VM runtime and placement comments rewritten to what the code does - disk name error names the reserved cocoon- prefix it actually rejects - drop design-doc section refs (§N) that point outside the repo, and comments that restate the code * cut(images): drop nil-entry guards no writer can produce Index entries come only from materialize (&e, never nil, a JSON null decodes to a zero entry) and the two backends' commits, and removal uses delete(); the nil checks date from the whole-file index. * cut(types): drop nil-receiver guards on the VM network resolvers Every caller passes a non-nil VM: the embedded record VM, or a FindVM result that is non-nil on success. * cut: drop the nil-config guards in the backend constructors Every caller passes a loaded config: cmd/core here and cocoon-macos's cni.New (built from netConf, never nil). * cut(hypervisor): ToVM stops cloning a map nothing else holds Both callers hand ToVM a record meta decoded fresh for this call; the clone dates from the in-memory index. * cut(cgroup): placeScope stops reading a scope it just created Since #306 Prepare always places into a freshly made scope, whose cpuset.cpus is empty, so the already-placed shortcut never fired and cost a sysfs read on every pinned launch. * cut(tombstone): drop the payload kind nothing reads Recovery reads only Payload.Mode and Payload.Cleanup; Kind was written on every lease and never consulted. Old rows that carry it still decode. * cut(sqlite): the events notifier drops its private cancel context PRAGMA data_version waits at most busy_timeout and Conn.Close already waits for an in-flight check, so the cancel plumbing guarded nothing and repeated cancel() in four cleanup tails. * cut(meta): encode the log cursor without a json.Marshal that cannot fail A uint64 marshals to its decimal digits; strconv.AppendUint writes the same bytes with no error branch. * docs: align docs and help text with the code - vm.md: a failed start stays error until the next successful start; a restore that fails before it stops the VM leaves no marker - networking.md: start and restore finish an interrupted vm rm instead of rebuilding NICs; FC guest hints print after every resize - daemon.md: a leased delete tombstone rolls back; SSE events are named change with the kind in type - cli.md: restore admits error VMs, --dns takes semicolons, --config, reseed skips, placement cleared only on stop, one FC console client - devices.md: fs tag and device id patterns; FC hints print on every call - gc.md, known-issues.md, snapshots.md, index.md: cni orphan records, a killed snapshot save, eject re-send on every guest, hibernate snapshot lifetime, SIGWINCH only on the CH PTY console - install.md: v0.7.7 release URLs - vm run --queue-size and vm debug --balloon help name CH-only and auto * build: least-privilege tokens for build, lint and test; drop dead CI branches - build, lint and test workflows declare contents: read like the release and os-image workflows already do - build-os-images: no os-image/windows dir and no .skip-generic-build marker exist any more - meta-upgrade.py reads and writes the json as UTF-8, matching Go - vm-inspect.sh: grep -c already prints 0, so || echo 0 printed a second line * docs: FC clone MAC conflict, daemon convergence scope, local root_dir - known-issues: an FC clone sends from the source's guest MAC through the unchanged TC redirect, so the bridge table flaps; cloud-hypervisor#7356 closed unfixed - daemon.md: dead-VMM convergence applies to running records or an open interval; the 0750 run dir is the daemon's only when it creates it - install.md: the sqlite engine refuses NFS, CIFS/SMB and FUSE roots * fix(scripts): meta-upgrade refuses an in-flight convert on json roots too Only the sqlite branch checked for the conversion manifest, so a json root whose json-to-sqlite convert had saved its manifest but not yet created meta.db got its namespace files rewritten under the manifest's pinned digests, and the resumed convert then failed as a changed source. * fix(images): gc logs a blob as collected only when every remover succeeded collectBlob printed "collected" before returning remover errors, so an EACCES or EBUSY left both a success line and a failure for the same blob. * docs: migrate capture rounds and the paused guest after a killed capture A capture killed after its last write leaves a gap in round numbers whose files the next round does not list; migrate capture pauses like save and hibernate. * fix: stop returning a variable alongside the call that assigns it `return id, b.view(ctx, fn)` where fn sets id relies on id being read after the call; the spec leaves the order between a plain operand and a call in the same return unspecified. gc happens to read it after, so nothing fails today. Take the error first in the seven read paths that did this. * review: name the PID and record hook func types spelled out more than once func(pid int) error appeared in three signatures and func(*VMRecord) error in three; /code rule 8 gives a repeated func type a name. * review: FC clone vocabulary types precede the method-bearing plan type Layout rule: method-less vocabulary types cluster ahead of the first method-bearing type in the file. * cut(vm): reseed stops re-inspecting a VM whose result already says no vsock Clone and restore results are built by SetRunningSockets after resume; the extra Inspect repeated the same vsock socket existence check and cost a meta read and a process check per clone or restore of a VM without vsock. * review: shellUnsafe also guards the sh -c runcmd lines The non-root runcmd interpolates the password raw into sh -c; the comment named only the YAML scalar. * review: two comments the code moved past refuseUnrecordedDevices now also refuses an unrecorded NIC, and the store's data dirs come from utils.EnsureDirs, not MkdirAll. * fix(cloudhypervisor): narrow the eject re-request to vm net, without a wait Review of the previous commit found two regressions: - device detach accepted any id present in device_tree, so --id _disk0 or a NIC id hot-removed that device instead of failing as not attached; VFIO detach goes back to Config.Devices - listing pending NIC ejects as orphans made every vm net on a Windows clone with a stranded hot-swap eject wait 30 s and fail vm net now re-sends vm.remove-device once for each pending NIC eject and goes on, so a retry after an eject timeout still asks the guest again. docs/daemon.md: a delete tombstone rolls back only while leased. * test(hypervisor): refusal tests assert the refusal they name The quarantine, interrupted-restore and creating tests passed on any error, and the delete-close test counted entries without checking their kinds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #301: the 133 test files that round did not read, two cross-file flows traced end to end, and a hardware run of both. It found five small production bugs and tests that could not catch what they name. No new mechanism.
Production fixes
fix(metering)Emitused the caller's context after the state change had committed. It now runs oncontext.WithoutCancel.fix(cloudhypervisor)vm netthat timed out on the guest's eject, or died between adding a NIC and recording it, leaves the VMM and the record out of step. A hibernate from there succeeded and could never wake: restore refuses the NIC count on every retry, andvm netneeds a running VM. The capture now refuses before pausing.fix(firecracker)vm netthat commits while a restore waits on the ops lock passed the prefix check and left a record with more NICs than the guest. The count now has to match, as on Cloud Hypervisor.fix(snapshot)omitemptydropped the emptynic_macs, so it read back as "no MACs" and skipped the check.omitzerokeeps the empty list. Sidecars from before v0.7.6 have no key and still skip the check rather than being refused.fix(metadata)mke2fssets a feature the gueste2fsckdoes not know (Debian 13 host, Ubuntu 22.04 guest:orphan_file), the second boot failed the fsck and stopped in the emergency shell with no network or sshd, despitenofail. Pass number 0 now. v0.7.5 does the same; the recovery for existing VMs is indocs/known-issues.md.Docs: when a clone-added disk mounts, Windows shutdown during background servicing, metering entries lost to a SIGKILL, and the fsck recovery, each from a hardware run. Also:
ensureVmlinuxmoves beside its helpers, Cloud Hypervisor drops a NIC count checkValidateRestoreMACsnow makes with the same message, anduuidV5andstart.shstop claiming to match each other (Go uses the URL namespace, the script DNS; nothing depends on them matching).Hot-path cost: nothing on start or clone.
WithoutCancelis one small allocation perEmit. The NIC check is an integer compare before the balloon wait.Tests
sh -c "sleep 60; :", which forks a child with the same cmdline. Under load the process scan flaked: 22 of 60 runs failed with 48 CPU spinners, 0 of 60 with the newexec -afixture and a wait for the exec.startFakeVMM,writeTwoGenerationsandreapChildreplace copies.TestUUIDV5Vectorpins a vector.Evidence
Gates at
19e258a8, and again on the tree this branch has after its rebase ontoba2020c4(#303); that tree was first computed withgit merge-treeand is identical to the rebased head:make lint0 issues on both GOOS,make fmt-checkclean,aslclean on both GOOS,go test -race -count=1 ./...in a linux/arm64golang:1.27container: all 38 packagesok.Hardware, bare metal, Cloud Hypervisor and Firecracker dev builds:
09485b7d, before #30119e258a819e258a8Second round, on a binary built from the rebased tree minus the last docs commit: regression 114 pass 0 fail, interrupted-delete metering 3 of 3 paired, upgrade from v0.7.5 49 pass 0 fail.
--pcisnapshot,vm net --nics 1racingvm restorevm net --nics 0then--nics 1, or a stop and start, heal the old state19e258a8): emergency shell. After: second and third boot mount and keep datavm hibernateandvm migrate capturererun vm net first; guest keeps running;vm net --nics 1healsvm migratecapture, delta, final, receive on one hostmax_vcpus384. With/sys/devices/system/cpu/onlinebind-mounted to0-15:max_vcpus16 and--cpu 17refusedInterrupted delete, metering pairs after the delete is finished:
09485b7d19e258a8gcvm.storageunpairedvm rmvm.storageunpairedLive NIC missing from the record, added through the VMM API:
35f884acsnapshot has 2 NICs, vm has 1;vm startboots cold and loses the memory image19e258a8has 2 NICs but its record has 1; rerun vm net first; the VM keeps running;vm net --nics 1, hibernate and wake succeed19e258a8vm net --nics 1also removes the NICA/B, p50 ms, n=20 (hibernate and restore n=10), arms swapped between runs:
35f884ac/ branch35f884acNo regression. Hibernate's second arm reads faster whichever binary it runs, so its gap is run order, not code.
Size
Production Go +51 −48 (net +3), plus
meta/contracttestscaffolding +17 −8. Tests +462 −351 (net +111). Comment lines in production +2 −3: one moved with its function, one corrected, one removed.Not covered
docs/daemon.mdnow says downstream must tolerate it. 600 random kills of an unmodifiedvm stopandvm startleft no unpaired compute entry.nic_macskey and keep the race window.TiWorkerrunning, and one clone took 73 s (its processes were not sampled). The power-button event is delivered;docs/known-issues.mdsays to raisestop_timeout_seconds.