From 48cad6838633344b70e50670c791ea3399097dd5 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Thu, 30 Nov 2023 12:59:32 +0100 Subject: [PATCH] controller: unready dep should not bump obs gen This ensures that any unfulfilled dependencies for which we requeue do not prematurely bump the observed generation by introducing typed errors. These typed errors ensure that the logic to bump the observed generation can continue to be the same, while ignoring them just in time before returning the final error. Signed-off-by: Hidde Beydals --- internal/controller/helmrelease_controller.go | 21 ++++++++-- .../controller/helmrelease_controller_test.go | 10 ++--- internal/errors/ignore.go | 25 ++++++++++++ internal/errors/ignore_test.go | 40 +++++++++++++++++++ 4 files changed, 87 insertions(+), 9 deletions(-) create mode 100644 internal/errors/ignore.go create mode 100644 internal/errors/ignore_test.go diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index d10911653..ad2ed11c2 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -59,6 +59,7 @@ import ( "github.com/fluxcd/helm-controller/internal/action" "github.com/fluxcd/helm-controller/internal/chartutil" "github.com/fluxcd/helm-controller/internal/digest" + interrors "github.com/fluxcd/helm-controller/internal/errors" "github.com/fluxcd/helm-controller/internal/features" "github.com/fluxcd/helm-controller/internal/kube" "github.com/fluxcd/helm-controller/internal/loader" @@ -97,6 +98,11 @@ type HelmReleaseReconcilerOptions struct { RateLimiter ratelimiter.RateLimiter } +var ( + errWaitForDependency = errors.New("must wait for dependency") + errWaitForChart = errors.New("must wait for chart") +) + func (r *HelmReleaseReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opts HelmReleaseReconcilerOptions) error { // Index the HelmRelease by the HelmChart references they point at if err := mgr.GetFieldIndexer().IndexField(ctx, &v2.HelmRelease{}, v2.SourceIndexKey, @@ -167,6 +173,13 @@ func (r *HelmReleaseReconciler) Reconcile(ctx context.Context, req ctrl.Request) patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{}) } + // We do not want to return these errors, but rather wait for the + // designated RequeueAfter to expire and try again. + // However, not returning an error will cause the patch helper to + // patch the observed generation, which we do not want. So we ignore + // these errors here after patching. + retErr = interrors.Ignore(retErr, errWaitForDependency, errWaitForChart) + if err := patchHelper.Patch(ctx, obj, patchOpts...); err != nil { if !obj.DeletionTimestamp.IsZero() { err = apierrutil.FilterOut(err, func(e error) bool { return apierrors.IsNotFound(e) }) @@ -230,7 +243,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe // Exponential backoff would cause execution to be prolonged too much, // instead we requeue on a fixed interval. - return ctrl.Result{RequeueAfter: r.requeueDependency}, nil + return ctrl.Result{RequeueAfter: r.requeueDependency}, errWaitForDependency } log.Info("all dependencies are ready") @@ -262,7 +275,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe conditions.MarkFalse(obj, meta.ReadyCondition, "HelmChartNotReady", msg) // Do not requeue immediately, when the artifact is created // the watcher should trigger a reconciliation. - return jitter.JitteredRequeueInterval(ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}), nil + return jitter.JitteredRequeueInterval(ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}), errWaitForChart } // Compose values based from the spec and references. @@ -276,10 +289,10 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe loadedChart, err := loader.SecureLoadChartFromURL(r.httpClient, hc.GetArtifact().URL, hc.GetArtifact().Digest) if err != nil { if errors.Is(err, loader.ErrFileNotFound) { - msg := fmt.Sprintf("Chart not ready: artifact not found. Retrying in %s", r.requeueDependency) + msg := fmt.Sprintf("Chart not ready: artifact not found. Retrying in %s", r.requeueDependency.String()) conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, msg) log.Info(msg) - return ctrl.Result{RequeueAfter: r.requeueDependency}, nil + return ctrl.Result{RequeueAfter: r.requeueDependency}, errWaitForDependency } conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, fmt.Sprintf("Could not load chart: %s", err.Error())) diff --git a/internal/controller/helmrelease_controller_test.go b/internal/controller/helmrelease_controller_test.go index e432a210b..e5c31ffa5 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/internal/controller/helmrelease_controller_test.go @@ -108,7 +108,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { } res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) - g.Expect(err).ToNot(HaveOccurred()) + g.Expect(err).To(Equal(errWaitForDependency)) g.Expect(res.RequeueAfter).To(Equal(r.requeueDependency)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ @@ -222,7 +222,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { } res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) - g.Expect(err).ToNot(HaveOccurred()) + g.Expect(err).To(Equal(errWaitForChart)) g.Expect(res.RequeueAfter).To(Equal(obj.Spec.Interval.Duration)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ @@ -274,7 +274,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { } res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) - g.Expect(err).ToNot(HaveOccurred()) + g.Expect(err).To(Equal(errWaitForChart)) g.Expect(res.RequeueAfter).To(Equal(obj.Spec.Interval.Duration)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ @@ -326,7 +326,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { } res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) - g.Expect(err).ToNot(HaveOccurred()) + g.Expect(err).To(Equal(errWaitForChart)) g.Expect(res.RequeueAfter).To(Equal(obj.Spec.Interval.Duration)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ @@ -438,7 +438,7 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { } res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj) - g.Expect(err).ToNot(HaveOccurred()) + g.Expect(err).To(Equal(errWaitForDependency)) g.Expect(res.RequeueAfter).To(Equal(r.requeueDependency)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ diff --git a/internal/errors/ignore.go b/internal/errors/ignore.go new file mode 100644 index 000000000..8cd247c92 --- /dev/null +++ b/internal/errors/ignore.go @@ -0,0 +1,25 @@ +/* +Copyright 2023 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package errors + +// Ignore returns nil if err is equal to any of the errs. +func Ignore(err error, errs ...error) error { + if IsOneOf(err, errs...) { + return nil + } + return err +} diff --git a/internal/errors/ignore_test.go b/internal/errors/ignore_test.go new file mode 100644 index 000000000..bcf204ea3 --- /dev/null +++ b/internal/errors/ignore_test.go @@ -0,0 +1,40 @@ +/* +Copyright 2023 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package errors + +import ( + "errors" + "testing" +) + +func TestIgnore(t *testing.T) { + err1 := errors.New("error1") + err2 := errors.New("error2") + + if err := Ignore(err1, err1, err2); err != nil { + t.Errorf("Expected Ignore to return nil when the error is in the list, but got %v", err) + } + + err3 := errors.New("error3") + if err := Ignore(err3, err1, err2); !errors.Is(err, err3) { + t.Errorf("Expected Ignore to return the error when it is not in the list, but got %v", err) + } + + if err := Ignore(err1); !errors.Is(err, err1) { + t.Errorf("Expected Ignore to return the error with an empty list of errors, but got %v", err) + } +}