disable deepcopy for list action

[CharlesQQ]

What type of PR is this?
/kind feature

What this PR does / why we need it:

When the node CPU usage rate is almost over 90%, it seems that deepcopy cost the most time:

I1113 14:36:11.161745      18 overridemanager.go:181] applyNamespacedOverrides, list configmap-beta-moa-demo-prod-681,  cluster member-cluster1, cost: 0.980144234
I1113 14:36:12.445315      18 overridemanager.go:181] applyNamespacedOverrides, list configmap-beta-moa-demo-prod-681,  cluster member-cluster2, cost: 0.811084427

After turning off list deepcopy, the list time is reduced to 0.1s or even lower.

I1113 15:43:39.212238      17 overridemanager.go:182] applyNamespacedOverrides, list beta-moa-demo-prod-283,  cluster member-cluster1, cost: 0.040210384
I1113 15:43:39.251189      17 overridemanager.go:182] applyNamespacedOverrides, list beta-moa-demo-prod-97,  cluster member-cluster2, cost: 0.018000362
I1113 15:43:39.314731      17 overridemanager.go:182] applyNamespacedOverrides, list beta-moa-demo-prod-283,  cluster member-cluster2, cost: 0.023638913

Which issue(s) this PR fixes:
part of #5790

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`Performance`: `karmada-controller-manager`: Significant performance improvements have been achieved by reducing deepcopy operations during list processes:
- ​​Binding Controller​​: Response time reduced by 50%
​​- Dependencies Controller​​: Execution time improved 5× faster 
​​- Detector Controller​​: Processing time cut by 50% 

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes missing coverage. Please review.

Project coverage is 48.15%. Comparing base (b3f93ec) to head (d58cbe1).

Files with missing lines	Patch %	Lines
pkg/util/overridemanager/overridemanager.go	0.00%	0 Missing and 2 partials ⚠️
pkg/controllers/binding/binding_controller.go	91.66%	0 Missing and 1 partial ⚠️
...ers/binding/cluster_resource_binding_controller.go	66.66%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5813      +/-   ##
==========================================
+ Coverage   48.14%   48.15%   +0.01%     
==========================================
  Files         677      677              
  Lines       56048    56068      +20     
==========================================
+ Hits        26982    26999      +17     
- Misses      27296    27298       +2     
- Partials     1770     1771       +1     

Flag	Coverage Δ
unittests	48.15% <90.69%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[zach593]

Regarding this performance optimization, which clearly introduces side effects, I have two suggestions:

1. How much of a performance improvement will it bring? Could you add benchmarks for these calls along with e2e performance comparison?

2. Is it possible to add some mechanisms to mitigate these side effects? For example, adding unit tests or e2e tests so that if someone later introduces code that triggers this side effect (i.e., directly modifying the informer cache), our test suite can detect such behavior.

[CharlesQQ]

How much of a performance improvement will it bring? Could you add benchmarks for these calls along with e2e performance comparison?

Through golang pprof analysis, it is found that during the restart period, deep copy will have obvious CPU time loss. After removing it, this part of the loss disappeared.
Maybe this change is difficult to compare through e2e testing. Do you have any good testing suggestions?

[zach593]

Through golang pprof analysis, it is found that during the restart period, deep copy will have obvious CPU time loss. After removing it, this part of the loss disappeared.

This part is what I mean by e2e performance comparison, I'm not saying e2e tests.

[CharlesQQ]

/retest

[CharlesQQ]

pls take a look at the e2e performance comparison @zach593

[zach593]

Regarding this pprof result, what was the resource level at that time? When the controller is cold started, without or with this commit, how long does it take to clear the queue? How long does a single reconciliation take?

Reducing the time taken for an operation by an order of magnitude is great, but I think it would be more helpful to see how much of an overall improvement the change made, especially for an optimization that has potential side effects.

[zach593]

2. Is it possible to add some mechanisms to mitigate these side effects? For example, adding unit tests or e2e tests so that if someone later introduces code that triggers this side effect (i.e., directly modifying the informer cache), our test suite can detect such behavior.

There is an env KUBE_CACHE_MUTATION_DETECTOR, which may cause panic when the informer cache is illegally modified, I think if we can enable this environment when running e2e tests, I guess it could eliminate the side effects of disable deepcopy.

[CharlesQQ]

Reducing the time taken for an operation by an order of magnitude is great, but I think it would be more helpful to see how much of an overall improvement the change made, especially for an optimization that has potential side effects.

I will test in a real test environment and give optimization data

[CharlesQQ]

here is an env KUBE_CACHE_MUTATION_DETECTOR, which may cause panic when the informer cache is illegally modified, I think if we can enable this environment when running e2e tests, I guess it could eliminate the side effects of disable deepcopy.

KUBE_CACHE_MUTATION_DETECTOR=true take effect in go-client sharedinformer, does it also work in controller-runtime? If so, this environment variable should be added in e2e tests

[zach593]

KUBE_CACHE_MUTATION_DETECTOR=true take effect in go-client sharedinformer, does it also work in controller-runtime?

karmada/vendor/sigs.k8s.io/controller-runtime/pkg/cache/internal/informers.go

Lines 59 to 61 in 0713e4b

	func NewInformers(config *rest.Config, options *InformersOpts) *Informers {
	newInformer := cache.NewSharedIndexInformer
	if options.NewInformer != nil {

The answer is yes, it also applies to controller-runtime, because controller-runtime also uses client-go informer.

[zach593]

A new concern just popped into my mind: does disabling deepCopy consistently lead to data being in a state of race conditions?

[CharlesQQ]

A new concern just popped into my mind: does disabling deepCopy consistently lead to data being in a state of race conditions?

I don't understand what this sentence means, can you give me an example?

[CharlesQQ]

4000 workload, 8000 rb, 12C cpu:

for binding-controller, before optimization, it tooks 9 minutes; after optimization, it takes 4 minutes and 30 seconds

for dependencies resource detector, before optimization, it tooks 7 minutes and 15 seconds; after optimization, it takes 1 minutes and 30 seconds

[CharlesQQ]

/retest

[zach593]

I'm wondering if there will be a data race between the reading thread and the informer writing thread. This is something I suddenly realized, and I didn't think about it very seriously. Since I raised this question, I did some research and the result is it should not cause a data race.

karmada/vendor/k8s.io/client-go/tools/cache/controller.go

Lines 548 to 557 in 9ea0382

	// from oldest to newest
	for _, d := range deltas {
	obj := d.Object
	switch d.Type {
	case Sync, Replaced, Added, Updated:
	if old, exists, err := clientState.Get(obj); err == nil && exists {
	if err := clientState.Update(obj); err != nil {
	return err
	}

From the code above, you can see that when writing to the cache, the entire variable is replaced with the new object from the deltas . If the reading thread holds an object, when the writing thread updates the cache, the object held by the reading thread and the cache will no longer have any connection.

In other words, if the reading thread modifies the held object in the *a.xxx = yyy way, then this will cause a data race, and will also pollute the cache and objects already held by other reading threads. The object read from the cache later will also be polluted. Pollution will continue until the informer updates the object.

[CharlesQQ]

In other words, if the reading thread modifies the held object in the *a.xxx = yyy way, then this will cause a data race, and will also pollute the cache and objects already held by other reading threads. The object read from the cache later will also be polluted. Pollution will continue until the informer updates the object.

list action disabling deepCopy, must make sure the list item will not be updated, or make a deepcopy of it before updating it.

[CharlesQQ]

4000 workload, 12C cpu, when restarting

detector controller: takes 2 minutes and 30 seconds

after disable deepcopy for list action, it takes 1 minute and 15 seconds

[RainbowMango]

Disable deepcopy indeed can improve performance, but should be used very cautiously.

/assign

[RainbowMango]

Generally looks good to me.
But I think we should rename variable name or add more comments to notice the object can not be mutated.

[RainbowMango]

Does it make sense to add a namespace filter? Like:

                bindingList := &workv1alpha2.ResourceBindingList{}
-               if err := c.Client.List(ctx, bindingList); err != nil {
+               if err := c.Client.List(ctx, bindingList, &client.ListOptions{UnsafeDisableDeepCopy: ptr.To(true), Namespace: namespace}); err != nil {

[RainbowMango]

bindingList --> readonlyBindingList?

[CharlesQQ]

Does it make sense to add a namespace filter

Yes

[CharlesQQ]

But I think we should rename variable name or add more comments to notice the object can not be mutated.

Controller-runtime has a detailed description of UnsafeDisableDeepCopy, which is very clear. Is there no need to add it?

karmada/vendor/sigs.k8s.io/controller-runtime/pkg/client/options.go

Lines 495 to 499 in 65e7f74

	// UnsafeDisableDeepCopy indicates not to deep copy objects during 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.
	// +optional
	UnsafeDisableDeepCopy *bool

[RainbowMango]

/lgtm
/approve

[karmada-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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 [RainbowMango]

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