Add request value verification for hugepage #98515
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
kind/api-change
|
/cc @bart0sh |
|
/test pull-kubernetes-node-e2e |
|
/test pull-kubernetes-e2e-gce-ubuntu-containerd |
|
@lala123912 Thank you for your contribution. As far as I remember Linux kernel supports huge pages of many different sizes depending on the hardware platform, e.g. 4K, 1M etc. What's the reason for limiting size to only two values? |
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
I have modified the code for different page sizes . the limitation can refer to https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1539-hugepages |
lala123912
force-pushed
the
huge_page
branch
3 times, most recently
from
90e17b8
to
2e483b6
Compare
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
It would be nice to have ValidateResourceRequirements API tested with hugepage requests and limits.
There was a problem hiding this comment.
I added two cases to test ValidateResourceRequirements in pkg/apis/core/validation/validation_test.go.
| if !helper.IsHugePageResourceValueValid(resourceName, quantity) { | ||
| limContainsHugePages = false | ||
| allErrs = append(allErrs, field.Invalid(fldPath, quantity.String(), | ||
| fmt.Sprintf("value must be integer multiple of %s limit", resourceName))) |
There was a problem hiding this comment.
if resourceName is "hugepages" the error would be "value must be integer multiple of hugepages limit". This is hard to understand from my point of view. Would it be better to somehow use quantity in the error message?
There was a problem hiding this comment.
I updated the error msg below.
fmt.Errorf("%s is not integer multiple of %s", quantity.String(), name)
test logs:
* spec.containers[0].resources.limits[hugepages-2Mi]: Invalid value: "5347737600m": 5347737600m is not integer multiple of hugepages-2Mi
* spec.containers[0].resources.requests[hugepages-2Mi]: Invalid value: "5347737600m": 5347737600m is not integer multiple of hugepages-2Mi
| if !helper.IsHugePageResourceValueValid(resourceName, quantity) { | ||
| reqContainsHugePages = false | ||
| allErrs = append(allErrs, field.Invalid(fldPath, quantity.String(), | ||
| fmt.Sprintf("value must be integer multiple of %s limit", resourceName))) |
There was a problem hiding this comment.
nit: I'd suggest to reorganize this code a little for better readability, e.g.:
if helper.IsHugePageResourceValueValid(resourceName, quantity) {
reqContainsHugePages = True
} else {
allErrs = append(allErrs, ...
}
There was a problem hiding this comment.
Another idea is to move this check into ValidateResourceQuantityValue as essentially it is a resource quantity value check. This would eliminate a need to do it twice in this place as ValidateResourceQuantityValue is already called for resources and limits here. wdyt?
There was a problem hiding this comment.
ValidateResourceQuantityValue is used for check various kinds of resource quantity value, such as node.Status.Capacity. Instead, I add a new function validateResourceQuantityHugePageValue so that the impact of the verification is smaller and IsHugePageResourceValueValid() is only called once here.
k8s-ci-robot
added
size/L
|
/test pull-kubernetes-integration |
|
/lgtm |
lala123912
force-pushed
the
huge_page
branch
2 times, most recently
from
8ca78e8
to
6bd1535
Compare
k8s-ci-robot
added
size/XL
|
/retest |
|
/retest |
lala123912
force-pushed
the
huge_page
branch
2 times, most recently
from
82ebb94
to
4d8ebba
Compare
|
/test pull-kubernetes-unit |
|
/retest |
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lala123912, liggitt 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
What type of PR is this?
/kind bug
What this PR does / why we need it:
Limit the request value of hugepage to integer multiple of page size to ensure the memory allocated is equal to the request.
Which issue(s) this PR fixes:
Fixes #98513
Special notes for your reviewer:
the limitation can refer to https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/1539-hugepages
The quantity of huge pages must be a positive amount of memory in bytes. The specified amount must align with the ; otherwise, the pod will fail validation. For example, it would be valid to request hugepages-2Mi: 4Mi, but invalid to request hugepages-2Mi: 3Mi.
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: