[itayherzberg]

Problem

When Ironic (metal3) provisions a VM-backed BareMetalHost it sends InsertMedia, then the boot-device PATCH (BootSourceOverrideEnabled: Once, target Cd) about 35 ms later, then the power-on. Some cycles leave the VirtualMachine with the cdrom0 volume but without the cdrom0 disk entry:

disks:
- name: rootdisk            # cdrom0 disk entry gone
  bootOrder: 1
volumes:
- name: rootdisk
- name: cdrom0              # volume survived
  persistentVolumeClaim: {claimName: ...-bootiso-...}

KubeVirt attaches a volume that has no disk entry as a writable virtio disk. QEMU opens the ISO read-write, the file belongs to the import helper with mode 644, so the launcher fails with Could not open '.../cdrom0/disk.img': Permission denied and the VM goes CrashLoopBackOff, forever, because the half-state is persisted in the VM spec. Ironic sees the host powered off, inspection times out and retries, and every retry (eject, insert, boot override) rolls the dice again, about every 35 minutes per host. In one deployment 4 of 6 VMs were corrupted within a day. The only repair was a manual oc patch adding the disk entry back, and nothing stopped the next retry from corrupting the VM again.

Cause

Two writers change the same VM at the same time:

• handleInsertVirtualMedia answers 202 and queues a job; a task-manager worker runs InsertVirtualMedia a little later.
• handleBootUpdate runs RecordVMBootOptionsAsAnnotations and then SetBootOnce synchronously on the HTTP goroutine.

Every VM write in pkg/kubevirt/client.go was GET, DeepCopy, edit, computeVMPatch (a JSON merge patch of the difference), Patch. Two properties of that pattern combine into the bug:

1. The patch never contained metadata.resourceVersion (both sides of the diff come from the same read), so the API server could not tell that the patch was computed from a stale object. The last writer wins.
2. A JSON merge patch (RFC 7386) replaces arrays wholesale. modifyVmBootOrder stamps bootOrder on every disk, so SetBootOnce ships the complete disks array from its own read.

When the boot-once read happens before the insert's patch lands, and the boot-once patch lands after it, the disks array from before the insert overwrites the one that has cdrom0. That patch does not mention volumes, so the volume survives. The retry loop in InsertVirtualMedia that looked for "the object has been modified" could never trigger: no patch carried a version.

Reproduced on OpenShift 4.22 / OpenShift Virtualization 4.22.12 with the stock kubevirt-redfish-rhel9:v4.22 image, whose log shows the same write pattern and the same tell-tale lines as this code on main, three VMs and an Ironic-style driver sending eject, insert and boot override 35 to 150 ms apart: 36 cycles, 3 corrupted. Bridge log of one of them (race-vm-2); the boot-once read saw no CD-ROM (order: 1) and its patch landed last:

17:51:18.540Z Creating CD-ROM device cdrom0 in VM spec first          ← insert reads the VM
17:51:19.499Z Added new CD-ROM device cdrom0
17:51:20.295Z Set disk rootdisk as secondary boot device (order: 1)   ← boot-once read: 0 CD-ROMs (stale)
17:51:20.535Z Successfully created CD-ROM device cdrom0 in VM spec    ← insert patch lands
17:51:21.333Z Successfully set boot once to Cd for VM redfish-vms/race-vm-2   ← stale disks array lands on top

race-vm-2  disks=[rootdisk:1]  volumes=[rootdisk,cdrom0]   CORRUPTED (volume without disk)

Unit test added by this PR, run against current main (the mock dynamic client applies RFC 7386 merge patches and now checks resourceVersion the way the API server does):

--- FAIL: TestSetBootOnce_StaleRead_RetriesOnFreshCopy (0.00s)
    vm_update_test.go:296: VM vm: device cdrom0 has volume=true disk=false, must have both or neither
    vm_update_test.go:296: VM vm: volume cdrom0 has no disk entry; KubeVirt would attach it as a writable hard disk
    vm_update_test.go:297: disk cdrom0: boot order <no disk>, want 1
    vm_update_test.go:298: disk rootdisk: boot order 1, want 2
    vm_update_test.go:300: expected the rejected patch and one retry, got 1 patch attempts
--- FAIL: TestConcurrentVMWriters_ConvergeRegardlessOfOrder (0.01s)
    vm_update_test.go:503: VM vm-5: device cdrom0 has volume=true disk=false, must have both or neither
    vm_update_test.go:503: VM vm-7: device cdrom0 has volume=true disk=false, must have both or neither
    ...

The same race has a milder outcome when the boot-once patch lands first: the insert's patch then overwrites the boot-once's bootOrder stamps, or the boot-once never saw the CD-ROM at all. Either way the root disk stays first and the CD-ROM has no boot order (18 of the 36 unfixed cycles ended like that). An empty root disk boots the ISO anyway because there is nothing else to boot; a provisioned one does not.

Fix

All in pkg/kubevirt.

• One path for every read-modify-write: updateVM (vm_update.go). It takes a per-VM lock, does a live GET, runs the caller's mutation on a copy, computes the merge patch and patches. The lock (vmlock.go, keyed by namespace/name, entries dropped as soon as nobody holds or waits for them) serializes the writers inside the process: HTTP handlers, task workers and the boot-once watcher. Different VMs never wait for each other, so the worker pool keeps its throughput. Only the spec change runs under the lock; PVC creation, the helper pod and the ISO download stay outside it.
• The patch carries the resourceVersion it was computed from (computeVMPatch). The API server then answers a write based on a stale read with 409 Conflict instead of applying it, and updateVM re-runs the mutation on a fresh read (5 attempts, 50 to 500 ms backoff). This also covers writers outside the process: oc edit, a second instance, KubeVirt's own controllers. A mutation that changes nothing does not write at all.
• Routed through it: RecordVMBootOptionsAsAnnotations, the spec step of InsertVirtualMedia, SetBootOrder, SetBootOnce, EjectVirtualMedia and the boot-order restore in the watcher (handleVMUpdate). Each mutation is a closure that takes every decision from the fresh copy; the insert generates the PVC name once, outside the retry, so a retried insert keeps the same claim. The dead "object has been modified" loop is gone.
• Left alone on purpose: setImportingLabel, SetPowerAfterImportLabel, removeImportingLabel, the label removal in handlePowerAfterImport, and SetVMPowerState (RFC 6902 replace /spec/runStrategy). These are blind single-key patches that touch no array; the API server applies them atomically and they can neither clobber nor be clobbered. Also not done: patching single disk entries by index, because indices shift on insert and eject; a whole-array patch from a fresh, version-checked read is correct.
• Both request orders give the same VM. An insert that finds a boot-once to CD already recorded (label and saved original boot order present) places the new CD-ROM first and adds it to the saved original order with no boot order of its own, so the restore after the one-time boot clears it instead of leaving two disks at position 1. It does not reorder when only the override annotations are present, the short window between the two writes of handleBootUpdate, because SetBootOnce would then save the reordered disks as the original.
• Self-heal. The VM watcher already sees every VM event and sweeps labeled VMs once a minute. It now re-adds the missing disk entry for any inserted device (virtual-media.redfish/<device> annotation) whose volume is present, and applies the pending CD boot override. VMs corrupted by earlier builds are repaired on the next event or sweep, with no new goroutine. The opposite half-state (disk without volume) is only logged.
• Mock dynamic client (mock_dynamic_client.go): patches are applied atomically under the store lock, every write bumps resourceVersion, a merge patch with a stale resourceVersion gets a Conflict, not-found is a typed NotFound; MutateVM and a BeforePatch hook let a test put another writer between a read and its write; IgnoreResourceVersion reproduces the old behaviour for the test that shows the bug.

Behaviour changes: a VM write that changes nothing no longer touches the API server; an insert reorders the boot devices when a boot-once to CD is already recorded; the watcher repairs a volume-without-disk half-state; after the retry limit a write fails with the Conflict error instead of silently overwriting.

Verification on the cluster

Same cluster and driver, this branch built from the tree and deployed next to the stock bridge:

• 36 cycles, 0 corrupted. Every cycle ended with disks=[rootdisk:2,cdrom0:1], volume and disk paired, whichever request reached the VM first. In 21 of the 36 cycles the boot override took the VM lock first and its read saw no CD-ROM (order: 1), the ordering that used to corrupt or leave the CD-ROM unordered; in each of them the insert that followed placed the CD-ROM first. The rest went insert first and the boot-once saw the CD-ROM. The lock means two writers inside the bridge never work from a stale read any more; the resourceVersion check fired 3 times during the run, each time against a writer outside the bridge (KubeVirt's own controllers updating a VM right after it was created or ejected), and each write succeeded on the retry:

18:02:09.864Z VM redfish-vms/race-vm-3 changed while an update was being prepared, retrying on a fresh copy (attempt 1/5)

• A cycle where the boot override reached the VM first, the order that used to leave the CD-ROM without a boot order:

18:01:11.884Z Set disk rootdisk as secondary boot device (order: 1)        ← boot-once, no CD-ROM yet
18:01:11.911Z Successfully set boot once to Cd for VM redfish-vms/race-vm-1
18:01:11.917Z Boot override to CD is pending for VM redfish-vms/race-vm-1, placing cdrom0 first in the boot order
18:01:11.917Z Set CD-ROM cdrom0 as primary boot device (order: 1)
18:01:12.424Z Successfully created CD-ROM device cdrom0 in VM spec

race-vm-1  disks=[rootdisk:2,cdrom0:1]  volumes=[rootdisk,cdrom0]   intact

• Self-heal: a VM created directly in the corrupted state (volume, annotation and boot-once to CD present, no disk entry) was repaired by the watcher within 5 s: Repaired VM redfish-vms/race-vm-4: re-added the missing CD-ROM disk entry for virtual media device(s) [cdrom0]; result disks=[rootdisk:2,cdrom0:1], saved original order [{rootdisk,1},{cdrom0}]. The VM corrupted by the stock bridge in the first run was repaired the same way seconds after the fixed bridge started.
• The precondition itself, checked on the API server before relying on it: oc patch vm --type=merge with a stale metadata.resourceVersion → Error from server (Conflict): ... the object has been modified; please apply your changes to the latest version and try again; with the current one → patched.

Tests

pkg/kubevirt/vm_update_test.go: the patch carries the resourceVersion and an unchanged VM yields an empty patch; the mock rejects a stale resourceVersion and accepts a blind patch; the production interleaving corrupts the VM without the precondition (IgnoreResourceVersion) and is retried on a fresh copy with it (fails on current main as shown above); updateVM gives up after vmUpdateMaxAttempts conflicts and never writes when nothing changed; a retried insert keeps its PVC name and another writer's annotation; an insert after a recorded boot-once puts the CD first and the later restore clears it; an insert between the override annotations and the boot-once state does not reorder; 24 VMs running insert, boot-option annotations and boot-once concurrently, then eject and boot-once, all converge, with no lock entries left behind; the watcher repairs a missing disk entry, leaves healthy and unmanaged VMs alone, and repairs then restores in one pass. pkg/kubevirt/vmlock_test.go: same key serializes, different keys do not block, entries are dropped.

go build ./..., go vet ./..., go test ./..., gofmt, go mod tidy (no change) pass; go test -race ./pkg/kubevirt/ passes. go test -race ./... reports data races in pkg/server and cmd that exist on main before this PR (88 failing tests there, 86 on this branch, none in pkg/kubevirt); not touched here.

Related

• feat(kubevirt): serve read-only VM/VMI lookups from an informer cache #32 (VM/VMI informer cache) also adds fields to the Client struct, in a different place, so the two merge cleanly in either order. feat(kubevirt): serve read-only VM/VMI lookups from an informer cache #32 keeps GetVM live and moves only read-only lookups to its cache, which is exactly what updateVM needs, so neither PR changes the other.
• fix(server): spread background jobs across workers; make the pool size configurable #31 (round-robin workers) makes the insert job start sooner after the request and so makes this race more likely; the two belong in the same release.
• fix(kubevirt): a failed ISO import must not start the VM, read as inserted, or block a re-insert #39 adds one annotation delete inside EjectVirtualMedia, which this PR rewrites around updateVM; the line moves into the closure, a one-line rebase for whichever lands second.
• fix(server): Worker.Stop blocked forever, so shutdown waited for SIGKILL #29, fix(config): make the client-go rate limit configurable (kubevirt.qps, kubevirt.burst) #30, 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(server): a power-off is never deferred by a pending ISO import, and withdraws a deferred power-on #37, fix(logger): do not log the virtual media image URL (it carries the image service token) #38: 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]

We can keep this, but I wonder if Ironic should check the device was inserted first as well. Because that is what HTTP 202 means. It does not guarantee the operation was finished yet.

[itayherzberg]

Agreed that a client should treat 202 as "accepted, not done". Ironic can do that check since 2026-09 (openstack/ironic c537ed92, bug 2166702, release note, first in 39.0.0), but it is opt-in via [redfish] enable_vmedia_insert_polling, default off, and neither metal3-io/ironic-image nor openshift/ironic-image sets it. sushy itself posts with blocking=False, so it ignores the 202 and goes straight to the boot-device PATCH. So the Ironic that corrupted these VMs cannot be configured to wait, and other clients (curl, a second replica, oc edit, KubeVirt's own controllers) would not either. The lock and the resourceVersion precondition are the actual fix; this block only makes both request orders end in the same VM spec.

Two follow-ups outside this PR: the 202 responses carry no Location header pointing at the Task, which the spec expects; and if someone enables the Ironic polling, Inserted here only turns true once the PVC is Bound, so with WaitForFirstConsumer storage Ironic's 60 s default could time out before the import starts.

[MarSik]

This looks good to me and the reasoning makes sense, but I will have to test it with our local use cases first before approving.

[itayherzberg]

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

[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