Enable canonical OpenVMM Linux boot test across backends - #111
Íñigo Goiri (goiri) with Copilot wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: goiri <3159513+goiri@users.noreply.github.com>
Co-authored-by: goiri <3159513+goiri@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The current OpenVMM Flowey path rejects the required Linux pipette artifact on Windows/WHP.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: goiri <3159513+goiri@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Windows/WHP remains excluded despite the stated three-backend requirement.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: goiri <3159513+goiri@users.noreply.github.com>
Co-authored-by: goiri <3159513+goiri@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The KVM runner cannot install the newly required musl target into its protected Rustup home.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
scripts/nvx_tools/ci.py:72
- This still omits
openvmm_linux_x64_bootfrom the Windows/WHP command, while the current PR title/description and issue #83 require the exact test on KVM, MSHV, and WHP. The WHP matrix calls this function on Windows, so it receives onlyOPENVMM_MICROVM_TEST_FILTER. Either add a supported Windows artifact path/pin or update the PR and issue scope so they no longer claim three-backend coverage.
if os.name != "nt":
targets.append(OPENVMM_LINUX_RUST_TARGET)
test_filter = f"{test_filter} + {OPENVMM_LINUX_X64_BOOT_TEST_FILTER}"
scripts/nvx_tools/ci.py:72
- This Linux-only branch leaves the Windows/WHP matrix running only the two existing tests, so the implementation does not satisfy the PR description/title or issue #83's explicit WHP acceptance criterion for
openvmm_linux_x64_boot. Either add a supported Windows artifact path before claiming the three-backend coverage, or update the PR metadata andFixes #83scope so it no longer claims to enable this test on WHP.
if os.name != "nt":
targets.append(OPENVMM_LINUX_RUST_TARGET)
test_filter = f"{test_filter} + {OPENVMM_LINUX_X64_BOOT_TEST_FILTER}"
scripts/nvx_tools/ci.py:72
- Because this condition excludes
openvmm_linux_x64_bootwhenever the host is Windows, the Windows/WHP matrix entry still runs only the two legacy tests. That conflicts with this PR's "across backends" description and issue #83's explicit WHP acceptance criterion. Either provide a supported Windows artifact path or update the PR/issue scope to state that the new test is Linux-only; the current code cannot satisfy the stated goal.
if os.name != "nt":
targets.append(OPENVMM_LINUX_RUST_TARGET)
test_filter = f"{test_filter} + {OPENVMM_LINUX_X64_BOOT_TEST_FILTER}"
scripts/nvx_tools/ci.py:72
- This branch deliberately omits
openvmm_linux_x64_bootfor every valid Windows/WHP invocation, so that matrix entry still runs only the two existing tests while the PR title/description and issue acceptance criteria say the canonical test is enabled across KVM, MSHV, and WHP. Please either add a supported Windows artifact path before enabling it here, or update the PR/issue scope to explicitly exclude WHP.
if os.name != "nt":
targets.append(OPENVMM_LINUX_RUST_TARGET)
test_filter = f"{test_filter} + {OPENVMM_LINUX_X64_BOOT_TEST_FILTER}"
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: goiri <3159513+goiri@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The implementation excludes WHP while the PR and linked issue still claim three-backend coverage.
Review details
Suppressed comments (3)
scripts/nvx_tools/ci.py:72
- This host guard still excludes
openvmm_linux_x64_bootfrom Windows/WHP, while the current PR title/description claim coverage across backends andFixes #83; issue #83 explicitly requires this exact test on WHP. The Windows matrix calls this function and therefore retains only the two existing tests. Either add a supported Windows-host pipette artifact path/OpenVMM pin, or narrow the PR and issue scope and stop claiming #83 is fixed.
if os.name != "nt":
targets.append(OPENVMM_LINUX_RUST_TARGET)
test_filter = f"{test_filter} + {OPENVMM_LINUX_X64_BOOT_TEST_FILTER}"
scripts/nvx_tools/ci.py:72
- This branch deliberately appends
openvmm_linux_x64_bootonly on non-Windows hosts, so the WHP matrix still runs just the two existing tests. That conflicts with the current PR description and issue #83, which promise execution on KVM, MSHV, and WHP (and mark the Windows/WHP acceptance criterion as required). Either add a supported Windows artifact path before enabling this filter, or update the PR/issue scope and acceptance criteria to state that this change is Linux-only.
if os.name != "nt":
targets.append(OPENVMM_LINUX_RUST_TARGET)
test_filter = f"{test_filter} + {OPENVMM_LINUX_X64_BOOT_TEST_FILTER}"
scripts/nvx_tools/ci.py:72
- This guard still makes the Windows/WHP matrix run only the two existing tests, while the current PR description and issue #83 acceptance criteria state that
openvmm_linux_x64_bootis enabled across KVM, MSHV, and WHP. Please either add a supported Windows artifact path before claiming three-backend coverage, or update the PR/issue scope to explicitly exclude WHP.
if os.name != "nt":
targets.append(OPENVMM_LINUX_RUST_TARGET)
test_filter = f"{test_filter} + {OPENVMM_LINUX_X64_BOOT_TEST_FILTER}"
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: goiri <3159513+goiri@users.noreply.github.com>
Addressed in a6d305c. KVM/MSHV use the provisioned musl target, while the exact pinned OpenVMM source now builds and uploads its Linux pipette for WHP; Windows prepares the remaining artifacts with the supported base selector and runs the complete selector through |
There was a problem hiding this comment.
🟡 Changes recommended
The WHP cold-cache path does not stage all Linux-direct boot artifacts required by the new test.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
| openvmm-tests: | ||
| name: OpenVMM tests / ${{ matrix.name }} | ||
| needs: [openvmm-changes] | ||
| needs: [openvmm-changes, openvmm-binaries] |
| build_command[build_command.index("--filter") + 1] = ( | ||
| OPENVMM_MICROVM_BASE_TEST_FILTER | ||
| ) |
| self.assertIn("--ci-profile", command) | ||
| self.assertIn("--skip-vhd-prompt", command) |
Adds the upstream Linux-direct boot smoke test to NVX’s existing KVM, MSHV, and WHP OpenVMM matrix. The existing fork-specific PVH lifecycle and ttrpc snapshot coverage remains selected.
OpenVMM test selection
OPENVMM_MICROVM_TEST_FILTERwith the canonical boot test:vmm-tests-run --release --ci-profile --skip-vhd-promptinvocation, preserving Flowey-managed artifacts, pipette interaction, guest poweroff, and teardown diagnostics.Focused regression coverage