-
Notifications
You must be signed in to change notification settings - Fork 41.6k
[FG:InPlacePodVerticalScaling] Move pod resize status to pod conditions #130733
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
[FG:InPlacePodVerticalScaling] Move pod resize status to pod conditions #130733
Conversation
|
Skipping CI for Draft Pull Request. |
pkg/kubelet/kubelet.go
Outdated
| return false, v1.PodResizeStatusInfeasible, msg | ||
| kl.recorder.Eventf(pod, v1.EventTypeWarning, events.ResizeInfeasible, msg) | ||
| return false, &v1.PodCondition{ | ||
| Type: v1.PodResizePending, |
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.
was planning to add observedGeneration here after #130573 goes in (I can either update it in this PR or submit a follow up depending on which merges first)
|
/test pull-kubernetes-node-kubelet-podresize |
| } | ||
|
|
||
| if oldConditionsCount != newConditionsCount { | ||
| return false |
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.
If the ResizePending condition was removed and replaced with a ResizeInProgress condition, wouldn't this still get skipped? We might need to create a set of types for each new condition, and fail if any old condition types are missing (or if the lengths differ).
a0658cc to
df8bbbd
Compare
| } | ||
|
|
||
| if oldConditionsCount != newConditionsCount { | ||
| return false |
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.
No, the previous (already-existing) loop would catch that. In your example, it would see that ResizeInProgress is present on the new status, receive nil when it tries to fetch it from the old status, and subsequently return false.
Ah, makes sense. I missed that check.
actually I think your suggestion is (marginally) more efficient than what it does today actually, so I'll go ahead and make the change.
I'm not sure that's true, it's usually more efficient to search through a small slice than index it in a map. But let's not block on this...
df8bbbd to
a15520f
Compare
|
/lgtm |
|
LGTM label has been added. Git tree hash: dae813c08e16badb500a41ebc5489576e9ca845b
|
|
/test pull-kubernetes-node-kubelet-podresize |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: natasha41575, tallclair, thockin 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 |
What type of PR is this?
/kind feature
/kind deprecation
/kind api-change
What this PR does / why we need it:
This PR deprecates the pod
resizeStatusand exposes the status of a pod resize under pod conditions as described in https://kep.k8s.io/1287.Which issue(s) this PR fixes:
Fixes #128922
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: