Skip to content

fix(kubevirt): a failed ISO import must not start the VM, read as inserted, or block a re-insert #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
itayherzberg wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(kubevirt): a failed ISO import must not start the VM, read as inserted, or block a re-insert #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e093c2
fix(kubevirt): a failed ISO import must not start the VM or read as i…
itayherzberg Oct 25, 2026
14a5670
fix(kubevirt): compare a request against a pending re-insert after a …
itayherzberg Nov 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions pkg/kubevirt/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
})
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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 {

@MarSik MarSik Nov 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the workflow cancels the run command and the user must retrigger both import and VM start?

@itayherzberg itayherzberg Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. When the import pod fails, the deferred power command is removed together with the importing label, so the client has to send InsertMedia again and then the power command again.

I dropped it instead of keeping it because a power-on left on the VM would run after whatever insert succeeds next, which can be much later and for a different image. That seemed worse than asking for the power command again.

What the client sees after the failure: PowerState stays Off, Inserted is false, and the same InsertMedia is answered 202 and imports again. The insert task is no help here: it already reads Completed, because the job ends once the import pod is created.

I expect Ironic to time out waiting for the power state and retry the step, but I have not run this against a BareMetalHost.

If you prefer to keep the deferred command on the VM until a later insert succeeds, so that only the insert has to be repeated, I can change it. It is a small change in markImportFailed.

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/<deviceName> label from a VM
func (c *Client) removeImportingLabel(namespace, vmName, deviceName string) error {
labelKey := ImportingLabelPrefix + deviceName
Expand Down
Loading