-
Notifications
You must be signed in to change notification settings - Fork 40.9k
fix: improve the pod level request validation #132551
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
base: master
Are you sure you want to change the base?
Conversation
The pod level request should be larger than the aggregated container requests. The fix is to skip those resources not supported at the pod level for better efficiency. A minor unit test is also added.
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
Welcome @chao-liang! |
Hi @chao-liang. 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. |
/ok-to-test |
/test all |
/lgtm |
LGTM label has been added. Git tree hash: 344a843947b6c47666e270abf50a7023f07c06c2
|
/assign @deads2k |
/test pull-kubernetes-node-kubelet-serial-containerd-alpha-features |
/test pull-kubernetes-node-kubelet-serial-podresources |
/hold While I concede that the current behavior is unexpected, we cannot simply change this here because in a downgrade case, you end up in a state where a flow like this is possible
This could be ratcheted in via options. See how we create PodValidationOptions in the strategy by inspecting the state of the old object. In one release we could add tolerance for this state if it already exists and in a future release we could allow it during create. It will also require some tests at that level to ensure we get the downgrade case correct. |
since this validation is only exercised when the field is populated, and it is an alpha field which cannot be populated by default yet, I think this is still fair game to tighten without worrying about ratcheting (once it reaches beta+, I agree we'd have to ratchet validation) |
New changes are detected. LGTM label has been removed. |
+1 Agree with this. I understand how this can affect downgrade scenario, but since it is an alpha feature/field ratcheting might not be needed. |
Thanks for reviewing. I've synced with dixita offline, and pushed another commit to align with the current way of limit check. |
Could you please squash both commits |
/label tide/merge-method-squash |
/retest |
I missed the alpha status. I agree we don't need to worry about downgrade on alpha. Thanks for the update to cover the case where not all eligible resources are specified. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chao-liang, deads2k 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 |
/remove-kind cleanup Since this is a user-facing change, I think it would be good to include a release note. Something like this:
And also, as noted in the PR template, could you add a link to the KEP in the following section? #### Which issue(s) this PR is related to:
Fixes #132449
KEP: https://github.com/kubernetes/enhancements/issues/2837 |
@toVersus: changing LGTM is restricted to collaborators 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-sigs/prow repository. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The pod level request should be larger than the aggregated container requests.
Which issue(s) this PR is related to:
Fixes #132449
Special notes for your reviewer:
The issue was actually addressed by #130131
The fix here is an improvement for efficiency.
Does this PR introduce a user-facing change?
NONE
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: