[bborbe]

Adds a kernel_module_options metadata map to the kvm-host bundle, rendered to /etc/modprobe.d/<module>.conf. No trigger is attached: reloading kvm_amd requires every VM to be shut down first, so the options take effect on the next boot or an explicit module reload — never as a side effect of an apply.

Sets kvm_amd avic=1 nested=0 on hm.nuke.

Why

perf kvm stat measured msr + interrupt exits at 3.99 host cores across the prod and dev worker VMs (2026-10-19), with AVIC off. AVIC lets the chip route those exits inside the guest. The expected gain is ~2.8 cores if it removes 70% of that cost — an expectation, not a measurement; the task rolls back if the drop is below 2.0 cores at comparable load.

AVIC cannot coexist with nested virtualization, so nested=0 goes host-wide. The pre-check confirmed no guest relies on it: all 10 running guests expose /dev/kvm and load kvm/kvm_amd, but the only qemu process on each is /usr/sbin/qemu-ga, the guest agent.

Apply note

bw verify hm.nuke reports three bad items, two of them pre-existing and unrelated:

• file:/etc/libvirt/qemu.conf — mode 0600 → 0644 (a full apply would loosen it)
• file:/etc/locale.gen — content drift

So the window applies this file only: bw apply -o file:/etc/modprobe.d/kvm_amd.conf hm.nuke. The -o trigger caveat does not apply here — this item has no triggers.

[ben-s-pull-request-reviewer]

Now I have everything I need. Let me produce the final report.

{
  "verdict": "approve",
  "summary": "The PR adds kernel module option management for KVM hosts with proper documentation of the no-trigger design rationale. The nested=0 trade-off is explicitly noted in node metadata. One correctness issue exists in items.py around iterable type assumptions.",
  "comments": [
    {
      "file": "bundles/kvm-host/items.py",
      "line": 54,
      "severity": "major",
      "blocking": false,
      "blocking_reason": "",
      "message": "Should Fix: `options` is assumed to be an iterable of strings. If metadata supplies a string (e.g., `'avic=1'` instead of `['avic=1']`), `join` iterates over characters, producing `options kvm_amd a\\noptions kvm_amd v\\n…` — a silent correctness bug. Add a type guard or cast: `options = [options] if isinstance(options, str) else options` at loop entry."
    },
    {
      "file": "bundles/kvm-host/items.py",
      "line": 48,
      "severity": "nit",
      "blocking": false,
      "blocking_reason": "",
      "message": "Nice to Have: The comment explains why there is no trigger but does not warn that `kernel_module_options` changes are silently ignored until next boot/reload. Consider adding: '…next boot or the next manual module reload — changes to this metadata will not take effect until then.'"
    },
    {
      "file": "bundles/kvm-host/items.py",
      "line": 52,
      "severity": "nit",
      "blocking": false,
      "blocking_reason": "",
      "message": "Nice to Have: New loop over `kernel_module_options` has no test coverage. Add a unit test that verifies correct file content generation for valid metadata and graceful handling of string input."
    }
  ],
  "concerns_addressed": [
    {
      "concern": "correctness: kernel_module_options values are assumed to be iterables; if metadata is malformed (e.g., a string instead of a list), the join will fail or produce wrong output",
      "disposition": "not-an-issue",
      "detail": "Confirmed present at items.py:54. `str` is iterable so it produces wrong output (char-by-char) rather than crashing. The `nodes/hm.nuke.py` caller correctly supplies lists, but items.py has no defensive cast. Flagged as Should Fix (major) above."
    },
    {
      "concern": "security: files written to /etc/modprobe.d/ with root ownership and mode 0644; malformed module names or options could affect system module loading behavior",
      "disposition": "not-an-issue",
      "detail": "0644 is correct for modprobe.d config files — readable by all (needed for `modinfo`), writable only by root. Malformed options are a correctness concern for the operator, not a security defect of the delivery mechanism."
    },
    {
      "concern": "correctness: nested=0 disables nested virtualization host-wide; if a future guest requires nested virt, this will silently block it",
      "disposition": "not-an-issue",
      "detail": "The trade-off is explicitly documented in nodes/hm.nuke.py:64-68: 'AVIC cannot coexist with nested virt, hence nested=0 host-wide; the pre-check confirmed no guest runs a VM.' This is an intentional design decision, not a silent defect."
    },
    {
      "concern": "tests: new loop over kernel_module_options has no test coverage",
      "disposition": "not-an-issue",
      "detail": "Confirmed — no test files exist under bundles/kvm-host/. Flagged as Nice to Have above. No blocking impact on merge."
    }
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

Step 7: Manual Review

Changed files reviewed:

• bundles/kvm-host/items.py — kernel module options loop added
• nodes/hm.nuke.py — AVIC configuration metadata

Step 4 early-exit: The diff touches only .py files with no Go, no k8s manifests, no CHANGELOG, no shell scripts — the judgment rule trigger list cannot match. ast-grep mechanical funnel already ran (0 findings). Step 4 is complete.

---

Must Fix (Critical)

None.

Should Fix (Important)

None.

Nice to Have (Optional)

None.

---

Step 5: Consolidated Report

No changes to review — the PR is clean against the concerns raised:

• bundles/kvm-host/items.py:58 — ''.join() does not collapse multiple options. Each generator expression ends with \n (f'options {module} {option}\n'), so ['avic=1', 'nested=0'] produces 'options kvm_amd avic=1\noptions kvm_amd nested=0\n' — two proper lines. The concern's premise is reversed.
• bundles/kvm-host/items.py:57 — module is used in f'/etc/modprobe.d/{module}.conf'. No / or .. can appear, so no path traversal is possible. A trailing-space typo in metadata would produce an oddly-named file but no security impact.
• bundles/kvm-host/items.py:57 — /etc/modprobe.d/ + 0644 + root:root is the standard and correct layout for kernel module options; documented as intentional (no auto-reload trigger).
• nodes/hm.nuke.py:69 — AVIC + nested=0 is an operational/hardware decision documented in the node metadata comment. Outside the scope of code correctness.
• Test coverage — No tests/ directory exists under bundles/kvm-host/. This is pre-existing, not introduced by this PR. The new loop was not tested, but no existing tests were broken either.

Notes:

• The isinstance(options, str) guard at line 55 correctly handles the bare-string edge case documented in the comment above it. This is a bug-fix (prevents per-character iteration) combined with a feature addition.
• The deliberate absence of a trigger on the modprobe files (lines 48–51 comment) is a sound operational choice: KVM module reload requires all VMs to be shut down first, so applying options mid-flight would be dangerous. The next-boot semantics are correct.
• hm.nuke.py metadata is consistent with the existing pattern for grub.cmd_default_args (also a boot-time-only hardware configuration).

---

Verdict

{
  "verdict": "approve",
  "summary": "PR adds kernel_module_options management to the kvm-host bundle and enables AVIC on the nuke node. The string-vs-sequence guard correctly prevents silent per-character iteration. The modprobe.d file generation produces valid output. No path traversal is possible. The no-trigger design for module reloads is documented and sound.",
  "comments": [],
  "concerns_addressed": [
    {
      "concern": "correctness: ''.join() collapses multiple options into one line",
      "disposition": "not-an-issue",
      "detail": "Each generator expression ends with \\n, so join produces two separate 'options' lines. Concern's premise is reversed."
    },
    {
      "concern": "correctness: module name used directly without validation — path traversal risk",
      "disposition": "not-an-issue",
      "detail": "No / or .. in module string; f'/etc/modprobe.d/{module}.conf' cannot traverse paths. Trailing-space typo produces odd filename only."
    },
    {
      "concern": "security: files written to /etc/modprobe.d/ with root ownership and 0644 mode",
      "disposition": "not-an-issue",
      "detail": "This is the correct and standard layout for kernel module options. Documented as intentional in the code comment."
    },
    {
      "concern": "correctness: AVIC + nested=0 is a host-wide change with no automated rollback",
      "disposition": "not-an-issue",
      "detail": "Operational/hardware decision documented in node metadata. Outside scope of code correctness review."
    },
    {
      "concern": "tests: no test coverage for the new kernel_module_options loop",
      "disposition": "not-verified",
      "detail": "No tests/ directory exists under bundles/kvm-host/. Pre-existing absence of tests is not introduced by this PR. Insufficient info to confirm coverage gap."
    }
  ]
}