From 53888a98226b2ec1ae81333d53bcc1d50f48f0ef Mon Sep 17 00:00:00 2001 From: pratap0007 Date: Thu, 7 Mar 2024 19:44:34 +0530 Subject: [PATCH] Fix reason of PipelineRun status for PipelineRun having tasks timeout This patch fixes to set the reason of PipelineRun status to PipelineRunTimeout if the tasks of a PipelineRun timeout because of the timeout.tasks parameter Signed-off-by: Shiv Verma --- .../pipelinerun/pipelinerun_test.go | 21 ++++++------ .../pipelinerun/resources/pipelinerunstate.go | 9 +++++ .../resources/pipelinerunstate_test.go | 33 +++++++++++++++++++ test/timeout_test.go | 2 +- 4 files changed, 54 insertions(+), 11 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index 780249b6a40..94918a4ec0a 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -2901,20 +2901,21 @@ status: defer prt.Cancel() wantEvents := []string{ - "Normal Started", + "Warning Failed PipelineRun", } reconciledRun, clients := prt.reconcileRun("foo", "test-pipeline-run-with-timeout", wantEvents, false) - if reconciledRun.Status.CompletionTime != nil { - t.Errorf("Expected nil CompletionTime on PipelineRun but was %s", reconciledRun.Status.CompletionTime) + if reconciledRun.Status.CompletionTime == nil { + t.Errorf("Expected CompletionTime on PipelineRun but was %s", reconciledRun.Status.CompletionTime) } - // The PipelineRun should be running. - if reconciledRun.Status.GetCondition(apis.ConditionSucceeded).Reason != v1.PipelineRunReasonRunning.String() { - t.Errorf("Expected PipelineRun to be running, but condition reason is %s", reconciledRun.Status.GetCondition(apis.ConditionSucceeded).Reason) + // The PipelineRun should be timeout. + if reconciledRun.Status.GetCondition(apis.ConditionSucceeded).Reason != v1.PipelineRunReasonTimedOut.String() { + t.Errorf("Expected PipelineRun to be timeout, but condition reason is %s", reconciledRun.Status.GetCondition(apis.ConditionSucceeded).Reason) } // Check that there is a skipped task for the expected reason + if len(reconciledRun.Status.SkippedTasks) != 1 { t.Errorf("expected one skipped task, found %d", len(reconciledRun.Status.SkippedTasks)) } else if reconciledRun.Status.SkippedTasks[0].Reason != v1.TasksTimedOutSkip { @@ -3041,7 +3042,7 @@ spec: pipelineRef: name: test-pipeline-with-finally timeouts: - tasks: 5m + tasks: 25m pipeline: 20m status: finallyStartTime: "2021-12-31T23:44:59Z" @@ -3269,7 +3270,7 @@ spec: pipelineRef: name: test-pipeline-with-finally timeouts: - tasks: 5m + tasks: 25m pipeline: 20m status: startTime: "2021-12-31T23:40:00Z" @@ -3318,7 +3319,7 @@ spec: pipelineRef: name: test-pipeline-with-finally timeouts: - tasks: 5m + tasks: 25m pipeline: 20m status: startTime: "2021-12-31T23:40:00Z" @@ -3358,7 +3359,7 @@ spec: pipelineRef: name: test-pipeline-with-finally timeouts: - tasks: 5m + tasks: 25m pipeline: 20m status: startTime: "2021-12-31T23:40:00Z" diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go index 0f1b82621cc..33a91213f59 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunstate.go @@ -497,6 +497,15 @@ func (facts *PipelineRunFacts) GetPipelineConditionStatus(ctx context.Context, p } } + if pr.HaveTasksTimedOut(ctx, c) { + return &apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + Reason: v1.PipelineRunReasonTimedOut.String(), + Message: fmt.Sprintf("PipelineRun %q failed due to tasks failed to finish within %q", pr.Name, pr.TasksTimeout().Duration.String()), + } + } + // report the count in PipelineRun Status // get the count of successful tasks, failed tasks, cancelled tasks, skipped task, and incomplete tasks s := facts.getPipelineTasksCount() diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go index 1c9be5a6793..67fb995bc79 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go @@ -2141,6 +2141,39 @@ func TestGetPipelineConditionStatus_PipelineTimeouts(t *testing.T) { } } +// pipeline should result in timeout if its runtime exceeds its spec.Timeouts.Tasks based on its status.Timeout +func TestGetPipelineConditionStatus_PipelineTasksTimeouts(t *testing.T) { + d, err := dagFromState(oneFinishedState) + if err != nil { + t.Fatalf("Unexpected error while building DAG for state %v: %v", oneFinishedState, err) + } + pr := &v1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "pipelinerun-no-tasks-started"}, + Spec: v1.PipelineRunSpec{ + Timeouts: &v1.TimeoutFields{ + Tasks: &metav1.Duration{Duration: 1 * time.Minute}, + }, + }, + Status: v1.PipelineRunStatus{ + PipelineRunStatusFields: v1.PipelineRunStatusFields{ + StartTime: &metav1.Time{Time: now.Add(-2 * time.Minute)}, + }, + }, + } + facts := PipelineRunFacts{ + State: oneFinishedState, + TasksGraph: d, + FinalTasksGraph: &dag.Graph{}, + TimeoutsState: PipelineRunTimeoutsState{ + Clock: testClock, + }, + } + c := facts.GetPipelineConditionStatus(context.Background(), pr, zap.NewNop().Sugar(), testClock) + if c.Status != corev1.ConditionFalse && c.Reason != v1.PipelineRunReasonTimedOut.String() { + t.Fatalf("Expected to get status %s but got %s for state %v", corev1.ConditionFalse, c.Status, oneFinishedState) + } +} + func TestGetPipelineConditionStatus_OnError(t *testing.T) { var oneFailedStateOnError = PipelineRunState{{ PipelineTask: &v1.PipelineTask{ diff --git a/test/timeout_test.go b/test/timeout_test.go index 2b407b66890..7996e51bd8e 100644 --- a/test/timeout_test.go +++ b/test/timeout_test.go @@ -511,7 +511,7 @@ spec: } t.Logf("Waiting for PipelineRun %s in namespace %s to be failed", pipelineRun.Name, namespace) - if err := WaitForPipelineRunState(ctx, c, pipelineRun.Name, timeout, FailedWithReason(v1.PipelineRunReasonFailed.String(), pipelineRun.Name), "PipelineRunFailed", v1Version); err != nil { + if err := WaitForPipelineRunState(ctx, c, pipelineRun.Name, timeout, FailedWithReason(v1.PipelineRunReasonTimedOut.String(), pipelineRun.Name), "PipelineRunFailed", v1Version); err != nil { t.Errorf("Error waiting for PipelineRun %s to finish: %s", pipelineRun.Name, err) }