✨ Add UnsafeDisableDeepCopy into cache option to avoid deep copy during get/list #1274

FillZpp · 2020-11-25T06:51:37Z

Signed-off-by: Siyu Wang FillZpp.pub@gmail.com

Add UnsafeDisableCacheDeepCopy into cache option to avoid deep copy during list.

@vincepri @alvaroaleman I fully understand why the CacheReader calls DeepCopyObject for each object by default, the directly modification to underlying cache object is terrible. However, we can also provide an option for those people who really need to avoid deep copy.

k8s-ci-robot · 2020-11-25T06:51:45Z

Hi @FillZpp. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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/test-infra repository.

alvaroaleman · 2020-11-25T15:45:29Z

@FillZpp what is the usecase here that requires listing large numbers of objects and can not be covered via an Index as described e.G. here?

FillZpp · 2020-11-26T12:13:21Z

@FillZpp what is the usecase here that requires listing large numbers of objects and can not be covered via an Index as described e.G. here?

In Alibaba, we have lots of K8s clusters and some of them have even more than 200k Pods in a single cluster.
In such cluster, operators like OpenKruise call ListWatch for resources including Pods. So there may be multiple controllers and reconcile workers in one operator process that list their Pods simultaneously, which brings huge pressure for cpu/memory and sometimes leads to OOM.

The default DeepCopy to avoid cache pollution is good for those people who are unfamiliar with controller/informer, but also prejudicial to performance in large-scale clusters. We should provide an option for users that really need to disable this behavior. @alvaroaleman

alvaroaleman · 2020-11-26T14:19:37Z

In Alibaba, we have lots of K8s clusters and some of them have even more than 200k Pods in a single cluster.
In such cluster, operators like OpenKruise call ListWatch for resources including Pods.

This change wont reduce the memory usage of the ListWatch

So there may be multiple controllers and reconcile workers in one operator process that list their Pods simultaneously, which brings huge pressure for cpu/memory and sometimes leads to OOM.

I would expect those lists to filter by either using a LabelSelector or an Index

The default DeepCopy to avoid cache pollution is good for those people who are unfamiliar with controller/informer, but also prejudicial to performance in large-scale clusters. We should provide an option for users that really need to disable this behavior.

I disagree, prejudical to performance for large-scale clusters is it to always filter lists via labels or indexers. I wouldn't trust myself to always correctly do the deepcopy. so IMHO this is a huge footgun, even for ppl that are very familiar with controller-runtime.

FillZpp · 2020-11-27T09:00:34Z

would expect those lists to filter by either using a LabelSelector or an Index

Yes, those lists do have label or field selectors. Let me explain it.

Take OpenKruise for example, currently there are seven different controllers in OpenKruise and each of them have multiple workers. In these workload controllers, I take CloneSet and SidecarSet for example:

CloneSet is a workload for stateless application, just like Deployment/ReplicaSet. When reconcile a CloneSet, the worker should list all Pods belongs to this CloneSet using ownerReference field selector. For multiple workers, they may list their Pods simultaneously and usually there are thousands of Pods in a single CloneSet.
SidecarSet is a workload that injects sidecar into Pods during creating. Our mesh and ops sidecars injected into all application Pods are all managed by SidecarSet. So each reconcile worker should list all Pods that have sidecar injected by it using label selector, and also multiple workers may list their Pods simultaneously.

These examples are so mush, I can't describe all of them. In one word, label/field selectors are useful, but they can not avoid the pressure caused by DeepCopy.

I disagree, prejudical to performance for large-scale clusters is it to always filter lists via labels or indexers. I wouldn't trust myself to always correctly do the deepcopy. so IMHO this is a huge footgun, even for ppl that are very familiar with controller-runtime.

I disagree. If so, why doesn't the informer or lister in client-go call DeepCopy for each object? The kube-controller-manager/kube-scheduler and other controllers based on client-go all list objects without DeepCopy. If someone adds DeepCopy into client-go lister and these controllers, it will degrade the performance of K8s.

I agree with the default DeepCopy, but users should have the right to choose not to use it.
@alvaroaleman

alvaroaleman · 2020-11-30T14:59:43Z

The kube-controller-manager/kube-scheduler and other controllers based on client-go all list objects without DeepCopy. If someone adds DeepCopy into client-go lister and these controllers, it will degrade the performance of K8s.

Upstream kube is also very careful about using the cache mutation detector in integration/e2e tests.
Do you have any data on how much memory exactly this saves you?

Generally speaking, I still think that this is likely to cause subtile bugs ppl will not detect and that the memory gain is too little to be worth introducing this. I would assume that if you can afford to run a cluster with 200k pods, you can afford to give your CM 2 gigs more ram - And that this will turn out to be a lot cheaper for you than having to recognize, diagnose and fix bugs that come out of accidentally mutating objects in your cache.

If you want to discuss this, could you join the biweekly meeting this Thursday?

vincepri · 2020-11-30T15:54:36Z

If we do allow the cache to expose internal objects, we should opt for the option to be something like UnsafeDisableCacheDeepCopy or something similar.

A similar use case like this we encountered in Cluster API has driven us to use metadata only watches, and use a live reader (with something like #1249) when querying for the objects

FillZpp · 2020-12-01T11:17:06Z

@alvaroaleman To hide sensitive data, I can only show part of the heep profile, which was dumped from one of our extend controller in the large-scale cluster. And you can find the DeepCopy in CacheReader costs huge heap memory.

FillZpp · 2020-12-01T11:26:44Z

If we do allow the cache to expose internal objects, we should opt for the option to be something like UnsafeDisableCacheDeepCopy or something similar.

Good idea. I have updated it.

vincepri · 2020-12-01T17:13:08Z

/ok-to-test

fejta-bot · 2021-03-01T17:25:59Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

fejta-bot · 2021-03-31T18:11:31Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

alvaroaleman · 2021-03-31T18:14:52Z

/retest

fejta-bot · 2021-04-30T19:13:30Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

k8s-ci-robot · 2021-04-30T19:13:35Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

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/test-infra repository.

FillZpp · 2021-07-27T08:51:15Z

/reopen

k8s-ci-robot · 2021-07-27T08:51:19Z

@FillZpp: Reopened this PR.

In response to this:

/reopen

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/test-infra repository.

FillZpp · 2021-07-30T06:53:59Z

I have moved UnsafeDisableCacheDeepCopy into cache.Options, which is set by developers using cache.BuilderWithOptions. The Unsafe in the name should sufficiently warn ppl.
The internal fields are disableDeepCopy as developers could not set them and I suppose to simplify the name.
Added some unit tests to verify the addresses of objects got from cache should be equal.

PTAL @alvaroaleman @joelanford

FillZpp · 2021-08-02T02:35:55Z

Please take a look when you have time, thanks. @alvaroaleman @vincepri @joelanford

alvaroaleman · 2021-08-02T13:38:41Z

pkg/cache/internal/informers_map.go

@@ -65,6 +66,7 @@ func newSpecificInformersMap(config *rest.Config,
 		createListWatcher: createListWatcher,
 		namespace:         namespace,
 		selectors:         selectors,
+		disableDeepCopy:   disableDeepCopy,


Generally this looks good, thank you for doing this change.

Only thing I was wondering is if it would make sense to key this by object type (maybe with a * wildcard to make it global). WDYT @vincepri @joelanford ?

Updated. I agree with that, for we might need to add some optional policies for it in future.

@alvaroaleman I also considered the need for this to be GVK-specific. My comment earlier touches on that:

This option would probably require something like the GVKCaches composite cache proposed in #1591, which allows separate cache implementations per GVK.

It's probably do-able to make this option work in the informersMap on a per-GVK basis, but I feel like there are going to be other use cases around GVK-specific cache semantics (e.g. watch GVKA in namespace foo, GVKB in namespace bar and GVKC cluster-wide) that it may be better to keep this minimal.

That said, I see we already have selectors keyed by GVK, so maybe its best to do the same here and re-evaluate where GVK-specifics should live in a separate feature?

That said, I see we already have selectors keyed by GVK, so maybe its best to do the same here and re-evaluate where GVK-specifics should live in a separate feature?

Yeah, that was what I thought, because otherwise we end up in a situation where ppl have to set different options on different levels and that will be pretty confusing. Discussing what option we want to be configurable by GVK how and where in a separate issue is a good idea 👍

pkg/cache/cache_test.go

joelanford

This is looking pretty good to me, just a few more comments. Thanks for working on this!

joelanford · 2021-08-03T13:22:37Z

pkg/cache/cache.go

+	// UnsafeDisableCacheDeepCopy indicates not to deep copy objects during get or list objects.
+	// Be very careful with this, when enabled you must DeepCopy any object before mutating it,
+	// otherwise you will mutate the object in the cache.
+	UnsafeDisableCacheDeepCopy *DisableCacheDeepCopy


Can this just be a bool now?

Also name nit: since this is in the cache package on the cache.Options struct, I don't think we need the word Cache in the name.

Suggested change

// UnsafeDisableCacheDeepCopy indicates not to deep copy objects during get or list objects.

// Be very careful with this, when enabled you must DeepCopy any object before mutating it,

// otherwise you will mutate the object in the cache.

UnsafeDisableCacheDeepCopy *DisableCacheDeepCopy

// UnsafeDisableDeepCopy indicates not to deep copy objects during get or list objects.

// Be very careful with this, when enabled you must DeepCopy any object before mutating it,

// otherwise you will mutate the object in the cache.

UnsafeDisableDeepCopy bool

Can this just be a bool now?

I changed the type from bool to struct in case we might need to add some optional policies for it in future.

Also name nit: since this is in the cache package on the cache.Options struct, I don't think we need the word Cache in the name.

Have removed Cache in the name.

Not sure if @vincepri @alvaroaleman and @varshaprasad96 have opinions, but I think I'd prefer to make this a bool since we have no options right now to deal with (and may never).

If we do end up needing to configure other aspects of this in the future, we could introduce an UnsafeDisableDeepCopyOptions struct alongside this bool if we wanted to avoid a breaking change.

Alright, I have changed it back to bool 🤔

joelanford · 2021-08-03T13:33:03Z

pkg/cache/internal/informers_map.go

@@ -65,6 +66,7 @@ func newSpecificInformersMap(config *rest.Config,
 		createListWatcher: createListWatcher,
 		namespace:         namespace,
 		selectors:         selectors,
+		disableDeepCopy:   disableDeepCopy,


@alvaroaleman I also considered the need for this to be GVK-specific. My comment earlier touches on that:

This option would probably require something like the GVKCaches composite cache proposed in #1591, which allows separate cache implementations per GVK.

It's probably do-able to make this option work in the informersMap on a per-GVK basis, but I feel like there are going to be other use cases around GVK-specific cache semantics (e.g. watch GVKA in namespace foo, GVKB in namespace bar and GVKC cluster-wide) that it may be better to keep this minimal.

That said, I see we already have selectors keyed by GVK, so maybe its best to do the same here and re-evaluate where GVK-specifics should live in a separate feature?

joelanford · 2021-08-03T13:35:44Z

pkg/cache/cache_test.go

+						By("verifying the pods have the same pointer addresses")
+						By("verifying the pointer fields in pod have the same addresses")
+						Expect(out1).To(Equal(out2))
+						Expect(reflect.ValueOf(out1.Labels).Pointer()).To(BeIdenticalTo(reflect.ValueOf(out2.Labels).Pointer()))


non-blocking nit: pretty sure there's no need to actually fetch the pointer for BeIdenticalTo. This could be simplified to:

Suggested change

Expect(reflect.ValueOf(out1.Labels).Pointer()).To(BeIdenticalTo(reflect.ValueOf(out2.Labels).Pointer()))

Expect(out1.Labels).To(BeIdenticalTo(out2.Labels))

I tried Expect(out1.Labels).To(BeIdenticalTo(out2.Labels)) before, but it failed. I'm not sure why this happened, the out1.Labels and out2.Labels do have the same pointer addresses.

Hmm, alright 🤷‍♂️. Thanks for checking that out!

joelanford · 2021-08-03T13:37:50Z

pkg/cache/internal/cache_reader.go

+		// you must DeepCopy any object before mutating it outside
+	} else {
+		// deep copy to avoid mutating cache
+		// TODO(directxman12): revisit the decision to always deepcopy


Can probably remove this comment now.

Suggested change

// TODO(directxman12): revisit the decision to always deepcopy

joelanford · 2021-08-03T13:39:00Z

pkg/cache/internal/cache_reader.go

+		if c.disableDeepCopy != nil {
+			// skip deep copy which might be unsafe
+			// you must DeepCopy any object before mutating it outside
+			outObj = obj


Do we need to set the GVK here?

Set the GVK here might modify the object in Informer. We may just ignore it according to the discussion earlier.

Ah got it! That earlier discussion makes more sense to me now :)

joelanford · 2021-08-03T13:41:12Z

pkg/cache/cache_test.go

+						By("verifying the pointer fields in pod have the same addresses")
+						Expect(out1).To(Equal(out2))
+						Expect(reflect.ValueOf(out1.Labels).Pointer()).To(BeIdenticalTo(reflect.ValueOf(out2.Labels).Pointer()))
+


Can we add another expectation that verifies the GVK is set on the returned objects after Get() and List() when deep copying is disabled?

When deep copying is disabled, we could not ensure the GVK is set on the returned objects.

joelanford · 2021-08-04T01:00:34Z

/remove-lifecycle rotten

FillZpp · 2021-08-05T04:11:58Z

@alvaroaleman @joelanford Thanks for all your comments. I have done all of them. PTAL again 😆

joelanford · 2021-08-05T20:06:17Z

Looks good to me. I think the only outstanding question (in this thread) is whether we should have:

UnsafeDisableDeepCopy bool

or

UnsafeDisableDeepCopy map[schema.GroupVersionKind]bool

which would align this with the selectors SelectorsByGVK field.

FillZpp · 2021-08-06T03:42:04Z

which would align this with the selectors SelectorsByGVK field.

So... How about two fields in cache.Options?

type Options {
    // ......

    // UnsafeDisableDeepCopy indicates not to deep copy objects during get or list objects.
    // Be very careful with this, when enabled you must DeepCopy any object before mutating it,
    // otherwise you will mutate the object in the cache.
    UnsafeDisableDeepCopy bool

    // UnsafeDisableDeepCopyByObject  indicates not to deep copy objects during
    // get or list objects per GVK at the specified object.
    // Be very careful with this, when enabled you must DeepCopy any object before mutating it,
    // otherwise you will mutate the object in the cache.
    UnsafeDisableDeepCopyByObject DisableDeepCopyByObject
}

// DisableDeepCopyByObject associate a client.Object's GVK to whether it should disable deep copy.
DisableDeepCopyByObject map[client.Object]bool

Then developers can choose to disable all with some GVK specified to enable, or enable all with some GVK specified to disable.

WDYT @joelanford

joelanford · 2021-08-06T16:29:32Z

It might be simpler to just define a constant:

package cache
const GroupVersionKindAll = schema.GroupVersionKind{}

And if it is present in the map, we would skip deep copy for all GVKs. I think this is what @alvaroaleman had in mind, and there's precendence for this in other areas of controller-runtime (e.g. cache.Options.Namespace) and core kube (e.g. corev1.NamespaceAll)

FillZpp · 2021-08-07T10:06:49Z

Defining its type as map[schema.GroupVersionKind]bool requires developers to set the option with GVK, which might be a little complicated, comparing to the SelectorsByObject option.

However if its type is map[client.Object]bool, how can we define the object for all object? Do you have any idea? @joelanford

…ng list Signed-off-by: Siyu Wang <FillZpp.pub@gmail.com>

FillZpp · 2021-08-09T07:44:13Z

@joelanford I have changed the option type to DisableDeepCopyByObject and DisableDeepCopyByGVK internal.

FillZpp · 2021-08-13T09:45:39Z

PTAL @alvaroaleman @joelanford when you have time, thanks.

joelanford · 2021-08-18T12:48:26Z

/approve

This looks great @FillZpp! Thanks for sticking with this!

FillZpp · 2021-08-19T03:00:40Z

Thanks for reviewing this @joelanford .Now we need lgtm, PTAL @alvaroaleman

alvaroaleman

thank you!

k8s-ci-robot · 2021-08-19T13:29:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, FillZpp, joelanford

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:

~~OWNERS~~ [alvaroaleman,joelanford]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 25, 2020

k8s-ci-robot requested review from droot and joelanford November 25, 2020 06:51

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 25, 2020

FillZpp changed the title ~~✨ Add DisableDeepCopy as list option to avoid deep copy during list~~ ✨ Add UnsafeDisableCacheDeepCopy as list option to avoid deep copy during list Dec 1, 2020

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 Dec 1, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 1, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 31, 2021

k8s-ci-robot closed this Apr 30, 2021

k8s-ci-robot reopened this Jul 27, 2021

alvaroaleman reviewed Aug 2, 2021

View reviewed changes

joelanford reviewed Aug 2, 2021

View reviewed changes

pkg/cache/cache_test.go Outdated Show resolved Hide resolved

joelanford reviewed Aug 2, 2021

View reviewed changes

pkg/cache/cache_test.go Outdated Show resolved Hide resolved

joelanford reviewed Aug 3, 2021

View reviewed changes

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 4, 2021

FillZpp changed the title ~~✨ Add UnsafeDisableCacheDeepCopy into cache option to avoid deep copy during get/list~~ ✨ Add UnsafeDisableDeepCopy into cache option to avoid deep copy during get/list Aug 5, 2021

Add UnsafeDisableCacheDeepCopy as list option to avoid deep copy duri…

0a6284d

…ng list Signed-off-by: Siyu Wang <FillZpp.pub@gmail.com>

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2021

alvaroaleman approved these changes Aug 19, 2021

View reviewed changes

k8s-ci-robot assigned alvaroaleman Aug 19, 2021

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

k8s-ci-robot merged commit 4e7f0c9 into kubernetes-sigs:master Aug 19, 2021

k8s-ci-robot added this to the v0.10.x milestone Aug 19, 2021

timebertt mentioned this pull request Nov 15, 2021

Add automax procs to gcm gardener/gardener#4979

Merged

FillZpp mentioned this pull request Nov 16, 2021

REQUEST: New membership for FillZpp kubernetes/org#3100

Closed

7 tasks

FillZpp mentioned this pull request Dec 22, 2021

🌱 Add FillZpp as a reviewer #1753

Merged

qiankunli mentioned this pull request Dec 5, 2022

✨ support disable deepcopy on list funcion #2076

Merged

	Expect(reflect.ValueOf(out1.Labels).Pointer()).To(BeIdenticalTo(reflect.ValueOf(out2.Labels).Pointer()))
	Expect(out1.Labels).To(BeIdenticalTo(out2.Labels))

✨ Add UnsafeDisableDeepCopy into cache option to avoid deep copy during get/list #1274

✨ Add UnsafeDisableDeepCopy into cache option to avoid deep copy during get/list #1274

Conversation

FillZpp commented Nov 25, 2020 • edited Loading

k8s-ci-robot commented Nov 25, 2020

alvaroaleman commented Nov 25, 2020

FillZpp commented Nov 26, 2020

alvaroaleman commented Nov 26, 2020

FillZpp commented Nov 27, 2020 • edited Loading

alvaroaleman commented Nov 30, 2020 • edited Loading

vincepri commented Nov 30, 2020

FillZpp commented Dec 1, 2020

FillZpp commented Dec 1, 2020

vincepri commented Dec 1, 2020

fejta-bot commented Mar 1, 2021

fejta-bot commented Mar 31, 2021

alvaroaleman commented Mar 31, 2021

fejta-bot commented Apr 30, 2021

k8s-ci-robot commented Apr 30, 2021

FillZpp commented Jul 27, 2021

k8s-ci-robot commented Jul 27, 2021

FillZpp commented Jul 30, 2021 • edited Loading

FillZpp commented Aug 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford commented Aug 4, 2021

FillZpp commented Aug 5, 2021

joelanford commented Aug 5, 2021

FillZpp commented Aug 6, 2021

joelanford commented Aug 6, 2021 • edited Loading

FillZpp commented Aug 7, 2021

FillZpp commented Aug 9, 2021

FillZpp commented Aug 13, 2021

joelanford commented Aug 18, 2021

FillZpp commented Aug 19, 2021

alvaroaleman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 19, 2021

FillZpp commented Nov 25, 2020 •

edited

Loading

FillZpp commented Nov 27, 2020 •

edited

Loading

alvaroaleman commented Nov 30, 2020 •

edited

Loading

FillZpp commented Jul 30, 2021 •

edited

Loading

joelanford commented Aug 6, 2021 •

edited

Loading