Add timeoutSeconds to RevisionSpec (#2437)

* Add timeoutSeconds to RevisionSpec re: issue: #457 Use ClusterIngress default timeout as max timeout for revisionspec. Add checks that cluster ingress timeout and revisionspec timeout does not exceed cluster ingress default timeout. Will implement the enforcement of the timeout in future PR. * Pulling in a new knative/pkg to pick up ErrOutOfBoundsValue helper
knative · Nov 15, 2018 · c92a249 · c92a249
1 parent 52f6c0c
commit c92a249
Show file tree

Hide file tree

Showing 12 changed files with 154 additions and 12 deletions.
diff --git a/Gopkg.lock b/Gopkg.lock
diff --git a/Gopkg.toml b/Gopkg.toml
@@ -23,8 +23,8 @@ required = [
 
 [[override]]
   name = "github.com/knative/pkg"
-  # HEAD as of 2018-11-08
-  revision = "4b704fa7948ad9ae8ec90d1cd5b4a34516b252ea"
+  # HEAD as of 2018-11-13
+  revision = "3e52d67e3d58f63f43ad61241b00a9974cb03ecd"
 
 [[override]]
   name = "go.uber.org/zap"

diff --git a/pkg/apis/networking/v1alpha1/clusteringress_validation_test.go b/pkg/apis/networking/v1alpha1/clusteringress_validation_test.go
@@ -382,7 +382,7 @@ func TestClusterIngressValidation(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			got := test.ci.Validate()
-			if diff := cmp.Diff(test.want, got); diff != "" {
+			if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" {
 				t.Errorf("Validate (-want, +got) = %v", diff)
 			}
 		})

diff --git a/pkg/apis/serving/v1alpha1/configuration_defaults_test.go b/pkg/apis/serving/v1alpha1/configuration_defaults_test.go
@@ -18,8 +18,10 @@ package v1alpha1
 
 import (
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestConfigurationDefaulting(t *testing.T) {
@@ -33,7 +35,11 @@ func TestConfigurationDefaulting(t *testing.T) {
 		want: &Configuration{
 			Spec: ConfigurationSpec{
 				RevisionTemplate: RevisionTemplateSpec{
-					Spec: RevisionSpec{},
+					Spec: RevisionSpec{
+						TimeoutSeconds: &metav1.Duration{
+							Duration: 60 * time.Second,
+						},
+					},
 				},
 			},
 		},
@@ -44,6 +50,9 @@ func TestConfigurationDefaulting(t *testing.T) {
 				RevisionTemplate: RevisionTemplateSpec{
 					Spec: RevisionSpec{
 						ContainerConcurrency: 1,
+						TimeoutSeconds: &metav1.Duration{
+							Duration: 99 * time.Second,
+						},
 					},
 				},
 			},
@@ -53,6 +62,9 @@ func TestConfigurationDefaulting(t *testing.T) {
 				RevisionTemplate: RevisionTemplateSpec{
 					Spec: RevisionSpec{
 						ContainerConcurrency: 1,
+						TimeoutSeconds: &metav1.Duration{
+							Duration: 99 * time.Second,
+						},
 					},
 				},
 			},

diff --git a/pkg/apis/serving/v1alpha1/revision_defaults.go b/pkg/apis/serving/v1alpha1/revision_defaults.go
@@ -16,6 +16,17 @@ limitations under the License.
 
 package v1alpha1
 
+import (
+	"time"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+const (
+	// defaultTimeout will be set if timeoutSeconds not specified.
+	defaultTimeout = 60 * time.Second
+)
+
 func (r *Revision) SetDefaults() {
 	r.Spec.SetDefaults()
 }
@@ -26,4 +37,8 @@ func (rs *RevisionSpec) SetDefaults() {
 	if rs.ConcurrencyModel == RevisionRequestConcurrencyModelSingle && rs.ContainerConcurrency == 0 {
 		rs.ContainerConcurrency = 1
 	}
+
+	if rs.TimeoutSeconds == nil {
+		rs.TimeoutSeconds = &metav1.Duration{Duration: defaultTimeout}
+	}
 }
diff --git a/pkg/apis/serving/v1alpha1/revision_defaults_test.go b/pkg/apis/serving/v1alpha1/revision_defaults_test.go
@@ -18,8 +18,10 @@ package v1alpha1
 
 import (
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestRevisionDefaulting(t *testing.T) {
@@ -33,17 +35,28 @@ func TestRevisionDefaulting(t *testing.T) {
 		want: &Revision{
 			Spec: RevisionSpec{
 				ContainerConcurrency: 0,
+				TimeoutSeconds: &metav1.Duration{
+					Duration: 60 * time.Second,
+				},
 			},
 		},
 	}, {
 		name: "no overwrite",
 		in: &Revision{
 			Spec: RevisionSpec{
-				ContainerConcurrency: 1},
+				ContainerConcurrency: 1,
+				TimeoutSeconds: &metav1.Duration{
+					Duration: 99 * time.Second,
+				},
+			},
 		},
 		want: &Revision{
 			Spec: RevisionSpec{
-				ContainerConcurrency: 1},
+				ContainerConcurrency: 1,
+				TimeoutSeconds: &metav1.Duration{
+					Duration: 99 * time.Second,
+				},
+			},
 		},
 	}, {
 		name: "partially initialized",
@@ -52,7 +65,11 @@ func TestRevisionDefaulting(t *testing.T) {
 		},
 		want: &Revision{
 			Spec: RevisionSpec{
-				ContainerConcurrency: 0},
+				ContainerConcurrency: 0,
+				TimeoutSeconds: &metav1.Duration{
+					Duration: 60 * time.Second,
+				},
+			},
 		},
 	}, {
 		name: "fall back to concurrency model",
@@ -66,6 +83,9 @@ func TestRevisionDefaulting(t *testing.T) {
 			Spec: RevisionSpec{
 				ConcurrencyModel:     "Single",
 				ContainerConcurrency: 1,
+				TimeoutSeconds: &metav1.Duration{
+					Duration: 60 * time.Second,
+				},
 			},
 		},
 	}}

diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go
@@ -178,6 +178,10 @@ type RevisionSpec struct {
 	// https://github.com/knative/serving/issues/627
 	// +optional
 	Container corev1.Container `json:"container,omitempty"`
+
+	// TimeoutSeconds holds the max duration the instance is allowed for responding to a request.
+	// +optional
+	TimeoutSeconds *metav1.Duration `json:"timeoutSeconds,omitempty"`
 }
 
 const (

diff --git a/pkg/apis/serving/v1alpha1/revision_validation.go b/pkg/apis/serving/v1alpha1/revision_validation.go
@@ -18,14 +18,17 @@ package v1alpha1
 
 import (
 	"strconv"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/validation"
 
 	"github.com/knative/pkg/apis"
+	networkingv1alpha1 "github.com/knative/serving/pkg/apis/networking/v1alpha1"
 )
 
 func (rt *Revision) Validate() *apis.FieldError {
@@ -49,9 +52,23 @@ func (rs *RevisionSpec) Validate() *apis.FieldError {
 	} else if err := ValidateContainerConcurrency(rs.ContainerConcurrency, rs.ConcurrencyModel); err != nil {
 		errs = errs.Also(err)
 	}
+
+	if err := validateTimeoutSeconds(rs.TimeoutSeconds); err != nil {
+		errs = errs.Also(err)
+	}
 	return errs
 }
 
+func validateTimeoutSeconds(timeoutSeconds *metav1.Duration) *apis.FieldError {
+	if timeoutSeconds != nil {
+		if timeoutSeconds.Duration > networkingv1alpha1.DefaultTimeout ||
+			timeoutSeconds.Duration < 0*time.Second {
+			return apis.ErrOutOfBoundsValue(timeoutSeconds.Duration.String(), "0s", networkingv1alpha1.DefaultTimeout.String(), "timeoutSeconds")
+		}
+	}
+	return nil
+}
+
 func (ss DeprecatedRevisionServingStateType) Validate() *apis.FieldError {
 	switch ss {
 	case DeprecatedRevisionServingStateType(""),

diff --git a/pkg/apis/serving/v1alpha1/revision_validation_test.go b/pkg/apis/serving/v1alpha1/revision_validation_test.go
@@ -21,6 +21,7 @@ import (
 	"strconv"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/knative/serving/pkg/apis/autoscaling"
@@ -384,6 +385,28 @@ func TestRevisionSpecValidation(t *testing.T) {
 			},
 		},
 		want: apis.ErrDisallowedFields("container.name"),
+	}, {
+		name: "exceed max timeout",
+		rs: &RevisionSpec{
+			Container: corev1.Container{
+				Image: "helloworld",
+			},
+			TimeoutSeconds: &metav1.Duration{
+				Duration: 600 * time.Second,
+			},
+		},
+		want: apis.ErrOutOfBoundsValue("10m0s", "0s", "1m0s", "timeoutSeconds"),
+	}, {
+		name: "negative timeout",
+		rs: &RevisionSpec{
+			Container: corev1.Container{
+				Image: "helloworld",
+			},
+			TimeoutSeconds: &metav1.Duration{
+				Duration: -30 * time.Second,
+			},
+		},
+		want: apis.ErrOutOfBoundsValue("-30s", "0s", "1m0s", "timeoutSeconds"),
 	}}
 
 	for _, test := range tests {

diff --git a/pkg/apis/serving/v1alpha1/service_defaults_test.go b/pkg/apis/serving/v1alpha1/service_defaults_test.go
@@ -18,8 +18,10 @@ package v1alpha1
 
 import (
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestServiceDefaulting(t *testing.T) {
@@ -57,7 +59,11 @@ func TestServiceDefaulting(t *testing.T) {
 				RunLatest: &RunLatestType{
 					Configuration: ConfigurationSpec{
 						RevisionTemplate: RevisionTemplateSpec{
-							Spec: RevisionSpec{},
+							Spec: RevisionSpec{
+								TimeoutSeconds: &metav1.Duration{
+									Duration: 60 * time.Second,
+								},
+							},
 						},
 					},
 				},
@@ -72,6 +78,9 @@ func TestServiceDefaulting(t *testing.T) {
 						RevisionTemplate: RevisionTemplateSpec{
 							Spec: RevisionSpec{
 								ContainerConcurrency: 1,
+								TimeoutSeconds: &metav1.Duration{
+									Duration: 60 * time.Second,
+								},
 							},
 						},
 					},
@@ -85,6 +94,9 @@ func TestServiceDefaulting(t *testing.T) {
 						RevisionTemplate: RevisionTemplateSpec{
 							Spec: RevisionSpec{
 								ContainerConcurrency: 1,
+								TimeoutSeconds: &metav1.Duration{
+									Duration: 60 * time.Second,
+								},
 							},
 						},
 					},
@@ -103,7 +115,11 @@ func TestServiceDefaulting(t *testing.T) {
 				Pinned: &PinnedType{
 					Configuration: ConfigurationSpec{
 						RevisionTemplate: RevisionTemplateSpec{
-							Spec: RevisionSpec{},
+							Spec: RevisionSpec{
+								TimeoutSeconds: &metav1.Duration{
+									Duration: 60 * time.Second,
+								},
+							},
 						},
 					},
 				},
@@ -118,6 +134,9 @@ func TestServiceDefaulting(t *testing.T) {
 						RevisionTemplate: RevisionTemplateSpec{
 							Spec: RevisionSpec{
 								ContainerConcurrency: 1,
+								TimeoutSeconds: &metav1.Duration{
+									Duration: 99 * time.Second,
+								},
 							},
 						},
 					},
@@ -131,6 +150,9 @@ func TestServiceDefaulting(t *testing.T) {
 						RevisionTemplate: RevisionTemplateSpec{
 							Spec: RevisionSpec{
 								ContainerConcurrency: 1,
+								TimeoutSeconds: &metav1.Duration{
+									Duration: 99 * time.Second,
+								},
 							},
 						},
 					},
@@ -149,7 +171,11 @@ func TestServiceDefaulting(t *testing.T) {
 				Release: &ReleaseType{
 					Configuration: ConfigurationSpec{
 						RevisionTemplate: RevisionTemplateSpec{
-							Spec: RevisionSpec{},
+							Spec: RevisionSpec{
+								TimeoutSeconds: &metav1.Duration{
+									Duration: 60 * time.Second,
+								},
+							},
 						},
 					},
 				},
@@ -164,6 +190,9 @@ func TestServiceDefaulting(t *testing.T) {
 						RevisionTemplate: RevisionTemplateSpec{
 							Spec: RevisionSpec{
 								ContainerConcurrency: 1,
+								TimeoutSeconds: &metav1.Duration{
+									Duration: 99 * time.Second,
+								},
 							},
 						},
 					},
@@ -177,6 +206,9 @@ func TestServiceDefaulting(t *testing.T) {
 						RevisionTemplate: RevisionTemplateSpec{
 							Spec: RevisionSpec{
 								ContainerConcurrency: 1,
+								TimeoutSeconds: &metav1.Duration{
+									Duration: 99 * time.Second,
+								},
 							},
 						},
 					},

diff --git a/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go