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 <[email protected]>

Author: darkowlzz

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

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",

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
+}

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))
+		})
+	}
+}