[Storage] Use source_dict for DataVolume source specification, refactor create_dv#5443
[Storage] Use source_dict for DataVolume source specification, refactor create_dv#5443jpeimer wants to merge 6 commits into
Conversation
jpeimer
requested a deployment
to
external-pr-tests
GitHub Actions
Waiting
github-actions
Bot
added
sig-iuo
sig-chaos
sig-infra
sig-scale
sig-storage
sig-virt
labels
|
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 (38)
👮 Files not reviewed due to content moderation or server errors (38)
📝 Walkthrough🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 inconclusive)
✨ 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
tox:verify-tc-requirement-polarion:running
|
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
Custom Commands
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
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-6
requested review from
SamAlber,
josemacassan and
sarahbx
openshift-virtualization-qe-bot-6
requested review from
Ahmad-Hafe,
acinko-rh,
akalenyu,
albarker-rh,
ema-aka-young,
kgoldbla,
mijankow,
rlobillo and
vsibirsk
openshift-virtualization-qe-bot
removed
the
tox:verify-tc-requirement-polarion:passed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
/approve |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5443 +/- ##
=======================================
Coverage 98.67% 98.67%
=======================================
Files 25 42 +17
Lines 2487 2493 +6
=======================================
+ Hits 2454 2460 +6
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:
|
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5443 published |
|
/test-plan |
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: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== diff stat ==="
git log --oneline -5
git diff origin/main...HEAD --stat -- utilities/storage.py || true
echo "=== modified functions in utilities/storage.py ==="
git diff origin/main...HEAD -- utilities/storage.py | grep -E '^[+-]def ' || true
echo "=== smoke-marked test files ==="
rg -l '`@pytest.mark.smoke`' tests/ || true
echo "=== gating-marked test files ==="
rg -l '`@pytest.mark.gating`' tests/ || true
echo "=== callers of create_dv ==="
rg -l '\bcreate_dv\(' tests/ | head -50
echo "=== callers of construct_datavolume_source_dict ==="
rg -l 'construct_datavolume_source_dict' tests/ utilities/ | head -50
echo "=== callers of data_volume( fixture / data_volume_template_dict ==="
rg -l 'data_volume_template_dict|def data_volume\(' tests/ utilities/ | head -50Length of output: 7410 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== test_container_disk_vm.py imports/fixtures ==="
rg -n 'create_dv|DataVolume|data_volume' tests/virt/node/general/test_container_disk_vm.py tests/virt/node/general/conftest.py 2>/dev/null || true
echo "=== cdi_upload conftest create_dv usage ==="
rg -n 'create_dv\(' tests/storage/cdi_upload/conftest.py
echo "=== test_upload_virtctl.py fixtures used ==="
rg -n 'def test_' tests/storage/cdi_upload/test_upload_virtctl.py | head -20
rg -n 'fixture' tests/storage/cdi_upload/conftest.py | head -20
echo "=== common_templates conftest data_volume_template_dict usage ==="
rg -n 'data_volume_template_dict' tests/virt/cluster/common_templates/conftest.py
echo "=== csv smoke test and install_upgrade_operators create_dv relevance ==="
rg -n 'create_dv|DataVolume' tests/install_upgrade_operators/csv/test_csv.py || trueLength of output: 2876 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -A10 'def get_data_volume_template_dict_with_default_storage_class' tests/virt/utils.pyLength of output: 722 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '473,500p' tests/virt/utils.pyLength of output: 1606 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== exact calls to data_volume_template_dict( ==="
rg -n '\bdata_volume_template_dict\(' tests/ utilities/ || true
echo "=== exact calls to create_dv( within cdi_clone/test_clone.py context ==="
rg -n 'create_dv\(' tests/storage/cdi_clone/test_clone.pyLength of output: 798
Test Execution Plan
Affected tests to run
Real tests (cluster required) Error path (the fix — renamed PVC clone params): Happy path (regression — HTTP source_dict + artifactory lifecycle): |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4959. Overlapping filestests/virt/upgrade_custom/vgpu/conftest.py |
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5443 published |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/virt/upgrade_custom/vgpu/conftest.py`:
- Around line 162-169: The fixture name
`non_existent_mdev_bus_nodes_session_scope` is misleading because it actually
verifies that `mdev_bus` is available via `verify_mdev_bus_available` and yields
vGPU nodes with that check passed. Rename this fixture to something that matches
its behavior, such as `mdev_bus_verified_vgpu_nodes_session_scope` or
`vgpu_nodes_with_mdev_bus_verified`, and update all downstream references/usages
of `non_existent_mdev_bus_nodes_session_scope` to the new name.
🪄 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: e6965cf2-cc74-4e52-89ff-2b36525da096
📒 Files selected for processing (4)
tests/storage/cdi_clone/test_clone.pytests/virt/upgrade_custom/vgpu/conftest.pyutilities/storage.pyutilities/unittests/test_storage.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/virt/upgrade_custom/vgpu/conftest.py`:
- Around line 162-169: The fixture name
`non_existent_mdev_bus_nodes_session_scope` is misleading because it actually
verifies that `mdev_bus` is available via `verify_mdev_bus_available` and yields
vGPU nodes with that check passed. Rename this fixture to something that matches
its behavior, such as `mdev_bus_verified_vgpu_nodes_session_scope` or
`vgpu_nodes_with_mdev_bus_verified`, and update all downstream references/usages
of `non_existent_mdev_bus_nodes_session_scope` to the new name.
🪄 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: e6965cf2-cc74-4e52-89ff-2b36525da096
📒 Files selected for processing (4)
tests/storage/cdi_clone/test_clone.pytests/virt/upgrade_custom/vgpu/conftest.pyutilities/storage.pyutilities/unittests/test_storage.py
📜 Review details
🔇 Additional comments (6)
utilities/storage.py (1)
143-143: LGTM!utilities/unittests/test_storage.py (1)
126-133: LGTM!tests/virt/upgrade_custom/vgpu/conftest.py (2)
3-25: LGTM!
94-159: LGTM!tests/storage/cdi_clone/test_clone.py (2)
219-229: LGTM!
203-203: 🗄️ Data Integrity & IntegrationThe Windows DV rename is safe; no stale
dv-source-winreferences remain.> Likely an incorrect or invalid review comment.
🛑 Comments failed to post (1)
tests/virt/upgrade_custom/vgpu/conftest.py (1)
162-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Confusing fixture name — inverts actual behavior.
non_existent_mdev_bus_nodes_session_scopecallsverify_mdev_bus_available, which fails whenmdev_busis missing on a node — i.e., it asserts mdev_bus does exist. The fixture name implies the opposite ("non-existent mdev bus"), which will mislead anyone reading the fixture dependency chain in other conftests/tests.Consider a name that reflects what the fixture actually provides, e.g.
vgpu_nodes_with_mdev_bus_verifiedormdev_bus_verified_vgpu_nodes_session_scope.♻️ Suggested rename
-@pytest.fixture(scope="session") -def non_existent_mdev_bus_nodes_session_scope(workers_utility_pods, vgpu_ready_nodes_session_scope): +@pytest.fixture(scope="session") +def mdev_bus_verified_vgpu_nodes_session_scope(workers_utility_pods, vgpu_ready_nodes_session_scope): """Verify mdev_bus is available on all vGPU nodes.""" verify_mdev_bus_available( workers_utility_pods=workers_utility_pods, vgpu_nodes=vgpu_ready_nodes_session_scope, ) yield vgpu_ready_nodes_session_scopeNote: rename all downstream usages of
non_existent_mdev_bus_nodes_session_scopeaccordingly.🧰 Tools
🪛 Ruff (0.15.20)
[warning] 169-169: No teardown in fixture
non_existent_mdev_bus_nodes_session_scope, usereturninstead ofyieldReplace
yieldwithreturn(PT022)
🤖 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/virt/upgrade_custom/vgpu/conftest.py` around lines 162 - 169, The fixture name `non_existent_mdev_bus_nodes_session_scope` is misleading because it actually verifies that `mdev_bus` is available via `verify_mdev_bus_available` and yields vGPU nodes with that check passed. Rename this fixture to something that matches its behavior, such as `mdev_bus_verified_vgpu_nodes_session_scope` or `vgpu_nodes_with_mdev_bus_verified`, and update all downstream references/usages of `non_existent_mdev_bus_nodes_session_scope` to the new name.
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5440. Overlapping filestests/conftest.py |
…ter naming Extract construct_datavolume_source_dict as a pure function for building DataVolume source dicts. Refactor create_dv to own artifactory lifecycle in try/finally, require explicit source parameter, use generic annotations dict instead of multus_annotation/bind_immediate, rename parameters for consistency (secret_name, cert_configmap_name, source_pvc_name, source_pvc_namespace), add full type hints, and remove unused hostpath_node. Update all 21 caller files to match the new interface. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
…ce_dict() Migrate all DataVolume instantiations from the legacy source=/url=/secret=/cert_configmap= parameters to the new source_dict= parameter using construct_datavolume_source_dict() for consistent source specification across the codebase. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
…espace from source dict, add unit tests
- Add source_pvc_namespace= to all 9 PVC clone call sites that relied
on the old wrapper fallback (self.source_namespace or self.namespace)
- Omit namespace key from PVC source dict when source_pvc_namespace is
None to avoid serializing {"namespace": None}
- Add 14 unit tests for construct_datavolume_source_dict covering all 5
source types, secret/cert enrichment, and unsupported source error
- Mock utilities.virt in unittests conftest.py to break circular import
Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
After create_dv refactoring, url is no longer passed to the DataVolume constructor (only embedded in source_dict). Check dv.name instead and use OS_FLAVOR_WINDOWS constant for the parametrized dv_name. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Jenia Peimer <jpeimer@redhat.com>
|
/retest all |
|
/retest tox |
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5443 published |
8 participants
What this PR does / why we need it:
DataVolumesourceargument is deprecated and needs to be replaced by thesource_dictRefactor
create_dvand allDataVolumeinstantiations to usesource_dict=with
construct_datavolume_source_dict()instead of the legacysource=.Why: The old
create_dvinterface mixed source specification, artifactorylifecycle, and annotation handling into a single flat parameter list. This made
it unclear which parameters applied to which source types and coupled every
caller to artifactory internals.
Changes:
construct_datavolume_source_dict()as a pure function for buildingDataVolume source dicts from typed parameters (
http,registry,pvc,blank,upload)create_dvto acceptsource_dict=directly, own artifactorylifecycle in
try/finally, use genericannotationsdict instead ofmultus_annotation/bind_immediate, and add full type hintssecret→secret_name,cert_configmap→cert_configmap_name,source_pvc→source_pvc_name,source_namespace→source_pvc_namespacehostpath_nodeparameterWhich issue(s) this PR fixes:
Special notes for reviewer:
create_dvand updates callersusing
create_dv, second migrates directDataVolume()instantiations touse
construct_datavolume_source_dict()DataVolumespecs as beforearchitecturein DV spec.jira-ticket:
Summary by CodeRabbit
New Features
Bug Fixes
Tests