karmada-search: wire cluster-api-qps and cluster-api-burst#7325
karmada-search: wire cluster-api-qps and cluster-api-burst#7325manmathbh wants to merge 1 commit intokarmada-io:masterfrom
Conversation
karmada-bot
added
the
size/M
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR wires configurable rate limiting for member-cluster API access in karmada-search by introducing and threading --cluster-api-qps / --cluster-api-burst into the search controller’s member-cluster dynamic client creation.
Changes:
- Add
--cluster-api-qpsand--cluster-api-burstflags tokarmada-searchoptions. - Pass a
util.ClientOptionwith a per-cluster rate limiter getter intosearch.NewController. - Extend the search controller to accept and use the client option when building member-cluster dynamic clients (and update unit tests accordingly).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cmd/karmada-search/app/options/options.go |
Adds new CLI flags for member-cluster API QPS/burst tuning. |
cmd/karmada-search/app/karmada-search.go |
Constructs ClientOption using the new flags and passes it into the search controller. |
pkg/search/controller.go |
Threads ClientOption through controller construction and dynamic cluster client creation. |
pkg/search/controllers_test.go |
Updates tests to match the updated controller/builder function signatures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| flags.Float32Var(&o.ClusterAPIQPS, "cluster-api-qps", 40.0, "QPS to use while talking with cluster kube-apiserver.") | ||
| flags.IntVar(&o.ClusterAPIBurst, "cluster-api-burst", 60, "Burst to use while talking with cluster kube-apiserver.") |
There was a problem hiding this comment.
This PR introduces new user-facing flags (--cluster-api-qps/--cluster-api-burst). The PR description’s release-note block is currently empty; please add an appropriate release note entry (or explicitly set it to "NONE" if you consider it not user-facing).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces QPS and burst rate limiting for API calls made to member clusters by karmada-search. New command-line flags (--cluster-api-qps and --cluster-api-burst) have been added to configure these limits, and the search.Controller has been updated to pass these options to the clusterDynamicClientBuilder when creating dynamic clients. The current tests for the controller compile by passing nil for the new client option, but a suggestion was made to add a test case to verify that the clientOption is correctly passed to the clusterDynamicClientBuilder to ensure the QPS and burst settings are applied.
| // It returns the created Controller or an error if initialization fails. | ||
| func createController(ctx context.Context, restConfig *rest.Config, factory informerfactory.SharedInformerFactory, restMapper meta.RESTMapper) (*Controller, error) { | ||
| newController, err := NewController(restConfig, factory, restMapper) | ||
| newController, err := NewController(restConfig, factory, restMapper, nil) |
There was a problem hiding this comment.
The tests have been updated to compile by passing nil for the new clientOption parameter. However, this doesn't verify that the option is correctly passed through the controller. To ensure the new QPS and burst settings are applied, please consider adding a test case that verifies the clientOption is passed to clusterDynamicClientBuilder. You could, for example, modify createController to accept a clientOption and then add a test that provides a non-nil option and asserts that the mocked clusterDynamicClientBuilder receives it.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7325 +/- ##
=======================================
Coverage 42.03% 42.03%
=======================================
Files 874 874
Lines 53551 53556 +5
=======================================
+ Hits 22511 22514 +3
- Misses 29349 29352 +3
+ Partials 1691 1690 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PTAL @XiShanYongYe-Chang |
|
Thanks flake test: /retest |
pkg/search/controller.go
Outdated
| informerFactory informerfactory.SharedInformerFactory | ||
| clusterLister clusterlister.ClusterLister | ||
| queue workqueue.TypedRateLimitingInterface[any] | ||
| clientOption *util.ClientOption |
There was a problem hiding this comment.
I suggest renaming it to clusterClientOption.
There was a problem hiding this comment.
Updated as suggested: renamed to clusterClientOption in the search controller path and re-ran search/app tests. PTAL.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
The lint has failed, can you help fix it? |
Thanks for the feedback! I've pushed a commit to take care of the linting issue. I'm just waiting to see if the CI passes, and then I'll squash the commits into one. |
Signed-off-by: manmathbh <manmathcode@gmail.com> search: rename clientOption to clusterClientOption Signed-off-by: manmathbh <manmathcode@gmail.com> search: format controller for gci lint Signed-off-by: manmathbh <manmathcode@gmail.com>
manmathbh
force-pushed
the
fix/search-cluster-api-qps-burst
branch
from
39393f5 to
84893a8
Compare
|
@XiShanYongYe-Chang IG we are good to go! |
XiShanYongYe-Chang
left a comment
XiShanYongYe-Chang
left a comment
There was a problem hiding this comment.
Thanks~
/lgtm
Ask the todo owner to help review it again.
/cc @zach593
What type of PR is this?
/kind feature
/kind cleanup
What this PR does / why we need it:
This PR adds --cluster-api-qps and --cluster-api-burst to karmada-search, and wires them into member-cluster dynamic client creation used by the search controller.
Previously, the dynamic client path passed nil client options. With this change, karmada-search now builds a util.ClientOption with a rate limiter and passes it through, so member-cluster API access uses the configured QPS/burst settings.
Also includes:
small naming cleanup (clientOption -> clusterClientOption) based on review feedback
formatting/lint fix in controller.go
Special notes reviewer:
Validated locally with:
go test [karmada-search]
go test [pkg]
[verify-staticcheck.sh]
[verify-import-aliases.sh]
Does this PR introduce a user-facing change?:
``karmada-search
: add--cluster-api-qps` and `--cluster-api-burst` flags, and use them when creating member-cluster dynamic clients in search controller.`