a bit more updates and UTs

Signed-off-by: Shelly Kagan <skagan@redhat.com>

pkg/controller/datavolume/controller-base.go

@@ -814,32 +814,9 @@ func (r ReconcilerBase) updateStatus(req reconcile.Request, phaseSync *statusPha
 		} else {
 			switch pvc.Status.Phase {
 			case corev1.ClaimPending:
-				usePopulator, err := CheckPVCUsingPopulators(pvc)
-				if err != nil {
+				if err := r.updateStatusPVCPending(pvc, dvc, dataVolumeCopy, &event); err != nil {
 					return reconcile.Result{}, err
 				}
-				if usePopulator {
-					// when using populators the target pvc phase will stay in pending
-					// until the population completes, hence if not wffc we should update
-					// the dv phase according to the pod phase
-					if shouldBeMarkedPendingPopulation, err := r.shouldBeMarkedPendingPopulation(pvc); err != nil {
-						return reconcile.Result{}, err
-					} else if shouldBeMarkedPendingPopulation {
-						dataVolumeCopy.Status.Phase = cdiv1.PendingPopulation
-					} else {
-						if err := dvc.updateStatusPhase(pvc, dataVolumeCopy, &event); err != nil {
-							return reconcile.Result{}, err
-						}
-					}
-				} else {
-					if shouldBeMarkedWaitForFirstConsumer, err := r.shouldBeMarkedWaitForFirstConsumer(pvc); err != nil {
-						return reconcile.Result{}, err
-					} else if shouldBeMarkedWaitForFirstConsumer {
-						dataVolumeCopy.Status.Phase = cdiv1.WaitForFirstConsumer
-					} else {
-						dataVolumeCopy.Status.Phase = cdiv1.Pending
-					}
-				}
 			case corev1.ClaimBound:
 				switch dataVolumeCopy.Status.Phase {
 				case cdiv1.Pending:
@@ -883,6 +860,38 @@ func (r ReconcilerBase) updateStatus(req reconcile.Request, phaseSync *statusPha
 	return result, r.emitEvent(dv, dataVolumeCopy, curPhase, currentCond, &event)
 }
 
+func (r ReconcilerBase) updateStatusPVCPending(pvc *corev1.PersistentVolumeClaim, dvc dvController, dataVolumeCopy *cdiv1.DataVolume, event *Event) error {
+	usePopulator, err := CheckPVCUsingPopulators(pvc)
+	if err != nil {
+		return err
+	}
+	if usePopulator {
+		// when using populators the target pvc phase will stay pending until the population completes,
+		// hence if not wffc we should update the dv phase according to the pod phase
+		shouldBeMarkedPendingPopulation, err := r.shouldBeMarkedPendingPopulation(pvc)
+		if err != nil {
+			return err
+		}
+		if shouldBeMarkedPendingPopulation {
+			dataVolumeCopy.Status.Phase = cdiv1.PendingPopulation
+		} else if err := dvc.updateStatusPhase(pvc, dataVolumeCopy, event); err != nil {
+			return err
+		}
+		return nil
+	}
+
+	shouldBeMarkedWaitForFirstConsumer, err := r.shouldBeMarkedWaitForFirstConsumer(pvc)
+	if err != nil {
+		return err
+	}
+	if shouldBeMarkedWaitForFirstConsumer {
+		dataVolumeCopy.Status.Phase = cdiv1.WaitForFirstConsumer
+	} else {
+		dataVolumeCopy.Status.Phase = cdiv1.Pending
+	}
+	return nil
+}
+
 func (r *ReconcilerBase) updateConditions(dataVolume *cdiv1.DataVolume, pvc *corev1.PersistentVolumeClaim, reason, message string) {
 	var anno map[string]string
 
@@ -1185,18 +1194,22 @@ func (r *ReconcilerBase) shouldUseCDIPopulator(syncState *dvSyncState) (bool, er
 		}
 		return boolUsePopulator, nil
 	}
+	log := r.log.WithValues("DataVolume", dv.Name, "Namespace", dv.Namespace)
 	// currently populators don't support retain pod annotation so don't use populators in that case
 	if retain := dv.Annotations[cc.AnnPodRetainAfterCompletion]; retain == "true" {
+		log.Info("Not using CDI populators, currently we don't support populators with retainAfterCompletion annotation")
 		return false, nil
 	}
 	// currently populators don't support immediate bind annotation so don't use populators in that case
 	if forceBind := dv.Annotations[cc.AnnImmediateBinding]; forceBind == "true" {
+		log.Info("Not using CDI populators, currently we don't support populators with bind immediate annotation")
 		return false, nil
 	}
 	// currently we don't support populator with import source of VDDK or Imageio
 	// or clone either from PVC nor snapshot
 	if dv.Spec.Source.Imageio != nil || dv.Spec.Source.VDDK != nil ||
 		dv.Spec.Source.PVC != nil || dv.Spec.Source.Snapshot != nil {
+		log.Info("Not using CDI populators, currently we don't support populators with Imageio/VDDk/Clone source")
 		return false, nil
 	}
 
@@ -1213,13 +1226,19 @@ func (r *ReconcilerBase) shouldUseCDIPopulator(syncState *dvSyncState) (bool, er
 	// is wffc but the honorWaitForFirstConsumer feature gate is disabled we can't
 	// do immediate bind anyways, instead do regular flow
 	if wffc && !honorWaitForFirstConsumerEnabled {
+		log.Info("Not using CDI populators, currently we don't WFFC storage with disabled honorWaitForFirstConsumer feature gate")
 		return false, nil
 	}
 
 	usePopulator, err := storageClassCSIDriverExists(r.client, r.log, syncState.pvcSpec.StorageClassName)
 	if err != nil {
 		return false, err
 	}
+	if !usePopulator {
+		if syncState.pvcSpec.StorageClassName != nil {
+			log.Info("Not using CDI populators, storage class is not a CSI storage", "storageClass", *syncState.pvcSpec.StorageClassName)
+		}
+	}
 
 	return usePopulator, nil
 }

pkg/controller/datavolume/import-controller.go

@@ -28,7 +28,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
 	cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1"
 
 	cc "kubevirt.io/containerized-data-importer/pkg/controller/common"
@@ -512,14 +511,10 @@ func (r *ImportReconciler) createVolumeImportSourceCR(syncState *dvSyncState) er
 	importSource := &cdiv1.VolumeImportSource{}
 	importSourceName := volumeImportSourceName(dv)
 
-	exists, err := cc.GetResource(context.TODO(), r.client, dv.Namespace, importSourceName, importSource)
-	if err != nil {
+	// check if import source already exists
+	if exists, err := cc.GetResource(context.TODO(), r.client, dv.Namespace, importSourceName, importSource); err != nil || exists {
 		return err
 	}
-	if exists {
-		// import source already exists
-		return nil
-	}
 
 	source := &cdiv1.ImportSourceType{}
 	if http := dv.Spec.Source.HTTP; http != nil {
@@ -566,17 +561,16 @@ func (r *ImportReconciler) createVolumeImportSourceCR(syncState *dvSyncState) er
 
 func (r *ImportReconciler) deleteVolumeImportSourceCR(syncState *dvSyncState) error {
 	importSourceName := volumeImportSourceName(syncState.dvMutated)
-	importSource := &cdiv1.VolumeImportSource{}
-	if err := r.client.Get(context.TODO(), types.NamespacedName{Name: importSourceName, Namespace: syncState.dvMutated.Namespace}, importSource); err != nil {
+	importSource := &cdiv1.VolumeImportSource{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      importSourceName,
+			Namespace: syncState.dvMutated.Namespace,
+		},
+	}
+	if err := r.client.Delete(context.TODO(), importSource); err != nil {
 		if !k8serrors.IsNotFound(err) {
 			return err
 		}
-	} else {
-		if err := r.client.Delete(context.TODO(), importSource); err != nil {
-			if !k8serrors.IsNotFound(err) {
-				return err
-			}
-		}
 	}
 
 	return nil

pkg/controller/datavolume/upload-controller.go

@@ -25,7 +25,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
 	cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1"
 	cc "kubevirt.io/containerized-data-importer/pkg/controller/common"
 	"kubevirt.io/containerized-data-importer/pkg/controller/populators"
@@ -241,14 +240,10 @@ func volumeUploadSourceName(dv *cdiv1.DataVolume) string {
 func (r *UploadReconciler) createVolumeUploadSourceCR(syncState *dvSyncState) error {
 	uploadSourceName := volumeUploadSourceName(syncState.dvMutated)
 	uploadSource := &cdiv1.VolumeUploadSource{}
-	exists, err := cc.GetResource(context.TODO(), r.client, syncState.dvMutated.Namespace, uploadSourceName, uploadSource)
-	if err != nil {
+	// check if uploadSource already exists
+	if exists, err := cc.GetResource(context.TODO(), r.client, syncState.dvMutated.Namespace, uploadSourceName, uploadSource); err != nil || exists {
 		return err
 	}
-	if exists {
-		// uploadSource already exists
-		return nil
-	}
 
 	uploadSource = &cdiv1.VolumeUploadSource{
 		ObjectMeta: metav1.ObjectMeta{
@@ -274,17 +269,16 @@ func (r *UploadReconciler) createVolumeUploadSourceCR(syncState *dvSyncState) er
 
 func (r *UploadReconciler) deleteVolumeUploadSourceCR(syncState *dvSyncState) error {
 	uploadSourceName := volumeUploadSourceName(syncState.dvMutated)
-	uploadSource := &cdiv1.VolumeUploadSource{}
-	if err := r.client.Get(context.TODO(), types.NamespacedName{Name: uploadSourceName, Namespace: syncState.dvMutated.Namespace}, uploadSource); err != nil {
+	uploadSource := &cdiv1.VolumeUploadSource{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      uploadSourceName,
+			Namespace: syncState.dv.Namespace,
+		},
+	}
+	if err := r.client.Delete(context.TODO(), uploadSource); err != nil {
 		if !k8serrors.IsNotFound(err) {
 			return err
 		}
-	} else {
-		if err := r.client.Delete(context.TODO(), uploadSource); err != nil {
-			if !k8serrors.IsNotFound(err) {
-				return err
-			}
-		}
 	}
 
 	return nil

pkg/controller/datavolume/util.go

@@ -307,7 +307,7 @@ func resolveVolumeSize(c client.Client, dvSpec cdiv1.DataVolumeSpec, pvcSpec *v1
 
 // storageClassCSIDriverExists returns true if the passed storage class has CSI drivers available
 func storageClassCSIDriverExists(client client.Client, log logr.Logger, storageClassName *string) (bool, error) {
-	log = log.WithName("getCsiDriverForStorageClass").V(3)
+	log = log.WithName("storageClassCSIDriverExists").V(3)
 
 	storageClass, err := cc.GetStorageClassByName(context.TODO(), client, storageClassName)
 	if err != nil {

pkg/controller/populators/import-populator_test.go

@@ -24,13 +24,16 @@ import (
 	"net/url"
 	"strconv"
 	"strings"
+	"time"
 
 	. "github.com/onsi/ginkgo"
+	"github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 
 	snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
 	corev1 "k8s.io/api/core/v1"
 	extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -258,6 +261,8 @@ var _ = Describe("Import populator tests", func() {
 			pvcPrime, err := reconciler.getPVCPrime(targetPvc)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(pvcPrime).ToNot(BeNil())
+			// make sure we didnt inflate size
+			Expect(pvcPrime.Spec.Resources.Requests[corev1.ResourceStorage]).To(Equal(resource.MustParse("1G")))
 			Expect(pvcPrime.GetAnnotations()).ToNot(BeNil())
 			Expect(pvcPrime.GetAnnotations()[AnnImmediateBinding]).To(Equal(""))
 			Expect(pvcPrime.GetAnnotations()[AnnUploadRequest]).To(Equal(""))
@@ -302,6 +307,57 @@ var _ = Describe("Import populator tests", func() {
 			Expect(err).To(Not(HaveOccurred()))
 			Expect(result).To(Not(BeNil()))
 		})
+
+		table.DescribeTable("should update target pvc with desired annotations from pvc prime", func(podPhase string) {
+			targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &sc.Name, nil, nil, corev1.ClaimPending)
+			targetPvc.Spec.DataSourceRef = dataSourceRef
+			volumeImportSource := getVolumeImportSource(true, metav1.NamespaceDefault)
+			pvcPrime := getPVCPrime(targetPvc, nil)
+			for _, ann := range desiredAnnotations {
+				AddAnnotation(pvcPrime, ann, "somevalue")
+			}
+			AddAnnotation(pvcPrime, AnnPodPhase, podPhase)
+			AddAnnotation(pvcPrime, "undesiredAnn", "somevalue")
+
+			pv := &corev1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "pv",
+				},
+				Spec: corev1.PersistentVolumeSpec{
+					ClaimRef: &corev1.ObjectReference{
+						Namespace: pvcPrime.Namespace,
+						Name:      pvcPrime.Name,
+					},
+				},
+			}
+			pvcPrime.Spec.VolumeName = pv.Name
+
+			By("Reconcile")
+			reconciler = createImportPopulatorReconciler(targetPvc, pvcPrime, pv, volumeImportSource, sc)
+			result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: targetPvcName, Namespace: metav1.NamespaceDefault}})
+			Expect(err).To(Not(HaveOccurred()))
+			Expect(result).ToNot(BeNil())
+			if podPhase == string(corev1.PodRunning) {
+				Expect(result.RequeueAfter).To(Equal(2 * time.Second))
+			} else {
+				Expect(result.RequeueAfter).To(Equal(0 * time.Second))
+			}
+
+			updatedPVC := &corev1.PersistentVolumeClaim{}
+			err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: targetPvcName, Namespace: metav1.NamespaceDefault}, updatedPVC)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(updatedPVC.GetAnnotations()).ToNot(BeNil())
+			for _, ann := range desiredAnnotations {
+				_, ok := updatedPVC.Annotations[ann]
+				Expect(ok).To(BeTrue())
+			}
+			_, ok := updatedPVC.Annotations["undesiredAnn"]
+			Expect(ok).To(BeFalse())
+		},
+			table.Entry("with pod running phase", string(corev1.PodRunning)),
+			table.Entry("with pod succeded phase", string(corev1.PodFailed)),
+			table.Entry("with pod succeded phase", string(corev1.PodSucceeded)),
+		)
 	})
 
 	var _ = Describe("Import populator progress report", func() {

pkg/controller/populators/populator-base.go

@@ -215,7 +215,9 @@ func (r *ReconcilerBase) createPVCPrime(pvc *corev1.PersistentVolumeClaim, sourc
 
 type updatePVCAnnotationsFunc func(pvc, pvcPrime *corev1.PersistentVolumeClaim)
 
-var desiredAnnotations = []string{cc.AnnPodPhase, cc.AnnPodReady, cc.AnnPodRestarts, cc.AnnPreallocationRequested, cc.AnnPreallocationApplied, cc.AnnRunningCondition, cc.AnnRunningConditionMessage, cc.AnnRunningConditionReason, cc.AnnBoundCondition, cc.AnnBoundConditionMessage, cc.AnnBoundConditionReason}
+var desiredAnnotations = []string{cc.AnnPodPhase, cc.AnnPodReady, cc.AnnPodRestarts,
+	cc.AnnPreallocationRequested, cc.AnnPreallocationApplied,
+	cc.AnnRunningCondition, cc.AnnRunningConditionMessage, cc.AnnRunningConditionReason}
 
 func (r *ReconcilerBase) updatePVCWithPVCPrimeAnnotations(pvc, pvcPrime *corev1.PersistentVolumeClaim, updateFunc updatePVCAnnotationsFunc) error {
 	pvcCopy := pvc.DeepCopy()

pkg/controller/populators/upload-populator_test.go

@@ -223,6 +223,52 @@ var _ = Describe("Datavolume controller reconcile loop", func() {
 		Expect(pvcPrime).ToNot(BeNil())
 		Expect(pvcPrime.Annotations[cc.AnnSelectedNode]).To(Equal("node01"))
 	})
+
+	DescribeTable("should update target pvc with desired annotations from pvc prime", func(podPhase string) {
+		pvc := newUploadPopulatorPVC("test-pvc")
+		cc.AddAnnotation(pvc, AnnPVCPrimeName, PVCPrimeName(pvc))
+		uploadPV := uploadPV(pvc)
+
+		volumeUploadSourceCR := newUploadPopulatorCR("", false)
+		scName := "test-sc"
+		sc := cc.CreateStorageClassWithProvisioner(scName, map[string]string{cc.AnnDefaultStorageClass: "true"}, map[string]string{}, "csi-plugin")
+		r := createUploadPopulatorReconciler(pvc, volumeUploadSourceCR, sc, uploadPV)
+
+		_, err := r.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-pvc", Namespace: metav1.NamespaceDefault}})
+		Expect(err).ToNot(HaveOccurred())
+		pvcPrime, err := r.getPVCPrime(pvc)
+		Expect(err).ToNot(HaveOccurred())
+
+		pvcPrime.Spec.VolumeName = "test-pv"
+		pvcPrime.UID = pvcPrimeUID
+		for _, ann := range desiredAnnotations {
+			cc.AddAnnotation(pvcPrime, ann, "somevalue")
+		}
+		cc.AddAnnotation(pvcPrime, cc.AnnPodPhase, podPhase)
+		cc.AddAnnotation(pvcPrime, "undesiredAnn", "somevalue")
+		err = r.client.Update(context.TODO(), pvcPrime)
+		Expect(err).ToNot(HaveOccurred())
+
+		By("Reconcile")
+		result, err := r.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-pvc", Namespace: metav1.NamespaceDefault}})
+		Expect(err).To(Not(HaveOccurred()))
+		Expect(result).ToNot(BeNil())
+
+		updatedPVC := &corev1.PersistentVolumeClaim{}
+		err = r.client.Get(context.TODO(), types.NamespacedName{Name: "test-pvc", Namespace: metav1.NamespaceDefault}, updatedPVC)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(updatedPVC.GetAnnotations()).ToNot(BeNil())
+		for _, ann := range desiredAnnotations {
+			_, ok := updatedPVC.Annotations[ann]
+			Expect(ok).To(BeTrue())
+		}
+		_, ok := updatedPVC.Annotations["undesiredAnn"]
+		Expect(ok).To(BeFalse())
+	},
+		Entry("with pod running phase", string(corev1.PodRunning)),
+		Entry("with pod succeded phase", string(corev1.PodFailed)),
+		Entry("with pod succeded phase", string(corev1.PodSucceeded)),
+	)
 })
 
 func newUploadPopulatorPVC(name string) *corev1.PersistentVolumeClaim {

tests/framework/pvc.go

@@ -77,8 +77,10 @@ func (f *Framework) ForceBindPvcIfDvIsWaitForFirstConsumer(dv *cdiv1.DataVolume)
 	pvc, err := utils.WaitForPVC(f.K8sClient, dv.Namespace, dv.Name)
 	gomega.Expect(err).ToNot(gomega.HaveOccurred(), "PVC should exist")
 	if f.IsBindingModeWaitForFirstConsumer(pvc.Spec.StorageClassName) {
+		// check if pvc is a population pvc but not from pvc or snapshot
 		if pvc.Spec.DataSourceRef != nil &&
-			dv.Spec.Source.PVC == nil && dv.Spec.Source.Snapshot == nil {
+			(dv.Spec.Source == nil || dv.Spec.Source.PVC == nil) &&
+			(dv.Spec.Source == nil || dv.Spec.Source.Snapshot == nil) {
 			err = utils.WaitForDataVolumePhase(f, dv.Namespace, cdiv1.PendingPopulation, dv.Name)
 			gomega.Expect(err).ToNot(gomega.HaveOccurred())
 			createConsumerPodForPopulationPVC(pvc, f)