Merge pull request #729 from fluxcd/observed-gen-no-condition

reconcile: Set observed gen only when conditions exist

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