WIP: [Storgae] [Mian] Drop explicit target size in block-to-fs clone test#4606
WIP: [Storgae] [Mian] Drop explicit target size in block-to-fs clone test#4606Ahmad-Hafe wants to merge 4 commits intoRedHatQE:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved the filesystem-overhead fixture and its helper; DataVolume template generation now omits resource requests when ChangesDV template & tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-3
requested review from
dalia-frank and
jpeimer
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4606 +/- ##
==========================================
+ Coverage 98.63% 98.67% +0.03%
==========================================
Files 25 25
Lines 2420 2485 +65
==========================================
+ Hits 2387 2452 +65
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
openshift-virtualization-qe-bot-3
requested review from
RoniKishner,
acinko-rh,
josemacassan,
kgoldbla,
kshvaika and
vsibirsk
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utilities/storage.py`:
- Around line 560-562: Add a brief inline comment immediately above the existing
conditional that clears dv.res["spec"][source_dv.api_name]["resources"] when
size is falsy: explain that an omitted size parameter (size is None) signals CDI
to auto-detect target capacity rather than locking to a precomputed value
(source_dv.size), which avoids problems with minimumSupportedPvcSize for
block-to-filesystem clones; keep the comment concise and place it alongside the
conditional in the function where dv.to_dict() is used and dv.res is modified.
🪄 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: 5487b00f-e8b5-4428-82c0-4428b8c18dbf
📒 Files selected for processing (2)
tests/storage/cdi_clone/test_clone.pyutilities/storage.py
💤 Files with no reviewable changes (1)
- tests/storage/cdi_clone/test_clone.py
Ahmad-Hafe
force-pushed
the
fix_block_to_fs_clone_drop_explicit_size
branch
from
dec32f9 to
7503001
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
Ahmad-Hafe
changed the title
Let CDI auto-detect the target size instead of manually calculating it with filesystem overhead. This fixes failures on environments with minimumSupportedPvcSize where the actual PVC capacity exceeds the DV spec size. Signed-off-by: Ahmad Hafe <ahafe@redhat.com> Made-with: Cursor Signed-off-by: Ahmad Hafe <ahafe@redhat.com>
Made-with: Cursor Signed-off-by: Ahmad Hafe <ahafe@redhat.com>
Signed-off-by: Ahmad Hafe <ahafe@redhat.com> Made-with: Cursor
Documents parameters and the non-obvious size=None behavior where CDI auto-detects capacity instead of using a pre-computed value. Signed-off-by: Ahmad Hafe <ahafe@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
/retest verify-bugs-are-open |
|
/verified execute following test under ::tests/storage/cdi_clone/test_clone.py with cloud providor sc (sp-balanced-storage) and also ocs-storagecluster-ceph-rbd-virtualization pass rate 100% |
|
/retest verify-bugs-are-open |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Your deliverable: Test Execution Plan
Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL: Before setting "Run smoke tests: True", you MUST use shell scripts to: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3630 🏁 Script executed: Length of output: 3258 |
|
/retest verify-bugs-are-open |
|
/reprocess |
|
/verified execute following test under ::tests/storage/cdi_clone/test_clone.py with cloud providor sc (sp-balanced-storage) and also ocs-storagecluster-ceph-rbd-virtualization pass rate 100% |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Your deliverable: Test Execution Plan
Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL: Before setting "Run smoke tests: True", you MUST use shell scripts to: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3726 |
|
/retest verify-bugs-are-open |
1 similar comment
|
/retest verify-bugs-are-open |
|
/reprocess |
|
/verified Tests passed on GCP (sp-balanced-storage) and ODF (ocs-storagecluster-ceph-rbd-virtualization) |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Your deliverable: Test Execution Plan
Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL: Before setting "Run smoke tests: True", you MUST use shell scripts to: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3468 |
|
@myakove The |
did you check the log? the error is very clear |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4603. Overlapping filesutilities/storage.py |
|
/wip need to raise this in grooming fort further determination |
|
/hold |
|
Ahmad-Hafe is not part of the approver, only approvers can mark pull request with hold |
11 participants
Summary
test_clone_from_block_to_fs_using_dv_templateresources: {}in the DV template when no size is specifiedoverhead_size_for_dv,default_fs_overhead, andimport mathminimumSupportedPvcSize(e.g. GCP withsp-balanced-storage) where the actual PVC capacity (4Gi) exceeds the DV spec size (1Gi), causing "target resources requests storage size is smaller than the source" errorsDetails
This is a test issue, not a CDI bug. When
minimumSupportedPvcSizeis set, the CSI driver rounds up the PVC capacity, but the test was calculating the target size based on the DV spec size rather than the actual PVC capacity. The correct approach (confirmed by CDI devs) is to omit the target size entirely so CDI auto-detects it, including filesystem overhead.Changes
tests/storage/cdi_clone/test_clone.py: Removesizeandoverhead_size_for_dvfromtest_clone_from_block_to_fs_using_dv_templateutilities/storage.py: Indata_volume_template_dict, setresources: {}when nosizeis provided; remove unusedoverhead_size_for_dvfunction andimport mathtests/storage/conftest.py: Remove unuseddefault_fs_overheadfixtureImpacted tests
test_clone_from_block_to_fs_using_dv_template-- directly fixedtest_clone_from_fs_to_block_using_dv_template-- also callsdata_volume_template_dictwithoutsize, now getsresources: {}(CDI auto-detect); no behavioral change expectedTest plan
test_clone_from_block_to_fs_using_dv_templateon an environment withminimumSupportedPvcSize(e.g. GCP withsp-balanced-storage)test_clone_from_block_to_fs_using_dv_templateon a standard environment (e.g. ODF/Ceph)test_clone_from_fs_to_block_using_dv_templatestill passesSummary by CodeRabbit