diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go index b1e11409a..5e3b21e4c 100644 --- a/internal/reconcile/reconcile.go +++ b/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 +} diff --git a/internal/reconcile/reconcile_test.go b/internal/reconcile/reconcile_test.go index 3d3f4fc0a..b9b2ccfea 100644 --- a/internal/reconcile/reconcile_test.go +++ b/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)) + }) + } +}