fix(vm): fix migration with images, hotplugs, local disks, and when changing the storage class (#1518)

Fixed:
- vm no longer reboots when a vmbda is removed
- pvc with data is no longer deleted when migrating a vm with local disks
- images no longer block vm migration
- it is now possible to hotplug images to a vm again
- vms with hotplugged images can now be migrated again
- vm no longer loses events related to virtual disks and reconciles faster
- add a restriction on migrating vms with local disks if the vm has hotplugs
- fix a panic in the test with storage class migration
- vmbda interface no longer disappears from the status of a stopped vm


Signed-off-by: Isteb4k <[email protected]>

Author: Isteb4k

build/components/versions.yml

@@ -3,7 +3,7 @@ firmware:
   libvirt: v10.9.0
   edk2: stable202411
 core:
-  3p-kubevirt: v1.3.1-v12n.14
+  3p-kubevirt: v1.3.1-v12n.15
   3p-containerized-data-importer: v1.60.3-v12n.10
   distribution: 2.8.3
 package:

images/virtualization-artifact/pkg/controller/kvbuilder/kvvm_utils.go

@@ -83,6 +83,7 @@ func GenerateSerial(input string) string {
 type HotPlugDeviceSettings struct {
 	VolumeName     string
 	PVCName        string
+	Image          string
 	DataVolumeName string
 }
 
@@ -91,6 +92,7 @@ func ApplyVirtualMachineSpec(
 	vdByName map[string]*v1alpha2.VirtualDisk,
 	viByName map[string]*v1alpha2.VirtualImage,
 	cviByName map[string]*v1alpha2.ClusterVirtualImage,
+	vmbdas map[v1alpha2.VMBDAObjectRef][]*v1alpha2.VirtualMachineBlockDeviceAttachment,
 	class *v1alpha2.VirtualMachineClass,
 	ipAddress string,
 	networkSpec network.InterfaceSpecList,
@@ -130,11 +132,11 @@ func ApplyVirtualMachineSpec(
 				PVCName:    volume.PersistentVolumeClaim.ClaimName,
 			})
 		}
-		// FIXME(VM): not used, now only supports PVC
-		if volume.DataVolume != nil && volume.DataVolume.Hotpluggable {
+
+		if volume.ContainerDisk != nil && isHotplugged(volume, vm, vmbdas) {
 			hotpluggedDevices = append(hotpluggedDevices, HotPlugDeviceSettings{
-				VolumeName:     volume.Name,
-				DataVolumeName: volume.DataVolume.Name,
+				VolumeName: volume.Name,
+				Image:      volume.ContainerDisk.Image,
 			})
 		}
 	}
@@ -218,6 +220,10 @@ func ApplyVirtualMachineSpec(
 		}
 	}
 
+	if err := kvvm.SetProvisioning(vm.Spec.Provisioning); err != nil {
+		return err
+	}
+
 	for _, device := range hotpluggedDevices {
 		switch {
 		case device.PVCName != "":
@@ -227,13 +233,15 @@ func ApplyVirtualMachineSpec(
 			}); err != nil {
 				return err
 			}
-			// FIXME(VM): not used, now only supports PVC
-		case device.DataVolumeName != "":
+		case device.Image != "":
+			if err := kvvm.SetDisk(device.VolumeName, SetDiskOptions{
+				ContainerDisk: pointer.GetPointer(device.Image),
+				IsHotplugged:  true,
+			}); err != nil {
+				return err
+			}
 		}
 	}
-	if err := kvvm.SetProvisioning(vm.Spec.Provisioning); err != nil {
-		return err
-	}
 
 	kvvm.SetOwnerRef(vm, schema.GroupVersionKind{
 		Group:   v1alpha2.SchemeGroupVersion.Group,
@@ -313,3 +321,15 @@ func setNetworksAnnotation(kvvm *KVVM, networkSpec network.InterfaceSpecList) er
 	kvvm.SetKVVMIAnnotation(annotations.AnnNetworksSpec, networkConfigStr)
 	return nil
 }
+
+func isHotplugged(volume virtv1.Volume, vm *v1alpha2.VirtualMachine, vmbdas map[v1alpha2.VMBDAObjectRef][]*v1alpha2.VirtualMachineBlockDeviceAttachment) bool {
+	name, kind := GetOriginalDiskName(volume.Name)
+	for _, bdRef := range vm.Spec.BlockDeviceRefs {
+		if bdRef.Name == name && bdRef.Kind == kind {
+			return false
+		}
+	}
+
+	_, ok := vmbdas[v1alpha2.VMBDAObjectRef{Name: name, Kind: v1alpha2.VMBDAObjectRefKind(kind)}]
+	return ok
+}

images/virtualization-artifact/pkg/controller/vd/internal/migration.go

@@ -18,6 +18,7 @@ package internal
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"log/slog"
 
@@ -306,6 +307,7 @@ func (h MigrationHandler) handleMigratePrepareTarget(ctx context.Context, vd *v1
 	}
 
 	// Reset migration info
+	targetPVCName := vd.Status.MigrationState.TargetPVC
 	vd.Status.MigrationState = v1alpha2.VirtualDiskMigrationState{}
 
 	var targetStorageClass *storagev1.StorageClass
@@ -398,11 +400,20 @@ func (h MigrationHandler) handleMigratePrepareTarget(ctx context.Context, vd *v1
 	}
 
 	log.Info("Start creating target PersistentVolumeClaim", slog.String("storageClass", targetStorageClass.Name), slog.String("capacity", size.String()))
-	pvc, err := h.createTargetPersistentVolumeClaim(ctx, vd, targetStorageClass, size)
+	pvc, err := h.createTargetPersistentVolumeClaim(ctx, vd, targetStorageClass, size, targetPVCName, vd.Status.Target.PersistentVolumeClaim)
 	if err != nil {
 		return err
 	}
-	log.Info("Target PersistentVolumeClaim was created or was already exists", slog.String("pvc.name", pvc.Name), slog.String("pvc.namespace", pvc.Namespace))
+
+	log.Info(
+		"The target PersistentVolumeClaim has been created or already exists",
+		slog.String("state.source.pvc", vd.Status.Target.PersistentVolumeClaim),
+		slog.String("state.target.pvc", pvc.Name),
+	)
+
+	if vd.Status.Target.PersistentVolumeClaim == pvc.Name {
+		return errors.New("the target PersistentVolumeClaim name matched the source PersistentVolumeClaim name, please report a bug")
+	}
 
 	vd.Status.MigrationState = v1alpha2.VirtualDiskMigrationState{
 		SourcePVC:      vd.Status.Target.PersistentVolumeClaim,
@@ -479,7 +490,10 @@ func (h MigrationHandler) handleMigrateSync(ctx context.Context, vd *v1alpha2.Vi
 func (h MigrationHandler) handleRevert(ctx context.Context, vd *v1alpha2.VirtualDisk) error {
 	log := logger.FromContext(ctx)
 	log.Info("Start reverting...")
-	log.Info("Delete target PersistentVolumeClaim", slog.String("pvc.name", vd.Status.MigrationState.TargetPVC), slog.String("pvc.namespace", vd.Namespace))
+
+	if vd.Status.MigrationState.TargetPVC == vd.Status.Target.PersistentVolumeClaim {
+		return errors.New("cannot revert: the target PersistentVolumeClaim name matched the source PersistentVolumeClaim name, please report a bug")
+	}
 
 	err := h.deleteTargetPersistentVolumeClaim(ctx, vd)
 	if err != nil {
@@ -573,7 +587,7 @@ func (h MigrationHandler) getInProgressMigratingVMOP(ctx context.Context, vm *v1
 	return nil, nil
 }
 
-func (h MigrationHandler) createTargetPersistentVolumeClaim(ctx context.Context, vd *v1alpha2.VirtualDisk, sc *storagev1.StorageClass, size resource.Quantity) (*corev1.PersistentVolumeClaim, error) {
+func (h MigrationHandler) createTargetPersistentVolumeClaim(ctx context.Context, vd *v1alpha2.VirtualDisk, sc *storagev1.StorageClass, size resource.Quantity, targetPVCName, sourcePVCName string) (*corev1.PersistentVolumeClaim, error) {
 	pvcs, err := listPersistentVolumeClaims(ctx, vd, h.client)
 	if err != nil {
 		return nil, err
@@ -584,7 +598,7 @@ func (h MigrationHandler) createTargetPersistentVolumeClaim(ctx context.Context,
 		for _, pvc := range pvcs {
 			// If TargetPVC is empty, that means previous reconciliation failed and not updated TargetPVC in status.
 			// So, we should use pvc, that is not equal to SourcePVC.
-			if pvc.Name == vd.Status.MigrationState.TargetPVC || pvc.Name != vd.Status.MigrationState.SourcePVC {
+			if pvc.Name == targetPVCName || pvc.Name != sourcePVCName {
 				return &pvc, nil
 			}
 		}

images/virtualization-artifact/pkg/controller/vd/internal/validator/spec_changes_validator.go

@@ -93,8 +93,8 @@ func (v *SpecChangesValidator) ValidateUpdate(ctx context.Context, oldVD, newVD
 				}
 
 				for _, bd := range vm.Status.BlockDeviceRefs {
-					if bd.Kind == v1alpha2.DiskDevice && bd.Name == oldVD.Name && bd.Hotplugged {
-						return nil, errors.New("storage class cannot be changed if the VirtualDisk is hotplugged to a running virtual machine")
+					if bd.Hotplugged {
+						return nil, errors.New("for now, changing the storage class is not allowed if the virtual machine has hot-plugged block devices")
 					}
 				}
 			} else {

images/virtualization-artifact/pkg/controller/vm/internal/block_device_handler.go

@@ -202,94 +202,26 @@ func (h *BlockDeviceHandler) getBlockDeviceWarnings(ctx context.Context, s state
 		return "", err
 	}
 
-	hotplugsByName := make(map[string]struct{})
-
-	for _, vmbdas := range vmbdasByBlockDevice {
-		for _, vmbda := range vmbdas {
-			switch vmbda.Status.Phase {
-			case v1alpha2.BlockDeviceAttachmentPhaseInProgress,
-				v1alpha2.BlockDeviceAttachmentPhaseAttached:
-			default:
-				continue
-			}
-
-			var (
-				cvi         *v1alpha2.ClusterVirtualImage
-				vi          *v1alpha2.VirtualImage
-				vd          *v1alpha2.VirtualDisk
-				bdStatusRef v1alpha2.BlockDeviceStatusRef
-			)
-
-			switch vmbda.Spec.BlockDeviceRef.Kind {
-			case v1alpha2.VMBDAObjectRefKindVirtualDisk:
-				vd, err = s.VirtualDisk(ctx, vmbda.Spec.BlockDeviceRef.Name)
-				if err != nil {
-					return "", err
-				}
-
-				if vd == nil {
-					continue
-				}
-
-				bdStatusRef = h.getBlockDeviceStatusRef(v1alpha2.DiskDevice, vmbda.Spec.BlockDeviceRef.Name)
-				bdStatusRef.Size = vd.Status.Capacity
-			case v1alpha2.VMBDAObjectRefKindVirtualImage:
-				vi, err = s.VirtualImage(ctx, vmbda.Spec.BlockDeviceRef.Name)
-				if err != nil {
-					return "", err
-				}
-
-				if vi == nil {
-					continue
-				}
-
-				bdStatusRef = h.getBlockDeviceStatusRef(v1alpha2.ImageDevice, vmbda.Spec.BlockDeviceRef.Name)
-				bdStatusRef.Size = vi.Status.Size.Unpacked
-
-			case v1alpha2.VMBDAObjectRefKindClusterVirtualImage:
-				cvi, err = s.ClusterVirtualImage(ctx, vmbda.Spec.BlockDeviceRef.Name)
-				if err != nil {
-					return "", err
-				}
-
-				if cvi == nil {
-					continue
-				}
-
-				bdStatusRef = h.getBlockDeviceStatusRef(v1alpha2.ClusterImageDevice, vmbda.Spec.BlockDeviceRef.Name)
-				bdStatusRef.Size = cvi.Status.Size.Unpacked
-			default:
-				return "", fmt.Errorf("unacceptable `Kind` of `BlockDeviceRef`: %s", vmbda.Spec.BlockDeviceRef.Kind)
-			}
-
-			hotplugsByName[bdStatusRef.Name] = struct{}{}
-		}
-	}
-
 	var conflictedRefs []string
-	vm := s.VirtualMachine().Current()
 
-	for _, bdSpecRef := range vm.Spec.BlockDeviceRefs {
+	for _, bdSpecRef := range s.VirtualMachine().Current().Spec.BlockDeviceRefs {
 		// It is a precaution to not apply changes in spec.blockDeviceRefs if disk is already
 		// hotplugged using the VMBDA resource.
 		// spec check is done by VirtualDisk status
 		// the reverse check is done by the vmbda-controller.
-		if bdSpecRef.Kind == v1alpha2.DiskDevice {
-			if _, conflict := hotplugsByName[bdSpecRef.Name]; conflict {
-				conflictedRefs = append(conflictedRefs, bdSpecRef.Name)
-				continue
-			}
-		}
-
-		if _, conflict := hotplugsByName[bdSpecRef.Name]; conflict {
-			conflictedRefs = append(conflictedRefs, bdSpecRef.Name)
+		_, conflict := vmbdasByBlockDevice[v1alpha2.VMBDAObjectRef{
+			Kind: v1alpha2.VMBDAObjectRefKind(bdSpecRef.Kind),
+			Name: bdSpecRef.Name,
+		}]
+		if conflict {
+			conflictedRefs = append(conflictedRefs, fmt.Sprintf("%s/%s", strings.ToLower(string(bdSpecRef.Kind)), bdSpecRef.Name))
 			continue
 		}
 	}
 
 	var warning string
 	if len(conflictedRefs) > 0 {
-		warning = fmt.Sprintf("spec.blockDeviceRefs field contains hotplugged disks (%s): unplug or remove them from spec to continue.", strings.Join(conflictedRefs, ", "))
+		warning = fmt.Sprintf("spec.blockDeviceRefs field contains block devices to hotplug (%s): unplug or remove them from spec to continue.", strings.Join(conflictedRefs, ", "))
 	}
 
 	return warning, nil

images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go

@@ -110,7 +110,13 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state.
 		return reconcile.Result{}, nil
 	}
 
-	kvvmiSynced := equality.Semantic.DeepEqual(kvvmInCluster.Spec.Template.Spec.Volumes, kvvmiInCluster.Spec.Volumes)
+	// The pull policy for container disks are only set on the VMI spec and not on the VM spec.
+	// In order to correctly compare the volumes set, we need to set the pull policy on the VM spec as well.
+	kvvmInClusterCopy := kvvmInCluster.DeepCopy()
+	s.fillContainerDiskImagePullPolicies(kvvmInClusterCopy, kvvmiInCluster)
+	s.fillContainerDiskImagePullPolicies(builtKVVM, kvvmiInCluster)
+
+	kvvmiSynced := equality.Semantic.DeepEqual(kvvmInClusterCopy.Spec.Template.Spec.Volumes, kvvmiInCluster.Spec.Volumes)
 	if !kvvmiSynced {
 		// kubevirt does not sync volumes with kvvmi yet
 		log.Info("kvvmi volumes are not synced yet, skip volume migration.")
@@ -199,6 +205,14 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state.
 	return reconcile.Result{}, nil
 }
 
+func getVolumesByName(vmiSpec *virtv1.VirtualMachineInstanceSpec) map[string]*virtv1.Volume {
+	volumes := map[string]*virtv1.Volume{}
+	for _, vol := range vmiSpec.Volumes {
+		volumes[vol.Name] = vol.DeepCopy()
+	}
+	return volumes
+}
+
 // if any volume in kvvmi is not exists in readWriteOnceDisks or storageClassChangedDisks,
 // this indicates that
 func (s MigrationVolumesService) shouldRevert(kvvmi *virtv1.VirtualMachineInstance, readWriteOnceDisks, storageClassChangedDisks map[string]*v1alpha2.VirtualDisk) bool {
@@ -230,7 +244,7 @@ func (s MigrationVolumesService) patchVolumes(ctx context.Context, kvvm *virtv1.
 		return err
 	}
 
-	logger.FromContext(ctx).Debug("Patch kvvm with migration volumes.", slog.String("patch", string(patchBytes)))
+	logger.FromContext(ctx).Info("The volume migration is detected: patch volumes", slog.String("patch", string(patchBytes)))
 
 	err = s.client.Patch(ctx, kvvm, client.RawPatch(types.JSONPatchType, patchBytes))
 	return err
@@ -261,10 +275,15 @@ func (s MigrationVolumesService) VolumesSynced(ctx context.Context, vmState stat
 		return false, nil
 	}
 
-	kvvmiSynced := equality.Semantic.DeepEqual(kvvmInCluster.Spec.Template.Spec.Volumes, kvvmiInCluster.Spec.Volumes)
+	// The pull policy for container disks are only set on the VMI spec and not on the VM spec.
+	// In order to correctly compare the volumes set, we need to set the pull policy on the VM spec as well.
+	kvvmInClusterCopy := kvvmInCluster.DeepCopy()
+	s.fillContainerDiskImagePullPolicies(kvvmInClusterCopy, kvvmiInCluster)
+
+	kvvmiSynced := equality.Semantic.DeepEqual(kvvmInClusterCopy.Spec.Template.Spec.Volumes, kvvmiInCluster.Spec.Volumes)
 	if !kvvmiSynced {
 		log.Info("kvvmi volumes are not synced yet")
-		log.Debug("", slog.Any("kvvmi", kvvmInCluster.Spec.Template.Spec.Volumes), slog.Any("kvvmi", kvvmiInCluster.Spec.Volumes))
+		log.Debug("", slog.Any("kvvmi", kvvmInClusterCopy.Spec.Template.Spec.Volumes), slog.Any("kvvmi", kvvmiInCluster.Spec.Volumes))
 		return false, nil
 	}
 
@@ -440,21 +459,42 @@ func (s MigrationVolumesService) getReadyTargetPVCs(ctx context.Context, disks m
 	return targetPVCs, nil
 }
 
+func (s MigrationVolumesService) fillContainerDiskImagePullPolicies(kvvm *virtv1.VirtualMachine, kvvmi *virtv1.VirtualMachineInstance) {
+	volsVMI := getVolumesByName(&kvvmi.Spec)
+	for i, volume := range kvvm.Spec.Template.Spec.Volumes {
+		if volume.ContainerDisk == nil {
+			continue
+		}
+		vmiVol, ok := volsVMI[volume.Name]
+		if !ok {
+			continue
+		}
+		if vmiVol.ContainerDisk == nil {
+			continue
+		}
+		kvvm.Spec.Template.Spec.Volumes[i].ContainerDisk.ImagePullPolicy = vmiVol.ContainerDisk.ImagePullPolicy
+	}
+}
+
 func (s MigrationVolumesService) makeKVVMFromVirtualMachineSpec(ctx context.Context, vmState state.VirtualMachineState) (*virtv1.VirtualMachine, *virtv1.VirtualMachine, error) {
 	kvvm, err := s.makeKVVMFromSpec(ctx, vmState)
 	if err != nil {
 		return nil, nil, err
 	}
+
 	kvvmBuilder := kvbuilder.NewKVVM(kvvm.DeepCopy(), kvbuilder.DefaultOptions(vmState.VirtualMachine().Current()))
 	vdByName, err := vmState.VirtualDisksByName(ctx)
 	if err != nil {
 		return nil, nil, err
 	}
+
 	err = kvbuilder.ApplyMigrationVolumes(kvvmBuilder, vmState.VirtualMachine().Changed(), vdByName)
 	if err != nil {
 		return nil, nil, err
 	}
+
 	kvvmWithMigrationVolumes := kvvmBuilder.GetResource()
+
 	return kvvm, kvvmWithMigrationVolumes, nil
 }
 

images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go

@@ -420,8 +420,13 @@ func MakeKVVMFromVMSpec(ctx context.Context, s state.VirtualMachineState) (*virt
 
 	networkSpec := network.CreateNetworkSpec(current, vmmacs)
 
+	vmbdas, err := s.VirtualMachineBlockDeviceAttachments(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("get vmbdas: %w", err)
+	}
+
 	// Create kubevirt VirtualMachine resource from d8 VirtualMachine spec.
-	err = kvbuilder.ApplyVirtualMachineSpec(kvvmBuilder, current, bdState.VDByName, bdState.VIByName, bdState.CVIByName, class, ip.Status.Address, networkSpec)
+	err = kvbuilder.ApplyVirtualMachineSpec(kvvmBuilder, current, bdState.VDByName, bdState.VIByName, bdState.CVIByName, vmbdas, class, ip.Status.Address, networkSpec)
 	if err != nil {
 		return nil, err
 	}