diff --git a/config/config-defaults.yaml b/config/config-defaults.yaml index 661246818e6..7a9ea392527 100644 --- a/config/config-defaults.yaml +++ b/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 diff --git a/docs/pipelineruns.md b/docs/pipelineruns.md index a2de5dfc82f..ad4608cc9ed 100644 --- a/docs/pipelineruns.md +++ b/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: diff --git a/docs/taskruns.md b/docs/taskruns.md index e665be35bee..6b0f5dd4b72 100644 --- a/docs/taskruns.md +++ b/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 diff --git a/pkg/apis/config/default.go b/pkg/apis/config/default.go index ba591d4fee7..f9521ee4560 100644 --- a/pkg/apis/config/default.go +++ b/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" ) diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go index fba755b9c43..0a64d38b8e1 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go +++ b/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") } } diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go index 75fc7df53c4..5bf0d300563 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go +++ b/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) + } + }) } } diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go index 08cc6a2cbc1..755a281d67d 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go +++ b/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 } diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go index a77f5123fd8..4a7a3c5f830 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go +++ b/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 { diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go index 4e3460772e6..d27dedfd327 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" + 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 diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun.go b/pkg/reconciler/v1alpha1/taskrun/taskrun.go index 46ed3a0b540..ffd3c5ddf4a 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun.go +++ b/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 { diff --git a/pkg/reconciler/v1alpha1/taskrun/timeout_check.go b/pkg/reconciler/v1alpha1/taskrun/timeout_check.go new file mode 100644 index 00000000000..622b121db9c --- /dev/null +++ b/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 + } +} diff --git a/pkg/reconciler/v1alpha1/taskrun/timeout_check_test.go b/pkg/reconciler/v1alpha1/taskrun/timeout_check_test.go new file mode 100644 index 00000000000..55991e330a2 --- /dev/null +++ b/pkg/reconciler/v1alpha1/taskrun/timeout_check_test.go @@ -0,0 +1,67 @@ +/* +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 ( + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/knative/pkg/apis" + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + tb "github.com/tektoncd/pipeline/test/builder" +) + +func TestCheckTimeout(t *testing.T) { + // IsZero reports whether t represents the zero time instant, January 1, year 1, 00:00:00 UTC + zeroTime := time.Date(1, 1, 1, 0, 0, 0, 0, time.UTC) + testCases := []struct { + name string + taskRun *v1alpha1.TaskRun + expectedStatus bool + }{{ + name: "TaskRun not started", + taskRun: tb.TaskRun("test-taskrun-not-started", "foo", tb.TaskRunSpec( + tb.TaskRunTaskRef(simpleTask.Name), + ), tb.TaskRunStatus(tb.Condition(apis.Condition{ + }), tb.TaskRunStartTime(zeroTime))), + expectedStatus: false, + }, { + name: "TaskRun no timeout", + taskRun: tb.TaskRun("test-taskrun-no-timeout", "foo", tb.TaskRunSpec( + tb.TaskRunTaskRef(simpleTask.Name), tb.TaskRunTimeout(0), + ), tb.TaskRunStatus(tb.Condition(apis.Condition{ + }), tb.TaskRunStartTime(time.Now().Add(-15 * time.Hour)))), + expectedStatus: false, + }, { + name: "TaskRun timed out", + taskRun: tb.TaskRun("test-taskrun-timeout", "foo", tb.TaskRunSpec( + tb.TaskRunTaskRef(simpleTask.Name), tb.TaskRunTimeout(10 * time.Second), + ), tb.TaskRunStatus(tb.Condition(apis.Condition{ + }), tb.TaskRunStartTime(time.Now().Add(-15 * time.Second)))), + expectedStatus: true, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := CheckTimeout(tc.taskRun) + if d := cmp.Diff(result, tc.expectedStatus); d != "" { + t.Fatalf("-want, +got: %v", d) + } + }) + } +}