Set QP resource defaults (knative#14039)

* enforce qp resource defaults

* add flag and several fixes

config/core/configmaps/deployment.yaml

@@ -22,7 +22,7 @@ metadata:
     app.kubernetes.io/component: controller
     app.kubernetes.io/version: devel
   annotations:
-    knative.dev/example-checksum: "410041a0"
+    knative.dev/example-checksum: "ed77183a"
 data:
   # This is the Go import path for the binary that is containerized
   # and substituted here.
@@ -57,15 +57,18 @@ data:
     queue-sidecar-cpu-request: "25m"
 
     # Sets the queue proxy's CPU limit.
-    # If omitted, no value is specified and the system default is used.
+    # If omitted, a default value (currently "1000m"), is used when
+    # `queueproxy.resource-defaults` is set to `Enabled`.
     queue-sidecar-cpu-limit: "1000m"
 
     # Sets the queue proxy's memory request.
-    # If omitted, no value is specified and the system default is used.
+    # If omitted, a default value (currently "400Mi"), is used when
+    # `queueproxy.resource-defaults` is set to `Enabled`.
     queue-sidecar-memory-request: "400Mi"
 
     # Sets the queue proxy's memory limit.
-    # If omitted, no value is specified and the system default is used.
+    # If omitted, a default value (currently "800Mi"), is used when
+    # `queueproxy.resource-defaults` is set to `Enabled`.
     queue-sidecar-memory-limit: "800Mi"
 
     # Sets the queue proxy's ephemeral storage request.

config/core/configmaps/features.yaml

@@ -22,7 +22,7 @@ metadata:
     app.kubernetes.io/component: controller
     app.kubernetes.io/version: devel
   annotations:
-    knative.dev/example-checksum: "d3565159"
+    knative.dev/example-checksum: "eb70e734"
 data:
   _example: |-
     ################################
@@ -201,3 +201,6 @@ data:
     #
     # NOTE THAT THIS IS AN EXPERIMENTAL / ALPHA FEATURE
     queueproxy.mount-podinfo: "disabled"
+
+    # Default queue proxy resource requests and limits to good values for most cases if set.
+    queueproxy.resource-defaults: "disabled"

pkg/apis/config/features.go

@@ -67,6 +67,7 @@ func defaultFeaturesConfig() *Features {
 		PodSpecPersistentVolumeClaim:     Disabled,
 		PodSpecPersistentVolumeWrite:     Disabled,
 		QueueProxyMountPodInfo:           Disabled,
+		QueueProxyResourceDefaults:       Disabled,
 		PodSpecInitContainers:            Disabled,
 		PodSpecDNSPolicy:                 Disabled,
 		PodSpecDNSConfig:                 Disabled,
@@ -102,6 +103,7 @@ func NewFeaturesConfigFromMap(data map[string]string) (*Features, error) {
 		asFlag("kubernetes.podspec-dnsconfig", &nc.PodSpecDNSConfig),
 		asFlag("secure-pod-defaults", &nc.SecurePodDefaults),
 		asFlag("tag-header-based-routing", &nc.TagHeaderBasedRouting),
+		asFlag("queueproxy.resource-defaults", &nc.QueueProxyResourceDefaults),
 		asFlag("queueproxy.mount-podinfo", &nc.QueueProxyMountPodInfo),
 		asFlag("autodetect-http2", &nc.AutoDetectHTTP2)); err != nil {
 		return nil, err
@@ -134,6 +136,7 @@ type Features struct {
 	PodSpecPersistentVolumeClaim     Flag
 	PodSpecPersistentVolumeWrite     Flag
 	QueueProxyMountPodInfo           Flag
+	QueueProxyResourceDefaults       Flag
 	PodSpecDNSPolicy                 Flag
 	PodSpecDNSConfig                 Flag
 	SecurePodDefaults                Flag

pkg/apis/config/features_test.go

@@ -73,6 +73,7 @@ func TestFeaturesConfiguration(t *testing.T) {
 			PodSpecDNSPolicy:                 Enabled,
 			PodSpecDNSConfig:                 Enabled,
 			SecurePodDefaults:                Enabled,
+			QueueProxyResourceDefaults:       Enabled,
 			TagHeaderBasedRouting:            Enabled,
 		}),
 		data: map[string]string{
@@ -90,6 +91,7 @@ func TestFeaturesConfiguration(t *testing.T) {
 			"kubernetes.podspec-dnspolicy":                 "Enabled",
 			"kubernetes.podspec-dnsconfig":                 "Enabled",
 			"secure-pod-defaults":                          "Enabled",
+			"queueproxy.resource-defaults":                 "Enabled",
 			"tag-header-based-routing":                     "Enabled",
 		},
 	}, {

pkg/deployment/config.go

@@ -75,6 +75,26 @@ var (
 	// queue sidecar. It is set at 25m for backwards-compatibility since this was
 	// the historic default before the field was operator-settable.
 	QueueSidecarCPURequestDefault = resource.MustParse("25m")
+
+	// QueueSidecarCPULimitDefault is the default limit.cpu to set for the
+	// queue sidecar.
+	QueueSidecarCPULimitDefault = resource.MustParse("1000m")
+
+	// QueueSidecarMemoryRequestDefault is the default request.memory to set for the
+	// queue sidecar.
+	QueueSidecarMemoryRequestDefault = resource.MustParse("400Mi")
+
+	// QueueSidecarMemoryLimitDefault is the default limit.memory to set for the
+	// queue sidecar.
+	QueueSidecarMemoryLimitDefault = resource.MustParse("800Mi")
+
+	// QueueSidecarEphemeralStorageRequestDefault is the default request.ephemeral-storage set for the
+	// queue sidecar.
+	QueueSidecarEphemeralStorageRequestDefault = resource.MustParse("512Mi")
+
+	// QueueSidecarEphemeralStorageLimitDefault is the default limit.ephemeral-storage to set for the
+	// queue sidecar.
+	QueueSidecarEphemeralStorageLimitDefault = resource.MustParse("1024Mi")
 )
 
 func defaultConfig() *Config {

pkg/reconciler/revision/resources/deploy_test.go

@@ -75,7 +75,7 @@ var (
 
 	defaultQueueContainer = &corev1.Container{
 		Name:      QueueContainerName,
-		Resources: createQueueResources(&deploymentConfig, make(map[string]string), &corev1.Container{}),
+		Resources: createQueueResources(&deploymentConfig, make(map[string]string), &corev1.Container{}, false),
 		Ports:     append(queueNonServingPorts, queueHTTPPort, queueHTTPSPort),
 		ReadinessProbe: &corev1.Probe{
 			ProbeHandler: corev1.ProbeHandler{

pkg/reconciler/revision/resources/queue.go

@@ -89,32 +89,42 @@ var (
 	}
 )
 
-func createQueueResources(cfg *deployment.Config, annotations map[string]string, userContainer *corev1.Container) corev1.ResourceRequirements {
+func createQueueResources(cfg *deployment.Config, annotations map[string]string, userContainer *corev1.Container, useDefaults bool) corev1.ResourceRequirements {
 	resourceRequests := corev1.ResourceList{}
 	resourceLimits := corev1.ResourceList{}
 
 	for _, r := range []struct {
-		Name    corev1.ResourceName
-		Request *resource.Quantity
-		Limit   *resource.Quantity
+		Name           corev1.ResourceName
+		Request        *resource.Quantity
+		RequestDefault *resource.Quantity
+		Limit          *resource.Quantity
+		LimitDefault   *resource.Quantity
 	}{{
-		Name:    corev1.ResourceCPU,
-		Request: cfg.QueueSidecarCPURequest,
-		Limit:   cfg.QueueSidecarCPULimit,
+		Name:           corev1.ResourceCPU,
+		Request:        cfg.QueueSidecarCPURequest,
+		RequestDefault: &deployment.QueueSidecarCPURequestDefault,
+		Limit:          cfg.QueueSidecarCPULimit,
+		LimitDefault:   &deployment.QueueSidecarCPULimitDefault,
 	}, {
-		Name:    corev1.ResourceMemory,
-		Request: cfg.QueueSidecarMemoryRequest,
-		Limit:   cfg.QueueSidecarMemoryLimit,
+		Name:           corev1.ResourceMemory,
+		Request:        cfg.QueueSidecarMemoryRequest,
+		RequestDefault: &deployment.QueueSidecarMemoryRequestDefault,
+		Limit:          cfg.QueueSidecarMemoryLimit,
+		LimitDefault:   &deployment.QueueSidecarMemoryLimitDefault,
 	}, {
 		Name:    corev1.ResourceEphemeralStorage,
 		Request: cfg.QueueSidecarEphemeralStorageRequest,
 		Limit:   cfg.QueueSidecarEphemeralStorageLimit,
 	}} {
 		if r.Request != nil {
 			resourceRequests[r.Name] = *r.Request
+		} else if useDefaults && r.RequestDefault != nil {
+			resourceRequests[r.Name] = *r.RequestDefault
 		}
 		if r.Limit != nil {
 			resourceLimits[r.Name] = *r.Limit
+		} else if useDefaults && r.LimitDefault != nil {
+			resourceLimits[r.Name] = *r.LimitDefault
 		}
 	}
 
@@ -288,10 +298,11 @@ func makeQueueContainer(rev *v1.Revision, cfg *config.Config) (*corev1.Container
 		}
 	}
 
+	useQPResourceDefaults := cfg.Features.QueueProxyResourceDefaults == apicfg.Enabled
 	c := &corev1.Container{
 		Name:            QueueContainerName,
 		Image:           cfg.Deployment.QueueSidecarImage,
-		Resources:       createQueueResources(cfg.Deployment, rev.GetAnnotations(), container),
+		Resources:       createQueueResources(cfg.Deployment, rev.GetAnnotations(), container, useQPResourceDefaults),
 		Ports:           ports,
 		StartupProbe:    execProbe,
 		ReadinessProbe:  httpProbe,

pkg/reconciler/revision/resources/queue_test.go

@@ -312,7 +312,7 @@ func TestMakeQueueContainer(t *testing.T) {
 			})
 		}),
 	}, {
-		name: "default resource config",
+		name: "default resource config with feature qp defaults disabled",
 		rev: revision("bar", "foo",
 			withContainers(containers)),
 		dc: deployment.Config{
@@ -321,9 +321,29 @@ func TestMakeQueueContainer(t *testing.T) {
 		want: queueContainer(func(c *corev1.Container) {
 			c.Env = env(map[string]string{})
 			c.Resources.Requests = corev1.ResourceList{
-				corev1.ResourceCPU: resource.MustParse("25m"),
+				corev1.ResourceCPU: deployment.QueueSidecarCPURequestDefault,
+			}
+		}),
+	}, {
+		name: "resource config with feature qp defaults enabled",
+		rev: revision("bar", "foo",
+			withContainers(containers)),
+		dc: deployment.Config{
+			QueueSidecarCPURequest: &deployment.QueueSidecarCPURequestDefault,
+		},
+		fc: apicfg.Features{
+			QueueProxyResourceDefaults: apicfg.Enabled,
+		},
+		want: queueContainer(func(c *corev1.Container) {
+			c.Env = env(map[string]string{})
+			c.Resources.Requests = corev1.ResourceList{
+				corev1.ResourceCPU:    deployment.QueueSidecarCPURequestDefault,
+				corev1.ResourceMemory: deployment.QueueSidecarMemoryRequestDefault,
+			}
+			c.Resources.Limits = corev1.ResourceList{
+				corev1.ResourceCPU:    deployment.QueueSidecarCPULimitDefault,
+				corev1.ResourceMemory: deployment.QueueSidecarMemoryLimitDefault,
 			}
-			c.Resources.Limits = nil
 		}),
 	}, {
 		name: "overridden resources",