Implement 0 as no timeout for TaskRun and PipelineRun

config/config-defaults.yaml

@@ -35,5 +35,5 @@ data:
     # to actually change the configuration.
 
     # default-timeout-minutes contains the default number of
-    # minutes to use for TaskRun, if none is specified.
+    # minutes to use for TaskRun and PipelineRun, if none is specified.
     default-timeout-minutes: "60"  # 60 minutes

docs/pipelineruns.md

@@ -41,7 +41,11 @@ following fields:
     information.
   - [`serviceAccounts`](#service-accounts) - Specifies a list of `ServiceAccount` 
     and `PipelineTask` pairs that enable you to overwrite `ServiceAccount` for concrete `PipelineTask`.
-  - `timeout` - Specifies timeout after which the `PipelineRun` will fail.
+  - [`timeout`] - Specifies timeout after which the `PipelineRun` will fail. If the value of
+    `timeout` is empty, the default timeout will be applied. If the value is set to 0,
+    there is no timeout. `PipelineRun` shares the same default timeout as `TaskRun`. You can
+    follow the instruction [here](taskruns.md#Configuring-default-timeout) to configure the
+    default timeout, the same way as `TaskRun`.
   - [`nodeSelector`] - A selector which must be true for the pod to fit on a
     node. The selector which must match a node's labels for the pod to be
     scheduled on that node. More info:

docs/taskruns.md

@@ -49,8 +49,10 @@ following fields:
   - [`inputs`] - Specifies [input parameters](#input-parameters) and
     [input resources](#providing-resources)
   - [`outputs`] - Specifies [output resources](#providing-resources)
-  - `timeout` - Specifies timeout after which the `TaskRun` will fail. Defaults
-    to 60 minutes.
+  - [`timeout`] - Specifies timeout after which the `TaskRun` will fail. If the value of
+    `timeout` is empty, the default timeout will be applied. If the value is set to 0,
+    there is no timeout. You can also follow the instruction [here](#Configuring-default-timeout)
+    to configure the default timeout.
   - [`nodeSelector`] - a selector which must be true for the pod to fit on a
     node. The selector which must match a node's labels for the pod to be
     scheduled on that node. More info:
@@ -146,6 +148,13 @@ spec:
               value: https://github.com/pivotal-nader-ziada/gohelloworld
 ```
 
+### Configuring Default Timeout
+
+You can configure the default timeout by changing the value of `default-timeout-minutes`
+in [`config/config-defaults.yaml`](./../config/config-defaults.yaml). The default timeout
+is 60 minutes, if `default-timeout-minutes` is not available. There is no timeout by
+default, if `default-timeout-minutes` is set to 0.
+
 ### Service Account
 
 Specifies the `name` of a `ServiceAccount` resource object. Use the

pkg/apis/config/default.go

@@ -19,6 +19,7 @@ package config
 import (
 	"fmt"
 	"strconv"
+	"time"
 
 	corev1 "k8s.io/api/core/v1"
 )
@@ -27,6 +28,7 @@ const (
 	// ConfigName is the name of the configmap
 	DefaultsConfigName       = "config-defaults"
 	DefaultTimeoutMinutes    = 60
+	NoTimeoutDuration        = 0 * time.Minute
 	defaultTimeoutMinutesKey = "default-timeout-minutes"
 )
 

pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go

@@ -51,8 +51,8 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) *apis.FieldError {
 
 	if ps.Timeout != nil {
 		// timeout should be a valid duration of at least 0.
-		if ps.Timeout.Duration <= 0 {
-			return apis.ErrInvalidValue(fmt.Sprintf("%s should be > 0", ps.Timeout.Duration.String()), "spec.timeout")
+		if ps.Timeout.Duration < 0 {
+			return apis.ErrInvalidValue(fmt.Sprintf("%s should be >= 0", ps.Timeout.Duration.String()), "spec.timeout")
 		}
 	}
 

pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go

@@ -77,7 +77,7 @@ func TestPipelineRun_Invalidate(t *testing.T) {
 					Timeout: &metav1.Duration{Duration: -48 * time.Hour},
 				},
 			},
-			want: apis.ErrInvalidValue("-48h0m0s should be > 0", "spec.timeout"),
+			want: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeout"),
 		},
 	}
 
@@ -92,21 +92,47 @@ func TestPipelineRun_Invalidate(t *testing.T) {
 }
 
 func TestPipelineRun_Validate(t *testing.T) {
-	tr := v1alpha1.PipelineRun{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "pipelinelineName",
-		},
-		Spec: v1alpha1.PipelineRunSpec{
-			PipelineRef: v1alpha1.PipelineRef{
-				Name: "prname",
+	tests := []struct {
+		name string
+		pr   v1alpha1.PipelineRun
+	}{
+		{
+			name: "normal case",
+			pr: v1alpha1.PipelineRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "pipelinelineName",
+				},
+				Spec: v1alpha1.PipelineRunSpec{
+					PipelineRef: v1alpha1.PipelineRef{
+						Name: "prname",
+					},
+					Results: &v1alpha1.Results{
+						URL:  "http://www.google.com",
+						Type: "gcs",
+					},
+				},
 			},
-			Results: &v1alpha1.Results{
-				URL:  "http://www.google.com",
-				Type: "gcs",
+		}, {
+			name: "no timeout",
+			pr: v1alpha1.PipelineRun{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "pipelinelineName",
+				},
+				Spec: v1alpha1.PipelineRunSpec{
+					PipelineRef: v1alpha1.PipelineRef{
+						Name: "prname",
+					},
+					Timeout: &metav1.Duration{Duration: 0},
+				},
 			},
 		},
 	}
-	if err := tr.Validate(context.Background()); err != nil {
-		t.Errorf("Unexpected PipelineRun.Validate() error = %v", err)
+
+	for _, ts := range tests {
+		t.Run(ts.name, func(t *testing.T) {
+			if err := ts.pr.Validate(context.Background()); err != nil {
+				t.Errorf("Unexpected PipelineRun.Validate() error = %v", err)
+			}
+		})
 	}
 }

pkg/apis/pipeline/v1alpha1/taskrun_validation.go

@@ -66,6 +66,13 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) *apis.FieldError {
 		}
 	}
 
+	if ts.Timeout != nil {
+		// timeout should be a valid duration of at least 0.
+		if ts.Timeout.Duration < 0 {
+			return apis.ErrInvalidValue(fmt.Sprintf("%s should be >= 0", ts.Timeout.Duration.String()), "spec.timeout")
+		}
+	}
+
 	return nil
 }
 

pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go

@@ -19,11 +19,13 @@ package v1alpha1_test
 import (
 	"context"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/knative/pkg/apis"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	tb "github.com/tektoncd/pipeline/test/builder"
 )
@@ -101,6 +103,16 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
 			},
 			wantErr: apis.ErrDisallowedFields("spec.taskspec", "spec.taskref"),
 		},
+		{
+			name: "negative pipeline timeout",
+			spec: v1alpha1.TaskRunSpec{
+				TaskRef: &v1alpha1.TaskRef{
+					Name: "taskrefname",
+				},
+				Timeout: &metav1.Duration{Duration: -48 * time.Hour},
+			},
+			wantErr: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeout"),
+		},
 	}
 
 	for _, ts := range tests {
@@ -129,6 +141,18 @@ func TestTaskRunSpec_Validate(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "no timeout",
+			spec: v1alpha1.TaskRunSpec{
+				Timeout: &metav1.Duration{Duration: 0},
+				TaskSpec: &v1alpha1.TaskSpec{
+					Steps: []corev1.Container{{
+						Name:  "mystep",
+						Image: "myimage",
+					}},
+				},
+			},
+		},
 	}
 
 	for _, ts := range tests {

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"
+	apisconfig "github.com/tektoncd/pipeline/pkg/apis/config"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	artifacts "github.com/tektoncd/pipeline/pkg/artifacts"
@@ -394,22 +395,22 @@ 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: 0 * time.Second}
+	var taskRunTimeout = &metav1.Duration{Duration: apisconfig.NoTimeoutDuration}
 
-	if pr.Spec.Timeout != nil {
+	// 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 time.Now().After(pTimeoutTime) {
 			// Just in case something goes awry and we're creating the TaskRun after it should have already timed out,
-			// set a timeout of 0.
+			// set the timeout to 1 second.
 			taskRunTimeout = &metav1.Duration{Duration: time.Until(pTimeoutTime)}
 			if taskRunTimeout.Duration < 0 {
-				taskRunTimeout = &metav1.Duration{Duration: 0 * time.Second}
+				taskRunTimeout = &metav1.Duration{Duration: 1 * time.Second}
 			}
 		} else {
 			taskRunTimeout = pr.Spec.Timeout
 		}
-	} else {
-		taskRunTimeout = nil
 	}
 
 	// If service account is configured for a given PipelineTask, override PipelineRun's seviceAccount

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -215,9 +215,10 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error
 
 	// Check if the TaskRun has timed out; if it is, this will set its status
 	// accordingly.
-	if timedOut, err := c.checkTimeout(tr, taskSpec, c.KubeClientSet.CoreV1().Pods(tr.Namespace).Delete); err != nil {
-		return err
-	} else if timedOut {
+	if CheckTimeout(tr) {
+		if err := c.updateTaskRunStatusForTimeout(tr, c.KubeClientSet.CoreV1().Pods(tr.Namespace).Delete); err != nil {
+			return err
+		}
 		return nil
 	}
 
@@ -481,41 +482,29 @@ func createRedirectedTaskSpec(kubeclient kubernetes.Interface, ts *v1alpha1.Task
 
 type DeletePod func(podName string, options *metav1.DeleteOptions) error
 
-func (c *Reconciler) checkTimeout(tr *v1alpha1.TaskRun, ts *v1alpha1.TaskSpec, dp DeletePod) (bool, error) {
-	// If tr has not started, startTime should be zero.
-	if tr.Status.StartTime.IsZero() {
-		return false, nil
-	}
-
+func (c *Reconciler) updateTaskRunStatusForTimeout(tr *v1alpha1.TaskRun, dp DeletePod) error {
 	timeout := tr.Spec.Timeout.Duration
-	runtime := time.Since(tr.Status.StartTime.Time)
-	c.Logger.Infof("Checking timeout for TaskRun %q (startTime %s, timeout %s, runtime %s)", tr.Name, tr.Status.StartTime, timeout, runtime)
-
-	if runtime > timeout {
-		c.Logger.Infof("TaskRun %q is timeout (runtime %s over %s), deleting pod", tr.Name, runtime, timeout)
-		// 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
-		// a namespace's ResourceQuota.
-		if tr.Status.PodName != "" {
-			if err := dp(tr.Status.PodName, &metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) {
-				c.Logger.Errorf("Failed to terminate pod: %v", err)
-				return true, err
-			}
+	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
+	// a namespace's ResourceQuota.
+	if tr.Status.PodName != "" {
+		if err := dp(tr.Status.PodName, &metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) {
+			c.Logger.Errorf("Failed to terminate pod: %v", err)
+			return err
 		}
-
-		timeoutMsg := fmt.Sprintf("TaskRun %q failed to finish within %q", tr.Name, timeout.String())
-		tr.Status.SetCondition(&apis.Condition{
-			Type:    apis.ConditionSucceeded,
-			Status:  corev1.ConditionFalse,
-			Reason:  status.ReasonTimedOut,
-			Message: timeoutMsg,
-		})
-		// update tr completed time
-		tr.Status.CompletionTime = &metav1.Time{Time: time.Now()}
-
-		return true, nil
 	}
-	return false, nil
+
+	timeoutMsg := fmt.Sprintf("TaskRun %q failed to finish within %q", tr.Name, timeout.String())
+	tr.Status.SetCondition(&apis.Condition{
+		Type:    apis.ConditionSucceeded,
+		Status:  corev1.ConditionFalse,
+		Reason:  status.ReasonTimedOut,
+		Message: timeoutMsg,
+	})
+	// update tr completed time
+	tr.Status.CompletionTime = &metav1.Time{Time: time.Now()}
+	return nil
 }
 
 func isExceededResourceQuotaError(err error) bool {

pkg/reconciler/v1alpha1/taskrun/timeout_check.go

@@ -0,0 +1,43 @@
+/*
+Copyright 2019 The Tekton Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package taskrun
+
+import (
+	"time"
+
+	apisconfig "github.com/tektoncd/pipeline/pkg/apis/config"
+	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
+)
+
+func CheckTimeout(tr *v1alpha1.TaskRun) bool {
+	// If tr has not started, startTime should be zero.
+	if tr.Status.StartTime.IsZero() {
+		return false
+	}
+
+	timeout := tr.Spec.Timeout.Duration
+	// If timeout is set to 0 or defaulted to 0, there is no timeout.
+	if timeout == apisconfig.NoTimeoutDuration {
+		return false
+	}
+	runtime := time.Since(tr.Status.StartTime.Time)
+	if runtime > timeout {
+		return true
+	} else {
+		return false
+	}
+}