feat: override task timeouts in pipelineruns #8636
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| apiVersion: tekton.dev/v1 | ||
| kind: Pipeline | ||
| metadata: | ||
| name: pipeline-with-timeouts | ||
| spec: | ||
| tasks: | ||
| - name: task-with-timeout | ||
| timeout: "1h0m0s" | ||
| taskSpec: | ||
| steps: | ||
| - name: sleep | ||
| image: ubuntu | ||
| script: | | ||
| echo "Starting task..." | ||
| sleep 30 | ||
| echo "Task completed!" | ||
|
|
||
| - name: another-task-with-timeout | ||
| timeout: "30m0s" | ||
| runAfter: | ||
| - task-with-timeout | ||
| taskSpec: | ||
| steps: | ||
| - name: sleep | ||
| image: ubuntu | ||
| script: | | ||
| echo "Starting another task..." | ||
| sleep 30 | ||
| echo "Another task completed!" | ||
|
|
||
| --- | ||
| apiVersion: tekton.dev/v1 | ||
| kind: PipelineRun | ||
| metadata: | ||
| name: pipeline-timeout-override-run | ||
| spec: | ||
| pipelineRef: | ||
| name: pipeline-with-timeouts | ||
| timeouts: | ||
| pipeline: "3h0m0s" # overall pipeline timeout | ||
| taskRunSpecs: | ||
| - pipelineTaskName: task-with-timeout | ||
| timeout: "2h0m0s" # override timeout to 2 hours | ||
| - pipelineTaskName: another-task-with-timeout | ||
| timeout: "1h0m0s" # override timeout to 1 hour |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1362,7 +1362,7 @@ func TestPipelineSpec_Validate_Failure(t *testing.T) { | |
| if err == nil { | ||
| t.Errorf("PipelineSpec.Validate() did not return error for invalid pipelineSpec") | ||
| } | ||
| if d := cmp.Diff(tt.expectedError.Error(), err.Error()); d != "" { | ||
| t.Errorf("PipelineSpec.Validate() errors diff %s", diff.PrintWantGot(d)) | ||
| } | ||
| }) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -656,6 +656,11 @@ type PipelineTaskRunSpec struct { | |||||||||
|
|
||||||||||
| // Compute resources to use for this TaskRun | ||||||||||
| ComputeResources *corev1.ResourceRequirements `json:"computeResources,omitempty"` | ||||||||||
|
|
||||||||||
| // Time after which the TaskRun times out. | ||||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||||
| // Refer Go's ParseDuration documentation for expected format: https://golang.org/pkg/time/#ParseDuration | ||||||||||
| // +optional | ||||||||||
| Timeout *metav1.Duration `json:"timeout,omitempty"` | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // GetTaskRunSpec returns the task specific spec for a given | ||||||||||
|
|
@@ -678,6 +683,7 @@ func (pr *PipelineRun) GetTaskRunSpec(pipelineTaskName string) PipelineTaskRunSp | |||||||||
| s.SidecarSpecs = task.SidecarSpecs | ||||||||||
| s.Metadata = task.Metadata | ||||||||||
| s.ComputeResources = task.ComputeResources | ||||||||||
| s.Timeout = task.Timeout | ||||||||||
| } | ||||||||||
| } | ||||||||||
| return s | ||||||||||
|
|
||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,6 +87,11 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) | |
| // Validate propagated parameters | ||
| errs = errs.Also(ps.validateInlineParameters(ctx)) | ||
|
|
||
| // Validate task run specs first | ||
| for _, trs := range ps.TaskRunSpecs { | ||
| errs = errs.Also(validateTaskRunSpec(ctx, trs).ViaFieldKey("taskRunSpecs", trs.PipelineTaskName)) | ||
| } | ||
|
|
||
| if ps.Timeouts != nil { | ||
| // tasks timeout should be a valid duration of at least 0. | ||
| errs = errs.Also(validateTimeoutDuration("tasks", ps.Timeouts.Tasks)) | ||
|
|
@@ -103,6 +108,8 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) | |
| defaultTimeout := time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) | ||
| errs = errs.Also(ps.validatePipelineTimeout(defaultTimeout, "should be <= default timeout duration")) | ||
| } | ||
|
|
||
| errs = errs.Also(validatePipelineTaskRunSpecTimeouts(ps, ctx)) | ||
| } | ||
|
|
||
| errs = errs.Also(validateSpecStatus(ps.Status)) | ||
|
|
@@ -117,9 +124,6 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) | |
| wsNames[ws.Name] = idx | ||
| } | ||
| } | ||
|
|
||
| if ps.TaskRunTemplate.PodTemplate != nil { | ||
| errs = errs.Also(validatePodTemplateEnv(ctx, *ps.TaskRunTemplate.PodTemplate).ViaField("taskRunTemplate")) | ||
|
|
@@ -329,5 +333,38 @@ func validateTaskRunSpec(ctx context.Context, trs PipelineTaskRunSpec) (errs *ap | |
| if trs.PodTemplate != nil { | ||
| errs = errs.Also(validatePodTemplateEnv(ctx, *trs.PodTemplate)) | ||
| } | ||
| if trs.Timeout != nil && trs.Timeout.Duration < 0 { | ||
| errs = errs.Also(apis.ErrInvalidValue(trs.Timeout.Duration.String()+" should be >= 0", "timeout")) | ||
| } | ||
| return errs | ||
| } | ||
|
|
||
| // validatePipelineTaskRunSpecTimeouts validates timeouts for TaskRunSpecs in a PipelineRun | ||
| func validatePipelineTaskRunSpecTimeouts(ps *PipelineRunSpec, ctx context.Context) *apis.FieldError { | ||
|
There was a problem hiding this comment. nit: conventionally the context is usually the first argument |
||
| var errs *apis.FieldError | ||
|
|
||
| if len(ps.TaskRunSpecs) == 0 { | ||
| return errs | ||
| } | ||
|
|
||
| pipelineTimeout := ps.Timeouts.Pipeline | ||
| if pipelineTimeout == nil { | ||
| pipelineTimeout = &metav1.Duration{Duration: time.Duration(config.FromContextOrDefaults(ctx).Defaults.DefaultTimeoutMinutes) * time.Minute} | ||
| } | ||
|
Comment on lines
+350
to
+353
There was a problem hiding this comment. I believe this should be comparing There was a problem hiding this comment. What do you think about something like this? This implementation adds a couple things:
// in the PipelineValidation
func validatePipelineTaskRunSpecTimeouts(ps *PipelineRunSpec, ctx context.Context) *apis.FieldError {
cfg := config.FromContextOrDefaults(ctx)
var errs *apis.FieldError
maxTimeout := &metav1.Duration{Duration: time.Duration(cfg.Defaults.DefaultTimeoutMinutes) * time.Minute}
if ps.Timeouts != nil {
// Loop over max-timeouts in order of precedence to find most applicable
for _, timeout := range []*metav1.Duration{ps.Timeouts.Tasks, ps.Timeouts.Pipeline} {
if timeout != nil {
timeout, err := validateTimeout(ps.Timeouts.Tasks, cfg.Defaults.DefaultTimeoutMinutes)
if err != nil {
return errs.Also(err)
}
maxTimeout = timeout
break
}
}
}
if maxTimeout.Duration == config.NoTimeoutDuration || len(ps.TaskRunSpecs) == 0 {
return errs
}
for _, taskRunSpec := range ps.TaskRunSpecs {
taskRunTimeout, err := validateTimeout(taskRunSpec.Timeout, cfg.Defaults.DefaultTimeoutMinutes)
if err != nil {
errs = errs.Also(err)
} else if taskRunTimeout.Duration > maxTimeout.Duration {
errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s should be <= pipeline tasks duration %s", &taskRunTimeout.Duration, maxTimeout.Duration), "taskRunSpecs["+taskRunSpec.PipelineTaskName+"].timeout"))
}
}
return errs
}
// In the TaskRun validation
func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
cfg := config.FromContextOrDefaults(ctx)
// ...
_, timeoutErr := validateTimeout(ts.Timeout, cfg.Defaults.DefaultTimeoutMinutes)
errs = errs.Also(timeoutErr)
// ...
}
// In some resource-agnostic location
func validateTimeout(timeout *metav1.Duration, defaultTimeoutMinutes int) (*metav1.Duration, *apis.FieldError) {
if timeout == nil {
return &metav1.Duration{Duration: time.Duration(defaultTimeoutMinutes) * time.Minute}, nil
}
if timeout.Duration < 0 {
return nil, apis.ErrInvalidValue(timeout.Duration.String()+" should be >= 0", "timeout")
}
return timeout, nil
}
|
||
|
|
||
| if pipelineTimeout.Duration == config.NoTimeoutDuration { | ||
| return errs | ||
| } | ||
|
|
||
| for _, taskRunSpec := range ps.TaskRunSpecs { | ||
| if taskRunSpec.Timeout == nil { | ||
| continue | ||
| } | ||
|
|
||
| if taskRunSpec.Timeout.Duration > pipelineTimeout.Duration { | ||
| errs = errs.Also(apis.ErrInvalidValue(taskRunSpec.Timeout.Duration.String()+" should be <= pipeline duration", "taskRunSpecs["+taskRunSpec.PipelineTaskName+"].timeout")) | ||
| } | ||
| } | ||
|
|
||
| return errs | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -805,6 +805,28 @@ func TestPipelineRun_Validate(t *testing.T) { | |
| }, | ||
| }, | ||
| wc: cfgtesting.EnableAlphaAPIFields, | ||
| }, { | ||
| name: "valid task-specific timeouts", | ||
| pr: v1.PipelineRun{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "pipelinelinename", | ||
| }, | ||
| Spec: v1.PipelineRunSpec{ | ||
| PipelineRef: &v1.PipelineRef{ | ||
| Name: "prname", | ||
| }, | ||
| Timeouts: &v1.TimeoutFields{ | ||
| Pipeline: &metav1.Duration{Duration: 1 * time.Hour}, | ||
| }, | ||
| TaskRunSpecs: []v1.PipelineTaskRunSpec{{ | ||
| PipelineTaskName: "task1", | ||
| Timeout: &metav1.Duration{Duration: 30 * time.Minute}, | ||
| }, { | ||
| PipelineTaskName: "task2", | ||
| Timeout: &metav1.Duration{Duration: 45 * time.Minute}, | ||
| }}, | ||
| }, | ||
| }, | ||
| }} | ||
|
|
||
| for _, ts := range tests { | ||
|
|
@@ -849,7 +871,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { | |
| }, | ||
| }, | ||
| withContext: EnableForbiddenEnv, | ||
| wantErr: apis.ErrInvalidValue("PodTemplate cannot update a forbidden env: TEST_ENV", "taskRunSpecs[task-1].PodTemplate.Env"), | ||
| }, { | ||
| name: "TaskRunTemplate.PodTemplate contains forbidden env", | ||
| spec: v1.PipelineRunSpec{ | ||
|
|
@@ -969,7 +991,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { | |
| }, | ||
| }, | ||
| }, | ||
| wantErr: apis.ErrMultipleOneOf("taskRunSpecs[bar].stepSpecs[1].name"), | ||
| withContext: cfgtesting.EnableAlphaAPIFields, | ||
| }, { | ||
| name: "stepSpecs disallowed without beta feature gate", | ||
|
|
@@ -1022,7 +1044,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { | |
| }, | ||
| }, | ||
| }, | ||
| wantErr: apis.ErrMissingField("taskRunSpecs[bar].stepSpecs[0].name"), | ||
| withContext: cfgtesting.EnableAlphaAPIFields, | ||
| }, { | ||
| name: "duplicate sidecarSpec names", | ||
|
|
@@ -1037,7 +1059,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { | |
| }, | ||
| }, | ||
| }, | ||
| wantErr: apis.ErrMultipleOneOf("taskRunSpecs[bar].sidecarSpecs[1].name"), | ||
| withContext: cfgtesting.EnableAlphaAPIFields, | ||
| }, { | ||
| name: "missing sidecarSpec name", | ||
|
|
@@ -1054,7 +1076,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { | |
| }, | ||
| }, | ||
| }, | ||
| wantErr: apis.ErrMissingField("taskRunSpecs[bar].sidecarSpecs[0].name"), | ||
| withContext: cfgtesting.EnableAlphaAPIFields, | ||
| }, { | ||
| name: "invalid both step-level (stepSpecs.resources) and task-level (taskRunSpecs.resources) resource requirements configured", | ||
|
|
@@ -1076,8 +1098,8 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) { | |
| }, | ||
| }, | ||
| wantErr: apis.ErrMultipleOneOf( | ||
| "taskRunSpecs[pipelineTask].stepSpecs.resources", | ||
| "taskRunSpecs[pipelineTask].computeResources", | ||
| ), | ||
| withContext: cfgtesting.EnableBetaAPIFields, | ||
| }, { | ||
|
|
@@ -1202,6 +1224,23 @@ func TestPipelineRun_InvalidTimeouts(t *testing.T) { | |
| }, | ||
| }, | ||
| want: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeouts.pipeline"), | ||
| }, { | ||
| name: "negative task-specific timeout", | ||
| pr: v1.PipelineRun{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "pipelinelinename", | ||
| }, | ||
| Spec: v1.PipelineRunSpec{ | ||
| PipelineRef: &v1.PipelineRef{ | ||
| Name: "prname", | ||
| }, | ||
| TaskRunSpecs: []v1.PipelineTaskRunSpec{{ | ||
| PipelineTaskName: "task1", | ||
| Timeout: &metav1.Duration{Duration: -1 * time.Hour}, | ||
| }}, | ||
| }, | ||
| }, | ||
| want: apis.ErrInvalidValue("-1h0m0s should be >= 0", "spec.taskRunSpecs[task1].timeout"), | ||
| }, { | ||
| name: "negative pipeline tasks Timeout", | ||
| pr: v1.PipelineRun{ | ||
|
|
@@ -1352,13 +1391,57 @@ func TestPipelineRun_InvalidTimeouts(t *testing.T) { | |
| }, | ||
| }, | ||
| want: apis.ErrInvalidValue(`0s (no timeout) should be <= pipeline duration`, "spec.timeouts.finally"), | ||
| }, { | ||
| name: "task-specific timeout exceeds pipeline timeout", | ||
|
There was a problem hiding this comment. You could add another valid test covering the case when the |
||
| pr: v1.PipelineRun{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "pipelinelinename", | ||
| }, | ||
| Spec: v1.PipelineRunSpec{ | ||
| PipelineRef: &v1.PipelineRef{ | ||
| Name: "prname", | ||
| }, | ||
| Timeouts: &v1.TimeoutFields{ | ||
| Pipeline: &metav1.Duration{Duration: 1 * time.Hour}, | ||
| }, | ||
| TaskRunSpecs: []v1.PipelineTaskRunSpec{{ | ||
| PipelineTaskName: "task1", | ||
| Timeout: &metav1.Duration{Duration: 2 * time.Hour}, | ||
| }}, | ||
| }, | ||
| }, | ||
| want: apis.ErrInvalidValue("2h0m0s should be <= pipeline duration", "spec.taskRunSpecs[task1].timeout"), | ||
| }, { | ||
| name: "when pipeline timeout is no timeout (0s), task timeouts can exceed it without error", | ||
| pr: v1.PipelineRun{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "pipelinelinename", | ||
| }, | ||
| Spec: v1.PipelineRunSpec{ | ||
| PipelineRef: &v1.PipelineRef{ | ||
| Name: "prname", | ||
| }, | ||
| Timeouts: &v1.TimeoutFields{ | ||
| Pipeline: &metav1.Duration{Duration: config.NoTimeoutDuration}, | ||
| }, | ||
| TaskRunSpecs: []v1.PipelineTaskRunSpec{{ | ||
| PipelineTaskName: "task1", | ||
| Timeout: &metav1.Duration{Duration: 10 * time.Hour}, | ||
| }}, | ||
| }, | ||
| }, | ||
| want: nil, // no error expected when pipeline has no timeout | ||
| }} | ||
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| ctx := context.Background() | ||
| err := tc.pr.Validate(ctx) | ||
| if tc.want == nil { | ||
| if err != nil { | ||
| t.Errorf("Expected no error but got: %v", err) | ||
| } | ||
| } else if d := cmp.Diff(tc.want.Error(), err.Error()); d != "" { | ||
| t.Error(diff.PrintWantGot(d)) | ||
| } | ||
| }) | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.