From 2240106c8e01da35cb65e37c592cea29eb8d15d7 Mon Sep 17 00:00:00 2001 From: Sunny Date: Fri, 29 Apr 2022 22:06:51 +0530 Subject: [PATCH] summarize: Consider obj status condition in result SummarizeAndPatch() should also consider the object's status conditions when computing and returning the runtime results to avoid any inconsistency in the runtime result and status condition of the object. When an object's Ready condition is False, the reconciler should retry unless it's in stalled condition. Signed-off-by: Sunny --- internal/reconcile/reconcile.go | 11 ++ internal/reconcile/reconcile_test.go | 53 ++++++-- internal/reconcile/summarize/summary.go | 27 ++++ internal/reconcile/summarize/summary_test.go | 122 +++++++++++++++---- 4 files changed, 182 insertions(+), 31 deletions(-) diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go index a3de4da95..9b4bd76af 100644 --- a/internal/reconcile/reconcile.go +++ b/internal/reconcile/reconcile.go @@ -52,7 +52,12 @@ const ( // can be implemented to build custom results based on the context of the // reconciler. type RuntimeResultBuilder interface { + // BuildRuntimeResult analyzes the result and error to return a runtime + // result. BuildRuntimeResult(rr Result, err error) ctrl.Result + // IsSuccess returns if a given runtime result is success for a + // RuntimeResultBuilder. + IsSuccess(ctrl.Result) bool } // AlwaysRequeueResultBuilder implements a RuntimeResultBuilder for always @@ -82,6 +87,12 @@ func (r AlwaysRequeueResultBuilder) BuildRuntimeResult(rr Result, err error) ctr } } +// IsSuccess returns true if the given Result has the same RequeueAfter value +// as of the AlwaysRequeueResultBuilder. +func (r AlwaysRequeueResultBuilder) IsSuccess(result ctrl.Result) bool { + return result.RequeueAfter == r.RequeueAfter +} + // ComputeReconcileResult analyzes the reconcile results (result + error), // updates the status conditions of the object with any corrections and returns // object patch configuration, runtime result and runtime error. The caller is diff --git a/internal/reconcile/reconcile_test.go b/internal/reconcile/reconcile_test.go index 26922f26d..a8edc5e4b 100644 --- a/internal/reconcile/reconcile_test.go +++ b/internal/reconcile/reconcile_test.go @@ -118,16 +118,6 @@ func TestComputeReconcileResult(t *testing.T) { t.Expect(patchOpts.IncludeStatusObservedGeneration).To(BeFalse()) }, }, - { - name: "requeue result", - result: ResultRequeue, - recErr: nil, - wantResult: ctrl.Result{Requeue: true}, - wantErr: false, - afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) { - t.Expect(patchOpts.IncludeStatusObservedGeneration).To(BeFalse()) - }, - }, { name: "stalling error", result: ResultEmpty, @@ -203,6 +193,49 @@ func TestComputeReconcileResult(t *testing.T) { } } +func TestAlwaysRequeueResultBuilder_IsSuccess(t *testing.T) { + interval := 5 * time.Second + + tests := []struct { + name string + resultBuilder AlwaysRequeueResultBuilder + runtimeResult ctrl.Result + result bool + }{ + { + name: "success result", + resultBuilder: AlwaysRequeueResultBuilder{RequeueAfter: interval}, + runtimeResult: ctrl.Result{RequeueAfter: interval}, + result: true, + }, + { + name: "requeue result", + resultBuilder: AlwaysRequeueResultBuilder{RequeueAfter: interval}, + runtimeResult: ctrl.Result{Requeue: true}, + result: false, + }, + { + name: "zero result", + resultBuilder: AlwaysRequeueResultBuilder{RequeueAfter: interval}, + runtimeResult: ctrl.Result{}, + result: false, + }, + { + name: "different requeue after", + resultBuilder: AlwaysRequeueResultBuilder{RequeueAfter: interval}, + runtimeResult: ctrl.Result{RequeueAfter: time.Second}, + result: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + g.Expect(tt.resultBuilder.IsSuccess(tt.runtimeResult)).To(Equal(tt.result)) + }) + } +} + func TestFailureRecovery(t *testing.T) { failCondns := []string{ "FooFailed", diff --git a/internal/reconcile/summarize/summary.go b/internal/reconcile/summarize/summary.go index 1c2f97aae..d274d03d5 100644 --- a/internal/reconcile/summarize/summary.go +++ b/internal/reconcile/summarize/summary.go @@ -18,12 +18,14 @@ package summarize import ( "context" + "errors" apierrors "k8s.io/apimachinery/pkg/api/errors" kerrors "k8s.io/apimachinery/pkg/util/errors" kuberecorder "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" + "github.com/fluxcd/pkg/apis/meta" "github.com/fluxcd/pkg/runtime/conditions" "github.com/fluxcd/pkg/runtime/patch" @@ -204,6 +206,18 @@ func (h *Helper) SummarizeAndPatch(ctx context.Context, obj conditions.Setter, o ) } + // If object is not stalled, result is success and runtime error is nil, + // ensure that Ready=True. Else, use the Ready failure message as the + // runtime error message. This ensures that the reconciliation would be + // retried as the object isn't ready. + // NOTE: This is applicable to Ready condition only because it is a special + // condition in kstatus that reflects the overall state of an object. + if isNonStalledSuccess(obj, opts.ResultBuilder, result, recErr) { + if !conditions.IsReady(obj) { + recErr = errors.New(conditions.GetMessage(obj, meta.ReadyCondition)) + } + } + // Finally, patch the resource. if err := h.patchHelper.Patch(ctx, obj, patchOpts...); err != nil { // Ignore patch error "not found" when the object is being deleted. @@ -215,3 +229,16 @@ func (h *Helper) SummarizeAndPatch(ctx context.Context, obj conditions.Setter, o return result, recErr } + +// isNonStalledSuccess checks if the reconciliation was successful and has not +// resulted in stalled situation. +func isNonStalledSuccess(obj conditions.Setter, rb reconcile.RuntimeResultBuilder, result ctrl.Result, recErr error) bool { + if !conditions.IsStalled(obj) && recErr == nil { + // Without result builder, it can't be determined if the result is + // success. + if rb != nil { + return rb.IsSuccess(result) + } + } + return false +} diff --git a/internal/reconcile/summarize/summary_test.go b/internal/reconcile/summarize/summary_test.go index 9dd439d85..b16d19e37 100644 --- a/internal/reconcile/summarize/summary_test.go +++ b/internal/reconcile/summarize/summary_test.go @@ -18,6 +18,7 @@ package summarize import ( "context" + "errors" "fmt" "testing" "time" @@ -27,6 +28,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/tools/record" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -91,18 +93,19 @@ func TestSummarizeAndPatch(t *testing.T) { afterFunc func(t *WithT, obj client.Object) assertConditions []metav1.Condition }{ - // Success/Fail indicates if a reconciliation succeeded or failed. On - // a successful reconciliation, the object generation is expected to - // match the observed generation in the object status. + // Success/Fail indicates if a reconciliation succeeded or failed. + // The object generation is expected to match the observed generation in + // the object status if Ready=True or Stalled=True at the end. // All the cases have some Ready condition set, even if a test case is // unrelated to the conditions, because it's neseccary for a valid // status. { - name: "Success, no extra conditions", + name: "Success, Ready=True", generation: 4, beforeFunc: func(obj conditions.Setter) { conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "test-msg") }, + result: reconcile.ResultSuccess, conditions: []Conditions{testReadyConditions}, assertConditions: []metav1.Condition{ *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "test-msg"), @@ -111,20 +114,6 @@ func TestSummarizeAndPatch(t *testing.T) { t.Expect(obj).To(HaveStatusObservedGeneration(4)) }, }, - { - name: "Success, Ready=True", - generation: 5, - beforeFunc: func(obj conditions.Setter) { - conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "created") - }, - conditions: []Conditions{testReadyConditions}, - assertConditions: []metav1.Condition{ - *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "created"), - }, - afterFunc: func(t *WithT, obj client.Object) { - t.Expect(obj).To(HaveStatusObservedGeneration(5)) - }, - }, { name: "Success, removes reconciling for successful result", generation: 2, @@ -216,7 +205,22 @@ func TestSummarizeAndPatch(t *testing.T) { }, }, { - name: "Success, multiple conditions summary", + name: "Success, multiple target conditions summary", + generation: 3, + beforeFunc: func(obj conditions.Setter) { + conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "test-msg") + conditions.MarkTrue(obj, "AAA", "ZZZ", "zzz") // Positive polarity True. + }, + conditions: []Conditions{testReadyConditions, testFooConditions}, + result: reconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "test-msg"), + *conditions.TrueCondition("Foo", "ZZZ", "zzz"), // True summary. + *conditions.TrueCondition("AAA", "ZZZ", "zzz"), + }, + }, + { + name: "Success, multiple target conditions, False non-Ready summary don't affect result", generation: 3, beforeFunc: func(obj conditions.Setter) { conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "test-msg") @@ -232,6 +236,20 @@ func TestSummarizeAndPatch(t *testing.T) { *conditions.TrueCondition("AAA", "ZZZ", "zzz"), }, }, + { + name: "Fail, success result but Ready=False", + generation: 3, + beforeFunc: func(obj conditions.Setter) { + conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision") + }, + conditions: []Conditions{testReadyConditions}, + result: reconcile.ResultSuccess, + assertConditions: []metav1.Condition{ + *conditions.FalseCondition(meta.ReadyCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), + }, + wantErr: true, + }, } for _, tt := range tests { @@ -291,6 +309,8 @@ func TestSummarizeAndPatch(t *testing.T) { // This tests the scenario where SummarizeAndPatch is used in the middle of // reconciliation. func TestSummarizeAndPatch_Intermediate(t *testing.T) { + interval := 5 * time.Second + var testStageAConditions = Conditions{ Target: "StageA", Owned: []string{"StageA", "A1", "A2", "A3"}, @@ -335,7 +355,7 @@ func TestSummarizeAndPatch_Intermediate(t *testing.T) { }, }, { - name: "multiple Conditions", + name: "multiple Conditions, mixed results", conditions: []Conditions{testStageAConditions, testStageBConditions}, beforeFunc: func(obj conditions.Setter) { conditions.MarkTrue(obj, "A3", "ZZZ", "zzz") // Negative polarity True. @@ -365,7 +385,7 @@ func TestSummarizeAndPatch_Intermediate(t *testing.T) { GenerateName: "test-", }, Spec: sourcev1.GitRepositorySpec{ - Interval: metav1.Duration{Duration: 5 * time.Second}, + Interval: metav1.Duration{Duration: interval}, }, Status: sourcev1.GitRepositoryStatus{ Conditions: []metav1.Condition{ @@ -386,6 +406,7 @@ func TestSummarizeAndPatch_Intermediate(t *testing.T) { summaryHelper := NewHelper(record.NewFakeRecorder(32), patchHelper) summaryOpts := []Option{ WithConditions(tt.conditions...), + WithResultBuilder(reconcile.AlwaysRequeueResultBuilder{RequeueAfter: interval}), } _, err = summaryHelper.SummarizeAndPatch(ctx, obj, summaryOpts...) g.Expect(err).ToNot(HaveOccurred()) @@ -394,3 +415,62 @@ func TestSummarizeAndPatch_Intermediate(t *testing.T) { }) } } + +func TestIsNonStalledSuccess(t *testing.T) { + interval := 5 * time.Second + + tests := []struct { + name string + beforeFunc func(obj conditions.Setter) + rb reconcile.RuntimeResultBuilder + recResult ctrl.Result + recErr error + wantResult bool + }{ + { + name: "non stalled success", + rb: reconcile.AlwaysRequeueResultBuilder{RequeueAfter: interval}, + recResult: ctrl.Result{RequeueAfter: interval}, + wantResult: true, + }, + { + name: "stalled success", + beforeFunc: func(obj conditions.Setter) { + conditions.MarkStalled(obj, "FooReason", "test-msg") + }, + rb: reconcile.AlwaysRequeueResultBuilder{RequeueAfter: interval}, + recResult: ctrl.Result{RequeueAfter: interval}, + wantResult: false, + }, + { + name: "error result", + rb: reconcile.AlwaysRequeueResultBuilder{RequeueAfter: interval}, + recResult: ctrl.Result{RequeueAfter: interval}, + recErr: errors.New("some-error"), + wantResult: false, + }, + { + name: "non success result", + rb: reconcile.AlwaysRequeueResultBuilder{RequeueAfter: interval}, + recResult: ctrl.Result{RequeueAfter: 2 * time.Second}, + wantResult: false, + }, + { + name: "no result builder", + recResult: ctrl.Result{RequeueAfter: interval}, + wantResult: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + obj := &sourcev1.GitRepository{} + if tt.beforeFunc != nil { + tt.beforeFunc(obj) + } + g.Expect(isNonStalledSuccess(obj, tt.rb, tt.recResult, tt.recErr)).To(Equal(tt.wantResult)) + }) + } +}