Fix clone to filesystem PVC sizing when storageProfile minimum applies#4098
Fix clone to filesystem PVC sizing when storageProfile minimum applies#4098arnongilboa wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
7fdfe12 to
e80bdad
Compare
|
fyi @arnongilboa #3901 |
|
/cc @akalenyu |
|
/hold |
e80bdad to
7d431d1
Compare
|
/hold |
7d431d1 to
67e56b2
Compare
67e56b2 to
1cc1766
Compare
|
/unhold |
noamasu
left a comment
There was a problem hiding this comment.
thanks! for clone to fs, make sense to to add fsoverhead after effective size.
added some comments
| Entry("[test_id:12423] empty", "", false), | ||
| ) | ||
|
|
||
| It("[test_id:XXXXX] Block volumeMode clone to filesystem, using storageProfile with minPvcSize annotation", Serial, func() { |
There was a problem hiding this comment.
need to add real test_id here
There was a problem hiding this comment.
No need, removing.
| } | ||
|
|
||
| func verifyPVCRequestedSize(dataVolume *cdiv1.DataVolume, f *framework.Framework, size string) { | ||
| func verifyPVCRequestedSize(dataVolume *cdiv1.DataVolume, f *framework.Framework, minSize, comparator string) { |
There was a problem hiding this comment.
maybe worth just adding another function like verifyPVCRequestedSizeExceeds for ">" instead of passing comparator?
| } | ||
|
|
||
| By(fmt.Sprintf("Create source block DataVolume %s", dataVolumeName)) | ||
| sc := createStorageWithMinimumSupportedPVCSize(f, "4Gi") |
There was a problem hiding this comment.
what if actual min size of the sc is larger than 4Gi?
for example if actual min size of the storage backend is 10Gi, this test will fail?
There was a problem hiding this comment.
You mean if the storage creates 10Gi in case of a smaller requested size (without using our annotation)? it will fail in the pre-clone check as the target will be too small, but I see no problem with such failure as we don't want to "guess" the backend behavior before the pvc is created. However, in a real use case, explicitly setting the minimum annotation to be aligned with the backup minimum size will solve this issue.
| f.ExpectEvent(dataVolume.Namespace).Should(ContainSubstring(controller.CloneSucceeded)) | ||
|
|
||
| verifyPVCRequestedSize(dataVolume, f, testMinPvcSize) | ||
| verifyPVCRequestedSize(dataVolume, f, testMinPvcSize, "==") |
There was a problem hiding this comment.
With the new logic for filesystem clones (minimum applied first, then overhead), shouldn't the resulting PVC size be greater than the minimum rather than equal to it?
There was a problem hiding this comment.
Sure, but this test is for block.
When cloning to filesystem, apply the storageProfile effective volume size (minimum supported PVC size) before inflating with filesystem overhead. It helps on cloning from block to filesystem where both have minimum supported size but the target also needs filesystem overhead on top of that, otherwise the pre-clone checks fail. Signed-off-by: Arnon Gilboa <agilboa@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
1cc1766 to
ebd0461
Compare
Compute testMinPvcSize in BeforeEach as max("4Gi", originalMinPvcSize)
instead of a hardcoded constant, to avoid test failures on environments
where the storage profile already has a minimum larger than 4Gi.
Signed-off-by: Arnon Gilboa <agilboa@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
/lgtm |
|
@Ahmad-Hafe: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
There has been no activity on this PR for 45 days. What you can do:
/lifecycle stale |
What this PR does / why we need it:
When cloning to filesystem, apply the storageProfile effective volume size (minimum supported PVC size) before inflating with filesystem overhead. It helps on cloning from block to filesystem where both have minimum supported size but the target also needs filesystem overhead on top of that, otherwise the pre-clone checks fail.
jira-ticket: https://redhat.atlassian.net/browse/CNV-79503
Release note: