[FG:InPlacePodVerticalScaling] Move resize allocation logic out of the sync loop #131612

natasha41575 · 2025-05-05T16:33:55Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This moves the in-place pod resize allocation logic out of the sync loop. This PR is organized into the following 5 commits:

Untangle the HandlePodResourcesResize unit tests and move them into the allocation package
Add helper methods IsPodResizeInfeasible and IsPodResizeDeferred to the status_manager.
Update the allocation_manager methods to hold all the control logic required for handling pod resize allocation + update the kubelet to no longer attempt to allocate pod resizes in the sync loop (and update unit tests accordingly).
Update the allocation manager's unit tests to cover PushPendingResizes and RetryPendingResizes
Skip pending resize evaluation if sources aren't ready (per discussion on the previous PR)

The intention of this PR is to reattempt pending resizes:

whenever HandlePodAdditions or HandlePodUpdates receives a resize request that it didn't already have,
upon deletion of another pod,
upon the successful actuation of another resize,
or periodically. This PR sets a timer for every 3 minutes, but we should probably think about if that is the right amount of time.

Special notes for your reviewer

Intended follow-ups:

This PR is required for but does not include implementation of prioritized resizes. That is because the PR was already getting a bit too large to review, and because design for prioritized resizes is still pending (KEP-1287: Priority of Resize Requests enhancements#5266). This is also useful as its own standalone change without having prioritized resizes yet, but I left a TODO for that.
Some cleanup (such as moving some unit tests around, unexporting functions that no longer need to be exported, removing some code that's not needed anymore etc), I left some of these things out of this PR to keep the size down

Which issue(s) this PR fixes:

Does not yet fix it, but this is part of #116971.

Does this PR introduce a user-facing change?

NONE

/sig node
/priority important-soon
/triage accepted
/cc @tallclair

TODO:

~~retry deferred resizes in HandlePodCleanups~~
- I don't think anything in HandlePodCleanups affects the admission decision (but I could be wrong)? It looks like the admission decision depends on the pod manager as the source of truth (through kl.podManager.GetPods), and the pod manager is not updated in HandlePodCleanups, so I don't think retrying the pending resizes here is necessary
double check the logic in HandlePodAdditions and HandlePodUpdates is correct (maybe add unit tests covering resize cases)
allocation manager unit tests
need to fix an issue where even when the resize is deferred and not allocated or actuated, the pod status is showing updated allocated and actual resources
need to fix an issue where a pending resize that gets reverted does not have its pending condition cleared quickly enough
sanity check with running this e2e locally
there seems to be more latency than should be necessary in accepting a pending resize after another pod is scaled down to make room, want to investigate this (but this doesn't necessarily have to be blocking)
skip retry of pending resizes if sources aren't ready (!kl.sourcesReady.AllReady())
rebase on move pod admission and resize logic into the allocation manager #131801

k8s-ci-robot · 2025-05-05T16:33:58Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

pkg/kubelet/allocation/allocation_manager.go

pkg/kubelet/kubelet.go

k8s-ci-robot · 2025-06-02T22:23:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: natasha41575
Once this PR has been reviewed and has the lgtm label, please ask for approval from tallclair. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/kubelet/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

natasha41575 · 2025-06-02T22:40:45Z

@tallclair this is rebased and I added skipping the pending resize evaluation when the sources are not ready

there's one more thing I want to check - it seems like there's still a little extra unnecessary latency after another pod is sized down before the space is considered "free" so that a pending resize can succeed, so I'm going to try to look into that, but other than that PR is ready for your review

natasha41575 · 2025-06-05T18:27:19Z

it seems like there's still a little extra unnecessary latency after another pod is sized down before the space is considered "free" so that a pending resize can succeed,

this is fixed now, it was just because of something dumb I did

…ng resizes

k8s-ci-robot · 2025-06-20T17:22:10Z

@natasha41575: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-unit-windows-master	`91a33e4`	link	false	`/test pull-kubernetes-unit-windows-master`

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.

tallclair

Finished reviewing, please include the relevant comments from #132342 (review) too.

tallclair · 2025-06-25T16:58:20Z

pkg/kubelet/allocation/allocation_manager.go

+	ticker.Reset(retryPeriod)
+
+	if !m.sourcesReady.AllReady() {
+		klog.V(4).InfoS("Skipping evaluation of pending resizes; sources are not ready")


If there's an empty source, it will send an empty update to mark the source as seen. Will that trigger a retry here? Otherwise, we might want to use a shorter retryPeriod until all sources are ready, otherwise there's a risk of a resize getting stuck waiting the full 3 minutes (although pretty unlikely).

tallclair · 2025-06-25T17:03:49Z

pkg/kubelet/kubelet.go

+			} else {
+				// We can hit this case if a pending resize has been reverted,
+				// so we need to clear the pending resize condition.
+				kl.allocationManager.ClearPodResizePendingCondition(pod.UID)


Should kubelet just call status manager directly here, rather than plumbing this through allocation manager? Alternatively, it might make more sense to call this "RemovePendingResize", which removes it from the queue and clears the pending condition (that's not necessary though)

tallclair · 2025-06-25T17:45:02Z

pkg/kubelet/status/status_manager.go

+	m.podStatusesLock.Lock()
+	defer m.podStatusesLock.Unlock()


nit: Rlock/RUnlock (same below)

Suggested change

m.podStatusesLock.Lock()

defer m.podStatusesLock.Unlock()

m.podStatusesLock.RLock()

defer m.podStatusesLock.RUnlock()

tallclair · 2025-06-25T17:49:10Z

pkg/kubelet/kubelet.go

@@ -2655,6 +2671,14 @@ func (kl *Kubelet) HandlePodAdditions(pods []*v1.Pod) {
 	for _, update := range updates {


The comment on the previous line no longer applies. I think you can just call UpdatePod directly in the previous loop now, since SyncPod doesn't resolve resizes anymore. Does that look correct?

tallclair · 2025-06-25T17:52:38Z

pkg/kubelet/kubelet.go

+				// existing pods are added.
+				allocatedPod, updatedFromAllocation := kl.allocationManager.UpdatePodFromAllocation(pod)
+				if updatedFromAllocation {
+					pod = allocatedPod


nit: I think this is unnecessary since UpdatePod handles it

tallclair · 2025-06-25T18:44:21Z

pkg/kubelet/kubelet.go

+
+		if utilfeature.DefaultFeatureGate.Enabled(features.InPlacePodVerticalScaling) {
+			if resizeRequest {
+				kl.allocationManager.RetryPendingResizes()


Maybe this should be moved to before the UpdatePod call? If the resize can be allocated immediately, it would be better to sync the pod with the resize first.

Ideally we'd avoid the double call to UpdatePod if the resize is immediately actuated, but I don't see a good way to do that right now. Maybe leave a TODO?

k8s-ci-robot · 2025-06-26T02:57:48Z

@shiya0705: GitHub didn't allow me to request PR reviews from the following users: shiya0705.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc

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.

k8s-ci-robot requested a review from tallclair May 5, 2025 16:33

github-project-automation bot added this to SIG Node: code and documentation PRs May 5, 2025

github-project-automation bot moved this to Triage in SIG Node: code and documentation PRs May 5, 2025

k8s-ci-robot added area/kubelet area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels May 5, 2025

github-project-automation bot added this to SIG Node CI/Test Board May 5, 2025

github-project-automation bot moved this to Triage in SIG Node CI/Test Board May 5, 2025

natasha41575 changed the title ~~Move resize logic~~ [FG:InPlacePodVerticalScaling] Move resize allocation logic out of the sync loop May 5, 2025

github-project-automation bot added this to SIG Node: In Place Pod Vertical Scaling May 5, 2025

natasha41575 moved this from Triage to Work in progress in SIG Node: code and documentation PRs May 5, 2025

natasha41575 moved this from Triage to Archive-it in SIG Node CI/Test Board May 5, 2025

natasha41575 force-pushed the move-resize-logic branch 3 times, most recently from 433aa61 to d52210d Compare May 6, 2025 21:19

natasha41575 mentioned this pull request May 7, 2025

[FG:InPlacePodVerticalScaling] Metrics #131648

Open

natasha41575 force-pushed the move-resize-logic branch 2 times, most recently from 9b16320 to 42ececf Compare May 7, 2025 20:33

natasha41575 commented May 7, 2025

View reviewed changes

pkg/kubelet/allocation/allocation_manager.go Outdated Show resolved Hide resolved

pkg/kubelet/kubelet.go Outdated Show resolved Hide resolved

natasha41575 marked this pull request as ready for review May 7, 2025 21:30

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 7, 2025

SergeyKanzhelev moved this from Needs Approver to Needs Reviewer in SIG Node: code and documentation PRs May 31, 2025

natasha41575 force-pushed the move-resize-logic branch from 501626e to 3eae312 Compare June 2, 2025 22:23

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2025

natasha41575 requested a review from tallclair June 2, 2025 22:37

natasha41575 mentioned this pull request Jun 4, 2025

Remove unecessary comment since podResizeMutex no longer exist. #132095

Merged

natasha41575 added 2 commits June 5, 2025 17:07

move handlePodResourcesResize unit tests into allocation_manager_test

019a8d1

add some additional helpers to kubelet status manager

6b38ee1

natasha41575 force-pushed the move-resize-logic branch 3 times, most recently from 6605d04 to 33db8ea Compare June 5, 2025 18:55

This was referenced Jun 6, 2025

[FG:InPlacePodVerticalScaling] Add Pod resize complete event #130387

Open

[FG:InPlacePodVerticalScaling] Add a more complex e2e test for deferred resizes #132152

Open

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2025

natasha41575 added a commit to natasha41575/kubernetes that referenced this pull request Jun 16, 2025

move resize allocation out of the sync loop (kubernetes#131612)

1b7e02b

natasha41575 mentioned this pull request Jun 16, 2025

[FG:InPlacePodVerticalScaling] Prioritize resize requests by priorityClass and qos class #132342

Open

natasha41575 force-pushed the move-resize-logic branch from 33db8ea to fdee11e Compare June 20, 2025 16:47

natasha41575 added 3 commits June 20, 2025 16:54

move resize allocation out of the sync loop

e0024c2

update allocation manager unit tests to cover push and retry of pendi…

3abcd01

…ng resizes

skip pending resize evaluation if sources aren't ready

91a33e4

natasha41575 force-pushed the move-resize-logic branch from fdee11e to 91a33e4 Compare June 20, 2025 16:54

natasha41575 added a commit to natasha41575/kubernetes that referenced this pull request Jun 20, 2025

move resize allocation out of the sync loop (kubernetes#131612)

bf73c8e

natasha41575 added a commit to natasha41575/kubernetes that referenced this pull request Jun 20, 2025

move resize allocation out of the sync loop (kubernetes#131612)

96a9bb1

natasha41575 mentioned this pull request Jun 20, 2025

Adding Alpha criteria for KEP to support IPPR & Pod Level Resources kubernetes/enhancements#5423

Merged

tallclair reviewed Jun 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[FG:InPlacePodVerticalScaling] Move resize allocation logic out of the sync loop #131612

[FG:InPlacePodVerticalScaling] Move resize allocation logic out of the sync loop #131612

Uh oh!

natasha41575 commented May 5, 2025 •

edited

Loading

Uh oh!

k8s-ci-robot commented May 5, 2025

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 2, 2025

Uh oh!

natasha41575 commented Jun 2, 2025

Uh oh!

natasha41575 commented Jun 5, 2025

Uh oh!

k8s-ci-robot commented Jun 20, 2025

Uh oh!

tallclair left a comment

Uh oh!

tallclair Jun 25, 2025

Uh oh!

tallclair Jun 25, 2025

Uh oh!

tallclair Jun 25, 2025

Uh oh!

tallclair Jun 25, 2025

Uh oh!

tallclair Jun 25, 2025

Uh oh!

tallclair Jun 25, 2025

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

Uh oh!

		@@ -2655,6 +2671,14 @@ func (kl Kubelet) HandlePodAdditions(pods []v1.Pod) {
		for _, update := range updates {

[FG:InPlacePodVerticalScaling] Move resize allocation logic out of the sync loop #131612

Are you sure you want to change the base?

[FG:InPlacePodVerticalScaling] Move resize allocation logic out of the sync loop #131612

Uh oh!

Conversation

natasha41575 commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Uh oh!

k8s-ci-robot commented May 5, 2025

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 2, 2025

Uh oh!

natasha41575 commented Jun 2, 2025

Uh oh!

natasha41575 commented Jun 5, 2025

Uh oh!

k8s-ci-robot commented Jun 20, 2025

Uh oh!

tallclair left a comment

Choose a reason for hiding this comment

Uh oh!

tallclair Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

tallclair Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

tallclair Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

tallclair Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

tallclair Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

tallclair Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

Uh oh!

natasha41575 commented May 5, 2025 •

edited

Loading