Configure local REST client QPS/Burst from global config#2051
Configure local REST client QPS/Burst from global config#2051tpantelis wants to merge 1 commit intosubmariner-io:develfrom
Conversation
Initialize global config before creating REST clients to allow QPS and Burst settings to be applied. This enables cluster-specific tuning of Kubernetes API request rates. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
tpantelis
requested review from
Oats87,
aswinsuryan,
dfarrell07,
maayanf24,
skitt,
sridhargaddam,
vthapar and
yboaron
as code owners
|
🤖 Created branch: z_pr2051/tpantelis/rest_qps |
WalkthroughUpdated module dependencies in go.mod and refactored pkg/agent/main.go to retrieve a global ConfigMap during initialization. The global ConfigMap is used to configure QPS and Burst settings before creating the dynamic Kubernetes client. API signatures for global.Init and configmap.WatchAndSignalOnChange were updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
go.mod (1)
11-11: LGTM —admiralpseudo-version correctly carries the new API signatures.The pseudo-version (
v0.24.0-m0.0.20260218133429-4ba4e2683354) is the expected cross-repo development pattern for the Submarinerdevelbranch when coordinating API changes ahead of a milestone tag. The commit timestamp (2026-02-18) aligns with the day before this PR was opened, and the AI summary confirms this version introduces the updatedglobal.Init/configmap.WatchAndSignalOnChangesignatures that the rest of this PR consumes.Just a reminder: this pseudo-version — along with the indirect
sigs.k8s.io/structured-merge-diff/v6 v6.3.2-0.20260122202528-d9cc6641c482on line 76 — should be replaced with proper tagged releases before any milestone/release cut.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 11, The go.mod entry for github.com/submariner-io/admiral uses the expected pseudo-version v0.24.0-m0.0.20260218133429-4ba4e2683354 which matches the new API signatures (global.Init and configmap.WatchAndSignalOnChange) referenced elsewhere; no functional change is required now, but ensure you replace this pseudo-version (and the indirect sigs.k8s.io/structured-merge-diff/v6 pseudo-version) with proper tagged releases before the milestone/release cut.pkg/agent/main.go (2)
115-116:k8sClient(used by the configmap watcher) is not re-created after QPS/Burst are applied.
kubernetes.NewForConfigcaptures a snapshot ofcfgat line 115 — modifications tocfg.QPS/cfg.Burstat lines 128-129 have no effect on it. The configmap watcher at line 126 therefore operates with the original (pre-global-config) rate-limit settings. For a simple watch this is likely inconsequential, but it's inconsistent with the stated goal of applying global QPS/Burst to all REST clients. If full coverage is desired, recreatek8sClientafter the QPS/Burst assignment (which would also require moving bothconfigmap.Getcalls below it, or caching their results).Also applies to: 126-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/agent/main.go` around lines 115 - 116, k8sClient is created with kubernetes.NewForConfig(cfg) before you mutate cfg.QPS/cfg.Burst, so the client keeps the old rate limits; recreate the client after setting cfg.QPS and cfg.Burst (i.e., call kubernetes.NewForConfig(cfg) again and assign to k8sClient) and then perform the configmap watcher and any configmap.Get calls (or alternatively cache the earlier Get results and reuse them) so the watcher uses the updated rate-limited REST client.
128-129: Unconditional zero-write may silently discard kubeconfig-specified QPS/Burst values.When
QPSis zero, the createdRESTClientfalls back toDefaultQPS: 5and whenBurstis zero it falls back toDefaultBurst: 10— so this won't disable rate limiting. However, ifclientcmd.BuildConfigFromFlagsalready populatedcfg.QPS/cfg.Burstfrom a kubeconfig that explicitly sets them, these lines unconditionally replace those values with0when the global config is absent. A guard preserves the intent of "only apply when explicitly configured":♻️ Proposed fix — guard on non-zero before overriding
- cfg.QPS = float32(global.Get(global.K8sClientQPS, 0)) - cfg.Burst = global.Get(global.K8sClientBurst, 0) + if qps := global.Get(global.K8sClientQPS, 0); qps > 0 { + cfg.QPS = float32(qps) + } + if burst := global.Get(global.K8sClientBurst, 0); burst > 0 { + cfg.Burst = burst + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/agent/main.go` around lines 128 - 129, The code unconditionally sets cfg.QPS and cfg.Burst to values returned by global.Get, which can overwrite kubeconfig-provided non-zero values with zero; change the logic in main.go to only assign cfg.QPS = float32(global.Get(global.K8sClientQPS, 0)) and cfg.Burst = global.Get(global.K8sClientBurst, 0) when the retrieved value is non-zero (i.e., check the returned value from global.Get before writing into cfg.QPS/cfg.Burst) so existing kubeconfig settings are preserved; update the assignment sites that reference cfg.QPS and cfg.Burst accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@go.mod`:
- Line 11: The go.mod entry for github.com/submariner-io/admiral uses the
expected pseudo-version v0.24.0-m0.0.20260218133429-4ba4e2683354 which matches
the new API signatures (global.Init and configmap.WatchAndSignalOnChange)
referenced elsewhere; no functional change is required now, but ensure you
replace this pseudo-version (and the indirect
sigs.k8s.io/structured-merge-diff/v6 pseudo-version) with proper tagged releases
before the milestone/release cut.
In `@pkg/agent/main.go`:
- Around line 115-116: k8sClient is created with kubernetes.NewForConfig(cfg)
before you mutate cfg.QPS/cfg.Burst, so the client keeps the old rate limits;
recreate the client after setting cfg.QPS and cfg.Burst (i.e., call
kubernetes.NewForConfig(cfg) again and assign to k8sClient) and then perform the
configmap watcher and any configmap.Get calls (or alternatively cache the
earlier Get results and reuse them) so the watcher uses the updated rate-limited
REST client.
- Around line 128-129: The code unconditionally sets cfg.QPS and cfg.Burst to
values returned by global.Get, which can overwrite kubeconfig-provided non-zero
values with zero; change the logic in main.go to only assign cfg.QPS =
float32(global.Get(global.K8sClientQPS, 0)) and cfg.Burst =
global.Get(global.K8sClientBurst, 0) when the retrieved value is non-zero (i.e.,
check the returned value from global.Get before writing into cfg.QPS/cfg.Burst)
so existing kubeconfig settings are preserved; update the assignment sites that
reference cfg.QPS and cfg.Burst accordingly.
2 participants
Initialize global config before creating REST clients to allow QPS and Burst settings to be applied. This enables cluster-specific tuning of Kubernetes API request rates.
Summary by CodeRabbit
Chores
Refactor