From 211cd49067e594e61e7a3926f05d0bacf7bdd49b Mon Sep 17 00:00:00 2001 From: Michael Henriksen Date: Tue, 19 Mar 2024 13:56:54 -0400 Subject: [PATCH] cdi.kubevirt.io/allowClaimAdoption annotation breaking "regular" operations Have to better track when a PVC was created for a particular DV (populate it) or createed for someone else (adopt it) Fixes: https://issues.redhat.com/browse/CNV-39618 Signed-off-by: Michael Henriksen --- pkg/controller/common/util.go | 9 +- pkg/controller/datavolume/controller-base.go | 1 + .../datavolume/import-controller.go | 8 +- .../datavolume/import-controller_test.go | 13 ++- .../datavolume/pvc-clone-controller_test.go | 9 ++ .../datavolume/upload-controller.go | 8 +- .../datavolume/upload-controller_test.go | 9 ++ tests/datavolume_test.go | 83 +++++++++++++++++++ 8 files changed, 130 insertions(+), 10 deletions(-) diff --git a/pkg/controller/common/util.go b/pkg/controller/common/util.go index ec40dc9a03..835facb62d 100644 --- a/pkg/controller/common/util.go +++ b/pkg/controller/common/util.go @@ -329,6 +329,9 @@ const ( // AnnCdiCustomizeComponentHash annotation is a hash of all customizations that live under spec.CustomizeComponents AnnCdiCustomizeComponentHash = AnnAPIGroup + "/customizer-identifier" + + // AnnCreatedForDataVolume stores the UID of the datavolume that the PVC was created for + AnnCreatedForDataVolume = AnnAPIGroup + "/createdForDataVolume" ) // Size-detection pod error codes @@ -2139,7 +2142,11 @@ func AllowClaimAdoption(c client.Client, pvc *corev1.PersistentVolumeClaim, dv * if pvc == nil || dv == nil { return false, nil } - anno, ok := dv.Annotations[AnnAllowClaimAdoption] + anno, ok := pvc.Annotations[AnnCreatedForDataVolume] + if ok && anno == string(dv.UID) { + return false, nil + } + anno, ok = dv.Annotations[AnnAllowClaimAdoption] // if annotation exists, go with that regardless of featuregate if ok { val, _ := strconv.ParseBool(anno) diff --git a/pkg/controller/datavolume/controller-base.go b/pkg/controller/datavolume/controller-base.go index 45b0399fd0..514172adf2 100644 --- a/pkg/controller/datavolume/controller-base.go +++ b/pkg/controller/datavolume/controller-base.go @@ -1128,6 +1128,7 @@ func (r *ReconcilerBase) newPersistentVolumeClaim(dataVolume *cdiv1.DataVolume, annotations[cc.AnnPriorityClassName] = dataVolume.Spec.PriorityClassName } annotations[cc.AnnPreallocationRequested] = strconv.FormatBool(cc.GetPreallocation(context.TODO(), r.client, dataVolume.Spec.Preallocation)) + annotations[cc.AnnCreatedForDataVolume] = string(dataVolume.UID) if dataVolume.Spec.Storage != nil && labels[common.PvcApplyStorageProfileLabel] == "true" { isWebhookPvcRenderingEnabled, err := featuregates.IsWebhookPvcRenderingEnabled(r.client) diff --git a/pkg/controller/datavolume/import-controller.go b/pkg/controller/datavolume/import-controller.go index 5ad7fe233d..def1b2c8c3 100644 --- a/pkg/controller/datavolume/import-controller.go +++ b/pkg/controller/datavolume/import-controller.go @@ -239,6 +239,10 @@ func isPVCImportPopulation(pvc *corev1.PersistentVolumeClaim) bool { func (r *ImportReconciler) shouldUpdateStatusPhase(pvc *corev1.PersistentVolumeClaim, dv *cdiv1.DataVolume) (bool, error) { pvcCopy := pvc.DeepCopy() + requiresWork, err := r.pvcRequiresWork(pvcCopy, dv) + if err != nil { + return false, err + } if isPVCImportPopulation(pvcCopy) { // Better to play it safe and check the PVC Prime too // before updating DV phase. @@ -252,10 +256,6 @@ func (r *ImportReconciler) shouldUpdateStatusPhase(pvc *corev1.PersistentVolumeC } } _, ok := pvcCopy.Annotations[cc.AnnImportPod] - requiresWork, err := r.pvcRequiresWork(pvcCopy, dv) - if err != nil { - return false, err - } return ok && pvcCopy.Status.Phase == corev1.ClaimBound && requiresWork, nil } diff --git a/pkg/controller/datavolume/import-controller_test.go b/pkg/controller/datavolume/import-controller_test.go index 761df7f867..96785c242c 100644 --- a/pkg/controller/datavolume/import-controller_test.go +++ b/pkg/controller/datavolume/import-controller_test.go @@ -182,7 +182,8 @@ var _ = Describe("All DataVolume Tests", func() { }) It("Should create a PVC on a valid import DV", func() { - reconciler = createImportReconciler(NewImportDataVolume("test-dv")) + dv := NewImportDataVolume("test-dv") + 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{} @@ -191,6 +192,9 @@ var _ = Describe("All DataVolume Tests", func() { Expect(pvc.Name).To(Equal("test-dv")) Expect(pvc.Labels[common.AppKubernetesPartOfLabel]).To(Equal("testing")) Expect(pvc.Labels[common.KubePersistentVolumeFillingUpSuppressLabelKey]).To(Equal(common.KubePersistentVolumeFillingUpSuppressLabelValue)) + val, ok := pvc.Annotations[AnnCreatedForDataVolume] + Expect(ok).To(BeTrue()) + Expect(val).To(Equal(string(dv.UID))) }) It("Should create a PVC on a valid import DV without delayed annotation then add on success", func() { @@ -817,6 +821,8 @@ var _ = Describe("All DataVolume Tests", func() { Expect(err).ToNot(HaveOccurred()) Expect(dv.Status.Phase).To(Equal(cdiv1.Succeeded)) Expect(string(dv.Status.Progress)).To(Equal("N/A")) + _, ok := pvc.Annotations[AnnCreatedForDataVolume] + Expect(ok).To(BeFalse()) }) It("Should adopt a unbound PVC (with annotation)", func() { @@ -839,6 +845,8 @@ var _ = Describe("All DataVolume Tests", func() { Expect(err).ToNot(HaveOccurred()) Expect(dv.Status.Phase).To(Equal(cdiv1.Succeeded)) Expect(string(dv.Status.Progress)).To(Equal("N/A")) + _, ok := pvc.Annotations[AnnCreatedForDataVolume] + Expect(ok).To(BeFalse()) }) It("Should adopt a PVC (with featuregate)", func() { @@ -860,6 +868,8 @@ var _ = Describe("All DataVolume Tests", func() { Expect(err).ToNot(HaveOccurred()) Expect(dv.Status.Phase).To(Equal(cdiv1.Succeeded)) Expect(string(dv.Status.Progress)).To(Equal("N/A")) + _, ok := pvc.Annotations[AnnCreatedForDataVolume] + Expect(ok).To(BeFalse()) }) It("Should set multistage migration annotations on a newly created PVC", func() { @@ -2098,6 +2108,7 @@ func newUploadDataVolume(name string) *cdiv1.DataVolume { ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: metav1.NamespaceDefault, + UID: types.UID("uid"), }, Spec: cdiv1.DataVolumeSpec{ Source: &cdiv1.DataVolumeSource{ diff --git a/pkg/controller/datavolume/pvc-clone-controller_test.go b/pkg/controller/datavolume/pvc-clone-controller_test.go index 0d8e78fc7c..effe8c72f8 100644 --- a/pkg/controller/datavolume/pvc-clone-controller_test.go +++ b/pkg/controller/datavolume/pvc-clone-controller_test.go @@ -152,6 +152,9 @@ var _ = Describe("All DataVolume Tests", func() { Expect(err).ToNot(HaveOccurred()) Expect(vcs.Spec.Source.Kind).To(Equal("PersistentVolumeClaim")) Expect(vcs.Spec.Source.Name).To(Equal(srcPvc.Name)) + val, ok := pvc.Annotations[AnnCreatedForDataVolume] + Expect(val).To(Equal(string(dv.UID))) + Expect(ok).To(BeTrue()) }, Entry("with same namespace", metav1.NamespaceDefault), Entry("with different namespace", "source-ns"), @@ -507,6 +510,8 @@ var _ = Describe("All DataVolume Tests", func() { Expect(pvc.OwnerReferences).To(HaveLen(1)) Expect(pvc.OwnerReferences[0].Name).To(Equal("test-dv")) Expect(pvc.OwnerReferences[0].Kind).To(Equal("DataVolume")) + _, ok := pvc.Annotations[AnnCreatedForDataVolume] + Expect(ok).To(BeFalse()) }) It("Validate clone will adopt unbound PVC (with annotation)", func() { @@ -534,6 +539,8 @@ var _ = Describe("All DataVolume Tests", func() { Expect(pvc.OwnerReferences).To(HaveLen(1)) Expect(pvc.OwnerReferences[0].Name).To(Equal("test-dv")) Expect(pvc.OwnerReferences[0].Kind).To(Equal("DataVolume")) + _, ok := pvc.Annotations[AnnCreatedForDataVolume] + Expect(ok).To(BeFalse()) }) It("Validate clone will adopt PVC (with featuregate)", func() { @@ -560,6 +567,8 @@ var _ = Describe("All DataVolume Tests", func() { Expect(pvc.OwnerReferences).To(HaveLen(1)) Expect(pvc.OwnerReferences[0].Name).To(Equal("test-dv")) Expect(pvc.OwnerReferences[0].Kind).To(Equal("DataVolume")) + _, ok := pvc.Annotations[AnnCreatedForDataVolume] + Expect(ok).To(BeFalse()) }) DescribeTable("Validation mechanism rejects or accepts the clone depending on the contentType combination", diff --git a/pkg/controller/datavolume/upload-controller.go b/pkg/controller/datavolume/upload-controller.go index 635d656392..904430f8c9 100644 --- a/pkg/controller/datavolume/upload-controller.go +++ b/pkg/controller/datavolume/upload-controller.go @@ -195,6 +195,10 @@ func isPVCUploadPopulation(pvc *corev1.PersistentVolumeClaim) bool { func (r *UploadReconciler) shouldUpdateStatusPhase(pvc *corev1.PersistentVolumeClaim, dv *cdiv1.DataVolume) (bool, error) { pvcCopy := pvc.DeepCopy() + requiresWork, err := r.pvcRequiresWork(pvcCopy, dv) + if err != nil { + return false, err + } if isPVCUploadPopulation(pvcCopy) { // Better to play it safe and check the PVC Prime too // before updating DV phase. @@ -208,10 +212,6 @@ func (r *UploadReconciler) shouldUpdateStatusPhase(pvc *corev1.PersistentVolumeC } } _, ok := pvcCopy.Annotations[cc.AnnUploadRequest] - requiresWork, err := r.pvcRequiresWork(pvcCopy, dv) - if err != nil { - return false, err - } return ok && pvcCopy.Status.Phase == corev1.ClaimBound && requiresWork, nil } diff --git a/pkg/controller/datavolume/upload-controller_test.go b/pkg/controller/datavolume/upload-controller_test.go index 1642b9fd0d..c34652dc97 100644 --- a/pkg/controller/datavolume/upload-controller_test.go +++ b/pkg/controller/datavolume/upload-controller_test.go @@ -174,6 +174,9 @@ var _ = Describe("All DataVolume Tests", func() { uploadSourceName := volumeUploadSourceName(dv) Expect(pvc.Spec.DataSourceRef.Name).To(Equal(uploadSourceName)) Expect(pvc.Spec.DataSourceRef.Kind).To(Equal(cdiv1.VolumeUploadSourceRef)) + val, ok := pvc.Annotations[AnnCreatedForDataVolume] + Expect(ok).To(BeTrue()) + Expect(val).To(Equal(string(dv.UID))) }) It("Should always report NA progress for upload population", func() { @@ -232,6 +235,8 @@ var _ = Describe("All DataVolume Tests", func() { Expect(err).ToNot(HaveOccurred()) Expect(dv.Status.Phase).To(Equal(cdiv1.Succeeded)) Expect(string(dv.Status.Progress)).To(Equal("N/A")) + _, ok := pvc.Annotations[AnnCreatedForDataVolume] + Expect(ok).To(BeFalse()) }) It("Should adopt a PVC (with featuregate)", func() { @@ -253,6 +258,8 @@ var _ = Describe("All DataVolume Tests", func() { Expect(err).ToNot(HaveOccurred()) Expect(dv.Status.Phase).To(Equal(cdiv1.Succeeded)) Expect(string(dv.Status.Progress)).To(Equal("N/A")) + _, ok := pvc.Annotations[AnnCreatedForDataVolume] + Expect(ok).To(BeFalse()) }) It("Should adopt a unbound PVC", func() { @@ -275,6 +282,8 @@ var _ = Describe("All DataVolume Tests", func() { Expect(err).ToNot(HaveOccurred()) Expect(dv.Status.Phase).To(Equal(cdiv1.Succeeded)) Expect(string(dv.Status.Progress)).To(Equal("N/A")) + _, ok := pvc.Annotations[AnnCreatedForDataVolume] + Expect(ok).To(BeFalse()) }) var _ = Describe("Reconcile Datavolume status", func() { diff --git a/tests/datavolume_test.go b/tests/datavolume_test.go index befc325db0..d5c45b4dc8 100644 --- a/tests/datavolume_test.go +++ b/tests/datavolume_test.go @@ -249,6 +249,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", phase cdiv1.DataVolumePhase repeat int checkPermissions bool + addClaimAdoptionAnnotation bool readyCondition *cdiv1.DataVolumeCondition boundCondition *cdiv1.DataVolumeCondition boundConditionWithPopulators *cdiv1.DataVolumeCondition @@ -318,6 +319,9 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", // Have to call the function in here, to make sure the BeforeEach in the Framework has run. dataVolume := args.dvFunc(args.name, args.size, args.url()) controller.AddAnnotation(dataVolume, controller.AnnDeleteAfterCompletion, "false") + if args.addClaimAdoptionAnnotation { + controller.AddAnnotation(dataVolume, controller.AnnAllowClaimAdoption, "true") + } repeat := 1 if utils.IsHostpathProvisioner() && args.repeat > 0 { // Repeat rapidly to make sure we don't get regular and scratch space on different nodes. @@ -368,6 +372,7 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", err := utils.DeleteVerifierPod(f.K8sClient, f.Namespace.Name) Expect(err).ToNot(HaveOccurred()) } + Expect(pvc.Annotations[controller.AnnCreatedForDataVolume]).To(Equal(string(dataVolume.UID))) By("Cleaning up") err = utils.DeleteDataVolume(f.CdiClient, f.Namespace.Name, dataVolume.Name) Expect(err).ToNot(HaveOccurred()) @@ -480,6 +485,30 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", Message: "Import Complete", Reason: "Completed", }}), + Entry("succeed creating import dv with adoption annotation", dataVolumeTestArguments{ + name: "dv-http-import", + size: "1Gi", + url: tinyCoreIsoURL, + dvFunc: utils.NewDataVolumeWithHTTPImport, + eventReason: dvc.ImportSucceeded, + phase: cdiv1.Succeeded, + addClaimAdoptionAnnotation: true, + readyCondition: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeReady, + Status: v1.ConditionTrue, + }, + boundCondition: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeBound, + Status: v1.ConditionTrue, + Message: "PVC dv-http-import Bound", + Reason: "Bound", + }, + runningCondition: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeRunning, + Status: v1.ConditionFalse, + Message: "Import Complete", + Reason: "Completed", + }}), Entry("[rfe_id:1115][crit:high][posneg:negative][test_id:1358]fail creating import dv due to invalid DNS entry", dataVolumeTestArguments{ name: "dv-http-import-invalid-url", size: "1Gi", @@ -791,6 +820,36 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", Status: v1.ConditionTrue, Reason: "Pod is running", }}), + Entry("succeed creating upload dv with adoption annotation", dataVolumeTestArguments{ + name: "upload-dv", + size: "1Gi", + url: func() string { return "" }, + dvFunc: createUploadDataVolume, + eventReason: dvc.UploadReady, + phase: cdiv1.UploadReady, + addClaimAdoptionAnnotation: true, + readyCondition: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeReady, + Status: v1.ConditionFalse, + Reason: "TransferRunning", + }, + boundCondition: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeBound, + Status: v1.ConditionTrue, + Message: "PVC upload-dv Bound", + Reason: "Bound", + }, + boundConditionWithPopulators: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeBound, + Status: v1.ConditionFalse, + Message: "PVC upload-dv Pending", + Reason: "Pending", + }, + runningCondition: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeRunning, + Status: v1.ConditionTrue, + Reason: "Pod is running", + }}), Entry("[rfe_id:1947][crit:high][test_id:2145]succeed creating import dv with given tar archive url", dataVolumeTestArguments{ name: "dv-tar-archive", size: "1Gi", @@ -943,6 +1002,30 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests", Message: "Clone Complete", Reason: "Completed", }}), + Entry("succeed creating clone dv with adoption annotation", dataVolumeTestArguments{ + name: "dv-clone-test1", + size: "1Gi", + url: func() string { return fillCommand }, // its not URL, but command, but the parameter lines up. + dvFunc: createCloneDataVolume, + eventReason: dvc.CloneSucceeded, + phase: cdiv1.Succeeded, + addClaimAdoptionAnnotation: true, + readyCondition: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeReady, + Status: v1.ConditionTrue, + }, + boundCondition: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeBound, + Status: v1.ConditionTrue, + Message: "PVC dv-clone-test1 Bound", + Reason: "Bound", + }, + runningCondition: &cdiv1.DataVolumeCondition{ + Type: cdiv1.DataVolumeRunning, + Status: v1.ConditionFalse, + Message: "Clone Complete", + Reason: "Completed", + }}), Entry("[rfe_id:1115][crit:high][test_id:1478]succeed creating import dv with given valid registry url", dataVolumeTestArguments{ name: "dv-import-registry", size: "1Gi",