Put --balloon in the snapshot key - #1054
Conversation
`fcvm podman run --balloon N` attaches a balloon device before boot, and a snapshot of that VM keeps the device and its target. The flag was not part of FirecrackerConfig, the struct the snapshot key hashes, so two runs that differ only in --balloon computed the same key: - A run with --balloon restored a cached snapshot taken without it and got a guest with no balloon device. - `fcvm podman prepare --tag T --balloon N` compared content keys, found the installed generation's equal, and kept a snapshot taken without a balloon device or at another target. Restore cannot absorb the difference: no restore step adds a balloon device or sets its target. FirecrackerConfig now carries `balloon_mib`, both config builders set it from --balloon, and the device is attached from the launch config, so the value keyed and the value attached are the same field. The target is in the key as well as the device's presence, because no restore step sets a target today. Setting it at restore, so that the key need only say whether a device exists, is #1053. None is skip-serialized, so the key of a run without --balloon is unchanged. A run that already passed --balloon gets a new key and boots once more. A snapshot that such a run stored before this change sits under the key of the same run without the flag, and nothing records that it holds a balloon device. `fcvm podman prepare --force`, or removing that cache entry, replaces it. Tested: make test-unit FILTER="-E 'test(/balloon_changes|carries_the_balloon|snapshot_key|guest_visible_inputs/)'": 39 passed, among them balloon_changes_the_snapshot_key, the_launch_config_carries_the_balloon and test_snapshot_key_golden On main with only the new test added, balloon_changes_the_snapshot_key fails with both keys equal: left "a65206085631", right "a65206085631" make lint: exit 0 make test-unit, the whole suite, twice before the launch config test was added: 1397 and 1396 of 1401 passed. The failures were timing tests (two uffd balloon tests, Makefile lease tests, one timeout) on a host whose load average was 205 to 301 from other users' processes. Run by name back to back at a load of about 117, the two uffd tests failed on main and all six passed on this tree.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe balloon target is now included in Firecracker configuration and snapshot keys. Launch configuration carries the target, and balloon device attachment uses the value stored there. ChangesBalloon target handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Runs with different balloon targets use distinct snapshots, and supported Firecracker versions restore the saved target. No actionable merge-blocking risk remains. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change separates snapshots that require different balloon devices or targets without expanding caller privileges. Remaining risk is compatibility with previously created snapshots and rollback behavior, rather than a newly established security weakness. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a259379566
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ejc3
left a comment
There was a problem hiding this comment.
DISAGREE: the one suggestion in this review is answered in its thread, and it is #1053
The review's suggestion is to hash only whether a balloon device exists and to set the caller's target with PATCH /balloon before the restored VM resumes. On main these runs already share a snapshot and the second gets the first's target, which is the defect this PR fixes. Sharing them correctly needs a restore step that does not exist yet: a call on the hypervisor trait for both backends, the cache-hit path passing --balloon to the restore, and a VM test. That is #1053, stacked on this, and the field's comment names it. Until then a different target costs one cold boot and nothing is served wrong.
ejc3
left a comment
There was a problem hiding this comment.
NOT-A-DEFECT: CodeRabbit's summary comment reports no actionable comments for a259379
The summary's review block says no actionable comments were generated, and its merge risk reads minimal up to this head. It restates the change and claims nothing to fix.
--balloonwas not part of the snapshot key, so a cached or prepared snapshot taken without a balloon device answered for a run that asked for one.The problem
fcvm podman run --balloon Nattaches a balloon device before boot, and a snapshot of that VM keeps the device and its target. The flag was not a field ofFirecrackerConfig, the struct the snapshot key hashes, so two runs that differ only in--ballooncomputed the same key:--balloonrestored a cached snapshot taken without it and got a guest with no balloon device.fcvm podman prepare --tag T --balloon Ncompared content keys, found the installed generation's equal, and kept a snapshot taken without a balloon device or at another target.Restore cannot absorb the difference: no restore step adds a balloon device or sets its target.
The change
FirecrackerConfigcarriesballoon_mib, and both config builders set it from--balloon.configure_and_boot_vmbuild that config from the arguments they pass, so which device a VM gets is unchanged.Noneis skip-serialized, so the key of a run without--balloonis unchanged (test_snapshot_key_goldenpasses untouched). A run that already passed--balloongets a new key and boots once more.One residue of the old key: a snapshot that a
--balloonrun stored before this change sits under the key of the same run without the flag, and nothing records that it holds a balloon device.fcvm podman prepare --force, or removing that cache entry, replaces it.Contract, impact and evidence
Contract: two runs that differ only in
--balloonnever share a snapshot. Impact: the snapshot cache key, which decides what guest a caller gets. Evidence: one test through the real config constructor, watched failing on main, one test that the launch config carries the flag, plus the key, lint and unit suites. One review of the commit found no HIGH, 2 MEDIUM and 2 LOW, all folded: the key's justification now names the restore-time alternative (#1053), the residue of the old key is stated, the launch config has its test, and one comment was made exact.Red on main, with only the new test added:
With the change:
The whole unit suite ran twice, before the launch config test was added, on a development host whose load average was 205 to 301 from other users' processes: 1397 and 1396 of 1401 passed. The failures were timing tests: two uffd balloon tests against their bounds, Makefile lease tests that timed out waiting for a process, and one timeout. Run by name back to back at a load of about 117, the two uffd tests failed on main and all six passed on this tree. CI is the quiet run.
Not changed here
A clone that reboots, and a disk-only clone, cold-boot from arguments rebuilt out of the snapshot's metadata, which records no balloon. Such a boot has no balloon device, before and after this change. That is #1052.
Summary by CodeRabbit