fix(kubernetes): raise kubevirt-csi API client rate limits - #3428
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe kubevirt CSI driver adds ChangesKubernetes API rate limits
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Driver as kubevirt-csi-driver
participant TenantConfig as tenant rest.Config
participant InfraConfig as infra rest.Config
participant KubernetesClients as Kubernetes clients
Driver->>Driver: Validate QPS and burst flags
Driver->>TenantConfig: Apply rate-limit settings
Driver->>InfraConfig: Apply rate-limit settings
TenantConfig->>KubernetesClients: Create tenant client
InfraConfig->>KubernetesClients: Create infrastructure client
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/kubernetes/images/kubevirt-csi-driver/main.go`:
- Around line 204-215: Update validateRateLimitFlags to reject QPS values that
are NaN, infinite, or cannot be represented as a finite, positive float32 before
assigning or casting to rest.Config.QPS; retain the existing positive burst
validation and error behavior for valid finite QPS values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52cc0e1f-7668-4805-b0fc-6aa3f72991fa
📒 Files selected for processing (2)
packages/apps/kubernetes/images/kubevirt-csi-driver/main.gopackages/apps/kubernetes/images/kubevirt-csi-driver/main_test.go
| // validateRateLimitFlags rejects non-positive limits. client-go treats QPS == 0 | ||
| // as "use the 5 QPS default" and QPS < 0 as "no rate limiting at all", so an | ||
| // operator passing 0 expecting "unlimited" would silently get the starving | ||
| // default this fix exists to avoid; refuse both rather than surprise them. | ||
| func validateRateLimitFlags(qps float64, burst int) error { | ||
| if qps <= 0 { | ||
| return fmt.Errorf("kube-api-qps must be positive, got %v", qps) | ||
| } | ||
| if burst <= 0 { | ||
| return fmt.Errorf("kube-api-burst must be positive, got %d", burst) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='packages/apps/kubernetes/images/kubevirt-csi-driver/main.go'
echo '--- validateRateLimitFlags and call sites ---'
rg -n 'validateRateLimitFlags|kube-api-qps|kube-api-burst|QPS|Burst|float32\(' "$file" -n -A4 -B4
echo
echo '--- nearby source around validation and config setup ---'
nl -ba "$file" | sed -n '180,260p'Repository: cozystack/cozystack
Length of output: 3591
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe Go conversion semantics relevant to the comment.
cat > /tmp/qps_probe.go <<'EOF'
package main
import (
"fmt"
"math"
)
func main() {
values := []float64{
math.NaN(),
math.Inf(1),
math.Inf(-1),
0,
-1,
1e-50, // underflows to float32(0)
1e-45, // subnormal / boundary-ish
1e38, // representable in float32
1e39, // overflows float32 to +Inf
1e100, // overflows float32 to +Inf
}
for _, v := range values {
f32 := float32(v)
fmt.Printf("v=%v f32=%v isNaN=%v isInf(v)=%v isInf(f32)=%v f32<=0=%v\n",
v, f32, math.IsNaN(v), math.IsInf(v, 0), math.IsInf(float64(f32), 0), f32 <= 0)
}
}
EOF
go run /tmp/qps_probe.goRepository: cozystack/cozystack
Length of output: 869
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect rest.Config definition and QPS/Burst docs if available in module cache or repo.
# Use the repository only as data; don't build anything.
rg -n 'type Config struct|QPS float32|Burst int' "$(go env GOPATH 2>/dev/null)/pkg/mod" -g 'config.go' -m 5 || trueRepository: cozystack/cozystack
Length of output: 2247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cfg='/home/jailuser/go/pkg/mod/k8s.io/client-go@v0.35.0/rest/config.go'
throttle='/home/jailuser/go/pkg/mod/k8s.io/client-go@v0.35.0/flowcontrol/throttler.go'
echo '--- rest.Config QPS/Burst docs and defaulting ---'
sed -n '45,155p' "$cfg"
echo
echo '--- client-side throttler construction ---'
sed -n '1,220p' "$throttle"Repository: cozystack/cozystack
Length of output: 4777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe relevant comparisons for NaN/Inf and float32 round-trip behavior.
python3 - <<'PY'
import math
vals = [math.nan, math.inf, -math.inf, 0.0, 1e-50, 1e-45, 1e38, 1e39]
for v in vals:
f32 = float(v) # Python float is float64; this is just a placeholder if numpy unavailable
print(v, math.isnan(v), math.isinf(v), f32 <= 0 if not math.isnan(f32) else 'nan')
PYRepository: cozystack/cozystack
Length of output: 335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cfg='/home/jailuser/go/pkg/mod/k8s.io/client-go@v0.35.0/rest/config.go'
throttle='/home/jailuser/go/pkg/mod/k8s.io/client-go@v0.35.0/util/flowcontrol/throttle.go'
echo '--- rest.Config around QPS/Burst fields and defaults ---'
sed -n '45,160p' "$cfg"
echo
echo '--- throttling implementation used by client-go ---'
sed -n '1,220p' "$throttle"Repository: cozystack/cozystack
Length of output: 10421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root='/home/jailuser/go/pkg/mod/k8s.io/client-go@v0.35.0'
rg -n 'func NewTokenBucketRateLimiter|NewTokenBucketRateLimiter\(' "$root" -g '*.go' -A40 -B10Repository: cozystack/cozystack
Length of output: 50375
Reject NaN, ±Inf, and float32-overflow/underflow QPS values. qps <= 0 still lets math.NaN(), math.Inf(1), and values that round to 0 or +Inf through the float32 cast into rest.Config.QPS; that can silently drop throttling or fall back to client-go’s default limiter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/apps/kubernetes/images/kubevirt-csi-driver/main.go` around lines 204
- 215, Update validateRateLimitFlags to reject QPS values that are NaN,
infinite, or cannot be represented as a finite, positive float32 before
assigning or casting to rest.Config.QPS; retain the existing positive burst
validation and error behavior for valid finite QPS values.
The kubevirt-csi controller built its infra- and tenant-cluster Kubernetes clients on a rest.Config that left QPS/Burst unset, so client-go applied its 5 QPS / 10 burst default. ControllerPublishVolume polls the infra PVC once per second for up to two minutes; when a tenant scales out and several volumes attach at once, the default limiter starves and the poll fails with "client rate limiter Wait returned an error: context deadline exceeded", which the tenant kubelet surfaces as FailedAttachVolume. Set QPS/Burst to 100/200, overridable via --kube-api-qps and --kube-api-burst, on both the infra and tenant configs so a concurrent-attach burst is admitted promptly. Reject non-positive flag values, which client-go would otherwise turn into the starving default (0) or unlimited (<0). Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
1b77976 to
2dfe380
Compare
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM — verified. QPS 100 / Burst 200 are coherent and applied to both the tenant and infra rest.Configs before any client is constructed, with validation guarding the 0-starves / negative-unlimited footguns and a real token-bucket regression test. The red E2E is the known node-join / cilium flake on kubernetes-latest and kubernetes-previous — the CSI path is healthy in that run (no FailedAttachVolume, no rate-limiter waits), so the failure is unrelated to this change.
Bring the worker-pool split branch up to date with main (111 commits), picking up the node-join and CI fixes it was missing: the 18m tenant node-join deadline, guest-fsync-off on ephemeral CI disks (#3455), kubevirt-csi client rate-limit raise (#3428), the Cilium ingress-IP race guard (#3430), and etcd-operator v0.5.4. Conflict resolution: main's #3535 (render the talos-reconcile Job for the default md0 group) modified packages/apps/kubernetes/templates/talos/ talos-reconcile-job.yaml, which this branch deletes because the worker split moves that Job into the kubernetes-nodes chart. Kept the parent template deleted and dropped the parent-scoped test talos_reconcile_nodegroups_test.yaml: the empty-nodeGroups md0-default gap it guards cannot occur in the per-pool child chart, and migration 54 materialises the implicit md0 into an explicit child HelmRelease. targetVersion resolves to 55 (migration 54 on top of main's 53). Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
What this PR does
The kubevirt-csi-driver builds its infra and tenant cluster clients from a
rest.Configwith QPS/Burst left unset, so client-go uses its default 5 QPS / 10 burst. The wrapper's NFSControllerPublishVolumepolls the infra PVC once per second for up to two minutes. When a tenant scales out and several volumes attach at once, that shared token bucket starves and the poll returnsclient rate limiter Wait returned an error: context deadline exceeded, which the tenant kubelet reports asFailedAttachVolume. It looks like an intermittent E2E failure attaching a tenant volume, with nothing wrong in the storage backend.This sets QPS/Burst to 100/200 on both configs, before any client is built. The new flags
--kube-api-qpsand--kube-api-burstmake the limits tunable. The compiled-in defaults are the real fix: the deployment passes no client flags, so both configs resolve to the in-cluster config and pick up the new defaults. Non-positive values are rejected, since client-go would read0as the starving default and a negative value as no rate limiting.Includes a regression test: a burst of 200 waiters times out against the default 5 QPS / 10 burst limiter and passes against the configured one.
Screenshots
None. No UI changes.
Downstream repositories
The diff touches only the driver's Go source in
packages/apps/kubernetes/images/kubevirt-csi-driver/. No package was added, renamed or removed, and novalues.schema.json, chart values,ApplicationDefinition, node prerequisite, metric or label changed. Nothing in the downstream trigger map is reached.Release note
Summary by CodeRabbit
New Features
Bug Fixes