Check image name length in preflight phase

... to ensure the resulting name of the `VirtualMachineImage` will match the 63 characters limit. - Shorten the image display name from `vm-import-<VMName>-<VMName>-<DiskIndex>` to the name of the disk `import-<VMName>-<DiskIndex>` to safe characters. This allows the import of VM with longer names. Related to: harvester/harvester#6463 Signed-off-by: Volker Theile <[email protected]>
harvester · Jan 30, 2025 · 4aa4195 · 4aa4195
1 parent 9a44174
commit 4aa4195
Show file tree

Hide file tree

Showing 4 changed files with 99 additions and 17 deletions.
diff --git a/pkg/controllers/migration/helper.go b/pkg/controllers/migration/helper.go
@@ -1,11 +1,13 @@
 package migration
 
 import (
+	"fmt"
 	"reflect"
 	"time"
 
 	"github.com/sirupsen/logrus"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/validation"
 
 	migration "github.com/harvester/vm-import-controller/pkg/apis/migration.harvesterhci.io/v1beta1"
 	"github.com/harvester/vm-import-controller/pkg/util"
@@ -45,14 +47,37 @@ func evaluateDiskImportStatus(diskImportStatus []migration.DiskInfo) *migration.
 
 func (h *virtualMachineHandler) reconcileDiskImageStatus(vm *migration.VirtualMachineImport) (*migration.VirtualMachineImport, error) {
 	orgStatus := vm.Status.DeepCopy()
+
 	// If VM has no disks associated ignore the VM
 	if len(orgStatus.DiskImportStatus) == 0 {
-		logrus.Errorf("Imported VM %s in namespace %s, has no disks, being marked as invalid and will be ignored", vm.Name, vm.Namespace)
+		logrus.WithFields(logrus.Fields{
+			"name":      vm.Name,
+			"namespace": vm.Namespace,
+		}).Error("The imported VM has no disks, being marked as invalid and will be ignored")
 		vm.Status.Status = migration.VirtualMachineImportInvalid
 		return h.importVM.UpdateStatus(vm)
 
 	}
 
+	// Make sure the `harvesterhci.io/imageDisplayName` labels that are
+	// set on the to be created VirtualMachineImages will be valid.
+	// We need to do this check here because this can't be done during
+	// the preflight check stage for all supported source systems, e.g.
+	// for VMware the necessary information is only available after the
+	// VM has been exported (which is too late for the preflight check).
+	for _, d := range vm.Status.DiskImportStatus {
+		_, err := GenerateImageDisplayNameLabelValue(d)
+		if err != nil {
+			logrus.WithFields(logrus.Fields{
+				"name":          vm.Name,
+				"namespace":     vm.Namespace,
+				"diskInfo.Name": d.Name,
+			}).Errorf("The label '%s' to be set on the to be created VM image will cause a failure: %v", labelImageDisplayName, err)
+			vm.Status.Status = migration.VirtualMachineImportInvalid
+			return h.importVM.UpdateStatus(vm)
+		}
+	}
+
 	err := h.createVirtualMachineImages(vm)
 	if err != nil {
 		// check if any disks have been updated. We need to save this info to eventually reconcile the VMI creation
@@ -136,3 +161,14 @@ func (h *virtualMachineHandler) triggerResubmit(vm *migration.VirtualMachineImpo
 	vm.Status.Status = migration.SourceReady
 	return h.importVM.UpdateStatus(vm)
 }
+
+// GenerateImageDisplayNameLabelValue generates the value for the `harvesterhci.io/imageDisplayName` label.
+// The value is also returned in the event of an error.
+func GenerateImageDisplayNameLabelValue(d migration.DiskInfo) (string, error) {
+	value := fmt.Sprintf("import-%s", d.Name)
+	err := validation.IsValidLabelValue(value)
+	if err != nil {
+		return value, fmt.Errorf("the label value '%s' is not valid: %v", value, err)
+	}
+	return value, nil
+}
diff --git a/pkg/controllers/migration/virtualmachine.go b/pkg/controllers/migration/virtualmachine.go
@@ -413,7 +413,6 @@ func (h *virtualMachineHandler) triggerExport(vm *migration.VirtualMachineImport
 
 // generateVMO is a wrapper to generate a VirtualMachineOperations client
 func (h *virtualMachineHandler) generateVMO(vm *migration.VirtualMachineImport) (VirtualMachineOperations, error) {
-
 	source, err := h.generateSource(vm)
 	if err != nil {
 		return nil, fmt.Errorf("error generating migration interface: %v", err)
@@ -752,18 +751,22 @@ func generateAnnotations(vm *migration.VirtualMachineImport, vmi *harvesterv1bet
 }
 
 func (h *virtualMachineHandler) checkAndCreateVirtualMachineImage(vm *migration.VirtualMachineImport, d migration.DiskInfo) (*harvesterv1beta1.VirtualMachineImage, error) {
+	// We can ignore the error here as the label value has already been
+	// checked in an earlier step.
+	imageDisplayName, _ := GenerateImageDisplayNameLabelValue(d)
+
 	imageList, err := h.vmi.Cache().List(vm.Namespace, labels.SelectorFromSet(map[string]string{
-		labelImageDisplayName: fmt.Sprintf("vm-import-%s-%s", vm.Name, d.Name),
+		labelImageDisplayName: imageDisplayName,
 	}))
 	if err != nil {
 		return nil, err
 	}
 
-	if len(imageList) > 1 {
-		return nil, fmt.Errorf("unexpected error: found %d images with label %s=%s, only expected to find one", len(imageList), labelImageDisplayName, fmt.Sprintf("vm-import-%s-%s", vm.Name, d.Name))
+	numImages := len(imageList)
+	if numImages > 1 {
+		return nil, fmt.Errorf("found %d images with label '%s=%s', only expected to find one", numImages, labelImageDisplayName, imageDisplayName)
 	}
-
-	if len(imageList) == 1 {
+	if numImages == 1 {
 		return imageList[0], nil
 	}
 
@@ -781,11 +784,15 @@ func (h *virtualMachineHandler) checkAndCreateVirtualMachineImage(vm *migration.
 				},
 			},
 			Labels: map[string]string{
-				labelImageDisplayName: fmt.Sprintf("vm-import-%s-%s", vm.Name, d.Name),
+				// Set the `harvesterhci.io/imageDisplayName` label to be
+				// able to search for the `VirtualMachineImage` object during
+				// the reconciliation phase. See code above at the beginning
+				// of this function.
+				labelImageDisplayName: imageDisplayName,
 			},
 		},
 		Spec: harvesterv1beta1.VirtualMachineImageSpec{
-			DisplayName: fmt.Sprintf("vm-import-%s-%s", vm.Name, d.Name),
+			DisplayName: imageDisplayName,
 			URL:         fmt.Sprintf("http://%s:%d/%s", server.Address(), server.DefaultPort(), d.Name),
 			SourceType:  "download",
 		},
@@ -816,16 +823,18 @@ func (h *virtualMachineHandler) sanitizeVirtualMachineImport(vm *migration.Virtu
 	if err != nil {
 		vm.Status.Status = migration.VirtualMachineImportInvalid
 		logrus.WithFields(logrus.Fields{
+			"kind":                              vm.Kind,
 			"name":                              vm.Name,
 			"namespace":                         vm.Namespace,
 			"spec.virtualMachineName":           vm.Spec.VirtualMachineName,
 			"status.importedVirtualMachineName": vm.Status.ImportedVirtualMachineName,
-		}).Errorf("Failed to sanitize the '%s' object: %v", vm.Kind, err)
+		}).Errorf("Failed to sanitize the import spec: %v", err)
 	} else {
 		// Make sure the ImportedVirtualMachineName is RFC 1123 compliant.
 		if errs := validation.IsDNS1123Label(vm.Status.ImportedVirtualMachineName); len(errs) != 0 {
 			vm.Status.Status = migration.VirtualMachineImportInvalid
 			logrus.WithFields(logrus.Fields{
+				"kind":                              vm.Kind,
 				"name":                              vm.Name,
 				"namespace":                         vm.Namespace,
 				"spec.virtualMachineName":           vm.Spec.VirtualMachineName,
@@ -834,11 +843,12 @@ func (h *virtualMachineHandler) sanitizeVirtualMachineImport(vm *migration.Virtu
 		} else {
 			vm.Status.Status = migration.SourceReady
 			logrus.WithFields(logrus.Fields{
+				"kind":                              vm.Kind,
 				"name":                              vm.Name,
 				"namespace":                         vm.Namespace,
 				"spec.virtualMachineName":           vm.Spec.VirtualMachineName,
 				"status.importedVirtualMachineName": vm.Status.ImportedVirtualMachineName,
-			}).Infof("The sanitization of the '%s' object was successful", vm.Kind)
+			}).Info("The sanitization of the import spec was successful")
 		}
 	}
 

diff --git a/pkg/source/openstack/client.go b/pkg/source/openstack/client.go
@@ -28,6 +28,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation"
 	kubevirt "kubevirt.io/api/core/v1"
 
 	migration "github.com/harvester/vm-import-controller/pkg/apis/migration.harvesterhci.io/v1beta1"
@@ -185,19 +186,19 @@ func (c *Client) Verify() error {
 }
 
 func (c *Client) PreFlightChecks(vm *migration.VirtualMachineImport) (err error) {
+	// Check the source network mappings.
 	for _, nm := range vm.Spec.Mapping {
 		logrus.WithFields(logrus.Fields{
 			"name":          vm.Name,
 			"namespace":     vm.Namespace,
 			"sourceNetwork": nm.SourceNetwork,
-		}).Info("Searching for the source network in preflight check")
+		}).Info("Checking the source network as part of the preflight check")
 
 		pgr := networks.List(c.networkClient, networks.ListOpts{Name: nm.SourceNetwork})
 		allPgs, err := pgr.AllPages()
 		if err != nil {
 			return fmt.Errorf("error while generating all pages during querying source network '%s': %v", nm.SourceNetwork, err)
 		}
-
 		ok, err := allPgs.IsEmpty()
 		if err != nil {
 			return fmt.Errorf("error while checking if pages were empty during querying source network '%s': %v", nm.SourceNetwork, err)
@@ -206,6 +207,32 @@ func (c *Client) PreFlightChecks(vm *migration.VirtualMachineImport) (err error)
 			return fmt.Errorf("source network '%s' not found", nm.SourceNetwork)
 		}
 	}
+
+	// Make sure the `harvesterhci.io/imageDisplayName` label to be set to
+	// the VirtualMachineImages are valid.
+	sanitizedVM := vm.DeepCopy()
+	if err := c.SanitizeVirtualMachineImport(sanitizedVM); err != nil {
+		return fmt.Errorf("failed to sanitize the copied object (Kind=%s, Name=%s) during preflight check: %v",
+			sanitizedVM.Kind, sanitizedVM.Name, err)
+	}
+	vmObj, err := c.findVM(sanitizedVM.Spec.VirtualMachineName)
+	if err != nil {
+		return fmt.Errorf("error find VM in PreFlightChecks :%v", err)
+	}
+	for vIndex := range vmObj.AttachedVolumes {
+		imageDisplayName := fmt.Sprintf("import-%s-%d.img", sanitizedVM.Status.ImportedVirtualMachineName, vIndex)
+
+		logrus.WithFields(logrus.Fields{
+			"name":             vm.Name,
+			"namespace":        vm.Namespace,
+			"imageDisplayName": imageDisplayName,
+		}).Info("Checking the VirtualMachineImage label as part of the preflight check")
+
+		if errs := validation.IsValidLabelValue(imageDisplayName); len(errs) != 0 {
+			return fmt.Errorf("the display image name label '%s' is not valid: %v", imageDisplayName, errs)
+		}
+	}
+
 	return nil
 }
 
@@ -318,7 +345,7 @@ func (c *Client) ExportVirtualMachine(vm *migration.VirtualMachineImport) error
 			return err
 		}
 
-		rawImageFileName := fmt.Sprintf("%s-%d.img", vmObj.Name, vIndex)
+		rawImageFileName := fmt.Sprintf("%s-%d.img", vm.Status.ImportedVirtualMachineName, vIndex)
 
 		logrus.WithFields(logrus.Fields{
 			"name":                    vm.Name,

diff --git a/pkg/source/vmware/client.go b/pkg/source/vmware/client.go
@@ -124,6 +124,7 @@ func (c *Client) Verify() error {
 }
 
 func (c *Client) PreFlightChecks(vm *migration.VirtualMachineImport) (err error) {
+	// Check the source network mappings.
 	f := find.NewFinder(c.Client.Client, true)
 	dc := c.dc
 	if !strings.HasPrefix(c.dc, "/") {
@@ -134,7 +135,7 @@ func (c *Client) PreFlightChecks(vm *migration.VirtualMachineImport) (err error)
 			"name":          vm.Name,
 			"namespace":     vm.Namespace,
 			"sourceNetwork": nm.SourceNetwork,
-		}).Info("Searching for the source network in preflight check")
+		}).Info("Checking the source network as part of the preflight check")
 
 		// The path looks like `/<datacenter>/network/<network-name>`.
 		path := filepath.Join(dc, "/network", nm.SourceNetwork)
@@ -143,6 +144,14 @@ func (c *Client) PreFlightChecks(vm *migration.VirtualMachineImport) (err error)
 			return fmt.Errorf("error getting source network '%s': %v", nm.SourceNetwork, err)
 		}
 	}
+
+	// Make sure the `harvesterhci.io/imageDisplayName` label to be set to
+	// the VirtualMachineImages are valid.
+	// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+	// This is not possible for VMware at this stage. The necessary information
+	// is only available after the VM has been exported.
+	// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+
 	return nil
 }
 
@@ -232,7 +241,7 @@ func (c *Client) ExportVirtualMachine(vm *migration.VirtualMachineImport) (err e
 	// once the disks are converted this needs to be updated to ".img"
 	// spec for how download_url is generated
 	// 				Spec: harvesterv1beta1.VirtualMachineImageSpec{
-	//					DisplayName: fmt.Sprintf("vm-import-%s-%s", vm.Name, d.Name),
+	//					DisplayName: fmt.Sprintf("import-%s", d.Name),
 	//					URL: fmt.Sprintf("http://%s:%d/%s.img", server.Address(), server.DefaultPort(), d.Name),
 	//				},
 
@@ -244,7 +253,7 @@ func (c *Client) ExportVirtualMachine(vm *migration.VirtualMachineImport) (err e
 		destFile := filepath.Join(server.TempDir(), rawDiskName)
 		err = qemu.ConvertVMDKtoRAW(sourceFile, destFile)
 		if err != nil {
-			return fmt.Errorf("error during conversion of vmdk to raw disk %v", err)
+			return fmt.Errorf("error during conversion of vmdk to raw disk: %v", err)
 		}
 		// update fields to reflect final location of raw image file
 		vm.Status.DiskImportStatus[i].DiskLocalPath = server.TempDir()