Skip to content

Put --balloon in the snapshot key - #1054

Merged
ejc3 merged 1 commit into
mainfrom
balloon-in-snapshot-key
Oct 3, 2026
Merged

ejc3 merged 1 commit into
mainfrom
balloon-in-snapshot-key

Conversation

@ejc3

@ejc3 ejc3 commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

--balloon was 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 N attaches a balloon device before boot, and a snapshot of that VM keeps the device and its target. The flag was not a field 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.

The change

  • FirecrackerConfig carries balloon_mib, and both config builders set it from --balloon.
  • The device is attached from the launch config, so the value keyed and the value attached are the same field. All three callers of configure_and_boot_vm build that config from the arguments they pass, so which device a VM gets is unchanged.
  • None is skip-serialized, so the key of a run without --balloon is unchanged (test_snapshot_key_golden passes untouched). A run that already passed --balloon gets a new key and boots once more.
  • 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 Key only the balloon device presence and set its target at restore #1053.

One residue of the old key: a snapshot that a --balloon 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.

Contract, impact and evidence

Contract: two runs that differ only in --balloon never 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:

$ make test-unit FILTER="-E 'test(/balloon|snapshot_key/)'"
  TRY 2 FAIL [   0.009s] (39/40) fcvm commands::podman::tests::balloon_changes_the_snapshot_key
    assertion `left != right` failed: a snapshot taken with no balloon device must not serve --balloon 0
      left: "a65206085631"
     right: "a65206085631"

With the change:

$ make test-unit FILTER="-E 'test(/balloon_changes|carries_the_balloon|snapshot_key|guest_visible_inputs/)'"
        PASS [   0.006s] ( 2/39) fcvm commands::podman::tests::the_launch_config_carries_the_balloon
        PASS [   0.006s] ( 4/39) fcvm commands::podman::tests::balloon_changes_the_snapshot_key
        PASS [   0.004s] (39/39) fcvm firecracker::config::tests::test_snapshot_key_golden
     Summary [   0.072s] 39 tests run: 39 passed, 1363 skipped
$ make lint        # exit 0

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

  • New Features
    • Balloon memory targets are now applied consistently when launching virtual machines. Snapshot matching distinguishes between runs with no target, a zero target, or a different configured target, so each run uses a snapshot associated with its balloon setting. When no target is configured, the existing snapshot configuration format is preserved.

`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.
@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: fd2256de-e6c7-45c3-84f5-e58cf5051943
📥 Commits

Reviewing files that changed from the base of the PR and between f90572a and a259379.

📒 Files selected for processing (5)
  • src/cli/args.rs
  • src/commands/podman/mod.rs
  • src/commands/podman/snapshot.rs
  • src/commands/podman/vm_config.rs
  • src/firecracker/config.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Balloon target handling

Layer / File(s) Summary
Include balloon target in snapshot configuration
src/firecracker/config.rs, src/commands/podman/snapshot.rs, src/commands/podman/mod.rs, src/cli/args.rs
FirecrackerConfig adds an optional balloon target. Snapshot configuration copies the argument into that field, so configured targets are included in snapshot keys. Tests check that three target settings produce distinct keys. The CLI documentation describes the snapshot-key behavior.
Pass balloon target through launch configuration
src/commands/podman/vm_config.rs, src/commands/podman/mod.rs
Launch configuration receives the balloon target. Balloon device attachment reads the target from launch configuration. A test checks that a 512 MiB argument is carried into that configuration.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to a2593

Runs with different balloon targets use distinct snapshots, and supported Firecracker versions restore the saved target. No actionable merge-blocking risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to a2593

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
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The traced scope is VM balloon configuration and snapshot generations selected through the existing Podman workflow and configured data directory. The change does not establish additional tenant, service, credential, or network exposure; deployment-wide isolation boundaries were not supplied.

Trust Boundaries and Controls

  • observed — The caller-controlled value remains the existing Option CLI input and reaches the same hypervisor balloon API. The PR changes configuration provenance rather than adding a caller, granting authority, or weakening an inspected control.

Resilience and Maintainability Implications

  • observed — Existing generation locks mediate creation, verification, replacement, and restore. Prepared verification rejects mismatched content and checks generation artifacts; creation writes artifacts and configuration into a temporary directory before installation. These controls remain applicable to the new balloon-specific identities.
  • inferred — Replacement is not crash-atomic across its two renames: interruption or failure after moving the installed generation aside can leave the final path absent. The helper is identical at base and head, so this is a pre-existing recovery limitation, not an established PR-introduced security concern.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: including the --balloon setting in the snapshot key.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/firecracker/config.rs
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-03T18:40:51.068264Z a259379 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@ejc3 ejc3 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ejc3 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ejc3
ejc3 merged commit 80767bb into main Oct 3, 2026
25 of 27 checks passed
@ejc3
ejc3 deleted the balloon-in-snapshot-key branch October 3, 2026 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant