vmm_tests: add hyper-v openhcl pcat tests #2602
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds Hyper-V OpenHCL PCAT tests by introducing new disk management infrastructure. The changes refactor how boot devices and storage controllers are handled across the test framework.
- Introduces
OpenhclPcatfirmware type and new boot device types (e.g.,IdeViaScsi,ScsiViaNvme) - Refactors storage configuration from imperative
with_custom_vtl2_settingsto declarativeadd_vtl2_storage_controllerAPI - Adds comprehensive disk and storage controller management types (
Drive,Disk,VmbusStorageController) - Implements Hyper-V backend storage APIs with new PowerShell cmdlets for drive management
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vmm_tests/vmm_tests/tests/tests/x86_64/storage.rs | Refactored to use new add_vtl2_storage_controller API instead of with_custom_vtl2_settings |
| vmm_tests/vmm_tests/tests/tests/x86_64/openhcl_uefi.rs | Removed [nvme] annotation, added explicit with_boot_device_type calls |
| vmm_tests/vmm_tests/tests/tests/x86_64/openhcl_linux_direct.rs | Updated to use new storage controller API |
| vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs | Updated to use new storage controller API and boot device type |
| vmm_tests/vmm_tests/tests/tests/multiarch.rs | Added three new hyperv_openhcl_pcat_x64 test configurations |
| vmm_tests/vmm_test_macros/src/lib.rs | Added OpenhclPcat firmware variant, removed nvme option from OpenhclUefiOptions |
| petri/src/vm/vtl2_settings.rs | Added channel field to Vtl2LunBuilder for IDE controller support |
| petri/src/vm/openvmm/start.rs | Simplified by moving boot disk and storage logic to construction phase |
| petri/src/vm/openvmm/runtime.rs | Added stub set_vmbus_drive method |
| petri/src/vm/openvmm/modify.rs | Updated to use new PetriVmProperties |
| petri/src/vm/openvmm/mod.rs | Removed boot device constants, added petri_disk_to_openvmm helper |
| petri/src/vm/openvmm/construct.rs | Major refactoring: added IDE and VMBus storage handling, removed old boot disk logic |
| petri/src/vm/mod.rs | Added core types (PetriVmProperties, VmbusStorageController, Drive, Disk), new boot device types, storage management methods |
| petri/src/vm/hyperv/vm.rs | Refactored storage APIs to use VSID instead of controller numbers |
| petri/src/vm/hyperv/powershell.rs | Added PowerShell functions for storage controller and drive management |
| petri/src/vm/hyperv/mod.rs | Simplified backend, added storage controller iteration, removed HyperVPetriConfig |
| petri/src/vm/hyperv/hyperv.psm1 | Added comprehensive PowerShell cmdlets for SCSI/IDE drive management using WMI |
| petri/src/tracing.rs | Added Debug derives |
| petri/src/disk_image.rs | Added ImageType parameter to build() method to support VHD output |
| petri/petri-tool/src/main.rs | Updated to pass ImageType::Raw to build() |
| }; | ||
|
|
||
| if let Some(agent_disk) = agent_image.and_then(|i| { | ||
| i.build(crate::disk_image::ImageType::Vhd) |
There was a problem hiding this comment.
The function signature includes ImageType parameter but AgentImage::build is called in two places with hardcoded values. In add_agent_disk_inner, it's called with ImageType::Vhd, but the old code didn't specify a type, which would have been Raw. This could change behavior for VTL0 agent images. Verify this is intentional.
| i.build(crate::disk_image::ImageType::Vhd) | |
| i.build(crate::disk_image::ImageType::Raw) |
| } else { | ||
| todo!("dvd ({}) or empty ({})", *is_dvd, disk.is_none()) |
There was a problem hiding this comment.
The todo! for DVD or empty drives means that DVD support is incomplete. This could cause runtime panics if a test tries to add a DVD to a VMBus SCSI controller. Consider returning an error or at least documenting this limitation.
| } | ||
|
|
||
| if ($drive -and (-not $AllowModifyExisting)) { | ||
| throw "drive $Lun on controller $controllerId already exists on vm $vmid" |
There was a problem hiding this comment.
The variable naming is inconsistent: lun (lowercase) is used in the parameter but $Lun (capitalized) is used in the error message. PowerShell is case-insensitive for variables, but consistency improves readability.
| throw "drive $Lun on controller $controllerId already exists on vm $vmid" | |
| throw "drive $lun on controller $controllerId already exists on vm $vmid" |
| /// Guest visible IDE controller number | ||
| pub fn with_channel(mut self, channel: u32) -> Self { | ||
| self.channel = Some(channel); | ||
| self | ||
| } |
There was a problem hiding this comment.
The channel field is added to the Lun protobuf but is marked as Option<u32>. For IDE controllers, the channel maps to the controller number. However, there's no validation that this field is only set for IDE controllers. Consider adding documentation or validation to ensure this field is only used appropriately.
| let mut lun = None; | ||
| for x in 0..u8::MAX as u32 { | ||
| if !self.drives.contains_key(&x) { | ||
| lun = Some(x) |
There was a problem hiding this comment.
The loop logic for finding the first available LUN has a bug. The code sets lun = Some(x) on every iteration where the LUN doesn't exist, meaning it will always use the last checked LUN (0-254), not the first available one. The loop should break after finding the first available LUN.
| lun = Some(x) | |
| lun = Some(x); | |
| break; |
1 participant
Add a few basic Hyper-V OpenHCL PCAT tests using the new disk management infrastructure added in #2551
Will rebase once the previous PR merges