From d61ee6c0bee9fd896c30d0c0d9de4894a0a6da38 Mon Sep 17 00:00:00 2001 From: Scott Date: Mon, 11 Oct 2021 13:17:19 -0400 Subject: [PATCH] Fix panic when pending pipelinerun is failed PipelineRuns that are created with status PipelineRunPending can be placed into a failed state before their execution begins. For example: a third-party controller may be watching for pending PipelineRuns to perform some checks on them prior to execution beginning. If those checks fail the controller might choose to set the PipelineRun status to failed with a relevant error message indicating which check failed and why. Prior to this commit when a pending PR failed our metrics code could panic because the PR's StartTime is nil. This commit adds a guard to the metrics code to ensure that StartTime is not nil before computing the PR's duration. If it is nil then we assume the duration is 0. A unit test confirming this behaviour has been added as well. (cherry picked from commit 0299c6c2c35b809f903e2dbb287047977e60cbcc) --- pkg/pipelinerunmetrics/metrics.go | 9 ++++++--- pkg/pipelinerunmetrics/metrics_test.go | 28 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/pkg/pipelinerunmetrics/metrics.go b/pkg/pipelinerunmetrics/metrics.go index 3d6a7d1ad05..f4e64b2aa32 100644 --- a/pkg/pipelinerunmetrics/metrics.go +++ b/pkg/pipelinerunmetrics/metrics.go @@ -216,9 +216,12 @@ func (r *Recorder) DurationAndCount(pr *v1beta1.PipelineRun) error { return fmt.Errorf("ignoring the metrics recording for %s , failed to initialize the metrics recorder", pr.Name) } - duration := time.Since(pr.Status.StartTime.Time) - if pr.Status.CompletionTime != nil { - duration = pr.Status.CompletionTime.Sub(pr.Status.StartTime.Time) + duration := time.Duration(0) + if pr.Status.StartTime != nil { + duration = time.Since(pr.Status.StartTime.Time) + if pr.Status.CompletionTime != nil { + duration = pr.Status.CompletionTime.Sub(pr.Status.StartTime.Time) + } } status := "success" diff --git a/pkg/pipelinerunmetrics/metrics_test.go b/pkg/pipelinerunmetrics/metrics_test.go index a036c909d1d..13d04bebb36 100644 --- a/pkg/pipelinerunmetrics/metrics_test.go +++ b/pkg/pipelinerunmetrics/metrics_test.go @@ -210,6 +210,34 @@ func TestRecordPipelineRunDurationCount(t *testing.T) { }, expectedDuration: 60, expectedCount: 1, + }, { + name: "for pipeline without start or completion time", + pipelineRun: &v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{Name: "pipelinerun-1", Namespace: "ns"}, + Spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{Name: "pipeline-1"}, + Status: v1beta1.PipelineRunSpecStatusPending, + }, + Status: v1beta1.PipelineRunStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + }}, + }, + }, + }, + expectedTags: map[string]string{ + "pipeline": "pipeline-1", + "pipelinerun": "pipelinerun-1", + "namespace": "ns", + "status": "failed", + }, + expectedCountTags: map[string]string{ + "status": "failed", + }, + expectedDuration: 0, + expectedCount: 1, }} { t.Run(test.name, func(t *testing.T) { unregisterMetrics()