Skip to content

Commit

Permalink
cdi.kubevirt.io/allowClaimAdoption annotation breaking "regular" oper…
Browse files Browse the repository at this point in the history 
…ations

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 <mhenriks@redhat.com>
  • Loading branch information
@mhenriks
mhenriks committed Mar 19, 2024
1 parent 10d6f5d commit 211cd49
Show file tree
Hide file tree
Showing 8 changed files with 130 additions and 10 deletions.
9 changes: 8 additions & 1 deletion pkg/controller/common/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions pkg/controller/datavolume/controller-base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions pkg/controller/datavolume/import-controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
}

Expand Down
13 changes: 12 additions & 1 deletion pkg/controller/datavolume/import-controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand All @@ -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() {
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand Down Expand Up @@ -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{
Expand Down
9 changes: 9 additions & 0 deletions pkg/controller/datavolume/pvc-clone-controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand All @@ -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",
Expand Down
8 changes: 4 additions & 4 deletions pkg/controller/datavolume/upload-controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
}

Expand Down
9 changes: 9 additions & 0 deletions pkg/controller/datavolume/upload-controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand Down
83 changes: 83 additions & 0 deletions tests/datavolume_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit 211cd49

Please sign in to comment.