Lab 8.2: Implement the Fix, Write the Test, Open the PR

Background

You have a red reproducer from Lab 8.1. Now you turn it green the right way — by finding the real defect, fixing it with the smallest correct diff, writing the test CONTRIBUTING requires, passing the full tools/devtool gate, and opening a pull request that two AWS maintainers can approve without asking you a single clarifying question.

This is where most first contributions fall apart, and almost always for the same reason: the contributor patches the symptom instead of the cause. They see a confusing failure at boot and "fix" the boot path, when the real defect is that a bad value was accepted three steps earlier at configuration time. The discipline of this lab is to walk the execution path from the symptom back to the point where the wrong thing first happened, fix it there, and prove with a fails-then-passes test that you fixed the right thing. The fact sheet's standards are the bar: one logical change per commit, an integration test for new behavior, a CHANGELOG entry, DCO sign-off, ≥2 maintainer approvals.

Why This Lab Matters for Contributors

• A minimal, correct diff with a fails-then-passes test is the single most reviewable PR shape. It is also the hardest to argue with.
• Walking the execution path is the skill that turns "I think this is the bug" into "here is the exact line where the wrong thing happens, and here is why." Maintainers can tell the difference instantly.
• Respecting the project's invariants — the threat model, snapshot/API compatibility, the minimal-device-model philosophy — is what separates a merged PR from a closed one.
• This is the implementation half of the Capstone. Do it well here and the graded version is muscle memory.

Why This Lab Matters — links to the deep dives

• The execution-path walk uses the API server and action channel and the VMM threading model.
• For a device bug, the cause lives in the MMIO bus and device manager and the relevant device deep dive (e.g. virtio-block).
• For a boot/memory bug, see the boot sequence and guest memory management.

Prerequisites

• A red reproducer on main from Lab 8.1 — the entry condition for this lab.

• The PR mechanics from Lab 2.2: fork-and-pull, DCO sign-off, the CHANGELOG convention.

• A working tools/devtool checkstyle / checkbuild / test from Level 5.

• Confirm your starting state:

  git fetch origin && git status                 # clean, on a branch off latest main
  git rev-parse --short HEAD
  rg -n "Signed-off-by" .git/config 2>/dev/null; git config user.name; git config user.email
  

Step-by-Step Tasks

Step 1 — Branch and re-confirm the red reproducer (5 min)

One logical change, one branch off the latest main:

git fetch origin
git switch -c fix/machine-config-reject-zero origin/main

Re-run the Lab 8.1 reproducer and confirm it is still red on this fresh branch:

cargo test -p vmm test_machine_config_rejects_zero        # expect FAILED
# and/or
tools/devtool test -- integration_tests/functional/test_machine_config_repro.py   # expect FAILED

If it is green now, your branch is not actually off main, or the bug was fixed upstream — stop and reconcile before writing a fix.

Step 2 — Walk the execution path from symptom to cause (30 min)

This is the core skill. Start at the symptom and grep your way upstream until you reach the point where the wrong thing first happens. For the worked example (a bad machine-config accepted instead of rejected), the path runs API → parse → action → validation → builder:

flowchart TD
    A["PUT /machine-config (HTTP on the UDS)"] --> B["api_server: parse body → ParsedRequest"]
    B --> C["build a VmmAction (rpc_interface.rs)"]
    C --> D["PrebootApiController dispatches the action"]
    D --> E["VmResources updates MachineConfig (resources.rs / vmm_config)"]
    E --> F{"Is the value validated here?"}
    F -- "no (the bug)" --> G["bad value stored; surfaces at InstanceStart / build_microvm_for_boot"]
    F -- "yes (the fix)" --> H["clean error → VmmActionError → 400 to the caller"]

Grep each hop on your branch — name code by role, never by a remembered line:

# 1. The action for machine config.
rg -n "MachineConfig\|machine_config\|SetMachineConfig\|UpdateMachineConfig" src/vmm/src/rpc_interface.rs

# 2. Where the action is handled and where VmResources is updated.
rg -n "fn update_machine_config\|fn set_machine_config\|MachineConfig" src/vmm/src/resources.rs

# 3. The struct, its validation, and the error enum.
rg -n "struct MachineConfig\|fn validate\|enum .*Error" src/vmm/src/vmm_config/machine_config.rs

# 4. Where the value is *consumed* at boot (to confirm that's only the symptom site).
rg -n "vcpu_count\|mem_size_mib" src/vmm/src/builder.rs

Write down the trail — the file and function at each hop, and the exact place the wrong thing first happens. That sentence ("the value is stored in VmResources without validation; it is only checked implicitly at boot, where it produces a confusing failure") is your root cause, and it goes verbatim into the PR.

Warning: Resist fixing at the consumption site (the builder, the KVM call). That is the symptom site. If you add the check at boot, the API still accepts the bad value, the bug is only half-fixed, and the PATCH path or the config-file path may still let it through. Fix where the value first enters the validated domain.

Step 3 — Implement the minimal correct fix (30 min)

Add the validation at the right hop. Read the existing error enum and an adjacent validation to match the project's idiom — Firecracker error types are typically thiserror-style enums returned as part of VmmActionError. Find the pattern:

rg -n "#\[derive\(.*Error.*\)\]\|thiserror\|#\[error" src/vmm/src/vmm_config/machine_config.rs
rg -n "enum MachineConfigError\|MachineConfig.*Error" src/vmm/src/vmm_config/

A minimal fix adds (a) error variants that name the offending field and include the bad value, and (b) the checks in the validation entry point. Adapt names to your branch (verify — these are version-sensitive):

#![allow(unused)]
fn main() {
// In src/vmm/src/vmm_config/machine_config.rs — the error enum (extend, don't rewrite).
#[derive(Debug, thiserror::Error, PartialEq, Eq)]
pub enum MachineConfigError {
    // ... existing variants ...
    #[error("vcpu_count must be greater than 0")]
    InvalidVcpuCount,
    #[error("mem_size_mib must be greater than 0, was {0}")]
    InvalidMemorySize(usize),
}

// The validation entry point the action handler calls. Keep the existing checks; add yours.
impl MachineConfig {
    pub fn validate(&self) -> Result<(), MachineConfigError> {
        // ... existing validation ...
        if self.vcpu_count == 0 {
            return Err(MachineConfigError::InvalidVcpuCount);
        }
        if self.mem_size_mib == 0 {
            return Err(MachineConfigError::InvalidMemorySize(self.mem_size_mib));
        }
        Ok(())
    }
}
}

Make sure the action handler actually calls validate() before storing the config — if it does not, that wiring is part of your fix. Confirm:

rg -n "\.validate()\|update_machine_config\|set_machine_config" src/vmm/src/resources.rs

Note (respect the invariants): Three constraints bound any Firecracker fix. (1) Threat model — never widen the seccomp surface or add an emulated device to "fix" a bug; a fix that adds attack surface will be rejected. (2) API/snapshot compatibility — do not change an existing field's type or a snapshot's on-disk shape without a deliberate, documented, versioned change; rejecting a previously-accepted value is itself a behavior change, so call it out in the PR. (3) Minimal-device-model philosophy — the smallest change that correctly fixes the bug, nothing more.

Step 4 — Make the reproducer pass; add the required test (25 min)

Re-run the Lab 8.1 reproducer. It must now be green:

cargo test -p vmm test_machine_config_rejects_zero        # expect ok
tools/devtool test -- integration_tests/functional/test_machine_config_repro.py   # expect PASSED

CONTRIBUTING requires an integration test for new behavior. If your Lab 8.1 reproducer was only a unit test, add the pytest integration test now so the API-level contract is covered (use the sketch from Lab 8.1 Step 6). Keep the unit test too — it pins the pure-logic boundary cheaply. Now prove the test actually guards the bug (the fails-then-passes discipline):

# Revert ONLY the fix, keep the test, and confirm the test goes red again.
git stash push -- src/vmm/src/vmm_config/machine_config.rs
cargo test -p vmm test_machine_config_rejects_zero        # expect FAILED  ← test guards the bug
git stash pop
cargo test -p vmm test_machine_config_rejects_zero        # expect ok      ← fix restores green

Warning: If the test stays green with the fix reverted, it does not test your change — it tests something already true. A reviewer will catch this. Fix the test until reverting the fix turns it red.

Step 5 — Run the full local gate (15 min)

Do not burn CI to find a clippy nit or a formatting diff. Run the same gate the maintainers run:

tools/devtool fmt           # cargo fmt + clippy --fix + cargo sort + python/markdown formatters
tools/devtool checkstyle    # style + license headers
tools/devtool checkbuild --all   # builds all targets; clippy is warnings-as-errors (-D warnings)
tools/devtool test          # unit + the pytest integration suite

Clippy runs as cargo clippy --all --all-targets --all-features -- -D warnings — a single warning fails the build. Fix warnings; do not #[allow(...)] them away unless you can justify it in the PR.

Step 6 — Write the CHANGELOG entry (5 min)

Required by the project. Add one line under the correct ### section of the [Unreleased] block — this is a fixed bug, so ### Fixed:

rg -n "## \[Unreleased\]\|### Fixed\|### Changed\|### Added" CHANGELOG.md | head

### Fixed

- [#NNNN](https://github.com/firecracker-microvm/firecracker/pull/NNNN): `PUT`/`PATCH /machine-config`
  now rejects `vcpu_count == 0` and `mem_size_mib == 0` with a clear `400`, instead of accepting the
  value and failing confusingly at boot.

Note: Match the existing entries' exact format on your branch (PR-number link style, tense, punctuation) — the CHANGELOG is reviewed too. If you change observable behavior (you do here — a previously-accepted value now errors), make sure the entry says so plainly.

Step 7 — Commit with DCO sign-off (5 min)

One logical change, a title ≤72 chars, an explanatory body, and a sign-off on every commit:

git add src/vmm/src/vmm_config/machine_config.rs \
        src/vmm/src/resources.rs \
        tests/integration_tests/functional/test_machine_config_repro.py \
        CHANGELOG.md
git commit -s -m "fix: reject zero vcpu_count and mem_size_mib in machine-config" -m \
"PUT/PATCH /machine-config previously accepted vcpu_count == 0 and
mem_size_mib == 0. The values were stored unvalidated and surfaced as a
confusing failure at InstanceStart. Validate them at configuration time and
return a clear MachineConfigError that names the offending field.

Fixes #NNNNN"

Verify the sign-off is present (the DCO bot enforces it):

git log -1 --format='%(trailers:key=Signed-off-by)'   # must print your Signed-off-by line
git log -1 --format='%s' | awk '{ if (length > 72) print "TITLE TOO LONG: " length }'

If you forgot -s: git commit --amend -s --no-edit.

Step 8 — Open the PR (15 min)

Push your branch and open the PR against main, filling in the template the repo provides:

git push -u origin fix/machine-config-reject-zero
gh pr create --repo firecracker-microvm/firecracker --base main \
  --title "fix: reject zero vcpu_count and mem_size_mib in machine-config" \
  --body-file -   # paste the body below, or open it in the editor

A description a reviewer can act on contains, at minimum:

## Changes
Validate `machine-config` at configuration time: `vcpu_count == 0` and
`mem_size_mib == 0` now return a clear 400 instead of being accepted and
failing at boot.

## Reason
Fixes #NNNNN. Reproduced on <commit>: the values were stored unvalidated in
VmResources and only surfaced as a confusing failure inside
build_microvm_for_boot. Root cause: no validation on the machine-config update
path. Fixed at the validation entry point so both PUT and PATCH are covered.

## License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

## PR Checklist
- [x] All commits are signed off (DCO).
- [x] New behavior is covered by an integration test (and a unit test).
- [x] `tools/devtool checkstyle` and `checkbuild --all` pass locally.
- [x] CHANGELOG.md updated under ### Fixed.
- [x] Linked the issue (Fixes #NNNNN).

Tip: Confirm CI is green before you ping anyone, and link the failing-then-passing reproducer explicitly in the description. The easier you make it to verify your fix, the faster two approvals arrive. Then expect review — see responding to feedback.

Implementation Requirements

• A branch off the latest main with one logical change.
• A written execution-path trail from symptom to root cause (the rg hops).
• A minimal, correct fix at the cause site, respecting the threat model and compatibility.
• The Lab 8.1 reproducer now green, plus an integration test for the new behavior.
• Proven fails-then-passes: reverting the fix turns the test red.
• Green tools/devtool checkstyle + checkbuild --all + test.
• A CHANGELOG.md entry under the right section.
• DCO-signed commit(s), title ≤72 chars, an explanatory body, the issue linked.
• A PR with a filled template, or a PR-ready branch if not submitting upstream.

Expected Output

$ cargo test -p vmm test_machine_config_rejects_zero
test result: ok. 2 passed; 0 failed

$ git stash push -- src/vmm/src/vmm_config/machine_config.rs && cargo test -p vmm test_machine_config_rejects_zero
test result: FAILED. 0 passed; 2 failed     ← test guards the bug
$ git stash pop && cargo test -p vmm test_machine_config_rejects_zero
test result: ok. 2 passed; 0 failed

$ tools/devtool checkbuild --all
... Finished. (clippy: 0 warnings)

$ git log -1 --format='%s%n%(trailers:key=Signed-off-by)'
fix: reject zero vcpu_count and mem_size_mib in machine-config
Signed-off-by: Your Name <you@example.com>

Troubleshooting

Clippy fails with -D warnings

The build treats every clippy lint as an error. Read the lint, fix it (often a needless clone, an unwrap clippy can prove panics, or a redundant pattern). Run tools/devtool fmt to auto-apply the fixable ones; resolve the rest by hand.

The fix passes locally but CI is red

Usually the pytest integration suite or a target you did not build locally. Run tools/devtool test (not just cargo test) and tools/devtool checkbuild --all (all targets, both arches if your setup supports it). Read the CI log; reproduce the exact failing command locally.

The DCO check blocks the PR

A commit lacks Signed-off-by. Amend the last one with git commit --amend -s --no-edit, or for older commits rebase and re-sign: git rebase --signoff origin/main. Force-push the branch.

A reviewer says the fix is in the wrong place

You likely fixed the symptom site. Re-walk the execution path (Step 2) and move the check to where the bad value first enters the validated domain. Thank the reviewer, fix it, push — see responding to feedback.

The behavior change breaks an existing test

A previously-accepted value now errors, and an old test asserted the old behavior. That is expected — update the test if the old behavior was the bug, and explain it in the PR. If the old test asserted something legitimate, your fix is too broad; narrow the trigger conditions.

tools/devtool test is slow / flaky in CI

The integration suite boots real microVMs. Keep your new test minimal (no boot if the bug is pre-boot). If a pre-existing test is flaky, that is a separate issue — do not bundle it.

Stretch Goals

1. Cover the PATCH path and the config-file path with the same validation and tests — a fix that only covers PUT is incomplete.
2. Read two merged bug-fix PRs and compare your diff's size and test placement to theirs: gh pr diff <N> --repo firecracker-microvm/firecracker. Are you in the same ballpark?
3. Add a unit test that asserts the exact error message names the offending field and value — that message is the operator's first signal (and the subject of Lab 8.3).
4. Squash any review-fixup commits into the logical commit before merge, re-signing with git rebase --signoff.
5. After merge, watch whether a backport label is applied and what that implies about Firecracker's release/maintenance branches (see the release process).

Validation / Self-check

Answer without notes. These gate completion:

1. Where exactly does the wrong thing first happen — name the file and function — and how do you know it is the cause, not the symptom?
2. Does reverting only the fix turn your test red? Show it.
3. What did your CHANGELOG line say, and which ### section, and why?
4. Which project invariant did your fix have to respect (threat model / compatibility / minimal model), and how did you stay inside it?
5. Is every commit signed off, the title ≤72 chars, and the issue linked?
6. Could two maintainers approve your PR without asking you a clarifying question? What in your description makes that true?
7. Does your PR add an integration test for the new behavior, as CONTRIBUTING requires?

Next: Lab 8.3 — Improve Error Messages and Diagnostics, a focused, high-value contribution class. The graded, end-to-end version of this work is the Capstone; for PR craft and review etiquette see PR quality and responding to feedback.