From 5e093c24fff3808dc369cfa1caeb7ccf0380160e Mon Sep 17 00:00:00 2001 From: itayherzberg Date: Sun, 25 Oct 2026 19:57:55 +0200 Subject: [PATCH 1/2] fix(kubevirt): a failed ISO import must not start the VM or read as inserted When the import helper pod failed, the importing label was removed and nothing else changed: the VM watcher then ran the deferred power-on and the VM booted from a CD-ROM whose PVC holds no image, GET VirtualMedia reported Inserted true because the PVC is bound, and a repeated InsertMedia with the same image was answered as already inserted. A failed import now removes the importing label, withdraws the deferred power command and marks the device with import-failed.redfish/ in one patch, so the watcher never sees the label gone with the power command still pending. The marker makes the device read as not inserted and lets the next InsertMedia import again; the insert and the eject clear it. A device whose import is still running also reads as not inserted. Signed-off-by: itayherzberg --- pkg/kubevirt/client.go | 64 +++++++ pkg/kubevirt/import_failed_test.go | 284 +++++++++++++++++++++++++++++ 2 files changed, 348 insertions(+) create mode 100644 pkg/kubevirt/import_failed_test.go diff --git a/pkg/kubevirt/client.go b/pkg/kubevirt/client.go index 329dcfc..e803316 100644 --- a/pkg/kubevirt/client.go +++ b/pkg/kubevirt/client.go @@ -1558,6 +1558,17 @@ func (c *Client) IsVirtualMediaInserted(namespace, name, mediaID string) (bool, return false, nil } + // A bound PVC holds no image while the import is running or after it + // failed; the device is not usable media in either case. + if vm.GetLabels()[ImportingLabelPrefix+mediaID] != "" { + logger.Debug("Virtual media %s of VM %s/%s is still importing", mediaID, namespace, name) + return false, nil + } + if failedPod := vm.GetAnnotations()[ImportFailedAnnotationPrefix+mediaID]; failedPod != "" { + logger.Debug("Virtual media %s of VM %s/%s has a failed import (%s)", mediaID, namespace, name, failedPod) + return false, nil + } + // Step 4: Check if the PVC is bound and has non-zero size pvcObj, err := c.kubernetesClient.CoreV1().PersistentVolumeClaims(namespace).Get(context.Background(), pvcName, metav1.GetOptions{}) if err != nil { @@ -1661,6 +1672,12 @@ func (c *Client) CheckInsertedMedia(namespace, name, mediaID, imageURL string) ( return MediaStateNone, fmt.Errorf("failed to get VM %s/%s: %w", namespace, name, err) } + // After a failed import the attached PVC holds no image; a new insert + // replaces it, so there is nothing to compare the request against. + if vm.GetAnnotations()[ImportFailedAnnotationPrefix+mediaID] != "" { + return MediaStateNone, nil + } + // Check if the device has a PVC volume attached hasPVC := false if vm.Spec.Template != nil { @@ -1806,6 +1823,7 @@ func (c *Client) insertVirtualMediaAsync(namespace, name, mediaID, imageURL stri annotations = map[string]string{} } annotations[VirtualMediaURLAnnotationPrefix+deviceName] = imageURL + delete(annotations, ImportFailedAnnotationPrefix+deviceName) vmCopy.SetAnnotations(annotations) // Ironic sends the boot override right after the insert request. When @@ -2462,6 +2480,7 @@ func (c *Client) EjectVirtualMedia(namespace, name, mediaID string) error { annotations := vmCopy.GetAnnotations() delete(annotations, VirtualMediaURLAnnotationPrefix+mediaID) + delete(annotations, ImportFailedAnnotationPrefix+mediaID) vmCopy.SetAnnotations(annotations) return nil }) @@ -2908,6 +2927,12 @@ const reconcileInterval = time.Minute // inserted virtual media. The device name is appended (e.g. "virtual-media.redfish/cdrom0"). const VirtualMediaURLAnnotationPrefix = "virtual-media.redfish/" +// ImportFailedAnnotationPrefix marks a device whose last ISO import failed +// (value: the import pod). The PVC is attached but holds no usable image, so +// the device reads as not inserted and the next InsertMedia imports again. +// The insert and the eject clear it. +const ImportFailedAnnotationPrefix = "import-failed.redfish/" + // MediaState describes the state of an already-inserted virtual media device. type MediaState int @@ -3571,11 +3596,50 @@ func (c *Client) handleImportPodCompleted(pod *corev1.Pod) { return } + if pod.Status.Phase == corev1.PodFailed { + if err := c.markImportFailed(vm, deviceName, pod.Name); err != nil { + logger.Error("Failed to record the failed import on VM %s/%s: %v", vmNamespace, vmName, err) + } + return + } + if err := c.removeImportingLabel(vmNamespace, vmName, deviceName); err != nil { logger.Error("Failed to remove importing label from VM %s/%s: %v", vmNamespace, vmName, err) } } +// markImportFailed ends a failed import: the importing label comes off, a +// power command deferred until the import is withdrawn, and the device is +// marked as failed. All three go in one patch: the VM watcher runs the +// deferred power command as soon as it sees the label gone, and a VM started +// on an empty CD-ROM is what the deferral exists to prevent. +func (c *Client) markImportFailed(vm *kubevirtv1.VirtualMachine, deviceName, podName string) error { + namespace, name := vm.GetNamespace(), vm.GetName() + deferredPower := vm.GetLabels()[PowerAfterImportLabel] + + patchJSON := fmt.Sprintf(`{"metadata":{"labels":{%q:null,%q:null},"annotations":{%q:%q}}}`, + ImportingLabelPrefix+deviceName, PowerAfterImportLabel, ImportFailedAnnotationPrefix+deviceName, podName) + + gvr := schema.GroupVersionResource{ + Group: "kubevirt.io", + Version: "v1", + Resource: "virtualmachines", + } + ctx, cancel := context.WithTimeout(context.Background(), c.timeout) + defer cancel() + + _, err := c.dynamicClient.Resource(gvr).Namespace(namespace).Patch(ctx, name, types.MergePatchType, []byte(patchJSON), metav1.PatchOptions{}) + if err != nil { + return fmt.Errorf("failed to patch VM %s/%s: %w", namespace, name, err) + } + if deferredPower != "" { + logger.Warning("Import of %s into VM %s/%s failed (pod %s); dropped the deferred power command %s", deviceName, namespace, name, podName, deferredPower) + } else { + logger.Warning("Import of %s into VM %s/%s failed (pod %s); the device reads as not inserted until it is inserted again", deviceName, namespace, name, podName) + } + return nil +} + // removeImportingLabel removes the importing.redfish/ label from a VM func (c *Client) removeImportingLabel(namespace, vmName, deviceName string) error { labelKey := ImportingLabelPrefix + deviceName diff --git a/pkg/kubevirt/import_failed_test.go b/pkg/kubevirt/import_failed_test.go new file mode 100644 index 0000000..761b9b5 --- /dev/null +++ b/pkg/kubevirt/import_failed_test.go @@ -0,0 +1,284 @@ +package kubevirt + +import ( + "context" + "sync/atomic" + "testing" + "time" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/dynamic" + "k8s.io/client-go/kubernetes/fake" + kubevirtv1 "kubevirt.io/api/core/v1" +) + +const ( + failedTestPod = "copy-iso-test-vm-bootiso-1" + failedTestLabel = ImportingLabelPrefix + "cdrom0" + failedTestMarker = ImportFailedAnnotationPrefix + "cdrom0" + failedTestURLAn = VirtualMediaURLAnnotationPrefix + "cdrom0" +) + +// patchCounter counts the patches that reach the dynamic client, so a test +// can show that a state change is one write and not a sequence. +type patchCounter struct { + dynamic.Interface + patches int32 +} + +func (p *patchCounter) Resource(gvr schema.GroupVersionResource) dynamic.NamespaceableResourceInterface { + return &countingNamespaceable{p.Interface.Resource(gvr), p} +} + +type countingNamespaceable struct { + dynamic.NamespaceableResourceInterface + p *patchCounter +} + +func (c *countingNamespaceable) Namespace(ns string) dynamic.ResourceInterface { + return &countingResource{c.NamespaceableResourceInterface.Namespace(ns), c.p} +} + +type countingResource struct { + dynamic.ResourceInterface + p *patchCounter +} + +func (c *countingResource) Patch(ctx context.Context, name string, pt types.PatchType, data []byte, opts metav1.PatchOptions, subresources ...string) (*unstructured.Unstructured, error) { + atomic.AddInt32(&c.p.patches, 1) + return c.ResourceInterface.Patch(ctx, name, pt, data, opts, subresources...) +} + +func failedImportPod(phase corev1.PodPhase) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: failedTestPod, + Namespace: "test-ns", + Labels: map[string]string{ + ImportPodVMLabel: "test-vm", + ImportPodVolumeLabel: "cdrom0", + }, + }, + Status: corev1.PodStatus{Phase: phase}, + } +} + +func withCDROMVolume(vm *kubevirtv1.VirtualMachine, claim string) *kubevirtv1.VirtualMachine { + vm.Spec.Template.Spec.Domain.Devices.Disks = append(vm.Spec.Template.Spec.Domain.Devices.Disks, kubevirtv1.Disk{ + Name: "cdrom0", + DiskDevice: kubevirtv1.DiskDevice{CDRom: &kubevirtv1.CDRomTarget{Bus: kubevirtv1.DiskBusSATA}}, + }) + vm.Spec.Template.Spec.Volumes = append(vm.Spec.Template.Spec.Volumes, kubevirtv1.Volume{ + Name: "cdrom0", + VolumeSource: kubevirtv1.VolumeSource{ + PersistentVolumeClaim: &kubevirtv1.PersistentVolumeClaimVolumeSource{ + PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{ClaimName: claim}, + }, + }, + }) + return vm +} + +func boundPVC(name string) *corev1.PersistentVolumeClaim { + return &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "test-ns"}, + Status: corev1.PersistentVolumeClaimStatus{ + Phase: corev1.ClaimBound, + Capacity: corev1.ResourceList{"storage": resource.MustParse("1Gi")}, + }, + } +} + +// A failed import must not let the deferred power-on start the VM on an +// empty CD-ROM, and the change must be a single write because the VM watcher +// acts on the first event that shows the importing label gone. +func TestHandleImportPodCompleted_FailedImportDropsDeferredPowerOn(t *testing.T) { + mockDynamic := NewMockDynamicClient() + halted := kubevirtv1.RunStrategyHalted + vm := newTestVM("test-ns", "test-vm", map[string]string{ + failedTestLabel: failedTestPod, + PowerAfterImportLabel: "On", + }, map[string]string{failedTestURLAn: "http://example.com/boot.iso"}) + vm.Spec.RunStrategy = &halted + mockDynamic.AddVM(vm) + counter := &patchCounter{Interface: mockDynamic} + fakeK8s := fake.NewSimpleClientset(failedImportPod(corev1.PodFailed)) + client := NewClientWithClients(fakeK8s, counter, 30*time.Second, nil) + + if err := client.reconcileExistingImportPods("test-ns"); err != nil { + t.Fatalf("reconcileExistingImportPods failed: %v", err) + } + + updated, _ := mockDynamic.GetVM("test-ns", "test-vm") + labels, annotations := updated.GetLabels(), updated.GetAnnotations() + if got, found := labels[failedTestLabel]; found { + t.Errorf("importing label still set: %q", got) + } + if got, found := labels[PowerAfterImportLabel]; found { + t.Errorf("deferred power command still set: %q", got) + } + if got := annotations[failedTestMarker]; got != failedTestPod { + t.Errorf("annotation %s = %q, want %q", failedTestMarker, got, failedTestPod) + } + if got := annotations[failedTestURLAn]; got != "http://example.com/boot.iso" { + t.Errorf("URL annotation was changed: %q", got) + } + if got := updated.Spec.RunStrategy; got == nil || *got != kubevirtv1.RunStrategyHalted { + t.Errorf("runStrategy = %v, want Halted", got) + } + if n := atomic.LoadInt32(&counter.patches); n != 1 { + t.Errorf("VM was patched %d times, want exactly 1", n) + } + if _, err := fakeK8s.CoreV1().Pods("test-ns").Get(context.Background(), failedTestPod, metav1.GetOptions{}); err == nil { + t.Error("failed import pod should have been deleted") + } +} + +func TestHandleImportPodCompleted_SucceededImportKeepsDeferredPowerOn(t *testing.T) { + mockDynamic := NewMockDynamicClient() + mockDynamic.AddVM(newTestVM("test-ns", "test-vm", map[string]string{ + failedTestLabel: failedTestPod, + PowerAfterImportLabel: "On", + }, nil)) + fakeK8s := fake.NewSimpleClientset(failedImportPod(corev1.PodSucceeded)) + client := NewClientWithClients(fakeK8s, mockDynamic, 30*time.Second, nil) + + if err := client.reconcileExistingImportPods("test-ns"); err != nil { + t.Fatalf("reconcileExistingImportPods failed: %v", err) + } + + updated, _ := mockDynamic.GetVM("test-ns", "test-vm") + if _, found := updated.GetLabels()[failedTestLabel]; found { + t.Error("importing label should have been removed") + } + if got := updated.GetLabels()[PowerAfterImportLabel]; got != "On" { + t.Errorf("deferred power command = %q, want On (the watcher runs it)", got) + } + if got, found := updated.GetAnnotations()[failedTestMarker]; found { + t.Errorf("a successful import must not set %s (got %q)", failedTestMarker, got) + } +} + +func TestHandleImportPodCompleted_StaleFailedPodIsIgnored(t *testing.T) { + mockDynamic := NewMockDynamicClient() + mockDynamic.AddVM(newTestVM("test-ns", "test-vm", map[string]string{ + failedTestLabel: "copy-iso-test-vm-bootiso-2", + PowerAfterImportLabel: "On", + }, nil)) + fakeK8s := fake.NewSimpleClientset(failedImportPod(corev1.PodFailed)) + client := NewClientWithClients(fakeK8s, mockDynamic, 30*time.Second, nil) + + if err := client.reconcileExistingImportPods("test-ns"); err != nil { + t.Fatalf("reconcileExistingImportPods failed: %v", err) + } + + updated, _ := mockDynamic.GetVM("test-ns", "test-vm") + if got := updated.GetLabels()[failedTestLabel]; got != "copy-iso-test-vm-bootiso-2" { + t.Errorf("importing label of the newer import was changed: %q", got) + } + if got := updated.GetLabels()[PowerAfterImportLabel]; got != "On" { + t.Errorf("deferred power command was changed: %q", got) + } + if _, found := updated.GetAnnotations()[failedTestMarker]; found { + t.Error("a stale pod must not mark the device as failed") + } +} + +// Inserted must mean "there is an image to boot": not while the import is +// still writing it, and not after the import failed. +func TestIsVirtualMediaInserted_ImportingOrFailedIsNotInserted(t *testing.T) { + tests := []struct { + name string + labels map[string]string + annotations map[string]string + want bool + }{ + {"import done", nil, nil, true}, + {"import running", map[string]string{failedTestLabel: failedTestPod}, nil, false}, + {"import failed", nil, map[string]string{failedTestMarker: failedTestPod}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockDynamic := NewMockDynamicClient() + mockDynamic.AddVM(withCDROMVolume(newTestVM("test-ns", "test-vm", tt.labels, tt.annotations), "test-vm-bootiso")) + fakeK8s := fake.NewSimpleClientset(boundPVC("test-vm-bootiso")) + client := NewClientWithClients(fakeK8s, mockDynamic, 30*time.Second, nil) + + inserted, err := client.IsVirtualMediaInserted("test-ns", "test-vm", "cdrom0") + if err != nil { + t.Fatalf("IsVirtualMediaInserted failed: %v", err) + } + if inserted != tt.want { + t.Errorf("Inserted = %v, want %v", inserted, tt.want) + } + }) + } +} + +// After a failure the next InsertMedia, same image or not, must import again. +func TestCheckInsertedMedia_FailedImportIsNotInserted(t *testing.T) { + for _, url := range []string{"http://example.com/boot.iso", "http://example.com/other.iso"} { + t.Run(url, func(t *testing.T) { + mockDynamic := NewMockDynamicClient() + mockDynamic.AddVM(withCDROMVolume(newTestVM("test-ns", "test-vm", nil, map[string]string{ + failedTestURLAn: "http://example.com/boot.iso", + failedTestMarker: failedTestPod, + }), "test-vm-bootiso")) + client := NewClientWithClients(fake.NewSimpleClientset(), mockDynamic, 30*time.Second, nil) + + state, err := client.CheckInsertedMedia("test-ns", "test-vm", "cdrom0", url) + if err != nil { + t.Fatalf("CheckInsertedMedia failed: %v", err) + } + if state != MediaStateNone { + t.Errorf("state = %d, want MediaStateNone", state) + } + }) + } +} + +func TestInsertVirtualMedia_ClearsFailedImportMarker(t *testing.T) { + client, mockDynamic, _ := setupInsertVirtualMediaTest(t, false) + vm, _ := mockDynamic.GetVM("test-ns", "test-vm") + annotations := vm.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[failedTestMarker] = failedTestPod + vm.SetAnnotations(annotations) + if err := mockDynamic.AddVM(vm); err != nil { + t.Fatalf("AddVM failed: %v", err) + } + + if err := client.insertVirtualMediaAsync("test-ns", "test-vm", "cdrom0", "http://example.com/image.iso"); err != nil { + t.Fatalf("insertVirtualMediaAsync failed: %v", err) + } + + updated, _ := mockDynamic.GetVM("test-ns", "test-vm") + if got, found := updated.GetAnnotations()[failedTestMarker]; found { + t.Errorf("annotation %s = %q survived the new insert", failedTestMarker, got) + } +} + +func TestEjectVirtualMedia_ClearsFailedImportMarker(t *testing.T) { + mockDynamic := NewMockDynamicClient() + mockDynamic.AddVM(withCDROMVolume(newTestVM("test-ns", "test-vm", nil, map[string]string{ + failedTestURLAn: "http://example.com/boot.iso", + failedTestMarker: failedTestPod, + }), "test-vm-bootiso-123")) + client := NewClientWithClients(fake.NewSimpleClientset(), mockDynamic, 30*time.Second, nil) + + if err := client.EjectVirtualMedia("test-ns", "test-vm", "cdrom0"); err != nil { + t.Fatalf("EjectVirtualMedia failed: %v", err) + } + + updated, _ := mockDynamic.GetVM("test-ns", "test-vm") + if got, found := updated.GetAnnotations()[failedTestMarker]; found { + t.Errorf("annotation %s = %q survived the eject", failedTestMarker, got) + } +} From 14a5670bafe019e0d226fd3f271f495b2d960ec9 Mon Sep 17 00:00:00 2001 From: itayherzberg Date: Thu, 12 Nov 2026 18:47:54 +0200 Subject: [PATCH 2/2] fix(kubevirt): compare a request against a pending re-insert after a failed import After a failed import the next InsertMedia is accepted and marks the VM as importing before its job replaces the PVC and clears the failed marker. CheckInsertedMedia looked at the marker first, so a request for another image in that window was answered as still in progress instead of as a conflict. The marker now only applies while no import is recorded for the device. Signed-off-by: itayherzberg --- pkg/kubevirt/client.go | 14 ++++++++------ pkg/kubevirt/import_failed_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/pkg/kubevirt/client.go b/pkg/kubevirt/client.go index e803316..33bbcc7 100644 --- a/pkg/kubevirt/client.go +++ b/pkg/kubevirt/client.go @@ -1672,12 +1672,6 @@ func (c *Client) CheckInsertedMedia(namespace, name, mediaID, imageURL string) ( return MediaStateNone, fmt.Errorf("failed to get VM %s/%s: %w", namespace, name, err) } - // After a failed import the attached PVC holds no image; a new insert - // replaces it, so there is nothing to compare the request against. - if vm.GetAnnotations()[ImportFailedAnnotationPrefix+mediaID] != "" { - return MediaStateNone, nil - } - // Check if the device has a PVC volume attached hasPVC := false if vm.Spec.Template != nil { @@ -1696,6 +1690,14 @@ func (c *Client) CheckInsertedMedia(namespace, name, mediaID, imageURL string) ( return MediaStateNone, nil } + // After a failed import the attached PVC holds no image; a new insert + // replaces it, so there is nothing to compare the request against. Once + // that new insert is accepted the importing label is back, and the marker + // only waits for the job to clear it. + if !importing && vm.GetAnnotations()[ImportFailedAnnotationPrefix+mediaID] != "" { + return MediaStateNone, nil + } + // Media is attached or on its way. Compare the stored URL. annotationKey := VirtualMediaURLAnnotationPrefix + mediaID storedURL := vm.GetAnnotations()[annotationKey] diff --git a/pkg/kubevirt/import_failed_test.go b/pkg/kubevirt/import_failed_test.go index 761b9b5..310a612 100644 --- a/pkg/kubevirt/import_failed_test.go +++ b/pkg/kubevirt/import_failed_test.go @@ -242,6 +242,36 @@ func TestCheckInsertedMedia_FailedImportIsNotInserted(t *testing.T) { } } +// A re-insert after a failure is accepted before its job replaces the PVC, so +// for a moment the VM carries both the marker and the new pending import. The +// pending import is what a further request must be compared against. +func TestCheckInsertedMedia_PendingReinsertAfterFailedImport(t *testing.T) { + cases := map[string]MediaState{ + "http://example.com/boot.iso": MediaStateImporting, + "http://example.com/other.iso": MediaStateConflict, + } + for url, want := range cases { + t.Run(url, func(t *testing.T) { + mockDynamic := NewMockDynamicClient() + mockDynamic.AddVM(withCDROMVolume(newTestVM("test-ns", "test-vm", + map[string]string{ImportingLabelPrefix + "cdrom0": importPendingValue(time.Now())}, + map[string]string{ + failedTestURLAn: "http://example.com/boot.iso", + failedTestMarker: failedTestPod, + }), "test-vm-bootiso")) + client := NewClientWithClients(fake.NewSimpleClientset(), mockDynamic, 30*time.Second, nil) + + state, err := client.CheckInsertedMedia("test-ns", "test-vm", "cdrom0", url) + if err != nil { + t.Fatalf("CheckInsertedMedia failed: %v", err) + } + if state != want { + t.Errorf("state = %d, want %d", state, want) + } + }) + } +} + func TestInsertVirtualMedia_ClearsFailedImportMarker(t *testing.T) { client, mockDynamic, _ := setupInsertVirtualMediaTest(t, false) vm, _ := mockDynamic.GetVM("test-ns", "test-vm")