VMM.Perf: add Windows support - #4376
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec48d4d4-a0ec-4ddb-958e-e2646e09f96d
There was a problem hiding this comment.
🟡 Changes recommended
The updated unit test in virtual_client.rs has been weakened enough that it can pass even if the path resolution is incorrect, and should be strengthened to assert the expected canonicalized path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Integrates Windows x64 support for running VMM.Perf locally and in CI, including downloading a pinned Windows runtime package and adding Windows AMD/Intel perf runs to existing Flowey-driven pipelines.
Changes:
- Add Windows path normalization for VMM.Perf work-dir base resolution and relax the corresponding unit test.
- Switch default VMM.Perf VM sizing to a fixed 16 vCPU / 64 GiB shape (while still validating requested capacity).
- Extend Flowey pipelines and generated CI workflows to build/publish Windows VMM.Perf runner artifacts and run dedicated Windows perf jobs.
File summaries
| File | Description |
|---|---|
| vmm_tests/vmm_perf/src/virtual_client.rs | Normalize canonicalized Windows paths for VMM.Perf virtual client consumption; adjust unit test. |
| vmm_tests/vmm_perf/src/config.rs | Change default config generation to a fixed 16 vCPU / 64 GiB shape with capacity validation. |
| Guide/src/dev_guide/dev_tools/xflowey.md | Document Windows x64 host support and the new default VM shape. |
| flowey/flowey_lib_hvlite/src/download_vmm_perf_runtime.rs | Add Windows x64 runtime archive + checksum support and select runtime by host platform. |
| flowey/flowey_hvlite/src/pipelines/vmm_perf.rs | Add windows-x64 target and default target selection based on host OS. |
| flowey/flowey_hvlite/src/pipelines/checkin_gates.rs | Publish Windows VMM.Perf runner artifacts and wire them into perf job setup. |
| ci-flowey/openvmm-pr.yaml | Generated pipeline updates to build/publish VMM.Perf runner artifacts. |
| .github/workflows/openvmm-pr.yaml | Generated workflow updates reflecting new artifacts/steps. |
| .github/workflows/openvmm-pr-release.yaml | Generated workflow updates adding Windows VMM.Perf perf jobs for opt-in PR release validation. |
| .github/workflows/openvmm-ci.yaml | Generated workflow updates adding Windows VMM.Perf perf jobs to CI. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec48d4d4-a0ec-4ddb-958e-e2646e09f96d
There was a problem hiding this comment.
🟡 Changes recommended
There are unresolved review items affecting correctness of stated scope (PR description vs enabled pipelines) and CI efficiency/clarity (unused artifacts plus needed messaging/docs tweaks for the new fixed default VM sizing).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 5
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec48d4d4-a0ec-4ddb-958e-e2646e09f96d
There was a problem hiding this comment.
🔵 Needs a closer look
It makes broad, platform-sensitive changes across multiple CI/workflow pipelines and execution paths (Linux+Windows), which warrants a final human review of the end-to-end CI behavior.
Review details
Suppressed comments (3)
vmm_tests/vmm_perf/src/config.rs:190
- If the fixed default VM shape (16 vCPUs / 64 GiB) exceeds host capacity, the error comes from validate_requested_capacity and doesn’t mention how to override the defaults. Adding context here would make the failure actionable (e.g., point at --vmm-perf-vmsizes / --parameters-json).
super::host::validate_requested_capacity(¶meters, capacity)?;
flowey/flowey_hvlite/src/pipelines/checkin_gates.rs:558
- This adds a Windows VMM.Perf runner artifact for both
aarch64andx86_64(e.g.aarch64-windows-vmm-perf-runner), but there are no consumers for theaarch64artifact in this pipeline. Building + publishing an unused artifact increases CI time and storage; consider gating VMM.Perf runner builds/artifacts tox86_64until anaarch64run job is added.
let (pub_vmm_tests_archive, use_vmm_tests_archive) =
pipeline.new_typed_artifact(format!("{arch_tag}-windows-vmm-tests-archive"));
let (pub_vmm_perf, use_vmm_perf) =
pipeline.new_typed_artifact(format!("{arch_tag}-windows-vmm-perf-runner"));
vmm_tests/vmm_perf/src/virtual_client.rs:421
- This test only exercises the path resolution logic, but doesn’t validate the Windows-only path rewriting (stripping the \?\ prefix / UNC normalization) that this change introduces. Adding a Windows-only assertion here would prevent regressions in the main behavior change.
let resolved = resolve_work_dir_base("relative-base", &runtime_dir)?;
assert!(resolved.is_absolute());
assert!(resolved.ends_with("relative-base"));
assert_eq!(
resolved,
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec48d4d4-a0ec-4ddb-958e-e2646e09f96d
There was a problem hiding this comment.
🔵 Needs a closer look
It makes broad, cross-cutting changes to Flowey pipelines and generated CI workflows (including new Windows perf jobs/artifacts), which warrants careful human validation of CI behavior and resource impact.
Review details
Suppressed comments (1)
flowey/flowey_hvlite/src/pipelines/checkin_gates.rs:557
- The Windows aarch64 build path appears to publish a
*-windows-vmm-perf-runnerartifact even though the only VMM.Perf execution jobs added here are x64 Windows (and onlyuse_vmm_perf_runner_windows_x64/use_vmm_perf_openvmm_windows_x64are wired up). This means the aarch64-windows VMM.Perf runner build/publish work is likely unused extra CI cost; either wire up a consumer for the aarch64 runner or gate the build/publish to x86_64 Windows only.
let (pub_test_igvm_agent_rpc_server, use_test_igvm_agent_rpc_server) = pipeline
.new_typed_artifact(format!("{arch_tag}-windows-test_igvm_agent_rpc_server"));
let (pub_vmm_tests_archive, use_vmm_tests_archive) =
pipeline.new_typed_artifact(format!("{arch_tag}-windows-vmm-tests-archive"));
let (pub_vmm_perf, use_vmm_perf) =
pipeline.new_typed_artifact(format!("{arch_tag}-windows-vmm-perf-runner"));
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec48d4d4-a0ec-4ddb-958e-e2646e09f96d
There was a problem hiding this comment.
🔵 Needs a closer look
It makes broad, cross-platform changes spanning VMM.Perf runtime behavior and multiple CI/workflow pipelines that need human validation in real CI environments.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec48d4d4-a0ec-4ddb-958e-e2646e09f96d
Update the pinned runtime to 20260906.1, add the Windows ARM64 archive, and run VMM.Perf on the ARM64 bare-metal pool. Remove temporary forced serial logging and failed seeded-disk publication while retaining the local runtime archive override. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec48d4d4-a0ec-4ddb-958e-e2646e09f96d
There was a problem hiding this comment.
🔵 Needs a closer look
The implemented CI pipeline adds an ARM64 Windows VMM.Perf job that is not reflected in the PR description, so the scope/intent needs alignment (doc update or code adjustment) before approval.
Review details
Suppressed comments (1)
flowey/flowey_hvlite/src/pipelines/checkin_gates.rs:1874
- The PR description mentions adding dedicated AMD/Intel Windows VMM.Perf jobs (and a pinned Windows x64 runtime), but this pipeline also schedules an additional
aarch64-windowsVMM.Perf job. If ARM64 Windows coverage is intentional, the PR description (and any release/CI notes) should be updated to mention the extra job/runtime; otherwise, remove this entry to keep CI scope aligned with the stated intent.
(
"aarch64-windows",
FlowPlatform::Windows,
FlowArch::Aarch64,
CommonArch::Aarch64,
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec48d4d4-a0ec-4ddb-958e-e2646e09f96d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec48d4d4-a0ec-4ddb-958e-e2646e09f96d
Remove the ARM64 Windows VMM.Perf job until the GNU sparse VHD extraction issue is fixed. Keep runtime 20260906.1 and the existing x64 VMM.Perf jobs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec48d4d4-a0ec-4ddb-958e-e2646e09f96d
There was a problem hiding this comment.
🔵 Needs a closer look
It makes broad CI/pipeline changes and adds new Windows-host execution paths whose correctness and stability depend on external environment/runtime assumptions best validated by a human reviewer and CI results.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
Run VMM.Perf after merge or through the opt-in release PR pipeline instead of regular PR validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec48d4d4-a0ec-4ddb-958e-e2646e09f96d
There was a problem hiding this comment.
🟡 Changes recommended
The updated cargo xflowey vmm-perf Guide text states the default scratch dir is target/vmm_perf/temp, but the pipeline actually uses target/vmm_perf/t, which will mislead users.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec48d4d4-a0ec-4ddb-958e-e2646e09f96d
There was a problem hiding this comment.
🟡 Changes recommended
Windows VMM.Perf jobs in openvmm-ci.yaml are gated on github.event.pull_request even though the workflow runs on push/dispatch, so those jobs will never run.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/workflows/openvmm-ci.yaml:7199
- Same issue as job30: this workflow runs on
push/workflow_dispatch, so gating ongithub.event.pull_request.draftwill skip the job entirely in CI.
needs:
- job6
- job7
if: github.event.pull_request.draft == false
steps:
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
| needs: | ||
| - job6 | ||
| - job7 | ||
| if: github.event.pull_request.draft == false |
Trevor Jones (tjones60)
added
the
release-ci-required
Trevor Jones (tjones60)
changed the title
e5d7246
into
microsoft:main
Summary
x64-windows-amd-vmm-perf-resultsandx64-windows-intel-vmm-perf-resultsthrough the existing Azure metrics upload flowinstall_vmm_tests_external_depsFlowey step to enable Windows Hyper-V/WHP management components while preserving Linux KVM/MSHV device and huge-page preparationCancelIoEx(handle, NULL)to cancel pending overlapped I/O regardless of the issuing thread