reconcile: Set observed gen when conditions exist

The observed generation must be set only when actual observation is
made. When an actual observation is made, some conditions are set on the
object. Introduce a helper function
addPatchOptionWithStatusObservedGeneration() to set the patcher option
WithStatusObservedGeneration only when there's any condition in the
status.

Updates the existing tests that depended on this behavior.

This fixes the issue where the observed generation is set by the patcher
when a reconciler does an early return for setting the finalizers only.
With this, the observed generation will be updated only when some
observations are made on the object based on the usual rules of success
result, no error, ignore error and stalled condition.

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

Author: darkowlzz

internal/reconcile/reconcile.go

@@ -128,11 +128,11 @@ func ComputeReconcileResult(obj conditions.Setter, res Result, recErr error, rb
 	switch t := recErr.(type) {
 	case *serror.Stalling:
 		if res == ResultEmpty {
+			conditions.MarkStalled(obj, t.Reason, t.Error())
 			// The current generation has been reconciled successfully and it
 			// has resulted in a stalled state. Return no error to stop further
 			// requeuing.
-			pOpts = append(pOpts, patch.WithStatusObservedGeneration{})
-			conditions.MarkStalled(obj, t.Reason, t.Error())
+			pOpts = addPatchOptionWithStatusObservedGeneration(obj, pOpts)
 			return pOpts, result, nil
 		}
 		// NOTE: Non-empty result with stalling error indicates that the
@@ -150,7 +150,7 @@ func ComputeReconcileResult(obj conditions.Setter, res Result, recErr error, rb
 		if t.Ignore {
 			// The current generation has been reconciled successfully with
 			// no-op result. Update status observed generation.
-			pOpts = append(pOpts, patch.WithStatusObservedGeneration{})
+			pOpts = addPatchOptionWithStatusObservedGeneration(obj, pOpts)
 			conditions.Delete(obj, meta.ReconcilingCondition)
 			return pOpts, result, nil
 		}
@@ -159,7 +159,7 @@ func ComputeReconcileResult(obj conditions.Setter, res Result, recErr error, rb
 		// state. If a requeue is requested, the current generation has not been
 		// reconciled successfully.
 		if res != ResultRequeue {
-			pOpts = append(pOpts, patch.WithStatusObservedGeneration{})
+			pOpts = addPatchOptionWithStatusObservedGeneration(obj, pOpts)
 		}
 		conditions.Delete(obj, meta.StalledCondition)
 	default:
@@ -207,3 +207,17 @@ func FailureRecovery(oldObj, newObj conditions.Getter, failConditions []string)
 	}
 	return failuresBefore > 0
 }
+
+// addPatchOptionWithStatusObservedGeneration adds patch option
+// WithStatusObservedGeneration to the provided patch option slice only if there
+// is any condition present on the object, and returns it. This is necessary to
+// prevent setting status observed generation without any effectual observation.
+// An object must have some condition in the status if it has been observed.
+// TODO: Move this to fluxcd/pkg/runtime/patch package after it has proven its
+// need.
+func addPatchOptionWithStatusObservedGeneration(obj conditions.Setter, opts []patch.Option) []patch.Option {
+	if len(obj.GetConditions()) > 0 {
+		opts = append(opts, patch.WithStatusObservedGeneration{})
+	}
+	return opts
+}

internal/reconcile/reconcile_test.go

@@ -71,11 +71,17 @@ func TestComputeReconcileResult(t *testing.T) {
 		afterFunc        func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions)
 	}{
 		{
-			name:       "successful result",
-			result:     ResultSuccess,
+			name:   "successful result",
+			result: ResultSuccess,
+			beforeFunc: func(obj conditions.Setter) {
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo")
+			},
 			recErr:     nil,
 			wantResult: ctrl.Result{RequeueAfter: testSuccessInterval},
 			wantErr:    false,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "foo"),
+			},
 			afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) {
 				t.Expect(patchOpts.IncludeStatusObservedGeneration).To(BeTrue())
 			},
@@ -85,10 +91,14 @@ func TestComputeReconcileResult(t *testing.T) {
 			result: ResultSuccess,
 			beforeFunc: func(obj conditions.Setter) {
 				conditions.MarkReconciling(obj, "NewRevision", "new revision")
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo")
 			},
 			recErr:     nil,
 			wantResult: ctrl.Result{RequeueAfter: testSuccessInterval},
 			wantErr:    false,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReadyCondition, meta.SucceededReason, "foo"),
+			},
 			afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) {
 				t.Expect(patchOpts.IncludeStatusObservedGeneration).To(BeTrue())
 				t.Expect(conditions.IsUnknown(obj, meta.ReconcilingCondition)).To(BeTrue())
@@ -367,3 +377,58 @@ func TestFailureRecovery(t *testing.T) {
 		})
 	}
 }
+
+func TestAddOptionWithStatusObservedGeneration(t *testing.T) {
+	tests := []struct {
+		name       string
+		beforeFunc func(obj conditions.Setter)
+		patchOpts  []patch.Option
+		want       bool
+	}{
+		{
+			name: "no conditions",
+			want: false,
+		},
+		{
+			name: "some condition",
+			beforeFunc: func(obj conditions.Setter) {
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo")
+			},
+			want: true,
+		},
+		{
+			name: "existing option with conditions",
+			beforeFunc: func(obj conditions.Setter) {
+				conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "foo")
+			},
+			patchOpts: []patch.Option{patch.WithForceOverwriteConditions{}, patch.WithStatusObservedGeneration{}},
+			want:      true,
+		},
+		{
+			name:      "existing option, no conditions, can't remove",
+			patchOpts: []patch.Option{patch.WithForceOverwriteConditions{}, patch.WithStatusObservedGeneration{}},
+			want:      true,
+		},
+	}
+
+	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)
+			}
+
+			tt.patchOpts = addPatchOptionWithStatusObservedGeneration(obj, tt.patchOpts)
+
+			// Apply the options and evaluate the result.
+			options := &patch.HelperOptions{}
+			for _, opt := range tt.patchOpts {
+				opt.ApplyToHelper(options)
+			}
+			g.Expect(options.IncludeStatusObservedGeneration).To(Equal(tt.want))
+		})
+	}
+}