storage: Add client to DataVolume creation in utilities/storage.py#5512
Closed
RoniKishner wants to merge 1 commit into
Closed
storage: Add client to DataVolume creation in utilities/storage.py#5512RoniKishner wants to merge 1 commit into
RoniKishner wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
utilities/storage.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
**⚙️ CodeRabbit configuration file
Files:
⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (28)📚 Learning: 2026-01-12T11:24:13.825ZApplied to files:
📚 Learning: 2026-01-12T14:25:05.723ZApplied to files:
📚 Learning: 2026-01-20T01:03:13.139ZApplied to files:
📚 Learning: 2026-01-21T21:26:41.805ZApplied to files:
📚 Learning: 2026-01-25T13:18:21.675ZApplied to files:
📚 Learning: 2026-02-18T06:35:39.536ZApplied to files:
📚 Learning: 2026-02-23T16:33:22.070ZApplied to files:
📚 Learning: 2026-03-17T01:32:02.617ZApplied to files:
📚 Learning: 2026-03-17T01:32:02.617ZApplied to files:
📚 Learning: 2026-05-04T13:45:29.122ZApplied to files:
📚 Learning: 2026-05-04T13:45:33.892ZApplied to files:
📚 Learning: 2026-05-05T17:01:15.294ZApplied to files:
📚 Learning: 2026-05-08T12:49:20.694ZApplied to files:
📚 Learning: 2026-05-12T05:10:24.601ZApplied to files:
📚 Learning: 2026-05-13T19:23:09.603ZApplied to files:
📚 Learning: 2026-05-18T06:30:56.781ZApplied to files:
📚 Learning: 2026-05-18T06:31:12.015ZApplied to files:
📚 Learning: 2026-05-18T06:31:15.083ZApplied to files:
📚 Learning: 2026-05-18T06:31:20.848ZApplied to files:
📚 Learning: 2026-05-18T09:09:09.479ZApplied to files:
📚 Learning: 2026-05-19T07:48:17.119ZApplied to files:
📚 Learning: 2026-05-19T07:48:17.119ZApplied to files:
📚 Learning: 2026-05-26T15:52:31.613ZApplied to files:
📚 Learning: 2026-06-21T20:28:07.727ZApplied to files:
📚 Learning: 2026-02-18T06:34:38.042ZApplied to files:
📚 Learning: 2026-05-03T15:38:09.624ZApplied to files:
📚 Learning: 2026-05-15T18:42:02.504ZApplied to files:
📚 Learning: 2026-06-15T10:56:21.758ZApplied to files:
🪛 Ruff (0.15.20)utilities/storage.py[warning] 151-151: Avoid specifying long messages outside the exception class (TRY003) [warning] 179-179: Boolean-typed positional argument in function definition (FBT001) [warning] 179-179: Boolean default positional argument in function definition (FBT002) [warning] 180-180: Boolean-typed positional argument in function definition (FBT001) [warning] 180-180: Boolean default positional argument in function definition (FBT002) [warning] 181-181: Boolean-typed positional argument in function definition (FBT001) [warning] 185-185: Boolean-typed positional argument in function definition (FBT001) [warning] 185-185: Boolean default positional argument in function definition (FBT002) [warning] 232-232: Avoid specifying long messages outside the exception class (TRY003) [warning] 237-237: Logging statement uses f-string (G004) [warning] 238-238: Logging statement uses f-string (G004) 🔇 Additional comments (2)
📝 WalkthroughWalkthroughTwo ChangesDataVolume client wiring
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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
the
tox:verify-tc-requirement-polarion:running
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5512 +/- ##
=======================================
Coverage 98.67% 98.67%
=======================================
Files 25 42 +17
Lines 2487 2495 +8
=======================================
+ Hits 2454 2462 +8
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
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. |
Contributor
Author
|
/test-plan |
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:
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: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
rm -rf repo
gh repo clone RedHatQE/openshift-virtualization-tests repo -- --depth=200 2>&1 | tail -5
cd repo
gh pr checkout 5512 2>&1 | tail -10 || true
git log --oneline -3
echo "---diff---"
git diff origin/main...HEAD -- utilities/storage.py || git diff HEAD~1 -- utilities/storage.pyLength of output: 1799 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp/repo
echo "=== callers of data_volume_template_dict ==="
rg -n 'data_volume_template_dict' --type=py | grep -v 'def data_volume_template_dict'
echo "=== callers of data_volume_template_with_source_ref_dict ==="
rg -n 'data_volume_template_with_source_ref_dict' --type=py | grep -v 'def data_volume_template_with_source_ref_dict'
echo "=== first changed line number in diff ==="
git diff origin/main...HEAD -- utilities/storage.py | head -20Length of output: 834 🏁 Script executed: #!/bin/bash
cd /tmp/repo
echo "=== all matches data_volume_template_dict ==="
rg -n 'data_volume_template_dict' --type=py
echo "=== all matches data_volume_template_with_source_ref_dict ==="
rg -n 'data_volume_template_with_source_ref_dict' --type=py
echo "=== smoke marked files ==="
rg -l '`@pytest.mark.smoke`' tests/ | head -50
echo "=== gating marked files ==="
rg -l '`@pytest.mark.gating`' tests/ | wc -lLength of output: 765 🏁 Script executed: #!/bin/bash
cd /tmp/repo
set -x
rg -n 'data_volume_template_dict' -g '*.py' || echo "no matches or err1"
rg -n 'data_volume_template_with_source_ref_dict' -g '*.py' || echo "no matches or err2"Length of output: 424 🏁 Script executed: #!/bin/bash
cd /tmp/repo
pwd
ls utilities/storage.py
grep -n "def data_volume_template" utilities/storage.py
grep -rn "data_volume_template" --include="*.py" . | head -50Length of output: 6145 🏁 Script executed: #!/bin/bash
cd /tmp/repo
echo "=== exact word match data_volume_template_dict ==="
grep -rn '\bdata_volume_template_dict\b' --include="*.py" .
echo "=== exact word match data_volume_template_with_source_ref_dict ==="
grep -rn '\bdata_volume_template_with_source_ref_dict\b' --include="*.py" .Length of output: 6993 🏁 Script executed: #!/bin/bash
cd /tmp/repo
echo "=== smoke files ==="
rg -l '`@pytest.mark.smoke`' tests/
echo "=== gating files intersect ==="
for f in tests/storage/cdi_clone/test_clone.py tests/virt/upgrade/conftest.py tests/virt/node/migration_and_maintenance/test_odf_vm_migration.py tests/storage/vm_export/conftest.py tests/storage/cross_cluster_live_migration/conftest.py tests/storage/storage_migration/conftest.py tests/storage/test_wffc.py tests/storage/data_import_cron/conftest.py tests/storage/general/conftest.py tests/storage/conftest.py tests/storage/golden_image/test_cached_snapshots.py tests/observability/metrics/conftest.py tests/infrastructure/numa/conftest.py tests/infrastructure/instance_types/supported_os/utils.py tests/infrastructure/instance_types/test_cpu_memory_hotplug_instancetype.py tests/infrastructure/instance_types/conftest.py tests/infrastructure/golden_images/update_boot_source/test_ssp_data_import_crons.py tests/infrastructure/golden_images/update_boot_source/conftest.py tests/infrastructure/golden_images/update_boot_source/test_boot_sources_vm.py tests/infrastructure/golden_images/test_common_templates_data_volumes.py tests/infrastructure/tekton/conftest.py; do
dir=$(dirname "$f")
# check test files in same directory as conftest for markers
echo "-- $f --"
done
echo "=== which test files in affected dirs have smoke marker ==="
for d in tests/storage/cdi_clone tests/virt/upgrade tests/virt/node/migration_and_maintenance tests/storage/vm_export tests/storage/cross_cluster_live_migration tests/storage/storage_migration tests/storage tests/storage/data_import_cron tests/storage/general tests/storage/golden_image tests/observability/metrics tests/infrastructure/numa tests/infrastructure/instance_types tests/infrastructure/instance_types/supported_os tests/infrastructure/golden_images/update_boot_source tests/infrastructure/golden_images tests/infrastructure/tekton; do
echo "-- dir: $d --"
rg -l '`@pytest.mark.smoke`' "$d" --max-depth 1 2>/dev/null
done
echo "=== gating marker in these dirs ==="
for d in tests/storage/cdi_clone tests/virt/upgrade tests/virt/node/migration_and_maintenance tests/storage/vm_export tests/storage/cross_cluster_live_migration tests/storage/storage_migration tests/storage tests/storage/data_import_cron tests/storage/general tests/storage/golden_image tests/observability/metrics tests/infrastructure/numa tests/infrastructure/instance_types tests/infrastructure/instance_types/supported_os tests/infrastructure/golden_images/update_boot_source tests/infrastructure/golden_images tests/infrastructure/tekton; do
echo "-- dir: $d --"
rg -l '`@pytest.mark.gating`' "$d" --max-depth 1 2>/dev/null
doneLength of output: 6726 🏁 Script executed: #!/bin/bash
cd /tmp/repo
echo "=== test_clone.py smoke marker + usage ==="
grep -n "pytest.mark.smoke\|data_volume_template_dict" tests/storage/cdi_clone/test_clone.py | head -20
echo "=== test_rhel_os.py smoke marker ==="
grep -n "pytest.mark.smoke" tests/infrastructure/instance_types/supported_os/test_rhel_os.py
echo "=== supported_os/utils.py usage ==="
grep -n "data_volume_template_with_source_ref_dict" tests/infrastructure/instance_types/supported_os/utils.py
echo "=== check test_rhel_os.py imports from utils ==="
grep -n "import" tests/infrastructure/instance_types/supported_os/test_rhel_os.py | grep -i utils
grep -n "utils\." tests/infrastructure/instance_types/supported_os/test_rhel_os.py | head -20Length of output: 1394 🏁 Script executed: #!/bin/bash
cd /tmp/repo
sed -n '1,60p' tests/storage/cdi_clone/test_clone.py
echo "---268 context---"
sed -n '255,290p' tests/storage/cdi_clone/test_clone.pyLength of output: 3088 🏁 Script executed: #!/bin/bash
cd /tmp/repo
FILE="utilities/storage.py"
LINE=557
BODY=$'**Test Execution Plan**\n\n- **Run smoke tests: True** — `tests/storage/cdi_clone/test_clone.py::test_clone_from_block_to_fs_using_dv_template` (smoke) → `create_vm_from_clone_dv_template()` (same file) → `data_volume_template_dict()` in `utilities/storage.py` (modified, adds `client=source_dv.client`).\n- **Run gating tests: True** — `data_volume_template_with_source_ref_dict()` (modified, adds `client=data_source.client`) is called from gating-adjacent conftests: `tests/virt/upgrade/conftest.py` → `tests/virt/upgrade/test_upgrade_virt.py` (gating); `tests/storage/vm_export/conftest.py` → `tests/storage/vm_export/test_vm_export.py` (gating); `tests/storage/conftest.py` → `tests/storage/golden_image/test_golden_image.py` (gating); `tests/infrastructure/instance_types/conftest.py` → `test_common_vm_instancetype.py`, `test_common_vm_preference.py`, `test_update_vm_with_instancetype_preference.py` (gating); `tests/infrastructure/instance_types/supported_os/utils.py` → `test_rhel_os.py` (smoke + gating).\n\n**Affected tests to run**\n- `tests/storage/cdi_clone/test_clone.py` (smoke, full file — uses `data_volume_template_dict`)\n- `tests/infrastructure/instance_types/supported_os/test_rhel_os.py` (smoke)\n- `tests/virt/upgrade/test_upgrade_virt.py` (gating)\n- `tests/storage/vm_export/test_vm_export.py` (gating)\n- `tests/storage/golden_image/test_golden_image.py` (gating)\n- `tests/infrastructure/instance_types/test_common_vm_instancetype.py` (gating)\n- `tests/infrastructure/instance_types/test_common_vm_preference.py` (gating)\n- `tests/infrastructure/instance_types/test_update_vm_with_instancetype_preference.py` (gating)\n- `tests/storage/test_wffc.py`, `tests/storage/data_import_cron/conftest.py`, `tests/storage/general/conftest.py`, `tests/storage/cross_cluster_live_migration/conftest.py`, `tests/storage/storage_migration/conftest.py`, `tests/observability/metrics/conftest.py`, `tests/infrastructure/numa/conftest.py`, `tests/infrastructure/golden_images/**` (other consumers of `data_volume_template_with_source_ref_dict`, no smoke/gating markers found, verify collection only)\n\nSince the change only adds an explicit `client` kwarg to existing `DataVolume(...)` calls (fixes a `FutureWarning`, no behavior change to returned dict), no separate real-cluster reproduction commands are required beyond running the listed tests.'
gh pr review 5512 --repo RedHatQE/openshift-virtualization-tests --comment -b "placeholder" 2>&1 | tail -5 || true
# Use API directly for inline comment since gh pr review doesn't support inline comments well
COMMIT_SHA=$(gh pr view 5512 --repo RedHatQE/openshift-virtualization-tests --json headRefOid -q .headRefOid)
echo "commit: $COMMIT_SHA"
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5512/comments \
-f body="$BODY" \
-f commit_id="$COMMIT_SHA" \
-f path="$FILE" \
-F line=$LINE \
-f side="RIGHT"Length of output: 8181 Inline comment with the test execution plan posted successfully. |
Contributor
|
/lgtm |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5443. Overlapping filesutilities/storage.py |
jpeimer
requested changes
Jul 6, 2026
Comment on lines
545
to
567
Contributor
There was a problem hiding this comment.
We need to rename this function because the current name is misleading...
It only creates a dv template dict for pvc clone
| size=size or source_dv.size, | ||
| source_pvc=source_dv.name, | ||
| source_namespace=source_dv.namespace, | ||
| api_name=source_dv.api_name, |
Contributor
There was a problem hiding this comment.
We should always prefer the storage api, unless there's a good reason not to
Suggested change
| api_name="storage", |
| client=source_dv.client, | ||
| source="pvc", | ||
| storage_class=storage_class or source_dv_pvc_spec.storageClassName, | ||
| volume_mode=volume_mode or source_dv_pvc_spec.volumeMode, |
Contributor
There was a problem hiding this comment.
No need to implicitly set the volume_mode
Suggested change
| volume_mode=volume_mode or source_dv_pvc_spec.volumeMode, | |
| volume_mode=volume_mode, |
| namespace=target_dv_namespace, | ||
| client=source_dv.client, | ||
| source="pvc", | ||
| storage_class=storage_class or source_dv_pvc_spec.storageClassName, |
Contributor
There was a problem hiding this comment.
No need to implicitly set the storage class
Suggested change
| storage_class=storage_class or source_dv_pvc_spec.storageClassName, | |
| storage_class=storage_class, |
Signed-off-by: rkishner <rkishner@redhat.com>
RoniKishner
force-pushed
the
storage-dv-client
branch
from
13542dc to
e0f2952
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Contributor
Author
rnetser
pushed a commit
that referenced
this pull request
Jul 9, 2026
##### What this PR does / why we need it: Rename `data_volume_template_dict` to `data_volume_template_dict_with_pvc_source` as [suggested by @jpeimer](#5512 (comment)) ##### Which issue(s) this PR fixes: ##### Special notes for reviewer: ##### jira-ticket: <!-- full-ticket-url needs to be provided. This would add a link to the pull request to the jira and close it when the pull request is merged If the task is not tracked by a Jira ticket, just write "NONE". --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Updated the storage helper used for clone-based virtual machine setup to a new, more explicitly typed variant. * Adjusted the related test wiring to use the updated helper when building clone templates. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: rkishner <rkishner@redhat.com>
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
8 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.
What this PR does / why we need it:
Add client to DataVolume in
data_volume_template_dictanddata_volume_template_with_source_ref_dictWhich issue(s) this PR fixes:
FutureWarningshowing when running testsSpecial notes for reviewer:
jira-ticket:
https://redhat.atlassian.net/browse/CNV-68519
Summary by CodeRabbit