From 442114b5c978d306b6d448346ab47bfede1bbd64 Mon Sep 17 00:00:00 2001 From: Michael Henriksen Date: Fri, 15 Dec 2023 16:25:14 -0500 Subject: [PATCH] allow unbound PVC to be adopted Signed-off-by: Michael Henriksen --- .../webhooks/datavolume-validate_test.go | 8 +++--- pkg/controller/common/util.go | 3 -- pkg/controller/datavolume/controller-base.go | 17 +++++------ .../datavolume/import-controller_test.go | 24 +++++++++++++++- .../datavolume/pvc-clone-controller_test.go | 28 +++++++++++++++++++ .../datavolume/upload-controller_test.go | 27 ++++++++++++------ 6 files changed, 83 insertions(+), 24 deletions(-) diff --git a/pkg/apiserver/webhooks/datavolume-validate_test.go b/pkg/apiserver/webhooks/datavolume-validate_test.go index b39e5e67ee..38fbf1aa8b 100644 --- a/pkg/apiserver/webhooks/datavolume-validate_test.go +++ b/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() { diff --git a/pkg/controller/common/util.go b/pkg/controller/common/util.go index 4cb0107468..05a0e5fc1b 100644 --- a/pkg/controller/common/util.go +++ b/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 } diff --git a/pkg/controller/datavolume/controller-base.go b/pkg/controller/datavolume/controller-base.go index 6718ae71f5..baa58a4f77 100644 --- a/pkg/controller/datavolume/controller-base.go +++ b/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: diff --git a/pkg/controller/datavolume/import-controller_test.go b/pkg/controller/datavolume/import-controller_test.go index 3cfad2e4d0..792b4f2dc6 100644 --- a/pkg/controller/datavolume/import-controller_test.go +++ b/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 diff --git a/pkg/controller/datavolume/pvc-clone-controller_test.go b/pkg/controller/datavolume/pvc-clone-controller_test.go index 1fefc05bd4..48a415d10f 100644 --- a/pkg/controller/datavolume/pvc-clone-controller_test.go +++ b/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) diff --git a/pkg/controller/datavolume/upload-controller_test.go b/pkg/controller/datavolume/upload-controller_test.go index 0929899766..173f959dc7 100644 --- a/pkg/controller/datavolume/upload-controller_test.go +++ b/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() {