diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go index a3de4da9..9b4bd76a 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 26922f26..a8edc5e4 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 1c2f97aa..d274d03d 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 9dd439d8..b16d19e3 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)) + }) + } +}