Allow resources in Revision

docs/spec/spec.md

@@ -174,6 +174,7 @@ spec:
         - ...
         livenessProbe: ...  # Optional
         readinessProbe: ...  # Optional
+        resources: ...  # Optional
 
       # +optional concurrency strategy.  Defaults to Multi.
       # Deprecated in favor of ContainerConcurrency.
@@ -249,6 +250,7 @@ spec:
     - ...
     livenessProbe: ...  # Optional
     readinessProbe: ...  # Optional
+    resources: ...  # Optional
 
   # Name of the service account the code should run as.
   serviceAccountName: ...
@@ -368,6 +370,7 @@ spec:  # One of "runLatest" or "pinned"
         - ...
         livenessProbe: ...  # Optional
         readinessProbe: ...  # Optional
+        resources: ...  # Optional
       containerConcurrency: ... # Optional
       timeoutSeconds: ...
       serviceAccountName: ...  # Name of the service account the code should run as

pkg/apis/serving/v1alpha1/revision_validation.go

@@ -22,6 +22,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/validation"
 
@@ -106,9 +107,6 @@ func validateContainer(container corev1.Container) *apis.FieldError {
 	if container.Name != "" {
 		ignoredFields = append(ignoredFields, "name")
 	}
-	if !equality.Semantic.DeepEqual(container.Resources, corev1.ResourceRequirements{}) {
-		ignoredFields = append(ignoredFields, "resources")
-	}
 	if len(container.Ports) > 0 {
 		ignoredFields = append(ignoredFields, "ports")
 	}
@@ -189,12 +187,17 @@ func (current *Revision) CheckImmutableFields(og apis.Immutable) *apis.FieldErro
 		return &apis.FieldError{Message: "The provided original was not a Revision"}
 	}
 
-	if diff := cmp.Diff(original.Spec, current.Spec); diff != "" {
+	quantityComparer := cmp.Comparer(func(x, y resource.Quantity) bool {
+		return x.Cmp(y) == 0
+	})
+
+	if diff := cmp.Diff(original.Spec, current.Spec, quantityComparer); diff != "" {
 		return &apis.FieldError{
 			Message: "Immutable fields changed (-old +new)",
 			Paths:   []string{"spec"},
 			Details: diff,
 		}
 	}
+
 	return nil
 }

pkg/apis/serving/v1alpha1/revision_validation_test.go

@@ -62,7 +62,7 @@ func TestContainerValidation(t *testing.T) {
 				},
 			},
 		},
-		want: apis.ErrDisallowedFields("resources"),
+		want: nil,
 	}, {
 		name: "has ports",
 		c: corev1.Container{
@@ -136,11 +136,6 @@ func TestContainerValidation(t *testing.T) {
 		name: "has numerous problems",
 		c: corev1.Container{
 			Name: "foo",
-			Resources: corev1.ResourceRequirements{
-				Requests: corev1.ResourceList{
-					corev1.ResourceName("cpu"): resource.MustParse("25m"),
-				},
-			},
 			Ports: []corev1.ContainerPort{{
 				Name:          "http",
 				ContainerPort: 8080,
@@ -151,7 +146,7 @@ func TestContainerValidation(t *testing.T) {
 			}},
 			Lifecycle: &corev1.Lifecycle{},
 		},
-		want: apis.ErrDisallowedFields("name", "resources", "ports", "volumeMounts", "lifecycle"),
+		want: apis.ErrDisallowedFields("name", "ports", "volumeMounts", "lifecycle"),
 	}}
 
 	for _, test := range tests {
@@ -576,6 +571,40 @@ func TestImmutableFields(t *testing.T) {
 		},
 		old:  &notARevision{},
 		want: &apis.FieldError{Message: "The provided original was not a Revision"},
+	}, {
+		name: "bad (resources image change)",
+		new: &Revision{
+			Spec: RevisionSpec{
+				Container: corev1.Container{
+					Resources: corev1.ResourceRequirements{
+						Requests: corev1.ResourceList{
+							corev1.ResourceName("cpu"): resource.MustParse("50m"),
+						},
+					},
+				},
+				ConcurrencyModel: "Multi",
+			},
+		},
+		old: &Revision{
+			Spec: RevisionSpec{
+				Container: corev1.Container{
+					Resources: corev1.ResourceRequirements{
+						Requests: corev1.ResourceList{
+							corev1.ResourceName("cpu"): resource.MustParse("100m"),
+						},
+					},
+				},
+				ConcurrencyModel: "Multi",
+			},
+		},
+		want: &apis.FieldError{
+			Message: "Immutable fields changed (-old +new)",
+			Paths:   []string{"spec"},
+			Details: `{v1alpha1.RevisionSpec}.Container.Resources.Requests["cpu"]:
+	-: resource.Quantity{i: resource.int64Amount{value: 100, scale: resource.Scale(-3)}, s: "100m", Format: resource.Format("DecimalSI")}
+	+: resource.Quantity{i: resource.int64Amount{value: 50, scale: resource.Scale(-3)}, s: "50m", Format: resource.Format("DecimalSI")}
+`,
+		},
 	}, {
 		name: "bad (container image change)",
 		new: &Revision{

pkg/reconciler/v1alpha1/revision/resources/deploy.go

@@ -95,12 +95,36 @@ func rewriteUserProbe(p *corev1.Probe) {
 	}
 }
 
+// applyDefaultResource
+// Implements a deep merge for ResourceRequirements
+// note: DeepCopyInto cannot be used because it replaces limits or requests instead of merging them
+func applyDefaultResources(defaults corev1.ResourceRequirements, out *corev1.ResourceRequirements) {
+	in := defaults.DeepCopy()
+	if in.Limits != nil {
+		in, out := &in.Limits, &out.Limits
+		for key, val := range *out {
+			(*in)[key] = val.DeepCopy()
+		}
+		(*out) = (*in)
+	}
+	if in.Requests != nil {
+		in, out := &in.Requests, &out.Requests
+		for key, val := range *out {
+			(*in)[key] = val.DeepCopy()
+		}
+		(*out) = (*in)
+	}
+}
+
 func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observabilityConfig *config.Observability, autoscalerConfig *autoscaler.Config, controllerConfig *config.Controller) *corev1.PodSpec {
 	userContainer := rev.Spec.Container.DeepCopy()
 	// Adding or removing an overwritten corev1.Container field here? Don't forget to
 	// update the validations in pkg/webhook.validateContainer.
 	userContainer.Name = userContainerName
-	userContainer.Resources = userResources
+
+	// If client provides for some resources, override default values
+	applyDefaultResources(userResources, &userContainer.Resources)
+
 	userContainer.Ports = userPorts
 	userContainer.VolumeMounts = append(userContainer.VolumeMounts, varLogVolumeMount)
 	userContainer.Lifecycle = userLifecycle

pkg/reconciler/v1alpha1/revision/resources/deploy_test.go

@@ -794,6 +794,16 @@ func TestMakePodSpec(t *testing.T) {
 						Name:  "BAZ",
 						Value: "blah",
 					}},
+					Resources: corev1.ResourceRequirements{
+						Requests: corev1.ResourceList{
+							corev1.ResourceMemory: resource.MustParse("666Mi"),
+							corev1.ResourceCPU:    resource.MustParse("666m"),
+						},
+						Limits: corev1.ResourceList{
+							corev1.ResourceMemory: resource.MustParse("888Mi"),
+							corev1.ResourceCPU:    resource.MustParse("888m"),
+						},
+					},
 				},
 			},
 		},
@@ -826,7 +836,16 @@ func TestMakePodSpec(t *testing.T) {
 					Name:  "K_SERVICE",
 					Value: "",
 				}},
-				Resources:    userResources,
+				Resources: corev1.ResourceRequirements{
+					Requests: corev1.ResourceList{
+						corev1.ResourceMemory: resource.MustParse("666Mi"),
+						corev1.ResourceCPU:    resource.MustParse("666m"),
+					},
+					Limits: corev1.ResourceList{
+						corev1.ResourceMemory: resource.MustParse("888Mi"),
+						corev1.ResourceCPU:    resource.MustParse("888m"),
+					},
+				},
 				Ports:        userPorts,
 				VolumeMounts: []corev1.VolumeMount{varLogVolumeMount},
 				Lifecycle:    userLifecycle,
@@ -872,8 +891,12 @@ func TestMakePodSpec(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
+			quantityComparer := cmp.Comparer(func(x, y resource.Quantity) bool {
+				return x.Cmp(y) == 0
+			})
+
 			got := makePodSpec(test.rev, test.lc, test.oc, test.ac, test.cc)
-			if diff := cmp.Diff(test.want, got, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" {
+			if diff := cmp.Diff(test.want, got, quantityComparer); diff != "" {
 				t.Errorf("makePodSpec (-want, +got) = %v", diff)
 			}
 		})
@@ -1178,3 +1201,86 @@ func TestMakeDeployment(t *testing.T) {
 		})
 	}
 }
+
+func TestApplyDefaultResources(t *testing.T) {
+	tests := []struct {
+		name     string
+		defaults corev1.ResourceRequirements
+		in       *corev1.ResourceRequirements
+		want     *corev1.ResourceRequirements
+	}{
+		{
+			name: "resources are empty",
+			defaults: corev1.ResourceRequirements{
+				Requests: corev1.ResourceList{
+					"resource": resource.MustParse("100m"),
+				},
+				Limits: corev1.ResourceList{
+					"resource": resource.MustParse("100m"),
+				},
+			},
+			in: &corev1.ResourceRequirements{},
+			want: &corev1.ResourceRequirements{
+				Requests: corev1.ResourceList{
+					"resource": resource.MustParse("100m"),
+				},
+				Limits: corev1.ResourceList{
+					"resource": resource.MustParse("100m"),
+				},
+			},
+		},
+		{
+			name: "requests are not empty",
+			defaults: corev1.ResourceRequirements{
+				Requests: corev1.ResourceList{
+					"same":  resource.MustParse("100m"),
+					"other": resource.MustParse("200m"),
+				},
+			},
+			in: &corev1.ResourceRequirements{
+				Requests: corev1.ResourceList{
+					"same": resource.MustParse("500m"),
+					"new":  resource.MustParse("300m"),
+				},
+			},
+			want: &corev1.ResourceRequirements{
+				Requests: corev1.ResourceList{
+					"same":  resource.MustParse("500m"),
+					"new":   resource.MustParse("200m"),
+					"other": resource.MustParse("300m"),
+				},
+			},
+		},
+		{
+			name: "limits are not empty",
+			defaults: corev1.ResourceRequirements{
+				Limits: corev1.ResourceList{
+					"same":  resource.MustParse("100m"),
+					"other": resource.MustParse("200m"),
+				},
+			},
+			in: &corev1.ResourceRequirements{
+				Limits: corev1.ResourceList{
+					"same": resource.MustParse("500m"),
+					"new":  resource.MustParse("300m"),
+				},
+			},
+			want: &corev1.ResourceRequirements{
+				Limits: corev1.ResourceList{
+					"same":  resource.MustParse("500m"),
+					"new":   resource.MustParse("200m"),
+					"other": resource.MustParse("300m"),
+				},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			applyDefaultResources(test.defaults, test.in)
+			if diff := cmp.Diff(test.want, test.in, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" { // Maybe this compare fails
+				t.Errorf("ApplyDefaultResources (-want, +got) = %v", diff)
+			}
+		})
+	}
+}

test/configuration.go

@@ -27,6 +27,7 @@ import (
 type Options struct {
 	EnvVars              []corev1.EnvVar
 	ContainerConcurrency int
+	ContainerResources   corev1.ResourceRequirements
 }
 
 // CreateConfiguration create a configuration resource in namespace with the name names.Config

test/crd.go

@@ -100,7 +100,8 @@ func Configuration(namespace string, names ResourceNames, imagePath string, opti
 			RevisionTemplate: v1alpha1.RevisionTemplateSpec{
 				Spec: v1alpha1.RevisionSpec{
 					Container: corev1.Container{
-						Image: imagePath,
+						Image:     imagePath,
+						Resources: options.ContainerResources,
 					},
 					ContainerConcurrency: v1alpha1.RevisionContainerConcurrencyType(options.ContainerConcurrency),
 				},