petri: add NVMe emulator support#3378
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Hyper-V NVMe coverage in the Petri-based vmm_tests suite by provisioning NVMe devices via a closed-source Hyper-V “NVMe emulator” and wiring it through the Hyper-V Petri backend.
Changes:
- Adds an (unstable) Hyper-V OpenHCL Linux storage test that validates discovery + IO via an NVMe emulator device.
- Extends the Hyper-V Petri backend to translate
VmbusStorageType::Nvmeinto post-create NVMe emulator device attachment. - Adds PowerShell-side plumbing to attach NVMe emulator devices after
New-CustomVMcompletes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
vmm_tests/vmm_tests/tests/tests/x86_64/storage.rs |
Adds an unstable Hyper-V NVMe emulator storage test that exercises discovery and IO. |
petri/src/vm/hyperv/mod.rs |
Maps VmbusStorageType::Nvme controllers to NVMe emulator device configs for Hyper-V backend runs. |
petri/src/vm/hyperv/powershell.rs |
Adds nvme_emulator_devices to VM creation args and attaches them post-create via PowerShell. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
petri/src/vm/hyperv/mod.rs:389
ide_controllersis still computed above, but it is no longer passed intoHyperVNewCustomVMArgs(onlystorage_controllersis set). This will silently drop IDE drive configuration for Hyper-V VMs and also leaves an unusedide_controllerslocal. Include theide_controllersfield inhyperv_args(or remove the mapping if IDE is intentionally unsupported).
guest_state_path,
storage_controllers,
com_3: supports_com3,
imc_hiv,
management_vtl_settings,
..HyperVNewCustomVMArgs::from_config(&config, &properties)?
};
let vm = HyperVVM::new(hyperv_args, log_source.clone(), driver.clone()).await?;
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
petri/src/vm/hyperv/mod.rs:365
ide_controllersis built from the firmware config above, but it is no longer passed intoHyperVNewCustomVMArgs(onlystorage_controllersis set here, andfrom_configinitializeside_controllersas empty). This silently drops IDE controller configuration for Hyper-V VMs and also leaves an unused local. Pass the computedide_controllersintohyperv_args(or remove the mapping if it’s intentionally unsupported).
let hyperv_args = HyperVNewCustomVMArgs {
firmware_file: igvm_file.clone(),
firmware_parameters: openhcl_command_line,
guest_state_path,
storage_controllers,
com_3: supports_com3,
imc_hiv,
management_vtl_settings,
..HyperVNewCustomVMArgs::from_config(&config, &properties)?
};
assert_eq! panics the test runner instead of returning a structured error. Use anyhow::ensure! so NSID validation failures surface as clean error messages. Co-authored-by: Copilot <[email protected]>
|
Fundamentally, why do we need a separate Hyper-V test? Why can't we just make the existing NVMe test(s) work on Hyper-V transparently? |
| # Vsid => { | ||
| # Vtl, | ||
| # Drives => @( | ||
| # @{ Nsid; DiskPath }, |
There was a problem hiding this comment.
We can simplify this a bit and make the comments consistent:
The word "DiskPath" does not need to appear in the actual object passed in, Drives can be a hashtable with int keys and string values (rather than having a hashtable with only one thing)
# NvmeControllers => {
# Vsid => {
# Vtl,
# Drives => {
# Nsid => DiskPath,
# ...
# }
# },
# ...
# }
| $targetVtl = $controller.Value["Vtl"] | ||
| $drives = $controller.Value["Drives"] | ||
| # Drives arrive pre-sorted by NSID from the Rust layer. | ||
| $vhdPaths = @($drives | ForEach-Object { $_["DiskPath"] }) |
There was a problem hiding this comment.
This line is unnecessary with the above change
This is a good point, we should now be able to write a backend-agnostic test for using the nvme emulator. I think the closest openvmm test would be We should also install the closed source components on at least one runner before we merge this to make sure the test actually works (even though it is still marked as unstable. |
Adds coverage for HyperV NVMe devices leveraging a closed source emulator
This test is currently marked as unstable, the CI runners are not guaranteed to have the NVMe emulator installed