Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use singleton ratelimiter for dynamically created clients #6192

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zach593
Copy link
Contributor

@zach593 zach593 commented Mar 9, 2025

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:

#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.

@Copilot Copilot bot review requested due to automatic review settings March 9, 2025 09:21
@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 9, 2025
@karmada-bot
Copy link
Collaborator

[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

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

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

⚠️ Please install the 'codecov app svg image' 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
Copy link
Contributor Author

zach593 commented Mar 9, 2025

/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.

@karmada-bot 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants