diff --git a/pkg/kubevirt/client.go b/pkg/kubevirt/client.go index 329dcfc..33bbcc7 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 { @@ -1679,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] @@ -1806,6 +1825,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 +2482,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 +2929,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 +3598,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..310a612 --- /dev/null +++ b/pkg/kubevirt/import_failed_test.go @@ -0,0 +1,314 @@ +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) + } + }) + } +} + +// 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") + 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) + } +}