[Storage] Windows tests should all use vTPM - module CDI Clone#4571
[Storage] Windows tests should all use vTPM - module CDI Clone#4571acinko-rh wants to merge 32 commits into
Conversation
|
/wip |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces a parametrized HTTP-based Windows DV clone test with a new tier3 test that builds a VM from a registry-backed cloned DV (Windows 2022) including vTPM. Adds fixtures to provision source and cloned DataVolumes from a registry and introduces a Windows 2K22 preference constant. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
acinko-rh
changed the title
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
requested review from
josemacassan and
kshvaika
openshift-virtualization-qe-bot
requested review from
Ahmad-Hafe,
jpeimer and
kgoldbla
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/storage/utils.py (1)
541-584:⚠️ Potential issue | 🟠 Major[HIGH] Landing a helper already labeled "DEBUG ONLY - MARKED FOR DELETION" contradicts the dead-code policy.
The docstring and the trailing
# TODO: Remove - debug onlyon Line 541 announce this function is scaffolding, yet it's imported and exercised bytest_successful_vm_from_cloned_dv_windows_with_vtpmintests/storage/cdi_clone/test_clone.py. Either the verification is part of the intended coverage (then drop the "debug only" labeling and keep it properly) or it isn't (then don't merge it). Shipping code pre-flagged for deletion is how dead code quietly permanents itself in the repo.As per coding guidelines: "No dead code in Python. Every function, variable, fixture MUST be used or removed."
✂️ Suggested cleanup
-def verify_vtpm_in_windows_vm(vm, admin_client): # TODO: Remove - debug only +def verify_vtpm_in_windows_vm(vm, admin_client): """ Verify vTPM is properly configured and detected in a Windows VM. - **DEBUG ONLY - MARKED FOR DELETION** - Performs two-layer validation:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/utils.py` around lines 541 - 584, The helper verify_vtpm_in_windows_vm is labelled "DEBUG ONLY - MARKED FOR DELETION" yet is referenced by test_successful_vm_from_cloned_dv_windows_with_vtpm, so remove the dead-code marker and make the function a first-class test utility: delete the "# TODO: Remove - debug only" comment, update the docstring to reflect it is an intentional test helper (remove "DEBUG ONLY"), and ensure any module exports/imports remain correct so tests continue to import verify_vtpm_in_windows_vm; alternatively, if you truly intend to delete it, remove all imports/usages (notably in tests/storage/cdi_clone/test_clone.py) and delete the function instead.tests/storage/cdi_clone/test_clone.py (1)
1-3:⚠️ Potential issue | 🟡 Minor[MEDIUM] New feature test requires an STP (or RFE/Jira epic) link in the module docstring.
The module docstring is still just
"""Clone tests""", but this PR adds a new feature test (test_successful_vm_from_cloned_dv_windows_with_vtpm) covering vTPM on cloned Windows DVs. As per coding guidelines: "Every new feature test module MUST include an STP link in the module docstring. If there is no STP, the module docstring MUST include a link to the RFE or Jira epic (not support cases) for coverage tracking." The PR description also has an empty Jira ticket field, which needs to be filled in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/storage/cdi_clone/test_clone.py` around lines 1 - 3, The module docstring currently just says "Clone tests" but this PR adds the new feature test test_successful_vm_from_cloned_dv_windows_with_vtpm, so update the module docstring to include the required STP link (or, if none, the RFE/Jira epic URL) for coverage tracking and also populate the empty Jira ticket field in the PR description; modify the top-of-file docstring in the test module to include a one-line reference (e.g., "STP: <link>" or "RFE/Jira: <epic-link>") alongside the existing summary so automated checks and reviewers can verify the traceability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/storage/cdi_clone/test_clone.py`:
- Line 248: The test contains a placeholder Polarion ID in the pytest marker:
replace pytest.mark.polarion("CNV-XXXXX") in
tests/storage/cdi_clone/test_clone.py with the real Polarion/Jira ID for this
test (use the official ticket string), and update the PR description’s empty
Jira ticket field to the same ID so reporting and traceability are correct;
ensure the marker value is a non-placeholder string and re-run the
test/reporting to verify the ID appears properly.
- Around line 72-110: Move the helper
create_windows_vm_with_vtpm_validate_guest_agent_info out of the test module
into tests/storage/utils.py (next to
create_windows_vm_validate_guest_agent_info), make admin_client a required
positional argument (remove the =None default) so vTPM validation always runs,
relocate the imports to the top of the new module (import
validate_os_info_vmi_vs_windows_os from utilities.ssp, wait_for_windows_vm from
utilities.virt, and verify_vtpm_in_windows_vm from tests.storage.utils or the
correct canonical module), and remove the runtime None check (do not gate
verify_vtpm_in_windows_vm with if admin_client; call it directly). Ensure any
remaining None comparisons use identity (if x is None) when applicable.
- Line 103: The test creates the VM in a halted state
(VirtualMachine.RunStrategy.HALTED) so wait_for_windows_vm(vm=vm, ...) fails
because it immediately runs SSH; call running_vm(vm=vm) to transition the VM to
Running before calling wait_for_windows_vm. Insert a running_vm(vm=vm)
invocation inside the context manager just prior to wait_for_windows_vm
(mirroring create_windows19_vm) so the VM is started and SSH connectivity is
available.
---
Outside diff comments:
In `@tests/storage/cdi_clone/test_clone.py`:
- Around line 1-3: The module docstring currently just says "Clone tests" but
this PR adds the new feature test
test_successful_vm_from_cloned_dv_windows_with_vtpm, so update the module
docstring to include the required STP link (or, if none, the RFE/Jira epic URL)
for coverage tracking and also populate the empty Jira ticket field in the PR
description; modify the top-of-file docstring in the test module to include a
one-line reference (e.g., "STP: <link>" or "RFE/Jira: <epic-link>") alongside
the existing summary so automated checks and reviewers can verify the
traceability.
In `@tests/storage/utils.py`:
- Around line 541-584: The helper verify_vtpm_in_windows_vm is labelled "DEBUG
ONLY - MARKED FOR DELETION" yet is referenced by
test_successful_vm_from_cloned_dv_windows_with_vtpm, so remove the dead-code
marker and make the function a first-class test utility: delete the "# TODO:
Remove - debug only" comment, update the docstring to reflect it is an
intentional test helper (remove "DEBUG ONLY"), and ensure any module
exports/imports remain correct so tests continue to import
verify_vtpm_in_windows_vm; alternatively, if you truly intend to delete it,
remove all imports/usages (notably in tests/storage/cdi_clone/test_clone.py) and
delete the function instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 11e3ec8d-d6fa-4f0b-9a1f-4e1aa2980145
📒 Files selected for processing (2)
tests/storage/cdi_clone/test_clone.pytests/storage/utils.py
openshift-virtualization-qe-bot
removed
the
tox:verify-tc-requirement-polarion:passed
Signed-off-by: Adam Cinko <acinko@redhat.com>
|
Clean rebase detected — no code changes compared to previous head ( |
|
Clean rebase detected — no code changes compared to previous head ( |
|
Clean rebase detected — no code changes compared to previous head ( |
|
Clean rebase detected — no code changes compared to previous head ( |
|
/build-and-push-container |
|
Verification failed for PR #4571. Execution details |
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-4571 published |
|
/verified All tests passed for PR #4571. Execution details |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Check smoke/gating markers in tests/storage/cdi_clone/ ==="
rg '`@pytest.mark.smoke`|`@pytest.mark.gating`|`@pytest.mark.tier`' tests/storage/cdi_clone/ -n
echo ""
echo "=== Find all usages of WINDOWS_2K22_PREFERENCE ==="
rg 'WINDOWS_2K22_PREFERENCE' -n
echo ""
echo "=== Find smoke-marked test files ==="
rg -l '`@pytest.mark.smoke`' tests/ | head -30
echo ""
echo "=== Find gating-marked test files ==="
rg -l '`@pytest.mark.gating`' tests/ | head -30
echo ""
echo "=== Check if any gating tests import from utilities/constants.py or use WINDOWS_2K22_PREFERENCE ==="
rg -l '`@pytest.mark.gating`' tests/ | xargs -I{} grep -l 'WINDOWS_2K22_PREFERENCE' {} 2>/dev/null || echo "No gating test uses WINDOWS_2K22_PREFERENCE"
echo ""
echo "=== Check conftest hierarchy for smoke tests that import WINDOWS_2K22_PREFERENCE ==="
# Check conftest.py files in cdi_clone hierarchy
for f in tests/storage/cdi_clone/conftest.py tests/storage/conftest.py tests/conftest.py conftest.py; do
if [ -f "$f" ]; then
echo "--- $f ---"
grep -n 'WINDOWS_2K22_PREFERENCE\|smoke\|gating' "$f" || echo "(no match)"
fi
done
echo ""
echo "=== Check markers on the new test ==="
rg 'tier|smoke|gating|vtpm|cloned_dv' tests/storage/cdi_clone/test_clone.py -n -A2 -B2Length of output: 6114 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get the PR diff to find first changed line position in the first file
# First, get commits on the PR to find base/head SHAs
gh pr view 4571 --repo RedHatQE/openshift-virtualization-tests --json headRefOid,baseRefOid
echo ""
echo "=== Get diff to find first changed line ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4571/files --jq '.[0] | {filename, patch: .patch[0:500]}'Length of output: 1204 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Post inline PR review comment on first changed line of first file
gh api \
--method POST \
repos/RedHatQE/openshift-virtualization-tests/pulls/4571/comments \
--field body='**Test Execution Plan**
- **Run smoke tests: False** — The smoke test in `tests/storage/cdi_clone/test_clone.py` (`test_clone_from_block_to_fs_using_dv_template`, s390x) does not use the new Windows fixtures (`source_dv_windows_registry_scope_function`, `cloned_windows_dv_from_registry_scope_function`) or the new `WINDOWS_2K22_PREFERENCE` constant. No conftest in the hierarchy imports these new symbols.
- **Run gating tests: False** — The gating tests in `test_clone.py` (`test_successful_vm_restart_with_cloned_dv`, CNV-3545 parametrized, CNV-5607) do not depend on the new Windows fixtures or the new constant. No other gating-marked file traces through `WINDOWS_2K22_PREFERENCE` → `create_windows2022_vm_with_vtpm`.
**Affected tests to run:**
- `tests/storage/cdi_clone/test_clone.py::test_successful_vm_from_cloned_dv_windows` (tier3) — new test exercising the new fixtures and `WINDOWS_2K22_PREFERENCE` constant end-to-end' \
--field commit_id='b7faa5ca19fbf2ec9e3e29f151cc3c5d5072112a' \
--field path='tests/storage/cdi_clone/conftest.py' \
--field position=5Length of output: 4997 |
|
Clean rebase detected — no code changes compared to previous head ( |
|
/lgtm |
|
Clean rebase detected — no code changes compared to previous head ( |
|
/lgtm |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4994. Overlapping filestests/storage/test_hotplug.py |
| @pytest.fixture() | ||
| def source_dv_windows_registry_scope_function( | ||
| unprivileged_client, | ||
| namespace, | ||
| storage_class_name_scope_function, | ||
| ): | ||
| """Fixture that creates a Windows 2022 DataVolume from registry.""" |
There was a problem hiding this comment.
For some of the Win tests across the modules, we can reuse this DV.
So we can create a source DV once and then clone from it for almost all the tests.
Let's have a session scope fixture that will be reused across the modules.
There is a similar fixture latest_windows_data_volume - https://github.com/RedHatQE/openshift-virtualization-tests/pull/4898/changes
I don't think we can reuse it because it's using default_sc, but we can create our own, very similar to that, but using the storage class matrix. (I'm just not yet sure how we will handle the tests with storage_class_matrix_snapshot_matrix__ or any other special matrix, but we can get to that later)
There was a problem hiding this comment.
couldn't find way to make it work... because session scoped dv is in one namespace and uses one storage class, we can have a conversation how to implement this...
| data_volume_multi_storage_scope_function, | ||
| vm_params, | ||
| namespace, | ||
| modern_cpu_for_migration, |
There was a problem hiding this comment.
We are not migrating this VM, so we can remove this fixture.
There was a problem hiding this comment.
@vsibirsk adviced to use this CPU for Windows test, is there any other CPU type I should use or shall I leave it blank?
| pytest.param( | ||
| { | ||
| "dv_name": "dv-source-win", | ||
| "image": f"{Images.Windows.DIR}/{Images.Windows.WIN11_IMG}", | ||
| "dv_size": Images.Windows.DEFAULT_DV_SIZE, | ||
| }, | ||
| marks=(pytest.mark.polarion("CNV-3552"), pytest.mark.tier3()), |
There was a problem hiding this comment.
There are more Windows tests in this module, do you want to include them in this PR?
There was a problem hiding this comment.
they are just verifying the DV without making a VM and the scope of the ticket is to just care for the VMs, should I change the Images to Win2k22 or something?
Signed-off-by: Adam Cinko <acinko@redhat.com>
Signed-off-by: Adam Cinko <acinko@redhat.com>
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4904. Overlapping filestests/storage/conftest.py |
13 participants
Short description:
PR merged to tier-2/3 that makes Windows test VMs in sig-storage use vTPM
(preferably through instancetypes, which should be the right and easy way to achieve this)
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Co-Authored: Claude Code
https://redhat.atlassian.net/browse/CNV-51351
jira-ticket: