use singleton ratelimiter for dynamically created clients #6192

zach593 · 2025-03-09T09:21:22Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

see #6094, this PR add the a ratelimiterGetter singleton for dynamically created cluster clients, to let the rate limiter to more accurately limit the total qps of multiple dynamically created clients .

Which issue(s) this PR fixes:

Fixes #6094

Special notes for your reviewer:

I found it difficult to test using mock client, because the existing fake client implementations do not accept ratelimiter parameters. However, I added some tests in pkg/util/ratelimiter_test.go to simulate and illustrate the final effect.

If we want to test the rate limit more completely, we may need to introduce envTest (a sub package of controller-runtime), but this requires more general considerations, because the impact is large, so it will not be expanded in this PR.

Does this PR introduce a user-facing change?:

karmada-controller-manager, karmada-agent, karmada-metrics-adapter: rate limit parameters of  'cluster-api-qps' and 'cluster-api-burst', these parameters will be stricter and administrators may need to adjust them to get the same effect as before the update.

karmada-bot · 2025-03-09T09:21:28Z

[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 xishanyongye-chang 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:

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

Copilot

PR Overview

This PR implements a new rate limiting mechanism by switching from setting QPS and Burst values directly on REST configurations to using a singleton ratelimiterGetter based on the token bucket algorithm. The changes span multiple components (agent, aggregated-apiserver, metrics-adapter, scheduler, controller-manager, etc.), ensuring that dynamically created clients consistently use the new rate limiter setup.

Replace direct QPS/Burst assignment with flowcontrol.NewTokenBucketRateLimiter.
Update client and dynamic client setup functions to accept a ClientOption containing the new RateLimiterGetter.
Amend tests to align with the new ClientOption structure.

Reviewed Changes

File	Description
cmd/agent/app/agent.go	Updates kubeconfig construction to use RateLimiter.
cmd/aggregated-apiserver/app/options/options.go	Switches REST config rate limiter to token bucket mechanism.
cmd/metrics-adapter/app/options/options.go	Modifies both REST config and metrics controller to use the new limiter.
cmd/scheduler-estimator/app/scheduler-estimator.go	Applies new rate limiter when building dynamic clients.
cmd/descheduler/app/descheduler.go	Similar rate limiter update for client configuration.
cmd/scheduler/app/scheduler.go	Replaces QPS/Burst assignment with rate limiter initialization.
cmd/karmada-search/app/karmada-search.go	Updates client config in search component to use the rate limiter.
cmd/controller-manager/app/controllermanager.go	Adjusts multiple client creation paths to use new ClientOption.
cmd/webhook/app/webhook.go	Refactors webhook config to adopt rate limiter setup.
pkg/metricsadapter/multiclient/client.go	Introduces nil-check logic before applying the new RateLimiterGetter.
pkg/controllers/status/cluster_status_controller_test.go	Updates test client options to remove explicit QPS/Burst values.
pkg/controllers/status/cluster_status_controller.go	Changes function signatures to pass ClientOption to dynamic client setup.
pkg/controllers/status/work_status_controller.go	Modifies dynamic client creation to include the new ClientOption.
pkg/search/controller.go	Adjusts dynamic client builder to pass nil for ClientOption where applicable.
pkg/controllers/context/context.go	Adds ClientOption to the controller context structure.
pkg/controllers/mcs/service_export_controller.go	Updates dynamic client setup to use ClientOption.
pkg/controllers/multiclusterservice/endpointslice_collect_controller.go	Ensures dynamic client creation includes ClientOption.

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

pkg/controllers/status/cluster_status_controller.go

pkg/controllers/status/cluster_status_controller_test.go

Signed-off-by: zach593 <zach_li@outlook.com>

codecov-commenter · 2025-03-09T09:43:36Z

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

Codecov Report

Attention: Patch coverage is 45.45455% with 42 lines in your changes missing coverage. Please review.

Project coverage is 47.95%. Comparing base (8d43fe2) to head (410c5d4).

Files with missing lines	Patch %	Lines
pkg/util/membercluster_client.go	12.50%	10 Missing and 4 partials ⚠️
cmd/controller-manager/app/controllermanager.go	0.00%	10 Missing ⚠️
cmd/agent/app/agent.go	0.00%	6 Missing ⚠️
pkg/metricsadapter/controller.go	0.00%	3 Missing ⚠️
pkg/metricsadapter/multiclient/client.go	0.00%	3 Missing ⚠️
cmd/metrics-adapter/app/options/options.go	0.00%	2 Missing ⚠️
pkg/controllers/mcs/service_export_controller.go	0.00%	1 Missing ⚠️
...clusterservice/endpointslice_collect_controller.go	0.00%	1 Missing ⚠️
pkg/search/controller.go	0.00%	1 Missing ⚠️
pkg/util/objectwatcher/objectwatcher.go	80.00%	1 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6192      +/-   ##
==========================================
- Coverage   47.97%   47.95%   -0.03%     
==========================================
  Files         674      675       +1     
  Lines       55841    55886      +45     
==========================================
+ Hits        26789    26798       +9     
- Misses      27305    27334      +29     
- Partials     1747     1754       +7

Flag	Coverage Δ
unittests	`47.95% <45.45%> (-0.03%)`	⬇️

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 · 2025-03-09T16:01:14Z

/remove-kind feature
/kind bug

I think the issue with the rate limiter not working is more like a bug, please feel free to correct me if anyone thinks it should not be.

XiShanYongYe-Chang · 2025-03-26T07:52:17Z

/assign

XiShanYongYe-Chang · 2025-03-27T08:13:31Z

pkg/util/membercluster_client.go

 		}
 		clusterClientSet.KubeClient = kubeclientset.NewForConfigOrDie(clusterConfig)
 	}
 	return &clusterClientSet, nil
 }

+// NewClusterDynamicClientSetFunc is a function that returns a dynamic client for the given member cluster.
+type NewClusterDynamicClientSetFunc = func(clusterName string, client client.Client, clientOption *ClientOption) (*DynamicClusterClient, error)


What is the difference between removing the equal sign and defining the function variable directly and the current definition method?
type NewClusterDynamicClientSetFunc func(clusterName string, client client.Client, clientOption *ClientOption) (*DynamicClusterClient, error)

There is no difference in actual results. I personally prefer the way to use type alias (with equal sign) because it changes nothing, it just acts as an abbreviation.

XiShanYongYe-Chang

Genericly lgtm, ask @RainbowMango to help a look
/cc @RainbowMango

RainbowMango · 2025-03-29T02:43:55Z

OK. Trying my best... I'm not sure if I will have enough time for this soon.

RainbowMango

/assign

Copilot

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

pkg/util/membercluster_client_test.go:198

[nitpick] Add tests for scenarios where ClientOption includes a non-nil RateLimiterGetter to ensure that the rate limiter is correctly applied to the client.

clientOption: &ClientOption{}

Copilot · 2025-04-17T06:35:36Z

pkg/util/membercluster_client.go

 	clusterConfig, err := BuildClusterConfig(clusterName, clusterGetter(client), secretGetter(client))
 	if err != nil {
 		return nil, err
 	}
 	var clusterClientSet = DynamicClusterClient{ClusterName: clusterName}

 	if clusterConfig != nil {
+		if clientOption != nil {
+			if clientOption.RateLimiterGetter != nil {
+				clusterConfig.RateLimiter = clientOption.RateLimiterGetter(clusterName)


Consider validating that clientOption.RateLimiterGetter is non-nil or providing a fallback default rate limiter so that client creation always enforces rate limiting.

Suggested change

clusterConfig.RateLimiter = clientOption.RateLimiterGetter(clusterName)

clusterConfig.RateLimiter = clientOption.RateLimiterGetter(clusterName)

} else {

clusterConfig.RateLimiter = flowcontrol.NewTokenBucketRateLimiter(10, 100) // Default rate limiter

RainbowMango · 2025-04-17T06:46:38Z

pkg/util/ratelimiter.go

+// RateLimiterGetter is a struct to get rate limiter.
+type RateLimiterGetter struct {
+	limiters map[string]flowcontrol.RateLimiter
+	mu       sync.Mutex
+	qps      float32
+	burst    int
+}


This structure looks weird, as RateLimiterGetter actually manages and holds multiple rate-limiter instances; it should not hold the qps and burst, which belong to a specific rate-limiter. Otherwise lead to unexpected behavior, think the following flow:

SetLimits(10, 20)

SetLimits(15, 30)

GetRateLimiter("member-cluster") // the rate-limiter will be initilized with qps=15, burst=30

Copilot AI review requested due to automatic review settings March 9, 2025 09:21

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 9, 2025

karmada-bot requested review from chaunceyjiang, Garrybest, huntsman-li, ikaven1024, jwcesign, lonelyCZ, pigletfly, Poor12, Vacant2333 and XiShanYongYe-Chang March 9, 2025 09:21

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 9, 2025

Copilot AI reviewed Mar 9, 2025

View reviewed changes

pkg/controllers/status/cluster_status_controller.go Show resolved Hide resolved

pkg/controllers/status/cluster_status_controller_test.go Show resolved Hide resolved

use singleton ratelimiter for dynamically created clients

410c5d4

Signed-off-by: zach593 <zach_li@outlook.com>

zach593 force-pushed the dynamic-ratelimiter branch from 592517f to 410c5d4 Compare March 9, 2025 09:29

karmada-bot added kind/bug Categorizes issue or PR as related to a bug. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Mar 9, 2025

zach593 mentioned this pull request Mar 11, 2025

unify http ratelimiter for control plane client #6095

Merged

karmada-bot assigned XiShanYongYe-Chang Mar 26, 2025

XiShanYongYe-Chang reviewed Mar 27, 2025

View reviewed changes

karmada-bot requested a review from RainbowMango March 27, 2025 09:45

zach593 mentioned this pull request Apr 15, 2025

Add deepequal check for objectWatcher.Update() #6150

Open

RainbowMango reviewed Apr 17, 2025

View reviewed changes

karmada-bot assigned RainbowMango Apr 17, 2025

RainbowMango requested a review from Copilot April 17, 2025 06:34

Copilot AI reviewed Apr 17, 2025

View reviewed changes

RainbowMango reviewed Apr 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use singleton ratelimiter for dynamically created clients #6192

use singleton ratelimiter for dynamically created clients #6192

zach593 commented Mar 9, 2025 •

edited by RainbowMango

Loading

karmada-bot commented Mar 9, 2025

Copilot AI left a comment

codecov-commenter commented Mar 9, 2025

zach593 commented Mar 9, 2025

XiShanYongYe-Chang commented Mar 26, 2025

XiShanYongYe-Chang Mar 27, 2025

zach593 Mar 27, 2025

XiShanYongYe-Chang left a comment

RainbowMango commented Mar 29, 2025

RainbowMango left a comment

Copilot AI left a comment

Copilot AI Apr 17, 2025

RainbowMango Apr 17, 2025 •

edited

Loading

use singleton ratelimiter for dynamically created clients #6192

Are you sure you want to change the base?

use singleton ratelimiter for dynamically created clients #6192

Conversation

zach593 commented Mar 9, 2025 • edited by RainbowMango Loading

karmada-bot commented Mar 9, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

codecov-commenter commented Mar 9, 2025

Codecov Report

zach593 commented Mar 9, 2025

XiShanYongYe-Chang commented Mar 26, 2025

XiShanYongYe-Chang Mar 27, 2025

Choose a reason for hiding this comment

zach593 Mar 27, 2025

Choose a reason for hiding this comment

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

RainbowMango commented Mar 29, 2025

RainbowMango left a comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Apr 17, 2025

Choose a reason for hiding this comment

RainbowMango Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

zach593 commented Mar 9, 2025 •

edited by RainbowMango

Loading

RainbowMango Apr 17, 2025 •

edited

Loading