Allow ImmediateBinding annotation when using populators (#2765)

* Allow ImmediateBind annotation when using populators

In case of using PVC with populators if the PVC has this
annotation we prevent from waiting for it to be schedueled
and we proceed with the process.
When using datavolumes with populators in case the dv has the annotation
it will be passed to the PVC. we prevent from being in pendingPopulation
in case the created pvc has the annotaion.
Plus when having honorWaitForFirstConsumer feature gate disabled we will
put on the target PVC the immediateBind annotation.
Now we allow to use populators when having the annotation the the
feature gate disabled.

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

* Add functional tests to population using PVCs

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

* Support immediate binding with clone datavolume

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

* Pass allowed annotations from target pvc to pvc prime

This annotations are used for the import/upload/clone
pods to define netork configurations.

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

---------

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

pkg/controller/clone-controller.go

@@ -722,7 +722,7 @@ func MakeCloneSourcePodSpec(sourceVolumeMode corev1.PersistentVolumeMode, image,
 	}
 
 	pod.Spec.Containers[0].Env = append(pod.Spec.Containers[0].Env, addVars...)
-	setPodPvcAnnotations(pod, targetPvc)
+	cc.SetPvcAllowedAnnotations(pod, targetPvc)
 	cc.SetRestrictedSecurityContext(&pod.Spec)
 	return pod
 }

pkg/controller/common/util.go

@@ -648,6 +648,12 @@ func GetPreallocation(ctx context.Context, client client.Client, preallocation *
 	return cdiconfig.Status.Preallocation
 }
 
+// ImmediateBindingRequested returns if an object has the ImmediateBinding annotation
+func ImmediateBindingRequested(obj metav1.Object) bool {
+	_, isImmediateBindingRequested := obj.GetAnnotations()[AnnImmediateBinding]
+	return isImmediateBindingRequested
+}
+
 // GetPriorityClass gets PVC priority class
 func GetPriorityClass(pvc *corev1.PersistentVolumeClaim) string {
 	anno := pvc.GetAnnotations()
@@ -1327,7 +1333,7 @@ func GetCloneSourceInfo(dv *cdiv1.DataVolume) (sourceType, sourceName, sourceNam
 // IsWaitForFirstConsumerEnabled tells us if we should respect "real" WFFC behavior or just let our worker pods randomly spawn
 func IsWaitForFirstConsumerEnabled(obj metav1.Object, gates featuregates.FeatureGates) (bool, error) {
 	// when PVC requests immediateBinding it cannot honor wffc logic
-	_, isImmediateBindingRequested := obj.GetAnnotations()[AnnImmediateBinding]
+	isImmediateBindingRequested := ImmediateBindingRequested(obj)
 	pvcHonorWaitForFirstConsumer := !isImmediateBindingRequested
 	globalHonorWaitForFirstConsumer, err := gates.HonorWaitForFirstConsumerEnabled()
 	if err != nil {
@@ -1337,6 +1343,18 @@ func IsWaitForFirstConsumerEnabled(obj metav1.Object, gates featuregates.Feature
 	return pvcHonorWaitForFirstConsumer && globalHonorWaitForFirstConsumer, nil
 }
 
+// AddImmediateBindingAnnotationIfWFFCDisabled adds the immediateBinding annotation if wffc feature gate is disabled
+func AddImmediateBindingAnnotationIfWFFCDisabled(obj metav1.Object, gates featuregates.FeatureGates) error {
+	globalHonorWaitForFirstConsumer, err := gates.HonorWaitForFirstConsumerEnabled()
+	if err != nil {
+		return err
+	}
+	if !globalHonorWaitForFirstConsumer {
+		AddAnnotation(obj, AnnImmediateBinding, "")
+	}
+	return nil
+}
+
 // GetRequiredSpace calculates space required taking file system overhead into account
 func GetRequiredSpace(filesystemOverhead float64, requestedSpace int64) int64 {
 	// the `image` has to be aligned correctly, so the space requested has to be aligned to
@@ -1867,3 +1885,21 @@ func OwnedByDataVolume(obj metav1.Object) bool {
 	owner := metav1.GetControllerOf(obj)
 	return owner != nil && owner.Kind == "DataVolume"
 }
+
+// SetPvcAllowedAnnotations applies PVC annotations on the given obj
+func SetPvcAllowedAnnotations(obj metav1.Object, pvc *corev1.PersistentVolumeClaim) {
+	allowedAnnotations := map[string]string{
+		AnnPodNetwork:              "",
+		AnnPodSidecarInjection:     AnnPodSidecarInjectionDefault,
+		AnnPodMultusDefaultNetwork: ""}
+	for ann, def := range allowedAnnotations {
+		val, ok := pvc.Annotations[ann]
+		if !ok && def != "" {
+			val = def
+		}
+		if val != "" {
+			klog.V(1).Info("Applying PVC annotation", "Name", obj.GetName(), ann, val)
+			AddAnnotation(obj, ann, val)
+		}
+	}
+}

pkg/controller/datavolume/clone-controller-base.go

@@ -191,6 +191,9 @@ func (r *CloneReconcilerBase) updatePVCForPopulation(dataVolume *cdiv1.DataVolum
 	if err := addCloneToken(dataVolume, pvc); err != nil {
 		return err
 	}
+	if err := cc.AddImmediateBindingAnnotationIfWFFCDisabled(pvc, r.featureGates); err != nil {
+		return err
+	}
 	if isCrossNamespaceClone(dataVolume) {
 		_, _, sourcNamespace := cc.GetCloneSourceInfo(dataVolume)
 		cc.AddAnnotation(pvc, populators.AnnDataSourceNamespace, sourcNamespace)

pkg/controller/datavolume/controller-base.go

@@ -1150,8 +1150,9 @@ func (r *ReconcilerBase) shouldBeMarkedPendingPopulation(pvc *corev1.PersistentV
 		return false, err
 	}
 	nodeName := pvc.Annotations[cc.AnnSelectedNode]
+	immediateBindingRequested := cc.ImmediateBindingRequested(pvc)
 
-	return wffc && nodeName == "", nil
+	return wffc && nodeName == "" && !immediateBindingRequested, nil
 }
 
 // handlePvcCreation works as a wrapper for non-clone PVC creation and error handling
@@ -1200,11 +1201,6 @@ func (r *ReconcilerBase) shouldUseCDIPopulator(syncState *dvSyncState) (bool, er
 		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 ||
@@ -1213,23 +1209,6 @@ func (r *ReconcilerBase) shouldUseCDIPopulator(syncState *dvSyncState) (bool, er
 		return false, nil
 	}
 
-	wffc, err := r.storageClassWaitForFirstConsumer(syncState.pvcSpec.StorageClassName)
-	if err != nil {
-		return false, err
-	}
-
-	honorWaitForFirstConsumerEnabled, err := r.featureGates.HonorWaitForFirstConsumerEnabled()
-	if err != nil {
-		return false, err
-	}
-	// currently since we don't support force bind in populators in case the storage class
-	// 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

pkg/controller/datavolume/import-controller.go

@@ -131,6 +131,9 @@ func (r *ImportReconciler) updatePVCForPopulation(dataVolume *cdiv1.DataVolume,
 		dataVolume.Spec.Source.Blank == nil {
 		return errors.Errorf("no source set for import datavolume")
 	}
+	if err := cc.AddImmediateBindingAnnotationIfWFFCDisabled(pvc, r.featureGates); err != nil {
+		return err
+	}
 	apiGroup := cc.AnnAPIGroup
 	pvc.Spec.DataSourceRef = &corev1.TypedObjectReference{
 		APIGroup: &apiGroup,

pkg/controller/datavolume/import-controller_test.go

@@ -194,7 +194,7 @@ var _ = Describe("All DataVolume Tests", func() {
 				},
 			}
 			dv := NewImportDataVolume("test-dv")
-			reconciler = createImportReconciler(dv, sc, csiDriver)
+			reconciler = createImportReconcilerWFFCDisabled(dv, sc, csiDriver)
 			_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})
 			Expect(err).ToNot(HaveOccurred())
 			pvc := &corev1.PersistentVolumeClaim{}
@@ -208,6 +208,8 @@ var _ = Describe("All DataVolume Tests", func() {
 			Expect(pvc.Spec.DataSourceRef.Name).To(Equal(importSourceName))
 			Expect(pvc.Spec.DataSourceRef.Kind).To(Equal(cdiv1.VolumeImportSourceRef))
 			Expect(pvc.GetAnnotations()[AnnUsePopulator]).To(Equal("true"))
+			_, annExists := pvc.Annotations[AnnImmediateBinding]
+			Expect(annExists).To(BeTrue())
 		})
 
 		It("Should report import population progress when use populators", func() {
@@ -1566,7 +1568,6 @@ var _ = Describe("All DataVolume Tests", func() {
 			Entry("AnnUsePopulator=true return true", AnnUsePopulator, "true", true),
 			Entry("AnnUsePopulator=false return false", AnnUsePopulator, "false", false),
 			Entry("AnnPodRetainAfterCompletion return false", AnnPodRetainAfterCompletion, "true", false),
-			Entry("AnnImmediateBinding return false", AnnImmediateBinding, "true", false),
 		)
 
 		DescribeTable("Should return false if source is", func(source *cdiv1.DataVolumeSource) {
@@ -1588,19 +1589,20 @@ var _ = Describe("All DataVolume Tests", func() {
 			}),
 		)
 
-		It("Should return false if storage class has wffc bindingMode and honorWaitForFirstConsumer feature gate is disabled", func() {
+		It("Should return true if storage class has wffc bindingMode and honorWaitForFirstConsumer feature gate is disabled", func() {
 			sc := createStorageClassWithBindingMode(scName,
 				map[string]string{
 					AnnDefaultStorageClass: "true",
 				},
 				storagev1.VolumeBindingWaitForFirstConsumer)
+			sc.Provisioner = "csi-plugin"
 			httpSource := &cdiv1.DataVolumeSource{
 				HTTP: &cdiv1.DataVolumeSourceHTTP{},
 			}
 			storageSpec := &cdiv1.StorageSpec{}
 			dv := createDataVolumeWithStorageAPI("test-dv", metav1.NamespaceDefault, httpSource, storageSpec)
 
-			reconciler = createImportReconcilerWFFCDisabled(sc)
+			reconciler = createImportReconcilerWFFCDisabled(sc, csiDriver)
 			syncState := dvSyncState{
 				dvMutated: dv,
 				pvcSpec: &corev1.PersistentVolumeClaimSpec{
@@ -1609,7 +1611,7 @@ var _ = Describe("All DataVolume Tests", func() {
 			}
 			usePopulator, err := reconciler.shouldUseCDIPopulator(&syncState)
 			Expect(err).ToNot(HaveOccurred())
-			Expect(usePopulator).To(BeFalse())
+			Expect(usePopulator).To(BeTrue())
 		})
 
 		It("Should return false if storage class doesnt exist", func() {

pkg/controller/datavolume/pvc-clone-controller_test.go

@@ -123,7 +123,7 @@ var _ = Describe("All DataVolume Tests", func() {
 					dv.Finalizers = append(dv.Finalizers, crossNamespaceFinalizer)
 				}
 				srcPvc := CreatePvcInStorageClass("test", sourceNamespace, &scName, nil, nil, corev1.ClaimBound)
-				reconciler = createCloneReconciler(storageClass, csiDriver, dv, srcPvc)
+				reconciler = createCloneReconcilerWFFCDisabled(storageClass, csiDriver, dv, srcPvc)
 				result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})
 				Expect(err).ToNot(HaveOccurred())
 				Expect(result.Requeue).To(BeFalse())
@@ -146,6 +146,8 @@ var _ = Describe("All DataVolume Tests", func() {
 				Expect(pvc.Spec.DataSourceRef.Name).To(Equal(cloneSourceName))
 				Expect(pvc.Spec.DataSourceRef.Kind).To(Equal(cdiv1.VolumeCloneSourceRef))
 				Expect(pvc.GetAnnotations()[AnnUsePopulator]).To(Equal("true"))
+				_, annExists := pvc.Annotations[AnnImmediateBinding]
+				Expect(annExists).To(BeTrue())
 				vcs := &cdiv1.VolumeCloneSource{}
 				err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: cloneSourceName, Namespace: sourceNamespace}, vcs)
 				Expect(err).ToNot(HaveOccurred())
@@ -711,6 +713,20 @@ var _ = Describe("All DataVolume Tests", func() {
 	})
 })
 
+func createCloneReconcilerWFFCDisabled(objects ...runtime.Object) *PvcCloneReconciler {
+	cdiConfig := MakeEmptyCDIConfigSpec(common.ConfigName)
+	cdiConfig.Status = cdiv1.CDIConfigStatus{
+		ScratchSpaceStorageClass: testStorageClass,
+	}
+	cdiConfig.Spec.FeatureGates = []string{}
+
+	objs := []runtime.Object{}
+	objs = append(objs, objects...)
+	objs = append(objs, cdiConfig)
+
+	return createCloneReconcilerWithoutConfig(objs...)
+}
+
 func createCloneReconciler(objects ...runtime.Object) *PvcCloneReconciler {
 	cdiConfig := MakeEmptyCDIConfigSpec(common.ConfigName)
 	cdiConfig.Status = cdiv1.CDIConfigStatus{

pkg/controller/datavolume/snapshot-clone-controller_test.go

@@ -268,7 +268,7 @@ var _ = Describe("All DataVolume Tests", func() {
 						dv.Finalizers = append(dv.Finalizers, crossNamespaceFinalizer)
 					}
 					snapshot := createSnapshotInVolumeSnapshotClass("test-snap", sourceNamespace, &expectedSnapshotClass, nil, nil, true)
-					reconciler = createSnapshotCloneReconciler(storageClass, csiDriver, dv, snapshot)
+					reconciler = createSnapshotCloneReconcilerWFFCDisabled(storageClass, csiDriver, dv, snapshot)
 					result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})
 					Expect(err).ToNot(HaveOccurred())
 					Expect(result.Requeue).To(BeFalse())
@@ -291,6 +291,8 @@ var _ = Describe("All DataVolume Tests", func() {
 					Expect(pvc.Spec.DataSourceRef.Name).To(Equal(cloneSourceName))
 					Expect(pvc.Spec.DataSourceRef.Kind).To(Equal(cdiv1.VolumeCloneSourceRef))
 					Expect(pvc.GetAnnotations()[AnnUsePopulator]).To(Equal("true"))
+					_, annExists := pvc.Annotations[AnnImmediateBinding]
+					Expect(annExists).To(BeTrue())
 					vcs := &cdiv1.VolumeCloneSource{}
 					err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: cloneSourceName, Namespace: sourceNamespace}, vcs)
 					Expect(err).ToNot(HaveOccurred())
@@ -474,6 +476,20 @@ var _ = Describe("All DataVolume Tests", func() {
 	})
 })
 
+func createSnapshotCloneReconcilerWFFCDisabled(objects ...runtime.Object) *SnapshotCloneReconciler {
+	cdiConfig := MakeEmptyCDIConfigSpec(common.ConfigName)
+	cdiConfig.Status = cdiv1.CDIConfigStatus{
+		ScratchSpaceStorageClass: testStorageClass,
+	}
+	cdiConfig.Spec.FeatureGates = []string{}
+
+	objs := []runtime.Object{}
+	objs = append(objs, objects...)
+	objs = append(objs, cdiConfig)
+
+	return createSnapshotCloneReconcilerWithoutConfig(objs...)
+}
+
 func createSnapshotCloneReconciler(objects ...runtime.Object) *SnapshotCloneReconciler {
 	cdiConfig := MakeEmptyCDIConfigSpec(common.ConfigName)
 	cdiConfig.Status = cdiv1.CDIConfigStatus{

pkg/controller/datavolume/upload-controller.go

@@ -119,6 +119,9 @@ func (r *UploadReconciler) updatePVCForPopulation(dataVolume *cdiv1.DataVolume,
 	if dataVolume.Spec.Source.Upload == nil {
 		return errors.Errorf("no source set for upload datavolume")
 	}
+	if err := cc.AddImmediateBindingAnnotationIfWFFCDisabled(pvc, r.featureGates); err != nil {
+		return err
+	}
 	apiGroup := cc.AnnAPIGroup
 	pvc.Spec.DataSourceRef = &corev1.TypedObjectReference{
 		APIGroup: &apiGroup,

pkg/controller/import-controller.go

@@ -1180,7 +1180,7 @@ func setImporterPodCommons(pod *corev1.Pod, podEnvVar *importPodEnvVar, pvc *cor
 
 	pod.Spec.Containers[0].Env = makeImportEnv(podEnvVar, ownerUID)
 
-	setPodPvcAnnotations(pod, pvc)
+	cc.SetPvcAllowedAnnotations(pod, pvc)
 }
 
 func makeImporterContainerSpec(image, verbose, pullPolicy string) *corev1.Container {

pkg/controller/populators/populator-base.go

@@ -199,6 +199,7 @@ func (r *ReconcilerBase) createPVCPrime(pvc *corev1.PersistentVolumeClaim, sourc
 			Kind:    "PersistentVolumeClaim",
 		}),
 	}
+	cc.SetPvcAllowedAnnotations(pvcPrime, pvc)
 	util.SetRecommendedLabels(pvcPrime, r.installerLabels, "cdi-controller")
 
 	// We use the populator-specific pvcModifierFunc to add required annotations

pkg/controller/populators/util.go

@@ -120,7 +120,8 @@ func claimReadyForPopulation(ctx context.Context, c client.Client, pvc *corev1.P
 
 	if storageClass.VolumeBindingMode != nil && *storageClass.VolumeBindingMode == storagev1.VolumeBindingWaitForFirstConsumer {
 		nodeName = pvc.Annotations[cc.AnnSelectedNode]
-		if nodeName == "" {
+		isImmediateBindingRequested := cc.ImmediateBindingRequested(pvc)
+		if nodeName == "" && !isImmediateBindingRequested {
 			// Wait for the PVC to get a node name before continuing
 			return false, nodeName, nil
 		}

pkg/controller/upload-controller.go

@@ -893,7 +893,7 @@ func (r *UploadReconciler) makeUploadPodSpec(args UploadPodArgs, resourceRequire
 			MountPath: common.ScratchDataDir,
 		})
 	}
-	setPodPvcAnnotations(pod, args.PVC)
+	cc.SetPvcAllowedAnnotations(pod, args.PVC)
 	cc.SetNodeNameIfPopulator(args.PVC, &pod.Spec)
 	cc.SetRestrictedSecurityContext(&pod.Spec)
 	return pod

pkg/controller/util.go

@@ -374,27 +374,6 @@ func getScratchNameFromPod(pod *v1.Pod) (string, bool) {
 	return "", false
 }
 
-// setPodPvcAnnotations applies PVC annotations on the pod
-func setPodPvcAnnotations(pod *v1.Pod, pvc *v1.PersistentVolumeClaim) {
-	allowedAnnotations := map[string]string{
-		cc.AnnPodNetwork:              "",
-		cc.AnnPodSidecarInjection:     cc.AnnPodSidecarInjectionDefault,
-		cc.AnnPodMultusDefaultNetwork: ""}
-	for ann, def := range allowedAnnotations {
-		val, ok := pvc.Annotations[ann]
-		if !ok && def != "" {
-			val = def
-		}
-		if val != "" {
-			klog.V(1).Info("Applying PVC annotation on the pod", ann, val)
-			if pod.Annotations == nil {
-				pod.Annotations = map[string]string{}
-			}
-			pod.Annotations[ann] = val
-		}
-	}
-}
-
 func podUsingPVC(pvc *corev1.PersistentVolumeClaim, readOnly bool) *corev1.Pod {
 	return &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{