feat(detector): resource detector matched policy potimization #5802

CharlesQQ · 2024-11-11T07:26:08Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

karmada-bot · 2024-11-11T07:26:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign rainbowmango for approval. For more information see the Kubernetes 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:

OWNERS

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

codecov-commenter · 2024-11-11T08:13:38Z

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

Codecov Report

Attention: Patch coverage is 28.15534% with 74 lines in your changes missing coverage. Please review.

Project coverage is 48.05%. Comparing base (8d43fe2) to head (23dc788).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/detector/detector.go	22.61%	63 Missing and 2 partials ⚠️
pkg/detector/policy.go	0.00%	4 Missing ⚠️
pkg/detector/preemption.go	71.42%	4 Missing ⚠️
cmd/controller-manager/app/controllermanager.go	0.00%	1 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5802      +/-   ##
==========================================
+ Coverage   47.97%   48.05%   +0.07%     
==========================================
  Files         674      674              
  Lines       55841    55823      -18     
==========================================
+ Hits        26789    26825      +36     
+ Misses      27305    27254      -51     
+ Partials     1747     1744       -3

Flag	Coverage Δ
unittests	`48.05% <28.15%> (+0.07%)`	⬆️

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.

pkg/detector/compare.go

chaosi-zju · 2024-11-11T08:59:28Z

does d.Client and d.InformerManager all have cache, and different cache?

Is it possible that there is a policy in the d.InformerManager cache, but it has not been synced to the d.Client's cache?

In that case, if the creation time of deployment and policy are very close, maybe d.client could not find the policy. (the failed e2e job 32793260336 maybe caused by it)

chaosi-zju · 2024-11-11T09:07:06Z

the failed e2e job 32793260336 maybe caused by it

my guass maybe correct, as you see corresponding detector log

2024-11-11T08:19:26.656651192Z stderr F I1111 08:19:26.656575       1 detector.go:236] Reconciling object: apps/v1, kind=Deployment, karmadatest-xnw86/deploy-4dpmc
2024-11-11T08:19:26.656739492Z stderr F I1111 08:19:26.656663       1 detector.go:919] PropagationPolicy(karmadatest-xnw86/deploy-4dpmc) has been added or updated.
2024-11-11T08:19:26.65699674Z stderr F I1111 08:19:26.656902       1 detector.go:361] Attempts to match policy for resource(apps/v1, kind=Deployment, karmadatest-xnw86/deploy-4dpmc)
2024-11-11T08:19:26.657175843Z stderr F I1111 08:19:26.657084       1 compare.go:66] No propagationpolicy match for resource(apps/v1, kind=Deployment, karmadatest-xnw86/deploy-4dpmc)
2024-11-11T08:19:26.657185351Z stderr F I1111 08:19:26.657101       1 detector.go:378] Attempts to match cluster policy for resource(apps/v1, kind=Deployment, karmadatest-xnw86/deploy-4dpmc)
2024-11-11T08:19:26.65742698Z stderr F I1111 08:19:26.657118       1 detector.go:386] No clusterpropagationpolicy find.
2024-11-11T08:19:26.660140465Z stderr F I1111 08:19:26.660036       1 detector.go:1160] Matched 0 resources by policy(karmadatest-xnw86/deploy-4dpmc)
2024-11-11T08:19:26.660781832Z stderr F I1111 08:19:26.660194       1 detector.go:785] Add object(apps/v1, kind=Deployment, karmadatest-xnw86/deploy-4dpmc) to waiting list, length of list is: 215
2024-11-11T08:19:26.667659376Z stderr F I1111 08:19:26.666134       1 detector.go:236] Reconciling object: apps/v1, kind=Deployment, karmadatest-xnw86/deploy-4dpmc
2024-11-11T08:19:26.667677359Z stderr F I1111 08:19:26.666184       1 detector.go:361] Attempts to match policy for resource(apps/v1, kind=Deployment, karmadatest-xnw86/deploy-4dpmc)
2024-11-11T08:19:26.667682419Z stderr F I1111 08:19:26.666302       1 compare.go:66] No propagationpolicy match for resource(apps/v1, kind=Deployment, karmadatest-xnw86/deploy-4dpmc)
2024-11-11T08:19:26.667686015Z stderr F I1111 08:19:26.666311       1 detector.go:378] Attempts to match cluster policy for resource(apps/v1, kind=Deployment, karmadatest-xnw86/deploy-4dpmc)
2024-11-11T08:19:26.667689341Z stderr F I1111 08:19:26.666326       1 detector.go:386] No clusterpropagationpolicy find.
2024-11-11T08:19:26.667692678Z stderr F I1111 08:19:26.666461       1 policy.go:98] No matched policy for object: apps/v1, kind=Deployment, karmadatest-xnw86/deploy-4dpmc

CharlesQQ · 2024-11-11T09:49:55Z

In that case, if the creation time of deployment and policy are very close, maybe d.client could not find the policy.

Both obtain resource objects from the cache. I think If d.Client has this problem, d.InformerManager may also have it.

chaosi-zju · 2024-11-12T12:22:54Z

The failed reason can be explained as follows:

XiShanYongYe-Chang · 2024-11-12T12:31:04Z

You are advised to obtain the policy list from karmada-apiserver and check the performance consumption caused by karmada-apiserver to see if the benefits are greatly improved.

XiShanYongYe-Chang · 2024-11-25T06:17:56Z

/retest

zach593 · 2024-11-30T13:37:14Z

Is the purpose of this PR to remove the dependency on unstructured?
Why do we need this generated typed informer manager instead of using the controller-runtime informer?

CharlesQQ · 2024-12-02T11:17:03Z

Why do we need this generated typed informer manager instead of using the controller-runtime informer?

If controller-runtime is used, the logic of resource detector resources entering the workqueue also needs to use controller-runtime.

zach593 · 2025-02-23T12:36:19Z

If controller-runtime is used, the logic of resource detector resources entering the workqueue also needs to use controller-runtime.

This is not the case... I tried replacing the generic InformerManger with the controller-runtime cache and it passes all the tests and works perfectly. This did not require replacing the asyncWorker, just replacing the lister and informer was enough.

Here is the code: ctripcloud@e358fd1

CharlesQQ · 2025-02-24T04:09:59Z

I tried replacing the generic InformerManger with the controller-runtime cache and it passes all the tests and works perfectly.

you replaced the informer with controller-runtime, so you think use controller-runtime is better than genericInformer? What do you think?

zach593 · 2025-02-24T05:37:52Z

so you think use controller-runtime is better than genericInformer?

Yes, because controller-runtime informer is also a typed informer.

CharlesQQ · 2025-02-24T08:48:10Z

Yes, because controller-runtime informer is also a type informer.

What do you think? @XiShanYongYe-Chang

zach593 · 2025-03-08T16:46:25Z

pkg/detector/detector.go

+	if err != nil {
+		klog.Errorf("Failed to add event handler for ClusterPropagationPolicy: %v", err)
+		return err
+	}


you need to put those two AddEventHandler() after the d.Processor.Run(d.ConcurrentResourceTemplateSyncs, d.stopCh), because AddEventHandler() will immediately start the workers, and in the end of the workers logic, they will call d.Processor.Add()

you mean d.Processor must run before d.ReconcilePropagationPolicy or d.ReconcileClusterPropagationPolicy trigger logic? If d.Processor has not been created, d.Processor.Add() might have problems?

yes, occasional panic on startup

I got it, tks

CharlesQQ · 2025-03-09T01:46:03Z

/retest

Signed-off-by: chang.qiangqiang <chang.qiangqiang@immomo.com>

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 11, 2024

karmada-bot requested review from chaunceyjiang and whitewindmills November 11, 2024 07:26

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 11, 2024

CharlesQQ force-pushed the resource-detector-optimization branch 2 times, most recently from 7527032 to cfc8912 Compare November 11, 2024 08:00

chaosi-zju reviewed Nov 11, 2024

View reviewed changes

pkg/detector/compare.go Outdated Show resolved Hide resolved

RainbowMango mentioned this pull request Nov 11, 2024

karmada controller reconcile performs performance optimization #5790

Open

CharlesQQ force-pushed the resource-detector-optimization branch 5 times, most recently from 5761d96 to a03b852 Compare November 12, 2024 07:19

CharlesQQ force-pushed the resource-detector-optimization branch 8 times, most recently from 8e1d81b to a3b488b Compare November 19, 2024 08:25

CharlesQQ force-pushed the resource-detector-optimization branch from a3b488b to fe7afcd Compare November 22, 2024 10:48

karmada-bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 22, 2024

karmada-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 22, 2024

CharlesQQ force-pushed the resource-detector-optimization branch 5 times, most recently from c345c81 to 0345eed Compare November 23, 2024 16:04

CharlesQQ force-pushed the resource-detector-optimization branch from 0345eed to e4064c0 Compare March 8, 2025 14:32

karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 8, 2025

CharlesQQ force-pushed the resource-detector-optimization branch 4 times, most recently from 566a305 to d6d5308 Compare March 8, 2025 16:10

zach593 reviewed Mar 8, 2025

View reviewed changes

CharlesQQ force-pushed the resource-detector-optimization branch from d6d5308 to 962701d Compare March 10, 2025 03:31

feat(detector): remove type convert in detector controller

23dc788

Signed-off-by: chang.qiangqiang <chang.qiangqiang@immomo.com>

CharlesQQ force-pushed the resource-detector-optimization branch from 962701d to 23dc788 Compare March 10, 2025 03:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(detector): resource detector matched policy potimization #5802

feat(detector): resource detector matched policy potimization #5802

CharlesQQ commented Nov 11, 2024 •

edited

Loading

karmada-bot commented Nov 11, 2024

codecov-commenter commented Nov 11, 2024 •

edited

Loading

chaosi-zju commented Nov 11, 2024

chaosi-zju commented Nov 11, 2024

CharlesQQ commented Nov 11, 2024

chaosi-zju commented Nov 12, 2024 •

edited

Loading

XiShanYongYe-Chang commented Nov 12, 2024

XiShanYongYe-Chang commented Nov 25, 2024

zach593 commented Nov 30, 2024

CharlesQQ commented Dec 2, 2024

zach593 commented Feb 23, 2025

CharlesQQ commented Feb 24, 2025 •

edited

Loading

zach593 commented Feb 24, 2025

CharlesQQ commented Feb 24, 2025 •

edited

Loading

zach593 Mar 8, 2025

CharlesQQ Mar 9, 2025

zach593 Mar 10, 2025

CharlesQQ Mar 10, 2025

CharlesQQ commented Mar 9, 2025

feat(detector): resource detector matched policy potimization #5802

Are you sure you want to change the base?

feat(detector): resource detector matched policy potimization #5802

Conversation

CharlesQQ commented Nov 11, 2024 • edited Loading

karmada-bot commented Nov 11, 2024

codecov-commenter commented Nov 11, 2024 • edited Loading

Codecov Report

chaosi-zju commented Nov 11, 2024

chaosi-zju commented Nov 11, 2024

CharlesQQ commented Nov 11, 2024

chaosi-zju commented Nov 12, 2024 • edited Loading

XiShanYongYe-Chang commented Nov 12, 2024

XiShanYongYe-Chang commented Nov 25, 2024

zach593 commented Nov 30, 2024

CharlesQQ commented Dec 2, 2024

zach593 commented Feb 23, 2025

CharlesQQ commented Feb 24, 2025 • edited Loading

zach593 commented Feb 24, 2025

CharlesQQ commented Feb 24, 2025 • edited Loading

zach593 Mar 8, 2025

Choose a reason for hiding this comment

CharlesQQ Mar 9, 2025

Choose a reason for hiding this comment

zach593 Mar 10, 2025

Choose a reason for hiding this comment

CharlesQQ Mar 10, 2025

Choose a reason for hiding this comment

CharlesQQ commented Mar 9, 2025

CharlesQQ commented Nov 11, 2024 •

edited

Loading

codecov-commenter commented Nov 11, 2024 •

edited

Loading

chaosi-zju commented Nov 12, 2024 •

edited

Loading

CharlesQQ commented Feb 24, 2025 •

edited

Loading

CharlesQQ commented Feb 24, 2025 •

edited

Loading