Add support for broker client-go qps and burst config#1272
Conversation
|
🤖 Created branch: z_pr1272/msft-paddy14/users/padgupta/add_rate_limit_config |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds propagation of global QPS/Burst into broker and local Kubernetes REST configs via a new applyQPSBurst helper; reorders Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Syncer
participant GlobalCfg
participant RESTConfig
participant K8sClient
Test->>GlobalCfg: Init()/set K8sClientQPS/Burst (or unset)
Test->>Syncer: Create broker client (may provide explicit REST configs)
Syncer->>RESTConfig: obtain or build BrokerRestConfig
Syncer->>GlobalCfg: read K8sBrokerClientQPS / K8sBrokerClientBurst
alt BrokerRestConfig has explicit QPS/Burst
RESTConfig-->>Syncer: keep explicit QPS/Burst
else explicit values absent
Syncer->>RESTConfig: applyQPSBurst -> set QPS/Burst from GlobalCfg
end
Syncer->>K8sClient: create client using BrokerRestConfig
K8sClient-->>Test: client uses applied QPS/Burst
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
msft-paddy14
changed the title
msft-paddy14
changed the title
Add BROKER_K8S_QPS and BROKER_K8S_BURST environment variables to configure client-go rate limiting for both local and broker REST clients. Defaults are QPS=5 and Burst=10. Changes: - Add QPS and Burst fields to brokerSpecification struct - Apply settings to both LocalRestConfig and BrokerRestConfig - Add unit tests for custom and default values Signed-off-by: Paddy <padgupta@microsoft.com>
msft-paddy14
force-pushed
the
users/padgupta/add_rate_limit_config
branch
from
9904ade to
e518085
Compare
msft-paddy14
requested review from
Oats87,
skitt,
sridhargaddam,
tpantelis and
vthapar
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/syncer/broker/syncer.go (1)
387-394:⚠️ Potential issue | 🔴 CriticalBug: QPS/Burst settings are applied to
BrokerRestConfigafter the broker dynamic client is already created, so they have no effect.
createBrokerClient()(line 388) callsresource.NewDynamicClient(c.BrokerRestConfig)internally (line 356), which readsQPSandBurstat client-creation time to configure the rate limiter.applyQPSBurstat line 394 mutates the config after the client is built, so the broker client will use the default rate limits instead of the configured ones.Move
applyQPSBurstfor the broker config intocreateBrokerClient, just before theNewDynamicClientcall:Proposed fix
In
createBrokerClient, apply QPS/Burst before creating the client. One approach — pass the spec intocreateBrokerClientand apply there:-func (c *SyncerConfig) createBrokerClient() error { +func (c *SyncerConfig) createBrokerClient(brokerSpec *brokerSpecification) error { _, gvr, e := util.ToUnstructuredResource(c.ResourceConfigs[0].BrokerResourceType, c.RestMapper) if e != nil { return e //nolint:wrapcheck // OK to return the error as is. @@ // ... existing broker config setup ... + applyQPSBurst(c.BrokerRestConfig, brokerSpec) + c.BrokerClient, err = resource.NewDynamicClient(c.BrokerRestConfig) return errors.Wrap(err, "error creating dynamic client") }And in
ensureClients, update the call site and remove the post-hoc apply:if c.BrokerClient == nil { - if err := c.createBrokerClient(); err != nil { + if err := c.createBrokerClient(brokerSpec); err != nil { return err } + } else { + applyQPSBurst(c.BrokerRestConfig, brokerSpec) } - // Apply QPS and Burst settings to broker REST config - applyQPSBurst(c.BrokerRestConfig, brokerSpec) - return nil
🤖 Fix all issues with AI agents
In `@pkg/syncer/broker/syncer_test.go`:
- Around line 730-735: The test currently asserts on actualBrokerRestConfig
after creation but doesn't verify the rest config as seen by the client at
creation time; move the assertions for actualBrokerRestConfig (QPS and Burst)
into the mock/generator callback that intercepts broker client creation so you
assert the config parameter passed into the client factory there (the same place
you already verify LocalRestConfig), e.g., inside the mock callback that
receives the rest.Config used to create the broker client; assert QPS ==
float32(5) and Burst == 10 on that config instead of only checking the captured
variable later.
- Around line 708-713: The test currently captures a pointer to
config.BrokerRestConfig (actualBrokerRestConfig = inConfig) so mutations after
client creation hide the ordering bug; update the mock for NewDynamicClient
(resourceutils.NewDynamicClient or dynamic.NewForConfig replacement in the test)
to capture and assert inConfig.QPS and inConfig.Burst at call time (inside the
mock) for the broker branch, and keep the existing post-creation assertions for
completeness; this ensures applyQPSBurst must set QPS/Burst before
NewDynamicClient is invoked (refer to symbols:
NewDynamicClient/resourceutils.NewDynamicClient, applyQPSBurst,
config.BrokerRestConfig, actualBrokerRestConfig).
🧹 Nitpick comments (1)
pkg/syncer/broker/syncer.go (1)
364-368:getBrokerSpecification()is called twice — once here and once insidecreateBrokerClient()(line 324).When
BrokerClientis nil andBrokerRestConfigis nil, both calls execute and parse the same environment variables. If you adopt the fix above (passingbrokerSpecintocreateBrokerClient), you can reuse the same spec insidecreateBrokerClientand eliminate the duplicate call at line 324.
pkg/syncer/broker/syncer.go
Outdated
| } | ||
|
|
||
| // Apply QPS and Burst settings to local REST config | ||
| applyQPSBurst(c.LocalRestConfig, brokerSpec) |
There was a problem hiding this comment.
The caller creates and provides the LocalRestConfig so the caller should configure the rate limiter settings and this component shouldn't set them as a side effect. Same with the BrokerRestConfig, if provided by the caller.
This component should only configure the rate limiter settings for the BrokerRestConfig if it creates it via the env variables (ie in createBrokerClient after line 345).
There was a problem hiding this comment.
Makes sense. Addressed the changes.
Signed-off-by: Paddy <padgupta@microsoft.com>
msft-paddy14
force-pushed
the
users/padgupta/add_rate_limit_config
branch
from
8d70cd1 to
bd956d6
Compare
Signed-off-by: Paddy <padgupta@microsoft.com>
Signed-off-by: Paddy <padgupta@microsoft.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/syncer/broker/syncer.go (1)
362-386:⚠️ Potential issue | 🟠 MajorQPS/Burst not applied to the local cluster client.
The PR description states the settings "apply to both the local cluster client and the broker cluster client," but
applyQPSBurstis only called forBrokerRestConfig(line 346). The local client created at line 373 fromLocalRestConfigdoes not get QPS/Burst applied. If this is intentional (e.g., local config is always caller-provided), please update the PR description; otherwise, add the call here too.Proposed fix if local client should also get QPS/Burst
if c.LocalClient == nil { + applyQPSBurst(c.LocalRestConfig) c.LocalClient, err = resource.NewDynamicClient(c.LocalRestConfig) if err != nil { return errors.Wrap(err, "error creating dynamic client") } }
🤖 Fix all issues with AI agents
In `@pkg/global/config.go`:
- Around line 35-38: Rename the two constants from K8S_CLIENT_QPS and
K8S_CLIENT_BURST to follow the project's MixedCaps style: K8sClientQPS and
K8sClientBurst; update every reference to these constants (search for
K8S_CLIENT_QPS and K8S_CLIENT_BURST) to use the new names (e.g., in any config
lookups or settings), and run go build/tests to ensure there are no remaining
references or import issues.
🧹 Nitpick comments (2)
pkg/syncer/broker/syncer.go (1)
388-400:applyQPSBurstsilently treats0as "not configured" — intentional zero values are impossible.Using
0as the sentinel means a caller cannot explicitly set QPS or Burst to0via global config. For QPS this is unlikely to matter, but it's worth documenting or considering a different sentinel (e.g., negative value, or a separate "is set" check).pkg/syncer/broker/syncer_test.go (1)
714-729: Consider asserting QPS/Burst inside the mock to guard against ordering regressions.The assertions at lines 726-727 check
actualBrokerRestConfigafter syncer creation. SinceactualBrokerRestConfigis a captured pointer, it will reflect mutations made both before and afterNewDynamicClient. IfapplyQPSBurstwere accidentally moved afterNewDynamicClientin production code, this test would still pass (pointer sees the final state) but the real client wouldn't get the configured rate limits. Asserting inside the mock callback would catch this.Proposed inline assertion in the mock
resourceutils.NewDynamicClient = func(inConfig *rest.Config) (dynamic.Interface, error) { if equality.Semantic.DeepDerivative(inConfig, config.LocalRestConfig) { return localDynClient, nil } else if equality.Semantic.DeepDerivative(inConfig, config.BrokerRestConfig) || (brokerAPIServer != "" && strings.HasSuffix(inConfig.Host, brokerAPIServer)) { actualBrokerRestConfig = inConfig + // Verify QPS/Burst are set at client-creation time + if qps := global.Get(global.K8S_CLIENT_QPS, 0); qps != 0 { + Expect(inConfig.QPS).To(Equal(float32(qps))) + } + if burst := global.Get(global.K8S_CLIENT_BURST, 0); burst != 0 { + Expect(inConfig.Burst).To(Equal(burst)) + } return brokerDynClient, nil }
Signed-off-by: Paddy <padgupta@microsoft.com>
Signed-off-by: Paddy <padgupta@microsoft.com>
|
🤖 Closed branches: [z_pr1272/msft-paddy14/users/padgupta/add_rate_limit_config] |
4 participants
Adds support for configuring client-go QPS and Burst settings via environment variables for the broker syncer component.
Addresses: #1271
When syncing resources between the local cluster and broker, the broker syncer can experience client-side throttling.
The environment variables apply to both the local cluster client and the broker cluster client created by the broker syncer in ensureClients().
Add support for configuring client-go QPS and Burst via environment variables:
Summary by CodeRabbit
New Features
Tests
Bug Fixes