From b3d27943341ad27652f149a6b4777659f9cb6240 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Tue, 16 Jul 2019 17:17:52 +0200 Subject: [PATCH] Do not panic if Timeout is nil `TaskRun` and `PipelineRun` should have a default Timeout set by the webhook. That said, if the webhook didn't do its work, or the object where created before those webhook defaults, it could be nil and makes the controller panicking. Signed-off-by: Vincent Demeester --- pkg/reconciler/timeout_handler.go | 17 ++++++++-- pkg/reconciler/timeout_handler_test.go | 31 +++++++++++++++++-- .../v1alpha1/pipelinerun/pipelinerun.go | 13 ++++++-- pkg/reconciler/v1alpha1/taskrun/taskrun.go | 6 +++- .../v1alpha1/taskrun/taskrun_test.go | 22 +++++++++++-- test/builder/pipeline.go | 7 ++++- test/builder/task.go | 5 +++ 7 files changed, 90 insertions(+), 11 deletions(-) diff --git a/pkg/reconciler/timeout_handler.go b/pkg/reconciler/timeout_handler.go index 240a219ea43..20896b90aba 100644 --- a/pkg/reconciler/timeout_handler.go +++ b/pkg/reconciler/timeout_handler.go @@ -7,6 +7,7 @@ import ( "time" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" clientset "github.com/tektoncd/pipeline/pkg/client/clientset/versioned" "go.uber.org/zap" @@ -204,14 +205,26 @@ func (t *TimeoutSet) checkTaskRunTimeouts(namespace string, pipelineclientset cl // 1. Stop signal, 2. TaskRun to complete or 3. Taskrun to time out, which is // determined by checking if the tr's timeout has occurred since the startTime func (t *TimeoutSet) WaitTaskRun(tr *v1alpha1.TaskRun, startTime *metav1.Time) { - t.waitRun(tr, tr.Spec.Timeout.Duration, startTime, t.taskRunCallbackFunc) + var timeout time.Duration + if tr.Spec.Timeout == nil { + timeout = config.DefaultTimeoutMinutes * time.Minute + } else { + timeout = tr.Spec.Timeout.Duration + } + t.waitRun(tr, timeout, startTime, t.taskRunCallbackFunc) } // WaitPipelineRun function creates a blocking function for pipelinerun to wait for // 1. Stop signal, 2. pipelinerun to complete or 3. pipelinerun to time out which is // determined by checking if the tr's timeout has occurred since the startTime func (t *TimeoutSet) WaitPipelineRun(pr *v1alpha1.PipelineRun, startTime *metav1.Time) { - t.waitRun(pr, pr.Spec.Timeout.Duration, startTime, t.pipelineRunCallbackFunc) + var timeout time.Duration + if pr.Spec.Timeout == nil { + timeout = config.DefaultTimeoutMinutes * time.Minute + } else { + timeout = pr.Spec.Timeout.Duration + } + t.waitRun(pr, timeout, startTime, t.pipelineRunCallbackFunc) } func (t *TimeoutSet) waitRun(runObj StatusKey, timeout time.Duration, startTime *metav1.Time, callback func(interface{})) { diff --git a/pkg/reconciler/timeout_handler_test.go b/pkg/reconciler/timeout_handler_test.go index cb792421576..0f465c4d3f7 100644 --- a/pkg/reconciler/timeout_handler_test.go +++ b/pkg/reconciler/timeout_handler_test.go @@ -44,6 +44,15 @@ func TestTaskRunCheckTimeouts(t *testing.T) { tb.TaskRunStartTime(time.Now()), )) + taskRunRunningNilTimeout := tb.TaskRun("test-taskrun-running-nil-timeout", testNs, tb.TaskRunSpec( + tb.TaskRunTaskRef(simpleTask.Name, tb.TaskRefAPIVersion("a1")), + tb.TaskRunNilTimeout, + ), tb.TaskRunStatus(tb.Condition(apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionUnknown}), + tb.TaskRunStartTime(time.Now()), + )) + taskRunDone := tb.TaskRun("test-taskrun-completed", testNs, tb.TaskRunSpec( tb.TaskRunTaskRef(simpleTask.Name, tb.TaskRefAPIVersion("a1")), tb.TaskRunTimeout(config.DefaultTimeoutMinutes*time.Minute), @@ -62,7 +71,7 @@ func TestTaskRunCheckTimeouts(t *testing.T) { )) d := test.Data{ - TaskRuns: []*v1alpha1.TaskRun{taskRunTimedout, taskRunRunning, taskRunDone, taskRunCancelled}, + TaskRuns: []*v1alpha1.TaskRun{taskRunTimedout, taskRunRunning, taskRunDone, taskRunCancelled, taskRunRunningNilTimeout}, Tasks: []*v1alpha1.Task{simpleTask}, Namespaces: []*corev1.Namespace{{ ObjectMeta: metav1.ObjectMeta{ @@ -98,6 +107,10 @@ func TestTaskRunCheckTimeouts(t *testing.T) { name: "running", taskRun: taskRunRunning, expectCallback: false, + }, { + name: "running-with-nil-timeout", + taskRun: taskRunRunningNilTimeout, + expectCallback: false, }, { name: "completed", taskRun: taskRunDone, @@ -152,6 +165,16 @@ func TestPipelinRunCheckTimeouts(t *testing.T) { tb.PipelineRunStartTime(time.Now()), ), ) + prRunningNilTimeout := tb.PipelineRun("test-pipeline-running-nil-timeout", testNs, + tb.PipelineRunSpec("test-pipeline", + tb.PipelineRunNilTimeout, + ), + tb.PipelineRunStatus(tb.PipelineRunStatusCondition(apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionUnknown}), + tb.PipelineRunStartTime(time.Now()), + ), + ) prDone := tb.PipelineRun("test-pipeline-done", testNs, tb.PipelineRunSpec("test-pipeline", tb.PipelineRunTimeout(config.DefaultTimeoutMinutes*time.Minute), @@ -172,7 +195,7 @@ func TestPipelinRunCheckTimeouts(t *testing.T) { ), ) d := test.Data{ - PipelineRuns: []*v1alpha1.PipelineRun{prTimeout, prRunning, prDone, prCancelled}, + PipelineRuns: []*v1alpha1.PipelineRun{prTimeout, prRunning, prDone, prCancelled, prRunningNilTimeout}, Pipelines: []*v1alpha1.Pipeline{simplePipeline}, Tasks: []*v1alpha1.Task{ts}, Namespaces: []*corev1.Namespace{{ @@ -205,6 +228,10 @@ func TestPipelinRunCheckTimeouts(t *testing.T) { name: "pr-running", pr: prRunning, expectCallback: false, + }, { + name: "pr-running-nil-timeout", + pr: prRunningNilTimeout, + expectCallback: false, }, { name: "pr-timedout", pr: prTimeout, diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go index d27dedfd327..6b64f77e1fe 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go @@ -26,6 +26,7 @@ import ( "github.com/knative/pkg/configmap" "github.com/knative/pkg/controller" "github.com/knative/pkg/tracker" + "github.com/tektoncd/pipeline/pkg/apis/config" apisconfig "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" @@ -397,10 +398,16 @@ func (c *Reconciler) updateTaskRunsStatusDirectly(pr *v1alpha1.PipelineRun) erro func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, rprt *resources.ResolvedPipelineRunTask, pr *v1alpha1.PipelineRun, storageBasePath string) (*v1alpha1.TaskRun, error) { var taskRunTimeout = &metav1.Duration{Duration: apisconfig.NoTimeoutDuration} + var timeout time.Duration + if pr.Spec.Timeout == nil { + timeout = config.DefaultTimeoutMinutes + } else { + timeout = pr.Spec.Timeout.Duration + } // If the value of the timeout is 0 for any resource, there is no timeout. // It is impossible for pr.Spec.Timeout to be nil, since SetDefault always assigns it with a value. - if pr.Spec.Timeout.Duration != apisconfig.NoTimeoutDuration { - pTimeoutTime := pr.Status.StartTime.Add(pr.Spec.Timeout.Duration) + if timeout != apisconfig.NoTimeoutDuration { + pTimeoutTime := pr.Status.StartTime.Add(timeout) if time.Now().After(pTimeoutTime) { // Just in case something goes awry and we're creating the TaskRun after it should have already timed out, // set the timeout to 1 second. @@ -409,7 +416,7 @@ func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, rprt *resources.Re taskRunTimeout = &metav1.Duration{Duration: 1 * time.Second} } } else { - taskRunTimeout = pr.Spec.Timeout + taskRunTimeout = &metav1.Duration{Duration: timeout} } } diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun.go b/pkg/reconciler/v1alpha1/taskrun/taskrun.go index ffd3c5ddf4a..dd64b900766 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun.go @@ -26,6 +26,7 @@ import ( "github.com/knative/pkg/apis" "github.com/knative/pkg/controller" "github.com/knative/pkg/tracker" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" listers "github.com/tektoncd/pipeline/pkg/client/listers/pipeline/v1alpha1" @@ -213,6 +214,9 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error tr.ObjectMeta.Annotations[key] = value } + if tr.Spec.Timeout == nil { + tr.Spec.Timeout = &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute} + } // Check if the TaskRun has timed out; if it is, this will set its status // accordingly. if CheckTimeout(tr) { @@ -483,7 +487,6 @@ func createRedirectedTaskSpec(kubeclient kubernetes.Interface, ts *v1alpha1.Task type DeletePod func(podName string, options *metav1.DeleteOptions) error func (c *Reconciler) updateTaskRunStatusForTimeout(tr *v1alpha1.TaskRun, dp DeletePod) error { - timeout := tr.Spec.Timeout.Duration c.Logger.Infof("TaskRun %q has timed out, deleting pod", tr.Name) // tr.Status.PodName will be empty if the pod was never successfully created. This condition // can be reached, for example, by the pod never being schedulable due to limits imposed by @@ -495,6 +498,7 @@ func (c *Reconciler) updateTaskRunStatusForTimeout(tr *v1alpha1.TaskRun, dp Dele } } + timeout := tr.Spec.Timeout.Duration timeoutMsg := fmt.Sprintf("TaskRun %q failed to finish within %q", tr.Name, timeout.String()) tr.Status.SetCondition(&apis.Condition{ Type: apis.ConditionSucceeded, diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go index 7e4ea8da901..11165d1d746 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go @@ -1364,8 +1364,8 @@ func TestReconcilePodUpdateStatus(t *testing.T) { t.Fatalf("Unexpected error fetching taskrun: %v", err) } if d := cmp.Diff(newTr.Status.GetCondition(apis.ConditionSucceeded), &apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionTrue, + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, Reason: status.ReasonSucceeded, Message: "All Steps have completed executing", }, ignoreLastTransitionTime); d != "" { @@ -1514,6 +1514,24 @@ func TestReconcileTimeouts(t *testing.T) { Message: `TaskRun "test-taskrun-default-timeout-60-minutes" failed to finish within "1h0m0s"`, }, }, + { + taskRun: tb.TaskRun("test-taskrun-nil-timeout-default-60-minutes", "foo", + tb.TaskRunSpec( + tb.TaskRunTaskRef(simpleTask.Name), + tb.TaskRunNilTimeout, + ), + tb.TaskRunStatus(tb.Condition(apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionUnknown}), + tb.TaskRunStartTime(time.Now().Add(-61*time.Minute)))), + + expectedStatus: &apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + Reason: "TaskRunTimeout", + Message: `TaskRun "test-taskrun-nil-timeout-default-60-minutes" failed to finish within "1h0m0s"`, + }, + }, } for _, tc := range testcases { diff --git a/test/builder/pipeline.go b/test/builder/pipeline.go index 09758a1329f..8d667d335e2 100644 --- a/test/builder/pipeline.go +++ b/test/builder/pipeline.go @@ -333,13 +333,18 @@ func PipelineRunParam(name, value string) PipelineRunSpecOp { } } -// PipelineRunTimeout sets the timeout to the PipelineSpec. +// PipelineRunTimeout sets the timeout to the PipelineRunSpec. func PipelineRunTimeout(duration time.Duration) PipelineRunSpecOp { return func(prs *v1alpha1.PipelineRunSpec) { prs.Timeout = &metav1.Duration{Duration: duration} } } +// PipelineRunNilTimeout sets the timeout to nil on the PipelineRunSpec +func PipelineRunNilTimeout(prs *v1alpha1.PipelineRunSpec) { + prs.Timeout = nil +} + // PipelineRunNodeSelector sets the Node selector to the PipelineSpec. func PipelineRunNodeSelector(values map[string]string) PipelineRunSpecOp { return func(prs *v1alpha1.PipelineRunSpec) { diff --git a/test/builder/task.go b/test/builder/task.go index dd3bcb7d17e..c2e3b6e576a 100644 --- a/test/builder/task.go +++ b/test/builder/task.go @@ -349,6 +349,11 @@ func TaskRunTimeout(d time.Duration) TaskRunSpecOp { } } +// TaskRunNilTimeout sets the timeout duration to nil on the TaskRunSpec. +func TaskRunNilTimeout(spec *v1alpha1.TaskRunSpec) { + spec.Timeout = nil +} + // TaskRunNodeSelector sets the NodeSelector to the PipelineSpec. func TaskRunNodeSelector(values map[string]string) TaskRunSpecOp { return func(spec *v1alpha1.TaskRunSpec) {