From 4fc27b2afa4a90fe20fd0e4487aa97bb061b5760 Mon Sep 17 00:00:00 2001 From: Soule BA Date: Wed, 28 Apr 2021 23:51:32 +0200 Subject: [PATCH] Puts the new timeouts field under feature gates --- .../pipelineruns/no-ci/pipeline-timeout.yaml | 10 +- go.sum | 2 - internal/builder/v1beta1/pipeline.go | 18 +- .../pipeline/v1beta1/openapi_generated.go | 41 +- .../pipeline/v1beta1/pipelinerun_types.go | 10 +- .../v1beta1/pipelinerun_validation.go | 39 +- .../v1beta1/pipelinerun_validation_test.go | 376 ++++++++++-------- pkg/apis/pipeline/v1beta1/swagger.json | 21 + pkg/reconciler/pipelinerun/pipelinerun.go | 18 +- test/timeout_test.go | 2 +- 10 files changed, 333 insertions(+), 204 deletions(-) diff --git a/examples/v1beta1/pipelineruns/no-ci/pipeline-timeout.yaml b/examples/v1beta1/pipelineruns/no-ci/pipeline-timeout.yaml index 0a48f5664b0..c3fdbba710f 100644 --- a/examples/v1beta1/pipelineruns/no-ci/pipeline-timeout.yaml +++ b/examples/v1beta1/pipelineruns/no-ci/pipeline-timeout.yaml @@ -14,8 +14,8 @@ spec: - sleep 90s args: - "$(inputs.params.MESSAGE)" ---- +--- apiVersion: tekton.dev/v1beta1 kind: PipelineRun metadata: @@ -54,8 +54,7 @@ spec: - name: NIGHT_GREETINGS value: "Good Night, Bob!" - --- - +--- apiVersion: tekton.dev/v1beta1 kind: PipelineRun metadata: @@ -64,7 +63,10 @@ spec: # 1 hour and half timeout for the pipeline # 1 hour and fifteen minutes for the pipeline tasks # 15 minutes for the finally tasks - timeouts: + # + # This field requires enable-api-fields: "alpha" flag + # Check https://github.com/tektoncd/pipeline/blob/main/config/config-feature-flags.yaml + timeouts: pipeline: 1h30m tasks: 1h15m pipelineSpec: diff --git a/go.sum b/go.sum index d4b664e820c..c3689892bb0 100644 --- a/go.sum +++ b/go.sum @@ -720,8 +720,6 @@ github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81P github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA= github.com/stretchr/testify v1.6.1 h1:hDPOHmpOpP40lSULcqw7IrRb/u7w6RpDC9399XyoNd0= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= -github.com/tektoncd/plumbing v0.0.0-20201021153918-6b7e894737b5 h1:Y2Gd3X79zqvCd6AdiWyi/pnSewSkLxKygpvXNFXwscg= -github.com/tektoncd/plumbing v0.0.0-20201021153918-6b7e894737b5/go.mod h1:WTWwsg91xgm+jPOKoyKVK/yRYxnVDlUYeDlypB1lDdQ= github.com/tektoncd/plumbing v0.0.0-20210420200944-17170d5e7bc9 h1:ZLPo8/vilaxvpdvvdd9ZgIhhQJPkHyS5GeKK8UH4/Yo= github.com/tektoncd/plumbing v0.0.0-20210420200944-17170d5e7bc9/go.mod h1:WTWwsg91xgm+jPOKoyKVK/yRYxnVDlUYeDlypB1lDdQ= github.com/tidwall/pretty v1.0.0/go.mod h1:XNkn88O1ChpSDQmQeStsy+sBenx6DDtFZJxhVysOjyk= diff --git a/internal/builder/v1beta1/pipeline.go b/internal/builder/v1beta1/pipeline.go index ea1eb69a9f5..a70f31b532a 100644 --- a/internal/builder/v1beta1/pipeline.go +++ b/internal/builder/v1beta1/pipeline.go @@ -529,12 +529,16 @@ func PipelineRunNilTimeout(prs *v1beta1.PipelineRunSpec) { prs.Timeout = nil } +func initTimeouts(prs *v1beta1.PipelineRunSpec) { + if prs.Timeouts == nil { + prs.Timeouts = &v1beta1.TimeoutFields{} + } +} + // PipelineRunTasksTimeout sets the timeout to the PipelineRunSpec. func PipelineRunTasksTimeout(duration time.Duration) PipelineRunSpecOp { return func(prs *v1beta1.PipelineRunSpec) { - if prs.Timeouts == nil { - prs.Timeouts = &v1beta1.TimeoutFields{} - } + initTimeouts(prs) prs.Timeouts.Tasks = &metav1.Duration{Duration: duration} } } @@ -542,9 +546,7 @@ func PipelineRunTasksTimeout(duration time.Duration) PipelineRunSpecOp { // PipelineRunFinallyTimeout sets the timeout to the PipelineRunSpec. func PipelineRunFinallyTimeout(duration time.Duration) PipelineRunSpecOp { return func(prs *v1beta1.PipelineRunSpec) { - if prs.Timeouts == nil { - prs.Timeouts = &v1beta1.TimeoutFields{} - } + initTimeouts(prs) prs.Timeouts.Finally = &metav1.Duration{Duration: duration} } } @@ -552,9 +554,7 @@ func PipelineRunFinallyTimeout(duration time.Duration) PipelineRunSpecOp { // PipelineRunPipelineTimeout sets the timeout to the PipelineRunSpec. func PipelineRunPipelineTimeout(duration time.Duration) PipelineRunSpecOp { return func(prs *v1beta1.PipelineRunSpec) { - if prs.Timeouts == nil { - prs.Timeouts = &v1beta1.TimeoutFields{} - } + initTimeouts(prs) prs.Timeouts.Pipeline = &metav1.Duration{Duration: duration} } } diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index 134d8e3b6c2..e422c22bd8c 100644 --- a/pkg/apis/pipeline/v1beta1/openapi_generated.go +++ b/pkg/apis/pipeline/v1beta1/openapi_generated.go @@ -95,6 +95,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskRunStatus": schema_pkg_apis_pipeline_v1beta1_TaskRunStatus(ref), "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskRunStatusFields": schema_pkg_apis_pipeline_v1beta1_TaskRunStatusFields(ref), "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TaskSpec": schema_pkg_apis_pipeline_v1beta1_TaskSpec(ref), + "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TimeoutFields": schema_pkg_apis_pipeline_v1beta1_TimeoutFields(ref), "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.WhenExpression": schema_pkg_apis_pipeline_v1beta1_WhenExpression(ref), "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.WorkspaceBinding": schema_pkg_apis_pipeline_v1beta1_WorkspaceBinding(ref), "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.WorkspaceDeclaration": schema_pkg_apis_pipeline_v1beta1_WorkspaceDeclaration(ref), @@ -1508,6 +1509,12 @@ func schema_pkg_apis_pipeline_v1beta1_PipelineRunSpec(ref common.ReferenceCallba Format: "", }, }, + "timeouts": { + SchemaProps: spec.SchemaProps{ + Description: "This is an alpha field. You must set the \"enable-api-fields\" feature flag to \"alpha\" for this field to be supported.\n\nTime after which the Pipeline times out. Currently three keys are accepted in the map pipeline, tasks and finally with Timeouts.pipeline >= Timeouts.tasks + Timeouts.finally", + Ref: ref("github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TimeoutFields"), + }, + }, "timeout": { SchemaProps: spec.SchemaProps{ Description: "Time after which the Pipeline times out. Defaults to never. Refer to Go's ParseDuration documentation for expected format: https://golang.org/pkg/time/#ParseDuration", @@ -1552,7 +1559,7 @@ func schema_pkg_apis_pipeline_v1beta1_PipelineRunSpec(ref common.ReferenceCallba }, }, Dependencies: []string{ - "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod.Template", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.Param", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.PipelineRef", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.PipelineResourceBinding", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.PipelineRunSpecServiceAccountName", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.PipelineSpec", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.PipelineTaskRunSpec", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.WorkspaceBinding", "k8s.io/apimachinery/pkg/apis/meta/v1.Duration"}, + "github.com/tektoncd/pipeline/pkg/apis/pipeline/pod.Template", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.Param", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.PipelineRef", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.PipelineResourceBinding", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.PipelineRunSpecServiceAccountName", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.PipelineSpec", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.PipelineTaskRunSpec", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.TimeoutFields", "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1.WorkspaceBinding", "k8s.io/apimachinery/pkg/apis/meta/v1.Duration"}, } } @@ -4163,6 +4170,38 @@ func schema_pkg_apis_pipeline_v1beta1_TaskSpec(ref common.ReferenceCallback) com } } +func schema_pkg_apis_pipeline_v1beta1_TimeoutFields(ref common.ReferenceCallback) common.OpenAPIDefinition { + return common.OpenAPIDefinition{ + Schema: spec.Schema{ + SchemaProps: spec.SchemaProps{ + Type: []string{"object"}, + Properties: map[string]spec.Schema{ + "pipeline": { + SchemaProps: spec.SchemaProps{ + Description: "Pipeline sets the maximum allowed duration for execution of the entire pipeline. The sum of individual timeouts for tasks and finally must not exceed this value.", + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1.Duration"), + }, + }, + "tasks": { + SchemaProps: spec.SchemaProps{ + Description: "Tasks sets the maximum allowed duration of this pipeline's tasks", + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1.Duration"), + }, + }, + "finally": { + SchemaProps: spec.SchemaProps{ + Description: "Finally sets the maximum allowed duration of this pipeline's finally", + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1.Duration"), + }, + }, + }, + }, + }, + Dependencies: []string{ + "k8s.io/apimachinery/pkg/apis/meta/v1.Duration"}, + } +} + func schema_pkg_apis_pipeline_v1beta1_WhenExpression(ref common.ReferenceCallback) common.OpenAPIDefinition { return common.OpenAPIDefinition{ Schema: spec.Schema{ diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go index 2f14774072d..52a46401240 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go @@ -179,6 +179,9 @@ type PipelineRunSpec struct { // Used for cancelling a pipelinerun (and maybe more later on) // +optional Status PipelineRunSpecStatus `json:"status,omitempty"` + // This is an alpha field. You must set the "enable-api-fields" feature flag to "alpha" + // for this field to be supported. + // // Time after which the Pipeline times out. // Currently three keys are accepted in the map // pipeline, tasks and finally @@ -201,9 +204,12 @@ type PipelineRunSpec struct { } type TimeoutFields struct { + // Pipeline sets the maximum allowed duration for execution of the entire pipeline. The sum of individual timeouts for tasks and finally must not exceed this value. Pipeline *metav1.Duration `json:"pipeline,omitempty"` - Tasks *metav1.Duration `json:"tasks,omitempty"` - Finally *metav1.Duration `json:"finally,omitempty"` + // Tasks sets the maximum allowed duration of this pipeline's tasks + Tasks *metav1.Duration `json:"tasks,omitempty"` + // Finally sets the maximum allowed duration of this pipeline's finally + Finally *metav1.Duration `json:"finally,omitempty"` } // PipelineRunSpecStatus defines the pipelinerun spec status the user can provide diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go index 22305272a78..5535209317c 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go @@ -24,6 +24,7 @@ import ( "github.com/google/go-containerregistry/pkg/name" "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/validate" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/pkg/apis" ) @@ -83,34 +84,30 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) } } + // This is an alpha feature and will fail validation if it's used in a pipelinerun spec + // when the enable-api-fields feature gate is anything but "alpha". if ps.Timeouts != nil { if ps.Timeout != nil { // can't have both at the same time errs = errs.Also(apis.ErrDisallowedFields("timeout", "timeouts")) } + errs = errs.Also(ValidateEnabledAPIFields(ctx, "timeouts", config.AlphaAPIFields)) + // tasks timeout should be a valid duration of at least 0. - if ps.Timeouts.Tasks != nil && ps.Timeouts.Tasks.Duration < 0 { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s should be >= 0", ps.Timeouts.Tasks.Duration.String()), "timeouts.tasks")) - } + errs = errs.Also(validateTimeoutDuration("tasks", ps.Timeouts.Tasks)) // finally timeout should be a valid duration of at least 0. - if ps.Timeouts.Finally != nil && ps.Timeouts.Finally.Duration < 0 { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s should be >= 0", ps.Timeouts.Finally.Duration.String()), "timeouts.finally")) - } + errs = errs.Also(validateTimeoutDuration("finally", ps.Timeouts.Finally)) - if ps.Timeouts.Pipeline != nil { - // pipeline timeout should be a valid duration of at least 0. - if ps.Timeouts.Pipeline.Duration < 0 { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s should be >= 0", ps.Timeouts.Pipeline.Duration.String()), "timeouts.pipeline")) - } + // pipeline timeout should be a valid duration of at least 0. + errs = errs.Also(validateTimeoutDuration("pipeline", ps.Timeouts.Pipeline)) + if ps.Timeouts.Pipeline != nil { errs = errs.Also(ps.validatePipelineTimeout(ps.Timeouts.Pipeline.Duration, "should be <= pipeline duration")) - } else { defaultTimeout := time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) errs = errs.Also(ps.validatePipelineTimeout(defaultTimeout, "should be <= default timeout duration")) - } } @@ -134,19 +131,27 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) return errs } +func validateTimeoutDuration(field string, d *metav1.Duration) (errs *apis.FieldError) { + if d != nil && d.Duration < 0 { + fieldPath := fmt.Sprintf("timeouts.%s", field) + return errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s should be >= 0", d.Duration.String()), fieldPath)) + } + return nil +} + func (ps *PipelineRunSpec) validatePipelineTimeout(timeout time.Duration, errorMsg string) (errs *apis.FieldError) { if ps.Timeouts.Tasks != nil && ps.Timeouts.Tasks.Duration > timeout { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s "+errorMsg, ps.Timeouts.Tasks.Duration.String()), "timeouts.tasks")) + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s %s", ps.Timeouts.Tasks.Duration.String(), errorMsg), "timeouts.tasks")) } if ps.Timeouts.Finally != nil && ps.Timeouts.Finally.Duration > timeout { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s "+errorMsg, ps.Timeouts.Finally.Duration.String()), "timeouts.finally")) + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s %s", ps.Timeouts.Finally.Duration.String(), errorMsg), "timeouts.finally")) } if ps.Timeouts.Tasks != nil && ps.Timeouts.Finally != nil { if ps.Timeouts.Tasks.Duration+ps.Timeouts.Finally.Duration > timeout { - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s + %s "+errorMsg, ps.Timeouts.Tasks.Duration.String(), ps.Timeouts.Finally.Duration.String()), "timeouts.tasks")) - errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s + %s "+errorMsg, ps.Timeouts.Tasks.Duration.String(), ps.Timeouts.Finally.Duration.String()), "timeouts.finally")) + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s + %s %s", ps.Timeouts.Tasks.Duration.String(), ps.Timeouts.Finally.Duration.String(), errorMsg), "timeouts.tasks")) + errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s + %s %s", ps.Timeouts.Tasks.Duration.String(), ps.Timeouts.Finally.Duration.String(), errorMsg), "timeouts.finally")) } } return errs diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go index 345858875f1..310b03501ed 100644 --- a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go @@ -18,6 +18,7 @@ package v1beta1_test import ( "context" + "fmt" "testing" "time" @@ -59,7 +60,6 @@ func TestPipelineRun_Invalid(t *testing.T) { Name: "prname", }, }, -<<<<<<< HEAD }, want: &apis.FieldError{ Message: `invalid resource name "pipelinerun,name": must be a valid DNS label`, @@ -70,121 +70,6 @@ func TestPipelineRun_Invalid(t *testing.T) { pr: v1beta1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ Name: "pipelinelinename", -======= - want: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeout"), - }, { - name: "negative pipeline Timeout", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinelineName", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "prname", - }, - Timeouts: &v1beta1.TimeoutFields{ - Pipeline: &metav1.Duration{Duration: -48 * time.Hour}, - }, - }, - }, - want: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeouts.pipeline"), - }, { - name: "negative pipeline tasks Timeout", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinelineName", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "prname", - }, - Timeouts: &v1beta1.TimeoutFields{ - Tasks: &metav1.Duration{Duration: -48 * time.Hour}, - }, - }, - }, - want: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeouts.tasks"), - }, { - name: "negative pipeline finally Timeout", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinelineName", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "prname", - }, - Timeouts: &v1beta1.TimeoutFields{ - Finally: &metav1.Duration{Duration: -48 * time.Hour}, - }, - }, - }, - want: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeouts.finally"), - }, { - name: "pipeline tasks Timeout > pipeline Timeout", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinelineName", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "prname", - }, - Timeouts: &v1beta1.TimeoutFields{ - Pipeline: &metav1.Duration{Duration: 25 * time.Minute}, - Tasks: &metav1.Duration{Duration: 1 * time.Hour}, - }, - }, - }, - want: apis.ErrInvalidValue("1h0m0s should be <= pipeline duration", "spec.timeouts.tasks"), - }, { - name: "pipeline finally Timeout > pipeline Timeout", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinelineName", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "prname", - }, - Timeouts: &v1beta1.TimeoutFields{ - Pipeline: &metav1.Duration{Duration: 25 * time.Minute}, - Finally: &metav1.Duration{Duration: 1 * time.Hour}, - }, - }, - }, - want: apis.ErrInvalidValue("1h0m0s should be <= pipeline duration", "spec.timeouts.finally"), - }, { - name: "pipeline tasks Timeout + pipeline finally Timeout > pipeline Timeout", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinelineName", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "prname", - }, - Timeouts: &v1beta1.TimeoutFields{ - Pipeline: &metav1.Duration{Duration: 50 * time.Minute}, - Tasks: &metav1.Duration{Duration: 30 * time.Minute}, - Finally: &metav1.Duration{Duration: 30 * time.Minute}, - }, - }, - }, - want: apis.ErrInvalidValue("30m0s + 30m0s should be <= pipeline duration", "spec.timeouts.finally, spec.timeouts.tasks"), - }, { - name: "wrong pipelinerun cancel", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinelineName", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "prname", - }, - Status: "PipelineRunCancell", - }, ->>>>>>> Add a TaskTimeout optional field to pipelinerun type }, Spec: v1beta1.PipelineRunSpec{ PipelineRef: &v1beta1.PipelineRef{ @@ -252,31 +137,29 @@ func TestPipelineRun_Invalid(t *testing.T) { }, want: apis.ErrInvalidValue("invalid bundle reference (could not parse reference: not a valid reference)", "spec.pipelineref.bundle"), wc: enableTektonOCIBundles(t), - }, - { - name: "pipelinerun pending while running", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinerunname", - }, - Spec: v1beta1.PipelineRunSpec{ - Status: v1beta1.PipelineRunSpecStatusPending, - PipelineRef: &v1beta1.PipelineRef{ - Name: "prname", - }, - }, - Status: v1beta1.PipelineRunStatus{ - PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{ - StartTime: &metav1.Time{time.Now()}, - }, + }, { + name: "pipelinerun pending while running", + pr: v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelinerunname", + }, + Spec: v1beta1.PipelineRunSpec{ + Status: v1beta1.PipelineRunSpecStatusPending, + PipelineRef: &v1beta1.PipelineRef{ + Name: "prname", }, }, - want: &apis.FieldError{ - Message: "invalid value: PipelineRun cannot be Pending after it is started", - Paths: []string{"spec.status"}, + Status: v1beta1.PipelineRunStatus{ + PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{ + StartTime: &metav1.Time{time.Now()}, + }, }, }, - } + want: &apis.FieldError{ + Message: "invalid value: PipelineRun cannot be Pending after it is started", + Paths: []string{"spec.status"}, + }, + }} for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -321,23 +204,6 @@ func TestPipelineRun_Validate(t *testing.T) { Timeout: &metav1.Duration{Duration: 0}, }, }, - }, { - name: "no tasksTimeout", - pr: v1beta1.PipelineRun{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pipelinelineName", - }, - Spec: v1beta1.PipelineRunSpec{ - PipelineRef: &v1beta1.PipelineRef{ - Name: "prname", - }, - Timeouts: &v1beta1.TimeoutFields{ - Pipeline: &metav1.Duration{Duration: 0}, - Tasks: &metav1.Duration{Duration: 0}, - Finally: &metav1.Duration{Duration: 0}, - }, - }, - }, }, { name: "array param with pipelinespec and taskspec", pr: v1beta1.PipelineRun{ @@ -393,7 +259,8 @@ func TestPipelineRun_Validate(t *testing.T) { for _, ts := range tests { t.Run(ts.name, func(t *testing.T) { - if err := ts.pr.Validate(context.Background()); err != nil { + ctx := context.Background() + if err := ts.pr.Validate(ctx); err != nil { t.Error(err) } }) @@ -501,6 +368,190 @@ func TestPipelineRunSpec_Validate(t *testing.T) { } } +func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) { + tests := []struct { + name string + pr v1beta1.PipelineRun + want *apis.FieldError + wc func(context.Context) context.Context + }{{ + name: "negative pipeline timeouts", + pr: v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelinelinename", + }, + Spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{ + Name: "prname", + }, + Timeouts: &v1beta1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: -48 * time.Hour}, + }, + }, + }, + want: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeouts.pipeline"), + wc: enableTektonTimeoutFields(), + }, { + name: "negative pipeline tasks Timeout", + pr: v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelinelinename", + }, + Spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{ + Name: "prname", + }, + Timeouts: &v1beta1.TimeoutFields{ + Tasks: &metav1.Duration{Duration: -48 * time.Hour}, + }, + }, + }, + want: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeouts.tasks"), + wc: enableTektonTimeoutFields(), + }, { + name: "negative pipeline finally Timeout", + pr: v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelinelinename", + }, + Spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{ + Name: "prname", + }, + Timeouts: &v1beta1.TimeoutFields{ + Finally: &metav1.Duration{Duration: -48 * time.Hour}, + }, + }, + }, + want: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeouts.finally"), + wc: enableTektonTimeoutFields(), + }, { + name: "pipeline tasks Timeout > pipeline Timeout", + pr: v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelinelinename", + }, + Spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{ + Name: "prname", + }, + Timeouts: &v1beta1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 25 * time.Minute}, + Tasks: &metav1.Duration{Duration: 1 * time.Hour}, + }, + }, + }, + want: apis.ErrInvalidValue("1h0m0s should be <= pipeline duration", "spec.timeouts.tasks"), + wc: enableTektonTimeoutFields(), + }, { + name: "pipeline finally Timeout > pipeline Timeout", + pr: v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelinelinename", + }, + Spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{ + Name: "prname", + }, + Timeouts: &v1beta1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 25 * time.Minute}, + Finally: &metav1.Duration{Duration: 1 * time.Hour}, + }, + }, + }, + want: apis.ErrInvalidValue("1h0m0s should be <= pipeline duration", "spec.timeouts.finally"), + wc: enableTektonTimeoutFields(), + }, { + name: "pipeline tasks Timeout + pipeline finally Timeout > pipeline Timeout", + pr: v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelinelinename", + }, + Spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{ + Name: "prname", + }, + Timeouts: &v1beta1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 50 * time.Minute}, + Tasks: &metav1.Duration{Duration: 30 * time.Minute}, + Finally: &metav1.Duration{Duration: 30 * time.Minute}, + }, + }, + }, + want: apis.ErrInvalidValue("30m0s + 30m0s should be <= pipeline duration", "spec.timeouts.finally, spec.timeouts.tasks"), + wc: enableTektonTimeoutFields(), + }, { + name: "Invalid Timeouts when alpha fields not enabled", + pr: v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelinelinename", + }, + Spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{ + Name: "prname", + }, + Timeouts: &v1beta1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 1 * time.Hour}, + Tasks: &metav1.Duration{Duration: 30 * time.Minute}, + Finally: &metav1.Duration{Duration: 30 * time.Minute}, + }, + }, + }, + want: apis.ErrGeneric(fmt.Sprintf(`timeouts requires "enable-api-fields" feature gate to be "alpha" but it is "stable"`)), + }} + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + if tc.wc != nil { + ctx = tc.wc(ctx) + } + err := tc.pr.Validate(ctx) + if d := cmp.Diff(err.Error(), tc.want.Error()); d != "" { + t.Error(diff.PrintWantGot(d)) + } + }) + } +} + +func TestPipelineRunWithAlphaFields_Validate(t *testing.T) { + tests := []struct { + name string + pr v1beta1.PipelineRun + wc func(context.Context) context.Context + }{{ + name: "no tasksTimeout", + pr: v1beta1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelinelinename", + }, + Spec: v1beta1.PipelineRunSpec{ + PipelineRef: &v1beta1.PipelineRef{ + Name: "prname", + }, + Timeouts: &v1beta1.TimeoutFields{ + Pipeline: &metav1.Duration{Duration: 0}, + Tasks: &metav1.Duration{Duration: 0}, + Finally: &metav1.Duration{Duration: 0}, + }, + }, + }, + wc: enableTektonTimeoutFields(), + }} + + for _, ts := range tests { + t.Run(ts.name, func(t *testing.T) { + ctx := context.Background() + if ts.wc != nil { + ctx = ts.wc(ctx) + } + if err := ts.pr.Validate(ctx); err != nil { + t.Error(err) + } + }) + } +} + func enableTektonOCIBundles(t *testing.T) func(context.Context) context.Context { return func(ctx context.Context) context.Context { s := config.NewStore(logtesting.TestLogger(t)) @@ -513,3 +564,18 @@ func enableTektonOCIBundles(t *testing.T) func(context.Context) context.Context return s.ToContext(ctx) } } + +func enableTektonTimeoutFields() func(context.Context) context.Context { + return func(ctx context.Context) context.Context { + featureFlags, _ := config.NewFeatureFlagsFromMap(map[string]string{ + "enable-api-fields": "alpha", + }) + cfg := &config.Config{ + Defaults: &config.Defaults{ + DefaultTimeoutMinutes: 60, + }, + FeatureFlags: featureFlags, + } + return config.ToContext(ctx, cfg) + } +} diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json index ec20a201170..e1299239a8c 100644 --- a/pkg/apis/pipeline/v1beta1/swagger.json +++ b/pkg/apis/pipeline/v1beta1/swagger.json @@ -989,6 +989,10 @@ "description": "Time after which the Pipeline times out. Defaults to never. Refer to Go's ParseDuration documentation for expected format: https://golang.org/pkg/time/#ParseDuration", "$ref": "#/definitions/v1.Duration" }, + "timeouts": { + "description": "This is an alpha field. You must set the \"enable-api-fields\" feature flag to \"alpha\" for this field to be supported.\n\nTime after which the Pipeline times out. Currently three keys are accepted in the map pipeline, tasks and finally with Timeouts.pipeline \u003e= Timeouts.tasks + Timeouts.finally", + "$ref": "#/definitions/v1beta1.TimeoutFields" + }, "workspaces": { "description": "Workspaces holds a set of workspace bindings that must match names with those declared in the pipeline.", "type": "array", @@ -2458,6 +2462,23 @@ } } }, + "v1beta1.TimeoutFields": { + "type": "object", + "properties": { + "finally": { + "description": "Finally sets the maximum allowed duration of this pipeline's finally", + "$ref": "#/definitions/v1.Duration" + }, + "pipeline": { + "description": "Pipeline sets the maximum allowed duration for execution of the entire pipeline. The sum of individual timeouts for tasks and finally must not exceed this value.", + "$ref": "#/definitions/v1.Duration" + }, + "tasks": { + "description": "Tasks sets the maximum allowed duration of this pipeline's tasks", + "$ref": "#/definitions/v1.Duration" + } + } + }, "v1beta1.WhenExpression": { "description": "WhenExpression allows a PipelineTask to declare expressions to be evaluated before the Task is run to determine whether the Task should be executed or skipped", "type": "object", diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index c2f883b70dd..3648d31d376 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -642,9 +642,9 @@ func (c *Reconciler) runNextSchedulableTask(ctx context.Context, pr *v1beta1.Pip } } else { if rprt.IsFinalTask(pipelineRunFacts) { - rprt.TaskRun, err = c.createFinallyTaskRun(ctx, rprt, pr, as.StorageBasePath(pr)) + rprt.TaskRun, err = c.createTaskRun(ctx, rprt, pr, as.StorageBasePath(pr), getFinallyTaskRunTimeout) } else { - rprt.TaskRun, err = c.createTaskRun(ctx, rprt, pr, as.StorageBasePath(pr)) + rprt.TaskRun, err = c.createTaskRun(ctx, rprt, pr, as.StorageBasePath(pr), getTaskRunTimeout) } if err != nil { recorder.Eventf(pr, corev1.EventTypeWarning, "TaskRunCreationFailed", "Failed to create TaskRun %q: %v", rprt.TaskRunName, err) @@ -698,15 +698,7 @@ func (c *Reconciler) updateRunsStatusDirectly(pr *v1beta1.PipelineRun) error { type getTimeoutFunc func(ctx context.Context, pr *v1beta1.PipelineRun, rprt *resources.ResolvedPipelineRunTask) *metav1.Duration -func (c *Reconciler) createTaskRun(ctx context.Context, rprt *resources.ResolvedPipelineRunTask, pr *v1beta1.PipelineRun, storageBasePath string) (*v1beta1.TaskRun, error) { - return c.createTaskRunHelper(ctx, rprt, pr, storageBasePath, getTaskRunTimeout) -} - -func (c *Reconciler) createFinallyTaskRun(ctx context.Context, rprt *resources.ResolvedPipelineRunTask, pr *v1beta1.PipelineRun, storageBasePath string) (*v1beta1.TaskRun, error) { - return c.createTaskRunHelper(ctx, rprt, pr, storageBasePath, getFinallyTaskRunTimeout) -} - -func (c *Reconciler) createTaskRunHelper(ctx context.Context, rprt *resources.ResolvedPipelineRunTask, pr *v1beta1.PipelineRun, storageBasePath string, getTimeoutFunc getTimeoutFunc) (*v1beta1.TaskRun, error) { +func (c *Reconciler) createTaskRun(ctx context.Context, rprt *resources.ResolvedPipelineRunTask, pr *v1beta1.PipelineRun, storageBasePath string, getTimeoutFunc getTimeoutFunc) (*v1beta1.TaskRun, error) { logger := logging.FromContext(ctx) tr, _ := c.taskRunLister.TaskRuns(pr.Namespace).Get(rprt.TaskRunName) @@ -950,7 +942,7 @@ func getFinallyTaskRunTimeout(ctx context.Context, pr *v1beta1.PipelineRun, rprt case pr.Spec.Timeout != nil: timeout = pr.Spec.Timeout.Duration case pr.Spec.Timeouts != nil: - // Take into account the elapsed time in order to check if we still enough time to run + // Take into account the elapsed time in order to check if we still have enough time to run // If task timeout is defined, add it to finally timeout // Else consider pipeline timeout as finally timeout switch { @@ -981,7 +973,7 @@ func getFinallyTaskRunTimeout(ctx context.Context, pr *v1beta1.PipelineRun, rprt // It is impossible for pr.Spec.Timeout to be nil, since SetDefault always assigns it with a value. taskRunTimeout = taskRunTimeoutHelper(timeout, pr, taskRunTimeout, rprt) - // Now that we know if we still have time to run the final task, substract tasksTimeout if needed + // Now that we know if we still have time to run the final task, subtract tasksTimeout if needed if taskRunTimeout.Duration > time.Second { taskRunTimeout.Duration -= tasksTimeout } diff --git a/test/timeout_test.go b/test/timeout_test.go index a348badc745..167466281b1 100644 --- a/test/timeout_test.go +++ b/test/timeout_test.go @@ -444,7 +444,7 @@ func TestPipelineRunTasksTimeout(t *testing.T) { // cancel the context after we have waited a suitable buffer beyond the given deadline. ctx, cancel := context.WithTimeout(context.Background(), timeout+2*time.Minute) defer cancel() - c, namespace := setup(ctx, t) + c, namespace := setup(ctx, t, requireAnyGate(map[string]string{"enable-api-fields": "alpha"})) knativetest.CleanupOnInterrupt(func() { tearDown(context.Background(), t, c, namespace) }, t.Logf) defer tearDown(context.Background(), t, c, namespace)