From 18fef776aebe0cb4895683c62de7ad629a143d38 Mon Sep 17 00:00:00 2001 From: Xinru Zhang Date: Tue, 6 Sep 2022 10:59:17 -0400 Subject: [PATCH] TEP-0114: Implements Retries in The Testing Wait Custom Task This commit implements `retries` in the testing Wait custom task, enabling e2e test to cover test cases of Custom Task `retries`. --- test/custom-task-ctrls/wait-task/README.md | 2 +- .../wait-task/pkg/reconciler/reconciler.go | 21 ++ .../pkg/reconciler/reconciler_test.go | 200 ++++++++++++++---- 3 files changed, 180 insertions(+), 43 deletions(-) diff --git a/test/custom-task-ctrls/wait-task/README.md b/test/custom-task-ctrls/wait-task/README.md index 255e6ad4032..bd9d72426cd 100644 --- a/test/custom-task-ctrls/wait-task/README.md +++ b/test/custom-task-ctrls/wait-task/README.md @@ -7,4 +7,4 @@ wait custom task removed. It provides a [Tekton Custom Task](https://tekton.dev/docs/pipelines/runs/) that, when run, simply waits a given amount of time before succeeding, specified by an input parameter named -`duration`. +`duration`. It also supports `timeout` and `retries`. diff --git a/test/custom-task-ctrls/wait-task/pkg/reconciler/reconciler.go b/test/custom-task-ctrls/wait-task/pkg/reconciler/reconciler.go index a783f20a49e..59988c99e8f 100644 --- a/test/custom-task-ctrls/wait-task/pkg/reconciler/reconciler.go +++ b/test/custom-task-ctrls/wait-task/pkg/reconciler/reconciler.go @@ -127,9 +127,30 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, r *v1alpha1.Run) kreconc logger.Infof("The Custom Task Run %v timed out", r.GetName()) r.Status.CompletionTime = &metav1.Time{Time: c.Clock.Now()} r.Status.MarkRunFailed("TimedOut", WaitTaskCancelledByRunTimeoutMsg) + + // Retry if the current RetriesStatus hasn't reached the retries limit + if r.Spec.Retries > len(r.Status.RetriesStatus) { + logger.Infof("Run timed out, retrying... %#v", r.Status) + retryRun(r) + return controller.NewRequeueImmediately() + } + return nil } // Don't emit events on nop-reconciliations, it causes scale problems. return nil } + +func retryRun(run *v1alpha1.Run) { + // Add retry history + newStatus := *run.Status.DeepCopy() + newStatus.RetriesStatus = nil + run.Status.RetriesStatus = append(run.Status.RetriesStatus, newStatus) + + // Clear status + run.Status.StartTime = nil + run.Status.CompletionTime = nil + + run.Status.MarkRunRunning("", "") +} diff --git a/test/custom-task-ctrls/wait-task/pkg/reconciler/reconciler_test.go b/test/custom-task-ctrls/wait-task/pkg/reconciler/reconciler_test.go index 1b789077b3b..a2ec0085cdc 100644 --- a/test/custom-task-ctrls/wait-task/pkg/reconciler/reconciler_test.go +++ b/test/custom-task-ctrls/wait-task/pkg/reconciler/reconciler_test.go @@ -62,7 +62,7 @@ func TestReconcile(t *testing.T) { wantRunConditionType apis.ConditionType wantRunConditionStatus corev1.ConditionStatus wantRunConditionReason string - isParentPRCancelled bool + isCancelled bool }{{ name: "duration elapsed", params: ` @@ -84,13 +84,13 @@ func TestReconcile(t *testing.T) { wantRunConditionType: apis.ConditionSucceeded, wantRunConditionStatus: corev1.ConditionFalse, wantRunConditionReason: "UnexpectedName", - isParentPRCancelled: false, + isCancelled: false, }, { name: "no duration param", wantRunConditionType: apis.ConditionSucceeded, wantRunConditionStatus: corev1.ConditionFalse, wantRunConditionReason: "MissingDuration", - isParentPRCancelled: false, + isCancelled: false, }, { name: "extra param", params: ` @@ -102,7 +102,7 @@ func TestReconcile(t *testing.T) { `, wantRunConditionType: apis.ConditionSucceeded, wantRunConditionStatus: corev1.ConditionFalse, wantRunConditionReason: "UnexpectedParams", - isParentPRCancelled: false, + isCancelled: false, }, { name: "duration param is not a string", params: ` @@ -116,7 +116,7 @@ func TestReconcile(t *testing.T) { wantRunConditionType: apis.ConditionSucceeded, wantRunConditionStatus: corev1.ConditionFalse, wantRunConditionReason: "MissingDuration", - isParentPRCancelled: false, + isCancelled: false, }, { name: "invalid duration value", params: ` @@ -127,7 +127,7 @@ func TestReconcile(t *testing.T) { wantRunConditionType: apis.ConditionSucceeded, wantRunConditionStatus: corev1.ConditionFalse, wantRunConditionReason: "InvalidDuration", - isParentPRCancelled: false, + isCancelled: false, }, { name: "timeout", timeout: "1s", @@ -139,7 +139,7 @@ func TestReconcile(t *testing.T) { wantRunConditionType: apis.ConditionSucceeded, wantRunConditionStatus: corev1.ConditionFalse, wantRunConditionReason: "TimedOut", - isParentPRCancelled: false, + isCancelled: false, }, { name: "timeout equals duration", timeout: "1s", @@ -151,7 +151,7 @@ func TestReconcile(t *testing.T) { wantRunConditionType: apis.ConditionSucceeded, wantRunConditionStatus: corev1.ConditionFalse, wantRunConditionReason: "InvalidTimeOut", - isParentPRCancelled: false, + isCancelled: false, }, { name: "parent pr timeout", timeout: "1s", @@ -163,7 +163,7 @@ func TestReconcile(t *testing.T) { wantRunConditionType: apis.ConditionSucceeded, wantRunConditionStatus: corev1.ConditionFalse, wantRunConditionReason: "Cancelled", - isParentPRCancelled: true, + isCancelled: true, }} { t.Run(tc.name, func(t *testing.T) { ctx := context.Background() @@ -186,40 +186,11 @@ spec: runYAML = runYAML + tc.params } r := parse.MustParseRun(t, runYAML) - if tc.isParentPRCancelled { + if tc.isCancelled { r.Spec.Status = v1alpha1.RunSpecStatusCancelled - r.Spec.StatusMessage = v1alpha1.RunCancelledByPipelineMsg } - // Start reconciling the Run. - // This will not return until the second Reconcile is done. - err := rec.ReconcileKind(ctx, r) - - t.Logf("%#v", r) - - for err != nil { - if strings.Contains(err.Error(), "requeue") { - // simulate EnqueueAfter - var dur time.Duration - var dr error - for _, p := range r.Spec.Params { - if p.Name == "duration" { - dur, dr = time.ParseDuration(p.Value.StringVal) - if dr != nil { - t.Error("failed to parse duration") - } - break - } - } - to := r.GetTimeout() - sleep := int(math.Min(to.Seconds(), dur.Seconds())) - testClock.SetTime(testClock.Now().Add(time.Duration(sleep) * time.Second)) - rec.Clock = testClock - err = rec.ReconcileKind(ctx, r) - } else { - t.Fatalf("ReconcileKind() = %v", err) - } - } + fakeReconcile(t, ctx, rec, r) // Compose expected Run wantRunYAML := fmt.Sprintf(` @@ -244,9 +215,8 @@ status: observedGeneration: 0 `, tc.wantRunConditionReason, tc.wantRunConditionStatus, tc.wantRunConditionType) wantRun := parse.MustParseRun(t, wantRunYAML) - if tc.isParentPRCancelled { + if tc.isCancelled { wantRun.Spec.Status = v1alpha1.RunSpecStatusCancelled - wantRun.Spec.StatusMessage = v1alpha1.RunCancelledByPipelineMsg } if d := cmp.Diff(wantRun, r, @@ -260,3 +230,149 @@ status: }) } } + +func TestReconcile_Retries(t *testing.T) { + for _, tc := range []struct { + name string + duration string + timeout string + retries int + params string + wantStatus string + isCancelled bool + }{{ + name: "retry when timeout", + duration: "2s", + timeout: "1s", + retries: 1, + wantStatus: fmt.Sprintf(` +status: + conditions: + - reason: %s + status: %q + type: %s + observedGeneration: 0 + retriesStatus: + - conditions: + - reason: %s + status: %q + type: %s +`, "TimedOut", corev1.ConditionFalse, apis.ConditionSucceeded, "TimedOut", corev1.ConditionFalse, apis.ConditionSucceeded), + isCancelled: false, + }, { + name: "don't retry if retries unspecified", + duration: "2s", + timeout: "1s", + wantStatus: fmt.Sprintf(` +status: + conditions: + - reason: %s + status: %q + type: %s + observedGeneration: 0 +`, "TimedOut", corev1.ConditionFalse, apis.ConditionSucceeded), + isCancelled: false, + }, { + name: "don't retry when canceled", + duration: "2s", + wantStatus: fmt.Sprintf(` +status: + conditions: + - reason: %s + status: %q + type: %s + observedGeneration: 0 +`, "Cancelled", corev1.ConditionFalse, apis.ConditionSucceeded), + isCancelled: true, + }} { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + rec := &Reconciler{ + Clock: testClock, + } + + runName := helpers.ObjectNameForTest(t) + runYAML := fmt.Sprintf(` +metadata: + name: %s +spec: + retries: %d + timeout: %s + ref: + apiVersion: %s + kind: %s + params: + - name: duration + value: %s +`, runName, tc.retries, tc.timeout, apiVersion, kind, tc.duration) + r := parse.MustParseRun(t, runYAML) + if tc.isCancelled { + r.Spec.Status = v1alpha1.RunSpecStatusCancelled + } + + fakeReconcile(t, ctx, rec, r) + + // Compose expected Run + wantRunYAML := fmt.Sprintf(` +metadata: + name: %s +spec: + retries: %d + timeout: %s + ref: + apiVersion: %s + kind: %s + params: + - name: duration + value: %s +`, runName, tc.retries, tc.timeout, apiVersion, kind, tc.duration) + wantRunYAML = wantRunYAML + tc.wantStatus + wantRun := parse.MustParseRun(t, wantRunYAML) + if tc.isCancelled { + wantRun.Spec.Status = v1alpha1.RunSpecStatusCancelled + } + + if d := cmp.Diff(wantRun, r, + filterTypeMeta, + filterObjectMeta, + filterCondition, + filterRunStatus, + ); d != "" { + t.Errorf("-got +want: %v", d) + } + }) + } +} + +// TODO(#5456) Avoid duplicate reconcile logic +func fakeReconcile(t *testing.T, ctx context.Context, rec *Reconciler, r *v1alpha1.Run) { + t.Helper() + + // Start reconciling the Run. + // This will not return until the second Reconcile is done. + err := rec.ReconcileKind(ctx, r) + + for err != nil { + if strings.Contains(err.Error(), "requeue") { + // simulate EnqueueAfter + var dur time.Duration + var dr error + for _, p := range r.Spec.Params { + if p.Name == "duration" { + dur, dr = time.ParseDuration(p.Value.StringVal) + if dr != nil { + t.Error("failed to parse duration") + } + break + } + } + to := r.GetTimeout() + sleep := int(math.Min(to.Seconds(), dur.Seconds())) + testClock.SetTime(testClock.Now().Add(time.Duration(sleep) * time.Second)) + rec.Clock = testClock + err = rec.ReconcileKind(ctx, r) + } else { + t.Fatalf("ReconcileKind() = %v", err) + } + } +}