[itayherzberg]

Problem

Every Redfish read goes to the API server, several times over. With about 15 BareMetalHosts being polled by Ironic at the same time, reads queued behind the client-go rate limiter and 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

A GET /redfish/v1/Systems/{id} is 6 Kubernetes API calls: one in resolveSystemVMandCheckAccess to find the VM, then the same VM four more times (power state, boot options, storage, memory/CPU) and the VMI once. A boot-device PATCH is 12, and Ironic's power poll of a stopped host is 6. Every one of them fetches an object that rarely changes, through a client-go limiter of 5 requests/second.

Measured on upstream main behaviour (first revision of this branch with the cache turned off), 100 × GET /Systems/{id}, 20 in parallel, OpenShift 4.22 / OpenShift Virtualization 4.22.12:

n=100 avg=19.16s max=21.32s codes: 200=100       (wall clock 100s)
client metrics delta: {"GetVM":501,"GetVM.direct":500}   ← 5 VM GETs per request
10 × "Waited before sending request … reason="client-side throttling"

Fix

For every namespace in the chassis config the client keeps a client-go informer (a watch plus a local copy) of VirtualMachines and one of VirtualMachineInstances, with managedFields stripped. Read-only lookups read the local copy instead of calling the API server: power state, boot options, CPU, memory, storage, network, virtual-media state, the Systems/Chassis/Managers collections, and the per-request lookup in resolveSystemVMandCheckAccess. The informers start and stop with the existing watchers (RestartWatchers / Close) and are rebuilt on config reload.

Redfish request	API calls before	after
GET /Systems/{id}	6	0
Boot-device PATCH /Systems/{id}	12	7
Ironic's power poll of a stopped VM	6	0

Two rules keep cached reads correct (both in pkg/kubevirt/cache.go):

1. Anything that reads a VM in order to change it, or reads it back right after changing it, does not use the cache. SetBootOrder, SetBootOnce, virtual-media insert/eject, power actions and the import labels still read live, exactly as before, so a patch is never computed from a stale copy. The boot-device PATCH handler builds its response with GetVMBootOptionsLive, a direct GET, so the body it returns to Ironic reflects the write it just made. No other write path reads through a cached getter. GetVM / GetVMI are unchanged; the cached variants are getVMForRead / getVMIForRead (GetVMForRead exported for the server package).
2. The cache is either fully trusted or not used. Until an informer finishes its first list, reads for that namespace go to the API server. After that its answer is final, including "not found". The alternative, a GET on every miss, would cost a GET on every power poll of a stopped VM (no VMI), the normal state of a host during inspection.

What the cache does not guarantee: a write, by someone else (oc edit vm, KubeVirt's own controllers) or by this process, reaches the cache with its watch event, usually within milliseconds and later when the API server is overloaded. Until then a cached read returns the previous version. This is acceptable because no write path reads from the cache: a stale read can make a response out of date, but can never be written back over someone's change.

Observability: GetMetrics counts GetVM.cache / GetVM.direct (and the VMI and List equivalents); startup logs VM cache started for namespaces […]. RBAC: the informers need list and watch on virtualmachines and virtualmachineinstances; the chart's ClusterRole already grants both. go.mod gains one indirect test-only dependency pulled in by k8s.io/client-go/dynamic/fake.

Behaviour change: read-only lookups are served from the cache. There is no off switch.

Verification on the cluster

Measured on the first revision of this branch, which differed only by a write-tracking layer that has since been removed (see the second commit) and an off switch. Same load, cache on, same default rate limiter:

VM cache started for namespaces [redfish-prtest-vms]
n=100 avg=0.83s max=1.17s codes: 200=100         (wall clock 4s, 0 throttling lines)
client metrics delta: {"GetVM.cache":500}         ← 0 API calls

Check	Result
Power on, PowerState polled every 0.3 s	Off → PoweringOn → On, matches oc get vmi
ForceOff	ForceOffInProgress → Off
External write: oc patch CPU cores 1 → 2	ProcessorSummary.Count: 2 through Redfish ~2 s later
Own write: boot-device PATCH	response body shows Once / Cd; matches the VM annotations
Metrics over all of the above	GetVM.direct: 20 (the write paths, by design), GetVM.cache: 227

Earlier rounds of the same code on the fork, with API calls counted from the kube-apiserver audit log, also covered the initial sync under the chart's ClusterRole (one list + watch per namespace for VMs and VMIs, no forbidden errors), pause/resume (Paused within ~1 s from the cached VMI), virtual-media insert (Inserted: true ~3 s after the insert) and eject (false on the first read after it), a pod restart with one VM paused, 8 rounds of On/ForceOff with 20 reads each (100 % cached, 0 warnings), and a user of chassis A reading a system of chassis B → 404.

Tests

pkg/kubevirt/cache_test.go, against k8s.io/client-go/dynamic/fake: a read is served from the cache even when the API server's GET is made to fail; a synced cache's "not found" is final; a namespace the cache does not cover falls back to the API server; GetVMBootOptionsLive returns the API server's version while the cached getter still returns the older one; the cache can be restarted with a different namespace set and stopped without deadlocking, and does not start on a client without a dynamic client; filterVMNames sorts and filters.

go build ./..., go vet ./..., go test ./..., gofmt, go mod tidy and helm lint pass.

Related

One of nine PRs from running many concurrent BMHs against kubevirt-redfish (#29–#33, #36–#39). None is stacked on another. This branch merges cleanly with each of the other eight, and a tree with all nine builds and passes the tests after two fixups that do not involve this PR (#30 and #31 add the same test helpers; #33 and #39 touch one line in EjectVirtualMedia).

• fix(kubevirt): concurrent virtual-media insert and boot override could drop the CD-ROM disk from the VM spec #33 (serialized VM writes) also adds fields to the Client struct. This PR keeps its fields next to the metrics block, so the two merge in either order. fix(kubevirt): concurrent virtual-media insert and boot override could drop the CD-ROM disk from the VM spec #33 serializes the write paths; this PR only moves read-only lookups to the cache and keeps GetVM live, so the two do not interact at runtime.
• fix(server): Worker.Stop blocked forever, so shutdown waited for SIGKILL #29 (shutdown hang): until it merges, Close() never runs in a pod, so the cache's stop path is exercised by unit tests only.
• fix(config): make the client-go rate limit configurable (kubevirt.qps, kubevirt.burst) #30 (client-go rate limit), fix(server): spread background jobs across workers; make the pool size configurable #31 (task worker pool), fix(server): Power On right after InsertMedia started the VM before its ISO import (empty CD-ROM, import pod stuck on the RWO volume) #36–fix(kubevirt): a failed ISO import must not start the VM, read as inserted, or block a re-insert #39 (ISO import fixes): no overlap.

[kubevirt-prow]

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.

[MarSik]

I think this reimplements too much. The client-go project already has informer implementation available and reimplementing it all is not a good idea maintenance wise.

See for example https://oneuptime.com/blog/post/2026-02-09-client-go-informers-watch-resources/view (random google result).

Also I think we can make the use of informers mandatory, saving api server requests is a good idea.

[MarSik]

Or maybe not quite "reimplements", it uses the informer tooling, but it does that too complicately. I think it would be enough to just use informers for few specific CRDs and not construct a complicated dynamic cache.

[itayherzberg]

Thanks, both points taken. I pushed the change as a second commit rather than amending, so the delta since your review is visible.

Mandatory cache. The cache_enabled setting is gone, together with its config, helm and test plumbing. Informers always run for the configured namespaces. The only fallback to a direct GET is while an informer has not finished its initial list.

The complexity. What made cache.go 600 lines was not the informers but a write-tracking layer around them: reads of an object bypassed the cache until the informer had stored the resourceVersion our own write returned. It existed for one real case. handleBootUpdate writes the VM and then reads it back through the cache to build the PATCH response, so with plain lister reads that body can show the pre-write boot options to Ironic whenever the watch event lags the write. I replaced the tracker with a live read at that one point (GetVMBootOptionsLive, one GET per PATCH) and checked that no other write path reads through a cached getter. What that trades away is the next request landing in the few milliseconds before the watch event arrives, which is the same window the PR already accepts for writes by oc or virt-controller.

cache.go is now the textbook shape, about 280 lines with comments: a per-namespace dynamicinformer factory with one informer for VMs and one for VMIs, SetTransform to drop managedFields, listers behind HasSynced, and the restart/stop hooks tied to the existing watchers.

"Informers for a few specific CRDs." I read that as exactly these two, VM and VMI. They are dynamic informers because the repo has no kubevirt.io/client-go dependency and uses the dynamic client throughout. If you meant typed KubeVirt informers instead, tell me and I will switch; that would add kubevirt.io/client-go to go.mod.

[itayherzberg]

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

[MarSik]

@itayherzberg I did some initial testing already, but it is a lot of patches so give me some more time please. One question from my side would be if the patches should be applied in some specific ordering. Just to avoid needless conflicts.

[itayherzberg]

@MarSik Thanks for testing, and take whatever time you need.

On ordering, across all nine (#29–#33, #36–#39): I merged every pair onto current main and the full set together. No branch is stacked on another, so there is no required order. With the two fixups below, the combined tree builds and passes all tests, go vet, helm lint and helm template.

Since your question I rebased #32 and #33 onto current main and moved #32's new struct fields so those two no longer collide, which was the one conflict #33's description used to mention. I also updated the Related sections of #32 and #33 to match what is below. Two spots still touch:

1. fix(config): make the client-go rate limit configurable (kubevirt.qps, kubevirt.burst) #30 + fix(server): spread background jobs across workers; make the pool size configurable #31: both add the same two small test helpers (minimalConfig, writeConfig) to pkg/config/config_test.go. Git merges it silently but go vet then fails on the redeclaration. Whichever lands second gets a one-line fixup. This one was not in either description, sorry.
2. fix(kubevirt): concurrent virtual-media insert and boot override could drop the CD-ROM disk from the VM spec #33 + fix(kubevirt): a failed ISO import must not start the VM, read as inserted, or block a re-insert #39: fix(kubevirt): a failed ISO import must not start the VM, read as inserted, or block a re-insert #39 adds one annotation delete in EjectVirtualMedia, which fix(kubevirt): concurrent virtual-media insert and boot override could drop the CD-ROM disk from the VM spec #33 rewrites around updateVM. The line just moves inside the closure. Noted in both descriptions now.

Tell me which one you plan to take next and I will rebase the other right away so it is clean when you get to it; otherwise I will do it the same day the first one merges.

Not merge constraints, but useful for testing: #36, #37 and #39 are one set (power deferral around ISO imports) and were verified together on the fork, so they are best in the same release; #33's description notes it and #31 belong together too; and #29 before #32 is only a soft preference, since #32's cache shutdown path runs in a pod only once #29 fixes Worker.Stop.

Also, the CI runs for #32, #33 and #36–#39 are waiting for a maintainer to approve the workflow run, in case you want them green before looking further.

[MarSik]

/lgtm
/approve

[kubevirt-prow]

[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