From ab66c3b326e3ea6838cf40299311ab5bd5a98fe6 Mon Sep 17 00:00:00 2001 From: Vincent Hou Date: Wed, 3 Jul 2019 14:55:50 -0400 Subject: [PATCH] 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 | 2 +- docs/pipelineruns.md | 6 +- docs/taskruns.md | 13 +++- pkg/apis/config/default.go | 2 + .../v1alpha1/pipelinerun_validation.go | 4 +- .../v1alpha1/pipelinerun_validation_test.go | 52 ++++++++++---- .../pipeline/v1alpha1/taskrun_validation.go | 7 ++ .../v1alpha1/taskrun_validation_test.go | 24 +++++++ .../v1alpha1/pipelinerun/pipelinerun.go | 13 ++-- pkg/reconciler/v1alpha1/taskrun/taskrun.go | 59 +++++++--------- .../v1alpha1/taskrun/timeout_check.go | 43 ++++++++++++ .../v1alpha1/taskrun/timeout_check_test.go | 67 +++++++++++++++++++ 12 files changed, 232 insertions(+), 60 deletions(-) create mode 100644 pkg/reconciler/v1alpha1/taskrun/timeout_check.go create mode 100644 pkg/reconciler/v1alpha1/taskrun/timeout_check_test.go 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) + } + }) + } +}