Check cdi.kubevirt.io/allowClaimAdoption on DataVolume rather than PVC

This allows for better backwards compatibality with DataVolumeTemplates

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

pkg/apiserver/webhooks/datavolume-validate_test.go

@@ -203,13 +203,13 @@ var _ = Describe("Validating Webhook", func() {
 
 		It("should accept DataVolume with PVC adoption annotation", func() {
 			dataVolume := newHTTPDataVolume("testDV", "http://www.example.com")
+			dataVolume.Annotations = map[string]string{
+				"cdi.kubevirt.io/allowClaimAdoption": "true",
+			}
 			pvc := &corev1.PersistentVolumeClaim{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      dataVolume.Name,
 					Namespace: dataVolume.Namespace,
-					Annotations: map[string]string{
-						"cdi.kubevirt.io/allowClaimAdoption": "true",
-					},
 				},
 				Spec: *dataVolume.Spec.PVC,
 			}
@@ -237,13 +237,13 @@ var _ = Describe("Validating Webhook", func() {
 
 		It("should accept DataVolume with unbound PVC and adoption annotation", func() {
 			dataVolume := newHTTPDataVolume("testDV", "http://www.example.com")
+			dataVolume.Annotations = map[string]string{
+				"cdi.kubevirt.io/allowClaimAdoption": "true",
+			}
 			pvc := &corev1.PersistentVolumeClaim{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      dataVolume.Name,
 					Namespace: dataVolume.Namespace,
-					Annotations: map[string]string{
-						"cdi.kubevirt.io/allowClaimAdoption": "true",
-					},
 				},
 				Spec: *dataVolume.Spec.PVC,
 			}
@@ -280,13 +280,13 @@ var _ = Describe("Validating Webhook", func() {
 
 		It("should reject DataVolume with PVC adoption annotation false and featuregate set", func() {
 			dataVolume := newHTTPDataVolume("testDV", "http://www.example.com")
+			dataVolume.Annotations = map[string]string{
+				"cdi.kubevirt.io/allowClaimAdoption": "false",
+			}
 			pvc := &corev1.PersistentVolumeClaim{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      dataVolume.Name,
 					Namespace: dataVolume.Namespace,
-					Annotations: map[string]string{
-						"cdi.kubevirt.io/allowClaimAdoption": "false",
-					},
 				},
 				Spec: *dataVolume.Spec.PVC,
 			}

pkg/controller/common/util.go

@@ -2126,17 +2126,20 @@ func ClaimMayExistBeforeDataVolume(c client.Client, pvc *corev1.PersistentVolume
 	if ClaimIsPopulatedForDataVolume(pvc, dv) {
 		return true, nil
 	}
-	return ClaimAllowsAdoption(c, pvc)
+	return AllowClaimAdoption(c, pvc, dv)
 }
 
 // ClaimIsPopulatedForDataVolume returns true if the PVC is populated for the given DataVolume
 func ClaimIsPopulatedForDataVolume(pvc *corev1.PersistentVolumeClaim, dv *cdiv1.DataVolume) bool {
-	return pvc.Annotations[AnnPopulatedFor] == dv.Name
+	return pvc != nil && dv != nil && pvc.Annotations[AnnPopulatedFor] == dv.Name
 }
 
-// ClaimAllowsAdoption returns true if the PVC may be adopted
-func ClaimAllowsAdoption(c client.Client, pvc *corev1.PersistentVolumeClaim) (bool, error) {
-	anno, ok := pvc.Annotations[AnnAllowClaimAdoption]
+// AllowClaimAdoption returns true if the PVC may be adopted
+func AllowClaimAdoption(c client.Client, pvc *corev1.PersistentVolumeClaim, dv *cdiv1.DataVolume) (bool, error) {
+	if pvc == nil || dv == nil {
+		return false, nil
+	}
+	anno, ok := dv.Annotations[AnnAllowClaimAdoption]
 	// if annotation exists, go with that regardless of featuregate
 	if ok {
 		val, _ := strconv.ParseBool(anno)

pkg/controller/datavolume/controller-base.go

@@ -83,7 +83,6 @@ var (
 
 	delayedAnnotations = []string{
 		cc.AnnPopulatedFor,
-		cc.AnnAllowClaimAdoption,
 	}
 )
 
@@ -1313,7 +1312,7 @@ func (r *ReconcilerBase) pvcRequiresWork(pvc *corev1.PersistentVolumeClaim, dv *
 	if pvcIsPopulatedForDataVolume(pvc, dv) {
 		return false, nil
 	}
-	canAdopt, err := cc.ClaimAllowsAdoption(r.client, pvc)
+	canAdopt, err := cc.AllowClaimAdoption(r.client, pvc, dv)
 	if err != nil {
 		return true, err
 	}

pkg/controller/datavolume/import-controller_test.go

@@ -196,15 +196,15 @@ var _ = Describe("All DataVolume Tests", func() {
 		It("Should create a PVC on a valid import DV without delayed annotation then add on success", func() {
 			dv := NewImportDataVolume("test-dv")
 			AddAnnotation(dv, "foo", "bar")
-			AddAnnotation(dv, AnnAllowClaimAdoption, "true")
+			AddAnnotation(dv, AnnPopulatedFor, "true")
 			reconciler = createImportReconciler(dv)
 			_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})
 			Expect(err).ToNot(HaveOccurred())
 			pvc := &corev1.PersistentVolumeClaim{}
 			err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, pvc)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(pvc.Annotations["foo"]).To(Equal("bar"))
-			Expect(pvc.Annotations).ToNot(HaveKey(AnnAllowClaimAdoption))
+			Expect(pvc.Annotations).ToNot(HaveKey(AnnPopulatedFor))
 
 			err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, dv)
 			Expect(err).ToNot(HaveOccurred())
@@ -216,7 +216,7 @@ var _ = Describe("All DataVolume Tests", func() {
 			err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, pvc)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(pvc.Annotations["foo"]).To(Equal("bar"))
-			Expect(pvc.Annotations[AnnAllowClaimAdoption]).To(Equal("true"))
+			Expect(pvc.Annotations[AnnPopulatedFor]).To(Equal("true"))
 		})
 
 		It("Should fail if dv source not import when use populators", func() {
@@ -799,10 +799,10 @@ var _ = Describe("All DataVolume Tests", func() {
 		})
 
 		It("Should adopt a PVC (with annotation)", func() {
-			annotations := map[string]string{AnnAllowClaimAdoption: "true"}
-			pvc := CreatePvc("test-dv", metav1.NamespaceDefault, annotations, nil)
+			pvc := CreatePvc("test-dv", metav1.NamespaceDefault, nil, nil)
 			pvc.Status.Phase = corev1.ClaimBound
 			dv := NewImportDataVolume("test-dv")
+			AddAnnotation(dv, AnnAllowClaimAdoption, "true")
 			reconciler = createImportReconciler(pvc, dv)
 			_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})
 			Expect(err).ToNot(HaveOccurred())
@@ -820,11 +820,11 @@ var _ = Describe("All DataVolume Tests", func() {
 		})
 
 		It("Should adopt a unbound PVC (with annotation)", func() {
-			annotations := map[string]string{AnnAllowClaimAdoption: "true"}
-			pvc := CreatePvc("test-dv", metav1.NamespaceDefault, annotations, nil)
+			pvc := CreatePvc("test-dv", metav1.NamespaceDefault, nil, nil)
 			pvc.Spec.VolumeName = ""
 			pvc.Status.Phase = corev1.ClaimPending
 			dv := NewImportDataVolume("test-dv")
+			AddAnnotation(dv, AnnAllowClaimAdoption, "true")
 			reconciler = createImportReconciler(pvc, dv)
 			_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})
 			Expect(err).ToNot(HaveOccurred())

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

@@ -485,10 +485,9 @@ var _ = Describe("All DataVolume Tests", func() {
 
 		It("Validate clone will adopt PVC (with annotation)", func() {
 			dv := newCloneDataVolume("test-dv")
+			AddAnnotation(dv, AnnAllowClaimAdoption, "true")
 			storageProfile := createStorageProfile(scName, nil, FilesystemMode)
 			pvc := CreatePvcInStorageClass("test-dv", metav1.NamespaceDefault, &scName, nil, nil, corev1.ClaimBound)
-			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}})
@@ -512,11 +511,10 @@ var _ = Describe("All DataVolume Tests", func() {
 
 		It("Validate clone will adopt unbound PVC (with annotation)", func() {
 			dv := newCloneDataVolume("test-dv")
+			AddAnnotation(dv, AnnAllowClaimAdoption, "true")
 			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}})

pkg/controller/datavolume/upload-controller_test.go

@@ -214,10 +214,10 @@ var _ = Describe("All DataVolume Tests", func() {
 	})
 
 	It("Should adopt a PVC (with annotation)", func() {
-		annotations := map[string]string{AnnAllowClaimAdoption: "true"}
-		pvc := CreatePvc("test-dv", metav1.NamespaceDefault, annotations, nil)
+		pvc := CreatePvc("test-dv", metav1.NamespaceDefault, nil, nil)
 		pvc.Status.Phase = corev1.ClaimBound
 		dv := newUploadDataVolume("test-dv")
+		AddAnnotation(dv, AnnAllowClaimAdoption, "true")
 		reconciler = createUploadReconciler(pvc, dv)
 		_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})
 		Expect(err).ToNot(HaveOccurred())
@@ -256,11 +256,11 @@ var _ = Describe("All DataVolume Tests", func() {
 	})
 
 	It("Should adopt a unbound PVC", func() {
-		annotations := map[string]string{AnnAllowClaimAdoption: "true"}
-		pvc := CreatePvc("test-dv", metav1.NamespaceDefault, annotations, nil)
+		pvc := CreatePvc("test-dv", metav1.NamespaceDefault, nil, nil)
 		pvc.Spec.VolumeName = ""
 		pvc.Status.Phase = corev1.ClaimPending
 		dv := newUploadDataVolume("test-dv")
+		AddAnnotation(dv, AnnAllowClaimAdoption, "true")
 		reconciler = createUploadReconciler(pvc, dv)
 		_, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}})
 		Expect(err).ToNot(HaveOccurred())

tests/adopt-volume_test.go

@@ -113,30 +113,23 @@ var _ = Describe("PVC adoption tests", func() {
 
 			DescribeTable("it should get adopted", func(defFunc func() *cdiv1.DataVolume) {
 				var pvcUID string
-				By("Adding annotation to PVC")
-				Eventually(func() error {
-					pvc, err := f.K8sClient.CoreV1().PersistentVolumeClaims(f.Namespace.Name).Get(context.TODO(), dvName, metav1.GetOptions{})
-					Expect(err).ToNot(HaveOccurred())
-					controller.AddAnnotation(pvc, controller.AnnAllowClaimAdoption, "true")
-					_, err = f.K8sClient.CoreV1().PersistentVolumeClaims(f.Namespace.Name).Update(context.TODO(), pvc, metav1.UpdateOptions{})
-					if err != nil {
-						return err
-					}
-					pvcUID = string(pvc.UID)
-					return nil
-				}, timeout, pollingInterval).Should(BeNil())
+				By("Getting PVC UID")
+				pvc, err := f.K8sClient.CoreV1().PersistentVolumeClaims(f.Namespace.Name).Get(context.TODO(), dvName, metav1.GetOptions{})
+				Expect(err).ToNot(HaveOccurred())
+				pvcUID = string(pvc.UID)
 
 				By("Creating target DV")
 				dvDef := defFunc()
 				controller.AddAnnotation(dvDef, controller.AnnDeleteAfterCompletion, "false")
+				controller.AddAnnotation(dvDef, controller.AnnAllowClaimAdoption, "true")
 				dv, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dvDef)
 				Expect(err).ToNot(HaveOccurred())
 
 				By("Waiting for target DV to succeed")
 				err = utils.WaitForDataVolumePhaseWithTimeout(f, f.Namespace.Name, cdiv1.Succeeded, dv.Name, 300*time.Second)
 				Expect(err).ToNot(HaveOccurred())
 
-				pvc, err := f.K8sClient.CoreV1().PersistentVolumeClaims(dv.Namespace).Get(context.TODO(), dv.Name, metav1.GetOptions{})
+				pvc, err = f.K8sClient.CoreV1().PersistentVolumeClaims(dv.Namespace).Get(context.TODO(), dv.Name, metav1.GetOptions{})
 				Expect(err).ToNot(HaveOccurred())
 				Expect(string(pvc.UID)).To(Equal(pvcUID))
 			},
@@ -194,21 +187,10 @@ var _ = Describe("PVC adoption tests", func() {
 			)
 
 			It("should not be adopted if annotation says not to", func() {
-				By("Adding annotation to PVC")
-				Eventually(func() error {
-					pvc, err := f.K8sClient.CoreV1().PersistentVolumeClaims(f.Namespace.Name).Get(context.TODO(), dvName, metav1.GetOptions{})
-					Expect(err).ToNot(HaveOccurred())
-					controller.AddAnnotation(pvc, controller.AnnAllowClaimAdoption, "false")
-					_, err = f.K8sClient.CoreV1().PersistentVolumeClaims(f.Namespace.Name).Update(context.TODO(), pvc, metav1.UpdateOptions{})
-					if err != nil {
-						return err
-					}
-					return nil
-				}, timeout, pollingInterval).Should(BeNil())
-
 				By("Creating target DV")
 				dvDef := importDef()
 				controller.AddAnnotation(dvDef, controller.AnnDeleteAfterCompletion, "false")
+				controller.AddAnnotation(dvDef, controller.AnnAllowClaimAdoption, "false")
 				_, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dvDef)
 				Expect(err).To(HaveOccurred())
 				Expect(err.Error()).To(ContainSubstring("already exists"))