-
Notifications
You must be signed in to change notification settings - Fork 546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cephfs: Fix cephfs PVC sizing #4180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change looks good.
Would it make sense to add an e2e test with a value that caused the failure?
internal/util/util.go
Outdated
@@ -67,9 +67,9 @@ func RoundOffCephFSVolSize(bytes int64) int64 { | |||
return 4 * helpers.MiB | |||
} | |||
|
|||
bytes /= helpers.MiB | |||
floatbytes := float64(bytes) / helpers.MiB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bytesInFloat sounds better? as @nixpanic suggested please add a E2E test for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
f1c4a6b
to
dc776b3
Compare
/test ci/centos/mini-e2e/k8s-1.28 |
e2e/cephfs.go
Outdated
@@ -2426,6 +2427,35 @@ var _ = Describe(cephfsType, func() { | |||
validateOmapCount(f, 0, cephfsType, metadataPool, volumesType) | |||
}) | |||
|
|||
By("Test 500MB PVC creation and check for PV and PVC binding", func() { | |||
size := "500M" | |||
err := createCephfsStorageClass(f.ClientSet, f, true, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be skipped, SC is already created here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
dc776b3
to
16b52da
Compare
/test ci/centos/mini-e2e/k8s-1.28 |
@Mergifyio rebase |
Issue: The RoundOffCephFSVolSize() function omits the fractional part when calculating the size for cephfs volumes, leading to the created volume capacity to be lesser than the requested volume capacity. Fix: Consider the fractional part during the size calculation so the rounded off volume size will be greater than or equal to the requested volume size. Signed-off-by: karthik-us <ksubrahm@redhat.com> Fixes: ceph#4179
Adding e2e test to check for successful PVC creation of 500MB. Signed-off-by: karthik-us <ksubrahm@redhat.com>
✅ Branch has been successfully rebased |
16b52da
to
0e1d5de
Compare
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 5ff0607 |
/test ci/centos/k8s-e2e-external-storage/1.26 |
/test ci/centos/upgrade-tests-cephfs |
/test ci/centos/mini-e2e-helm/k8s-1.26 |
/test ci/centos/k8s-e2e-external-storage/1.27 |
/test ci/centos/upgrade-tests-rbd |
/test ci/centos/k8s-e2e-external-storage/1.28 |
/test ci/centos/mini-e2e/k8s-1.26 |
/test ci/centos/mini-e2e-helm/k8s-1.27 |
/test ci/centos/mini-e2e-helm/k8s-1.28 |
/test ci/centos/mini-e2e/k8s-1.27 |
/test ci/centos/mini-e2e/k8s-1.28 |
/test ci/centos/mini-e2e-helm/k8s-1.28 |
This CI job seemed to be missing. Started it again to get a status report and have this PR merged. |
Issue:
The RoundOffCephFSVolSize() function omits the fractional part when calculating the size for cephfs volumes, leading to the created volume capacity to be lesser than the requested volume capacity.
Fix:
Consider the fractional part during the size calculation so the rounded off volume size will be greater than or equal to the requested volume size.
Fixes: #4179