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 target conditions are not ready/True, the reconciler
should retry unless it's in stalled condition.

Signed-off-by: Sunny <darkowlzz@protonmail.com>

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,6 +18,7 @@ package summarize
 
 import (
 	"context"
+	"errors"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
@@ -204,6 +205,19 @@ 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 the TargetCondition=True. Else, use the TargetCondition
+	// failure message as the runtime error message. This ensures that the
+	// reconciliation would be retried as the object isn't ready.
+	if isNonStalledSuccess(obj, opts.ResultBuilder, result, recErr) {
+		for _, c := range opts.Conditions {
+			if !conditions.IsTrue(obj, c.Target) {
+				err := errors.New(conditions.GetMessage(obj, c.Target))
+				recErr = kerrors.NewAggregate([]error{recErr, err})
+			}
+		}
+	}
+
 	// 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. If there's a
+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"
 
@@ -216,7 +218,22 @@ func TestSummarizeAndPatch(t *testing.T) {
 			},
 		},
 		{
-			name:       "Success, multiple conditions summary",
+			name:       "Success, multiple conditions True 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:       "Fail, multiple conditions mixed summary",
 			generation: 3,
 			beforeFunc: func(obj conditions.Setter) {
 				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "test-msg")
@@ -231,6 +248,35 @@ func TestSummarizeAndPatch(t *testing.T) {
 				*conditions.TrueCondition("BBB", "YYY", "yyy"),
 				*conditions.TrueCondition("AAA", "ZZZ", "zzz"),
 			},
+			wantErr: true,
+		},
+		{
+			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,
+		},
+		{
+			name:       "Success, 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,
 		},
 	}
 
@@ -291,6 +337,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 +383,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 +413,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 +434,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 +443,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))
+		})
+	}
+}