allow unbound PVC to be adopted

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>

pkg/apiserver/webhooks/datavolume-validate_test.go

@@ -235,7 +235,7 @@ var _ = Describe("Validating Webhook", func() {
 			Expect(resp.Allowed).To(BeFalse())
 		})
 
-		It("should reject DataVolume with unbound PVC and adoption annotation", func() {
+		It("should accept DataVolume with unbound PVC and adoption annotation", func() {
 			dataVolume := newHTTPDataVolume("testDV", "http://www.example.com")
 			pvc := &corev1.PersistentVolumeClaim{
 				ObjectMeta: metav1.ObjectMeta{
@@ -248,7 +248,7 @@ var _ = Describe("Validating Webhook", func() {
 				Spec: *dataVolume.Spec.PVC,
 			}
 			resp := validateDataVolumeCreate(dataVolume, pvc)
-			Expect(resp.Allowed).To(BeFalse())
+			Expect(resp.Allowed).To(BeTrue())
 		})
 
 		It("should accept DataVolume with PVC and adoption featurgate set", func() {
@@ -265,7 +265,7 @@ var _ = Describe("Validating Webhook", func() {
 			Expect(resp.Allowed).To(BeTrue())
 		})
 
-		It("should reject DataVolume with unbound PVC and adoption featurgate set", func() {
+		It("should accept DataVolume with unbound PVC and adoption featurgate set", func() {
 			dataVolume := newHTTPDataVolume("testDV", "http://www.example.com")
 			pvc := &corev1.PersistentVolumeClaim{
 				ObjectMeta: metav1.ObjectMeta{
@@ -275,7 +275,7 @@ var _ = Describe("Validating Webhook", func() {
 				Spec: *dataVolume.Spec.PVC,
 			}
 			resp := validateDataVolumeCreateEx(dataVolume, []runtime.Object{pvc}, nil, nil, []string{"DataVolumeClaimAdoption"})
-			Expect(resp.Allowed).To(BeFalse())
+			Expect(resp.Allowed).To(BeTrue())
 		})
 
 		It("should reject DataVolume with PVC adoption annotation false and featuregate set", func() {

pkg/controller/common/util.go

@@ -2109,9 +2109,6 @@ func SetPvcAllowedAnnotations(obj metav1.Object, pvc *corev1.PersistentVolumeCla
 
 // ClaimMayExistBeforeDataVolume returns true if the PVC may exist before the DataVolume
 func ClaimMayExistBeforeDataVolume(c client.Client, pvc *corev1.PersistentVolumeClaim, dv *cdiv1.DataVolume) (bool, error) {
-	if IsUnbound(pvc) {
-		return false, nil
-	}
 	if ClaimIsPopulatedForDataVolume(pvc, dv) {
 		return true, nil
 	}

pkg/controller/datavolume/controller-base.go

@@ -701,9 +701,6 @@ func (r *ReconcilerBase) validatePVC(dv *cdiv1.DataVolume, pvc *corev1.Persisten
 			return err
 		}
 		if !requiresWork {
-			if cc.IsUnbound(pvc) {
-				return fmt.Errorf("unbound populated/adoptable PVC %s/%s", pvc.Namespace, pvc.Name)
-			}
 			if err := r.addOwnerRef(pvc, dv); err != nil {
 				return err
 			}
@@ -905,8 +902,12 @@ func (r *ReconcilerBase) updateStatus(req reconcile.Request, phaseSync *statusPh
 		} else {
 			switch pvc.Status.Phase {
 			case corev1.ClaimPending:
-				if err := r.updateStatusPVCPending(pvc, dvc, dataVolumeCopy, &event); err != nil {
-					return reconcile.Result{}, err
+				if requiresWork {
+					if err := r.updateStatusPVCPending(pvc, dvc, dataVolumeCopy, &event); err != nil {
+						return reconcile.Result{}, err
+					}
+				} else {
+					dataVolumeCopy.Status.Phase = cdiv1.Succeeded
 				}
 			case corev1.ClaimBound:
 				switch dataVolumeCopy.Status.Phase {
@@ -918,12 +919,12 @@ func (r *ReconcilerBase) updateStatus(req reconcile.Request, phaseSync *statusPh
 					dataVolumeCopy.Status.Phase = cdiv1.PVCBound
 				}
 
-				if !requiresWork {
-					dataVolumeCopy.Status.Phase = cdiv1.Succeeded
-				} else {
+				if requiresWork {
 					if err := dvc.updateStatusPhase(pvc, dataVolumeCopy, &event); err != nil {
 						return reconcile.Result{}, err
 					}
+				} else {
+					dataVolumeCopy.Status.Phase = cdiv1.Succeeded
 				}
 
 			case corev1.ClaimLost:

pkg/controller/datavolume/import-controller_test.go

@@ -790,7 +790,7 @@ var _ = Describe("All DataVolume Tests", func() {
 		})
 
 		It("Should adopt a PVC (with annotation)", func() {
-			annotations := map[string]string{"cdi.kubevirt.io/allowClaimAdoption": "true"}
+			annotations := map[string]string{AnnAllowClaimAdoption: "true"}
 			pvc := CreatePvc("test-dv", metav1.NamespaceDefault, annotations, nil)
 			pvc.Status.Phase = corev1.ClaimBound
 			dv := NewImportDataVolume("test-dv")
@@ -810,6 +810,28 @@ var _ = Describe("All DataVolume Tests", func() {
 			Expect(string(dv.Status.Progress)).To(Equal("N/A"))
 		})
 
+		It("Should adopt a unbound PVC (with annotation)", func() {
+			annotations := map[string]string{AnnAllowClaimAdoption: "true"}
+			pvc := CreatePvc("test-dv", metav1.NamespaceDefault, annotations, nil)
+			pvc.Spec.VolumeName = ""
+			pvc.Status.Phase = corev1.ClaimPending
+			dv := NewImportDataVolume("test-dv")
+			reconciler = createImportReconciler(pvc, dv)
+			_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})
+			Expect(err).ToNot(HaveOccurred())
+
+			err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, pvc)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(pvc.OwnerReferences).To(HaveLen(1))
+			or := pvc.OwnerReferences[0]
+			Expect(or.UID).To(Equal(dv.UID))
+
+			err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, dv)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(dv.Status.Phase).To(Equal(cdiv1.Succeeded))
+			Expect(string(dv.Status.Progress)).To(Equal("N/A"))
+		})
+
 		It("Should adopt a PVC (with featuregate)", func() {
 			pvc := CreatePvc("test-dv", metav1.NamespaceDefault, nil, nil)
 			pvc.Status.Phase = corev1.ClaimBound
@@ -955,7 +977,7 @@ var _ = Describe("All DataVolume Tests", func() {
 					Kind:       "DataVolume",
 					Name:       dv.Name,
 					UID:        dv.UID,
-					Controller: pointer.Bool(true),
+					Controller: ptr.To(true),
 				},
 			}
 

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

@@ -511,6 +511,34 @@ var _ = Describe("All DataVolume Tests", func() {
 			Expect(pvc.OwnerReferences[0].Kind).To(Equal("DataVolume"))
 		})
 
+		It("Validate clone will adopt unbound PVC (with annotation)", func() {
+			dv := newCloneDataVolume("test-dv")
+			storageProfile := createStorageProfile(scName, nil, FilesystemMode)
+			pvc := CreatePvcInStorageClass("test-dv", metav1.NamespaceDefault, &scName, nil, nil, corev1.ClaimPending)
+			pvc.Spec.VolumeName = ""
+			pvc.SetAnnotations(make(map[string]string))
+			pvc.GetAnnotations()[AnnAllowClaimAdoption] = "true"
+			reconciler = createCloneReconciler(dv, pvc, storageProfile, sc)
+
+			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())
+			Expect(result.RequeueAfter).To(BeZero())
+
+			err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, dv)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(dv.Status.ClaimName).To(Equal("test-dv"))
+			Expect(dv.Status.Phase).To(Equal(cdiv1.Succeeded))
+			Expect(dv.Annotations[AnnCloneType]).To(BeEmpty())
+
+			pvc = &corev1.PersistentVolumeClaim{}
+			err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, pvc)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(pvc.OwnerReferences).To(HaveLen(1))
+			Expect(pvc.OwnerReferences[0].Name).To(Equal("test-dv"))
+			Expect(pvc.OwnerReferences[0].Kind).To(Equal("DataVolume"))
+		})
+
 		It("Validate clone will adopt PVC (with featuregate)", func() {
 			dv := newCloneDataVolume("test-dv")
 			storageProfile := createStorageProfile(scName, nil, FilesystemMode)

pkg/controller/datavolume/upload-controller_test.go

@@ -33,14 +33,15 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/tools/record"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
 	cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1"
 	"kubevirt.io/containerized-data-importer/pkg/common"
 	. "kubevirt.io/containerized-data-importer/pkg/controller/common"
 	"kubevirt.io/containerized-data-importer/pkg/controller/populators"
 	featuregates "kubevirt.io/containerized-data-importer/pkg/feature-gates"
-	"sigs.k8s.io/controller-runtime/pkg/client/fake"
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
-	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
 var (
@@ -205,7 +206,7 @@ var _ = Describe("All DataVolume Tests", func() {
 	})
 
 	It("Should adopt a PVC (with annotation)", func() {
-		annotations := map[string]string{"cdi.kubevirt.io/allowClaimAdoption": "true"}
+		annotations := map[string]string{AnnAllowClaimAdoption: "true"}
 		pvc := CreatePvc("test-dv", metav1.NamespaceDefault, annotations, nil)
 		pvc.Status.Phase = corev1.ClaimBound
 		dv := newUploadDataVolume("test-dv")
@@ -246,16 +247,26 @@ var _ = Describe("All DataVolume Tests", func() {
 		Expect(string(dv.Status.Progress)).To(Equal("N/A"))
 	})
 
-	It("Should NOT adopt a unbound PVC", func() {
-		annotations := map[string]string{"cdi.kubevirt.io/allowClaimAdoption": "true"}
+	It("Should adopt a unbound PVC", func() {
+		annotations := map[string]string{AnnAllowClaimAdoption: "true"}
 		pvc := CreatePvc("test-dv", metav1.NamespaceDefault, annotations, nil)
 		pvc.Spec.VolumeName = ""
 		pvc.Status.Phase = corev1.ClaimPending
 		dv := newUploadDataVolume("test-dv")
 		reconciler = createUploadReconciler(pvc, dv)
 		_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})
-		Expect(err).To(HaveOccurred())
-		Expect(err.Error()).To(ContainSubstring("unbound populated/adoptable PVC"))
+		Expect(err).ToNot(HaveOccurred())
+
+		err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, pvc)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(pvc.OwnerReferences).To(HaveLen(1))
+		or := pvc.OwnerReferences[0]
+		Expect(or.UID).To(Equal(dv.UID))
+
+		err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, dv)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(dv.Status.Phase).To(Equal(cdiv1.Succeeded))
+		Expect(string(dv.Status.Progress)).To(Equal("N/A"))
 	})
 
 	var _ = Describe("Reconcile Datavolume status", func() {