Skip to content

Commit

Permalink
Allow ImmediateBind annotation when using populators
Browse files Browse the repository at this point in the history
In case of using PVC with populators if the PVC has this
annotation we prevent from waiting for it to be schedueled
and we proceed with the process.
When using datavolumes with populators in case the dv has the annotation
it will be passed to the PVC. we prevent from being in pendingPopulation
in case the created pvc has the annotaion.
Plus when having honorWaitForFirstConsumer feature gate disabled we will
put on the target PVC the immediateBind annotation.
Now we allow to use populators when having the annotation the the
feature gate disabled.

Signed-off-by: Shelly Kagan <skagan@redhat.com>
  • Loading branch information
ShellyKa13 committed Jun 22, 2023
1 parent 8ced302 commit 88f1bda
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 33 deletions.
12 changes: 12 additions & 0 deletions pkg/controller/common/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -1270,6 +1270,18 @@ func IsWaitForFirstConsumerEnabled(obj metav1.Object, gates featuregates.Feature
return pvcHonorWaitForFirstConsumer && globalHonorWaitForFirstConsumer, nil
}

// AddImmediateBindingAnnotationIfWFFCDisabled adds the immediateBinding annotation if wffc feature gate is disabled
func AddImmediateBindingAnnotationIfWFFCDisabled(obj metav1.Object, gates featuregates.FeatureGates) error {
globalHonorWaitForFirstConsumer, err := gates.HonorWaitForFirstConsumerEnabled()
if err != nil {
return err
}
if !globalHonorWaitForFirstConsumer {
AddAnnotation(obj, AnnImmediateBinding, "")
}
return nil
}

// GetRequiredSpace calculates space required taking file system overhead into account
func GetRequiredSpace(filesystemOverhead float64, requestedSpace int64) int64 {
// the `image` has to be aligned correctly, so the space requested has to be aligned to
Expand Down
25 changes: 2 additions & 23 deletions pkg/controller/datavolume/controller-base.go
Original file line number Diff line number Diff line change
Expand Up @@ -1150,8 +1150,9 @@ func (r *ReconcilerBase) shouldBeMarkedPendingPopulation(pvc *corev1.PersistentV
return false, err
}
nodeName := pvc.Annotations[cc.AnnSelectedNode]
_, immediateBindingRequested := pvc.Annotations[cc.AnnImmediateBinding]

return wffc && nodeName == "", nil
return wffc && nodeName == "" && !immediateBindingRequested, nil
}

// handlePvcCreation works as a wrapper for non-clone PVC creation and error handling
Expand Down Expand Up @@ -1200,11 +1201,6 @@ func (r *ReconcilerBase) shouldUseCDIPopulator(syncState *dvSyncState) (bool, er
log.Info("Not using CDI populators, currently we don't support populators with retainAfterCompletion annotation")
return false, nil
}
// currently populators don't support immediate bind annotation so don't use populators in that case
if forceBind := dv.Annotations[cc.AnnImmediateBinding]; forceBind == "true" {
log.Info("Not using CDI populators, currently we don't support populators with bind immediate annotation")
return false, nil
}
// currently we don't support populator with import source of VDDK or Imageio
// or clone either from PVC nor snapshot
if dv.Spec.Source.Imageio != nil || dv.Spec.Source.VDDK != nil ||
Expand All @@ -1213,23 +1209,6 @@ func (r *ReconcilerBase) shouldUseCDIPopulator(syncState *dvSyncState) (bool, er
return false, nil
}

wffc, err := r.storageClassWaitForFirstConsumer(syncState.pvcSpec.StorageClassName)
if err != nil {
return false, err
}

honorWaitForFirstConsumerEnabled, err := r.featureGates.HonorWaitForFirstConsumerEnabled()
if err != nil {
return false, err
}
// currently since we don't support force bind in populators in case the storage class
// is wffc but the honorWaitForFirstConsumer feature gate is disabled we can't
// do immediate bind anyways, instead do regular flow
if wffc && !honorWaitForFirstConsumerEnabled {
log.Info("Not using CDI populators, currently we don't WFFC storage with disabled honorWaitForFirstConsumer feature gate")
return false, nil
}

usePopulator, err := storageClassCSIDriverExists(r.client, r.log, syncState.pvcSpec.StorageClassName)
if err != nil {
return false, err
Expand Down
3 changes: 3 additions & 0 deletions pkg/controller/datavolume/import-controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@ func (r *ImportReconciler) updatePVCForPopulation(dataVolume *cdiv1.DataVolume,
dataVolume.Spec.Source.Blank == nil {
return errors.Errorf("no source set for import datavolume")
}
if err := cc.AddImmediateBindingAnnotationIfWFFCDisabled(pvc, r.featureGates); err != nil {
return err
}
apiGroup := cc.AnnAPIGroup
pvc.Spec.DataSourceRef = &corev1.TypedObjectReference{
APIGroup: &apiGroup,
Expand Down
12 changes: 7 additions & 5 deletions pkg/controller/datavolume/import-controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ var _ = Describe("All DataVolume Tests", func() {
},
}
dv := NewImportDataVolume("test-dv")
reconciler = createImportReconciler(dv, sc, csiDriver)
reconciler = createImportReconcilerWFFCDisabled(dv, sc, csiDriver)
_, 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 @@ -209,6 +209,8 @@ var _ = Describe("All DataVolume Tests", func() {
Expect(pvc.Spec.DataSourceRef.Name).To(Equal(importSourceName))
Expect(pvc.Spec.DataSourceRef.Kind).To(Equal(cdiv1.VolumeImportSourceRef))
Expect(pvc.GetAnnotations()[AnnUsePopulator]).To(Equal("true"))
_, annExists := pvc.Annotations[AnnImmediateBinding]
Expect(annExists).To(BeTrue())
})

It("Should report import population progress when use populators", func() {
Expand Down Expand Up @@ -1565,7 +1567,6 @@ var _ = Describe("All DataVolume Tests", func() {
Entry("AnnUsePopulator=true return true", AnnUsePopulator, "true", true),
Entry("AnnUsePopulator=false return false", AnnUsePopulator, "false", false),
Entry("AnnPodRetainAfterCompletion return false", AnnPodRetainAfterCompletion, "true", false),
Entry("AnnImmediateBinding return false", AnnImmediateBinding, "true", false),
)

DescribeTable("Should return false if source is", func(source *cdiv1.DataVolumeSource) {
Expand Down Expand Up @@ -1593,19 +1594,20 @@ var _ = Describe("All DataVolume Tests", func() {
}),
)

It("Should return false if storage class has wffc bindingMode and honorWaitForFirstConsumer feature gate is disabled", func() {
It("Should return true if storage class has wffc bindingMode and honorWaitForFirstConsumer feature gate is disabled", func() {
sc := createStorageClassWithBindingMode(scName,
map[string]string{
AnnDefaultStorageClass: "true",
},
storagev1.VolumeBindingWaitForFirstConsumer)
sc.Provisioner = "csi-plugin"
httpSource := &cdiv1.DataVolumeSource{
HTTP: &cdiv1.DataVolumeSourceHTTP{},
}
storageSpec := &cdiv1.StorageSpec{}
dv := createDataVolumeWithStorageAPI("test-dv", metav1.NamespaceDefault, httpSource, storageSpec)

reconciler = createImportReconcilerWFFCDisabled(sc)
reconciler = createImportReconcilerWFFCDisabled(sc, csiDriver)
syncState := dvSyncState{
dvMutated: dv,
pvcSpec: &corev1.PersistentVolumeClaimSpec{
Expand All @@ -1614,7 +1616,7 @@ var _ = Describe("All DataVolume Tests", func() {
}
usePopulator, err := reconciler.shouldUseCDIPopulator(&syncState)
Expect(err).ToNot(HaveOccurred())
Expect(usePopulator).To(BeFalse())
Expect(usePopulator).To(BeTrue())
})

It("Should return false if storage class doesnt exist", func() {
Expand Down
3 changes: 3 additions & 0 deletions pkg/controller/datavolume/upload-controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ func (r *UploadReconciler) updatePVCForPopulation(dataVolume *cdiv1.DataVolume,
if dataVolume.Spec.Source.Upload == nil {
return errors.Errorf("no source set for upload datavolume")
}
if err := cc.AddImmediateBindingAnnotationIfWFFCDisabled(pvc, r.featureGates); err != nil {
return err
}
apiGroup := cc.AnnAPIGroup
pvc.Spec.DataSourceRef = &corev1.TypedObjectReference{
APIGroup: &apiGroup,
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/populators/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ func claimReadyForPopulation(ctx context.Context, c client.Client, pvc *corev1.P

if storageClass.VolumeBindingMode != nil && *storageClass.VolumeBindingMode == storagev1.VolumeBindingWaitForFirstConsumer {
nodeName = pvc.Annotations[cc.AnnSelectedNode]
if nodeName == "" {
_, isImmediateBindingRequested := pvc.Annotations[cc.AnnImmediateBinding]
if nodeName == "" && !isImmediateBindingRequested {
// Wait for the PVC to get a node name before continuing
return false, nodeName, nil
}
Expand Down
18 changes: 14 additions & 4 deletions tests/datavolume_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2657,10 +2657,15 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
Expect(err).ToNot(HaveOccurred())

// verify PVC was created
By("verifying pvc was created and is Bound")
By("verifying pvc was created")
pvc, err := utils.WaitForPVC(f.K8sClient, dataVolume.Namespace, dataVolume.Name)
Expect(err).ToNot(HaveOccurred())
err = utils.WaitForPersistentVolumeClaimPhase(f.K8sClient, pvc.Namespace, v1.ClaimBound, pvc.Name)
expectedPVCPhase := v1.ClaimBound
if phase != cdiv1.Succeeded && pvc.Spec.DataSourceRef != nil {
expectedPVCPhase = v1.ClaimPending
}
By(fmt.Sprintf("waiting for pvc to match phase %s", string(expectedPVCPhase)))
err = utils.WaitForPersistentVolumeClaimPhase(f.K8sClient, pvc.Namespace, expectedPVCPhase, pvc.Name)
Expect(err).ToNot(HaveOccurred())

By(fmt.Sprintf("waiting for datavolume to match phase %s", string(phase)))
Expand Down Expand Up @@ -2755,10 +2760,15 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
Expect(err).ToNot(HaveOccurred())

// verify PVC was created
By("verifying pvc was created and is Bound")
By("verifying pvc was created")
pvc, err := utils.WaitForPVC(f.K8sClient, dataVolume.Namespace, dataVolume.Name)
Expect(err).ToNot(HaveOccurred())
err = utils.WaitForPersistentVolumeClaimPhase(f.K8sClient, pvc.Namespace, v1.ClaimBound, pvc.Name)
expectedPVCPhase := v1.ClaimBound
if phase != cdiv1.Succeeded && pvc.Spec.DataSourceRef != nil {
expectedPVCPhase = v1.ClaimPending
}
By(fmt.Sprintf("waiting for pvc to match phase %s", string(expectedPVCPhase)))
err = utils.WaitForPersistentVolumeClaimPhase(f.K8sClient, pvc.Namespace, expectedPVCPhase, pvc.Name)
Expect(err).ToNot(HaveOccurred())

By(fmt.Sprintf("waiting for datavolume to match phase %s", string(phase)))
Expand Down

0 comments on commit 88f1bda

Please sign in to comment.