Storage: updated the snapshot tests to use DataSources instead of DVs #3011
+101
−112
Conversation
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
WalkthroughReplaces Cirros-based test fixtures and helpers with RHEL-based equivalents: renames fixtures and parameters (cirros_* → rhel_* and snapshots_with_content → snapshot_with_content), adds RHEL instance-type/preference imports and constants, and renames the storage helper run_command_on_cirros_vm_and_check_output → run_command_on_vm_and_check_output across tests. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Spot-check items:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (28)📓 Common learnings📚 Learning: 2025-06-15T13:57:10.548ZApplied to files:
📚 Learning: 2025-05-18T09:24:43.335ZApplied to files:
📚 Learning: 2025-10-30T10:43:48.886ZApplied to files:
📚 Learning: 2025-06-18T09:21:34.315ZApplied to files:
📚 Learning: 2025-06-23T19:24:28.327ZApplied to files:
📚 Learning: 2025-08-06T13:57:51.928ZApplied to files:
📚 Learning: 2025-06-23T19:18:12.275ZApplied to files:
📚 Learning: 2025-09-15T06:49:53.478ZApplied to files:
📚 Learning: 2025-08-14T10:28:22.958ZApplied to files:
📚 Learning: 2025-06-05T11:36:25.219ZApplied to files:
📚 Learning: 2025-05-26T13:36:16.136ZApplied to files:
📚 Learning: 2025-09-03T07:22:50.446ZApplied to files:
📚 Learning: 2025-06-17T14:45:39.291ZApplied to files:
📚 Learning: 2025-06-18T09:31:06.311ZApplied to files:
📚 Learning: 2025-05-28T10:50:56.122ZApplied to files:
📚 Learning: 2025-09-03T07:23:37.045ZApplied to files:
📚 Learning: 2025-05-27T11:44:14.859ZApplied to files:
📚 Learning: 2025-10-16T12:47:04.521ZApplied to files:
📚 Learning: 2025-09-29T20:33:51.007ZApplied to files:
📚 Learning: 2025-08-09T01:46:48.039ZApplied to files:
📚 Learning: 2025-09-10T13:25:59.799ZApplied to files:
📚 Learning: 2025-06-17T07:45:37.776ZApplied to files:
📚 Learning: 2025-09-28T14:43:07.181ZApplied to files:
📚 Learning: 2025-06-18T09:19:05.769ZApplied to files:
📚 Learning: 2025-05-21T09:41:46.532ZApplied to files:
📚 Learning: 2025-09-17T14:02:24.619ZApplied to files:
📚 Learning: 2025-08-20T23:57:48.380ZApplied to files:
🧬 Code graph analysis (3)tests/storage/upgrade/test_upgrade_storage.py (1)
tests/storage/snapshots/test_snapshots.py (3)
tests/storage/conftest.py (1)
🪛 Ruff (0.14.7)tests/storage/snapshots/test_snapshots.py375-375: Unused function argument: (ARG001) tests/storage/conftest.py487-487: Unused function argument: (ARG001) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (21)
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 |
Contributor
|
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
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-3
requested review from
Ahmad-Hafe,
RoniKishner,
dshchedr,
duyanyan,
geetikakay,
hmeir,
josemacassan,
kgoldbla,
rlobillo,
rnetser,
stesrn and
vsibirsk
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3011 +/- ##
=======================================
Coverage 98.28% 98.28%
=======================================
Files 25 25
Lines 2157 2157
=======================================
Hits 2120 2120
Misses 37 37
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
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
approved these changes
Dec 5, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/storage/conftest.py (1)
485-528: Consider removing unusednamespaceparameter.The
namespaceparameter on line 487 is not used in the fixture body. The fixture accesses the namespace throughrhel_vm_for_snapshot.namespaceinstead.Apply this diff to remove the unused parameter:
def snapshot_with_content( request, - namespace, admin_client, rhel_vm_for_snapshot, ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/storage/conftest.py(6 hunks)tests/storage/snapshots/test_snapshots.py(13 hunks)tests/storage/upgrade/test_upgrade_storage.py(4 hunks)utilities/storage.py(1 hunks)
🧰 Additional context used
🧠 Learnings (28)
📓 Common learnings
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 2014
File: tests/storage/cdi_clone/test_clone.py:0-0
Timestamp: 2025-09-10T13:25:59.799Z
Learning: In the openshift-virtualization-tests repository, when creating DataVolume objects using ocp_resources, use `api_name="storage"` rather than the default `api_name="pvc"`.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 2469
File: utilities/sanity.py:139-142
Timestamp: 2025-11-08T07:36:57.616Z
Learning: In the openshift-virtualization-tests repository, user rnetser prefers to keep refactoring PRs (like PR #2469) strictly focused on moving/organizing code into more granular modules without adding new functionality, error handling, or behavioral changes. Such improvements should be handled in separate PRs.
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 2045
File: tests/virt/cluster/vm_lifecycle/conftest.py:46-47
Timestamp: 2025-09-15T06:49:53.478Z
Learning: In the openshift-virtualization-tests repo, large fixture refactoring efforts like the golden image data source migration are handled incrementally by directory/team ownership. The virt/cluster directory is handled separately from virt/node, tests/infra, tests/storage, etc., with each area managed by relevant teams in follow-up PRs.
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 2014
File: tests/storage/cdi_clone/test_clone.py:215-218
Timestamp: 2025-09-08T12:38:53.119Z
Learning: In OpenShift Virtualization tests, DataSource.source points to a PVC or VolumeSnapshot object, which means data_source.source.instance.to_dict()["spec"] is valid since both PVC and VolumeSnapshot objects contain spec sections.
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 1729
File: tests/virt/cluster/common_templates/utils.py:655-661
Timestamp: 2025-08-11T05:58:04.127Z
Learning: In the OpenShift Virtualization test suite (tests/virt/cluster/common_templates/utils.py), DataSource objects are pre-deployed on the cluster and validated after deployment to ensure they have source in their spec. Therefore, `data_source.source` is guaranteed to exist when checking `data_source.source.exists` in the `get_matrix_os_golden_image_data_source` function, making null-checks unnecessary.
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1716
File: tests/virt/conftest.py:289-297
Timestamp: 2025-08-09T01:52:26.683Z
Learning: When user dshchedr moves working code from one location to another in the openshift-virtualization-tests repository, they prefer not to modify it unless there's a real issue, maintaining the original implementation to avoid introducing unnecessary changes.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1723
File: conftest.py:496-502
Timestamp: 2025-08-27T12:38:26.496Z
Learning: In PR #1723, the user rnetser clarified that updating downstream string comparisons to handle DataVolume enums from StorageClassConfig is not part of the current PR scope, which focuses only on adding conformance storage class support.
Learnt from: vamsikrishna-siddu
Repo: RedHatQE/openshift-virtualization-tests PR: 2199
File: tests/storage/test_online_resize.py:108-113
Timestamp: 2025-09-28T14:43:07.181Z
Learning: In the openshift-virtualization-tests repo, PR #2199 depends on PR #2139 which adds the OS_FLAVOR attribute to the Images.Cirros class, making Images.Cirros.OS_FLAVOR available for conditional logic in tests.
Learnt from: rlobillo
Repo: RedHatQE/openshift-virtualization-tests PR: 2249
File: tests/install_upgrade_operators/must_gather/test_must_gather.py:428-441
Timestamp: 2025-10-16T12:47:04.521Z
Learning: In openshift-virtualization-tests repository, DataVolumes in the openshift-virtualization-os-images namespace are volatile resources managed by DataImportCron. They can be created/destroyed between must-gather collection listing and file retrieval, requiring FileNotFoundError exception handling in test_crd_resources to skip these volatile resources gracefully while still validating DataVolumes in other namespaces. There is no pytest_generate_tests hook that filters out datavolumes from test parametrization.
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 1160
File: tests/storage/storage_migration/constants.py:4-5
Timestamp: 2025-06-08T08:05:46.691Z
Learning: In the openshift-virtualization-tests repository's storage migration tests, `/dev/sda` is the correct device path for hotplugged volumes in the VM testing environment, even though it might typically be associated with the root disk in other contexts.
Learnt from: josemacassan
Repo: RedHatQE/openshift-virtualization-tests PR: 2300
File: tests/storage/online_resize/conftest.py:94-96
Timestamp: 2025-10-30T10:43:48.886Z
Learning: In tests/storage/online_resize/conftest.py, the `running_rhel_vm` fixture is intentionally designed to only invoke `running_vm(vm=rhel_vm_for_online_resize)` without returning or yielding a value. It's used as a dependency fixture to ensure the VM is running before other fixtures/tests execute, while the actual VM object is accessed via the `rhel_vm_for_online_resize` fixture.
Learnt from: vamsikrishna-siddu
Repo: RedHatQE/openshift-virtualization-tests PR: 2199
File: tests/storage/test_online_resize.py:108-113
Timestamp: 2025-09-28T14:43:07.181Z
Learning: In the openshift-virtualization-tests repo, PR #2199 depends on PR #2139 which adds architecture-specific OS_FLAVOR attributes to the Images.Cirros class (OS_FLAVOR_CIRROS for x86_64/ARM64, OS_FLAVOR_FEDORA for s390x), enabling conditional logic based on the underlying OS flavor in tests.
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1932
File: tests/virt/node/migration_and_maintenance/conftest.py:57-64
Timestamp: 2025-09-08T21:34:28.924Z
Learning: In OpenShift Virtualization tests, MigrationPolicy fixtures should use static names rather than unique suffixes to enable collision detection. If parallel test runs collide on cluster-scoped resource names like MigrationPolicy, it's better to know about the collision rather than hide it with unique naming, as confirmed by maintainer dshchedr.
Learnt from: servolkov
Repo: RedHatQE/openshift-virtualization-tests PR: 2838
File: .github/workflows/net-utils-builder-staging.yml:37-37
Timestamp: 2025-11-25T01:56:54.902Z
Learning: In the openshift-virtualization-tests repository, when renaming container images that are built and used by GitHub Actions workflows, the changes must be done sequentially: first merge the workflow files (.github/workflows/) that update the image name in the CI/CD pipelines, then update the code references (like constants.py and manifest files) in a follow-up PR. This prevents the old workflow from running with mismatched image names during the transition.
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1702
File: tests/chaos/oadp/conftest.py:23-45
Timestamp: 2025-09-17T14:02:24.619Z
Learning: In the openshift-virtualization-tests repository, VirtualMachineForTests class automatically generates unique VM names when generate_unique_name=True (default). It uses utilities.infra.unique_name() which appends a timestamp to create names like "vm-oadp-chaos-{timestamp}", so hardcoded VM names in fixtures don't cause collisions during parallel test execution.
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 1160
File: tests/storage/storage_migration/test_mtc_storage_class_migration.py:165-176
Timestamp: 2025-06-17T07:45:37.776Z
Learning: In the openshift-virtualization-tests repository, user jpeimer prefers explicit fixture parameters over composite fixtures in test methods, even when there are many parameters, as they find this approach more readable and maintainable for understanding test dependencies.
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/conftest.py:1065-1077
Timestamp: 2025-06-18T09:21:34.315Z
Learning: In tests/observability/metrics/conftest.py, when creating fixtures that modify shared Windows VM state (like changing nodeSelector), prefer using function scope rather than class scope to ensure ResourceEditor context managers properly restore the VM state after each test, maintaining test isolation while still reusing expensive Windows VM fixtures.
📚 Learning: 2025-06-15T13:57:10.548Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 1160
File: tests/storage/storage_migration/conftest.py:358-366
Timestamp: 2025-06-15T13:57:10.548Z
Learning: In the openshift-virtualization-tests codebase, prefer using `shlex.split()` with `run_ssh_commands` function, even for commands containing shell operators like pipes. The function handles split commands correctly and this is the established pattern throughout the codebase.
Applied to files:
tests/storage/upgrade/test_upgrade_storage.py
📚 Learning: 2025-10-30T10:43:48.886Z
Learnt from: josemacassan
Repo: RedHatQE/openshift-virtualization-tests PR: 2300
File: tests/storage/online_resize/conftest.py:94-96
Timestamp: 2025-10-30T10:43:48.886Z
Learning: In tests/storage/online_resize/conftest.py, the `running_rhel_vm` fixture is intentionally designed to only invoke `running_vm(vm=rhel_vm_for_online_resize)` without returning or yielding a value. It's used as a dependency fixture to ensure the VM is running before other fixtures/tests execute, while the actual VM object is accessed via the `rhel_vm_for_online_resize` fixture.
Applied to files:
tests/storage/upgrade/test_upgrade_storage.pytests/storage/snapshots/test_snapshots.pytests/storage/conftest.py
📚 Learning: 2025-05-18T09:24:43.335Z
Learnt from: EdDev
Repo: RedHatQE/openshift-virtualization-tests PR: 990
File: utilities/virt.py:1704-1710
Timestamp: 2025-05-18T09:24:43.335Z
Learning: The `migrate_vm_and_verify` function in utilities/virt.py has inconsistent return behavior - it returns a VirtualMachineInstanceMigration object when wait_for_migration_success=False and returns None when wait_for_migration_success=True. This has been properly typed but should be refactored in a future PR for better API design.
Applied to files:
tests/storage/upgrade/test_upgrade_storage.py
📚 Learning: 2025-08-06T13:57:51.928Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1249-1254
Timestamp: 2025-08-06T13:57:51.928Z
Learning: User rnetser verified that all calls to get_infrastructure() function use the admin_client parameter, confirming that signature changes requiring this parameter don't cause breaking changes in the openshift-virtualization-tests codebase.
Applied to files:
tests/storage/snapshots/test_snapshots.py
📚 Learning: 2025-06-18T09:21:34.315Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/conftest.py:1065-1077
Timestamp: 2025-06-18T09:21:34.315Z
Learning: In tests/observability/metrics/conftest.py, when creating fixtures that modify shared Windows VM state (like changing nodeSelector), prefer using function scope rather than class scope to ensure ResourceEditor context managers properly restore the VM state after each test, maintaining test isolation while still reusing expensive Windows VM fixtures.
Applied to files:
tests/storage/snapshots/test_snapshots.pytests/storage/conftest.py
📚 Learning: 2025-06-23T19:24:28.327Z
Learnt from: akri3i
Repo: RedHatQE/openshift-virtualization-tests PR: 1210
File: tests/virt/cluster/general/mass_machine_type_transition_tests/test_mass_machine_type_transition.py:97-104
Timestamp: 2025-06-23T19:24:28.327Z
Learning: In OpenShift Virtualization machine type transition tests, the test_machine_type_transition_without_restart method with restart_required=false parameter validates that VM machine types do NOT change when the lifecycle job runs with restart disabled, so the assertion should check against the original machine type rather than the target machine type.
Applied to files:
tests/storage/snapshots/test_snapshots.py
📚 Learning: 2025-06-23T19:18:12.275Z
Learnt from: akri3i
Repo: RedHatQE/openshift-virtualization-tests PR: 1210
File: tests/virt/cluster/general/mass_machine_type_transition_tests/conftest.py:142-149
Timestamp: 2025-06-23T19:18:12.275Z
Learning: In OpenShift Virtualization machine type transition tests, the kubevirt_api_lifecycle_automation_job updates VM machine types to the latest version based on a MACHINE_TYPE_GLOB pattern, and subsequent fixtures may intentionally revert the machine type to test bidirectional transition behavior.
Applied to files:
tests/storage/snapshots/test_snapshots.py
📚 Learning: 2025-08-14T10:28:22.958Z
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 1785
File: tests/virt/cluster/common_templates/utils.py:65-66
Timestamp: 2025-08-14T10:28:22.958Z
Learning: The restart_qemu_guest_agent_service function in tests/virt/cluster/common_templates/utils.py is only called for RHEL7 VMs, with OS version checks handled by the calling test code, not within the function itself. Guest agent functionality is verified by subsequent tests in the test class.
Applied to files:
tests/storage/snapshots/test_snapshots.pytests/storage/conftest.py
📚 Learning: 2025-06-05T11:36:25.219Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 906
File: tests/observability/metrics/conftest.py:19-19
Timestamp: 2025-06-05T11:36:25.219Z
Learning: In the openshift-virtualization-tests repository, the pyhelper_utils package (specifically pyhelper_utils.shell with run_ssh_commands) is installed and available in the test execution environment where the tests are run.
Applied to files:
tests/storage/snapshots/test_snapshots.py
📚 Learning: 2025-05-26T13:36:16.136Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 584
File: tests/observability/metrics/conftest.py:1136-1136
Timestamp: 2025-05-26T13:36:16.136Z
Learning: For Windows VM testing in metrics tests, the `verify_wsl2_guest_works(vm=vm)` call in the `windows_vm_for_test` fixture uses a 60-second timeout as a deliberate "fail fast" validation. This upfront check prevents more expensive failures later since Windows VM deployment is time-consuming, and WSL2 functionality is critical for the Windows VM tests to work properly.
Applied to files:
tests/storage/snapshots/test_snapshots.py
📚 Learning: 2025-09-03T07:22:50.446Z
Learnt from: RoniKishner
Repo: RedHatQE/openshift-virtualization-tests PR: 1946
File: tests/infrastructure/instance_types/supported_os/test_windows_os.py:27-28
Timestamp: 2025-09-03T07:22:50.446Z
Learning: In tests/infrastructure/instance_types/supported_os/test_windows_os.py, Windows guest images are configured with SSH support (likely through WSL2), so running_vm() should keep the default check_ssh_connectivity=True to verify SSH connectivity is working properly in the Windows guest images.
Applied to files:
tests/storage/snapshots/test_snapshots.py
📚 Learning: 2025-06-17T14:45:39.291Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 1224
File: tests/storage/storage_migration/conftest.py:419-425
Timestamp: 2025-06-17T14:45:39.291Z
Learning: In tests/storage/storage_migration/conftest.py, the PowerShell command using shlex.split() with properly escaped quotes works correctly and does not break the command structure when passed to run_ssh_commands.
Applied to files:
tests/storage/snapshots/test_snapshots.py
📚 Learning: 2025-06-18T09:31:06.311Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/conftest.py:1065-1077
Timestamp: 2025-06-18T09:31:06.311Z
Learning: In tests/observability/metrics/conftest.py, ResourceEditor context managers automatically restore VM configuration when the context exits, including nodeSelector patches. The fixture pattern with `with ResourceEditor(patches={vm: {...}})` followed by `yield` properly restores the VM to its original state without requiring manual teardown logic.
Applied to files:
tests/storage/snapshots/test_snapshots.py
📚 Learning: 2025-05-28T10:50:56.122Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 954
File: tests/storage/storage_migration/conftest.py:264-269
Timestamp: 2025-05-28T10:50:56.122Z
Learning: In the openshift-virtualization-tests codebase, cleanup pytest fixtures like `deleted_old_dvs_of_stopped_vms`, `deleted_completed_virt_launcher_source_pod`, and `deleted_old_dvs_of_online_vms` do not require yield statements. These fixtures perform cleanup operations and work correctly without yielding values.
Applied to files:
tests/storage/snapshots/test_snapshots.py
📚 Learning: 2025-09-03T07:23:37.045Z
Learnt from: RoniKishner
Repo: RedHatQE/openshift-virtualization-tests PR: 1946
File: tests/infrastructure/instance_types/supported_os/test_windows_os.py:30-31
Timestamp: 2025-09-03T07:23:37.045Z
Learning: In tests/infrastructure/instance_types/supported_os/test_windows_os.py, test_vm_deletion should depend on test_create_vm (not test_start_vm) to ensure resource cleanup happens even if the VM fails to start, preventing resource leaks in the test environment.
Applied to files:
tests/storage/snapshots/test_snapshots.py
📚 Learning: 2025-05-27T11:44:14.859Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 584
File: tests/observability/metrics/test_network_metrics.py:62-76
Timestamp: 2025-05-27T11:44:14.859Z
Learning: The windows_vm_for_test fixture in tests/observability/metrics/conftest.py does not have a request argument, so it cannot be parametrized using pytest.mark.parametrize with indirect=True. This is different from vm_for_test fixture which accepts parameters through parametrization.
Applied to files:
tests/storage/snapshots/test_snapshots.pytests/storage/conftest.py
📚 Learning: 2025-10-16T12:47:04.521Z
Learnt from: rlobillo
Repo: RedHatQE/openshift-virtualization-tests PR: 2249
File: tests/install_upgrade_operators/must_gather/test_must_gather.py:428-441
Timestamp: 2025-10-16T12:47:04.521Z
Learning: In openshift-virtualization-tests repository, DataVolumes in the openshift-virtualization-os-images namespace are volatile resources managed by DataImportCron. They can be created/destroyed between must-gather collection listing and file retrieval, requiring FileNotFoundError exception handling in test_crd_resources to skip these volatile resources gracefully while still validating DataVolumes in other namespaces. There is no pytest_generate_tests hook that filters out datavolumes from test parametrization.
Applied to files:
tests/storage/snapshots/test_snapshots.pytests/storage/conftest.py
📚 Learning: 2025-09-15T06:49:53.478Z
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 2045
File: tests/virt/cluster/vm_lifecycle/conftest.py:46-47
Timestamp: 2025-09-15T06:49:53.478Z
Learning: In the openshift-virtualization-tests repo, large fixture refactoring efforts like the golden image data source migration are handled incrementally by directory/team ownership. The virt/cluster directory is handled separately from virt/node, tests/infra, tests/storage, etc., with each area managed by relevant teams in follow-up PRs.
Applied to files:
tests/storage/conftest.py
📚 Learning: 2025-09-28T14:43:07.181Z
Learnt from: vamsikrishna-siddu
Repo: RedHatQE/openshift-virtualization-tests PR: 2199
File: tests/storage/test_online_resize.py:108-113
Timestamp: 2025-09-28T14:43:07.181Z
Learning: In the openshift-virtualization-tests repo, PR #2199 depends on PR #2139 which adds the OS_FLAVOR attribute to the Images.Cirros class, making Images.Cirros.OS_FLAVOR available for conditional logic in tests.
Applied to files:
tests/storage/conftest.py
📚 Learning: 2025-09-10T13:25:59.799Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 2014
File: tests/storage/cdi_clone/test_clone.py:0-0
Timestamp: 2025-09-10T13:25:59.799Z
Learning: In the openshift-virtualization-tests repository, when creating DataVolume objects using ocp_resources, use `api_name="storage"` rather than the default `api_name="pvc"`.
Applied to files:
tests/storage/conftest.py
📚 Learning: 2025-09-29T20:33:51.007Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1932
File: tests/virt/node/migration_and_maintenance/conftest.py:65-72
Timestamp: 2025-09-29T20:33:51.007Z
Learning: In tests/virt/node/migration_and_maintenance/conftest.py, the added_vm_cpu_limit fixture doesn't require ResourceEditor as a context manager because it's the final test to modify the vm_for_multifd_test VM before teardown, so restoration of CPU limits is unnecessary overhead as confirmed by maintainer dshchedr.
Applied to files:
tests/storage/conftest.py
📚 Learning: 2025-08-09T01:46:48.039Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1716
File: tests/virt/node/workload_density/test_swap.py:71-85
Timestamp: 2025-08-09T01:46:48.039Z
Learning: In the openshift-virtualization-tests repository, user dshchedr prefers that all test setup fixtures be explicitly declared as parameters in the test method itself rather than chaining fixtures through dependencies. This makes all setup steps visible in one place at the test method level, improving test readability and making dependencies explicit, even if a fixture isn't directly used within another fixture's implementation.
Applied to files:
tests/storage/conftest.py
📚 Learning: 2025-09-28T14:43:07.181Z
Learnt from: vamsikrishna-siddu
Repo: RedHatQE/openshift-virtualization-tests PR: 2199
File: tests/storage/test_online_resize.py:108-113
Timestamp: 2025-09-28T14:43:07.181Z
Learning: In the openshift-virtualization-tests repo, PR #2199 depends on PR #2139 which adds architecture-specific OS_FLAVOR attributes to the Images.Cirros class (OS_FLAVOR_CIRROS for x86_64/ARM64, OS_FLAVOR_FEDORA for s390x), enabling conditional logic based on the underlying OS flavor in tests.
Applied to files:
tests/storage/conftest.py
📚 Learning: 2025-06-18T09:19:05.769Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/test_vms_metrics.py:129-137
Timestamp: 2025-06-18T09:19:05.769Z
Learning: For Windows VM testing in tests/observability/metrics/test_vms_metrics.py, it's acceptable to have more fixture parameters than typical pylint recommendations when reusing expensive Windows VM fixtures for performance. Windows VMs take a long time to deploy, so reusing fixtures like windows_vm_for_test and adding labels via windows_vm_with_low_bandwidth_migration_policy is preferred over creating separate fixtures that would require additional VM deployments.
Applied to files:
tests/storage/conftest.py
📚 Learning: 2025-05-21T09:41:46.532Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 954
File: tests/storage/storage_migration/conftest.py:156-176
Timestamp: 2025-05-21T09:41:46.532Z
Learning: In the RedHatQE/openshift-virtualization-tests codebase, the pattern of calling to_dict() on a DataVolume object and then accessing dv.res to populate data_volume_template is the preferred approach rather than building the template dict explicitly.
Applied to files:
tests/storage/conftest.py
📚 Learning: 2025-09-17T14:02:24.619Z
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1702
File: tests/chaos/oadp/conftest.py:23-45
Timestamp: 2025-09-17T14:02:24.619Z
Learning: In the openshift-virtualization-tests repository, VirtualMachineForTests class automatically generates unique VM names when generate_unique_name=True (default). It uses utilities.infra.unique_name() which appends a timestamp to create names like "vm-oadp-chaos-{timestamp}", so hardcoded VM names in fixtures don't cause collisions during parallel test execution.
Applied to files:
tests/storage/conftest.py
📚 Learning: 2025-08-20T23:57:48.380Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1840
File: tests/virt/node/workload_density/test_swap.py:88-89
Timestamp: 2025-08-20T23:57:48.380Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, CNV installation and health are verified by sanity tests before other tests run, so hco_namespace is guaranteed to exist in the testing environment. Defensive programming against nil hco_namespace scenarios is not needed in fixtures.
Applied to files:
tests/storage/conftest.py
🧬 Code graph analysis (3)
tests/storage/upgrade/test_upgrade_storage.py (1)
utilities/storage.py (1)
run_command_on_vm_and_check_output(637-640)
tests/storage/snapshots/test_snapshots.py (3)
utilities/storage.py (1)
run_command_on_vm_and_check_output(637-640)tests/storage/conftest.py (2)
rhel_vm_for_snapshot(461-481)snapshot_with_content(485-528)tests/storage/snapshots/utils.py (2)
expected_output_after_restore(10-27)fail_to_create_snapshot_no_permissions(30-41)
tests/storage/conftest.py (2)
utilities/storage.py (2)
data_volume_template_with_source_ref_dict(566-585)write_file(627-634)tests/conftest.py (3)
namespace(661-674)rhel10_data_source_scope_session(885-891)snapshot_storage_class_name_scope_module(2475-2478)
🪛 Ruff (0.14.7)
tests/storage/snapshots/test_snapshots.py
375-375: Unused function argument: permissions_for_dv
(ARG001)
tests/storage/conftest.py
487-487: Unused function argument: namespace
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: build-container
- GitHub Check: tox
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
🔇 Additional comments (12)
utilities/storage.py (1)
637-641: LGTM! Generic function naming improves reusability.The rename from
run_command_on_cirros_vm_and_check_outputtorun_command_on_vm_and_check_outputremoves VM-type-specific naming, making the function more versatile and aligned with its actual behavior.tests/storage/upgrade/test_upgrade_storage.py (2)
19-19: LGTM! Import updated to match renamed function.The import correctly reflects the function rename in utilities/storage.py.
75-79: LGTM! Function call updated consistently.The call to
run_command_on_vm_and_check_outputis consistent with the renamed function.tests/storage/conftest.py (5)
24-29: LGTM! Necessary imports for RHEL VM support.The imports for
VirtualMachineClusterInstancetypeandVirtualMachineClusterPreferenceare correctly added to support the RHEL-based VM fixtures.
54-58: LGTM! Constants added for RHEL configuration.The
OS_FLAVOR_RHELandU1_SMALLconstants are appropriately added to support RHEL VM configuration.
71-71: LGTM! Helper function import for DataSource support.The import of
data_volume_template_with_source_ref_dictenables the fixture to create DataVolume templates from DataSources.
430-431: LGTM! Fixture renamed to reflect RHEL VM usage.The rename from
cirros_vm_nametorhel_vm_namecorrectly aligns the fixture name with its new purpose.
461-481: LGTM! Fixture successfully migrated to DataSource-based RHEL VM.The migration from Cirros to RHEL VM using DataSource is well-implemented:
- Uses
data_volume_template_with_source_ref_dicthelper withrhel10_data_source_scope_session- Properly configured with
VirtualMachineClusterInstancetypeandVirtualMachineClusterPreference- Correctly uses
OS_FLAVOR_RHELfor the VM flavortests/storage/snapshots/test_snapshots.py (4)
29-29: LGTM! Import updated to match renamed function.The import correctly reflects the function rename in utilities/storage.py.
50-113: LGTM! Test parametrization updated consistently.The parametrization correctly updates from Cirros to RHEL VM terminology:
cirros_vm_name→rhel_vm_namesnapshots_with_content→snapshot_with_contentAll test parameters are consistently updated across all test cases.
115-137: LGTM! Test implementation updated for RHEL VM fixtures.The test correctly uses:
rhel_vm_for_snapshotinstead ofcirros_vm_for_snapshotsnapshot_with_contentinstead ofsnapshots_with_contentrun_command_on_vm_and_check_outputinstead of the Cirros-specific variantAll references are consistent with the fixture changes.
373-383: LGTM! Thepermissions_for_dvfixture is used correctly.The static analysis warning about unused
permissions_for_dvis a false positive. This fixture is used for its side effect (setting up permissions for the test), which is a common pytest pattern. The fixture doesn't need to be referenced in the test body.
kshvaika
force-pushed
the
use_datasources_in_test_snapshot
branch
from
323b333 to
6199b70
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
approved these changes
Dec 5, 2025
Contributor
Author
|
/verified
|
Contributor
Test execution plan request detailsCRITICAL: You MUST respond with a review comment on the Files Changed tab, NOT as a regular PR comment. 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:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8587 🏁 Script executed: Length of output: 4423 @openshift-virtualization-qe-bot-3, I'll analyze the PR changes and create a test execution plan. Based on my analysis of the modified files, here are the key changes:
Test Execution Plan: Since you've already run
Note: The upgrade tests have special dependencies and markers ( I'll now post this as a review comment on the Files Changed tab as requested. 🧠 Learnings used |
Contributor
Author
|
/retest tox |
rnetser
requested changes
Dec 6, 2025
| "spec": cirros_dv_for_snapshot_dict["spec"], | ||
| }, | ||
| os_flavor=OS_FLAVOR_RHEL, | ||
| vm_instance_type=VirtualMachineClusterInstancetype(name=U1_SMALL), |
| }, | ||
| os_flavor=OS_FLAVOR_RHEL, | ||
| vm_instance_type=VirtualMachineClusterInstancetype(name=U1_SMALL), | ||
| vm_preference=VirtualMachineClusterPreference(name="rhel.10"), |
Collaborator
There was a problem hiding this comment.
ditto
and please use RHEL10_PREFERENCE constant
| ): | ||
| for idx in range(len(snapshots_to_restore_idx)): | ||
| snap_idx = snapshots_to_restore_idx[idx] | ||
| with VirtualMachineRestore( |
Collaborator
There was a problem hiding this comment.
if you're already modifying this code, please add client
jpeimer
requested changes
Dec 7, 2025
| The default behavior of the fixture is creating an offline | ||
| snapshot unless {online_vm = True} declared in the test | ||
| """ | ||
| snapshot unless {online_vm = True} declared in the test""" |
Contributor
There was a problem hiding this comment.
Suggested change
| snapshot unless {online_vm = True} declared in the test""" | |
| snapshot unless {online_vm = True} declared in the test | |
| """ |
| ) | ||
| if is_online_test: | ||
| cirros_vm_for_snapshot.start(wait=True) | ||
| rhel_vm_for_snapshot.start(wait=True) |
Contributor
There was a problem hiding this comment.
I'm not sure why we were using start() here, but it's better to use running_vm, as it waits for VM to be fully booted
Suggested change
| rhel_vm_for_snapshot.start(wait=True) | |
| running_vm(vm=rhel_vm_for_snapshot) |
| cirros_vm_for_snapshot.start(wait=True) | ||
| run_command_on_cirros_vm_and_check_output( | ||
| vm=cirros_vm_for_snapshot, | ||
| rhel_vm_for_snapshot.start(wait=True) |
Contributor
There was a problem hiding this comment.
Please replace with running_vm() in all places (unless there's a good reason not to)
Suggested change
| rhel_vm_for_snapshot.start(wait=True) | |
| running_vm(vm=rhel_vm_for_snapshot) |
|
|
||
|
|
||
| def run_command_on_cirros_vm_and_check_output(vm, command, expected_result): | ||
| def run_command_on_vm_and_check_output(vm, command, expected_result): |
Contributor
There was a problem hiding this comment.
Please create a new function under tests/storage/snapshots/utils.py
Upgrade tests will be changed separately.
Please try using run_ssh_commands instead of console. You can see an example in test_vmexport_snapshot_manifests (in vmexport test it only validates the content of one file, but for the snapshot tests we want to keep the validation of the ls command output).
| snapshot_with_content, | ||
| ): | ||
| cirros_vm_for_snapshot.start(wait=True) | ||
| rhel_vm_for_snapshot.start(wait=True) |
Comment on lines
502
to
+508
| write_file( | ||
| vm=cirros_vm_for_snapshot, | ||
| vm=rhel_vm_for_snapshot, | ||
| filename=f"{before_snap_index}.txt", | ||
| content=before_snap_index, | ||
| ) | ||
| if is_online_test: | ||
| cirros_vm_for_snapshot.start(wait=True) | ||
| rhel_vm_for_snapshot.start(wait=True) |
Contributor
There was a problem hiding this comment.
write_file stops the VM by default, and then we start it if is_online_test (doesn't seem very efficient).
Could you change that - so if is_online_test - pass write_file(..., stop_vm=False)
Contributor
There was a problem hiding this comment.
And actually, let's write the file via ssh, like in rhel_vm_for_snapshot_with_content (instead of console)
Comment on lines
480
to
481
| ) as vm: | ||
| yield vm |
Contributor
There was a problem hiding this comment.
I think it makes sense to add running_vm(vm=vm) here
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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:
Replaced the use of DVs with DataSources in the snapshot tests.
More details:
Updated the snapshot tests to use DataSources (RHEL VM) instead of DVs
https://issues.redhat.com/browse/CNV-69120
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.