Skip to content

fix(kubernetes): raise kubevirt-csi API client rate limits - #3428

Merged
Andrei Kvapil (kvaps) merged 1 commit into
mainfrom
fix/kubevirt-csi-api-rate-limits
Jul 28, 2026
Merged

fix(kubernetes): raise kubevirt-csi API client rate limits#3428
Andrei Kvapil (kvaps) merged 1 commit into
mainfrom
fix/kubevirt-csi-api-rate-limits

Conversation

@lexfrei

@lexfrei Aleksei Sviridkin (lexfrei) commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

What this PR does

The kubevirt-csi-driver builds its infra and tenant cluster clients from a rest.Config with QPS/Burst left unset, so client-go uses its default 5 QPS / 10 burst. The wrapper's NFS ControllerPublishVolume polls 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 returns client rate limiter Wait returned an error: context deadline exceeded, which the tenant kubelet reports as FailedAttachVolume. 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-qps and --kube-api-burst make 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 read 0 as 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 no values.schema.json, chart values, ApplicationDefinition, node prerequisite, metric or label changed. Nothing in the downstream trigger map is reached.

Release note

fix(kubernetes): raise the kubevirt-csi-driver API client rate limits (QPS/Burst, tunable via --kube-api-qps and --kube-api-burst) above the client-go defaults so a burst of concurrent volume attaches no longer times out as FailedAttachVolume.

Summary by CodeRabbit

  • New Features

    • Added configurable Kubernetes API request rate limits for improved handling of high-volume operations.
    • Added validation to prevent invalid or non-positive rate-limit settings.
  • Bug Fixes

    • Improved reliability during bursts of concurrent volume attachment requests by reducing request throttling and timeout risk.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@github-actions github-actions Bot added size/L This PR changes 100-499 lines, ignoring generated files area/kubernetes Issues or PRs related to the tenant Kubernetes app kind/bug Categorizes issue or PR as related to a bug labels Jul 22, 2026
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@lexfrei, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: bc386ebf-a172-435d-9b9f-b4a384990a12

📥 Commits

Reviewing files that changed from the base of the PR and between 1b77976 and 2dfe380.

📒 Files selected for processing (2)
  • packages/apps/kubernetes/images/kubevirt-csi-driver/main.go
  • packages/apps/kubernetes/images/kubevirt-csi-driver/main_test.go
📝 Walkthrough

Walkthrough

The kubevirt CSI driver adds kube-api-qps and kube-api-burst flags, validates positive values, applies them to tenant and infrastructure Kubernetes configurations, and adds tests for propagation, validation, and burst handling.

Changes

Kubernetes API rate limits

Layer / File(s) Summary
Rate-limit flags and helpers
packages/apps/kubernetes/images/kubevirt-csi-driver/main.go
Adds configurable QPS and burst flags, validates positive values, and applies them to rest.Config.
Startup wiring and regression coverage
packages/apps/kubernetes/images/kubevirt-csi-driver/main.go, packages/apps/kubernetes/images/kubevirt-csi-driver/main_test.go
Validates flags during startup, configures both Kubernetes clients, and tests defaults, explicit values, invalid inputs, and burst admission.

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
Loading

Suggested reviewers: androndo

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: raising kubevirt-csi API client rate limits.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/kubevirt-csi-api-rate-limits

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 706c3f0 and 1b77976.

📒 Files selected for processing (2)
  • packages/apps/kubernetes/images/kubevirt-csi-driver/main.go
  • packages/apps/kubernetes/images/kubevirt-csi-driver/main_test.go

Comment on lines +204 to +215
// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 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.go

Repository: 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 || true

Repository: 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')
PY

Repository: 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 -B10

Repository: 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>
@lexfrei
Aleksei Sviridkin (lexfrei) force-pushed the fix/kubevirt-csi-api-rate-limits branch from 1b77976 to 2dfe380 Compare July 27, 2026 20:01

@kvaps Andrei Kvapil (kvaps) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@kvaps
Andrei Kvapil (kvaps) merged commit 07cefee into main Jul 28, 2026
15 of 16 checks passed
@kvaps
Andrei Kvapil (kvaps) deleted the fix/kubevirt-csi-api-rate-limits branch July 28, 2026 15:45
IvanHunters added a commit that referenced this pull request Aug 4, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/kubernetes Issues or PRs related to the tenant Kubernetes app kind/bug Categorizes issue or PR as related to a bug size/L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants