[Storage] Windows Session Scoped Data Source + CDI Clone and Hotplug implementation of it#4571
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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
|
/verified |
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.
Still blocked: The CodeRabbit GitHub App installation on What actually needs to happen: A repo/org admin grants the CodeRabbit GitHub App Pull requests: Read & write permission (Settings → GitHub Apps → CodeRabbit → Permissions). Until then, this automated workflow cannot post here. I'd ask that this identical request not be retriggered further on this PR — it consumes cycles without a different outcome. If the permission automation is misconfigured, that's worth fixing at the workflow level rather than retrying against the same blocked PR repeatedly. (－_－) zzz... sixth time, still waiting on that permission grant. |
|
Clean rebase detected — no code changes compared to previous head ( |
|
/verified cancel |
|
/verified |
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.
Status unchanged: The CodeRabbit GitHub App on Please hold off on further automated retriggers of this exact request until that permission is granted; I've already detailed the analysis and blocker three times earlier in this thread and won't repeat it a fourth time to avoid adding more noise. (－ω－) still zzz-ing on that permission grant... |
|
/approve |
|
/verified cancel |
|
/verified |
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.
(－ω－) zzz... |
chill, this was just to re-trigger the smoke test CI because there's no command that can do it |
|
Tip For best results, initiate chat on the files or code changes.
(・ω・)ノ |
|
/lgtm |
|
/check-can-merge |
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-4571. |
|
/cherry-pick cnv-4.22 cnv-4.21 cnv-4.20 |
|
Manual cherry-pick is needed |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
|
Manual cherry-pick is needed |
|
Manual cherry-pick is needed |
13 participants
Short description:
Using
validation-os-imagesnamespace, we import the Windows image only once and it can be used by other modules because it's session scoped.More details:
This PR also makes changes in Hotplug and CDI Clone modules to take advantage of this new approach. Other modules will follow.
What this PR does / why we need it:
This creates session scoped Data Source fixture that imports the image only once if needed or uses a golden image already on a cluster.
It also makes CDI Clone and Hotplug modules utilising to fixture as a proof of concept.
Which issue(s) this PR fixes:
Special notes for reviewer:
Co-Authored: Claude Code
https://redhat.atlassian.net/browse/CNV-51351
jira-ticket: