refactor: codebase cleanup batch — F1/F13 + Q F6/F7/F15#31
Merged
Conversation
Combined batch of high- and low-value findings from the 5-agent codebase audit, double-checked one by one. F1 — CH-local vmPutJSON[T any] mirrors firecracker.putJSON. addDiskVM, addNetVM, removeDeviceVM collapse from 4-line marshal+vmAPIOnce blocks to one-line wrappers. CH/FC HTTP+JSON helpers are now structurally symmetric. F13 — statusEventLoop and statusEventLoopJSON shared a 30-line list-snapshot-diff-emit body differing only in formatter. Extract statusEventDiffLoop driven by an eventEmitter struct (begin/emit/end); text variant flushes a tabwriter, JSON variant encodes per event. Q F6 — chDebugSpec.Memory and CowSize were precomputed >>20/>>30 unit conversions of vmCfg fields with no separate write path. Drop the redundant fields and compute inline at print time. Q F7 — Hoist 100ms/200ms/1ms watch and poll intervals into named package-level consts (logFollowDebounce, statusWatchDebounce, socketReadyPollInterval, netnsDeleteRetryInterval), removes 4 //nolint:mnd suppressions. Q F15 — Trim BlobHexFromPath godoc from 2 lines to 1. Skipped after closer review (not actionable, agent overstated, or risk > value): F2 (IndexLayout — backend embedding shapes differ), F3 (vm list/status N+1 — actual cost ~1ms at 100 VMs, well under CLI latency threshold), F6 (image list parallel — same), F11 (VMRecord persisted RunDir/LogDir is config-change safety net, not redundancy), Q F3/F5/F9/F12/F14, Eff F4/F5/F7/F8. Lint clean both platforms; tests pass; testbed regression 25/25.
CMGS
force-pushed
the
chore/codebase-cleanup-batch
branch
from
5c487b5 to
d0e9967
Compare
There was a problem hiding this comment.
Pull request overview
This PR performs a small batch of refactors/cleanup across the CLI, hypervisor helpers, and networking utilities, primarily aimed at deduplicating repeated logic and replacing magic timing literals with named constants while keeping behavior unchanged.
Changes:
- Added a Cloud Hypervisor-local generic JSON+PUT helper (
vmPutJSON) and refactored hot-plug API helpers to use it. - Deduplicated
vm status --eventtext/JSON logic by extracting a shared diff loop driven by aneventEmitter. - Replaced several hard-coded polling/debounce intervals with named constants; simplified CH debug printing by removing redundant precomputed fields; trimmed a doc comment.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| network/cni/create_linux.go | Introduces a named retry interval constant for netns deletion polling. |
| hypervisor/utils.go | Adds a named socket readiness poll interval constant; trims BlobHexFromPath godoc; uses the const in WaitForSocket. |
| hypervisor/cloudhypervisor/helper.go | Adds vmPutJSON[T] helper and refactors CH PUT+JSON endpoint helpers to one-liners. |
| cmd/vm/status.go | Extracts shared status event diff loop and uses an emitter abstraction for text vs JSON output; adds a named watch debounce const. |
| cmd/vm/lifecycle.go | Replaces a magic debounce literal for log-follow with a named constant. |
| cmd/vm/debug.go | Removes redundant fields from chDebugSpec and computes units inline when printing debug commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WriteSnapshotEnvelope used utils.AtomicWriteJSON (compact) while ExportToTar built the same envelope inline with json.MarshalIndent (indented). User-facing snapshot.json bytes diverged depending on which export mode (--to-dir vs tar) was used. Add snapshot.MarshalEnvelope as the single source of envelope bytes; use indented form everywhere so the JSON inside extracted tars and under --to-dir is identical. WriteSnapshotEnvelope now goes through MarshalEnvelope + utils.AtomicWriteFile. Verified on testbed: --to-dir snapshot.json is indented; tar export still works; import-from-tar parses the new format.
Copilot flagged `&e.vm` as a range-loop pointer footgun. Go 1.22+ loop scoping makes the current code safe (each iteration has its own e), so this is theoretical, not a bug. But the pointer carried no semantic — it was just "share data" — and pass-by-value is hair cleaner: copies ~200 bytes per emit (negligible at typical VM count and 5s tick), kills the lifetime ambiguity entirely, and frees future emitters to retain copies without reasoning about iteration scope.
Codex flagged: statusEventDiffLoop mutated the *types.VM returned by listAndFilter (vm.State = ...) and then takeSnapshot called ReconcileState on the same vm again. The mutation was local (vm is a fresh ToVM-built copy per List call) and the second call was cheap (state already non-Running short-circuits IsProcessAlive), so neither was a bug — but mutating someone-else's-return is a smell, and doing the same work twice is just waste. takeSnapshot now takes the reconciled state as an arg. The event-diff loop computes state once, copies vm into vmCopy with the reconciled State, and never touches the upstream pointer. snapshotAll feeds ReconcileState into takeSnapshot directly. status_test updated for the new signature.
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.
Summary
Combined batch from the 5-agent codebase audit, all double-checked file-by-file before applying. Mix of high-value structural dedup and low-value cleanup that share the same risk profile (no behavior change, all unit-tested or testbed-verified).
What changed
F1 — CH-local
vmPutJSON[T any](hypervisor/cloudhypervisor/helper.go). Mirrorsfirecracker.putJSON[T]so the two backends' HTTP+JSON helpers are structurally symmetric.addDiskVM,addNetVM,removeDeviceVMcollapse from 4-line marshal+vmAPIOnce blocks to one-line wrappers.F13 —
statusEventLoop/statusEventLoopJSONdedup (cmd/vm/status.go). Both shared a 30-line list-snapshot-diff-emit body differing only in formatter. ExtractstatusEventDiffLoopdriven by aneventEmitterstruct (begin/emit/end). Text variant flushes a tabwriter; JSON variant encodes per event.Q F6 —
chDebugSpecfield redundancy (cmd/vm/debug.go).MemoryandCowSizewere precomputed>>20/>>30unit conversions ofvmCfgfields with no separate write path. Drop the fields, compute inline at print time. Eliminates drift risk.Q F7 — Magic timing literals → named consts.
100ms/200ms/1mswatch and poll intervals hoisted into package-level consts (logFollowDebounce,statusWatchDebounce,socketReadyPollInterval,netnsDeleteRetryInterval). Removes 4//nolint:mndsuppressions.Q F15 — Trim
BlobHexFromPathgodoc from 2 lines to 1 (function name + example were redundant).Skipped after closer review
nowchange has timestamp drift risk;info.Hypervisor json:\"-\"would breakvm inspectJSON output; 2-entry slice→map is slower not faster)Net diff
Test plan
make lintclean on darwin + linuxgo test ./...all pass (incl.cmd/vmevent-loop change exercises both formatters via existing tests)