Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement timeout for custom tasks. #3976

Merged
merged 3 commits into from
Aug 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 52 additions & 9 deletions docs/runs.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,20 +139,63 @@ the `example.dev` API group, with the version `v1alpha1`.
#### Developer guide for custom controllers supporting `spec`.

1. A custom controller may or may not support a `Spec`. In cases where it is
not supported the custom controller should respond with proper validation error.

2. Validation of the fields of the custom task is delegated to the custom task controller. It is recommended to
implement validations as asynchronous
(i.e. at reconcile time), rather than part of the webhook. Using a webhook for validation is problematic because, it
is not possible to filter custom task resource objects before validation step, as a result each custom task resource
has to undergo validation by all the installed custom task controllers.

not supported the custom controller should respond with proper validation
error.

2. Validation of the fields of the custom task is delegated to the custom
task controller. It is recommended to implement validations as asynchronous
(i.e. at reconcile time), rather than part of the webhook. Using a webhook
for validation is problematic because, it is not possible to filter custom
task resource objects before validation step, as a result each custom task
resource has to undergo validation by all the installed custom task
controllers.

3. A custom task may have an empty spec, but cannot have an empty
`ApiVersion` and `Kind`. Custom task controllers should handle
`ApiVersion` and `Kind`. Custom task controllers should handle
an empty spec, either with a default behaviour, in a case no default
behaviour is supported then, appropriate validation error should be
updated to the `Run`'s status.

### Specifying `Timeout`

A custom task specification can be created with `Timeout` as follows:

```yaml
apiVersion: tekton.dev/v1alpha1
kind: Run
metadata:
generateName: simpleexample
spec:
timeout: 10s # set timeouts here.
params:
- name: searching
value: the purpose of my existence
ref:
apiVersion: custom.tekton.dev/v1alpha1
kind: Example
name: exampleName
```

Supporting timeouts is optional but recommended.

#### Developer guide for custom controllers supporting `Timeout`

1. Tekton controllers will never directly update the status of the
`Run`, it is the responsibility of the custom task controller to support
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I right that what we mean here is that the custom task developer is expected to update the status/condition of the Run? It might be worth describing a simple common status/condition/reason that is used for timeouts. I'm thinking something like:

  1. Once resources or timers are cleaned up it is good practice to set a
    condition on the Run's status of Succeeded/False with a Reason
    of RunTimedOut.

timeout. If timeouts are not supported, it's the responsibility of the custom
task controller to reject `Run`s that specify a timeout value.
2. On a `pipelineRun` or `pipelineTask` timeout, the status of the
`Run.Spec.Status` is updated to `RunCancelled`. It is up to the custom task
controller to respond to it. An existing controller, which does not yet
support timeout, will be able to cleanup, if it supports a cancel.
3. A Custom Task author can watch for this status update
(i.e. `Run.Spec.Status == RunCancelled`) and or `Run.HasTimedOut()` and take
any corresponding actions (i.e. a clean up e.g., cancel a cloud build, stop
the waiting timer, tear down the approval listener).
4. Once resources or timers are cleaned up it is good practice to set a
`conditions` on the `Run`'s `status` of `Succeeded/False` with a `Reason`
of `RunTimedOut`.

### Specifying `Parameters`

If a custom task supports [`parameters`](tasks.md#parameters), you can use the
Expand Down
35 changes: 33 additions & 2 deletions pkg/apis/pipeline/v1alpha1/run_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ package v1alpha1

import (
"fmt"
"time"

apisconfig "github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
runv1alpha1 "github.com/tektoncd/pipeline/pkg/apis/run/v1alpha1"
Expand Down Expand Up @@ -64,6 +66,11 @@ type RunSpec struct {
// +optional
PodTemplate *PodTemplate `json:"podTemplate,omitempty"`

// Time after which the custom-task times out.
// Refer Go's ParseDuration documentation for expected format: https://golang.org/pkg/time/#ParseDuration
// +optional
Timeout *metav1.Duration `json:"timeout,omitempty"`

// Workspaces is a list of WorkspaceBindings from volumes to workspaces.
// +optional
Workspaces []v1beta1.WorkspaceBinding `json:"workspaces,omitempty"`
Expand Down Expand Up @@ -95,6 +102,8 @@ func (rs RunSpec) GetParam(name string) *v1beta1.Param {
const (
// RunReasonCancelled must be used in the Condition Reason to indicate that a Run was cancelled.
RunReasonCancelled = "RunCancelled"
// RunReasonTimedOut must be used in the Condition Reason to indicate that a Run was timed out.
RunReasonTimedOut = "RunTimedOut"
// RunReasonWorkspaceNotSupported can be used in the Condition Reason to indicate that the
// Run contains a workspace which is not supported by this custom task.
RunReasonWorkspaceNotSupported = "RunWorkspaceNotSupported"
Expand Down Expand Up @@ -188,8 +197,30 @@ func (r *Run) IsSuccessful() bool {
return r.Status.GetCondition(apis.ConditionSucceeded).IsTrue()
}

// GetRunKey return the taskrun key for timeout handler map
// GetRunKey return the run's key for timeout handler map
func (r *Run) GetRunKey() string {
// The address of the pointer is a threadsafe unique identifier for the taskrun
// The address of the pointer is a threadsafe unique identifier for the run
return fmt.Sprintf("%s/%p", "Run", r)
}

// HasTimedOut returns true if the Run's running time is beyond the allowed timeout
func (r *Run) HasTimedOut() bool {
if r.Status.StartTime == nil || r.Status.StartTime.IsZero() {
return false
}
timeout := r.GetTimeout()
// If timeout is set to 0 or defaulted to 0, there is no timeout.
if timeout == apisconfig.NoTimeoutDuration {
return false
}
runtime := time.Since(r.Status.StartTime.Time)
return runtime > timeout
}

func (r *Run) GetTimeout() time.Duration {
// Use the platform default if no timeout is set
if r.Spec.Timeout == nil {
return apisconfig.DefaultTimeoutMinutes * time.Minute
}
return r.Spec.Timeout.Duration
}
96 changes: 96 additions & 0 deletions pkg/apis/pipeline/v1alpha1/run_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ import (
"time"

"github.com/google/go-cmp/cmp"
apisconfig "github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
v1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/client/clientset/versioned/scheme"
"github.com/tektoncd/pipeline/test/diff"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
runtime "k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -253,3 +255,97 @@ func TestEncodeDecodeExtraFields(t *testing.T) {
t.Fatalf("Diff(-want,+got): %s", d)
}
}

func TestRunGetTimeOut(t *testing.T) {
testCases := []struct {
name string
run v1alpha1.Run
expectedValue time.Duration
}{{
name: "runWithNoTimeout",
run: v1alpha1.Run{},
expectedValue: apisconfig.DefaultTimeoutMinutes * time.Minute,
}, {
name: "runWithTimeout",
run: v1alpha1.Run{
TypeMeta: metav1.TypeMeta{Kind: "kind", APIVersion: "apiVersion"},
Spec: v1alpha1.RunSpec{Timeout: &metav1.Duration{10 * time.Second}},
},
expectedValue: 10 * time.Second,
}}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
result := tc.run.GetTimeout()
if d := cmp.Diff(result, tc.expectedValue); d != "" {
t.Fatalf(diff.PrintWantGot(d))
}
})
}
}

func TestRunHasTimedOut(t *testing.T) {
testCases := []struct {
name string
run v1alpha1.Run
expectedValue bool
}{{
name: "runWithNoStartTimeNoTimeout",
run: v1alpha1.Run{},
expectedValue: false,
}, {
name: "runWithStartTimeNoTimeout",
run: v1alpha1.Run{
TypeMeta: metav1.TypeMeta{Kind: "kind", APIVersion: "apiVersion"},
Status: v1alpha1.RunStatus{
RunStatusFields: v1alpha1.RunStatusFields{
StartTime: &metav1.Time{Time: time.Now()},
},
}},
expectedValue: false,
}, {
name: "runWithStartTimeNoTimeout2",
run: v1alpha1.Run{
TypeMeta: metav1.TypeMeta{Kind: "kind", APIVersion: "apiVersion"},
Status: v1alpha1.RunStatus{
RunStatusFields: v1alpha1.RunStatusFields{
StartTime: &metav1.Time{Time: time.Now().Add(-1 * time.Hour)},
},
}},
expectedValue: true,
}, {
name: "runWithStartTimeAndTimeout",
run: v1alpha1.Run{
TypeMeta: metav1.TypeMeta{Kind: "kind", APIVersion: "apiVersion"},
Spec: v1alpha1.RunSpec{Timeout: &metav1.Duration{10 * time.Second}},
Status: v1alpha1.RunStatus{RunStatusFields: v1alpha1.RunStatusFields{
StartTime: &metav1.Time{Time: time.Now().Add(-1 * time.Hour)},
}}},
expectedValue: true,
}, {
name: "runWithNoStartTimeAndTimeout",
run: v1alpha1.Run{
TypeMeta: metav1.TypeMeta{Kind: "kind", APIVersion: "apiVersion"},
Spec: v1alpha1.RunSpec{Timeout: &metav1.Duration{1 * time.Second}},
},
expectedValue: false,
}, {
name: "runWithStartTimeAndTimeout2",
run: v1alpha1.Run{
TypeMeta: metav1.TypeMeta{Kind: "kind", APIVersion: "apiVersion"},
Spec: v1alpha1.RunSpec{Timeout: &metav1.Duration{10 * time.Second}},
Status: v1alpha1.RunStatus{RunStatusFields: v1alpha1.RunStatusFields{
StartTime: &metav1.Time{Time: time.Now()},
}}},
expectedValue: false,
}}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
result := tc.run.HasTimedOut()
if d := cmp.Diff(result, tc.expectedValue); d != "" {
t.Fatalf(diff.PrintWantGot(d))
}
})
}
}
5 changes: 5 additions & 0 deletions pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions pkg/apis/pipeline/v1beta1/pipeline_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,6 @@ func (pt PipelineTask) validateCustomTask() (errs *apis.FieldError) {
if pt.Resources != nil {
errs = errs.Also(apis.ErrInvalidValue("custom tasks do not support PipelineResources", "resources"))
}
if pt.Timeout != nil {
errs = errs.Also(apis.ErrInvalidValue("custom tasks do not support timeout", "timeout"))
}
return errs
}

Expand Down
13 changes: 0 additions & 13 deletions pkg/apis/pipeline/v1beta1/pipeline_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,10 @@ package v1beta1
import (
"context"
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/tektoncd/pipeline/test/diff"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/sets"
"knative.dev/pkg/apis"
Expand Down Expand Up @@ -206,17 +204,6 @@ func TestPipelineTask_ValidateCustomTask(t *testing.T) {
Message: `invalid value: custom tasks do not support PipelineResources`,
Paths: []string{"resources"},
},
}, {
name: "custom task doesn't support timeout",
task: PipelineTask{
Name: "foo",
Timeout: &metav1.Duration{Duration: time.Duration(3)},
TaskRef: &TaskRef{APIVersion: "example.dev/v0", Kind: "Example"},
},
expectedError: apis.FieldError{
Message: `invalid value: custom tasks do not support timeout`,
Paths: []string{"timeout"},
},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
7 changes: 6 additions & 1 deletion pkg/reconciler/pipelinerun/cancel.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ func init() {
}
}

func cancelRun(ctx context.Context, runName string, namespace string, clientSet clientset.Interface) error {
_, err := clientSet.TektonV1alpha1().Runs(namespace).Patch(ctx, runName, types.JSONPatchType, cancelRunPatchBytes, metav1.PatchOptions{}, "")
return err
}

// cancelPipelineRun marks the PipelineRun as cancelled and any resolved TaskRun(s) too.
func cancelPipelineRun(ctx context.Context, logger *zap.SugaredLogger, pr *v1beta1.PipelineRun, clientSet clientset.Interface) error {
errs := cancelPipelineTaskRuns(ctx, logger, pr, clientSet)
Expand Down Expand Up @@ -108,7 +113,7 @@ func cancelPipelineTaskRuns(ctx context.Context, logger *zap.SugaredLogger, pr *
for runName := range pr.Status.Runs {
logger.Infof("cancelling Run %s", runName)

if _, err := clientSet.TektonV1alpha1().Runs(pr.Namespace).Patch(ctx, runName, types.JSONPatchType, cancelRunPatchBytes, metav1.PatchOptions{}, ""); err != nil {
if err := cancelRun(ctx, runName, pr.Namespace, clientSet); err != nil {
errs = append(errs, fmt.Errorf("Failed to patch Run `%s` with cancellation: %s", runName, err).Error())
continue
}
Expand Down
Loading