Avoid overflowing int64 in RoundUpSize and return error if overflow int #66464
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
sig/storage
|
@wongma7: GitHub didn't allow me to request PR reviews from the following users: if, you, are, curious. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In 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/test-infra repository. |
pkg/volume/util/util.go
Outdated
| @@ -376,6 +380,32 @@ func RoundUpToGiB(size resource.Quantity) int64 { | |||
| return RoundUpSize(requestBytes, GIB) | |||
| } | |||
|
|
|||
| // RoundUpSizeInt calculates how many allocation units are needed to accommodate | |||
| // a volume of given size. It returns an int instead of an int64 and true if | |||
There was a problem hiding this comment.
says 'return true', i will fix the comment on monday
|
@wongma7 Thanks for the fix!! Yeah, this overflow was not handled and its causing issues. |
|
lgtm, waiting for the "return true" comment to be updated to make it official. (I particularly appreciate the attention to detail around GB vs GiB) |
|
@wongma7, thanks for working on this! |
|
comments updated, thanks for the quick review @anguslees |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anguslees, gnufied, saad-ali, wongma7 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 |
k8s-ci-robot
added
the
approved
|
Automatic merge from submit-queue (batch tested with PRs 66464, 66488). If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
There are many places in plugins (some I may have missed) that we naively convert a resource.Quantity.Value() which is an int64, to an int, which may be only 32 bits long.
Background, optional to read :): Kubernetes canonicalizes resource.Quantities, and from what I have seen testing creating PVCs, decimalSI is the default. If a quantity is in
decimalSIformat and its value in bytes would overflow an int64, e.g.10E, nothing happens. If it is in binarySI and its value in bytes would overflow an int64, e.g.10Ei, it is set down to 2^63-1 and there's no overflow of the field value. But there may be overflow later in the code which is what this PR is addressing.RoundUpSizeimplementation to avoid overflowingint64RoundUp*Intfunctions for use when anintis expected instead of anint64, becauseintmay be 32bits and naively doingint($INT64_VALUE)can lead to silent overflow. These functions return an error if overflow has occurred.*GBvariables to*GiBwhere appropriate for maximum clarityRoundUpToGiBinstead ofRoundUpSizewhere possibleWhich issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer: please review carefully as we don't have e2e tests for most plugins!
Release note:
edit: remove 'we do not need to worry about...'. yes we do, i worded that badly :))