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
Fix Vsphere rouding up of volume size #76719
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.
@kubernetes/cloud-provider-vsphere-maintainers
718c050
to
d98db30
Compare
CC @saad-ali |
|
||
By("Creating Storage Class") | ||
storageclass, err := client.StorageV1().StorageClasses().Create(getVSphereStorageClassSpec(DiskSizeSCName, scParameters, nil)) | ||
framework.ExpectNoError(err) |
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.
Error validation in this package is done in many different ways, example:
Expect(err).To(HaveOccurred())
framework.ExpectNoError(err, fmt.Sprintf("Failed to create storage class with err: %v", err))
framework.ExpectNoError(err)
For consistency's sake, in this patch I only used framework.ExpectNoError(err)
.
@@ -35,13 +34,12 @@ const ( | |||
) | |||
|
|||
/* | |||
Test to verify disk size specified in PVC is being honored while volume creation. | |||
Test to verify disk size specified in PVC is being rounded up correctly. |
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.
It seems we don't run these tests at all, however, I updated this one to make sure it works.
looks good to me. But I want @divyenpatel @BaluDontu to chime in as well. /approve |
/assign @divyenpatel @BaluDontu |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto, gnufied The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm /hold (holding until somebody from the storage team reviews) |
@frapposelli since I have already reviewed from storage team we can remove the hold. Or did you mean someone from vmware storage team? |
|
/hold cancel |
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.
/lgtm
What type of PR is this?
/kind bug
What this PR does / why we need it:
Storage plugins treat numbers-only storage size as bytes, however, they round it up to the nearest allocation unit, which is vendor-dependant. The Vsphere plugin, on the other hand, rounds the value up to at least 1 KiB and dispatches the request to the server. Since Vsphere's minimum allocation size is 1 MiB, the request returns an error.
This PR changes the Vsphere plugin so it acts like the other storage plugins. Even though the rounding might not be ideal (doing something different from what was requested by the user), we should have consistency across the the storage plugins.
/sig storage