Support immediate binding with clone datavolume

Incase of immediate binding we fall back to host assisted
clone.

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

pkg/controller/clone/planner.go

@@ -272,7 +272,8 @@ func (p *Planner) watchOwned(log logr.Logger, obj client.Object) error {
 }
 
 func (p *Planner) computeStrategyForSourcePVC(ctx context.Context, args *ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error) {
-	if ok, err := p.validateTargetStorageClassAssignment(ctx, args); !ok || err != nil {
+	targetStorageClass, err := p.validateTargetStorageClassAssignment(ctx, args)
+	if targetStorageClass == nil || err != nil {
 		return nil, err
 	}
 
@@ -295,6 +296,14 @@ func (p *Planner) computeStrategyForSourcePVC(ctx context.Context, args *ChooseS
 		return nil, err
 	}
 
+	if targetStorageClass.VolumeBindingMode != nil &&
+		*targetStorageClass.VolumeBindingMode == storagev1.VolumeBindingWaitForFirstConsumer &&
+		cc.ImmediateBindingRequested(args.TargetClaim) {
+		args.Log.V(3).Info("Immediate binding requested, need to fall back to host assisted")
+		strategy := cdiv1.CloneStrategyHostAssisted
+		return &strategy, nil
+	}
+
 	strategy := cdiv1.CloneStrategySnapshot
 	cs, err := GetGlobalCloneStrategyOverride(ctx, p.Client)
 	if err != nil {
@@ -350,7 +359,8 @@ func (p *Planner) computeStrategyForSourceSnapshot(ctx context.Context, args *Ch
 	// Defaulting to host-assisted clone since it has fewer requirements
 	strategy := cdiv1.CloneStrategyHostAssisted
 
-	if ok, err := p.validateTargetStorageClassAssignment(ctx, args); !ok || err != nil {
+	targetStorageClass, err := p.validateTargetStorageClassAssignment(ctx, args)
+	if targetStorageClass == nil || err != nil {
 		return nil, err
 	}
 
@@ -367,14 +377,14 @@ func (p *Planner) computeStrategyForSourceSnapshot(ctx context.Context, args *Ch
 		return nil, nil
 	}
 
-	// Do snapshot and storage class validation
-	targetStorageClass, err := GetStorageClassForClaim(ctx, p.Client, args.TargetClaim)
-	if err != nil {
-		return nil, err
-	}
-	if targetStorageClass == nil {
-		return nil, fmt.Errorf("target claim's storageclass doesn't exist, clone will not work")
+	if targetStorageClass.VolumeBindingMode != nil &&
+		*targetStorageClass.VolumeBindingMode == storagev1.VolumeBindingWaitForFirstConsumer &&
+		cc.ImmediateBindingRequested(args.TargetClaim) {
+		args.Log.V(3).Info("Immediate binding requested, need to fall back to host assisted")
+		return &strategy, nil
 	}
+
+	// Do snapshot and storage class validation
 	valid, err := cc.ValidateSnapshotCloneProvisioners(ctx, p.Client, sourceSnapshot, targetStorageClass)
 	if err != nil {
 		return nil, err
@@ -396,28 +406,28 @@ func (p *Planner) computeStrategyForSourceSnapshot(ctx context.Context, args *Ch
 	return &strategy, nil
 }
 
-func (p *Planner) validateTargetStorageClassAssignment(ctx context.Context, args *ChooseStrategyArgs) (bool, error) {
+func (p *Planner) validateTargetStorageClassAssignment(ctx context.Context, args *ChooseStrategyArgs) (*storagev1.StorageClass, error) {
 	if args.TargetClaim.Spec.StorageClassName == nil {
 		args.Log.V(3).Info("Target PVC has nil storage class, cannot compute strategy")
-		return false, nil
+		return nil, nil
 	}
 
 	if *args.TargetClaim.Spec.StorageClassName == "" {
 		args.Log.V(3).Info("Target PVC has \"\" storage class, cannot compute strategy")
-		return false, fmt.Errorf("claim has emptystring storageclass, will not work")
+		return nil, fmt.Errorf("claim has emptystring storageclass, will not work")
 	}
 
 	sc, err := GetStorageClassForClaim(ctx, p.Client, args.TargetClaim)
 	if err != nil {
-		return false, err
+		return nil, err
 	}
 
 	if sc == nil {
 		args.Log.V(3).Info("Target PVC has no storage class, cannot compute strategy")
-		return false, fmt.Errorf("target storage class not found")
+		return nil, fmt.Errorf("target storage class not found")
 	}
 
-	return true, nil
+	return sc, nil
 }
 
 func (p *Planner) validateSourcePVC(args *ChooseStrategyArgs, sourceClaim *corev1.PersistentVolumeClaim) error {

pkg/controller/clone/planner_test.go

@@ -330,6 +330,22 @@ var _ = Describe("Planner test", func() {
 				expectEvent(planner, CloneValidationFailed)
 			})
 
+			It("should return host assited if wffc and immediate bind annotation", func() {
+				args := &ChooseStrategyArgs{
+					TargetClaim: createTargetClaim(),
+					DataSource:  createPVCDataSource(),
+					Log:         log,
+				}
+				storageClass := createStorageClass()
+				volumeMode := storagev1.VolumeBindingWaitForFirstConsumer
+				storageClass.VolumeBindingMode = &volumeMode
+				planner := createPlanner(storageClass, createSourceClaim())
+				strategy, err := planner.ChooseStrategy(context.Background(), args)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(strategy).ToNot(BeNil())
+				Expect(*strategy).To(Equal(cdiv1.CloneStrategyHostAssisted))
+			})
+
 			It("should return host assisted with no volumesnapshotclass", func() {
 				args := &ChooseStrategyArgs{
 					TargetClaim: createTargetClaim(),
@@ -534,6 +550,24 @@ var _ = Describe("Planner test", func() {
 				Expect(strategy).To(BeNil())
 			})
 
+			It("should return host assited if wffc and immediate bind annotation", func() {
+				source := createSourceSnapshot(sourceName, "test-snapshot-content-name", "vsc")
+				target := createTargetClaim()
+				args := &ChooseStrategyArgs{
+					TargetClaim: target,
+					DataSource:  createSnapshotDataSource(),
+					Log:         log,
+				}
+				storageClass := createStorageClass()
+				volumeMode := storagev1.VolumeBindingWaitForFirstConsumer
+				storageClass.VolumeBindingMode = &volumeMode
+				planner := createPlanner(storageClass, source, createDefaultVolumeSnapshotContent("test"))
+				strategy, err := planner.ChooseStrategy(context.Background(), args)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(strategy).ToNot(BeNil())
+				Expect(*strategy).To(Equal(cdiv1.CloneStrategyHostAssisted))
+			})
+
 			It("should fallback to host-assisted when snapshot and storage class provisioners differ", func() {
 				source := createSourceSnapshot(sourceName, "test-snapshot-content-name", "vsc")
 				target := createTargetClaim()

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 {

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,7 +1150,7 @@ func (r *ReconcilerBase) shouldBeMarkedPendingPopulation(pvc *corev1.PersistentV
 		return false, err
 	}
 	nodeName := pvc.Annotations[cc.AnnSelectedNode]
-	_, immediateBindingRequested := pvc.Annotations[cc.AnnImmediateBinding]
+	immediateBindingRequested := cc.ImmediateBindingRequested(pvc)
 
 	return wffc && nodeName == "" && !immediateBindingRequested, nil
 }

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/populators/util.go

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

tests/clone-populator_test.go

@@ -19,6 +19,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	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/tests/framework"
 	"kubevirt.io/containerized-data-importer/tests/utils"
 )
@@ -122,7 +123,7 @@ var _ = Describe("Clone Populator tests", func() {
 		return vcs
 	}
 
-	createTargetWithStrategy := func(sz resource.Quantity, vm corev1.PersistentVolumeMode, strategy, scName string) *corev1.PersistentVolumeClaim {
+	generateTargetPVCWithStrategy := func(sz resource.Quantity, vm corev1.PersistentVolumeMode, strategy, scName string) *corev1.PersistentVolumeClaim {
 		pvc := &corev1.PersistentVolumeClaim{
 			ObjectMeta: metav1.ObjectMeta{
 				Namespace: f.Namespace.Name,
@@ -151,6 +152,11 @@ var _ = Describe("Clone Populator tests", func() {
 				"cdi.kubevirt.io/cloneType": strategy,
 			}
 		}
+		return pvc
+	}
+
+	createTargetWithStrategy := func(sz resource.Quantity, vm corev1.PersistentVolumeMode, strategy, scName string) *corev1.PersistentVolumeClaim {
+		pvc := generateTargetPVCWithStrategy(sz, vm, strategy, scName)
 		err := f.CrClient.Create(context.Background(), pvc)
 		Expect(err).ToNot(HaveOccurred())
 		f.ForceSchedulingIfWaitForFirstConsumerPopulationPVC(pvc)
@@ -160,6 +166,17 @@ var _ = Describe("Clone Populator tests", func() {
 		return result
 	}
 
+	createTargetWithImmediateBinding := func(sz resource.Quantity, vm corev1.PersistentVolumeMode) *corev1.PersistentVolumeClaim {
+		pvc := generateTargetPVCWithStrategy(sz, vm, "", utils.DefaultStorageClass.GetName())
+		cc.AddAnnotation(pvc, cc.AnnImmediateBinding, "")
+		err := f.CrClient.Create(context.Background(), pvc)
+		Expect(err).ToNot(HaveOccurred())
+		result := &corev1.PersistentVolumeClaim{}
+		err = f.CrClient.Get(context.Background(), client.ObjectKeyFromObject(pvc), result)
+		Expect(err).ToNot(HaveOccurred())
+		return result
+	}
+
 	createTarget := func(sz resource.Quantity, vm corev1.PersistentVolumeMode) *corev1.PersistentVolumeClaim {
 		return createTargetWithStrategy(sz, vm, "", utils.DefaultStorageClass.GetName())
 	}
@@ -192,10 +209,10 @@ var _ = Describe("Clone Populator tests", func() {
 	}
 
 	Context("Clone from PVC", func() {
-		It("should do filesystem to filesystem clone", func() {
+		It("should do filesystem to filesystem clone, with immediateBinding annotation", func() {
 			source := createSource(defaultSize, corev1.PersistentVolumeFilesystem)
 			createDataSource()
-			target := createTarget(defaultSize, corev1.PersistentVolumeFilesystem)
+			target := createTargetWithImmediateBinding(defaultSize, corev1.PersistentVolumeFilesystem)
 			target = waitSucceeded(target)
 			sourceHash := getHash(source, 0)
 			targetHash := getHash(target, 0)

tests/cloner_test.go

@@ -900,9 +900,13 @@ var _ = Describe("all clone tests", func() {
 					By("Wait for target DV Succeeded phase")
 					err = utils.WaitForDataVolumePhase(f, f.Namespace.Name, cdiv1.Succeeded, targetDataVolume.Name)
 					Expect(err).ToNot(HaveOccurred())
-					Expect(targetPvc.Annotations[controller.AnnCloneRequest]).To(Equal(fmt.Sprintf("%s/%s", sourcePvc.Namespace, sourcePvc.Name)))
-					Expect(targetPvc.Spec.DataSource).To(BeNil())
-					Expect(targetPvc.Spec.DataSourceRef).To(BeNil())
+					if targetPvc.Spec.DataSourceRef != nil && targetPvc.Spec.DataSourceRef.Kind == cdiv1.VolumeCloneSourceRef {
+						Expect(targetPvc.Annotations[controller.AnnCloneType]).To(Equal(string(cdiv1.CloneStrategyHostAssisted)))
+					} else {
+						Expect(targetPvc.Annotations[controller.AnnCloneRequest]).To(Equal(fmt.Sprintf("%s/%s", sourcePvc.Namespace, sourcePvc.Name)))
+						Expect(targetPvc.Spec.DataSource).To(BeNil())
+						Expect(targetPvc.Spec.DataSourceRef).To(BeNil())
+					}
 
 					By("Verify content")
 					same, err := f.VerifyTargetPVCContentMD5(f.Namespace, targetPvc, utils.DefaultImagePath, utils.UploadFileMD5, utils.UploadFileSize)
@@ -2786,10 +2790,14 @@ var _ = Describe("all clone tests", func() {
 				By("Check host assisted clone is taking place")
 				pvc, err := f.K8sClient.CoreV1().PersistentVolumeClaims(dataVolume.Namespace).Get(context.TODO(), dataVolume.Name, metav1.GetOptions{})
 				Expect(err).ToNot(HaveOccurred())
-				suffix := "-host-assisted-source-pvc"
-				Expect(pvc.Annotations[controller.AnnCloneRequest]).To(HaveSuffix(suffix))
-				Expect(pvc.Spec.DataSource).To(BeNil())
-				Expect(pvc.Spec.DataSourceRef).To(BeNil())
+				if pvc.Spec.DataSourceRef != nil && pvc.Spec.DataSourceRef.Kind == cdiv1.VolumeCloneSourceRef {
+					Expect(pvc.Annotations[controller.AnnCloneType]).To(Equal(string(cdiv1.CloneStrategyHostAssisted)))
+				} else {
+					suffix := "-host-assisted-source-pvc"
+					Expect(pvc.Annotations[controller.AnnCloneRequest]).To(HaveSuffix(suffix))
+					Expect(pvc.Spec.DataSource).To(BeNil())
+					Expect(pvc.Spec.DataSourceRef).To(BeNil())
+				}
 
 				By("Verify content")
 				same, err := f.VerifyTargetPVCContentMD5(f.Namespace, pvc, utils.DefaultImagePath, utils.UploadFileMD5, utils.UploadFileSize)

tests/datasource_test.go

@@ -226,11 +226,15 @@ var _ = Describe("DataSource", func() {
 			dv, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv)
 			Expect(err).ToNot(HaveOccurred())
 
-			By("Verify DV conditions")
-			utils.WaitForConditions(f, dv.Name, dv.Namespace, time.Minute, pollingInterval,
-				&cdiv1.DataVolumeCondition{Type: cdiv1.DataVolumeBound, Status: corev1.ConditionUnknown, Message: "The source snapshot snap1 doesn't exist", Reason: dvc.CloneWithoutSource},
-				&cdiv1.DataVolumeCondition{Type: cdiv1.DataVolumeReady, Status: corev1.ConditionFalse, Reason: dvc.CloneWithoutSource},
-				&cdiv1.DataVolumeCondition{Type: cdiv1.DataVolumeRunning, Status: corev1.ConditionFalse})
+			pvc, err := utils.WaitForPVC(f.K8sClient, dv.Namespace, dv.Name)
+			Expect(err).ToNot(HaveOccurred())
+			if pvc.Spec.DataSourceRef == nil || pvc.Spec.DataSourceRef.Kind != cdiv1.VolumeCloneSourceRef {
+				By("Verify DV conditions")
+				utils.WaitForConditions(f, dv.Name, dv.Namespace, time.Minute, pollingInterval,
+					&cdiv1.DataVolumeCondition{Type: cdiv1.DataVolumeBound, Status: corev1.ConditionUnknown, Message: "The source snapshot snap1 doesn't exist", Reason: dvc.CloneWithoutSource},
+					&cdiv1.DataVolumeCondition{Type: cdiv1.DataVolumeReady, Status: corev1.ConditionFalse, Reason: dvc.CloneWithoutSource},
+					&cdiv1.DataVolumeCondition{Type: cdiv1.DataVolumeRunning, Status: corev1.ConditionFalse})
+			}
 			f.ExpectEvent(dv.Namespace).Should(ContainSubstring(dvc.CloneWithoutSource))
 
 			By("Create snapshot so the DataSource will be ready")