Storage: Add windows OADP test#4603
Merged
rnetser merged 5 commits intoRedHatQE:mainfrom May 6, 2026
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR consolidates Windows VM file handling utilities by migrating Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
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 #4603 +/- ##
==========================================
+ Coverage 98.63% 98.67% +0.03%
==========================================
Files 25 25
Lines 2420 2483 +63
==========================================
+ Hits 2387 2450 +63
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:
|
openshift-virtualization-qe-bot-5
requested review from
Ahmad-Hafe,
RoniKishner,
acinko-rh,
dshchedr,
ema-aka-young,
geetikakay,
josemacassan,
jpeimer and
kshvaika
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/data_protection/oadp/conftest.py`:
- Around line 178-195: The fixture currently uses latest_windows =
py_config.get("latest_windows_os_dict") or {} which masks missing config and
passes None into create_windows_vm_from_dv_template and wait_for_windows_vm;
change to require and validate the config: fetch latest_windows via
py_config["latest_windows_os_dict"] (or raise a clear exception if missing) and
assert that IMAGE_PATH_STR, DV_SIZE_STR, and OS_VERSION_STR keys are present and
non-empty in latest_windows, raising a ValueError/RuntimeError with a
descriptive message if any are missing before calling
create_windows_vm_from_dv_template and get_test_artifact_server_url so the test
fails fast on bad/missing metadata.
In `@tests/data_protection/oadp/test_velero.py`:
- Around line 101-130: The new Windows OADP test
(test_backup_and_restore_windows_vm) lacks the required STD/STP traceability:
add or update the module-level docstring to include an STP link (or if no STP
exists, an RFE/Jira-epic link) and create an STD placeholder test/marker before
the implemented test (e.g., a minimal test function named
test_std_windows_backup_restore or a module-level note) that contains the STD
docstring describing scope, preconditions and test steps for review; ensure the
STD placeholder is reviewed/approved before keeping the implemented test and
include the STP/RFE/Jira identifier in the implemented test docstring or as a
module-level comment to satisfy traceability rules.
In `@utilities/virt.py`:
- Around line 1740-1742: The current call builds a single shell string for
Get-Content which breaks on spaces/wildcards; change the command tokens so
PowerShell receives Get-Content with the -LiteralPath parameter and the path as
a separate token (use the existing variables cmd, file_name_with_path and
run_ssh_commands), e.g. construct cmd so it invokes "powershell -command
Get-Content -LiteralPath <path>" with the path passed as its own token rather
than interpolated into a quoted string; keep the call to run_ssh_commands and
the assertion unchanged.
🪄 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: b453c7f3-eafb-4ce7-9edf-155ca92ba735
📒 Files selected for processing (6)
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero.pytests/data_protection/oadp/utils.pytests/storage/storage_migration/test_storage_class_migration.pytests/storage/storage_migration/utils.pyutilities/virt.py
💤 Files with no reviewable changes (1)
- tests/storage/storage_migration/utils.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/data_protection/oadp/utils.py (1)
22-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Add Google-format docstrings for the two public side-effect helpers.
Line 22 (
wait_for_restored_dv) and Line 27 (write_file_windows_vm_for_oadp) are public helpers with side effects but don’t provide required Google-format docstrings (Args, and side-effect/behavior context).Proposed fix
def wait_for_restored_dv(dv: DataVolume) -> None: + """Wait until a restored DataVolume is usable. + + Args: + dv: Restored DataVolume to validate. + + Side effects: + Blocks until the bound PVC status and DV success state are reached. + """ dv.pvc.wait_for_status(status=PersistentVolumeClaim.Status.BOUND, timeout=TIMEOUT_15SEC) dv.wait_for_dv_success(timeout=TIMEOUT_10SEC) @@ def write_file_windows_vm_for_oadp(vm: VirtualMachineForTests) -> None: - """Write test data to marker file on Windows VM for OADP backup verification.""" + """Write OADP marker content to a Windows VM before backup. + + Args: + vm: Windows VM that should contain the backup marker file. + + Side effects: + Writes `TEXT_TO_TEST` into `FILE_PATH_FOR_WINDOWS_BACKUP` on the guest. + """ write_file_windows_vm(vm=vm, file_path=FILE_PATH_FOR_WINDOWS_BACKUP, content=TEXT_TO_TEST)As per coding guidelines, "Google-format docstrings are REQUIRED for all public functions with non-obvious return values OR side effects".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/data_protection/oadp/utils.py` around lines 22 - 29, Add Google-style docstrings to the public helper functions wait_for_restored_dv and write_file_windows_vm_for_oadp: for wait_for_restored_dv document the function purpose, Args (dv: DataVolume), behavior/side effects (waits for PVC to be BOUND and for DV restore success), and any exceptions/timeouts; for write_file_windows_vm_for_oadp document purpose, Args (vm: VirtualMachineForTests), behavior/side effects (writes TEXT_TO_TEST to FILE_PATH_FOR_WINDOWS_BACKUP on the VM), and note any return value (None) and potential errors/timeouts so the docstrings follow the required Google format.
🤖 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/data_protection/oadp/conftest.py`:
- Around line 55-62: After applying the unsupportedOverrides patch to the
DataProtectionApplication (dpa) with ResourceEditor, block until the Velero pods
have rolled out the new image and likewise block again after the patch is
reverted; implement this by using timeout_sampler to poll the Velero
deployment/pods in the same namespace for the container image to equal
OADP_VELERO_IMAGE_FQIN_OVERRIDE (after entering the ResourceEditor context) and
to equal the original image (after exiting/reverting), rather than sleeping;
reference the DataProtectionApplication/dpa and ResourceEditor in your changes
and use timeout_sampler from the timeout_sampler package for both pre-yield and
post-teardown wait loops.
In `@utilities/storage.py`:
- Around line 658-659: The write_file_windows_vm implementation is building cmd
with shlex.split and interpolating file_path unquoted, which breaks on paths
with spaces; update the function to build cmd as a list (like
verify_file_in_windows_vm does) using PowerShell's -LiteralPath with the
file_path wrapped in single quotes and pass the content safely (e.g., as a
quoted string argument or separate list element) so run_ssh_commands receives a
proper argument array (modify the cmd variable construction and the
run_ssh_commands call in write_file_windows_vm to mirror the pattern used in
verify_file_in_windows_vm).
---
Duplicate comments:
In `@tests/data_protection/oadp/utils.py`:
- Around line 22-29: Add Google-style docstrings to the public helper functions
wait_for_restored_dv and write_file_windows_vm_for_oadp: for
wait_for_restored_dv document the function purpose, Args (dv: DataVolume),
behavior/side effects (waits for PVC to be BOUND and for DV restore success),
and any exceptions/timeouts; for write_file_windows_vm_for_oadp document
purpose, Args (vm: VirtualMachineForTests), behavior/side effects (writes
TEXT_TO_TEST to FILE_PATH_FOR_WINDOWS_BACKUP on the VM), and note any return
value (None) and potential errors/timeouts so the docstrings follow the required
Google format.
🪄 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: cdc5233e-56ff-47e7-9d6b-7a02e8ea5149
📒 Files selected for processing (7)
tests/data_protection/oadp/conftest.pytests/data_protection/oadp/test_velero.pytests/data_protection/oadp/utils.pytests/storage/storage_migration/conftest.pytests/storage/storage_migration/test_storage_class_migration.pytests/storage/storage_migration/utils.pyutilities/storage.py
💤 Files with no reviewable changes (1)
- tests/storage/storage_migration/utils.py
jpeimer
requested changes
May 5, 2026
jpeimer
approved these changes
May 5, 2026
Contributor
|
/approve |
Contributor
Author
|
/verified |
Contributor
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:
Your deliverable: Test Execution Plan
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: Before setting "Run smoke tests: True", you MUST use shell scripts to: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6883 🏁 Script executed: Length of output: 1807 |
` Signed-off-by: Dalia Frank <dafrank@redhat.com>
Switch from Template-based VM creation to modern InstanceType/Preference: - Use VirtualMachineClusterInstancetype (u1.large) - Use VirtualMachineClusterPreference (windows.2k22) for vTPM support - Change DataVolume source from HTTP to registry container disk - Remove unused modern_cpu_for_migration parameter Consolidate Windows file operations into shared utilities: - Add write_file_windows_vm() to utilities/storage.py with retry - Move verify_file_in_windows_vm() from virt.py to storage.py with retry - Update all imports in test files Document DPA Velero workaround: - Add comment referencing velero-io/velero#9601 - Workaround still needed until fix merges into OADP 1.6 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dalia Frank <dafrank@redhat.com>
Reduce DataVolume wait timeout from 60 min to 30 min (default): - Registry container disks are faster than HTTP source - Other OADP tests (RHEL) use default 30 min timeout - Test passed with reduced timeout Remove unused TIMEOUT_60MIN import. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Dalia Frank <dafrank@redhat.com>
Signed-off-by: Dalia Frank <dafrank@redhat.com>
- Use shlex.split() in verify_file_in_windows_vm for consistency - Replace hardcoded "u1.large" with U1_LARGE constant - Remove unnecessary disk_type=None parameter - Remove write_file_windows_vm_for_oadp wrapper, use write_file_windows_vm directly - Fix line length issue (E501) Signed-off-by: Dalia Frank <dafrank@redhat.com>
|
Clean rebase detected — no code changes compared to previous head ( |
dshchedr
approved these changes
May 6, 2026
rnetser
approved these changes
May 6, 2026
Collaborator
|
/approve |
This was referenced May 6, 2026
Contributor
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
acinko-rh
reviewed
May 6, 2026
Comment on lines
+162
to
+202
| """Windows 2022 VM with InstanceType and Preference in the backup namespace for OADP backup testing.""" | ||
| artifactory_secret = None | ||
| artifactory_config_map = None | ||
|
|
||
| try: | ||
| artifactory_secret = get_artifactory_secret(namespace=namespace_for_backup.name) | ||
| artifactory_config_map = get_artifactory_config_map(namespace=namespace_for_backup.name) | ||
|
|
||
| dv = DataVolume( | ||
| name="oadp-windows-dv", | ||
| namespace=namespace_for_backup.name, | ||
| storage_class=snapshot_storage_class_name_scope_module, | ||
| source="registry", | ||
| url=( | ||
| f"{get_test_artifact_server_url(schema='registry')}/" | ||
| f"{py_config['latest_windows_os_dict'][CONTAINER_DISK_IMAGE_PATH_STR]}" | ||
| ), | ||
| size=Images.Windows.CONTAINER_DISK_DV_SIZE, | ||
| client=admin_client, | ||
| api_name="storage", | ||
| secret=artifactory_secret, | ||
| cert_configmap=artifactory_config_map.name, | ||
| ) | ||
| dv.to_dict() | ||
|
|
||
| with VirtualMachineForTests( | ||
| name="oadp-windows-vm", | ||
| namespace=namespace_for_backup.name, | ||
| client=admin_client, | ||
| vm_instance_type=VirtualMachineClusterInstancetype(client=admin_client, name=U1_LARGE), | ||
| vm_preference=VirtualMachineClusterPreference(client=admin_client, name="windows.2k22"), | ||
| data_volume_template=dv.res, | ||
| os_flavor=OS_FLAVOR_WIN_CONTAINER_DISK, | ||
| ) as vm: | ||
| running_vm(vm=vm) | ||
| write_file_windows_vm(vm=vm, file_path=FILE_PATH_FOR_WINDOWS_BACKUP, content=TEXT_TO_TEST) | ||
| yield vm | ||
| finally: | ||
| cleanup_artifactory_secret_and_config_map( | ||
| artifactory_secret=artifactory_secret, artifactory_config_map=artifactory_config_map | ||
| ) |
Contributor
There was a problem hiding this comment.
Which errors are you catching? The secret and config map get or the DV and/or VM creation?
The try blocks should usually be very specific.
So something like:
try:
artifactory_secret = get_artifactory_secret(namespace=namespace_for_backup.name)
artifactory_config_map = get_artifactory_config_map(namespace=namespace_for_backup.name)
else:
# DV and VM creation here.
finally:
# Artifactory cleanup hereLet me know if I misinterpreted something.
|
Where are the logs this testing? Perhaps this is something that can only run internal? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short description:
Add automated backup and restore tests for Windows.
More details:
Now that the foundational Windows bug (CNV-84547) has been resolved, we are enabling the test suite for Windows backup and restore. These tests will provide the initial baseline coverage and ensure this functionality remains stable moving forward.
What this PR does / why we need it:
Feature Validation: Implements tests for Windows backup and restore functionality, which was previously blocked.
Continuous Coverage: Ensures that Windows support is tested on a regular basis now that the environment is ready.
Which issue(s) this PR fixes:
Fixes: CNV-84548
Special notes for reviewer:
jira-ticket:
https://redhat.atlassian.net/browse/CNV-84548
Assisted-by: cursor
Summary by CodeRabbit
Tests
Refactor