Implement 0 as no timeout for TaskRun and PipelineRun

If the timeout is set to 0, there is no timeout for the TaskRun and PipelineRun.
If the timeout is empty, the default timeout 60 mins will be applied.
If the timeout is set to -1, the taskrun is created in pipelinerun after
it timed out.

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 timouet 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

@@ -48,8 +48,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:
@@ -145,6 +147,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,48 @@ 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
+		want *apis.FieldError
+	}{
+		{
+			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"
@@ -439,22 +440,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{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

@@ -261,7 +261,7 @@ 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 {
+	if timedOut, err := CheckTimeout(tr, taskSpec, c.KubeClientSet.CoreV1().Pods(tr.Namespace).Delete); err != nil {
 		return err
 	} else if timedOut {
 		return nil
@@ -527,43 +527,6 @@ 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
-	}
-
-	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
-			}
-		}
-
-		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
-}
-
 func isExceededResourceQuotaError(err error) bool {
 	return err != nil && errors.IsForbidden(err) && strings.Contains(err.Error(), "exceeded quota")
 }

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

@@ -1477,6 +1477,50 @@ func TestReconcileOnCancelledTaskRun(t *testing.T) {
 	}
 }
 
+func TestReconcileNoTimeouts(t *testing.T) {
+	// We indicate this TaskRun was launched 15 hours ago. Since the timeout is set to 0,
+	// the TaskRun will never timeout.
+	taskRun := tb.TaskRun("test-taskrun-run-success", "foo",
+		tb.TaskRunSpec(tb.TaskRunTaskRef("test-task"), tb.TaskRunTimeout(10 * time.Minute)),
+		tb.TaskRunStatus(tb.Condition(apis.Condition{
+			Type:   apis.ConditionSucceeded,
+			Status: corev1.ConditionUnknown,}),
+			tb.TaskRunStartTime(time.Now().Add(-15 * time.Hour))))
+
+	pod, err := makePod(taskRun, simpleTask)
+	if err != nil {
+		t.Fatalf("MakePod: %v", err)
+	}
+	taskRun.Status = v1alpha1.TaskRunStatus{
+		PodName: pod.Name,
+	}
+	d := test.Data{
+		TaskRuns: []*v1alpha1.TaskRun{taskRun},
+		Tasks:    []*v1alpha1.Task{simpleTask},
+		Pods:     []*corev1.Pod{pod},
+	}
+
+	testAssets := getTaskRunController(t, d)
+	c := testAssets.Controller
+	clients := testAssets.Clients
+
+	if err := c.Reconciler.Reconcile(context.Background(), fmt.Sprintf("%s/%s", taskRun.Namespace, taskRun.Name)); err != nil {
+		t.Fatalf("Unexpected error when Reconcile() : %v", err)
+	}
+	newTr, err := clients.Pipeline.TektonV1alpha1().TaskRuns(taskRun.Namespace).Get(taskRun.Name, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Expected TaskRun %s to exist but instead got error when getting it: %v", taskRun.Name, err)
+	}
+	if d := cmp.Diff(newTr.Status.GetCondition(apis.ConditionSucceeded), &apis.Condition{
+		Type:    apis.ConditionSucceeded,
+		Status:  corev1.ConditionUnknown,
+		Message: "Running",
+		Reason:  "Running",
+	}, ignoreLastTransitionTime); d != "" {
+		t.Fatalf("-got, +want: %v", d)
+	}
+}
+
 func TestReconcileTimeouts(t *testing.T) {
 	type testCase struct {
 		taskRun        *v1alpha1.TaskRun