Deprecate timeout and promote timeouts in PR

With TEP-0046 a combination of timeouts was proposed as timeouts: pipeline: 30m task: 30m finally: 30m The following was introduced as alpha feature and it's been almost a while and now we can safely move that out of alpha gate. This commit promotes the combined timeouts from alpha to stable and also deprecation existing timeout which can only be used to apply timeout at Pipeline level. Signed-off-by: vinamra28 [email protected]
yannickhilber · May 12, 2022 · 84229fc · 84229fc
1 parent 9c0e081
commit 84229fc
Show file tree

Hide file tree

Showing 10 changed files with 132 additions and 56 deletions.
diff --git a/docs/deprecations.md b/docs/deprecations.md
@@ -25,3 +25,4 @@ being deprecated.
 | [`PipelineRunCancelled` is deprecated and will be removed](https://github.com/tektoncd/pipeline/issues/4611)                                                        | [v0.25.0](https://github.com/tektoncd/pipeline/releases/tag/v0.25.0) | Beta                                                                                                   | July 12 2022                       |
 | [`PipelineResources` are deprecated.](https://github.com/tektoncd/community/blob/main/teps/0074-deprecate-pipelineresources.md)                                     | [v0.30.0](https://github.com/tektoncd/pipeline/releases/tag/v0.30.0) | Alpha                                                                                                  | Dec 20 2021                         |
 | [The `PipelineRun.Status.TaskRuns` and `PipelineRun.Status.Runs` fields are deprecated and will be removed.](https://github.com/tektoncd/community/blob/main/teps/0100-embedded-taskruns-and-runs-status-in-pipelineruns.md) | v0.35.0 (to be released)                                             | Beta                                                                                                   | Jan 25, 2023                        |
+| [PipelineRun.Timeout is deprecated and will be removed](https://github.com/tektoncd/community/blob/main/teps/0046-finallytask-execution-post-timeout.md) | v0.36.0 (to be released)                                             | Beta                                                                                                   | Feb 25, 2023                        |
diff --git a/docs/pipelineruns.md b/docs/pipelineruns.md
@@ -513,12 +513,6 @@ For more information, see the [`LimitRange` support in Pipeline](./compute-resou
 
 ### Configuring a failure timeout
 
-You can use the `timeout` field to set the `PipelineRun's` desired timeout value in minutes.
-If you do not specify this value in the `PipelineRun`, the global default timeout value applies.
-If you set the timeout to 0, the `PipelineRun` fails immediately upon encountering an error.
-
-> :warning: ** `timeout`will be deprecated in future versions. Consider using `timeouts` instead.
-
 You can use the `timeouts` field to set the `PipelineRun's` desired timeout value in minutes.  There are three sub-fields than can be used to specify failures timeout for the entire pipeline, for tasks, and for `finally` tasks.
 
 ```yaml
@@ -552,6 +546,12 @@ spec:
     finally: "0h3m0s"
 ```
 
+You can also use the *Deprecated* `timeout` field to set the `PipelineRun's` desired timeout value in minutes.
+If you do not specify this value in the `PipelineRun`, the global default timeout value applies.
+If you set the timeout to 0, the `PipelineRun` fails immediately upon encountering an error.
+
+> :warning: ** `timeout` is deprecated and will be removed in future versions. Consider using `timeouts` instead.
+
 If you do not specify the `timeout` value or `timeouts.pipeline` in the `PipelineRun`, the global default timeout value applies.
 If you set the `timeout` value or `timeouts.pipeline` to 0, the `PipelineRun` fails immediately upon encountering an error.
 If `timeouts.tasks` or `timeouts.finally` is set to 0, `timeouts.pipeline` must also be set to 0.

diff --git a/examples/v1beta1/pipelineruns/no-ci/pipeline-timeout.yaml b/examples/v1beta1/pipelineruns/no-ci/pipeline-timeout.yaml
@@ -63,9 +63,6 @@ spec:
   # 1 hour and half timeout for the pipeline
   # 1 hour and fifteen minutes for the pipeline tasks
   # 15 minutes for the finally tasks
-  #
-  # 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

diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go
diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_types.go b/pkg/apis/pipeline/v1beta1/pipelinerun_types.go
@@ -219,15 +219,14 @@ 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
 	// with Timeouts.pipeline >= Timeouts.tasks + Timeouts.finally
 	// +optional
 	Timeouts *TimeoutFields `json:"timeouts,omitempty"`
+
+	// Timeout Deprecated: use pipelineRunSpec.Timeouts.Pipeline instead
 	// 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
 	// +optional

diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go
@@ -80,8 +80,6 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError)
 			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.
 		errs = errs.Also(validateTimeoutDuration("tasks", ps.Timeouts.Tasks))
 

diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go
@@ -651,12 +651,11 @@ func TestPipelineRunSpec_Validate(t *testing.T) {
 	}
 }
 
-func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) {
+func TestPipelineRun_InvalidTimeouts(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{
@@ -673,7 +672,6 @@ func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) {
 			},
 		},
 		want: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeouts.pipeline"),
-		wc:   enableAlphaAPIFields,
 	}, {
 		name: "negative pipeline tasks Timeout",
 		pr: v1beta1.PipelineRun{
@@ -690,7 +688,6 @@ func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) {
 			},
 		},
 		want: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeouts.tasks"),
-		wc:   enableAlphaAPIFields,
 	}, {
 		name: "negative pipeline finally Timeout",
 		pr: v1beta1.PipelineRun{
@@ -707,7 +704,6 @@ func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) {
 			},
 		},
 		want: apis.ErrInvalidValue("-48h0m0s should be >= 0", "spec.timeouts.finally"),
-		wc:   enableAlphaAPIFields,
 	}, {
 		name: "pipeline tasks Timeout > pipeline Timeout",
 		pr: v1beta1.PipelineRun{
@@ -725,7 +721,6 @@ func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) {
 			},
 		},
 		want: apis.ErrInvalidValue("1h0m0s should be <= pipeline duration", "spec.timeouts.tasks"),
-		wc:   enableAlphaAPIFields,
 	}, {
 		name: "pipeline finally Timeout > pipeline Timeout",
 		pr: v1beta1.PipelineRun{
@@ -743,7 +738,6 @@ func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) {
 			},
 		},
 		want: apis.ErrInvalidValue("1h0m0s should be <= pipeline duration", "spec.timeouts.finally"),
-		wc:   enableAlphaAPIFields,
 	}, {
 		name: "pipeline tasks Timeout +  pipeline finally Timeout > pipeline Timeout",
 		pr: v1beta1.PipelineRun{
@@ -762,25 +756,6 @@ func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) {
 			},
 		},
 		want: apis.ErrInvalidValue("30m0s + 30m0s should be <= pipeline duration", "spec.timeouts.finally, spec.timeouts.tasks"),
-		wc:   enableAlphaAPIFields,
-	}, {
-		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(`timeouts requires "enable-api-fields" feature gate to be "alpha" but it is "stable"`),
 	}, {
 		name: "Tasks timeout = 0 but Pipeline timeout not set",
 		pr: v1beta1.PipelineRun{
@@ -796,7 +771,6 @@ func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) {
 				},
 			},
 		},
-		wc:   enableAlphaAPIFields,
 		want: apis.ErrInvalidValue(`0s (no timeout) should be <= default timeout duration`, "spec.timeouts.tasks"),
 	}, {
 		name: "Tasks timeout = 0 but Pipeline timeout is not 0",
@@ -814,7 +788,6 @@ func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) {
 				},
 			},
 		},
-		wc:   enableAlphaAPIFields,
 		want: apis.ErrInvalidValue(`0s (no timeout) should be <= pipeline duration`, "spec.timeouts.tasks"),
 	}, {
 		name: "Finally timeout = 0 but Pipeline timeout not set",
@@ -831,7 +804,6 @@ func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) {
 				},
 			},
 		},
-		wc:   enableAlphaAPIFields,
 		want: apis.ErrInvalidValue(`0s (no timeout) should be <= default timeout duration`, "spec.timeouts.finally"),
 	}, {
 		name: "Finally timeout = 0 but Pipeline timeout is not 0",
@@ -849,16 +821,29 @@ func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) {
 				},
 			},
 		},
-		wc:   enableAlphaAPIFields,
 		want: apis.ErrInvalidValue(`0s (no timeout) should be <= pipeline duration`, "spec.timeouts.finally"),
+	}, {
+		name: "Timeout and Timeouts both are set",
+		pr: v1beta1.PipelineRun{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "pipelinelinename",
+			},
+			Spec: v1beta1.PipelineRunSpec{
+				PipelineRef: &v1beta1.PipelineRef{
+					Name: "prname",
+				},
+				Timeout: &metav1.Duration{Duration: 10 * time.Minute},
+				Timeouts: &v1beta1.TimeoutFields{
+					Pipeline: &metav1.Duration{Duration: 10 * time.Minute},
+				},
+			},
+		},
+		want: apis.ErrDisallowedFields("spec.timeout", "spec.timeouts"),
 	}}
 
 	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))
@@ -867,7 +852,7 @@ func TestPipelineRunWithAlphaFields_Invalid(t *testing.T) {
 	}
 }
 
-func TestPipelineRunWithAlphaFields_Validate(t *testing.T) {
+func TestPipelineRunWithTimeout_Validate(t *testing.T) {
 	tests := []struct {
 		name string
 		pr   v1beta1.PipelineRun
@@ -889,7 +874,23 @@ func TestPipelineRunWithAlphaFields_Validate(t *testing.T) {
 				},
 			},
 		},
-		wc: enableAlphaAPIFields,
+	}, {
+		name: "Timeouts set for all three Task, Finally and Pipeline",
+		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},
+				},
+			},
+		},
 	}}
 
 	for _, ts := range tests {

diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json
@@ -1104,11 +1104,11 @@
           "x-kubernetes-list-type": "atomic"
         },
         "timeout": {
-          "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",
+          "description": "Timeout Deprecated: use pipelineRunSpec.Timeouts.Pipeline instead 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",
+          "description": "Time 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": {

diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go
@@ -2432,8 +2432,8 @@ spec:
 	}
 }
 
-func TestReconcileWithTimeout(t *testing.T) {
-	// TestReconcileWithTimeout runs "Reconcile" on a PipelineRun that has timed out.
+func TestReconcileWithTimeoutDeprecated(t *testing.T) {
+	// TestReconcileWithTimeoutDeprecated runs "Reconcile" on a PipelineRun that has timed out.
 	// It verifies that reconcile is successful, the pipeline status updated and events generated.
 	ps := []*v1beta1.Pipeline{simpleHelloWorldPipeline}
 	prs := []*v1beta1.PipelineRun{parse.MustParsePipelineRun(t, `
@@ -2481,6 +2481,56 @@ status:
 	}
 }
 
+func TestReconcileWithTimeouts(t *testing.T) {
+	// TestReconcileWithTimeouts runs "Reconcile" on a PipelineRun that has timed out.
+	// It verifies that reconcile is successful, the pipeline status updated and events generated.
+	ps := []*v1beta1.Pipeline{simpleHelloWorldPipeline}
+	prs := []*v1beta1.PipelineRun{parse.MustParsePipelineRun(t, `
+metadata:
+  name: test-pipeline-run-with-timeout
+  namespace: foo
+spec:
+  pipelineRef:
+    name: test-pipeline
+  serviceAccountName: test-sa
+  timeouts:
+    pipeline: 12h0m0s
+status:
+  startTime: "2021-12-31T00:00:00Z"
+`)}
+	ts := []*v1beta1.Task{simpleHelloWorldTask}
+
+	d := test.Data{
+		PipelineRuns: prs,
+		Pipelines:    ps,
+		Tasks:        ts,
+	}
+	prt := newPipelineRunTest(d, t)
+	defer prt.Cancel()
+
+	wantEvents := []string{
+		"Warning Failed PipelineRun \"test-pipeline-run-with-timeout\" failed to finish within \"12h0m0s\"",
+	}
+	reconciledRun, clients := prt.reconcileRun("foo", "test-pipeline-run-with-timeout", wantEvents, false)
+
+	if reconciledRun.Status.CompletionTime == nil {
+		t.Errorf("Expected a CompletionTime on invalid PipelineRun but was nil")
+	}
+
+	// The PipelineRun should be timed out.
+	if reconciledRun.Status.GetCondition(apis.ConditionSucceeded).Reason != "PipelineRunTimeout" {
+		t.Errorf("Expected PipelineRun to be timed out, but condition reason is %s", reconciledRun.Status.GetCondition(apis.ConditionSucceeded))
+	}
+
+	// Check that the expected TaskRun was created
+	actual := getTaskRunCreations(t, clients.Pipeline.Actions(), 2)[0]
+
+	// The TaskRun timeout should be less than or equal to the PipelineRun timeout.
+	if actual.Spec.Timeout.Duration > prs[0].Spec.Timeouts.Pipeline.Duration {
+		t.Errorf("TaskRun timeout %s should be less than or equal to PipelineRun timeout %s", actual.Spec.Timeout.Duration.String(), prs[0].Spec.Timeouts.Pipeline.Duration.String())
+	}
+}
+
 func TestReconcileWithoutPVC(t *testing.T) {
 	// TestReconcileWithoutPVC runs "Reconcile" on a PipelineRun that has two unrelated tasks.
 	// It verifies that reconcile is successful and that no PVC is created

diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunstate_test.go
@@ -1769,7 +1769,7 @@ func TestGetPipelineConditionStatus_WithFinalTasks(t *testing.T) {
 }
 
 // pipeline should result in timeout if its runtime exceeds its spec.Timeout based on its status.Timeout
-func TestGetPipelineConditionStatus_PipelineTimeouts(t *testing.T) {
+func TestGetPipelineConditionStatus_PipelineTimeoutDeprecated(t *testing.T) {
 	d, err := dagFromState(oneFinishedState)
 	if err != nil {
 		t.Fatalf("Unexpected error while building DAG for state %v: %v", oneFinishedState, err)
@@ -1796,6 +1796,36 @@ func TestGetPipelineConditionStatus_PipelineTimeouts(t *testing.T) {
 	}
 }
 
+// pipeline should result in timeout if its runtime exceeds its spec.Timeouts.Pipeline based on its status.Timeout
+func TestGetPipelineConditionStatus_PipelineTimeouts(t *testing.T) {
+	d, err := dagFromState(oneFinishedState)
+	if err != nil {
+		t.Fatalf("Unexpected error while building DAG for state %v: %v", oneFinishedState, err)
+	}
+	pr := &v1beta1.PipelineRun{
+		ObjectMeta: metav1.ObjectMeta{Name: "pipelinerun-no-tasks-started"},
+		Spec: v1beta1.PipelineRunSpec{
+			Timeouts: &v1beta1.TimeoutFields{
+				Pipeline: &metav1.Duration{Duration: 1 * time.Minute},
+			},
+		},
+		Status: v1beta1.PipelineRunStatus{
+			PipelineRunStatusFields: v1beta1.PipelineRunStatusFields{
+				StartTime: &metav1.Time{Time: now.Add(-2 * time.Minute)},
+			},
+		},
+	}
+	facts := PipelineRunFacts{
+		State:           oneFinishedState,
+		TasksGraph:      d,
+		FinalTasksGraph: &dag.Graph{},
+	}
+	c := facts.GetPipelineConditionStatus(context.Background(), pr, zap.NewNop().Sugar(), testClock)
+	if c.Status != corev1.ConditionFalse && c.Reason != v1beta1.PipelineRunReasonTimedOut.String() {
+		t.Fatalf("Expected to get status %s but got %s for state %v", corev1.ConditionFalse, c.Status, oneFinishedState)
+	}
+}
+
 func TestAdjustStartTime(t *testing.T) {
 	baseline := metav1.Time{Time: now}