Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new failure-path tests don’t tear down the VM after the expected error (risking leaked workers/mesh) and one expect_err message is misleading for the scenario being validated.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR updates the OpenHCL servicing integration tests (Petri) to exercise NVMe + MANA keepalive behavior more realistically by provisioning an actual NVMe controller/resource and expanding coverage across boot-time vs servicing-time keepalive flag combinations, including two tests that document a known restore-validation failure mode.
Changes:
- Introduces a shared
ServicingKeepAliveOperationsoptions struct and amana_nic_servicing_configbuilder helper that creates a concrete NVMe controller resource for keepalive to act on. - Expands the MANA NIC servicing test matrix to cover multiple permutations of boot keepalive enablement and servicing flags.
- Adds two negative tests that assert the DMA-manager restore validation failure when keepalive allocations are left unrestored.
File summaries
| File | Description |
|---|---|
| vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs | Extends OpenHCL servicing tests to create a real NVMe resource and broaden keepalive coverage (NVMe + MANA) across servicing scenarios. |
Review details
Suppressed comments (1)
vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs:2050
- After the expected restore-validation failure, the test returns without tearing down the VM. Since
PetriVmdoesn't auto-teardown on drop, this can leak the OpenVMM worker/mesh and make the test run flaky or resource-heavy.
// No graceful shutdown.
Ok(())
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The new negative-path tests don’t tear down the VM after the expected failure, which can leak test resources and cause flakiness.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs:1814
- The options struct mixes an inverted
disable_nvme_keepalive_at_bootflag with the otherenable_*flags. This inversion is easy to misread at call sites (especially in the many inline initializers below) and makes it more likely to introduce incorrect combinations in future edits. Consider renaming this field to anenable_nvme_keepalive_at_boot(and invert when formatting OPENHCL_DISABLE_NVME_KEEP_ALIVE), or otherwise make the polarity consistent across all fields.
vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs:2016
- These negative-path tests intentionally skip a guest shutdown, but they also skip tearing down the VM. Petri’s OpenVMM backend doesn’t have a guaranteed Drop-based cleanup for the worker, so this can leak a running VM/worker into subsequent tests and cause flakiness or resource contention. Tear down the VM explicitly after capturing the expected error.
// No graceful shutdown.
Ok(())
vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs:2050
- Same as above: after the expected servicing failure, the test should still tear down the VM explicitly to avoid leaving a running worker/VM behind when the guest cannot be shut down cleanly.
// No graceful shutdown.
Ok(())
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Updating VMM Petri tests for MANA and NVME keepalive during servicing.
DMA-manager restore validation fails when a VM boots with keepalive enabled for both NVME and MANA, then the servicing request disables one and not the other. The DMA snapshot contains resource allocations for both devices, but the saved state will only recreate (and claim) the resources for whichever keepalive was enabled. Strict private-pool validation then fails with unrestored allocations found, preventing VTL0 from starting.
Example: