summarize: consider bipolarity in status condition

This introduces the consideration of bipolarity conditions in the status
condition summary for Ready condition. The summarize.HelperOptions can
now be configured with a list of bipolarity conditions which are used in
SummarizeAndPatch() to set the Ready condition to failing bipolarity
condition with the highest priority.

Bipolarity condition is not a typical status property. It is a mix of
positive and negative polarities. It's "normal-true" and
"abnormal-false". Failing bipolarity conditions are prioritized over
other conditions to show the actual reason of failure on the Ready
status.

Signed-off-by: Sunny <[email protected]>

Author: darkowlzz

internal/reconcile/summarize/summary.go

@@ -90,6 +90,9 @@ type HelperOptions struct {
 	// PatchFieldOwner defines the field owner configuration for the Kubernetes
 	// patch operation.
 	PatchFieldOwner string
+	// BiPolarityConditionTypes is a list of bipolar conditions in the order
+	// of priority.
+	BiPolarityConditionTypes []string
 }
 
 // Option is configuration that modifies SummarizeAndPatch.
@@ -149,6 +152,14 @@ func WithPatchFieldOwner(fieldOwner string) Option {
 	}
 }
 
+// WithBiPolarityConditionTypes sets the BiPolarityConditionTypes used to
+// calculate the value of Ready condition in SummarizeAndPatch.
+func WithBiPolarityConditionTypes(types ...string) Option {
+	return func(s *HelperOptions) {
+		s.BiPolarityConditionTypes = types
+	}
+}
+
 // SummarizeAndPatch summarizes and patches the result to the target object.
 // When used at the very end of a reconciliation, the result builder must be
 // specified using the Option WithResultBuilder(). The returned result and error
@@ -206,6 +217,26 @@ func (h *Helper) SummarizeAndPatch(ctx context.Context, obj conditions.Setter, o
 		)
 	}
 
+	// Check any BiPolarity conditions in the status that are False. Failing
+	// BiPolarity condition should be set as the Ready condition value to
+	// reflect the actual cause of the reconciliation failure.
+	// NOTE: This is applicable to Ready condition only because it is a special
+	// condition in kstatus that reflects the overall state of an object.
+	// IMPLEMENTATION NOTE: An implementation of this within the
+	// conditions.merge() exists in fluxcd/pkg repo branch `bipolarity`
+	// (https://github.com/fluxcd/pkg/commit/756b9e6d253a4fae93c05419b7019d0169454858).
+	// If that gets added to conditions.merge, the following can be removed.
+	var failedBiPolarity []string
+	for _, c := range opts.BiPolarityConditionTypes {
+		if conditions.IsFalse(obj, c) {
+			failedBiPolarity = append(failedBiPolarity, c)
+		}
+	}
+	if len(failedBiPolarity) > 0 {
+		topFailedBiPolarity := conditions.Get(obj, failedBiPolarity[0])
+		conditions.MarkFalse(obj, meta.ReadyCondition, topFailedBiPolarity.Reason, topFailedBiPolarity.Message)
+	}
+
 	// 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

internal/reconcile/summarize/summary_test.go

@@ -44,18 +44,26 @@ import (
 // This tests the scenario where SummarizeAndPatch is used at the very end of a
 // reconciliation.
 func TestSummarizeAndPatch(t *testing.T) {
+	testBipolarCondition1 := "FooChecked1"
+	testBipolarCondition2 := "FooChecked2"
 	var testReadyConditions = Conditions{
 		Target: meta.ReadyCondition,
 		Owned: []string{
 			sourcev1.FetchFailedCondition,
 			sourcev1.ArtifactOutdatedCondition,
+			sourcev1.SourceVerifiedCondition,
+			testBipolarCondition1,
+			testBipolarCondition2,
 			meta.ReadyCondition,
 			meta.ReconcilingCondition,
 			meta.StalledCondition,
 		},
 		Summarize: []string{
 			sourcev1.FetchFailedCondition,
 			sourcev1.ArtifactOutdatedCondition,
+			sourcev1.SourceVerifiedCondition,
+			testBipolarCondition1,
+			testBipolarCondition2,
 			meta.StalledCondition,
 			meta.ReconcilingCondition,
 		},
@@ -66,6 +74,7 @@ func TestSummarizeAndPatch(t *testing.T) {
 			meta.ReconcilingCondition,
 		},
 	}
+	var testBipolarConditions = []string{sourcev1.SourceVerifiedCondition, testBipolarCondition1, testBipolarCondition2}
 	var testFooConditions = Conditions{
 		Target: "Foo",
 		Owned: []string{
@@ -83,15 +92,16 @@ func TestSummarizeAndPatch(t *testing.T) {
 	}
 
 	tests := []struct {
-		name             string
-		generation       int64
-		beforeFunc       func(obj conditions.Setter)
-		result           reconcile.Result
-		reconcileErr     error
-		conditions       []Conditions
-		wantErr          bool
-		afterFunc        func(t *WithT, obj client.Object)
-		assertConditions []metav1.Condition
+		name              string
+		generation        int64
+		beforeFunc        func(obj conditions.Setter)
+		result            reconcile.Result
+		reconcileErr      error
+		conditions        []Conditions
+		bipolarConditions []string
+		wantErr           bool
+		afterFunc         func(t *WithT, obj client.Object)
+		assertConditions  []metav1.Condition
 	}{
 		// Success/Fail indicates if a reconciliation succeeded or failed.
 		// The object generation is expected to match the observed generation in
@@ -250,6 +260,64 @@ func TestSummarizeAndPatch(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name:       "Fail, reconciling with bipolar condition False, Ready gets bipolar failure value",
+			generation: 2,
+			beforeFunc: func(obj conditions.Setter) {
+				conditions.MarkReconciling(obj, "NewRevision", "new index revision")
+				conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, "VerifyFailed", "verify failed")
+			},
+			result:            reconcile.ResultEmpty,
+			reconcileErr:      errors.New("failed to verify source"),
+			conditions:        []Conditions{testReadyConditions},
+			bipolarConditions: testBipolarConditions,
+			wantErr:           true,
+			assertConditions: []metav1.Condition{
+				*conditions.FalseCondition(meta.ReadyCondition, "VerifyFailed", "verify failed"),
+				*conditions.FalseCondition(sourcev1.SourceVerifiedCondition, "VerifyFailed", "verify failed"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"),
+			},
+		},
+		{
+			name:       "Fail, bipolar condition True, negative polarity True, Ready gets negative polarity value",
+			generation: 2,
+			beforeFunc: func(obj conditions.Setter) {
+				conditions.MarkReconciling(obj, "NewGeneration", "new obj gen")
+				conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new digest")
+				conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, "Success", "verified")
+			},
+			result:            reconcile.ResultEmpty,
+			reconcileErr:      errors.New("failed to create dir"),
+			conditions:        []Conditions{testReadyConditions},
+			bipolarConditions: testBipolarConditions,
+			wantErr:           true,
+			assertConditions: []metav1.Condition{
+				*conditions.FalseCondition(meta.ReadyCondition, "NewRevision", "new digest"),
+				*conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new digest"),
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NewGeneration", "new obj gen"),
+				*conditions.TrueCondition(sourcev1.SourceVerifiedCondition, "Success", "verified"),
+			},
+		},
+		{
+			name:       "Fail, multiple bipolar conditions False, Ready gets the bipolar with high priority",
+			generation: 2,
+			beforeFunc: func(obj conditions.Setter) {
+				conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, "Success", "verified")
+				conditions.MarkFalse(obj, testBipolarCondition1, "AAA", "aaa")
+				conditions.MarkFalse(obj, testBipolarCondition2, "BBB", "bbb")
+			},
+			result:            reconcile.ResultEmpty,
+			reconcileErr:      errors.New("some failure"),
+			conditions:        []Conditions{testReadyConditions},
+			bipolarConditions: testBipolarConditions,
+			wantErr:           true,
+			assertConditions: []metav1.Condition{
+				*conditions.FalseCondition(meta.ReadyCondition, "AAA", "aaa"),
+				*conditions.FalseCondition(testBipolarCondition1, "AAA", "aaa"),
+				*conditions.FalseCondition(testBipolarCondition2, "BBB", "bbb"),
+				*conditions.TrueCondition(sourcev1.SourceVerifiedCondition, "Success", "verified"),
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -289,6 +357,9 @@ func TestSummarizeAndPatch(t *testing.T) {
 				WithProcessors(RecordContextualError, RecordReconcileReq),
 				WithResultBuilder(reconcile.AlwaysRequeueResultBuilder{RequeueAfter: obj.Spec.Interval.Duration}),
 			}
+			if tt.bipolarConditions != nil {
+				summaryOpts = append(summaryOpts, WithBiPolarityConditionTypes(tt.bipolarConditions...))
+			}
 			_, gotErr := summaryHelper.SummarizeAndPatch(ctx, obj, summaryOpts...)
 			g.Expect(gotErr != nil).To(Equal(tt.wantErr))