[FG:InPlacePodVerticalScaling] Rework handling of allocated resources#128269
[FG:InPlacePodVerticalScaling] Rework handling of allocated resources#128269k8s-ci-robot merged 8 commits intokubernetes:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
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 DetailsInstructions 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. |
|
Hello @tallclair |
|
Thanks for the review, @vinaykul ! |
yujuhong
left a comment
There was a problem hiding this comment.
Looks good overall. Have some minor questions
| } | ||
|
|
||
| if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) && isPodResizeInProgress(pod, &apiPodStatus) { | ||
| if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) && isPodResizeInProgress(pod, podStatus) { |
There was a problem hiding this comment.
I see. The comment definitely helped. Do we plan to augment the generic PLEG to detect changes beyond state transition, or is the plan to rely on EventedPLEG in the future?
| op := p.DeepCopy() | ||
| kl.updateContainerResourceAllocation(op) | ||
|
|
||
| op, _ := kl.statusManager.UpdatePodFromAllocation(p) |
There was a problem hiding this comment.
Enumerating the cases where the function is called is actually very helpful. Verifying my understanding below:
- (1) and (3). Called in
HandlePodAddition-- we look at the pod and its allocated resources (if it exists) and the desired spec (if it's considered new). - (2). Called in
canResizePod-- we look at the pod's updated desired spec
Does that sound right to you?
| return nil, err | ||
| } | ||
| } else { | ||
| // If resize isn't immediately feasible, proceed with the allocated pod. |
There was a problem hiding this comment.
My mental model (which might be wrong):
- If resizing is feasible, update the allocated resources of the pod
- If resizing is not feasible, the allocated resources stays the same
Either way, we'd be looking at "(updated) allocated resources" of the pod. I think the naming of the variables here might be my source of confusion....
Another question I have is that do we always set "allocated resources" even when there's no resize request? If not, doing that might help simplify some logic in the code.
| } | ||
|
|
||
| if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) && isPodResizeInProgress(pod, &apiPodStatus) { | ||
| if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) && isPodResizeInProgress(pod, podStatus) { |
There was a problem hiding this comment.
Add some way to indicate that a container is being "watched", and should be reinspected.
yeah marking it dirty and relying on the regular relisting seems to fit the existing design better (unless we want to go with (1)).
| updatedPod.Status.Resize = resizeStatus | ||
| } | ||
| kl.podManager.UpdatePod(updatedPod) | ||
| kl.statusManager.SetPodStatus(updatedPod, updatedPod.Status) |
There was a problem hiding this comment.
Why was this needed previously?
There was a problem hiding this comment.
Allocated resources were read from the pod status. Honestly it was very confusing, and there are a lot of bugs around inconsistent use of allocated vs desired. I don't fully understand what the implications of setting these were, but I'm confident they're no longer needed.
| return nil, err | ||
| } | ||
| } else { | ||
| // If resize isn't immediately feasible, proceed with the allocated pod. |
There was a problem hiding this comment.
The first part of the function gets the allocated pod, but if there's no resize (no update), it just returns early with that. Is that what you mean?
My bad. That was a more general question. When kubelet sees a pod for the first time with the desired spec (and no resize request yet), do we also set the allocated resources in the status?
(BTW, this question doesn't need to block the PR)
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 4a16f5ed9e1b45e13fc9f7c0ef9946fed80772f9 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tallclair, yujuhong The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
any ideas for #124308 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Improve the consistency of handling allocated vs. requested resources.
The previous implementation stored allocated resources in the pod status, but downstream consumers were inconsistent about when the spec resources vs allocated resources were used. Furthermore, it is often unclear from Kubelet code what version the
pod.Statusrepresents, and how up-to-date it is.This change handles the resize logic at the beginning of the pod sync loop, and overwrites the pod spec with the allocated resources as necessary. As a result, downstream consumers do not need to worry about which resources to pull from (spec vs allocated) for the most part.
Which issue(s) this PR fixes:
For #128065
Related to #116971
Special notes for your reviewer:
Does this PR introduce a user-facing change?
/sig node
/priority important-soon
/milestone v1.32