Merge pull request #703 from fluxcd/target-condition-in-result

summarize: Consider obj status condition in result

Author: stefanprodan

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