[client-go #1415] Use transformer from provided store within internal stores in reflector to limit memory usage bursts #131799

valerian-roche · 2025-05-15T19:20:19Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR updates client-go. It ensures the reflector used in SharedInformers does not keep a full list of objects retrieved through watchList without transformation prior to passing the objects one at a time to the underlying Store (which may have a transformer defined).
Without this change, the informer ends up holding a full copy of unmutated objects in memory prior to being passed to the transformer, greatly limiting its value as a way to avoid large memory allocations. In our example (referring to the issue), we gain more than one OOM in the memory we need to provide a controller running a pod informer.

To not change the semantic of watchList (especially on not altering the backing store if the watch does not conclude), the code still uses a temporary store but applies the same transformer as the provided Store if applicable. This has the side effect of potentially running the transformer twice, but this side effect is already specifically mentioned within the TransformFunc comment, and users will not be impacted if not explicitly activating the WatchList feature flag in the client

Which issue(s) this PR fixes:

Fixes kubernetes/client-go#1415

Special notes for your reviewer:

Does this PR introduce a user-facing change?

 - Ensure objects are transformed prior to storage in SharedInformers if a transformer is provided and `WatchList` is activated

linux-foundation-easycla · 2025-05-15T19:20:24Z

The committers listed above are authorized under a signed CLA.

✅ login: valerian-roche / name: Valerian Roche (585ed0a)

k8s-ci-robot · 2025-05-15T19:20:29Z

Welcome @valerian-roche!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-05-15T19:20:30Z

Hi @valerian-roche. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

jpbetz · 2025-05-15T20:05:53Z

/assign
/triage accepted

aojea · 2025-05-18T16:16:51Z

/assign @wojtek-t @deads2k

wojtek-t · 2025-05-21T09:51:30Z

Let's wait for the final decision what we will do with watch-list feature.
Discussion is happening on https://docs.google.com/document/d/1x30DjXSSF5krpyoTCwManJg6vphpnGI37xfCRaiHAbs/edit?resourcekey=0-TluZ5mrM8l9HvkmzyKVfJQ&tab=t.0#heading=h.49d8j8iwvgue

Once we have the decision, I will review this PR.

wojtek-t · 2025-06-25T08:56:23Z

Given we're progressing with the feature (although with slightly adjusted scope), let me review this.

/ok-to-test

wojtek-t

Few minor comments, but overall this lgtm

staging/src/k8s.io/client-go/tools/cache/reflector.go

wojtek-t · 2025-06-25T08:59:40Z

staging/src/k8s.io/client-go/tools/cache/store_test.go

@@ -143,6 +144,21 @@ func TestCache(t *testing.T) {
 	doTestStore(t, NewStore(testStoreKeyFunc))
 }

+func TestCacheWithTransformer(t *testing.T) {
+	informerCalled := &atomic.Bool{}


please call it transformerCalled instead

Updated to transformerCalled

wojtek-t · 2025-06-25T09:06:43Z

staging/src/k8s.io/client-go/tools/cache/reflector_test.go

+			return fw, nil
+		},
+		// ListFunc should never be used in WatchList mode
+		ListFunc: nil,


It can be used as fallback in some cases.

So to avoid panics, let's actually define a function and just call t.Error() there

Updated to fail the test and return an error

Had to update to not fail here as this triggers a panic when the reflector shuts down (as some internal goroutine keeps working passed returning and calling this function)
Confirmed that returning an error here is sufficient to have the test failing in this case

wojtek-t · 2025-06-25T09:07:28Z

staging/src/k8s.io/client-go/tools/cache/reflector_test.go

+	}
+
+	r := NewReflector(lw, &v1.Pod{}, store, 0)
+	r.UseWatchList = ptr.To(true)


It's being removed in #132453

Updated to use the featureflag override instead

wojtek-t · 2025-06-25T09:09:10Z

staging/src/k8s.io/client-go/tools/cache/store.go

@@ -161,6 +161,8 @@ type cache struct {
 	// keyFunc is used to make the key for objects stored in and retrieved from items, and
 	// should be deterministic.
 	keyFunc KeyFunc
+	// Called with every object if non-nil.


nit: transformer is called for every object that is put into store.

aojea · 2025-06-25T09:19:35Z

staging/src/k8s.io/client-go/tools/cache/delta_fifo.go

@@ -303,6 +303,11 @@ func (f *DeltaFIFO) KeyOf(obj interface{}) (string, error) {
 	return f.keyFunc(obj)
 }

+// Transformer implements the TransformingStore interface.
+func (f *DeltaFIFO) Transformer() TransformFunc {


do we need to update the RealFIFO too? staging/src/k8s.io/client-go/tools/cache/the_real_fifo.go

Good catch - we would regress again after promoting InOrderInformers again..

Thanks for raising this part. I added this interface on it

wojtek-t · 2025-06-25T15:00:22Z

staging/src/k8s.io/client-go/tools/cache/reflector_test.go

+
+	clientfeaturestesting.SetFeatureDuringTest(t, clientfeatures.WatchListClient, true)
+	r := NewReflector(lw, &v1.Pod{}, store, 0)
+	// r.useWatchList = true


nit: please remove this

Sorry missed this. Working on ensuring it does not panic when not set

wojtek-t · 2025-06-25T15:02:24Z

staging/src/k8s.io/client-go/tools/cache/reflector_test.go

+	doneCh, stopCh := make(chan struct{}), make(chan struct{})
+	go func() {
+		defer close(doneCh)
+		//nolint:logcheck // Intentionally uses the old API.


Let's not do that. You can achieve everything with you need with context too - sth like:

ctx, cancel := context.WithCancel(context.Background()) go func() { defer close(doneCh) r.RunWithContext(ctx) }() ... cancel() select { ...

Updated to use the new API. Missed it when copying another test

valerian-roche · 2025-06-25T17:03:52Z

staging/src/k8s.io/client-go/tools/cache/store.go

 // NewStore returns a Store implemented simply with a map and a lock.
-func NewStore(keyFunc KeyFunc) Store {
-	return &cache{
+func NewStore(keyFunc KeyFunc, opts ...StoreOption) Store {


I might be wrong, but I do expect this API change to have no impact as it's a free standing function that should not be implementing an interface.
It's failing the API check though, so I can add a release note if desired.

Yeah - I think this is fine.
@aojea - can you confirm as a second pair of eyes?

wojtek-t · 2025-06-26T08:46:49Z

/lgtm
/approve

/hold
Waiting for @aojea to confirm

k8s-ci-robot · 2025-06-26T08:46:56Z

LGTM label has been added.

Git tree hash: 2f7353f9dfe0e541715a8cce1f09370ad04f4107

k8s-ci-robot · 2025-06-26T08:47:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: valerian-roche, wojtek-t

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:

~~staging/src/k8s.io/client-go/tools/cache/OWNERS~~ [wojtek-t]

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

wojtek-t · 2025-06-26T08:48:18Z

@valerian-roche - this label "do-not-merge/contains-merge-commits" that was automatically added suggests that you need to somehow rebase this PR to unblock it maybe?

valerian-roche · 2025-06-26T19:01:11Z

@valerian-roche - this label "do-not-merge/contains-merge-commits" that was automatically added suggests that you need to somehow rebase this PR to unblock it maybe?

Yes I will rebase and squash the PR as it's been fully reviewed now. Wanted to avoid confusion during the review

aojea · 2025-06-26T21:10:18Z

waiting for the rebase

…n internal stores in reflector to limit memory usage bursts Signed-off-by: Valerian Roche <[email protected]>

valerian-roche · 2025-06-27T02:03:01Z

waiting for the rebase

Force-pushed the squash and rebase

k8s-ci-robot · 2025-06-27T02:05:13Z

@valerian-roche: 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-apidiff-client-go	`585ed0a`	link	false	`/test pull-kubernetes-apidiff-client-go`

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.

aojea · 2025-06-30T14:30:02Z

/hold cancel

/lgtm
Thanks

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 15, 2025

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 15, 2025

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 15, 2025

k8s-ci-robot requested review from liggitt and soltysh May 15, 2025 19:20

k8s-ci-robot assigned jpbetz May 15, 2025

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 15, 2025

k8s-ci-robot assigned deads2k and wojtek-t May 18, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 25, 2025

wojtek-t reviewed Jun 25, 2025

View reviewed changes

aojea reviewed Jun 25, 2025

View reviewed changes

k8s-ci-robot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jun 25, 2025

wojtek-t reviewed Jun 25, 2025

View reviewed changes

valerian-roche commented Jun 25, 2025

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 26, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2025

[client-go kubernetes#1415] Use transformer from provided store withi…

585ed0a

…n internal stores in reflector to limit memory usage bursts Signed-off-by: Valerian Roche <[email protected]>

valerian-roche force-pushed the client-go-reflector-transformer branch from 1bcf358 to 585ed0a Compare June 27, 2025 02:02

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jun 27, 2025

valerian-roche requested review from aojea and wojtek-t June 30, 2025 13:31

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 30, 2025

k8s-ci-robot assigned aojea Jun 30, 2025

hashim21223445 mentioned this pull request Jun 30, 2025

[FG:InPlacePodVerticalScaling] Enable removing resource limits from containers #127143

Open

k8s-ci-robot merged commit 3f0a1c6 into kubernetes:master Jun 30, 2025
13 of 14 checks passed

k8s-ci-robot added this to the v1.34 milestone Jun 30, 2025

[client-go #1415] Use transformer from provided store within internal stores in reflector to limit memory usage bursts #131799

[client-go #1415] Use transformer from provided store within internal stores in reflector to limit memory usage bursts #131799

Conversation

valerian-roche commented May 15, 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:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

linux-foundation-easycla bot commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented May 15, 2025

Uh oh!

k8s-ci-robot commented May 15, 2025

Uh oh!

jpbetz commented May 15, 2025

Uh oh!

aojea commented May 18, 2025

Uh oh!

wojtek-t commented May 21, 2025

Uh oh!

wojtek-t commented Jun 25, 2025

Uh oh!

wojtek-t left a comment

Choose a reason for hiding this comment

Uh oh!

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!

aojea Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

valerian-roche Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojtek-t commented Jun 26, 2025

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

k8s-ci-robot commented Jun 26, 2025

Uh oh!

wojtek-t commented Jun 26, 2025

Uh oh!

valerian-roche commented Jun 26, 2025

Uh oh!

aojea commented Jun 26, 2025

Uh oh!

valerian-roche commented May 15, 2025 •

edited

Loading

linux-foundation-easycla bot commented May 15, 2025 •

edited

Loading

aojea Jun 25, 2025 •

edited

Loading

valerian-roche Jun 25, 2025 •

edited

Loading