From 17da6c1b42e93fad4b39da5ff5704312af0efea2 Mon Sep 17 00:00:00 2001 From: Roberto Jimenez Sanchez Date: Fri, 2 Nov 2018 13:34:14 +0100 Subject: [PATCH] Allow resources in Revision [knative/serving##2099] --- docs/spec/spec.md | 3 + .../serving/v1alpha1/revision_validation.go | 11 +- .../v1alpha1/revision_validation_test.go | 43 ++++- .../v1alpha1/revision/resources/deploy.go | 26 +++- .../revision/resources/deploy_test.go | 110 ++++++++++++- test/configuration.go | 1 + test/crd.go | 3 +- test/e2e/resources_test.go | 147 ++++++++++++++++++ test/test_images/bloatingcow/README.md | 13 ++ test/test_images/bloatingcow/bloatingcow.go | 60 +++++++ 10 files changed, 402 insertions(+), 15 deletions(-) create mode 100644 test/e2e/resources_test.go create mode 100644 test/test_images/bloatingcow/README.md create mode 100644 test/test_images/bloatingcow/bloatingcow.go diff --git a/docs/spec/spec.md b/docs/spec/spec.md index 85889f707570..8738a2752a12 100644 --- a/docs/spec/spec.md +++ b/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 diff --git a/pkg/apis/serving/v1alpha1/revision_validation.go b/pkg/apis/serving/v1alpha1/revision_validation.go index 882682d939d9..34a44e7c4034 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation.go +++ b/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 } diff --git a/pkg/apis/serving/v1alpha1/revision_validation_test.go b/pkg/apis/serving/v1alpha1/revision_validation_test.go index d559d817aa30..b754b0586845 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation_test.go +++ b/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: ¬ARevision{}, 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{ diff --git a/pkg/reconciler/v1alpha1/revision/resources/deploy.go b/pkg/reconciler/v1alpha1/revision/resources/deploy.go index 2008d487a2a4..137e17d4836a 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/deploy.go +++ b/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 diff --git a/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go b/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go index 82b89e105df4..7dcb0e1ee402 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go +++ b/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) + } + }) + } +} diff --git a/test/configuration.go b/test/configuration.go index 895f15c7a42f..ea283167cd47 100644 --- a/test/configuration.go +++ b/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 diff --git a/test/crd.go b/test/crd.go index 1427c9acd65b..eee21954d90a 100644 --- a/test/crd.go +++ b/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), }, diff --git a/test/e2e/resources_test.go b/test/e2e/resources_test.go new file mode 100644 index 000000000000..cf91176eead9 --- /dev/null +++ b/test/e2e/resources_test.go @@ -0,0 +1,147 @@ +// +build e2e + +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package e2e + +import ( + "fmt" + "net/http" + "strings" + "testing" + + pkgTest "github.com/knative/pkg/test" + "github.com/knative/pkg/test/logging" + "github.com/knative/pkg/test/spoof" + "github.com/knative/serving/test" + "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestCustomResourcesLimits(t *testing.T) { + clients = Setup(t) + + //add test case specific name to its own logger + logger = logging.GetContextLogger("TestCustomResourcesLimits") + + var imagePath = test.ImagePath("bloatingcow") + + logger.Infof("Creating a new Route and Configuration") + resources := corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("350Mi"), + }, + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("350Mi"), + }, + } + names, err := CreateRouteAndConfig(clients, logger, imagePath, &test.Options{ContainerResources: resources}) + if err != nil { + t.Fatalf("Failed to create Route and Configuration: %v", err) + } + test.CleanupOnInterrupt(func() { TearDown(clients, names, logger) }, logger) + defer TearDown(clients, names, logger) + + logger.Infof("When the Revision can have traffic routed to it, the Route is marked as Ready.") + if err := test.WaitForRouteState(clients.ServingClient, names.Route, test.IsRouteReady, "RouteIsReady"); err != nil { + t.Fatalf("The Route %s was not marked as Ready to serve traffic: %v", names.Route, err) + } + + route, err := clients.ServingClient.Routes.Get(names.Route, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Error fetching Route %s: %v", names.Route, err) + } + domain := route.Status.Domain + + logger.Info("Waiting for pod list to contain desired resource configuration") + err = test.WaitForPodListState( + clients.KubeClient, + func(p *v1.PodList) (bool, error) { + for _, pod := range p.Items { + if strings.HasPrefix(pod.Name, names.Config) { + for _, c := range pod.Spec.Containers { + if c.Name == "user-container" { + want := corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("350Mi"), + }, + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("350Mi"), + corev1.ResourceCPU: resource.MustParse("400m"), // Default value + }, + } + + if !equality.Semantic.DeepEqual(c.Resources, want) { + return false, fmt.Errorf("Invalid resource configuration for pod %v. Want: %+v, got: %+v.", pod.Name, want, c.Resources) + } else { + return true, nil + } + } + } + } + } + return false, nil + }, + "WaitForAvailablePods") + if err != nil { + logger.Fatalf(`Waiting for Pod.List to have pods with custom resources: %v`, err) + } + + logger.Info("pods are running with the desired configuration.") + + pokeCowForMB := func(mb int) error { + response, err := sendPostRequest(test.ServingFlags.ResolvableDomain, domain, fmt.Sprintf("memory_in_mb=%d", mb)) + if err != nil { + return err + } + want := "Moo!" + if want != strings.TrimSpace(string(response.Body)) { + return fmt.Errorf("The response '%s' is not equal to expected response '%s'.", string(response.Body), want) + } + return nil + } + + logger.Info("Querying the application to see if the memory limits are enforced.") + if err := pokeCowForMB(100); err != nil { + t.Fatalf("Didn't get a response from bloating cow with %d MBs of Memory: %v", 100, err) + } + + if err := pokeCowForMB(200); err != nil { + t.Fatalf("Didn't get a response from bloating cow with %d MBs of Memory: %v", 200, err) + } + + if err := pokeCowForMB(500); err == nil { + t.Fatalf("We shouldn't have got a response from bloating cow with %d MBs of Memory: %v", 500, err) + } +} + +func sendPostRequest(resolvableDomain bool, domain string, query string) (*spoof.Response, error) { + logger.Infof("The domain of request is %s and its query is %s", domain, query) + client, err := pkgTest.NewSpoofingClient(clients.KubeClient, logger, domain, resolvableDomain) + if err != nil { + return nil, err + } + + req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("http://%s?%s", domain, query), nil) + if err != nil { + return nil, err + } + return client.Do(req) +} diff --git a/test/test_images/bloatingcow/README.md b/test/test_images/bloatingcow/README.md new file mode 100644 index 000000000000..cf9a008f4159 --- /dev/null +++ b/test/test_images/bloatingcow/README.md @@ -0,0 +1,13 @@ +# Bloating cow test image + +This directory contains the test image used in the resources e2e test. + +The image contains a simple Go webserver, `bloatingcow.go`, that will, by default, listen on port `8080` and expose a service at `/`. + +When called, the server emits a "Moo!" message, if query parameter `memory_in_mb` is specified, the app will allocated so many Mbs of memory. + +## Building + +For details about building and adding new images, see the [section about test +images](/test/README.md#test-images). + diff --git a/test/test_images/bloatingcow/bloatingcow.go b/test/test_images/bloatingcow/bloatingcow.go new file mode 100644 index 000000000000..da41a3148268 --- /dev/null +++ b/test/test_images/bloatingcow/bloatingcow.go @@ -0,0 +1,60 @@ +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "flag" + "fmt" + "log" + "net/http" + "strconv" +) + +func bloat(mb int) { + if mb == 0 { + return + } + fmt.Printf("Bloat %v Mb of memory.\n", mb) + + b := make([]byte, mb*1024*1024) + b[0] = 1 + b[len(b)-1] = 1 +} + +func handler(w http.ResponseWriter, r *http.Request) { + memoryInMB := r.URL.Query().Get("memory_in_mb") + if memoryInMB != "" { + if mb, err := strconv.Atoi(memoryInMB); err != nil { + fmt.Fprintf(w, "cannot convert %s to int", memoryInMB) + w.WriteHeader(http.StatusBadRequest) + return + } else { + bloat(mb) + } + } + + log.Print("Moo!") + fmt.Fprintln(w, "Moo!") +} + +func main() { + flag.Parse() + log.Print("Bloating cow app started.") + + http.HandleFunc("/", handler) + http.ListenAndServe(":8080", nil) +}