[Storage] refactor snapshots test to use win 2k22 and vtpm#4687
[Storage] refactor snapshots test to use win 2k22 and vtpm#4687acinko-rh wants to merge 16 commits into
Conversation
|
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4687 +/- ##
==========================================
- Coverage 98.67% 98.65% -0.02%
==========================================
Files 25 25
Lines 2487 2457 -30
==========================================
- Hits 2454 2424 -30
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/wip |
openshift-virtualization-qe-bot-5
requested review from
Ahmad-Hafe,
dshchedr,
ema-aka-young,
geetikakay,
jpeimer,
kgoldbla,
kshvaika and
rnetser
openshift-virtualization-qe-bot-5
changed the title
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughReplace legacy Windows-2019 snapshot fixtures with VTpm-enabled Windows 2022 DV-template fixtures; update downstream snapshot fixtures/tests, rename registry-based helpers to template variants, remove the old create_windows19_vm helper, and add a WINDOWS_2K22_PREFERENCE constant. ChangesWindows Snapshot Test Fixture Migration
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
acinko-rh
force-pushed
the
add_vtpm_to_win_machines_snapshots
branch
from
6eacf99 to
25e81ae
Compare
|
Clean rebase detected — no code changes compared to previous head ( |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:passed
Signed-off-by: Adam Cinko <acinko@redhat.com>
for more information, see https://pre-commit.ci
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Signed-off-by: Adam Cinko <acinko@redhat.com>
|
made a mild refactor of the helper function names and their reference fix so it doesn't break any other tests (currently only |
|
/wip cancel |
Signed-off-by: Adam Cinko <acinko@redhat.com>
|
/lgtm |
|
Remember to update the PR description to make coderabbit and reviewers happier :) |
|
@ema-aka-young done, thx for pointing it out |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4727. Overlapping filestests/storage/utils.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4954. Overlapping filestests/utils.py |
|
Clean rebase detected — no code changes compared to previous head ( |
|
/lgtm |
|
Clean rebase detected — no code changes compared to previous head ( |
|
Clean rebase detected — no code changes compared to previous head ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/storage/snapshots/conftest.py`:
- Around line 53-60: The fixture windows_dv_template_from_registry
unconditionally reads request.param["dv_name"] but
test_write_to_file_while_snapshot does not indirect-parametrize it, causing a
missing request.param; fix by making the test parametrize
windows_dv_template_from_registry as well (add it to the
`@pytest.mark.parametrize`(..., indirect=True) list with the dv_name dict) or
refactor to extract a separate parametrized dv_name fixture consumed by both
windows_dv_template_from_registry and windows_vm_with_vtpm_for_snapshot so
neither fixture reads request.param directly; update references to
create_windows2022_dv_template_from_registry accordingly.
In `@tests/storage/utils.py`:
- Around line 300-314: Before starting dv.watcher, check the current DataVolume
conditions on dv (e.g., (dv.status or {}).get("conditions", [])) using the same
message-match logic as inside the loop and return early if the expected_message
is already present; keep the existing variables (last_conditions, deleted) and
behavior otherwise so the subsequent watcher loop only handles future events and
avoids false negatives and DataVolumeConditionMessageNotFoundError.
In `@tests/utils.py`:
- Line 760: Update the docstring to reference the actual parameter name
dv_template (not dv_dict) wherever the parameter is described; locate the
function or helper in tests/utils.py that declares dv_template (the parameter
named dv_template on or around line 750) and replace occurrences of dv_dict in
the docstring with dv_template so the docstring and parameter list match.
In `@utilities/constants.py`:
- Around line 962-964: Remove the duplicate WINDOWS_2K22_PREFERENCE assignment
and rely on the existing WINDOWS_2K22_PREFERENCE definition already present in
the file; specifically, keep WINDOWS_11_PREFERENCE as defined here but delete
the second WINDOWS_2K22_PREFERENCE line so there is a single source of truth for
WINDOWS_2K22_PREFERENCE (do not redefine or shadow it).
🪄 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: c2dc3872-d2ac-40ce-a32c-9c9322d3c64f
📒 Files selected for processing (6)
tests/storage/snapshots/conftest.pytests/storage/snapshots/test_snapshots.pytests/storage/test_hotplug.pytests/storage/utils.pytests/utils.pyutilities/constants.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/storage/snapshots/conftest.py`:
- Around line 53-60: The fixture windows_dv_template_from_registry
unconditionally reads request.param["dv_name"] but
test_write_to_file_while_snapshot does not indirect-parametrize it, causing a
missing request.param; fix by making the test parametrize
windows_dv_template_from_registry as well (add it to the
`@pytest.mark.parametrize`(..., indirect=True) list with the dv_name dict) or
refactor to extract a separate parametrized dv_name fixture consumed by both
windows_dv_template_from_registry and windows_vm_with_vtpm_for_snapshot so
neither fixture reads request.param directly; update references to
create_windows2022_dv_template_from_registry accordingly.
In `@tests/storage/utils.py`:
- Around line 300-314: Before starting dv.watcher, check the current DataVolume
conditions on dv (e.g., (dv.status or {}).get("conditions", [])) using the same
message-match logic as inside the loop and return early if the expected_message
is already present; keep the existing variables (last_conditions, deleted) and
behavior otherwise so the subsequent watcher loop only handles future events and
avoids false negatives and DataVolumeConditionMessageNotFoundError.
In `@tests/utils.py`:
- Line 760: Update the docstring to reference the actual parameter name
dv_template (not dv_dict) wherever the parameter is described; locate the
function or helper in tests/utils.py that declares dv_template (the parameter
named dv_template on or around line 750) and replace occurrences of dv_dict in
the docstring with dv_template so the docstring and parameter list match.
In `@utilities/constants.py`:
- Around line 962-964: Remove the duplicate WINDOWS_2K22_PREFERENCE assignment
and rely on the existing WINDOWS_2K22_PREFERENCE definition already present in
the file; specifically, keep WINDOWS_11_PREFERENCE as defined here but delete
the second WINDOWS_2K22_PREFERENCE line so there is a single source of truth for
WINDOWS_2K22_PREFERENCE (do not redefine or shadow it).
🪄 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: c2dc3872-d2ac-40ce-a32c-9c9322d3c64f
📒 Files selected for processing (6)
tests/storage/snapshots/conftest.pytests/storage/snapshots/test_snapshots.pytests/storage/test_hotplug.pytests/storage/utils.pytests/utils.pyutilities/constants.py
📜 Review details
🔇 Additional comments (7)
tests/utils.py (2)
704-746: LGTM!
7-7: LGTM!Also applies to: 22-23, 552-556
tests/storage/snapshots/test_snapshots.py (2)
400-428: LGTM!
431-453: LGTM!tests/storage/test_hotplug.py (3)
14-14: LGTM!
75-87: LGTM!
91-105: LGTM!
🛑 Comments failed to post (4)
tests/storage/snapshots/conftest.py (1)
53-60:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify which fixture is actually parametrized in the snapshot tests. # Expected: if only `windows_vm_with_vtpm_for_snapshot` is parametrized, # then `request.param` is unavailable inside `windows_dv_template_from_registry`. rg -n -C3 'windows_vm_with_vtpm_for_snapshot|windows_dv_template_from_registry|parametrize\(' tests/storage/snapshotsRepository: RedHatQE/openshift-virtualization-tests
Length of output: 13014
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) Inspect fixture implementation around the reported lines nl -ba tests/storage/snapshots/conftest.py | sed -n '1,140p' # 2) Inspect the test parametrizations around the indirect usage nl -ba tests/storage/snapshots/test_snapshots.py | sed -n '360,520p'Repository: RedHatQE/openshift-virtualization-tests
Length of output: 126
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== tests/storage/snapshots/conftest.py (top + fixtures) ===" cat -n tests/storage/snapshots/conftest.py | sed -n '1,160p' echo "=== tests/storage/snapshots/test_snapshots.py (indirect parametrization) ===" cat -n tests/storage/snapshots/test_snapshots.py | sed -n '350,480p'Repository: RedHatQE/openshift-virtualization-tests
Length of output: 9115
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "windows_dv_template_from_registry" tests/storage/snapshots/test_snapshots.pyRepository: RedHatQE/openshift-virtualization-tests
Length of output: 163
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "parametrize\([^)]*windows_dv_template_from_registry|windows_dv_template_from_registry" tests/storage/snapshotsRepository: RedHatQE/openshift-virtualization-tests
Length of output: 521
🏁 Script executed:
#!/bin/bash set -euo pipefail cat -n tests/storage/snapshots/test_snapshots.py | sed -n '380,520p'Repository: RedHatQE/openshift-virtualization-tests
Length of output: 2873
Fix indirect parametrization:
windows_dv_template_from_registryreadsrequest.parambut is not parametrized intest_write_to_file_while_snapshot
windows_dv_template_from_registryunconditionally doesdv_name=request.param["dv_name"]. That works fortest_online_windows_vm_successful_restore(both fixtures are indirect-parametrized), buttest_write_to_file_while_snapshotonly indirect-parametrizeswindows_vm_with_vtpm_for_snapshot, sowindows_dv_template_from_registryis invoked withoutrequest.paramand fixture setup will fail (typicallyAttributeError/ missingparam).Make
test_write_to_file_while_snapshotparametrizewindows_dv_template_from_registrytoo (e.g., add it to the@pytest.mark.parametrize(..., indirect=True)list with the dv_name dict), or refactor so both fixtures share a dedicated parametrizeddv_namedependency instead of each reading its ownrequest.param.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/storage/snapshots/conftest.py` around lines 53 - 60, The fixture windows_dv_template_from_registry unconditionally reads request.param["dv_name"] but test_write_to_file_while_snapshot does not indirect-parametrize it, causing a missing request.param; fix by making the test parametrize windows_dv_template_from_registry as well (add it to the `@pytest.mark.parametrize`(..., indirect=True) list with the dv_name dict) or refactor to extract a separate parametrized dv_name fixture consumed by both windows_dv_template_from_registry and windows_vm_with_vtpm_for_snapshot so neither fixture reads request.param directly; update references to create_windows2022_dv_template_from_registry accordingly.tests/storage/utils.py (1)
300-314:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Check the current DV conditions before opening the watch.
This helper only looks at future watch events. If CDI has already set the target message by the time this function is called, the watch can stay quiet until timeout and raise
DataVolumeConditionMessageNotFoundErroreven though the condition is already present. That turns fast failures into false negatives.Suggested fix
def wait_for_dv_condition_message(dv: DataVolume, expected_message: str, timeout: int = TIMEOUT_5MIN) -> None: @@ LOGGER.info(f"Watching {dv.name} for message: {expected_message} for up to {timeout} seconds.") - last_conditions: list[dict[str, str]] = [] + last_conditions = (dv.instance.status or {}).get("conditions", []) + if any(expected_message in condition.get("message", "") for condition in last_conditions): + LOGGER.info(f"Found expected message in {dv.name}: {expected_message}") + return + deleted = False for event in dv.watcher(timeout=timeout):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.LOGGER.info(f"Watching {dv.name} for message: {expected_message} for up to {timeout} seconds.") last_conditions = (dv.instance.status or {}).get("conditions", []) if any(expected_message in condition.get("message", "") for condition in last_conditions): LOGGER.info(f"Found expected message in {dv.name}: {expected_message}") return deleted = False for event in dv.watcher(timeout=timeout): event_type = event["type"] if event_type == "DELETED": deleted = True break if event_type not in ("ADDED", "MODIFIED"): LOGGER.info(f"Ignoring {event_type} event for DataVolume {dv.name}") continue last_conditions = (event["object"].status or {}).get("conditions", []) if any(expected_message in condition.get("message", "") for condition in last_conditions): LOGGER.info(f"Found expected message in {dv.name}: {expected_message}") return🧰 Tools
🪛 Ruff (0.15.15)
[warning] 300-300: Logging statement uses f-string
(G004)
[warning] 309-309: Logging statement uses f-string
(G004)
[warning] 313-313: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/storage/utils.py` around lines 300 - 314, Before starting dv.watcher, check the current DataVolume conditions on dv (e.g., (dv.status or {}).get("conditions", [])) using the same message-match logic as inside the loop and return early if the expected_message is already present; keep the existing variables (last_conditions, deleted) and behavior otherwise so the subsequent watcher loop only handles future events and avoids false negatives and DataVolumeConditionMessageNotFoundError.tests/utils.py (1)
760-760:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix docstring parameter name.
The docstring references
dv_dictbut the actual parameter is nameddv_template(line 750). Update the docstring to match the parameter name.📝 Proposed fix
Args: - dv_dict: DataVolume template dictionary with metadata and spec + dv_template: DataVolume template dictionary with metadata and spec namespace: Kubernetes namespace📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.dv_template: DataVolume template dictionary with metadata and spec🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils.py` at line 760, Update the docstring to reference the actual parameter name dv_template (not dv_dict) wherever the parameter is described; locate the function or helper in tests/utils.py that declares dv_template (the parameter named dv_template on or around line 750) and replace occurrences of dv_dict in the docstring with dv_template so the docstring and parameter list match.utilities/constants.py (1)
962-964: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
LOW: Remove the second
WINDOWS_2K22_PREFERENCEdefinition.
WINDOWS_2K22_PREFERENCEis already defined at Line 822, so this assignment silently shadows the first one. It works today only because both values match, but it creates an easy drift point for the VM preference contract later. Keep a single source of truth and reuse it here instead of redefining it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utilities/constants.py` around lines 962 - 964, Remove the duplicate WINDOWS_2K22_PREFERENCE assignment and rely on the existing WINDOWS_2K22_PREFERENCE definition already present in the file; specifically, keep WINDOWS_11_PREFERENCE as defined here but delete the second WINDOWS_2K22_PREFERENCE line so there is a single source of truth for WINDOWS_2K22_PREFERENCE (do not redefine or shadow it).
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4916. Overlapping filestests/utils.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5099. Overlapping filestests/storage/utils.py |
9 participants
Short description:
Refactor
snapshotsmodule to use Win 2k22 with vTPM.More details:
Using registry image of Windows 2022 with preference and instance types to create a VM with vTPM for the tests.
What this PR does / why we need it:
Many customers are using Windows 2022 with vTPM and we need to reflect it in our testing.
Which issue(s) this PR fixes:
https://redhat.atlassian.net/browse/CNV-51351
Special notes for reviewer:
Co-Authored: Claude Code
jira-ticket:
Summary by CodeRabbit