From 736e69852d7bafdaf67e856b844df626d268eee2 Mon Sep 17 00:00:00 2001 From: Arik Hadas Date: Mon, 25 Dec 2023 12:05:10 +0200 Subject: [PATCH] address comments Signed-off-by: Arik Hadas --- pkg/controller/plan/adapter/base/doc.go | 2 +- pkg/controller/plan/adapter/ocp/builder.go | 2 +- .../plan/adapter/openstack/builder.go | 13 +++++++------ .../plan/adapter/openstack/client.go | 18 ++++++++---------- pkg/controller/plan/adapter/ova/builder.go | 2 +- pkg/controller/plan/adapter/ovirt/builder.go | 4 ++-- pkg/controller/plan/adapter/vsphere/builder.go | 2 +- pkg/controller/plan/kubevirt.go | 14 ++++++++++---- pkg/controller/plan/migration.go | 7 +++---- 9 files changed, 34 insertions(+), 30 deletions(-) diff --git a/pkg/controller/plan/adapter/base/doc.go b/pkg/controller/plan/adapter/base/doc.go index f468a8455..7a2d5395b 100644 --- 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 index 1c32d52de..ca0982fc0 100644 --- 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 index 9a4e79096..990478c0d 100644 --- 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 index b7db2c20e..209a54cd6 100644 --- a/pkg/controller/plan/adapter/openstack/client.go +++ b/pkg/controller/plan/adapter/openstack/client.go @@ -771,14 +771,13 @@ func (r *Client) ensureImagesFromVolumesReady(vm *libclient.VM) (ready bool, err "vm", vm.Name) return } - ready = true for _, image := range imagesFromVolumes { - ready, err = r.ensureImageFromVolumeReady(vm, &image) - if err != nil { - err = liberr.Wrap(err) + imageReady, imageReadyErr := r.ensureImageFromVolumeReady(vm, &image) + if imageReadyErr != nil { + err = liberr.Wrap(imageReadyErr) return } - if !ready { + if !imageReady { continue } originalVolumeID := image.Properties[forkliftPropertyOriginalVolumeID].(string) @@ -789,13 +788,12 @@ func (r *Client) ensureImagesFromVolumesReady(vm *libclient.VM) (ready bool, err return } } - if len(vm.AttachedVolumes) != len(imagesFromVolumes) { + if len(vm.AttachedVolumes) == len(imagesFromVolumes) { + ready = true + r.Log.Info("all steps finished!", "vm", vm.Name) + } else { 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) } return } diff --git a/pkg/controller/plan/adapter/ova/builder.go b/pkg/controller/plan/adapter/ova/builder.go index e33482d72..5c6b84dd3 100644 --- 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 index fa43bd12e..3e9ba26cc 100644 --- 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 index 6ab8461d2..77240d5da 100644 --- 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 index 84603c7e0..1784d0bac 100644 --- 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 index 845dab646..b76a1e28f 100644 --- 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())