From cdf6f29880a7d75d632ed4a4d2b8417e7ee2c07a Mon Sep 17 00:00:00 2001 From: Navid Shaikh Date: Thu, 28 May 2020 14:15:59 +0530 Subject: [PATCH] Add --requests and --limits resource flags (#859) * Add --requests and --limits resource flags Fixes #490 - Add deprecation message for --requests-cpu, --requests-memory, --limits-cpu and --limits-memory flag usage. - Add error message if new and deprecated, requests and limits flags are used together. * Add resources requests and limits example for service create * Add mock style unit tests * Update existing unit tests for deprecated flags * Add integration tests * Add CHANGELOG entry * Fix lint warnings * Add unit tests for resource flags * Fix typo for service name in e2e tests * Refactor resource validation utility into test lib * Remove commented out code * Add references about managing resources and GPU in example * Update image references in examples --- CHANGELOG.adoc | 4 + docs/cmd/kn_service_create.md | 17 ++- docs/cmd/kn_service_update.md | 10 +- lib/test/service.go | 34 +++++ .../service/configuration_edit_flags.go | 53 +++++-- pkg/kn/commands/service/create.go | 7 +- pkg/kn/commands/service/create_mock_test.go | 81 ++++++++++ pkg/kn/commands/service/create_test.go | 143 +++++++++++++++++- pkg/kn/flags/resources.go | 72 +++++++++ pkg/kn/flags/resources_test.go | 86 +++++++++++ pkg/serving/config_changes.go | 30 +++- test/e2e/service_options_test.go | 7 +- 12 files changed, 517 insertions(+), 27 deletions(-) create mode 100644 pkg/kn/flags/resources.go create mode 100644 pkg/kn/flags/resources_test.go diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 9728988b86..c2d5e480a6 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -16,6 +16,10 @@ |=== | | Description | PR +| 🎁 +| Add --requests and --limits flags for resource requirements +| https://github.com/knative/client/pull/859[#859] + | 🐣 | Replaced non-standard errors package with standard library functions | https://github.com/knative/client/pull/853[#853] diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index 9e925718c7..329a09d020 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -39,7 +39,12 @@ kn service create NAME --image IMAGE [flags] kn service create s3 --image knativesamples/helloworld --annotation sidecar.istio.io/inject=false # Create a private service (that is a service with no external endpoint) - kn service create s4 --image knativesamples/helloworld --cluster-local + kn service create s1 --image knativesamples/helloworld --cluster-local + + # Create a service with 250MB memory, 200m CPU requests and a GPU resource limit + # [https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/] + # [https://kubernetes.io/docs/tasks/manage-gpus/scheduling-gpus/] + kn service create s4gpu --image knativesamples/hellocuda-go --requests memory=250Mi,cpu=200m --limits nvidia.com/gpu=1 ``` ### Options @@ -62,8 +67,9 @@ kn service create NAME --image IMAGE [flags] -l, --label stringArray Labels to set for both Service and Revision. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). --label-revision stringArray Revision label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). This flag takes precedence over "label" flag. --label-service stringArray Service label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). This flag takes precedence over "label" flag. - --limits-cpu string The limits on the requested CPU (e.g., 1000m). - --limits-memory string The limits on the requested memory (e.g., 1024Mi). + --limits string The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'. + --limits-cpu string DEPRECATED: please use --limits instead. The limits on the requested CPU (e.g., 1000m). + --limits-memory string DEPRECATED: please use --limits instead. The limits on the requested memory (e.g., 1024Mi). --lock-to-digest Keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision) (default true) --max-scale int Maximal number of replicas. --min-scale int Minimal number of replicas. @@ -74,8 +80,9 @@ kn service create NAME --image IMAGE [flags] --no-wait Do not wait for 'service create' operation to be completed. -p, --port int32 The port where application listens on. --pull-secret string Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace. - --requests-cpu string The requested CPU (e.g., 250m). - --requests-memory string The requested memory (e.g., 64Mi). + --requests string The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. + --requests-cpu string DEPRECATED: please use --requests instead. The requested CPU (e.g., 250m). + --requests-memory string DEPRECATED: please use --requests instead. The requested memory (e.g., 64Mi). --revision-name string The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}") --service-account string Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace. --user int The user ID to run the container (e.g., 1001). diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index 4a4da61b19..a96f2add4e 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -54,8 +54,9 @@ kn service update NAME [flags] -l, --label stringArray Labels to set for both Service and Revision. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). --label-revision stringArray Revision label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). This flag takes precedence over "label" flag. --label-service stringArray Service label to set. name=value; you may provide this flag any number of times to set multiple labels. To unset, specify the label name followed by a "-" (e.g., name-). This flag takes precedence over "label" flag. - --limits-cpu string The limits on the requested CPU (e.g., 1000m). - --limits-memory string The limits on the requested memory (e.g., 1024Mi). + --limits string The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'. + --limits-cpu string DEPRECATED: please use --limits instead. The limits on the requested CPU (e.g., 1000m). + --limits-memory string DEPRECATED: please use --limits instead. The limits on the requested memory (e.g., 1024Mi). --lock-to-digest Keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision) (default true) --max-scale int Maximal number of replicas. --min-scale int Minimal number of replicas. @@ -66,8 +67,9 @@ kn service update NAME [flags] --no-wait Do not wait for 'service update' operation to be completed. -p, --port int32 The port where application listens on. --pull-secret string Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace. - --requests-cpu string The requested CPU (e.g., 250m). - --requests-memory string The requested memory (e.g., 64Mi). + --requests string The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. + --requests-cpu string DEPRECATED: please use --requests instead. The requested CPU (e.g., 250m). + --requests-memory string DEPRECATED: please use --requests instead. The requested memory (e.g., 64Mi). --revision-name string The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}") --service-account string Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace. --tag strings Set tag (format: --tag revisionRef=tagName) where revisionRef can be a revision or '@latest' string representing latest ready revision. This flag can be specified multiple times. diff --git a/lib/test/service.go b/lib/test/service.go index 4224574b27..508e106e83 100644 --- a/lib/test/service.go +++ b/lib/test/service.go @@ -15,7 +15,14 @@ package test import ( + "encoding/json" + "strings" + "gotest.tools/assert" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + servingv1 "knative.dev/serving/pkg/apis/serving/v1" + "knative.dev/client/pkg/util" ) @@ -78,3 +85,30 @@ func ServiceDescribeWithJSONPath(r *KnRunResultCollector, serviceName, jsonpath r.AssertNoError(out) return out.Stdout } + +func ValidateServiceResources(r *KnRunResultCollector, serviceName string, requestsMemory, requestsCPU, limitsMemory, limitsCPU string) { + var err error + rlist := corev1.ResourceList{} + rlist[corev1.ResourceCPU], err = resource.ParseQuantity(requestsCPU) + assert.NilError(r.T(), err) + rlist[corev1.ResourceMemory], err = resource.ParseQuantity(requestsMemory) + assert.NilError(r.T(), err) + + llist := corev1.ResourceList{} + llist[corev1.ResourceCPU], err = resource.ParseQuantity(limitsCPU) + assert.NilError(r.T(), err) + llist[corev1.ResourceMemory], err = resource.ParseQuantity(limitsMemory) + assert.NilError(r.T(), err) + + out := r.KnTest().Kn().Run("service", "describe", serviceName, "-ojson") + data := json.NewDecoder(strings.NewReader(out.Stdout)) + var service servingv1.Service + err = data.Decode(&service) + assert.NilError(r.T(), err) + + serviceRequestResourceList := service.Spec.Template.Spec.Containers[0].Resources.Requests + assert.DeepEqual(r.T(), serviceRequestResourceList, rlist) + + serviceLimitsResourceList := service.Spec.Template.Spec.Containers[0].Resources.Limits + assert.DeepEqual(r.T(), serviceLimitsResourceList, llist) +} diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index a2d101d6d6..73a1e2131a 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -25,7 +25,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "knative.dev/client/pkg/kn/flags" + knflags "knative.dev/client/pkg/kn/flags" servinglib "knative.dev/client/pkg/serving" "knative.dev/client/pkg/util" "knative.dev/serving/pkg/apis/serving" @@ -43,7 +43,8 @@ type ConfigurationEditFlags struct { Command string Arg []string - RequestsFlags, LimitsFlags ResourceFlags + RequestsFlags, LimitsFlags ResourceFlags // TODO: Flag marked deprecated in release v0.15.0, remove in release v0.18.0 + Resources knflags.ResourceOptions MinScale int MaxScale int ConcurrencyTarget int @@ -144,17 +145,27 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { "You can use this flag multiple times.") p.markFlagMakesRevision("arg") - command.Flags().StringVar(&p.RequestsFlags.CPU, "requests-cpu", "", "The requested CPU (e.g., 250m).") + command.Flags().StringVar(&p.Resources.Limits, "limits", "", "The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'.") + p.markFlagMakesRevision("limits") + command.Flags().StringVar(&p.Resources.Requests, "requests", "", "The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'.") + p.markFlagMakesRevision("requests") + + command.Flags().StringVar(&p.RequestsFlags.CPU, "requests-cpu", "", + "DEPRECATED: please use --requests instead. The requested CPU (e.g., 250m).") p.markFlagMakesRevision("requests-cpu") - command.Flags().StringVar(&p.RequestsFlags.Memory, "requests-memory", "", "The requested memory (e.g., 64Mi).") + command.Flags().StringVar(&p.RequestsFlags.Memory, "requests-memory", "", + "DEPRECATED: please use --requests instead. The requested memory (e.g., 64Mi).") p.markFlagMakesRevision("requests-memory") - command.Flags().StringVar(&p.LimitsFlags.CPU, "limits-cpu", "", "The limits on the requested CPU (e.g., 1000m).") + // TODO: Flag marked deprecated in release v0.15.0, remove in release v0.18.0 + command.Flags().StringVar(&p.LimitsFlags.CPU, "limits-cpu", "", + "DEPRECATED: please use --limits instead. The limits on the requested CPU (e.g., 1000m).") p.markFlagMakesRevision("limits-cpu") + // TODO: Flag marked deprecated in release v0.15.0, remove in release v0.18.0 command.Flags().StringVar(&p.LimitsFlags.Memory, "limits-memory", "", - "The limits on the requested memory (e.g., 1024Mi).") + "DEPRECATED: please use --limits instead. The limits on the requested memory (e.g., 1024Mi).") p.markFlagMakesRevision("limits-memory") command.Flags().IntVar(&p.MinScale, "min-scale", 0, "Minimal number of replicas.") @@ -166,7 +177,7 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { command.Flags().StringVar(&p.AutoscaleWindow, "autoscale-window", "", "Duration to look back for making auto-scaling decisions. The service is scaled to zero if no request was received in during that time. (eg: 10s)") p.markFlagMakesRevision("autoscale-window") - flags.AddBothBoolFlagsUnhidden(command.Flags(), &p.ClusterLocal, "cluster-local", "", false, + knflags.AddBothBoolFlagsUnhidden(command.Flags(), &p.ClusterLocal, "cluster-local", "", false, "Specify that the service be private. (--no-cluster-local will make the service publicly available)") //TODO: Need to also not change revision when already set (solution to issue #646) p.markFlagMakesRevision("cluster-local") @@ -214,7 +225,7 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { "{{.Generation}} for the generation, and {{.Random [n]}} for n random consonants.") p.markFlagMakesRevision("revision-name") - flags.AddBothBoolFlagsUnhidden(command.Flags(), &p.LockToDigest, "lock-to-digest", "", true, + knflags.AddBothBoolFlagsUnhidden(command.Flags(), &p.LockToDigest, "lock-to-digest", "", true, "Keep the running image for the service constant when not explicitly specifying "+ "the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)") // Don't mark as changing the revision. @@ -340,6 +351,20 @@ func (p *ConfigurationEditFlags) Apply( servinglib.UnsetUserImageAnnot(template) } + if cmd.Flags().Changed("limits-cpu") || cmd.Flags().Changed("limits-memory") { + if cmd.Flags().Changed("limits") { + return fmt.Errorf("only one of (DEPRECATED) --limits-cpu / --limits-memory and --limits can be specified") + } + fmt.Fprintf(cmd.OutOrStdout(), "\nWARNING: flags --limits-cpu / --limits-memory are deprecated and going to be removed in future release, please use --limits instead.\n\n") + } + + if cmd.Flags().Changed("requests-cpu") || cmd.Flags().Changed("requests-memory") { + if cmd.Flags().Changed("requests") { + return fmt.Errorf("only one of (DEPRECATED) --requests-cpu / --requests-memory and --requests can be specified") + } + fmt.Fprintf(cmd.OutOrStdout(), "\nWARNING: flags --requests-cpu / --requests-memory are deprecated and going to be removed in future release, please use --requests instead.\n\n") + } + limitsResources, err := p.computeResources(p.LimitsFlags) if err != nil { return err @@ -348,7 +373,17 @@ func (p *ConfigurationEditFlags) Apply( if err != nil { return err } - err = servinglib.UpdateResources(template, requestsResources, limitsResources) + err = servinglib.UpdateResourcesDeprecated(template, requestsResources, limitsResources) + if err != nil { + return err + } + + err = p.Resources.Validate() + if err != nil { + return err + } + + err = servinglib.UpdateResources(template, p.Resources.ResourceRequirements) if err != nil { return err } diff --git a/pkg/kn/commands/service/create.go b/pkg/kn/commands/service/create.go index caf89adebb..9be6564019 100644 --- a/pkg/kn/commands/service/create.go +++ b/pkg/kn/commands/service/create.go @@ -59,7 +59,12 @@ var create_example = ` kn service create s3 --image knativesamples/helloworld --annotation sidecar.istio.io/inject=false # Create a private service (that is a service with no external endpoint) - kn service create s4 --image knativesamples/helloworld --cluster-local` + kn service create s1 --image knativesamples/helloworld --cluster-local + + # Create a service with 250MB memory, 200m CPU requests and a GPU resource limit + # [https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/] + # [https://kubernetes.io/docs/tasks/manage-gpus/scheduling-gpus/] + kn service create s4gpu --image knativesamples/hellocuda-go --requests memory=250Mi,cpu=200m --limits nvidia.com/gpu=1` func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command { var editFlags ConfigurationEditFlags diff --git a/pkg/kn/commands/service/create_mock_test.go b/pkg/kn/commands/service/create_mock_test.go index 37500b55c5..ac220292a9 100644 --- a/pkg/kn/commands/service/create_mock_test.go +++ b/pkg/kn/commands/service/create_mock_test.go @@ -409,6 +409,87 @@ func TestServiceCreateWithUser(t *testing.T) { r.Validate() } +func TestServiceCreateWithResources(t *testing.T) { + client := knclient.NewMockKnServiceClient(t) + + r := client.Recorder() + r.GetService("foo", nil, errors.NewNotFound(servingv1.Resource("service"), "foo")) + + service := getService("foo") + template := &service.Spec.Template + + template.Spec.Containers[0].Resources.Requests = corev1.ResourceList{ + corev1.ResourceCPU: parseQuantity(t, "250m"), + corev1.ResourceMemory: parseQuantity(t, "64Mi"), + } + + template.Spec.Containers[0].Resources.Limits = corev1.ResourceList{ + corev1.ResourceCPU: parseQuantity(t, "1000m"), + corev1.ResourceMemory: parseQuantity(t, "1024Mi"), + corev1.ResourceName("nvidia.com/gpu"): parseQuantity(t, "1"), + } + + template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz" + template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"} + r.CreateService(service, nil) + + output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", + "--requests", "cpu=250m,memory=64Mi", + "--limits", "cpu=1000m,memory=1024Mi,nvidia.com/gpu=1", + "--no-wait", "--revision-name=") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(output, "created", "foo", "default")) + + r.Validate() +} + +func TestServiceCreateWithResourcesWarning(t *testing.T) { + client := knclient.NewMockKnServiceClient(t) + + r := client.Recorder() + r.GetService("foo", nil, errors.NewNotFound(servingv1.Resource("service"), "foo")) + + service := getService("foo") + template := &service.Spec.Template + + template.Spec.Containers[0].Resources.Requests = corev1.ResourceList{ + corev1.ResourceMemory: parseQuantity(t, "64Mi"), + } + + template.Spec.Containers[0].Resources.Limits = corev1.ResourceList{ + corev1.ResourceCPU: parseQuantity(t, "1000m"), + } + + template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz" + template.Annotations = map[string]string{servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz"} + r.CreateService(service, nil) + + output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", + "--requests-memory", "64Mi", + "--limits-cpu", "1000m", + "--no-wait", "--revision-name=") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(output, "WARNING", "--requests-cpu", "--requests-memory", "deprecated", "removed", "--requests", "instead")) + assert.Assert(t, util.ContainsAll(output, "WARNING", "--limits-cpu", "--limits-memory", "deprecated", "removed", "--limits", "instead")) + assert.Assert(t, util.ContainsAll(output, "created", "foo", "default")) + + r.Validate() +} + +func TestServiceCreateWithResourcesError(t *testing.T) { + client := knclient.NewMockKnServiceClient(t) + r := client.Recorder() + + output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", + "--requests-memory", "64Mi", + "--requests", "memory=64Mi", + "--no-wait", "--revision-name=") + assert.Assert(t, err != nil) + assert.Assert(t, util.ContainsAll(output, "only one of", "DEPRECATED", "--requests-cpu", "--requests-memory", "--requests", "can be specified")) + + r.Validate() +} + func getService(name string) *servingv1.Service { service := &servingv1.Service{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go index 568bbcbb62..31d35447c9 100644 --- a/pkg/kn/commands/service/create_test.go +++ b/pkg/kn/commands/service/create_test.go @@ -237,7 +237,7 @@ func TestServiceCreateEnv(t *testing.T) { } } -func TestServiceCreateWithRequests(t *testing.T) { +func TestServiceCreateWithDeprecatedRequests(t *testing.T) { action, created, _, err := fakeServiceCreate([]string{ "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--requests-cpu", "250m", "--requests-memory", "64Mi", @@ -265,7 +265,35 @@ func TestServiceCreateWithRequests(t *testing.T) { } } -func TestServiceCreateWithLimits(t *testing.T) { +func TestServiceCreateWithRequests(t *testing.T) { + action, created, _, err := fakeServiceCreate([]string{ + "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", + "--requests", "cpu=250m,memory=64Mi", + "--no-wait"}, false) + + if err != nil { + t.Fatal(err) + } else if !action.Matches("create", "services") { + t.Fatalf("Bad action %v", action) + } + + expectedRequestsVars := corev1.ResourceList{ + corev1.ResourceCPU: parseQuantity(t, "250m"), + corev1.ResourceMemory: parseQuantity(t, "64Mi"), + } + + template := &created.Spec.Template + + if err != nil { + t.Fatal(err) + } else if !reflect.DeepEqual( + template.Spec.Containers[0].Resources.Requests, + expectedRequestsVars) { + t.Fatalf("wrong requests vars %v", template.Spec.Containers[0].Resources.Requests) + } +} + +func TestServiceCreateWithDeprecatedLimits(t *testing.T) { action, created, _, err := fakeServiceCreate([]string{ "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--limits-cpu", "1000m", "--limits-memory", "1024Mi", @@ -293,7 +321,35 @@ func TestServiceCreateWithLimits(t *testing.T) { } } -func TestServiceCreateRequestsLimitsCPU(t *testing.T) { +func TestServiceCreateWithLimits(t *testing.T) { + action, created, _, err := fakeServiceCreate([]string{ + "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", + "--limits", "cpu=1000m,memory=1024Mi", + "--no-wait"}, false) + + if err != nil { + t.Fatal(err) + } else if !action.Matches("create", "services") { + t.Fatalf("Bad action %v", action) + } + + expectedLimitsVars := corev1.ResourceList{ + corev1.ResourceCPU: parseQuantity(t, "1000m"), + corev1.ResourceMemory: parseQuantity(t, "1024Mi"), + } + + template := &created.Spec.Template + + if err != nil { + t.Fatal(err) + } else if !reflect.DeepEqual( + template.Spec.Containers[0].Resources.Limits, + expectedLimitsVars) { + t.Fatalf("wrong limits vars %v", template.Spec.Containers[0].Resources.Limits) + } +} + +func TestServiceCreateDeprecatedRequestsLimitsCPU(t *testing.T) { action, created, _, err := fakeServiceCreate([]string{ "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--requests-cpu", "250m", "--limits-cpu", "1000m", @@ -332,7 +388,46 @@ func TestServiceCreateRequestsLimitsCPU(t *testing.T) { } } -func TestServiceCreateRequestsLimitsMemory(t *testing.T) { +func TestServiceCreateRequestsLimitsCPU(t *testing.T) { + action, created, _, err := fakeServiceCreate([]string{ + "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", + "--requests", "cpu=250m", "--limits", "cpu=1000m", + "--no-wait"}, false) + + if err != nil { + t.Fatal(err) + } else if !action.Matches("create", "services") { + t.Fatalf("Bad action %v", action) + } + + expectedRequestsVars := corev1.ResourceList{ + corev1.ResourceCPU: parseQuantity(t, "250m"), + } + + expectedLimitsVars := corev1.ResourceList{ + corev1.ResourceCPU: parseQuantity(t, "1000m"), + } + + template := &created.Spec.Template + + if err != nil { + t.Fatal(err) + } else { + if !reflect.DeepEqual( + template.Spec.Containers[0].Resources.Requests, + expectedRequestsVars) { + t.Fatalf("wrong requests vars %v", template.Spec.Containers[0].Resources.Requests) + } + + if !reflect.DeepEqual( + template.Spec.Containers[0].Resources.Limits, + expectedLimitsVars) { + t.Fatalf("wrong limits vars %v", template.Spec.Containers[0].Resources.Limits) + } + } +} + +func TestServiceCreateDeprecatedRequestsLimitsMemory(t *testing.T) { action, created, _, err := fakeServiceCreate([]string{ "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", @@ -372,6 +467,46 @@ func TestServiceCreateRequestsLimitsMemory(t *testing.T) { } } +func TestServiceCreateRequestsLimitsMemory(t *testing.T) { + action, created, _, err := fakeServiceCreate([]string{ + "service", "create", "foo", + "--image", "gcr.io/foo/bar:baz", + "--requests", "memory=64Mi", + "--limits", "memory=1024Mi", "--no-wait"}, false) + + if err != nil { + t.Fatal(err) + } else if !action.Matches("create", "services") { + t.Fatalf("Bad action %v", action) + } + + expectedRequestsVars := corev1.ResourceList{ + corev1.ResourceMemory: parseQuantity(t, "64Mi"), + } + + expectedLimitsVars := corev1.ResourceList{ + corev1.ResourceMemory: parseQuantity(t, "1024Mi"), + } + + template := &created.Spec.Template + + if err != nil { + t.Fatal(err) + } else { + if !reflect.DeepEqual( + template.Spec.Containers[0].Resources.Requests, + expectedRequestsVars) { + t.Fatalf("wrong requests vars %v", template.Spec.Containers[0].Resources.Requests) + } + + if !reflect.DeepEqual( + template.Spec.Containers[0].Resources.Limits, + expectedLimitsVars) { + t.Fatalf("wrong limits vars %v", template.Spec.Containers[0].Resources.Limits) + } + } +} + func TestServiceCreateMaxMinScale(t *testing.T) { action, created, _, err := fakeServiceCreate([]string{ "service", "create", "foo", "--image", "gcr.io/foo/bar:baz", diff --git a/pkg/kn/flags/resources.go b/pkg/kn/flags/resources.go new file mode 100644 index 0000000000..bf58f00a44 --- /dev/null +++ b/pkg/kn/flags/resources.go @@ -0,0 +1,72 @@ +// Copyright © 2020 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 flags + +import ( + "fmt" + "strings" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +// ResourceOptions to hold the container resource requirements values +type ResourceOptions struct { + Requests string + Limits string + ResourceRequirements corev1.ResourceRequirements +} + +// Validate parses the limits and requests parameters if specified and +// sets ResourceRequirements for ResourceOptions or returns error if any +func (o *ResourceOptions) Validate() (err error) { + limits, err := populateResourceListV1(o.Limits) + if err != nil { + return err + } + o.ResourceRequirements.Limits = limits + + requests, err := populateResourceListV1(o.Requests) + if err != nil { + return err + } + o.ResourceRequirements.Requests = requests + return nil +} + +// populateResourceListV1 takes strings of form =,= +// and returns ResourceList. +func populateResourceListV1(spec string) (corev1.ResourceList, error) { + // empty input gets a nil response to preserve generator test expected behaviors + if spec == "" { + return nil, nil + } + + result := corev1.ResourceList{} + resourceStatements := strings.Split(spec, ",") + for _, resourceStatement := range resourceStatements { + parts := strings.Split(resourceStatement, "=") + if len(parts) != 2 { + return nil, fmt.Errorf("invalid argument syntax %v, expected =", resourceStatement) + } + resourceName := corev1.ResourceName(parts[0]) + resourceQuantity, err := resource.ParseQuantity(parts[1]) + if err != nil { + return nil, err + } + result[resourceName] = resourceQuantity + } + return result, nil +} diff --git a/pkg/kn/flags/resources_test.go b/pkg/kn/flags/resources_test.go new file mode 100644 index 0000000000..a9aed75ad7 --- /dev/null +++ b/pkg/kn/flags/resources_test.go @@ -0,0 +1,86 @@ +// Copyright © 2020 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 flags + +import ( + "testing" + + "gotest.tools/assert" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +type resourceOptionsTestCase struct { + requests string + limits string + expectedRequests corev1.ResourceList + expectedLimits corev1.ResourceList + expectedErr bool +} + +func parseQuantity(value string) resource.Quantity { + q, _ := resource.ParseQuantity(value) + return q +} + +func TestResourceOptions(t *testing.T) { + cases := []*resourceOptionsTestCase{ + {"memory=200Mi,cpu=200m", + "memory=1024Mi,cpu=500m", + corev1.ResourceList{corev1.ResourceMemory: parseQuantity("200Mi"), + corev1.ResourceCPU: parseQuantity("200m")}, + corev1.ResourceList{corev1.ResourceMemory: parseQuantity("1024Mi"), + corev1.ResourceCPU: parseQuantity("500m")}, + false, + }, + {"", + "nvidia.com/gpu=1", + nil, + corev1.ResourceList{corev1.ResourceName("nvidia.com/gpu"): parseQuantity("1")}, + false, + }, + + {"", + "memory:500Mi", + nil, + nil, + true, + }, + {"memory:200Mi", + "", + nil, + nil, + true, + }, + {"memory=200MB", + "", + nil, + nil, + true, + }, + } + for _, c := range cases { + options := &ResourceOptions{} + options.Requests = c.requests + options.Limits = c.limits + err := options.Validate() + + if c.expectedErr { + assert.Assert(t, err != nil) + } else { + assert.DeepEqual(t, options.ResourceRequirements, corev1.ResourceRequirements{Limits: c.expectedLimits, Requests: c.expectedRequests}) + } + } +} diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 9af4aa7a9f..5c16096575 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -355,8 +355,34 @@ func UpdateUser(template *servingv1.RevisionTemplateSpec, user int64) error { return nil } -// UpdateResources updates resources as requested -func UpdateResources(template *servingv1.RevisionTemplateSpec, requestsResourceList corev1.ResourceList, limitsResourceList corev1.ResourceList) error { +// UpdateResources updates container resources for given revision template +func UpdateResources(template *servingv1.RevisionTemplateSpec, resources corev1.ResourceRequirements) error { + container, err := ContainerOfRevisionTemplate(template) + if err != nil { + return err + } + + if container.Resources.Requests == nil { + container.Resources.Requests = corev1.ResourceList{} + } + + for k, v := range resources.Requests { + container.Resources.Requests[k] = v + } + + if container.Resources.Limits == nil { + container.Resources.Limits = corev1.ResourceList{} + } + + for k, v := range resources.Limits { + container.Resources.Limits[k] = v + } + + return nil +} + +// UpdateResourcesDeprecated updates resources as requested +func UpdateResourcesDeprecated(template *servingv1.RevisionTemplateSpec, requestsResourceList corev1.ResourceList, limitsResourceList corev1.ResourceList) error { container, err := ContainerOfRevisionTemplate(template) if err != nil { return err diff --git a/test/e2e/service_options_test.go b/test/e2e/service_options_test.go index 884cc3aabb..4e6fae5f06 100644 --- a/test/e2e/service_options_test.go +++ b/test/e2e/service_options_test.go @@ -26,11 +26,10 @@ import ( "testing" "gotest.tools/assert" + servingv1 "knative.dev/serving/pkg/apis/serving/v1" "knative.dev/client/lib/test" "knative.dev/client/pkg/util" - - servingv1 "knative.dev/serving/pkg/apis/serving/v1" ) func TestServiceOptions(t *testing.T) { @@ -124,6 +123,10 @@ func TestServiceOptions(t *testing.T) { t.Log("create and validate service and revision labels") serviceCreateWithOptions(r, "svc7", "--label-service", "svc=helloworld-svc", "--label-revision", "rev=helloworld-rev") validateLabels(r, "svc7", map[string]string{"svc": "helloworld-svc"}, map[string]string{"rev": "helloworld-rev"}) + + t.Log("create and validate service resource options") + serviceCreateWithOptions(r, "svc8", "--limits", "memory=500Mi,cpu=1000m", "--requests", "memory=250Mi,cpu=200m") + test.ValidateServiceResources(r, "svc8", "250Mi", "200m", "500Mi", "1000m") } func serviceCreateWithOptions(r *test.KnRunResultCollector, serviceName string, options ...string) {