Simplify shouldReconcile function arguments. (#1602)

* Simplify shouldReconcile function arguments.

By having the function itself grab things it needs and are easily
obtained.

Signed-off-by: Maya Rashish <mrashish@redhat.com>

* Adapt unit tests to simpler shouldReconcilePVC

Don't set any feature gates for WFFC being disabled.
When the second argument is true, pass the immediate binding annotation
to the PVC itself.

Signed-off-by: Maya Rashish <mrashish@redhat.com>
This commit is contained in:
Maya Rashish 2021-02-04 15:38:26 +02:00 committed by GitHub
parent b300b3838f
commit 9d1b94f47e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 36 additions and 18 deletions

View File

@ -181,11 +181,10 @@ func addImportControllerWatches(mgr manager.Manager, importController controller
return nil
}
func shouldReconcilePVC(pvc *corev1.PersistentVolumeClaim,
isImmediateBindingRequested bool,
featureGates featuregates.FeatureGates,
func (r *ImportReconciler) shouldReconcilePVC(pvc *corev1.PersistentVolumeClaim,
log logr.Logger) (bool, error) {
waitForFirstConsumerEnabled, err := isWaitForFirstConsumerEnabled(isImmediateBindingRequested, featureGates)
_, isImmediateBindingRequested := pvc.Annotations[AnnImmediateBinding]
waitForFirstConsumerEnabled, err := isWaitForFirstConsumerEnabled(isImmediateBindingRequested, r.featureGates)
if err != nil {
return false, err
@ -214,9 +213,8 @@ func (r *ImportReconciler) Reconcile(req reconcile.Request) (reconcile.Result, e
}
return reconcile.Result{}, err
}
_, isImmediateBindingRequested := pvc.Annotations[AnnImmediateBinding]
shouldReconcile, err := shouldReconcilePVC(pvc, isImmediateBindingRequested, r.featureGates, log)
shouldReconcile, err := r.shouldReconcilePVC(pvc, log)
if err != nil {
return reconcile.Result{}, err
}

View File

@ -76,37 +76,57 @@ var _ = Describe("Test PVC annotations status", func() {
})
It("Should be interesting if NOT complete, and endpoint and source is set", func() {
r := createImportReconciler()
testPvc := createPvc("testPvc1", "default", map[string]string{AnnPodPhase: string(corev1.PodPending), AnnEndpoint: testEndPoint, AnnSource: SourceHTTP}, nil)
Expect(shouldReconcilePVC(testPvc, false, &FakeFeatureGates{honorWaitForFirstConsumerEnabled: false}, importLog)).To(BeTrue())
Expect(r.shouldReconcilePVC(testPvc, importLog)).To(BeTrue())
})
It("Should NOT be interesting if complete, and endpoint and source is set", func() {
r := createImportReconciler()
testPvc := createPvc("testPvc1", "default", map[string]string{AnnPodPhase: string(corev1.PodSucceeded), AnnEndpoint: testEndPoint, AnnSource: SourceHTTP}, nil)
Expect(shouldReconcilePVC(testPvc, false, &FakeFeatureGates{honorWaitForFirstConsumerEnabled: false}, importLog)).To(BeFalse())
Expect(r.shouldReconcilePVC(testPvc, importLog)).To(BeFalse())
})
It("Should be interesting if NOT complete, and endpoint missing and source is set", func() {
r := createImportReconciler()
testPvc := createPvc("testPvc1", "default", map[string]string{AnnPodPhase: string(corev1.PodRunning), AnnSource: SourceHTTP}, nil)
Expect(shouldReconcilePVC(testPvc, false, &FakeFeatureGates{honorWaitForFirstConsumerEnabled: false}, importLog)).To(BeTrue())
Expect(r.shouldReconcilePVC(testPvc, importLog)).To(BeTrue())
})
It("Should be interesting if NOT complete, and endpoint set and source is missing", func() {
r := createImportReconciler()
testPvc := createPvc("testPvc1", "default", map[string]string{AnnPodPhase: string(corev1.PodPending), AnnEndpoint: testEndPoint}, nil)
Expect(shouldReconcilePVC(testPvc, false, &FakeFeatureGates{honorWaitForFirstConsumerEnabled: false}, importLog)).To(BeTrue())
Expect(r.shouldReconcilePVC(testPvc, importLog)).To(BeTrue())
})
It("Should NOT be interesting if NOT BOUND, and endpoint and source is set, and honorWaitForFirstConsumerEnabled", func() {
r := createImportReconciler()
r.featureGates = &FakeFeatureGates{honorWaitForFirstConsumerEnabled: true}
testPvc := createPendingPvc("testPvc1", "default", map[string]string{AnnPodPhase: string(corev1.PodPending), AnnEndpoint: testEndPoint, AnnSource: SourceHTTP}, nil)
Expect(shouldReconcilePVC(testPvc, false, &FakeFeatureGates{honorWaitForFirstConsumerEnabled: true}, importLog)).To(BeFalse())
Expect(r.shouldReconcilePVC(testPvc, importLog)).To(BeFalse())
})
It("Should be interesting if NOT BOUND, and endpoint and source is set, and honorWaitForFirstConsumerEnabled and isImmediateBindingRequested is requested", func() {
testPvc := createPendingPvc("testPvc1", "default", map[string]string{AnnPodPhase: string(corev1.PodPending), AnnEndpoint: testEndPoint, AnnSource: SourceHTTP}, nil)
Expect(shouldReconcilePVC(testPvc, true, &FakeFeatureGates{honorWaitForFirstConsumerEnabled: true}, importLog)).To(BeTrue())
r := createImportReconciler()
r.featureGates = &FakeFeatureGates{honorWaitForFirstConsumerEnabled: true}
testPvc := createPendingPvc("testPvc1", "default", map[string]string{
AnnPodPhase: string(corev1.PodPending),
AnnEndpoint: testEndPoint,
AnnSource: SourceHTTP,
AnnImmediateBinding: "true",
}, nil)
Expect(r.shouldReconcilePVC(testPvc, importLog)).To(BeTrue())
})
It("Should be interesting if NOT BOUND, and endpoint and source is set, and honorWaitForFirstConsumerEnabled is false and isImmediateBindingRequested is requested", func() {
testPvc := createPendingPvc("testPvc1", "default", map[string]string{AnnPodPhase: string(corev1.PodPending), AnnEndpoint: testEndPoint, AnnSource: SourceHTTP}, nil)
Expect(shouldReconcilePVC(testPvc, true, &FakeFeatureGates{honorWaitForFirstConsumerEnabled: false}, importLog)).To(BeTrue())
r := createImportReconciler()
r.featureGates = &FakeFeatureGates{honorWaitForFirstConsumerEnabled: false}
testPvc := createPendingPvc("testPvc1", "default", map[string]string{
AnnPodPhase: string(corev1.PodPending),
AnnEndpoint: testEndPoint,
AnnSource: SourceHTTP,
AnnImmediateBinding: "true",
}, nil)
Expect(r.shouldReconcilePVC(testPvc, importLog)).To(BeTrue())
})
})

View File

@ -117,13 +117,12 @@ func (r *UploadReconciler) Reconcile(req reconcile.Request) (reconcile.Result, e
_, isUpload := pvc.Annotations[AnnUploadRequest]
_, isCloneTarget := pvc.Annotations[AnnCloneRequest]
_, isImmediateBindingRequested := pvc.Annotations[AnnImmediateBinding]
if isUpload && isCloneTarget {
log.V(1).Info("PVC has both clone and upload annotations")
return reconcile.Result{}, errors.New("PVC has both clone and upload annotations")
}
shouldReconcile, err := r.shouldReconcile(isUpload, isCloneTarget, isImmediateBindingRequested, pvc, log)
shouldReconcile, err := r.shouldReconcile(isUpload, isCloneTarget, pvc, log)
if err != nil {
return reconcile.Result{}, err
}
@ -145,7 +144,8 @@ func (r *UploadReconciler) Reconcile(req reconcile.Request) (reconcile.Result, e
return r.reconcilePVC(log, pvc, isCloneTarget)
}
func (r *UploadReconciler) shouldReconcile(isUpload bool, isCloneTarget bool, isImmediateBindingRequested bool, pvc *v1.PersistentVolumeClaim, log logr.Logger) (bool, error) {
func (r *UploadReconciler) shouldReconcile(isUpload bool, isCloneTarget bool, pvc *v1.PersistentVolumeClaim, log logr.Logger) (bool, error) {
_, isImmediateBindingRequested := pvc.Annotations[AnnImmediateBinding]
waitForFirstConsumerEnabled, err := isWaitForFirstConsumerEnabled(isImmediateBindingRequested, r.featureGates)
if err != nil {
return false, err