From e253e4c62bf7afe6dbc67d78e8b7b7010670b88b Mon Sep 17 00:00:00 2001 From: Sunny Date: Sun, 4 Dec 2022 18:38:28 +0000 Subject: [PATCH] reconcile: Add support for progressive status Replace the patch Helper with SerialPatcher which is used for progressive status patching. Update the tests to use progressive status reasons in tests. Add ProgressingWithRetry Reconciling reason for failed reconciliation result to indicate a finished failure operation. Signed-off-by: Sunny --- internal/reconcile/reconcile.go | 9 +++++++ internal/reconcile/reconcile_test.go | 15 +++++++++++ internal/reconcile/summarize/summary.go | 12 ++++----- internal/reconcile/summarize/summary_test.go | 26 +++++++++----------- 4 files changed, 42 insertions(+), 20 deletions(-) diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go index 3c25474d..58a160b8 100644 --- a/internal/reconcile/reconcile.go +++ b/internal/reconcile/reconcile.go @@ -124,6 +124,15 @@ func ComputeReconcileResult(obj conditions.Setter, res Result, recErr error, rb conditions.Delete(obj, meta.ReconcilingCondition) } + // Presence of reconciling means that the reconciliation didn't succeed. + // Set the Reconciling reason to ProgressingWithRetry to indicate a failure + // retry. + if conditions.IsReconciling(obj) { + reconciling := conditions.Get(obj, meta.ReconcilingCondition) + reconciling.Reason = meta.ProgressingWithRetryReason + conditions.Set(obj, reconciling) + } + // Analyze the reconcile error. switch t := recErr.(type) { case *serror.Stalling: diff --git a/internal/reconcile/reconcile_test.go b/internal/reconcile/reconcile_test.go index b9b2ccfe..15a60b0d 100644 --- a/internal/reconcile/reconcile_test.go +++ b/internal/reconcile/reconcile_test.go @@ -206,6 +206,21 @@ func TestComputeReconcileResult(t *testing.T) { t.Expect(conditions.IsUnknown(obj, meta.StalledCondition)).To(BeTrue()) }, }, + { + name: "failed with Reconciling=True adds ProgressingWithRetry reason", + beforeFunc: func(obj conditions.Setter) { + conditions.MarkReconciling(obj, meta.ProgressingReason, "some msg") + }, + result: ResultEmpty, + recErr: fmt.Errorf("some error"), + wantResult: ctrl.Result{}, + wantErr: true, + afterFunc: func(t *WithT, obj conditions.Setter, patchOpts *patch.HelperOptions) { + }, + assertConditions: []metav1.Condition{ + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "some msg"), + }, + }, } for _, tt := range tests { diff --git a/internal/reconcile/summarize/summary.go b/internal/reconcile/summarize/summary.go index 6a556a18..3977ccdf 100644 --- a/internal/reconcile/summarize/summary.go +++ b/internal/reconcile/summarize/summary.go @@ -50,15 +50,15 @@ type Conditions struct { // Helper is SummarizeAndPatch helper. type Helper struct { - recorder kuberecorder.EventRecorder - patchHelper *patch.Helper + recorder kuberecorder.EventRecorder + serialPatcher *patch.SerialPatcher } // NewHelper returns an initialized Helper. -func NewHelper(recorder kuberecorder.EventRecorder, patchHelper *patch.Helper) *Helper { +func NewHelper(recorder kuberecorder.EventRecorder, serialPatcher *patch.SerialPatcher) *Helper { return &Helper{ - recorder: recorder, - patchHelper: patchHelper, + recorder: recorder, + serialPatcher: serialPatcher, } } @@ -250,7 +250,7 @@ func (h *Helper) SummarizeAndPatch(ctx context.Context, obj conditions.Setter, o } // Finally, patch the resource. - if err := h.patchHelper.Patch(ctx, obj, patchOpts...); err != nil { + if err := h.serialPatcher.Patch(ctx, obj, patchOpts...); err != nil { // Ignore patch error "not found" when the object is being deleted. if opts.IgnoreNotFound && !obj.GetDeletionTimestamp().IsZero() { err = kerrors.FilterOut(err, func(e error) bool { return apierrors.IsNotFound(e) }) diff --git a/internal/reconcile/summarize/summary_test.go b/internal/reconcile/summarize/summary_test.go index 18de95f4..48ee5648 100644 --- a/internal/reconcile/summarize/summary_test.go +++ b/internal/reconcile/summarize/summary_test.go @@ -128,7 +128,7 @@ func TestSummarizeAndPatch(t *testing.T) { name: "Success, removes reconciling for successful result", generation: 2, beforeFunc: func(obj conditions.Setter) { - conditions.MarkReconciling(obj, "NewRevision", "new index version") + conditions.MarkReconciling(obj, meta.ProgressingReason, "new index version") conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "stored artifact") }, conditions: []Conditions{testReadyConditions}, @@ -167,7 +167,7 @@ func TestSummarizeAndPatch(t *testing.T) { generation: 7, beforeFunc: func(obj conditions.Setter) { conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision") - conditions.MarkReconciling(obj, "NewRevision", "new index revision") + conditions.MarkReconciling(obj, meta.ProgressingReason, "new index revision") }, conditions: []Conditions{testReadyConditions}, reconcileErr: fmt.Errorf("failed to create dir"), @@ -175,7 +175,7 @@ func TestSummarizeAndPatch(t *testing.T) { assertConditions: []metav1.Condition{ *conditions.FalseCondition(meta.ReadyCondition, "NewRevision", "new index revision"), *conditions.TrueCondition(sourcev1.ArtifactOutdatedCondition, "NewRevision", "new index revision"), - *conditions.TrueCondition(meta.ReconcilingCondition, "NewRevision", "new index revision"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "new index revision"), }, afterFunc: func(t *WithT, obj client.Object) { t.Expect(obj).ToNot(HaveStatusObservedGeneration(7)) @@ -264,7 +264,7 @@ func TestSummarizeAndPatch(t *testing.T) { 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.MarkReconciling(obj, meta.ProgressingReason, "new index revision") conditions.MarkFalse(obj, sourcev1.SourceVerifiedCondition, "VerifyFailed", "verify failed") }, result: reconcile.ResultEmpty, @@ -275,14 +275,14 @@ func TestSummarizeAndPatch(t *testing.T) { 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"), + *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "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.MarkReconciling(obj, meta.ProgressingReason, "new obj gen") conditions.MarkTrue(obj, sourcev1.ArtifactOutdatedCondition, "NewRevision", "new digest") conditions.MarkTrue(obj, sourcev1.SourceVerifiedCondition, "Success", "verified") }, @@ -294,7 +294,7 @@ func TestSummarizeAndPatch(t *testing.T) { 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(meta.ReconcilingCondition, meta.ProgressingWithRetryReason, "new obj gen"), *conditions.TrueCondition(sourcev1.SourceVerifiedCondition, "Success", "verified"), }, }, @@ -345,10 +345,9 @@ func TestSummarizeAndPatch(t *testing.T) { ctx := context.TODO() g.Expect(client.Create(ctx, obj)).To(Succeed()) - patchHelper, err := patch.NewHelper(obj, client) - g.Expect(err).ToNot(HaveOccurred()) + serialPatcher := patch.NewSerialPatcher(obj, client) - summaryHelper := NewHelper(record.NewFakeRecorder(32), patchHelper) + summaryHelper := NewHelper(record.NewFakeRecorder(32), serialPatcher) summaryOpts := []Option{ WithReconcileResult(tt.result), WithReconcileError(tt.reconcileErr), @@ -471,15 +470,14 @@ func TestSummarizeAndPatch_Intermediate(t *testing.T) { ctx := context.TODO() g.Expect(kclient.Create(ctx, obj)).To(Succeed()) - patchHelper, err := patch.NewHelper(obj, kclient) - g.Expect(err).ToNot(HaveOccurred()) + serialPatcher := patch.NewSerialPatcher(obj, kclient) - summaryHelper := NewHelper(record.NewFakeRecorder(32), patchHelper) + summaryHelper := NewHelper(record.NewFakeRecorder(32), serialPatcher) summaryOpts := []Option{ WithConditions(tt.conditions...), WithResultBuilder(reconcile.AlwaysRequeueResultBuilder{RequeueAfter: interval}), } - _, err = summaryHelper.SummarizeAndPatch(ctx, obj, summaryOpts...) + _, err := summaryHelper.SummarizeAndPatch(ctx, obj, summaryOpts...) g.Expect(err).ToNot(HaveOccurred()) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions))