From e279a64b17c5ee407c3875f008b2f6cfe2358c2e Mon Sep 17 00:00:00 2001 From: Volker Theile Date: Mon, 9 Sep 2024 10:22:03 +0200 Subject: [PATCH 1/3] Enhance importer to handle several issue - Import OpenStack server by UUID - Import OpenStack server with upper case characters in its name The following improvements have been done: - Sanitize the configured `VirtualMachineName` field, e.g. convert upper case to lower case to make it RFC 1123 compliant. - Convert UUID to real name for OpenStack imports - Reduce waiting time to recheck if created VM is running from 5min to 2min - Rename variable `uuid` to `serverUUID` in the OpenStack client code to do not collide with the imported uuid module - Improve log messages - Fix typos - Add comments Related to: https://github.com/harvester/harvester/issues/6500 Related to: https://github.com/harvester/harvester/issues/6505 Signed-off-by: Volker Theile --- .../v1beta1/virtualmachines.go | 26 ++- pkg/controllers/migration/helper.go | 20 +-- pkg/controllers/migration/openstack.go | 8 +- pkg/controllers/migration/virtualmachine.go | 148 +++++++++++++----- pkg/controllers/migration/vmware.go | 9 +- pkg/source/openstack/client.go | 50 +++++- pkg/source/vmware/client.go | 35 ++++- tests/integration/common_test.go | 4 +- 8 files changed, 230 insertions(+), 70 deletions(-) diff --git a/pkg/apis/migration.harvesterhci.io/v1beta1/virtualmachines.go b/pkg/apis/migration.harvesterhci.io/v1beta1/virtualmachines.go index c3e3a79..f402861 100644 --- a/pkg/apis/migration.harvesterhci.io/v1beta1/virtualmachines.go +++ b/pkg/apis/migration.harvesterhci.io/v1beta1/virtualmachines.go @@ -21,11 +21,18 @@ type VirtualMachineImport struct { // VirtualMachineImportSpec is used to create kubevirt VirtualMachines by exporting VM's from migration clusters. type VirtualMachineImportSpec struct { - SourceCluster corev1.ObjectReference `json:"sourceCluster"` - VirtualMachineName string `json:"virtualMachineName"` - Folder string `json:"folder,omitempty"` - Mapping []NetworkMapping `json:"networkMapping,omitempty"` //If empty new VirtualMachineImport will be mapped to Management Network - StorageClass string `json:"storageClass,omitempty"` + SourceCluster corev1.ObjectReference `json:"sourceCluster"` + + // VirtualMachineName is the name of the virtual machine that will be + // imported. It contains the name or ID of the source virtual machine. + // Note that these names may not be DNS1123 compliant and will therefore + // be sanitized later. + // Examples: "vm-1234", "my-VM" or "5649cac7-3871-4bb5-aab6-c72b8c18d0a2" + VirtualMachineName string `json:"virtualMachineName"` + + Folder string `json:"folder,omitempty"` + Mapping []NetworkMapping `json:"networkMapping,omitempty"` //If empty new VirtualMachineImport will be mapped to Management Network + StorageClass string `json:"storageClass,omitempty"` } // VirtualMachineImportStatus tracks the status of the VirtualMachineImport export from migration and import into the Harvester cluster @@ -34,6 +41,11 @@ type VirtualMachineImportStatus struct { DiskImportStatus []DiskInfo `json:"diskImportStatus,omitempty"` ImportConditions []common.Condition `json:"importConditions,omitempty"` NewVirtualMachine string `json:"newVirtualMachine,omitempty"` + + // DefiniteVirtualMachineName is the sanitized and definite name of the + // target virtual machine that will be created in the Harvester cluster. + // The name is DNS1123 compliant. + DefiniteVirtualMachineName string `json:"definiteVirtualMachineName,omitempty"` } // DiskInfo contains the information about associated Disk in the Import migration. @@ -70,14 +82,14 @@ const ( DiskImagesFailed ImportStatus = "diskImageFailed" VirtualMachineCreated ImportStatus = "virtualMachineCreated" VirtualMachineRunning ImportStatus = "virtualMachineRunning" - VirtualMachineInvalid ImportStatus = "virtualMachineInvalid" + VirtualMachineImportValid ImportStatus = "virtualMachineImportValid" + VirtualMachineImportInvalid ImportStatus = "virtualMachineImportInvalid" VirtualMachinePoweringOff condition.Cond = "VMPoweringOff" VirtualMachinePoweredOff condition.Cond = "VMPoweredOff" VirtualMachineExported condition.Cond = "VMExported" VirtualMachineImageSubmitted condition.Cond = "VirtualMachineImageSubmitted" VirtualMachineImageReady condition.Cond = "VirtualMachineImageReady" VirtualMachineImageFailed condition.Cond = "VirtualMachineImageFailed" - NotValidDNS1123Label string = "not a valid DNS1123 label" VirtualMachineExportFailed condition.Cond = "VMExportFailed" VirtualMachineMigrationFailed ImportStatus = "VMMigrationFailed" ) diff --git a/pkg/controllers/migration/helper.go b/pkg/controllers/migration/helper.go index 16d0225..0bfa44d 100644 --- a/pkg/controllers/migration/helper.go +++ b/pkg/controllers/migration/helper.go @@ -48,7 +48,7 @@ func (h *virtualMachineHandler) reconcileDiskImageStatus(vm *migration.VirtualMa // 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) - vm.Status.Status = migration.VirtualMachineInvalid + vm.Status.Status = migration.VirtualMachineImportInvalid return h.importVM.UpdateStatus(vm) } @@ -86,8 +86,8 @@ func (h *virtualMachineHandler) reconcileVirtualMachineStatus(vm *migration.Virt return vm, err } if !ok { - // VM not running, requeue and check after 5mins - h.importVM.EnqueueAfter(vm.Namespace, vm.Name, 5*time.Minute) + // VM not running, requeue and check after 2mins + h.importVM.EnqueueAfter(vm.Namespace, vm.Name, 2*time.Minute) return vm, nil } @@ -98,13 +98,15 @@ func (h *virtualMachineHandler) reconcileVirtualMachineStatus(vm *migration.Virt func (h *virtualMachineHandler) reconcilePreFlightChecks(vm *migration.VirtualMachineImport) (*migration.VirtualMachineImport, error) { err := h.preFlightChecks(vm) if err != nil { - if err.Error() != migration.NotValidDNS1123Label { - return vm, err - } - logrus.Errorf("vm migration target %s in VM %s in namespace %s is not RFC 1123 compliant", vm.Spec.VirtualMachineName, vm.Name, vm.Namespace) - vm.Status.Status = migration.VirtualMachineInvalid + logrus.WithFields(logrus.Fields{ + "name": vm.Name, + "namespace": vm.Namespace, + "spec.virtualMachineName": vm.Spec.VirtualMachineName, + }).Errorf("The preflight checks failed: %v", err) + // Stop the reconciling for good as the checks failed. + vm.Status.Status = migration.VirtualMachineImportInvalid } else { - vm.Status.Status = migration.SourceReady + vm.Status.Status = migration.VirtualMachineImportValid } return h.importVM.UpdateStatus(vm) } diff --git a/pkg/controllers/migration/openstack.go b/pkg/controllers/migration/openstack.go index 2b6c139..2555cc5 100644 --- a/pkg/controllers/migration/openstack.go +++ b/pkg/controllers/migration/openstack.go @@ -33,7 +33,7 @@ func RegisterOpenstackController(ctx context.Context, os migrationController.Ope os.OnChange(ctx, "openstack-migration-change", oHandler.OnSourceChange) } -func (h *openstackHandler) OnSourceChange(_ string, o *migration.OpenstackSource) (*migration.OpenstackSource, error) { +func (h *openstackHandler) OnSourceChange(key string, o *migration.OpenstackSource) (*migration.OpenstackSource, error) { if o == nil || o.DeletionTimestamp != nil { return o, nil } @@ -42,7 +42,9 @@ func (h *openstackHandler) OnSourceChange(_ string, o *migration.OpenstackSource "kind": o.Kind, "name": o.Name, "namespace": o.Namespace, - }).Info("Reconciling migration source") + "key": key, + }).Info("Reconciling source") + if o.Status.Status != migration.ClusterReady { // process migration logic secretObj, err := h.secret.Get(o.Spec.Credentials.Namespace, o.Spec.Credentials.Name, metav1.GetOptions{}) @@ -52,7 +54,7 @@ func (h *openstackHandler) OnSourceChange(_ string, o *migration.OpenstackSource client, err := openstack.NewClient(h.ctx, o.Spec.EndpointAddress, o.Spec.Region, secretObj) if err != nil { - return o, fmt.Errorf("error generating openstack client for openstack migration: %s: %v", o.Name, err) + return o, fmt.Errorf("error generating openstack client for openstack migration '%s': %v", o.Name, err) } err = client.Verify() diff --git a/pkg/controllers/migration/virtualmachine.go b/pkg/controllers/migration/virtualmachine.go index 5fa48b5..8cfc05b 100644 --- a/pkg/controllers/migration/virtualmachine.go +++ b/pkg/controllers/migration/virtualmachine.go @@ -38,11 +38,15 @@ import ( ) const ( - vmiAnnotation = "migration.harvesterhci.io/virtualmachineimport" - imageDisplayName = "harvesterhci.io/imageDisplayName" + annotationVirtualMachineImport = "migration.harvesterhci.io/virtualmachineimport" + labelImageDisplayName = "harvesterhci.io/imageDisplayName" + expectedAPIVersion = "migration.harvesterhci.io/v1beta1" ) type VirtualMachineOperations interface { + // SanitizeVirtualMachineImport is responsible for sanitizing the VirtualMachineImport object. + SanitizeVirtualMachineImport(vm *migration.VirtualMachineImport) error + // ExportVirtualMachine is responsible for generating the raw images for each disk associated with the VirtualMachineImport // Any image format conversion will be performed by the VM Operation ExportVirtualMachine(vm *migration.VirtualMachineImport) error @@ -93,12 +97,12 @@ func RegisterVMImportController(ctx context.Context, vmware migrationController. } func (h *virtualMachineHandler) OnVirtualMachineChange(_ string, vmObj *migration.VirtualMachineImport) (*migration.VirtualMachineImport, error) { - if vmObj == nil || vmObj.DeletionTimestamp != nil { return nil, nil } vm := vmObj.DeepCopy() + switch vm.Status.Status { case "": // run preflight checks and make vm ready for import @@ -108,6 +112,13 @@ func (h *virtualMachineHandler) OnVirtualMachineChange(_ string, vmObj *migratio "spec.virtualMachineName": vm.Spec.VirtualMachineName, }).Info("Running preflight checks ...") return h.reconcilePreFlightChecks(vm) + case migration.VirtualMachineImportValid: + logrus.WithFields(logrus.Fields{ + "name": vm.Name, + "namespace": vm.Namespace, + "spec.virtualMachineName": vm.Spec.VirtualMachineName, + }).Info("Sanitizing the import spec ...") + return h.sanitizeVirtualMachineImport(vm) case migration.SourceReady: // vm migration is valid and ready. trigger migration specific import logrus.WithFields(logrus.Fields{ @@ -139,7 +150,9 @@ func (h *virtualMachineHandler) OnVirtualMachineChange(_ string, vmObj *migratio if newStatus == nil { return vm, nil } + vm.Status.Status = *newStatus + return h.importVM.UpdateStatus(vm) case migration.DiskImagesFailed: logrus.WithFields(logrus.Fields{ @@ -159,7 +172,9 @@ func (h *virtualMachineHandler) OnVirtualMachineChange(_ string, vmObj *migratio if err != nil { return vm, err } + vm.Status.Status = migration.VirtualMachineCreated + return h.importVM.UpdateStatus(vm) case migration.VirtualMachineCreated: // wait for VM to be running using a watch on VM's @@ -176,7 +191,7 @@ func (h *virtualMachineHandler) OnVirtualMachineChange(_ string, vmObj *migratio "spec.virtualMachineName": vm.Spec.VirtualMachineName, }).Info("The VM was imported successfully") return vm, h.tidyUpObjects(vm) - case migration.VirtualMachineInvalid: + case migration.VirtualMachineImportInvalid: logrus.WithFields(logrus.Fields{ "name": vm.Name, "namespace": vm.Namespace, @@ -197,13 +212,8 @@ func (h *virtualMachineHandler) OnVirtualMachineChange(_ string, vmObj *migratio // preFlightChecks is used to validate that the associate sources and VM migration references are valid func (h *virtualMachineHandler) preFlightChecks(vm *migration.VirtualMachineImport) error { - - if errs := validation.IsDNS1123Label(vm.Spec.VirtualMachineName); len(errs) != 0 { - return fmt.Errorf(migration.NotValidDNS1123Label) - } - - if vm.Spec.SourceCluster.APIVersion != "migration.harvesterhci.io/v1beta1" { - return fmt.Errorf("expected migration cluster apiversion to be migration.harvesterhci.io/v1beta1 but got %s", vm.Spec.SourceCluster.APIVersion) + if vm.Spec.SourceCluster.APIVersion != expectedAPIVersion { + return fmt.Errorf("expected migration cluster apiversion to be '%s' but got '%s'", expectedAPIVersion, vm.Spec.SourceCluster.APIVersion) } var ss migration.SourceInterface @@ -213,21 +223,21 @@ func (h *virtualMachineHandler) preFlightChecks(vm *migration.VirtualMachineImpo case "vmwaresource", "openstacksource": ss, err = h.generateSource(vm) if err != nil { - return fmt.Errorf("error generating migration in preflight checks :%v", err) + return fmt.Errorf("error generating migration in preflight checks: %v", err) } default: - return fmt.Errorf("unsupported migration kind. Currently supported values are vmware/openstack but got %s", strings.ToLower(vm.Spec.SourceCluster.Kind)) + return fmt.Errorf("unsupported migration kind. Currently supported values are vmware/openstack but got '%s'", strings.ToLower(vm.Spec.SourceCluster.Kind)) } if ss.ClusterStatus() != migration.ClusterReady { - return fmt.Errorf("migration not yet ready. current status is %s", ss.ClusterStatus()) + return fmt.Errorf("migration not yet ready. Current status is '%s'", ss.ClusterStatus()) } // verify specified storage class exists. Empty storage class means default storage class if vm.Spec.StorageClass != "" { _, err := h.sc.Get(vm.Spec.StorageClass) if err != nil { - logrus.Errorf("error looking up storageclass %s: %v", vm.Spec.StorageClass, err) + logrus.Errorf("error looking up storageclass '%s': %v", vm.Spec.StorageClass, err) return err } } @@ -304,13 +314,16 @@ func (h *virtualMachineHandler) triggerExport(vm *migration.VirtualMachineImport // power off machine if !util.ConditionExists(vm.Status.ImportConditions, migration.VirtualMachinePoweringOff, v1.ConditionTrue) { logrus.WithFields(logrus.Fields{ - "name": vm.Name, - "namespace": vm.Namespace, - "spec.virtualMachineName": vm.Spec.VirtualMachineName, - }).Info("Powering off client VM ...") + "name": vm.Name, + "namespace": vm.Namespace, + "spec.virtualMachineName": vm.Spec.VirtualMachineName, + "spec.sourceCluster.kind": vm.Spec.SourceCluster.Kind, + "spec.sourceCluster.name": vm.Spec.SourceCluster.Name, + "status.definiteVirtualMachineName": vm.Status.DefiniteVirtualMachineName, + }).Info("Power off the source VM") err = vmo.PowerOffVirtualMachine(vm) if err != nil { - return fmt.Errorf("error in poweroff call: %v", err) + return fmt.Errorf("failed to power off the source VM: %v", err) } conds := []common.Condition{ { @@ -352,6 +365,14 @@ func (h *virtualMachineHandler) triggerExport(vm *migration.VirtualMachineImport util.ConditionExists(vm.Status.ImportConditions, migration.VirtualMachinePoweringOff, v1.ConditionTrue) && !util.ConditionExists(vm.Status.ImportConditions, migration.VirtualMachineExported, v1.ConditionTrue) && !util.ConditionExists(vm.Status.ImportConditions, migration.VirtualMachineExportFailed, v1.ConditionTrue) { + logrus.WithFields(logrus.Fields{ + "name": vm.Name, + "namespace": vm.Namespace, + "spec.virtualMachineName": vm.Spec.VirtualMachineName, + "spec.sourceCluster.name": vm.Spec.SourceCluster.Name, + "spec.sourceCluster.kind": vm.Spec.SourceCluster.Kind, + "status.definiteVirtualMachineName": vm.Status.DefiniteVirtualMachineName, + }).Info("Exporting source VM") err := vmo.ExportVirtualMachine(vm) if err != nil { // avoid retrying if vm export fails @@ -365,7 +386,14 @@ func (h *virtualMachineHandler) triggerExport(vm *migration.VirtualMachineImport }, } vm.Status.ImportConditions = util.MergeConditions(vm.Status.ImportConditions, conds) - logrus.Errorf("error exporting virtualmachine %s for virtualmachineimport %s-%s: %v", vm.Spec.VirtualMachineName, vm.Namespace, vm.Name, err) + logrus.WithFields(logrus.Fields{ + "name": vm.Name, + "namespace": vm.Namespace, + "spec.virtualMachineName": vm.Spec.VirtualMachineName, + "spec.sourceCluster.name": vm.Spec.SourceCluster.Name, + "spec.sourceCluster.kind": vm.Spec.SourceCluster.Kind, + "status.definiteVirtualMachineName": vm.Status.DefiniteVirtualMachineName, + }).Errorf("Failed to export source VM: %v", err) return nil } conds := []common.Condition{ @@ -502,8 +530,9 @@ func (h *virtualMachineHandler) reconcileVMIStatus(vm *migration.VirtualMachineI func (h *virtualMachineHandler) createVirtualMachine(vm *migration.VirtualMachineImport) error { vmo, err := h.generateVMO(vm) if err != nil { - return fmt.Errorf("error generating VMO in createVirtualMachine :%v", err) + return fmt.Errorf("error generating VMO in createVirtualMachine: %v", err) } + runVM, err := vmo.GenerateVirtualMachine(vm) if err != nil { return fmt.Errorf("error generating Kubevirt VM: %v", err) @@ -545,37 +574,37 @@ func (h *virtualMachineHandler) createVirtualMachine(vm *migration.VirtualMachin runVM.Spec.Template.Spec.Volumes = vmVols runVM.Spec.Template.Spec.Domain.Devices.Disks = disks - // apply virtualmachineimport annotation + // Apply annotations to the `VirtualMachine` object to make the newly + // created VM identifiable. if runVM.GetAnnotations() == nil { runVM.Annotations = make(map[string]string) } - runVM.Annotations[vmiAnnotation] = fmt.Sprintf("%s-%s", vm.Name, vm.Namespace) + runVM.Annotations[annotationVirtualMachineImport] = fmt.Sprintf("%s-%s", vm.Name, vm.Namespace) + // Make sure the new VM is created only if it does not exist. found := false - existingVMO, err := h.kubevirt.Get(runVM.Namespace, runVM.Name, metav1.GetOptions{}) + existingVM, err := h.kubevirt.Get(runVM.Namespace, runVM.Name, metav1.GetOptions{}) if err == nil { - if existingVMO.Annotations[vmiAnnotation] == fmt.Sprintf("%s-%s", vm.Name, vm.Namespace) { + if existingVM.Annotations[annotationVirtualMachineImport] == fmt.Sprintf("%s-%s", vm.Name, vm.Namespace) { found = true - vm.Status.NewVirtualMachine = existingVMO.Name } } if !found { - runVMObj, err := h.kubevirt.Create(runVM) + _, err := h.kubevirt.Create(runVM) if err != nil { - return fmt.Errorf("error creating kubevirt VM in createVirtualMachine :%v", err) + return fmt.Errorf("error creating kubevirt VM in createVirtualMachine: %v", err) } - vm.Status.NewVirtualMachine = runVMObj.Name } return nil } func (h *virtualMachineHandler) checkVirtualMachine(vm *migration.VirtualMachineImport) (bool, error) { - vmObj, err := h.kubevirt.Get(vm.Namespace, vm.Status.NewVirtualMachine, metav1.GetOptions{}) + vmObj, err := h.kubevirt.Get(vm.Namespace, vm.Status.DefiniteVirtualMachineName, metav1.GetOptions{}) if err != nil { - return false, fmt.Errorf("error querying kubevirt vm in checkVirtualMachine :%v", err) + return false, fmt.Errorf("error querying kubevirt vm in checkVirtualMachine: %v", err) } return vmObj.Status.Ready, nil @@ -706,7 +735,7 @@ func generateAnnotations(vm *migration.VirtualMachineImport, vmi *harvesterv1bet annotationSchemaOwners := ref.AnnotationSchemaOwners{} _ = annotationSchemaOwners.Add(kubevirt.VirtualMachineGroupVersionKind.GroupKind(), vm) var schemaID = ref.GroupKindToSchemaID(kubevirt.VirtualMachineGroupVersionKind.GroupKind()) - var ownerRef = ref.Construct(vm.GetNamespace(), vm.Spec.VirtualMachineName) + var ownerRef = ref.Construct(vm.GetNamespace(), vm.Status.DefiniteVirtualMachineName) schemaRef := ref.AnnotationSchemaReference{SchemaID: schemaID, References: ref.NewAnnotationSchemaOwnerReferences()} schemaRef.References.Insert(ownerRef) annotationSchemaOwners[schemaID] = schemaRef @@ -723,15 +752,14 @@ func generateAnnotations(vm *migration.VirtualMachineImport, vmi *harvesterv1bet func (h *virtualMachineHandler) checkAndCreateVirtualMachineImage(vm *migration.VirtualMachineImport, d migration.DiskInfo) (*harvesterv1beta1.VirtualMachineImage, error) { imageList, err := h.vmi.Cache().List(vm.Namespace, labels.SelectorFromSet(map[string]string{ - imageDisplayName: fmt.Sprintf("vm-import-%s-%s", vm.Name, d.Name), + labelImageDisplayName: fmt.Sprintf("vm-import-%s-%s", vm.Name, d.Name), })) - 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), imageDisplayName, fmt.Sprintf("vm-import-%s-%s", vm.Name, d.Name)) + 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)) } if len(imageList) == 1 { @@ -752,7 +780,7 @@ func (h *virtualMachineHandler) checkAndCreateVirtualMachineImage(vm *migration. }, }, Labels: map[string]string{ - imageDisplayName: fmt.Sprintf("vm-import-%s-%s", vm.Name, d.Name), + labelImageDisplayName: fmt.Sprintf("vm-import-%s-%s", vm.Name, d.Name), }, }, Spec: harvesterv1beta1.VirtualMachineImageSpec{ @@ -769,5 +797,49 @@ func (h *virtualMachineHandler) checkAndCreateVirtualMachineImage(vm *migration. } } - return h.vmi.Create(vmi) + vmiObj, err := h.vmi.Create(vmi) + if err != nil { + return nil, fmt.Errorf("failed to create Kubevirt VMI (namespace=%s spec.displayName=%s): %v", vmi.Namespace, vmi.Spec.DisplayName, err) + } + + return vmiObj, nil +} + +func (h *virtualMachineHandler) sanitizeVirtualMachineImport(vm *migration.VirtualMachineImport) (*migration.VirtualMachineImport, error) { + vmo, err := h.generateVMO(vm) + if err != nil { + return nil, fmt.Errorf("error generating VMO in sanitizeVirtualMachineImport: %v", err) + } + + err = vmo.SanitizeVirtualMachineImport(vm) + if err != nil { + vm.Status.Status = migration.VirtualMachineImportInvalid + logrus.WithFields(logrus.Fields{ + "name": vm.Name, + "namespace": vm.Namespace, + "spec.virtualMachineName": vm.Spec.VirtualMachineName, + "status.definiteVirtualMachineName": vm.Status.DefiniteVirtualMachineName, + }).Errorf("Failed to sanitize the '%s' object: %v", vm.Kind, err) + } else { + // Make sure the DefiniteVirtualMachineName is RFC 1123 compliant. + if errs := validation.IsDNS1123Label(vm.Status.DefiniteVirtualMachineName); len(errs) != 0 { + vm.Status.Status = migration.VirtualMachineImportInvalid + logrus.WithFields(logrus.Fields{ + "name": vm.Name, + "namespace": vm.Namespace, + "spec.virtualMachineName": vm.Spec.VirtualMachineName, + "status.definiteVirtualMachineName": vm.Status.DefiniteVirtualMachineName, + }).Error("The definite name of the target VM is not RFC 1123 compliant") + } else { + vm.Status.Status = migration.SourceReady + logrus.WithFields(logrus.Fields{ + "name": vm.Name, + "namespace": vm.Namespace, + "spec.virtualMachineName": vm.Spec.VirtualMachineName, + "status.definiteVirtualMachineName": vm.Status.DefiniteVirtualMachineName, + }).Infof("The sanitization of the '%s' object was successful", vm.Kind) + } + } + + return h.importVM.UpdateStatus(vm) } diff --git a/pkg/controllers/migration/vmware.go b/pkg/controllers/migration/vmware.go index 07642af..8dc67dd 100644 --- a/pkg/controllers/migration/vmware.go +++ b/pkg/controllers/migration/vmware.go @@ -33,7 +33,7 @@ func RegisterVmwareController(ctx context.Context, vc migrationController.Vmware vc.OnChange(ctx, "vmware-migration-change", vHandler.OnSourceChange) } -func (h *vmwareHandler) OnSourceChange(_ string, v *migration.VmwareSource) (*migration.VmwareSource, error) { +func (h *vmwareHandler) OnSourceChange(key string, v *migration.VmwareSource) (*migration.VmwareSource, error) { if v == nil || v.DeletionTimestamp != nil { return v, nil } @@ -42,15 +42,18 @@ func (h *vmwareHandler) OnSourceChange(_ string, v *migration.VmwareSource) (*mi "kind": v.Kind, "name": v.Name, "namespace": v.Namespace, - }).Info("Reconciling migration source") + "key": key, + }).Info("Reconciling source") + if v.Status.Status != migration.ClusterReady { secretObj, err := h.secret.Get(v.Spec.Credentials.Namespace, v.Spec.Credentials.Name, metav1.GetOptions{}) if err != nil { return v, fmt.Errorf("error looking up secret for vmware migration: %v", err) } + client, err := vmware.NewClient(h.ctx, v.Spec.EndpointAddress, v.Spec.Datacenter, secretObj) if err != nil { - return v, fmt.Errorf("error generating vmware client for vmware migration: %s: %v", v.Name, err) + return v, fmt.Errorf("error generating vmware client for vmware migration '%s': %v", v.Name, err) } err = client.Verify() diff --git a/pkg/source/openstack/client.go b/pkg/source/openstack/client.go index 3bbbb7a..42cd86d 100644 --- a/pkg/source/openstack/client.go +++ b/pkg/source/openstack/client.go @@ -9,6 +9,7 @@ import ( "net/http" "os" "path/filepath" + "strings" "time" "github.com/google/uuid" @@ -243,6 +244,15 @@ func (c *Client) ExportVirtualMachine(vm *migration.VirtualMachineImport) error return fmt.Errorf("timeout waiting for volume %s to be available: %v", volObj.ID, err) } + logrus.WithFields(logrus.Fields{ + "name": vm.Name, + "namespace": vm.Namespace, + "spec.virtualMachineName": vm.Spec.VirtualMachineName, + "spec.sourceCluster.name": vm.Spec.SourceCluster.Name, + "spec.sourceCluster.kind": vm.Spec.SourceCluster.Kind, + "attachedVolumeID": v.ID, + }).Info("Creating a new image from a volume") + volImage, err := volumeactions.UploadImage(c.storageClient, volObj.ID, volumeactions.UploadImageOpts{ ImageName: imageName, DiskFormat: "raw", @@ -270,6 +280,15 @@ func (c *Client) ExportVirtualMachine(vm *migration.VirtualMachineImport) error time.Sleep(defaultInterval) } + logrus.WithFields(logrus.Fields{ + "name": vm.Name, + "namespace": vm.Namespace, + "spec.virtualMachineName": vm.Spec.VirtualMachineName, + "spec.sourceCluster.name": vm.Spec.SourceCluster.Name, + "spec.sourceCluster.kind": vm.Spec.SourceCluster.Kind, + "imageID": volImage.ImageID, + }).Info("Downloading an image") + contents, err := imagedata.Download(c.imageClient, volImage.ImageID).Extract() if err != nil { return err @@ -316,7 +335,7 @@ func (c *Client) PowerOffVirtualMachine(vm *migration.VirtualMachineImport) erro if err != nil { return fmt.Errorf("error generating compute client during poweroffvirtualmachine: %v", err) } - uuid, err := c.checkOrGetUUID(vm.Spec.VirtualMachineName) + serverUUID, err := c.checkOrGetUUID(vm.Spec.VirtualMachineName) if err != nil { return err } @@ -326,13 +345,12 @@ func (c *Client) PowerOffVirtualMachine(vm *migration.VirtualMachineImport) erro return err } if !ok { - return startstop.Stop(computeClient, uuid).ExtractErr() + return startstop.Stop(computeClient, serverUUID).ExtractErr() } return nil } func (c *Client) IsPoweredOff(vm *migration.VirtualMachineImport) (bool, error) { - s, err := c.findVM(vm.Spec.VirtualMachineName) if err != nil { return false, err @@ -370,7 +388,7 @@ func (c *Client) GenerateVirtualMachine(vm *migration.VirtualMachineImport) (*ku newVM := &kubevirt.VirtualMachine{ ObjectMeta: metav1.ObjectMeta{ - Name: vm.Spec.VirtualMachineName, + Name: vm.Status.DefiniteVirtualMachineName, Namespace: vm.Namespace, }, } @@ -387,7 +405,7 @@ func (c *Client) GenerateVirtualMachine(vm *migration.VirtualMachineImport) (*ku Template: &kubevirt.VirtualMachineInstanceTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - "harvesterhci.io/vmName": vm.Spec.VirtualMachineName, + "harvesterhci.io/vmName": vm.Status.DefiniteVirtualMachineName, }, }, Spec: kubevirt.VirtualMachineInstanceSpec{ @@ -696,6 +714,28 @@ func generateNetworkInfo(info map[string]interface{}) ([]networkInfo, error) { return uniqueNetworks, nil } +// SanitizeVirtualMachineImport is used to sanitize the VirtualMachineImport object. +func (c *Client) SanitizeVirtualMachineImport(vm *migration.VirtualMachineImport) error { + // If the given `spec.virtualMachineName` is a UUID, then we need to + // get the name from the OpenStack server object. + parsedUUID, err := uuid.Parse(vm.Spec.VirtualMachineName) + if err == nil { + vmObj, err := c.findVM(parsedUUID.String()) + if err != nil { + return err + } + vm.Status.DefiniteVirtualMachineName = vmObj.Name + } else { + vm.Status.DefiniteVirtualMachineName = vm.Spec.VirtualMachineName + } + + // Note, server objects might have upper case characters in OpenStack, + // so we need to convert them to lower case to be RFC 1123 compliant. + vm.Status.DefiniteVirtualMachineName = strings.ToLower(vm.Status.DefiniteVirtualMachineName) + + return nil +} + // writeRawImageFile Download and write the raw image file to the specified path in chunks of 32KiB. func writeRawImageFile(name string, src io.ReadCloser) error { dst, err := os.Create(name) diff --git a/pkg/source/vmware/client.go b/pkg/source/vmware/client.go index 2d28f21..7b39b50 100644 --- a/pkg/source/vmware/client.go +++ b/pkg/source/vmware/client.go @@ -155,7 +155,7 @@ func (c *Client) ExportVirtualMachine(vm *migration.VirtualMachineImport) (err e vmObj, err = c.findVM(vm.Spec.Folder, vm.Spec.VirtualMachineName) if err != nil { - return fmt.Errorf("error finding vm in ExportVirtualMacine: %v", err) + return fmt.Errorf("error finding vm in ExportVirtualMachine: %v", err) } lease, err = vmObj.Export(c.ctx) @@ -178,6 +178,17 @@ func (c *Client) ExportVirtualMachine(vm *migration.VirtualMachineImport) (err e i.Path = vm.Name + "-" + vm.Namespace + "-" + i.Path } + logrus.WithFields(logrus.Fields{ + "name": vm.Name, + "namespace": vm.Namespace, + "spec.virtualMachineName": vm.Spec.VirtualMachineName, + "spec.sourceCluster.name": vm.Spec.SourceCluster.Name, + "spec.sourceCluster.kind": vm.Spec.SourceCluster.Kind, + "deviceId": i.DeviceId, + "path": i.Path, + "size": i.Size, + }).Info("Downloading an image") + exportPath := filepath.Join(tmpPath, i.Path) err = lease.DownloadFile(c.ctx, exportPath, i, soap.DefaultDownload) if err != nil { @@ -188,6 +199,17 @@ func (c *Client) ExportVirtualMachine(vm *migration.VirtualMachineImport) (err e DiskSize: i.Size, BusType: adapterType(i.DeviceId), }) + } else { + logrus.WithFields(logrus.Fields{ + "name": vm.Name, + "namespace": vm.Namespace, + "spec.virtualMachineName": vm.Spec.VirtualMachineName, + "spec.sourceCluster.name": vm.Spec.SourceCluster.Name, + "spec.sourceCluster.kind": vm.Spec.SourceCluster.Kind, + "deviceId": i.DeviceId, + "path": i.Path, + "size": i.Size, + }).Info("Skipping an image") } } @@ -270,9 +292,10 @@ func (c *Client) GenerateVirtualMachine(vm *migration.VirtualMachineImport) (*ku if err != nil { return nil, fmt.Errorf("error quering vm in GenerateVirtualMachine: %v", err) } + newVM := &kubevirt.VirtualMachine{ ObjectMeta: metav1.ObjectMeta{ - Name: vm.Spec.VirtualMachineName, + Name: vm.Status.DefiniteVirtualMachineName, Namespace: vm.Namespace, }, } @@ -292,7 +315,7 @@ func (c *Client) GenerateVirtualMachine(vm *migration.VirtualMachineImport) (*ku Template: &kubevirt.VirtualMachineInstanceTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - "harvesterhci.io/vmName": vm.Spec.VirtualMachineName, + "harvesterhci.io/vmName": vm.Status.DefiniteVirtualMachineName, }, }, Spec: kubevirt.VirtualMachineInstanceSpec{ @@ -470,3 +493,9 @@ func adapterType(deviceID string) kubevirt.DiskBus { } return kubevirt.DiskBusSATA } + +// SanitizeVirtualMachineImport is used to sanitize the VirtualMachineImport object. +func (c *Client) SanitizeVirtualMachineImport(vm *migration.VirtualMachineImport) error { + vm.Status.DefiniteVirtualMachineName = vm.Spec.VirtualMachineName + return nil +} diff --git a/tests/integration/common_test.go b/tests/integration/common_test.go index 0da9cc4..57218db 100644 --- a/tests/integration/common_test.go +++ b/tests/integration/common_test.go @@ -78,11 +78,11 @@ var _ = Describe("perform valid dns names", func() { return err } - if vmObj.Status.Status == migration.VirtualMachineInvalid { + if vmObj.Status.Status == migration.VirtualMachineImportInvalid { return nil } - return fmt.Errorf("waiiting for vm obj to be marked invalid") + return fmt.Errorf("waiting for vm obj to be marked invalid") }, "30s", "5s").ShouldNot(HaveOccurred()) }) }) From f5cea3e7b373425df9785af504660a607bd073ec Mon Sep 17 00:00:00 2001 From: Volker Theile Date: Mon, 28 Oct 2024 13:02:12 +0100 Subject: [PATCH 2/3] 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 `--` to the name of the disk `-` to safe characters. This allows the import of VM with longer names. Related to: https://github.com/harvester/harvester/issues/6463 Signed-off-by: Volker Theile --- pkg/controllers/migration/virtualmachine.go | 26 ++++++++++++++------ pkg/source/openstack/client.go | 27 ++++++++++++++++++++- pkg/source/vmware/client.go | 4 +-- 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/pkg/controllers/migration/virtualmachine.go b/pkg/controllers/migration/virtualmachine.go index 8cfc05b..0eb53e5 100644 --- 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) @@ -751,15 +750,19 @@ func generateAnnotations(vm *migration.VirtualMachineImport, vmi *harvesterv1bet } func (h *virtualMachineHandler) checkAndCreateVirtualMachineImage(vm *migration.VirtualMachineImport, d migration.DiskInfo) (*harvesterv1beta1.VirtualMachineImage, error) { + // Note, the RFC 1123 compliant name of the disk is used here because + // the variable value is later used as a label. + imageDisplayName := fmt.Sprintf("vm-import-%s", d.Name) + 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)) + return nil, fmt.Errorf("unexpected error: found %d images with label %s=%s, only expected to find one", len(imageList), labelImageDisplayName, imageDisplayName) } if len(imageList) == 1 { @@ -780,11 +783,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", }, @@ -799,7 +806,7 @@ func (h *virtualMachineHandler) checkAndCreateVirtualMachineImage(vm *migration. vmiObj, err := h.vmi.Create(vmi) if err != nil { - return nil, fmt.Errorf("failed to create Kubevirt VMI (namespace=%s spec.displayName=%s): %v", vmi.Namespace, vmi.Spec.DisplayName, err) + return nil, fmt.Errorf("failed to create Kubevirt VMI (namespace=%s, spec.displayName=%s): %v", vmi.Namespace, vmi.Spec.DisplayName, err) } return vmiObj, nil @@ -815,16 +822,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.definiteVirtualMachineName": vm.Status.DefiniteVirtualMachineName, - }).Errorf("Failed to sanitize the '%s' object: %v", vm.Kind, err) + }).Errorf("Failed to sanitize the import spec: %v", err) } else { // Make sure the DefiniteVirtualMachineName is RFC 1123 compliant. if errs := validation.IsDNS1123Label(vm.Status.DefiniteVirtualMachineName); 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, @@ -833,11 +842,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.definiteVirtualMachineName": vm.Status.DefiniteVirtualMachineName, - }).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 index 42cd86d..67989d9 100644 --- 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" @@ -178,12 +179,36 @@ 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 { _, err := networks.Get(c.computeClient, nm.SourceNetwork).Extract() if err != nil { return fmt.Errorf("error getting source network '%s': %v", nm.SourceNetwork, err) } } + + // Make sure the `harvesterhci.io/imageDisplayName` label set to the + // VirtualMachineImages meets the DNS-1123 standard. + // Note, in order to be able to validate the label content, we must + // temporarily bring forward the step `SanitizeVirtualMachineImport`, + // which is actually carried out after the preflight checks, as we + // need the definitive VM name here. + sanitizedVM := vm.DeepCopy() + if err := c.SanitizeVirtualMachineImport(sanitizedVM); err != nil { + return fmt.Errorf("failed to sanitize the 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 err + } + for vIndex := range vmObj.AttachedVolumes { + imageDisplayName := fmt.Sprintf("vm-import-%s-%d.img", sanitizedVM.Status.DefiniteVirtualMachineName, vIndex) + if errs := validation.IsDNS1123Label(imageDisplayName); len(errs) != 0 { + return fmt.Errorf("the VM display image name '%s' will not be RFC 1123 compliant: %v", imageDisplayName, errs) + } + } + return nil } @@ -294,7 +319,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.DefiniteVirtualMachineName, vIndex) logrus.WithFields(logrus.Fields{ "name": vm.Name, diff --git a/pkg/source/vmware/client.go b/pkg/source/vmware/client.go index 7b39b50..14baf85 100644 --- a/pkg/source/vmware/client.go +++ b/pkg/source/vmware/client.go @@ -226,7 +226,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("vm-import-%s", d.Name), // URL: fmt.Sprintf("http://%s:%d/%s.img", server.Address(), server.DefaultPort(), d.Name), // }, @@ -238,7 +238,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() From 99bb1351854b809c85ed34ceb7f885b52c0473d1 Mon Sep 17 00:00:00 2001 From: Volker Theile Date: Mon, 28 Oct 2024 17:51:12 +0100 Subject: [PATCH 3/3] Fix networking Signed-off-by: Volker Theile --- pkg/source/openstack/client.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/source/openstack/client.go b/pkg/source/openstack/client.go index 67989d9..0e886fc 100644 --- a/pkg/source/openstack/client.go +++ b/pkg/source/openstack/client.go @@ -51,6 +51,7 @@ type Client struct { storageClient *gophercloud.ServiceClient computeClient *gophercloud.ServiceClient imageClient *gophercloud.ServiceClient + networkClient *gophercloud.ServiceClient } type ExtendedVolume struct { @@ -138,6 +139,11 @@ func NewClient(ctx context.Context, endpoint string, region string, secret *core return nil, fmt.Errorf("error generating image client: %v", err) } + networkClient, err := openstack.NewNetworkV2(client, endPointOpts) + if err != nil { + return nil, fmt.Errorf("error generating network client: %v", err) + } + return &Client{ ctx: ctx, pClient: client, @@ -145,6 +151,7 @@ func NewClient(ctx context.Context, endpoint string, region string, secret *core storageClient: storageClient, computeClient: computeClient, imageClient: imageClient, + networkClient: networkClient, }, nil } @@ -181,7 +188,7 @@ 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 { - _, err := networks.Get(c.computeClient, nm.SourceNetwork).Extract() + _, err := networks.Get(c.networkClient, nm.SourceNetwork).Extract() if err != nil { return fmt.Errorf("error getting source network '%s': %v", nm.SourceNetwork, err) }