address comments

Signed-off-by: Arik Hadas <[email protected]>
kubev2v · Dec 25, 2023 · 8c38ac2 · 8c38ac2
1 parent cea3700
commit 8c38ac2
Show file tree

Hide file tree

Showing 9 changed files with 47 additions and 40 deletions.
diff --git a/pkg/controller/plan/adapter/base/doc.go b/pkg/controller/plan/adapter/base/doc.go
@@ -63,7 +63,7 @@ type Builder interface {
 	// check whether the builder supports Volume Populators
 	SupportsVolumePopulators() bool
 	// Build populator volumes
-	PopulatorVolumes(vmRef ref.Ref, annotations map[string]string, secretName string) (pvcNames []string, err error)
+	PopulatorVolumes(vmRef ref.Ref, annotations map[string]string, secretName string) ([]*core.PersistentVolumeClaim, error)
 	// Transferred bytes
 	PopulatorTransferredBytes(persistentVolumeClaim *core.PersistentVolumeClaim) (transferredBytes int64, err error)
 	// Set the populator PVC labels

diff --git a/pkg/controller/plan/adapter/ocp/builder.go b/pkg/controller/plan/adapter/ocp/builder.go
@@ -604,7 +604,7 @@ func (r *Builder) SupportsVolumePopulators() bool {
 	return false
 }
 
-func (r *Builder) PopulatorVolumes(vmRef ref.Ref, annotations map[string]string, secretName string) (pvcNames []string, err error) {
+func (r *Builder) PopulatorVolumes(vmRef ref.Ref, annotations map[string]string, secretName string) (pvcs []*core.PersistentVolumeClaim, err error) {
 	err = planbase.VolumePopulatorNotSupportedError
 	return
 }

diff --git a/pkg/controller/plan/adapter/openstack/builder.go b/pkg/controller/plan/adapter/openstack/builder.go
@@ -870,7 +870,7 @@ func (r *Builder) SupportsVolumePopulators() bool {
 	return true
 }
 
-func (r *Builder) PopulatorVolumes(vmRef ref.Ref, annotations map[string]string, secretName string) (pvcNames []string, err error) {
+func (r *Builder) PopulatorVolumes(vmRef ref.Ref, annotations map[string]string, secretName string) (pvcs []*core.PersistentVolumeClaim, err error) {
 	workload := &model.Workload{}
 	err = r.Source.Inventory.Find(workload, vmRef)
 	if err != nil {
@@ -897,9 +897,7 @@ func (r *Builder) PopulatorVolumes(vmRef ref.Ref, annotations map[string]string,
 			continue
 		}
 		if pvc, pvcErr := r.getCorrespondingPvc(image, workload, vmRef, annotations, secretName); pvcErr == nil {
-			if pvc != nil {
-				pvcNames = append(pvcNames, pvc.Name)
-			}
+			pvcs = append(pvcs, pvc)
 		} else {
 			err = pvcErr
 			return
@@ -930,8 +928,7 @@ func (r *Builder) ensureVolumePopulator(workload *model.Workload, image *model.I
 }
 
 func (r *Builder) ensureVolumePopulatorPVC(workload *model.Workload, image *model.Image, annotations map[string]string, populatorName string) (pvc *core.PersistentVolumeClaim, err error) {
-	_, err = r.getVolumePopulatorPVC(image.ID)
-	if err != nil {
+	if pvc, err = r.getVolumePopulatorPVC(image.ID); err != nil {
 		if !k8serr.IsNotFound(err) {
 			err = liberr.Wrap(err)
 			return
@@ -1014,6 +1011,7 @@ func (r *Builder) createVolumePopulatorCR(image model.Image, secretName, vmId st
 	}
 	err = r.Context.Client.Create(context.TODO(), populatorCR, &client.CreateOptions{})
 	if err != nil {
+		err = liberr.Wrap(err)
 		return
 	}
 	name = populatorCR.Name
@@ -1144,6 +1142,9 @@ func (r *Builder) persistentVolumeClaimWithSourceRef(image model.Image, storageC
 	}
 
 	err = r.Client.Create(context.TODO(), pvc, &client.CreateOptions{})
+	if err != nil {
+		err = liberr.Wrap(err)
+	}
 	return
 }
 

diff --git a/pkg/controller/plan/adapter/openstack/client.go b/pkg/controller/plan/adapter/openstack/client.go
@@ -771,32 +771,33 @@ func (r *Client) ensureImagesFromVolumesReady(vm *libclient.VM) (ready bool, err
 			"vm", vm.Name)
 		return
 	}
-	ready = true
+	if len(vm.AttachedVolumes) != len(imagesFromVolumes) {
+		r.Log.Info("not all the images have been created",
+			"vm", vm.Name, "attachedVolumes", vm.AttachedVolumes, "imagesFromVolumes", imagesFromVolumes)
+		return
+	}
 	for _, image := range imagesFromVolumes {
-		ready, err = r.ensureImageFromVolumeReady(vm, &image)
-		if err != nil {
-			err = liberr.Wrap(err)
+		imageReady, imageReadyErr := r.ensureImageFromVolumeReady(vm, &image)
+		switch {
+		case imageReadyErr != nil:
+			err = liberr.Wrap(imageReadyErr)
 			return
-		}
-		if !ready {
-			continue
-		}
-		originalVolumeID := image.Properties[forkliftPropertyOriginalVolumeID].(string)
-		err = r.cleanup(vm, originalVolumeID)
-		if err != nil {
-			r.Log.Error(err, "cleaning the image",
+		case !imageReady:
+			r.Log.Info("found an image that is not ready",
 				"vm", vm.Name, "image", image.Name)
 			return
+		case imageReady:
+			originalVolumeID := image.Properties[forkliftPropertyOriginalVolumeID].(string)
+			err = r.cleanup(vm, originalVolumeID)
+			if err != nil {
+				r.Log.Error(err, "cleaning the image",
+					"vm", vm.Name, "image", image.Name)
+				return
+			}
 		}
 	}
-	if len(vm.AttachedVolumes) != len(imagesFromVolumes) {
-		r.Log.Info("not all the images have been created",
-			"vm", vm.Name, "attachedVolumes", vm.AttachedVolumes, "imagesFromVolumes", imagesFromVolumes)
-		ready = false
-	}
-	if ready {
-		r.Log.Info("all steps finished!", "vm", vm.Name)
-	}
+	ready = true
+	r.Log.Info("all steps finished!", "vm", vm.Name)
 	return
 }
 

diff --git a/pkg/controller/plan/adapter/ova/builder.go b/pkg/controller/plan/adapter/ova/builder.go
@@ -539,7 +539,7 @@ func (r *Builder) SupportsVolumePopulators() bool {
 	return false
 }
 
-func (r *Builder) PopulatorVolumes(vmRef ref.Ref, annotations map[string]string, secretName string) (pvcNames []string, err error) {
+func (r *Builder) PopulatorVolumes(vmRef ref.Ref, annotations map[string]string, secretName string) (pvcs []*core.PersistentVolumeClaim, err error) {
 	err = planbase.VolumePopulatorNotSupportedError
 	return
 }

diff --git a/pkg/controller/plan/adapter/ovirt/builder.go b/pkg/controller/plan/adapter/ovirt/builder.go
@@ -701,7 +701,7 @@ func (r *Builder) SupportsVolumePopulators() bool {
 	return !r.Context.Plan.Spec.Warm && r.Context.Plan.Provider.Destination.IsHost()
 }
 
-func (r *Builder) PopulatorVolumes(vmRef ref.Ref, annotations map[string]string, secretName string) (pvcNames []string, err error) {
+func (r *Builder) PopulatorVolumes(vmRef ref.Ref, annotations map[string]string, secretName string) (pvcs []*core.PersistentVolumeClaim, err error) {
 	workload := &model.Workload{}
 	err = r.Source.Inventory.Find(workload, vmRef)
 	if err != nil {
@@ -737,7 +737,7 @@ func (r *Builder) PopulatorVolumes(vmRef ref.Ref, annotations map[string]string,
 				err = nil
 				continue
 			}
-			pvcNames = append(pvcNames, pvc.Name)
+			pvcs = append(pvcs, pvc)
 		}
 	}
 	return

diff --git a/pkg/controller/plan/adapter/vsphere/builder.go b/pkg/controller/plan/adapter/vsphere/builder.go
@@ -835,7 +835,7 @@ func (r *Builder) SupportsVolumePopulators() bool {
 	return false
 }
 
-func (r *Builder) PopulatorVolumes(vmRef ref.Ref, annotations map[string]string, secretName string) (pvcNames []string, err error) {
+func (r *Builder) PopulatorVolumes(vmRef ref.Ref, annotations map[string]string, secretName string) (pvcs []*core.PersistentVolumeClaim, err error) {
 	err = planbase.VolumePopulatorNotSupportedError
 	return
 }

diff --git a/pkg/controller/plan/kubevirt.go b/pkg/controller/plan/kubevirt.go
@@ -471,7 +471,7 @@ func (r *KubeVirt) DataVolumes(vm *plan.VMStatus) (dataVolumes []cdi.DataVolume,
 	return
 }
 
-func (r *KubeVirt) PopulatorVolumes(vmRef ref.Ref) (pvcNames []string, err error) {
+func (r *KubeVirt) PopulatorVolumes(vmRef ref.Ref) (pvcs []*core.PersistentVolumeClaim, err error) {
 	secret, err := r.ensureSecret(vmRef, r.copyDataFromProviderSecret)
 	if err != nil {
 		err = liberr.Wrap(err)
@@ -535,8 +535,14 @@ func (r *KubeVirt) EnsureDataVolumes(vm *plan.VMStatus, dataVolumes []cdi.DataVo
 	return
 }
 
-func (r *KubeVirt) EnsurePopulatorVolumes(vm *plan.VMStatus, pvcNames []string) (err error) {
-	err = r.createPodToBindPVCs(vm, pvcNames)
+func (r *KubeVirt) EnsurePopulatorVolumes(vm *plan.VMStatus, pvcs []*core.PersistentVolumeClaim) (err error) {
+	var pendingPvcNames []string
+	for _, pvc := range pvcs {
+		if pvc.Status.Phase == core.ClaimPending {
+			pendingPvcNames = append(pendingPvcNames, pvc.Name)
+		}
+	}
+	err = r.createPodToBindPVCs(vm, pendingPvcNames)
 	if err != nil {
 		err = liberr.Wrap(err)
 	}
@@ -2058,7 +2064,7 @@ func (r *KubeVirt) CreatePvcForNfs(pvcName string) (err error) {
 			r.Log.Error(err, "Failed to get OVA plan PVC")
 			return false, err
 		}
-		return pvc.Status.Phase == "Bound", nil
+		return pvc.Status.Phase == core.ClaimBound, nil
 
 	}); err != nil {
 		r.Log.Error(err, "Failed to bind OVA PVC to PV ")

diff --git a/pkg/controller/plan/migration.go b/pkg/controller/plan/migration.go
@@ -700,9 +700,8 @@ func (r *Migration) execute(vm *plan.VMStatus) (err error) {
 		}
 
 		if r.builder.SupportsVolumePopulators() {
-			var pvcNames []string
-			pvcNames, err = r.kubevirt.PopulatorVolumes(vm.Ref)
-			if err != nil {
+			var pvcs []*core.PersistentVolumeClaim
+			if pvcs, err = r.kubevirt.PopulatorVolumes(vm.Ref); err != nil {
 				if !errors.As(err, &web.ProviderNotReadyError{}) {
 					r.Log.Error(err, "error creating volumes", "vm", vm.Name)
 					step.AddError(err.Error())
@@ -712,7 +711,7 @@ func (r *Migration) execute(vm *plan.VMStatus) (err error) {
 					return
 				}
 			}
-			err = r.kubevirt.EnsurePopulatorVolumes(vm, pvcNames)
+			err = r.kubevirt.EnsurePopulatorVolumes(vm, pvcs)
 			if err != nil {
 				if !errors.As(err, &web.ProviderNotReadyError{}) {
 					step.AddError(err.Error())