fix(config): make the client-go rate limit configurable (kubevirt.qps, kubevirt.burst) by itayherzberg · Pull Request #30 · kubevirt/redfish-controller

itayherzberg · 2026-10-21T16:53:51Z

Problem

With about 15 BareMetalHosts provisioning at the same time through one kubevirt-redfish instance, Redfish requests took 10–35 seconds. Ironic's boot-device PATCH ran past the OpenShift route's 30-second timeout, got a 504, and inspection looped forever.

Cause

The Kubernetes client is created with client-go's default rate limiter: 5 requests/second, burst 10. That default is sized for kubectl, not for a server. Every Redfish request fans out into several Kubernetes API calls (GET /Systems/{id} is 6: one to resolve the system name, then the VM four times and the VMI once; a boot-device PATCH is 12), and Ironic polls every host it manages. With N hosts in flight the slowest request waits roughly 12 × N / 5 seconds: at 15 hosts, ~36 s. client-go reports the waits as client-side throttling.

Reproduced on OpenShift 4.22 / OpenShift Virtualization 4.22.12 with 100 × GET /redfish/v1/Systems/{id}, 20 in parallel, on this branch with the limiter left at its default (identical to upstream behaviour):

n=100 avg=19.22s max=20.97s codes: 200=100        (wall clock 100s)

I1021 16:24:06.232028  1 request.go:752] "Waited before sending request" delay="4.179412515s" reason="client-side throttling, not priority and fairness" verb="GET" URL="https://172.231.0.1:443/api..."

(10 such lines in the pod log during the run)

Fix

Make the limiter configurable. The default is not changed.

kubevirt.qps / kubevirt.burst in the config file. They default to client-go's own rest.DefaultQPS / rest.DefaultBurst (5 / 10); unset or 0 keeps the default, negative values fail validation.
KUBEVIRT_REDFISH_KUBEVIRT_QPS / KUBEVIRT_REDFISH_KUBEVIRT_BURST override the file. No extra code: this is the viper environment mapping every other key already has.
pkg/kubevirt/client.go: NewClient sets rest.Config.QPS / Burst from the pure helper kubeClientRateLimits, which reads the values through a small interface (GetKubeClientRateLimits()), the same way the client already reads GetDataVolumeConfig(). The value in use is logged at startup: Kubernetes client rate limits: qps=… burst=….
cmd/main.go: the client is created after the logger is initialized. Before, it was created first, so anything the client logged at startup was lost.
Helm: kubevirt.qps / kubevirt.burst values (default 0, which leaves the keys out of the rendered config.yaml); values-example.yaml shows 100 / 200.

Sizing for many concurrent hosts: with qps: 100, burst: 200 the worst request is ~12 × N / 100 s. 100 requests/second at ~20 ms each is about two concurrent requests against an API server that allows thousands in flight. If client-side throttling still appears in the log under load, the limit is too low.

Verification on the cluster

Same load, same cluster, with kubevirt.qps: 100, kubevirt.burst: 200 in the chart values:

Kubernetes client rate limits: qps=100 burst=200
n=100 avg=0.89s max=1.30s codes: 200=100          (wall clock 5s, 0 throttling lines)

Environment wins over the file: file at 100 / 200 plus KUBEVIRT_REDFISH_KUBEVIRT_QPS=50 → Kubernetes client rate limits: qps=50 burst=200.

Invalid value is rejected at startup: KUBEVIRT_REDFISH_KUBEVIRT_BURST=-5 →
Failed to load configuration: … Invalid KubeVirt burst (kubevirt.burst must be greater than 0, got -5).

Tests

pkg/config/config_test.go: defaults are 5 / 10; values from the file and from the environment; negative values rejected; zero means default.
pkg/kubevirt/ratelimit_test.go: kubeClientRateLimits with no config, a config without the method, configured values, and per-field zero fallback.
go build ./..., go vet ./..., go test ./..., gofmt, go mod tidy (no change) and helm lint pass; helm template renders qps / burst only when set.

One of four independent PRs from running many concurrent BMHs against kubevirt-redfish; each stands on its own and they can merge in any order: #31 (task worker pool), #32 (VM/VMI informer cache), #29 (shutdown hang). The three config PRs add the same two test helpers (minimalConfig, writeConfig) to pkg/config/config_test.go; whichever lands later drops its copy on rebase.

kubevirt-prow · 2026-10-21T16:54:00Z

Hi @itayherzberg. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

…ory, write tracker dropped Picks up bd12d08, the review revision of kubevirt#32. Conflicts were the adjacent hunks the rebase note in SCALEFIX-CHANGES.md describes: kept kubevirt#30/kubevirt#31's qps/burst/workers lines and their shared test helpers, dropped cache_enabled and the write tracker. # Conflicts: # config/config.yaml # helm/templates/secret.yaml # helm/values-example.yaml # helm/values.yaml # pkg/config/config.go # pkg/config/config_test.go # pkg/kubevirt/client.go

itayherzberg · 2026-11-03T11:07:16Z

@MarSik Gentle ping on this when you have a chance! Let me know if you need any additional context

aburdenthehand · 2026-11-04T15:24:23Z

/cc @0xFelix

MarSik · 2026-11-12T13:23:26Z

/lgtm
/approve

kubevirt-prow · 2026-11-12T13:23:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MarSik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [MarSik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubevirt-prow · 2026-11-12T13:27:23Z

@itayherzberg: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-redfish-controller-unit-test	`cdf6a51`	link	true	`/test pull-redfish-controller-unit-test`
pull-redfish-controller-sanity	`cdf6a51`	link	true	`/test pull-redfish-controller-sanity`

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

…s, kubevirt.burst) The Kubernetes client used client-go's default limiter of 5 requests/s, burst 10, a default sized for kubectl. Every Redfish request fans out into several API calls (GET System: 6, boot-device PATCH: 12), so with many BareMetalHosts polling at once requests queue for tens of seconds and run past the OpenShift route's 30s timeout. Add kubevirt.qps / kubevirt.burst (and the matching KUBEVIRT_REDFISH_KUBEVIRT_QPS / _BURST environment variables, through the viper mapping every other key already has). Unset or zero keeps client-go's defaults, so nothing changes unless an operator raises them; negative values are rejected. The value in use is logged at startup, which required creating the client after the logger is initialized in cmd/main.go. Signed-off-by: itayherzberg <itayherzberg@gmail.com>

kubevirt-prow · 2026-11-12T16:13:26Z

New changes are detected. LGTM label has been removed.

itayherzberg · 2026-11-12T16:23:24Z

@MarSik rebased onto main: after #31 merged, the minimalConfig/writeConfig test helpers were declared twice in pkg/config/config_test.go, which broke unit-test and sanity. No functional change; this PR now reuses the helpers already in main. Could you re-add /lgtm?

/test all

kubevirt-prow · 2026-11-12T16:23:43Z

@itayherzberg: PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test.

Details

In response to this:

@MarSik rebased onto main: after #31 merged, the minimalConfig/writeConfig test helpers were declared twice in pkg/config/config_test.go, which broke unit-test and sanity. No functional change; this PR now reuses the helpers already in main. Could you re-add /lgtm?

/test all

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

0xFelix · 2026-11-13T15:14:20Z

+	// Zero means "not set" (client-go's own convention): apply the defaults.
+	if kv.QPS == 0 {
+		kv.QPS = DefaultKubeClientQPS
+	}
+	if kv.Burst == 0 {
+		kv.Burst = DefaultKubeClientBurst
+	}


Drop this and only reject negatives here? kubeClientRateLimits already falls back to the defaults for <= 0, and SetDefault covers the unset case, so the validator wouldn't need to mutate the config.

Done in d2951f1: validateKubeVirtConfig only rejects negative values now and no longer mutates the config. The zero fallback lives in kubeClientRateLimits alone. With that the two DefaultKubeClient* constants had no purpose left, so I removed them and SetDefault uses rest.DefaultQPS / rest.DefaultBurst directly.

0xFelix · 2026-11-13T15:14:20Z

+	QPS   float32 `mapstructure:"qps"`
+	Burst int     `mapstructure:"burst"`


Mention that changing these needs a restart? The config is hot-reloaded via watchConfigFile, but the client is only built once at startup. Same for the comment in helm/values.yaml.

Good catch, the client is created once in main and a config reload does not touch it. Added a note that a change needs a restart, both to the KubeVirtConfig comment and to helm/values.yaml (d2951f1).

0xFelix · 2026-11-13T15:14:20Z

+  # defaults saturate at ~15 BMHs. Watch the pod log for "Waited for ... due
+  # to client-side throttling"; if it appears under load, raise these


With client-go v0.34 the log line is "Waited before sending request" ... reason="client-side throttling..." (as in the PR description). Maybe just say to grep for client-side throttling?

Right, the line I quoted is the wording of older client-go versions. The comment now just says to grep the pod log for client-side throttling (d2951f1).

0xFelix · 2026-11-13T15:14:20Z

+// kubeClientRateLimits returns the client-go QPS and burst for the Kubernetes
+// API client. They come from kubevirt.qps / kubevirt.burst in the app config
+// (or KUBEVIRT_REDFISH_KUBEVIRT_QPS / _BURST). Without a config, or for a
+// value left at zero, client-go's own defaults apply. Large deployments need
+// higher values: every Redfish request fans out to several API calls, and with
+// many BMHs polling at once the default limiter queues requests long enough
+// for the OpenShift route to time out.


nit: The fan-out / many BMHs explanation is repeated in 4 places (here, KubeVirtConfig, the defaults const block and helm/values.yaml). Keep it in one spot?

Agreed. The explanation now lives only in helm/values.yaml, where an operator sizing the values will read it. The comments on kubeClientRateLimits and KubeVirtConfig are trimmed to what the code does (the struct comment points to the values file), and the constants block is gone (d2951f1).

…restart Review follow-up: - validateKubeVirtConfig only rejects negative kubevirt.qps / burst and no longer rewrites zero to the defaults; kubeClientRateLimits already falls back to client-go's defaults for zero, and SetDefault covers unset keys. - Note that the client is built once at startup, so a change to these values needs a restart even though the config file is hot-reloaded. - Point at the "client-side throttling" log text instead of quoting a log line whose wording differs between client-go versions. - Keep the fan-out / sizing explanation in helm/values.yaml only. Signed-off-by: itayherzberg <itayherzberg@gmail.com>

kubevirt-prow Bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Oct 21, 2026

kubevirt-prow Bot requested review from MarSik and yanirq October 21, 2026 16:53

itayherzberg mentioned this pull request Oct 21, 2026

fix(server): spread background jobs across workers; make the pool size configurable #31

Merged

kubevirt-prow Bot added the size/L label Oct 21, 2026

itayherzberg changed the title ~~feat(config): make the client-go rate limit configurable (kubevirt.qps, kubevirt.burst)~~ fix(config): make the client-go rate limit configurable (kubevirt.qps, kubevirt.burst) Oct 22, 2026

kubevirt-prow Bot requested a review from 0xFelix November 4, 2026 15:24

kubevirt-prow Bot assigned MarSik Nov 12, 2026

kubevirt-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2026

kubevirt-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2026

itayherzberg force-pushed the client-go-rate-limits branch from cdf6a51 to 1d7b14f Compare November 12, 2026 16:13

kubevirt-prow Bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2026

0xFelix reviewed Nov 13, 2026

View reviewed changes

		QPS float32 `mapstructure:"qps"`
		Burst int `mapstructure:"burst"`

		# defaults saturate at ~15 BMHs. Watch the pod log for "Waited for ... due
		# to client-side throttling"; if it appears under load, raise these

Conversation

itayherzberg commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Cause

Fix

Verification on the cluster

Tests

Related

Uh oh!

kubevirt-prow Bot commented Oct 21, 2026

Uh oh!

itayherzberg commented Nov 3, 2026

Uh oh!

aburdenthehand commented Nov 4, 2026

Uh oh!

MarSik commented Nov 12, 2026

Uh oh!

kubevirt-prow Bot commented Nov 12, 2026

Uh oh!

kubevirt-prow Bot commented Nov 12, 2026

Uh oh!

kubevirt-prow Bot commented Nov 12, 2026

Uh oh!

itayherzberg commented Nov 12, 2026

Uh oh!

kubevirt-prow Bot commented Nov 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

itayherzberg commented Oct 21, 2026 •

edited

Loading