Repository navigation
review: 8 fixes, cuts, docs against the code - #304
Merged
Merged
Conversation
TarDirStream closed the tar writer after a TarDir error, so a failure between two entries wrote the end-of-archive trailer and ExtractTar saw a valid, shorter archive. PersistSnapshotDir's stream fallback then finalized a snapshot with files missing and deleted the source dir, because the error only surfaced on the deferred Close it discards.
…cords Setting meta_backend=json before running meta convert opened an empty json store over live sqlite data: every process in that window saw no VMs, so the cgroup, CNI, TAP and vmlock GC modules reclaimed live VMs' resources and new records landed in the wrong engine. The sqlite side already refuses to bootstrap over legacy json; this is the mirror.
Start and restore un-quiesce a VM's plumbing before the VMM runs. A SIGKILL in that window (vk-cocoon's exec.CommandContext sends one on cancel) left a stopped record with no QuiescePending, so nothing ever brought the veth back down and the TC redirect pointed at a TAP no VMM held, the #130 softirq condition. The placement write now schedules the quiesce, restore places before it recovers the network, and the transitions to running clear it as before. Clone placeholders are excluded: a dead clone is reclaimed as a stale create. Start also checks the record invariants before placing it, so a record it rejects no longer keeps its cpus counted in the host placement.
…ries A canceled capture returns while CH is still writing the dump, and CH serves its API one request at a time. Ending the series first sent that request on the 30s client timeout behind the dump; CH drops a request whose client has gone, so the series stayed open with dirty logging on and the directory was deleted, leaving abort nothing to act on. Resume raises the timeout and waits out the dump, then the series ends.
The no-lease failure branch kept the master open in the CLI process. Nobody reads it, so a full PTY buffer can block the guest's console writes until the CLI exits; the relay child holds its own copy on the success path, so the master closes once on every path.
--max-cpu defaults to 8 and --cpu has no upper bound, so --cpu 16 printed boot=16,max=8, which Cloud Hypervisor rejects.
netns.NewNamed enters the new netns before it creates and mounts the name, so a failure there returned the locked thread to the scheduler inside an empty netns, where a later exec in the same process could run host-side plugin work.
HandleStopResult had one caller and no user outside this repo. A qcow2 v3 source was renamed to a fixed shared temp name before commit renamed it again into the blob store; every caller already passes a temp file it owns in the same temp dir, so commit takes it directly.
The GC removal-failure test chmods the snapshot data dir, so it reaches the removal and checks the tombstone it keeps; the TC redirect test re-runs setup on partial plumbing; the final migrate round checks the reference file a reflink would create; teardown tests assert the operation happened. Remove tests that duplicated others or could not fail, add the missing vm migrate and no-watchdog cases, and move a logstream test next to its code.
Compress multi-fact comments to one fact, delete comments that restate the code or an interface's contract, fix comments the code outgrew (convergeOrphanedPause after live migration, Network's omitzero, the missingDir cause, ReverseLayers' parameter), name func types spelled more than once, rename parameters that shadow imported packages, and use slices.Concat, min and a direct return where they are shorter.
Pin the quick start to v0.7.6; correct the Windows clone NIC, Android SELinux, data-disk size and Debian SSH statements; document -o json on mutating vm verbs, hibernate's --name, device --id and the pull_conns cap; replace duplicated sections with links to the canonical copy and drop sections no supported setup can hit.
goreleaser archives use ids; fmt-check applies the local import prefix like fmt and CI; make test uses the CI timeout; .gitignore no longer hides cmd/storebench; cocoon-check stops requiring ch-remote, which cocoon never runs; vm-inspect reads the netns from the record so scoped installs work; meta-upgrade.py exits with main's status and is ruff-formatted.
CMGS
changed the title
The daemon keeps its store open across the cutover, and checkQuiesced only probes the namespace locks for an instant, which an idle daemon does not hold. After json to sqlite the json engine reads the retired files as empty, so the next --gc-interval sweep treats every VM as an orphan: it kills the VMM and deletes the COW disk. After sqlite to json it keeps reading the renamed database, so VMs created later die the same way. Convert now holds the daemon's single-instance lock for the whole run, resume included. Hot-path cost is zero.
A json root on NFS, CIFS or FUSE cannot host sqlite, yet the CLI suggests meta convert on every json start. The filesystem refusal came after the manifest was saved, so every later verb refused with 'a conversion is in flight' and the rerun hit the same refusal; only deleting the manifest by hand got out.
3ff73bb set the flag at placement, but a restore of a running VM has already committed Stopped by then. A SIGKILL during apply left Stopped with no flag and the plumbing up: the daemon skips that record and vm stop finds nothing to do, so only another restore or vm rm cleared it. The commit that erases the Running signal now sets the flag. The test also pins placement before network recovery for a stopped origin.
RecoverTombstone commits a fresh lease before its teardown fails. That commit fires the meta event, so a persistent failure such as a removed CNI conflist ran a full pass about four times a second instead of every five seconds: a /proc walk, several durable commits per stuck VM and plugin execs, all contending with CLI writers. After a degraded pass the loop leaves the next pass to the ticker.
The reclaim's busy outcome was discarded, so the receive blocked on the lock and then adopted the placeholder: over a deleting tombstone the next roll-forward destroyed the received VM, and over a concurrent receive it removed the directories under that receive's VMM.
Also correct the QuiescePending comment and the daemon event note after the flag started covering launches, and name the way out in meta-upgrade's in-flight refusal.
…d NIC A clone hot-swaps its NICs while paused, and the old NIC stays in the device tree until the guest acks the eject. A snapshot taken before that ack saves the node without the pending-eject bit, so every VM cloned from it keeps a half-ejected NIC forever and passes it down the chain. On hardware, 4 of 5 zero-delay snapshots carried it, and one clone came up with no NIC in the guest while vm clone reported success. Pause now waits up to ejectWaitTimeout. On timeout it warns and goes on instead of refusing, because descendants of an already poisoned snapshot hold a node that never ejects and must stay capturable. A VM with nothing pending pays nothing: vm.info is already read here.
…er VM cocoon-check --subnet wrote ipMasq true, the setting known-issues says leaks a POSTROUTING rule and a CNI chain for every VM, because the bridge plugin reads the addresses to remove off an interface cocoon has flushed. Generate ipMasq false and masquerade the subnet on the host once.
… again 3ff73bb moved placement first so the quiesce flag preceded the un-quiesce. Measured across commits on an idle node-pinned host, that order alone adds 6-7ms to every restore (p50 130 to 136, n=60, flag kept and order reverted measures 129.7), and hibernate wake is a latency path. A running origin already has the flag from the kill's commit, so this only reopens a 1-2ms gap for a stopped origin between un-quiesce and the placement commit. The window up to the VMM exec stays covered.
d7146d5 added the rule once at generation and printed FIXED even when iptables failed. A later run skipped generation, and a reboot dropped the rule, so guests lost outbound NAT with no report. It is now checked like the FORWARD rules: reported on every run and added by --fix, so a rerun converges.
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.
Whole-repo audit at 3b885b8. Every tracked file was read in full (389 Go files, docs, scripts, CI config), except
go.sum,LICENSEand the first 40 lines only of 9 testdata files. Seven cross-file flows were then traced window by window. Most of the serious bugs came from the flow pass, not the per-file pass.Fixes
Data loss
meta convertundercocoon daemon --gc-intervalleft the daemon reading retired json as empty, or the renamed database. Its next sweep treated VMs as orphans, killed the VMM and deleted the COW disk. Reproduced in both directions with the realconvert.Runand GC module.TarDirStreamfailure between entries reaches the reader (c94d958).PersistSnapshotDir's EXDEV fallback could finalize a snapshot with files missing and delete the source. Present since 71fefed.meta.dbholds the records (04b65c3). Settingmeta_backend=jsonbefore converting hid every VM from GC.Silent VM damage
vm clonereported success. Afterwards 8 of 8 rounds are clean. On timeout the capture warns and goes on, so descendants of an already poisoned snapshot stay capturable.migrate receive --idrefuses a busy id (ba3f005). It could adopt a tombstoned placeholder, and roll-forward then destroyed the received VM.Host safety
vm stopconverge it; with the daemon it converges by itself.Smaller
b77c198f: convert checks the target filesystem before the manifest is durable, so a json root on NFS no longer locks itself out.0d0e5013: a VM that cannot converge no longer wakes its own daemon about 4 times a second.d7146d58,7e20c1e4:cocoon-check --subnetno longer generates theipMasqsetting that known-issues says leaks a rule per VM. Each run now checks the subnet's masquerade rule, and--fixadds it, so a reboot or a failed insert is reported and a rerun converges.0e116223: a failed migrate round resumes before it ends the series.7d026caa,1be2f69e,c576e5d7,60896a44: FC PTY master close,vm debugmax vCPU, netns thread restore, script annotation.Each Go fix has a regression test that fails without the fix, except the
vm debugone-liner,ba3f0057andc576e5d7.ba3f0057has a killed-receive retry leg on hardware, which covers the retry but not the busy branch.c576e5d7was probed in a privileged container only.Cuts, tests, review
97864be4inlines single-callerHandleStopResultand removes cloudimg's rename of a qcow2 v3 source.81e59cdfmakes tests prove what they name and removes duplicates.522bac52is the comment budget, naming and modern-idiom pass.Docs and tooling
33125b67,b8571e3b: docs match the code; the release pin moves to v0.7.6; duplicated sections become links.eaec44ed: goreleaserids;fmt-checklocal prefix;.gitignoreno longer hidescmd/storebench;cocoon-checkno longer requiresch-remote;vm-inspect.shreads the netns from the record.Latency
The clean A/B on an idle NUMA-pinned host caught a regression from this branch: the first form of 3ff73bb placed before recovering the network, which cost 6-7ms per restore.
bf83dbe4restores the order. The root cause of that cost was not found. The trade-off is a 1-2ms gap for a stopped-origin restore between un-quiesce and the placement commit, with no flag.One unexplained detail: split by position in the iteration, head is 128.4 at position 0 against about 117 for the other two, and faster at positions 1 and 2. Each cell has n=20. Hibernate shows no regression. The clone, stop, cold start and rm numbers come from the earlier build and stayed within noise with labels swapped.
Hardware verification
Run on an idle bare-metal test host, NUMA-pinned, at bf83dbe (the later 7e20c1e only changes
doctor/check.sh), with CH dev 35b098c65260 and FC dev 71b5754994de.quiesce_pending=False, eth0 still up aftervm stopmeta convertunder a live daemonNot verified on hardware:
check.shchange ran in a privileged container's own netns rather than on a test host, because shared hosts'cni0is off limits: fresh--fix, an idempotent rerun, a flushed POSTROUTING reported as FAIL, and--fixrestoring the rule. A shim that fails everyiptables -Agives WARN and no FIXED for that rule, and a healthy rerun adds it. The real xtables lock was not exercised, because Debian's iptables-nft does not take it.Gates
make lint: 0 issues on GOOS=linux and darwin.make fmt-checkis clean.aslreports zero findings on both GOOS.go test -race ./...: all 38 packages with tests pass in a privileged linux/arm64 container at bf83dbe.meta/json, which is in third-party code (backend_kqueue.go:443).Known and not fixed
For a maintainer to decide
--finalplus a resume can silently lose punched blocks.docs/snapshots.mdforbids that sequence; closing it adds a FICLONE per disk to final-round downtime.os-image/**change rebuilds and publishes images on merge:!assertions inverify-vm-inner.sh, including an internal network prefix;validate-image.sh;start.sh;AcquireandMarkDeleting, and oneRTM_SETLINKfor CNI TAP setup.Bounded, no code
snapshot saveracingvm rm --force.