fix(storage): pass client to openshift-python-wrapper resources in storage tests - #5871
fix(storage): pass client to openshift-python-wrapper resources in storage tests#5871davmarro wants to merge 5 commits into
Conversation
GitHub Actions
Waiting
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughWalkthroughChangesStorage utilities, fixtures, and storage tests now pass explicit Kubernetes clients to resource constructors, lookups, polling helpers, and cleanup checks. Several DataVolume workflows also disable WFFC consumption and apply immediate-binding annotations. Storage client configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change makes storage tests pass explicit clients to resource constructors without introducing an evidenced production behavior change; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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 |
|
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
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
📌 Additional InformationCustom Commands:
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
removed
the
tox:verify-tc-requirement-polarion:running
|
/approve |
|
/check-can-merge |
|
hey @rnetser @josemacassan @jpeimer, also the smoke tests passed. Thanks |
|
/lgtm |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5672. Overlapping filestests/conftest.py |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/conftest.py (1)
56-56: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMEDIUM: Replace the bare module import.
import utilities.hcoviolates the repository import rule. Usefrom utilities import hcoand update qualified references to usehco. This rule keeps module dependencies explicit and consistent.Proposed change
-import utilities.hco +from utilities import hcoAs per coding guidelines, “Use absolute imports, prefer specific imports, and do not use bare
import modulewithout afromclause.”🤖 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/conftest.py` at line 56, Replace the bare utilities.hco import with a specific from utilities import hco import, then update all references in the affected test configuration to use hco instead of utilities.hco.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@tests/conftest.py`:
- Line 56: Replace the bare utilities.hco import with a specific from utilities
import hco import, then update all references in the affected test configuration
to use hco instead of utilities.hco.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4a309171-7459-4c08-a1d5-f26a41377a4b
📒 Files selected for processing (1)
tests/conftest.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
RedHatQE/openshift-virtualization-tests-design-docs(manual)
|
Clean rebase detected — no code changes compared to previous head ( |
|
/verified openshift-virtualization-tests-runner/6129 -s -o log_cli=true -rsx --jira -m 'tier2 and storage and gating' --latest-rhel tests/storage --storage-class-matrix=ocs-storagecluster-ceph-rbd-virtualization openshift-virtualization-tests-runner/6133 |
Missing
|
|
Clean rebase detected — no code changes compared to previous head ( |
@rnetser thanks for spotting it. |
|
Clean rebase detected — no code changes compared to previous head ( |
|
/approve |
|
/lgtm |
|
/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: |
|
Hi @rnetser do you think it can be merged ? |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5708. Overlapping filestests/storage/test_hotplug.py |
…orage tests Pass an explicit DynamicClient to all ocp_resources constructors in tests/storage/, utilities/storage.py, and tests/data_protection/oadp/ so resource creation does not fall back to implicit get_client(). jira-ticket: https://redhat.atlassian.net/browse/CNV-68536 Signed-off-by: davmarro <dmarro@redhat.com>
…hods Signed-off-by: davmarro <dmarro@redhat.com>
…rInstancetype and VirtualMachineClusterPreference Signed-off-by: davmarro <dmarro@redhat.com>
Signed-off-by: davmarro <dmarro@redhat.com>
Signed-off-by: davmarro <dmarro@redhat.com>
|
/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: |
|
|
||
| @pytest.fixture(scope="session") | ||
| def storage_class_with_filesystem_volume_mode(available_storage_classes_names): | ||
| def storage_class_with_filesystem_volume_mode(admin_client, available_storage_classes_names): |
There was a problem hiding this comment.
Test Execution Plan
Run smoke tests: True — tests/storage/cdi_clone/test_clone.py::test_clone_from_block_to_fs_using_dv_template → fedora_dv_with_block_volume_mode and storage_class_with_filesystem_volume_mode → get_storage_class_with_specified_volume_mode(..., client=admin_client). This validates explicit client propagation in the shared session fixture.
Run gating tests: True — tests/storage/cdi_clone/test_clone.py::test_clone_from_fs_to_block_using_dv_template → the shared block and filesystem storage-class fixtures → get_storage_class_with_specified_volume_mode(..., client=admin_client). tests/storage/test_hotplug.py migration tests → is_dv_migratable(..., client=admin_client) → StorageProfile. tests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning_vms.py::test_create_vm_with_cloned_data_volume_permissions_for_pods_positive → data_volume_clone_settings(..., unprivileged_client).
Affected tests to run
tests/storage/cdi_clone/test_clone.py::test_clone_from_block_to_fs_using_dv_template(smoke)tests/storage/cdi_clone/test_clone.py::test_successful_vm_restart_with_cloned_dv(gating)tests/storage/cdi_clone/test_clone.py::test_clone_from_fs_to_block_using_dv_template(gating)tests/storage/test_hotplug.py::TestHotPlugWithPersist::test_hotplug_volume_with_bus_and_persist_migrate(gating)tests/storage/test_hotplug.py::TestHotPlugWithSerialPersist::test_hotplug_volume_with_serial_and_persist_migrate(gating)tests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning_vms.py::test_create_vm_with_cloned_data_volume_permissions_for_pods_positive(gating)tests/storage/cdi_config/test_cdi_config.py::test_cdi_spec_reconciled_by_hcotests/storage/cdi_config/test_cdi_config.py::test_different_route_for_upload_proxytests/storage/cdi_upload/test_upload.py::test_cdi_uploadproxy_route_owner_referencestests/storage/cdi_upload/test_upload.py::test_successful_concurrent_uploadstests/storage/cdi_upload/test_upload_virtctl.py::test_virtctl_image_upload_with_exist_dv_imagetests/storage/cdi_upload/test_upload_virtctl.py::test_virtctl_image_upload_pvc(gating)tests/storage/cdi_upload/test_upload_virtctl.py::test_virtctl_image_upload_with_exist_pvc_imagetests/storage/fs_overhead/test_fs_overhead.pytests/storage/golden_image/test_cached_snapshots.pytests/storage/data_import_cron/test_data_import_cron_pvc_source.pytests/storage/test_data_import_cron.py::test_data_import_cron_garbage_collectiontests/storage/hpp/test_hpp_node_placement.pytests/storage/hpp/test_hostpath.pytests/storage/snapshots/test_snapshots.py::test_online_windows_vm_successful_restoretests/storage/snapshots/test_snapshots.py::test_write_to_file_while_snapshottests/storage/test_cdi_certificate.py::test_upload_after_validate_aggregated_api_certtests/storage/test_wffc.py::test_wffc_vm_with_two_data_volume_templates
Real tests (cluster required)
Error path (explicit client propagation):
pytest tests/storage/test_hotplug.py -m gating -k 'persist_migrate'
Expected: StorageProfile uses admin_client. The migration tests complete without a missing-client constructor error.
Happy path (shared storage-class fixtures):
pytest tests/storage/cdi_clone/test_clone.py::test_clone_from_block_to_fs_using_dv_template
Expected: filesystem and block StorageProfile lookups use admin_client, and the clone completes.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5871"
echo '=== PR metadata and changed files ==='
gh pr view "$pr" --repo "$repo" --json headRefOid,baseRefName,files \
--jq '{head: .headRefOid, base: .baseRefName, files: [.files[].path]}'
base_ref="$(gh pr view "$pr" --repo "$repo" --json baseRefName --jq '.baseRefName')"
base="$(git merge-base HEAD "origin/$base_ref" 2>/dev/null || true)"
if [ -n "$base" ]; then
echo "=== Diff base: $base ==="
git diff --name-only "$base" HEAD
echo '=== First changed-file patch ==='
first_file="$(git diff --name-only "$base" HEAD | head -n1)"
printf '%s\n' "$first_file"
git diff --unified=2 "$base" HEAD -- "$first_file"
echo '=== Changed utility definitions ==='
git diff --unified=0 "$base" HEAD -- utilities/storage.py |
grep -E '^[+-][[:space:]]*(async[[:space:]]+)?def[[:space:]]+' || true
else
echo '=== PR patches from GitHub ==='
gh api "repos/$repo/pulls/$pr/files?per_page=100" --paginate \
--jq '.[] | "FILE: \(.filename)\n\(.patch // "")"'
fi
echo '=== Modified storage helper callers ==='
for symbol in \
create_dummy_first_consumer_pod \
cdi_feature_gate_list_with_added_feature \
get_storage_class_with_specified_volume_mode \
verify_dv_and_pvc_does_not_exist \
wait_for_volume_snapshot_ready_to_use \
wait_for_succeeded_dv
do
echo "--- $symbol ---"
rg -n -C2 "\b${symbol}\s*\(" tests utilities --glob '*.py' || true
done
echo '=== Changed fixture and local helper consumers ==='
rg -n -C2 \
'\b(storage_class_with_filesystem_volume_mode|storage_class_with_block_volume_mode|uploadproxy_route_deleted|new_route_created|imported_data_source|rhel9_cached_snapshot|cirros_pv_on_hpp|data_volume_clone_settings|windows_snapshot|blank_dv_template_wffc_scope_function|is_dv_migratable|wait_for_succeeded_imported_object|assert_first_imported_object_was_deleted)\b' \
tests --glob '*.py' || true
echo '=== Fixture teardown paths and utility wrappers ==='
rg -n -C3 'yield from|^[[:space:]]*yield\b|`@contextmanager`' tests utilities/storage.py --glob '*.py' || true
echo '=== Smoke and gating test declarations ==='
rg -n -C2 '`@pytest`\.mark\.(smoke|gating)' tests --glob '*.py' || trueLength of output: 162561 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5871"
echo '=== Current head and changed-file order ==='
gh pr view "$pr" --repo "$repo" --json headRefOid,files \
--jq '"HEAD=" + .headRefOid, (.files[] | .path)'
echo '=== First changed-file patch from GitHub ==='
gh api "repos/$repo/pulls/$pr/files?per_page=100" --paginate \
--jq '.[] | select(.filename == "tests/conftest.py") | .patch'
echo '=== utilities/storage.py patch ==='
gh api "repos/$repo/pulls/$pr/files?per_page=100" --paginate \
--jq '.[] | select(.filename == "utilities/storage.py") | .patch'
echo '=== Exact changed-fixture/helper consumers in storage tests ==='
rg -n -C2 \
'\b(storage_class_with_filesystem_volume_mode|storage_class_with_block_volume_mode|uploadproxy_route_deleted|new_route_created|imported_data_source|rhel9_cached_snapshot|cirros_pv_on_hpp|data_volume_clone_settings|windows_snapshot|blank_dv_template_wffc_scope_function|is_dv_migratable|wait_for_succeeded_imported_object|assert_first_imported_object_was_deleted|create_dummy_first_consumer_pod|cdi_feature_gate_list_with_added_feature|get_storage_class_with_specified_volume_mode|verify_dv_and_pvc_does_not_exist|wait_for_volume_snapshot_ready_to_use|wait_for_succeeded_dv)\b' \
tests/storage tests/conftest.py utilities/storage.py --glob '*.py'
echo '=== Relevant marked test definitions ==='
rg -n -C4 \
'`@pytest`\.mark\.(smoke|gating)|^(class TestHotPlugWithPersist|class TestHotPlugWithSerialPersist|def test_(clone_from_block_to_fs_using_dv_template|successful_vm_restart_with_cloned_dv|clone_from_fs_to_block_using_dv_template|create_vm_with_cloned_data_volume_permissions_for_pods_positive|cdi_spec_reconciled_by_hco|different_route_for_upload_proxy|cdi_uploadproxy_route_owner_references|successful_concurrent_uploads|virtctl_image_upload_with_exist_dv_image|virtctl_image_upload_pvc|virtctl_image_upload_with_exist_pvc_image|data_import_cron_garbage_collection|wffc_vm_with_two_data_volume_templates))' \
tests/storage/cdi_clone/test_clone.py \
tests/storage/cdi_config/test_cdi_config.py \
tests/storage/cdi_upload/test_upload.py \
tests/storage/cdi_upload/test_upload_virtctl.py \
tests/storage/test_hotplug.py \
tests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning_vms.py \
tests/storage/test_data_import_cron.py \
tests/storage/test_wffc.py
echo '=== Fixture definitions in directly affected conftest files ==='
rg -n -C2 \
'^(def (storage_class_with_filesystem_volume_mode|storage_class_with_block_volume_mode|uploadproxy_route_deleted|new_route_created|imported_data_source|rhel9_cached_snapshot|cirros_pv_on_hpp|data_volume_clone_settings|windows_snapshot)|`@pytest`\.fixture)' \
tests/conftest.py tests/storage/conftest.py tests/storage/data_import_cron/conftest.py \
tests/storage/hpp/conftest.py tests/storage/restricted_namespace_cloning/conftest.py \
tests/storage/snapshots/conftest.pyLength of output: 119605 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5871"
head_sha="0e698a3561d483fb549aa54a71e9742ff42fadb8"
body_file="$(mktemp)"
cat >"$body_file" <<'EOF'
## Test Execution Plan
**Run smoke tests: True** — `tests/storage/cdi_clone/test_clone.py::test_clone_from_block_to_fs_using_dv_template` → `fedora_dv_with_block_volume_mode` and `storage_class_with_filesystem_volume_mode` → `get_storage_class_with_specified_volume_mode(..., client=admin_client)`. This validates explicit client propagation in the shared session fixture.
**Run gating tests: True** — `tests/storage/cdi_clone/test_clone.py::test_clone_from_fs_to_block_using_dv_template` → the shared block and filesystem storage-class fixtures → `get_storage_class_with_specified_volume_mode(..., client=admin_client)`. `tests/storage/test_hotplug.py` migration tests → `is_dv_migratable(..., client=admin_client)` → `StorageProfile`. `tests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning_vms.py::test_create_vm_with_cloned_data_volume_permissions_for_pods_positive` → `data_volume_clone_settings(..., unprivileged_client)`.
**Affected tests to run**
- `tests/storage/cdi_clone/test_clone.py::test_clone_from_block_to_fs_using_dv_template` (smoke)
- `tests/storage/cdi_clone/test_clone.py::test_successful_vm_restart_with_cloned_dv` (gating)
- `tests/storage/cdi_clone/test_clone.py::test_clone_from_fs_to_block_using_dv_template` (gating)
- `tests/storage/test_hotplug.py::TestHotPlugWithPersist::test_hotplug_volume_with_bus_and_persist_migrate` (gating)
- `tests/storage/test_hotplug.py::TestHotPlugWithSerialPersist::test_hotplug_volume_with_serial_and_persist_migrate` (gating)
- `tests/storage/restricted_namespace_cloning/test_restricted_namespace_cloning_vms.py::test_create_vm_with_cloned_data_volume_permissions_for_pods_positive` (gating)
- `tests/storage/cdi_config/test_cdi_config.py::test_cdi_spec_reconciled_by_hco`
- `tests/storage/cdi_config/test_cdi_config.py::test_different_route_for_upload_proxy`
- `tests/storage/cdi_upload/test_upload.py::test_cdi_uploadproxy_route_owner_references`
- `tests/storage/cdi_upload/test_upload.py::test_successful_concurrent_uploads`
- `tests/storage/cdi_upload/test_upload_virtctl.py::test_virtctl_image_upload_with_exist_dv_image`
- `tests/storage/cdi_upload/test_upload_virtctl.py::test_virtctl_image_upload_pvc` (gating)
- `tests/storage/cdi_upload/test_upload_virtctl.py::test_virtctl_image_upload_with_exist_pvc_image`
- `tests/storage/fs_overhead/test_fs_overhead.py`
- `tests/storage/golden_image/test_cached_snapshots.py`
- `tests/storage/data_import_cron/test_data_import_cron_pvc_source.py`
- `tests/storage/test_data_import_cron.py::test_data_import_cron_garbage_collection`
- `tests/storage/hpp/test_hpp_node_placement.py`
- `tests/storage/hpp/test_hostpath.py`
- `tests/storage/snapshots/test_snapshots.py::test_online_windows_vm_successful_restore`
- `tests/storage/snapshots/test_snapshots.py::test_write_to_file_while_snapshot`
- `tests/storage/test_cdi_certificate.py::test_upload_after_validate_aggregated_api_cert`
- `tests/storage/test_wffc.py::test_wffc_vm_with_two_data_volume_templates`
**Real tests (cluster required)**
Error path (explicit client propagation):
`pytest tests/storage/test_hotplug.py -m gating -k 'persist_migrate'`
Expected: `StorageProfile` uses `admin_client`. The migration tests complete without a missing-client constructor error.
Happy path (shared storage-class fixtures):
`pytest tests/storage/cdi_clone/test_clone.py::test_clone_from_block_to_fs_using_dv_template`
Expected: filesystem and block `StorageProfile` lookups use `admin_client`, and the clone completes.
EOF
gh api --method POST "repos/$repo/pulls/$pr/comments" \
-f body="$(cat "$body_file")" \
-f commit_id="$head_sha" \
-f path="tests/conftest.py" \
-F line=1275 \
-f side="RIGHT" \
>/dev/null
rm -f "$body_file"Length of output: 4107
You are interacting with an AI system. |
13 participants
What this PR does / why we need it:
Pass an explicit DynamicClient to all ocp_resources constructors in tests/storage/, utilities/storage.py, and tests/data_protection/oadp/ so resource creation does not fall back to implicit get_client().
The client is selected based following these criteria:
admin_clientfor cluster-scoped resources and admin operationsunprivileged_clientfor test resources that verify RBACresource.clientwhen a parent resource already has the correct clientWhich issue(s) this PR fixes:
Not issue at all.
In the next major release, all the openshift wrapper constructor will have a mandatory
clientarg. So in this PR we're fixing all the methods where theclientis not passed.Special notes for reviewer:
The PR is quite big (19 files modified) but the changes are really simple, most of them are just adding a client to method signatures, so nothing complex.
jira-ticket:
https://redhat.atlassian.net/browse/CNV-68536
Summary by CodeRabbit
Bug Fixes
Tests