-
Notifications
You must be signed in to change notification settings - Fork 163
vmm_tests: enable uefi and pcat openhcl servicing tests #2683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enables UEFI and PCAT Hyper-V x64 OpenHCL servicing tests that were previously failing in CI. The tests are now configured to run on CVM runners which have a newer host OS that should resolve issue #1652.
Changes:
- Enabled
hyperv_openhcl_pcat_x64andhyperv_openhcl_uefi_x64test configurations for thebasic_servicingtest - Moved servicing test execution from standard runners to CVM runners (except for ADO backend where they remain disabled due to GitHub authentication limitations)
- Consolidated filter logic to unconditionally exclude servicing tests from standard runners
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs | Uncommented two previously disabled Hyper-V x64 test configurations (PCAT and UEFI) for the basic_servicing test |
| flowey/flowey_hvlite/src/pipelines/checkin_gates.rs | Refactored test filters to exclude servicing tests from standard runners and conditionally include them in CVM runners (except ADO backend) |
| enable_nvme_keepalive: false, // TODO: Support NVMe KA in the Hyper-V Petri Backend | ||
| enable_mana_keepalive: false, | ||
| override_version_checks: false, | ||
| override_version_checks: true, // TODO: figure out why our tests don't pass the version check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the HyperV variant of the servicing test actually change the underlying firmware? IIRC I had to remove that when I brought these online (meaning that these are just "null payload" tests). Maybe that's why?
PS ... huzzaaahhh!! Thanks for turning these on :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I see you fixed that as well. Bummer. Do we have an issue to track this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mattkur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor process question, but overall I think this LGTM.
| enable_nvme_keepalive: false, // TODO: Support NVMe KA in the Hyper-V Petri Backend | ||
| enable_mana_keepalive: false, | ||
| override_version_checks: false, | ||
| override_version_checks: true, // TODO: figure out why our tests don't pass the version check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I see you fixed that as well. Bummer. Do we have an issue to track this?
| // Currently, we don't have a good way for ADO runners to authenticate in GitHub | ||
| // (that don't involve PATs) which is a requirement to download GH Workflow Artifacts | ||
| // required by the servicing tests. For now, we will exclude servicing tests from running | ||
| // in the internal mirror. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recognize this is not a new restriction, but this seems overly restrictive. Presumably we only need these artifacts for the tests that do servicing to/from published revisions. A "null payload" servicing test should just need the pipeline build .bin file artifact, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah can probably change that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These filters are getting a bit out of hand. More motivation for doing better automatic dependency detection and updating the runners, but we should get these tests running first.
petri/src/vm/hyperv/mod.rs
Outdated
| // overwrite the igvm file | ||
| fs_err::copy(new_openhcl.get(), self.temp_dir.path().join(IGVM_FILE_NAME)) | ||
| .context("failed to replace igvm file")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please leave a comment indicating why a cold patch is sufficient, as opposed to calling the WMI to update the firmware path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mike said this is how it works in Azure. We cold patch the vmfirmwareigvm.dll and then when you restart openhcl it picks up the new version. Here we have configured the vm to pick up the new version from a custom bin, but it works the same. I can expand on the comment a bit.
Enable UEFI and PCAT Hyper-V x64 OpenHCL servicing tests using the CVM runners. This should hopefully resolve #1652, since they have a newer host OS. Also enables downgrade and upgrade servicing for Hyper-V by cold patching the OpenHCL binary.