Add some race-condition protection to VPA recommender #8320

adrianmoisey · 2025-07-13T19:33:28Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

#7992 introduces goroutines in order to process a batch of VPAs quicker. This inadvertently lead to some race conditions that we're picked up in our tests.
This PR attempts to fix those race conditions

Which issue(s) this PR fixes:

Fixes #8318

Special notes for your reviewer:

Opening this PR early, I still plan to write some tests. I'd like to make a breaking-test that these fixes solve.
/hold

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2025-07-13T19:33:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianmoisey

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:

~~vertical-pod-autoscaler/OWNERS~~ [adrianmoisey]

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

adrianmoisey · 2025-07-13T19:34:36Z

vertical-pod-autoscaler/docs/flags.md

@@ -125,7 +125,7 @@ This document is auto-generated from the flag definitions in the VPA recommender
 | `storage` | string |  | Specifies storage mode. Supported values: prometheus, checkpoint  |
 | `target-cpu-percentile` | float |  0.9 | CPU usage percentile that will be used as a base for CPU target recommendation. Doesn't affect CPU lower bound, CPU upper bound nor memory recommendations.  |
 | `target-memory-percentile` | float |  0.9 | Memory usage percentile that will be used as a base for memory target recommendation. Doesn't affect memory lower bound nor memory upper bound.  |
-| `update-worker-count` |  |  10 | kube-api-qps                       Number of concurrent workers to update VPA recommendations and checkpoints. When increasing this setting, make sure the client-side rate limits (kube-api-qps and `kube-api-burst`) are either increased or turned off as well. Determines the minimum number of VPA checkpoints written per recommender loop.  |
+| `update-worker-count` | int |  10 | Number of concurrent workers to update VPA recommendations and checkpoints. When increasing this setting, make sure the client-side rate limits ('kube-api-qps' and 'kube-api-burst') are either increased or turned off as well. Determines the minimum number of VPA checkpoints written per recommender loop.  |


I happen to notice this weirdness in the --update-worker-count due to the back-ticks, so I fixed them while I was here.

Reference: https://pkg.go.dev/flag#PrintDefaults

The listed type, here int, can be changed by placing a back-quoted name in the flag's usage string; the first such item in the message is taken to be a parameter name to show in the message and the back quotes are stripped from the message when displayed

adrianmoisey · 2025-07-14T19:10:18Z

vertical-pod-autoscaler/pkg/utils/metrics/recommender/recommender.go

+	oc.mutex.Lock()
 	oc.cnt[key]++
+	oc.mutex.Unlock()


I looked at changing this to use atomic.AddInt64().

It works, but my concern is that the map need to gaurentee that all keys exist in the map. This is possible by pre-filling the map with all possible combinations. However, we need to remember to update this file each time a new type is added. Ie:

autoscaler/vertical-pod-autoscaler/pkg/utils/metrics/recommender/recommender.go

Lines 41 to 46 in 9bc4220

modes = []string{

string(vpa_types.UpdateModeOff),

string(vpa_types.UpdateModeInitial),

string(vpa_types.UpdateModeRecreate),

string(vpa_types.UpdateModeAuto),

}

The problem is that there doesn't seem to be a good way to dynamically generate that list in the VPA.
What we may be able to do is write a generate script that will keep this file up to date, which we can run in CI.

I also looked at sync.Map. I ran a benchmark of the test I wrote and changing to a sync.Map doesn't seem to change the overall benchmark's speed much

atomic.AddInt64() should be faster right?
We can open a follow-up issue for this

Yup, it will be faster.
I can make a follow-up issue

At a glance, the way you have it now seems ok to me!

…ndition

Also move it to a private field, so that reads/writes are all via the protected methods

From https://pkg.go.dev/flag#PrintDefaults > The listed type, here int, can be changed by placing a back-quoted > name in the flag's usage string; the first such item in the message > is taken to be a parameter name to show in the message and the back > quotes are stripped from the message when displayed.

adrianmoisey · 2025-07-29T19:39:29Z

/unhold

I've added a test which seems to catch the race condition without the fixes

adrianmoisey · 2025-07-30T06:27:37Z

cc @raywainman @omerap12 @voelzmo

omerap12 · 2025-08-03T14:21:55Z

Looks good to me. I would love to hear others thoughts on this

raywainman · 2025-08-05T01:06:09Z

vertical-pod-autoscaler/pkg/recommender/model/aggregate_container_state.go

 	IsUnderVPA          bool
 	UpdateMode          *vpa_types.UpdateMode
 	ScalingMode         *vpa_types.ContainerScalingMode
 	ControlledResources *[]ResourceName
+
+	mutex sync.RWMutex
 }

 // GetLastRecommendation returns last recorded recommendation.


You could also mention thread safety here.

raywainman · 2025-08-05T01:11:12Z

vertical-pod-autoscaler/pkg/recommender/model/cluster.go

@@ -464,6 +468,8 @@ func (cluster *clusterState) getContributiveAggregateStateKeys(ctx context.Conte
 // keep track of empty recommendations and log information about them
 // periodically.
 func (cluster *clusterState) RecordRecommendation(vpa *Vpa, now time.Time) error {
+	cluster.mutex.Lock()


I think we also need read locks throughout this file as well don't we?

To make things easier we could consider using the sync.Map type for all the problematic maps to just have all that handled for us.

I think we also need read locks throughout this file as well don't we?

Doesn't seem so, the race condition happens in the map cluster.emptyVPAs. The is only accessed from this method and DeleteVpa().

I'll add a mutex to DeleteVpa() and leave a TODO to change to a sync.Map

raywainman · 2025-08-05T01:18:59Z

vertical-pod-autoscaler/pkg/recommender/routines/recommender_test.go

+	}
+	close(vpaUpdates)
+
+	wg.Wait()


would it make sense to validate the number of VPA updates processed here?

You could have an atomic counter in here just counting how many updates have been processed.

raywainman · 2025-08-05T01:23:28Z

vertical-pod-autoscaler/pkg/utils/metrics/recommender/recommender.go

+	oc.mutex.Lock()
 	oc.cnt[key]++
+	oc.mutex.Unlock()


At a glance, the way you have it now seems ok to me!

raywainman · 2025-08-05T01:23:49Z

vertical-pod-autoscaler/pkg/utils/metrics/recommender/recommender.go

 	oc.cnt[key]++
+	oc.mutex.Unlock()
 }

 // Observe passes all the computed bucket values to metrics


Do we need to lock down here when reading oc.cnt?

🤔 a read lock? I guess so.

raywainman · 2025-08-05T01:24:38Z

vertical-pod-autoscaler/pkg/recommender/model/aggregate_container_state.go

@@ -147,7 +159,7 @@ func (a *AggregateContainerState) GetControlledResources() []ResourceName {
 // a VPA object.
 func (a *AggregateContainerState) MarkNotAutoscaled() {
 	a.IsUnderVPA = false
-	a.LastRecommendation = nil
+	a.lastRecommendation = nil


Technically we should also protect this with the mutex as well since we are mutating the value.

And add a read lock when reading

adrianmoisey · 2025-08-09T09:09:24Z

cc @raywainman

raywainman · 2025-08-13T21:46:49Z

/lgtm

k8s-ci-robot requested review from kwiesmueller and raywainman July 13, 2025 19:33

k8s-ci-robot added the area/vertical-pod-autoscaler label Jul 13, 2025

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/needs-area labels Jul 13, 2025

adrianmoisey commented Jul 13, 2025

View reviewed changes

voelzmo mentioned this pull request Jul 14, 2025

VPA race conditions #8318

Closed

adrianmoisey commented Jul 14, 2025

View reviewed changes

adrianmoisey added 4 commits July 27, 2025 22:08

Protect clusterstate.getContributiveAggregateStateKeys() from race co…

b2df71a

…ndition

Protect ObjectCounter.Add() from race condition

c7f0511

Protect AggregateContainerState.LastRecommendation from race condition

5aab4b6

Also move it to a private field, so that reads/writes are all via the protected methods

adrianmoisey force-pushed the fix-race branch from 4068a07 to 18d9971 Compare July 29, 2025 19:38

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 29, 2025

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

Add a test to detect race conditions with processVPAUpdate()

fc3e97e

adrianmoisey force-pushed the fix-race branch from 18d9971 to fc3e97e Compare July 29, 2025 19:47

raywainman reviewed Aug 5, 2025

View reviewed changes

Add a counter to the test to ensure the correct amount of jobs ran

1943ec9

adrianmoisey added 4 commits August 6, 2025 08:58

Update comment to include thread safety

bdec0f8

Use thread-safe method

f5ce3b4

Add mutex to cluster.emptyVPAs

f7c435e

Switch to RWMutex

44e0f63

And add a read lock when reading

k8s-ci-robot assigned raywainman Aug 13, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2025

k8s-ci-robot merged commit 13dde43 into kubernetes:master Aug 13, 2025
7 checks passed

	modes = []string{
	string(vpa_types.UpdateModeOff),
	string(vpa_types.UpdateModeInitial),
	string(vpa_types.UpdateModeRecreate),
	string(vpa_types.UpdateModeAuto),
	}

Add some race-condition protection to VPA recommender #8320

Add some race-condition protection to VPA recommender #8320

Uh oh!

Conversation

adrianmoisey commented Jul 13, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

k8s-ci-robot commented Jul 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianmoisey commented Jul 29, 2025

Uh oh!

adrianmoisey commented Jul 30, 2025

Uh oh!

omerap12 commented Aug 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrianmoisey commented Aug 9, 2025

Uh oh!

raywainman commented Aug 13, 2025

Uh oh!

Uh oh!

Uh oh!