-
Notifications
You must be signed in to change notification settings - Fork 41.6k
[PodLevelResources] Propagate Pod level hugepage cgroup to containers #131089
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
[PodLevelResources] Propagate Pod level hugepage cgroup to containers #131089
Conversation
|
Hi @KevinTMtz. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
/assign @ndixita |
b48a3ed to
3ea8028
Compare
3ea8028 to
ec198a6
Compare
| } | ||
|
|
||
| // We do not default pod-level hugepage limits if there is a hugepage request. | ||
| if _, exists := pod.Spec.Resources.Requests[key]; exists { |
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.
Ok, this looks good, make sure we have the following test scenarios covered:
-
PodSpec in a Deployment with container hugepages limit and no request
- I think this doesn't get defaulted (e.g. in a Deployment), and passes validation by skipping the
limit && requestequality check on podspec - the eventual pod would default container hugepages request, and still pass validation
- make sure pod-level resources also avoids defaulting hugepages, and validation is happy with this scenario both for podspec and pod
- I think this doesn't get defaulted (e.g. in a Deployment), and passes validation by skipping the
-
Pod with a pod-level hugepages request and no limit errors as expected
-
Pod with a pod-level hugepages request and limit works as expected
|
Defaulting change looks good, this is on a field that was alpha-gated in 1.33. The change makes it clearer that someone explicitly setting pod-level hugepages requests must set the corresponding pod-level limit themselves. Marked as API approved, will tag once the test coverage is verified and the clarification at #131089 (comment) is updated |
The hugepage aggregated container limits cannot be greater than pod-level limits. This was already enforced with the defaulted requests from the specfied limits, however it did not make it clear about both hugepage requests and limits.
Pod level hugepage resources are not propagated to the containers, only pod level cgroup values are propagated to the containers when they do not specify hugepage resources.
5e7693f to
1bc995c
Compare
Currently there are no E2E tests that focus the PodSpec, current E2E tests focus on pods. Would it be possible to add those in a follow up PR? Deployments undergo the same resource validation as pod, so we would actually be testing the same cases, however only the ones that are valid without the defaulting. In regards to unit test, the newly added
Unit and E2E tests added in #130577 together with the new tests added in this PR cover this functionality.
Unit and E2E tests added in #130577 together with the new tests added in this PR cover this functionality. |
|
/lgtm |
|
LGTM label has been added. Git tree hash: 6922a6382899bc3073cb388b5738aeef21eb5eec
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KevinTMtz, liggitt, tallclair 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 |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/test pull-kubernetes-e2e-gce |
|
@KevinTMtz: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
/retest |
What type of PR is this?
What this PR does / why we need it:
Follow up of [PodLevelResources] Pod Level Hugepage Resources.
This PR propagates Pod level hugepage cgroup to containers with the following changes:
Additionally adds:
Which issue(s) this PR fixes:
Fixes #132543
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: