Follow up: Integrate VMM.Perf for Windows - #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
| if updated { | ||
| fs_err::write(profile_path, serde_json::to_vec_pretty(&profile)?).with_context(|| { | ||
| format!( | ||
| "failed to enable guest serial logging in VMM.Perf profile {}", | ||
| profile_path.display() | ||
| ) | ||
| })?; | ||
| } |
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
It introduces a real concurrency/runner-caching risk by mutating cached extracted runtime profiles in-place, and it appears to build/publish an unused aarch64 Windows VMM.Perf runner artifact that adds avoidable CI cost without being consumed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
flowey/flowey_hvlite/src/pipelines/checkin_gates.rs:558
- The Windows artifacts loop creates and publishes a
{arch_tag}-windows-vmm-perf-runnerfor bothaarch64andx64, but only thex64runner is ever wired into the VMM.Perf jobs (use_vmm_perf_runner_windows_x64). This adds unnecessary build time and artifact storage for theaarch64-windows-vmm-perf-runnerthat is currently unused.
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: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
| pub(crate) fn prepare_profile(&self, profile: VmmPerfProfile) -> anyhow::Result<()> { | ||
| let profile_path = self.root.join("profiles").join(profile.file()); | ||
| ensure_file(&profile_path, "VMM.Perf profile")?; | ||
| enable_guest_serial_log(&profile_path) | ||
| } |
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 new *-seeded.raw diagnostics classification is inconsistently case-sensitive vs the already case-insensitive extension handling, which can cause expected disk diagnostics files to be skipped.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
| let category = match ( | ||
| path.file_name().and_then(|name| name.to_str()), | ||
| extension.as_str(), | ||
| ) { | ||
| (Some("metrics.csv"), _) => "results", | ||
| (Some(filename), "raw") if filename.ends_with("-seeded.raw") => { | ||
| "disk-diagnostics" | ||
| } | ||
| (_, "log") => "openvmm-logs", |
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
Local cargo xflowey vmm-perf runs can fail because install_vmm_tests_external_deps is configured with auto_install: None while the local backend requires an explicit true/false.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
flowey/flowey_lib_hvlite/src/_jobs/setup_and_run_vmm_perf.rs:83
install_vmm_tests_external_depsrequiresConfig.auto_installto be explicitly set for the local backend, but this job configuresauto_install: Noneand still requestsRequest::Install. As a result, localcargo xflowey vmm-perfruns will fail early with "Missing essential request: AutoInstall" instead of running (or cleanly skipping) dependency installation.
vmm_tests/vmm_perf/src/virtual_client.rs:383- On Windows,
path_for_virtual_clientstrips the\\?\verbatim prefix unconditionally. For verbatim paths that are not drive-letter paths (e.g.\\?\Volume{...}\...or\\?\GLOBALROOT\...), stripping\\?\produces a relative path, which can break directory resolution. Consider only stripping for\\?\UNC\...and\\?\<DriveLetter>:\..., and otherwise returning the originalPathBuf.
vmm_tests/vmm_perf/src/diagnostics.rs:77
extensionis lowercased before matching, but thefilename.ends_with("-seeded.raw")check is case-sensitive. If VirtualClient produces an uppercase/lowercase-mixed name (e.g.FAILED-GUEST-SEEDED.RAW), this file will be skipped unexpectedly. Consider performing the suffix check case-insensitively.
(Some("metrics.csv"), _) => "results",
(Some(filename), "raw") if filename.ends_with("-seeded.raw") => {
"disk-diagnostics"
}
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
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 preparation