From 0cac223c04f924a47c5d4208d57ed8b24661b7d8 Mon Sep 17 00:00:00 2001 From: Navid Shaikh Date: Wed, 3 Jun 2020 17:37:17 +0530 Subject: [PATCH] Update flag names to --request and --limit (#872) (#873) * Update flag names to --request and --limit - Use singular flag names and support comma separated or repeated flag values - Update tests and docs * Update CHANGELOG * Update the flag description and examples * Remove release 0.15.0 header from CHANGELOG --- CHANGELOG.adoc | 6 +- docs/cmd/kn_service_create.md | 14 ++-- docs/cmd/kn_service_update.md | 16 ++--- .../service/configuration_edit_flags.go | 43 ++++++----- pkg/kn/commands/service/create.go | 2 +- pkg/kn/commands/service/create_mock_test.go | 12 ++-- pkg/kn/commands/service/create_test.go | 10 +-- pkg/kn/commands/service/update.go | 4 +- pkg/kn/flags/resources.go | 71 +++++++++++-------- pkg/kn/flags/resources_test.go | 52 ++++++++++---- pkg/serving/config_changes.go | 10 ++- test/e2e/service_options_test.go | 2 +- 12 files changed, 153 insertions(+), 89 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 31dd48d028..d726ecd8a0 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -12,12 +12,16 @@ | https://github.com/knative/client/pull/[#] //// -## v0.15.0 (2020-06-02) +## v0.15.1 (2020-06-03) [cols="1,10,3", options="header", width="100%"] |=== | | Description | PR +| 🐛 +| Update flag names to --request and --limit +| https://github.com/knative/client/pull/872[#872] + | 🐛 | Fix `kn source -h` | https://github.com/knative/client/pull/846[#846] diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index 329a09d020..8519b29dfc 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -44,7 +44,7 @@ kn service create NAME --image IMAGE [flags] # 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 + kn service create s4gpu --image knativesamples/hellocuda-go --request memory=250Mi,cpu=200m --limit nvidia.com/gpu=1 ``` ### Options @@ -67,9 +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 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). + --limit strings The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource limit, append "-" to the resource name, e.g. '--limit memory-'. + --limits-cpu string DEPRECATED: please use --limit instead. The limits on the requested CPU (e.g., 1000m). + --limits-memory string DEPRECATED: please use --limit 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. @@ -80,9 +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 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). + --request strings The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'. + --requests-cpu string DEPRECATED: please use --request instead. The requested CPU (e.g., 250m). + --requests-memory string DEPRECATED: please use --request 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 a96f2add4e..e4ea08e070 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -20,8 +20,8 @@ kn service update NAME [flags] # Update a service 'svc' with new port kn service update svc --port 80 - # Updates a service 'svc' with new requests and limits parameters - kn service update svc --requests-cpu 500m --limits-memory 1024Mi + # Updates a service 'svc' with new request and limit parameters + kn service update svc --request cpu=500m --limit memory=1024Mi --limit cpu=1000m # Assign tag 'latest' and 'stable' to revisions 'echo-v2' and 'echo-v1' respectively kn service update svc --tag echo-v2=latest --tag echo-v1=stable @@ -54,9 +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 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). + --limit strings The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource limit, append "-" to the resource name, e.g. '--limit memory-'. + --limits-cpu string DEPRECATED: please use --limit instead. The limits on the requested CPU (e.g., 1000m). + --limits-memory string DEPRECATED: please use --limit 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. @@ -67,9 +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 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). + --request strings The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'. + --requests-cpu string DEPRECATED: please use --request instead. The requested CPU (e.g., 250m). + --requests-memory string DEPRECATED: please use --request 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/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 73a1e2131a..a188d694db 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -145,27 +145,38 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { "You can use this flag multiple times.") p.markFlagMakesRevision("arg") - 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().StringSliceVar(&p.Resources.Limits, + "limit", + nil, + "The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'. "+ + "You can use this flag multiple times. "+ + "To unset a resource limit, append \"-\" to the resource name, e.g. '--limit memory-'.") + p.markFlagMakesRevision("limit") + + command.Flags().StringSliceVar(&p.Resources.Requests, + "request", + nil, + "The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. "+ + "You can use this flag multiple times. "+ + "To unset a resource request, append \"-\" to the resource name, e.g. '--request cpu-'.") + p.markFlagMakesRevision("request") command.Flags().StringVar(&p.RequestsFlags.CPU, "requests-cpu", "", - "DEPRECATED: please use --requests instead. The requested CPU (e.g., 250m).") + "DEPRECATED: please use --request instead. The requested CPU (e.g., 250m).") p.markFlagMakesRevision("requests-cpu") command.Flags().StringVar(&p.RequestsFlags.Memory, "requests-memory", "", - "DEPRECATED: please use --requests instead. The requested memory (e.g., 64Mi).") + "DEPRECATED: please use --request instead. The requested memory (e.g., 64Mi).") p.markFlagMakesRevision("requests-memory") // 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).") + "DEPRECATED: please use --limit 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", "", - "DEPRECATED: please use --limits instead. The limits on the requested memory (e.g., 1024Mi).") + "DEPRECATED: please use --limit 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.") @@ -352,17 +363,17 @@ func (p *ConfigurationEditFlags) Apply( } 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") + if cmd.Flags().Changed("limit") { + return fmt.Errorf("only one of (DEPRECATED) --limits-cpu / --limits-memory and --limit 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") + fmt.Fprintf(cmd.OutOrStdout(), "\nWARNING: flags --limits-cpu / --limits-memory are deprecated and going to be removed in future release, please use --limit 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") + if cmd.Flags().Changed("request") { + return fmt.Errorf("only one of (DEPRECATED) --requests-cpu / --requests-memory and --request 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") + fmt.Fprintf(cmd.OutOrStdout(), "\nWARNING: flags --requests-cpu / --requests-memory are deprecated and going to be removed in future release, please use --request instead.\n\n") } limitsResources, err := p.computeResources(p.LimitsFlags) @@ -378,12 +389,12 @@ func (p *ConfigurationEditFlags) Apply( return err } - err = p.Resources.Validate() + requestsToRemove, limitsToRemove, err := p.Resources.Validate() if err != nil { return err } - err = servinglib.UpdateResources(template, p.Resources.ResourceRequirements) + err = servinglib.UpdateResources(template, p.Resources.ResourceRequirements, requestsToRemove, limitsToRemove) if err != nil { return err } diff --git a/pkg/kn/commands/service/create.go b/pkg/kn/commands/service/create.go index 9be6564019..ac7795e7e9 100644 --- a/pkg/kn/commands/service/create.go +++ b/pkg/kn/commands/service/create.go @@ -64,7 +64,7 @@ var create_example = ` # 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` + kn service create s4gpu --image knativesamples/hellocuda-go --request memory=250Mi,cpu=200m --limit 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 ac220292a9..7ab5133b21 100644 --- a/pkg/kn/commands/service/create_mock_test.go +++ b/pkg/kn/commands/service/create_mock_test.go @@ -434,8 +434,8 @@ func TestServiceCreateWithResources(t *testing.T) { 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", + "--request", "cpu=250m,memory=64Mi", + "--limit", "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")) @@ -469,8 +469,8 @@ func TestServiceCreateWithResourcesWarning(t *testing.T) { "--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, "WARNING", "--requests-cpu", "--requests-memory", "deprecated", "removed", "--request", "instead")) + assert.Assert(t, util.ContainsAll(output, "WARNING", "--limits-cpu", "--limits-memory", "deprecated", "removed", "--limit", "instead")) assert.Assert(t, util.ContainsAll(output, "created", "foo", "default")) r.Validate() @@ -482,10 +482,10 @@ func TestServiceCreateWithResourcesError(t *testing.T) { output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", "--requests-memory", "64Mi", - "--requests", "memory=64Mi", + "--request", "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")) + assert.Assert(t, util.ContainsAll(output, "only one of", "DEPRECATED", "--requests-cpu", "--requests-memory", "--request", "can be specified")) r.Validate() } diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go index 31d35447c9..d31ec821cc 100644 --- a/pkg/kn/commands/service/create_test.go +++ b/pkg/kn/commands/service/create_test.go @@ -268,7 +268,7 @@ func TestServiceCreateWithDeprecatedRequests(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", + "--request", "cpu=250m,memory=64Mi", "--no-wait"}, false) if err != nil { @@ -324,7 +324,7 @@ func TestServiceCreateWithDeprecatedLimits(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", + "--limit", "cpu=1000m", "--limit", "memory=1024Mi", "--no-wait"}, false) if err != nil { @@ -391,7 +391,7 @@ func TestServiceCreateDeprecatedRequestsLimitsCPU(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", + "--request", "cpu=250m", "--limit", "cpu=1000m", "--no-wait"}, false) if err != nil { @@ -471,8 +471,8 @@ 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) + "--request", "memory=64Mi", + "--limit", "memory=1024Mi", "--no-wait"}, false) if err != nil { t.Fatal(err) diff --git a/pkg/kn/commands/service/update.go b/pkg/kn/commands/service/update.go index 1d72dc040f..3499472b3a 100644 --- a/pkg/kn/commands/service/update.go +++ b/pkg/kn/commands/service/update.go @@ -36,8 +36,8 @@ var updateExample = ` # Update a service 'svc' with new port kn service update svc --port 80 - # Updates a service 'svc' with new requests and limits parameters - kn service update svc --requests-cpu 500m --limits-memory 1024Mi + # Updates a service 'svc' with new request and limit parameters + kn service update svc --request cpu=500m --limit memory=1024Mi --limit cpu=1000m # Assign tag 'latest' and 'stable' to revisions 'echo-v2' and 'echo-v1' respectively kn service update svc --tag echo-v2=latest --tag echo-v1=stable diff --git a/pkg/kn/flags/resources.go b/pkg/kn/flags/resources.go index bf58f00a44..b703604648 100644 --- a/pkg/kn/flags/resources.go +++ b/pkg/kn/flags/resources.go @@ -15,58 +15,73 @@ package flags import ( - "fmt" - "strings" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + + "knative.dev/client/pkg/util" ) // ResourceOptions to hold the container resource requirements values type ResourceOptions struct { - Requests string - Limits string + 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) +func (o *ResourceOptions) Validate() ([]string, []string, error) { + requests, requestsToRemove, err := populateResourceListV1(o.Requests) if err != nil { - return err + return []string{}, []string{}, err } - o.ResourceRequirements.Limits = limits + o.ResourceRequirements.Requests = requests - requests, err := populateResourceListV1(o.Requests) + limits, limitsToRemove, err := populateResourceListV1(o.Limits) if err != nil { - return err + return []string{}, []string{}, err } - o.ResourceRequirements.Requests = requests - return nil + o.ResourceRequirements.Limits = limits + + return requestsToRemove, limitsToRemove, nil } -// populateResourceListV1 takes strings of form =,= -// and returns ResourceList. -func populateResourceListV1(spec string) (corev1.ResourceList, error) { +// populateResourceListV1 takes array of strings of form = +// and returns ResourceList , an array of resource keys to remove and error if any +func populateResourceListV1(resourceStatements []string) (corev1.ResourceList, []string, error) { // empty input gets a nil response to preserve generator test expected behaviors - if spec == "" { - return nil, nil + if len(resourceStatements) == 0 { + return nil, []string{}, 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) + resources, err := util.MapFromArrayAllowingSingles(resourceStatements, "=") + if err != nil { + return result, []string{}, err + } + + resourcesToRemove := util.ParseMinusSuffix(resources) + + for res, value := range resources { + parse := true + // do not parse the quantity OR throw error if the key is being asked for removal + for _, toRemove := range resourcesToRemove { + if res == toRemove { + parse = false + break + } + } + if !parse { + continue } - resourceName := corev1.ResourceName(parts[0]) - resourceQuantity, err := resource.ParseQuantity(parts[1]) + + resourceQuantity, err := resource.ParseQuantity(value) if err != nil { - return nil, err + return nil, resourcesToRemove, err } - result[resourceName] = resourceQuantity + + result[corev1.ResourceName(res)] = resourceQuantity } - return result, nil + + return result, resourcesToRemove, nil } diff --git a/pkg/kn/flags/resources_test.go b/pkg/kn/flags/resources_test.go index a9aed75ad7..5c25b77c79 100644 --- a/pkg/kn/flags/resources_test.go +++ b/pkg/kn/flags/resources_test.go @@ -23,8 +23,8 @@ import ( ) type resourceOptionsTestCase struct { - requests string - limits string + requests []string + limits []string expectedRequests corev1.ResourceList expectedLimits corev1.ResourceList expectedErr bool @@ -37,49 +37,75 @@ func parseQuantity(value string) resource.Quantity { func TestResourceOptions(t *testing.T) { cases := []*resourceOptionsTestCase{ - {"memory=200Mi,cpu=200m", - "memory=1024Mi,cpu=500m", + {[]string{"memory=200Mi", "cpu=200m"}, + []string{"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", + {[]string{}, + []string{"nvidia.com/gpu=1"}, nil, corev1.ResourceList{corev1.ResourceName("nvidia.com/gpu"): parseQuantity("1")}, false, }, - {"", - "memory:500Mi", + {[]string{}, + []string{"memory:500Mi"}, nil, nil, true, }, - {"memory:200Mi", - "", + {[]string{"memory:200Mi"}, + []string{}, nil, nil, true, }, - {"memory=200MB", - "", + {[]string{"memory=200MB"}, + []string{}, nil, nil, true, }, + {[]string{"cpu=500m", "cpu-"}, + []string{}, + corev1.ResourceList{}, + nil, + false, + }, + {[]string{}, + // resource being asked for removal is considered to be removed and its value isnt validated (200MB which is incorrect) + []string{"memory=200Mi", "cpu=200m", "memory-=200MB"}, + nil, + corev1.ResourceList{corev1.ResourceName("cpu"): parseQuantity("200m")}, + false, + }, } for _, c := range cases { options := &ResourceOptions{} options.Requests = c.requests options.Limits = c.limits - err := options.Validate() + reqToRemove, limToRemove, err := options.Validate() if c.expectedErr { assert.Assert(t, err != nil) } else { + // do the resource removal here as we arent dealing with container template in tests + if len(reqToRemove) > 0 { + for _, req := range reqToRemove { + delete(options.ResourceRequirements.Requests, corev1.ResourceName(req)) + } + } + + if len(limToRemove) > 0 { + for _, lim := range limToRemove { + delete(options.ResourceRequirements.Limits, corev1.ResourceName(lim)) + } + } + 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 5c16096575..71f0513333 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -356,7 +356,7 @@ func UpdateUser(template *servingv1.RevisionTemplateSpec, user int64) error { } // UpdateResources updates container resources for given revision template -func UpdateResources(template *servingv1.RevisionTemplateSpec, resources corev1.ResourceRequirements) error { +func UpdateResources(template *servingv1.RevisionTemplateSpec, resources corev1.ResourceRequirements, requestsToRemove, limitsToRemove []string) error { container, err := ContainerOfRevisionTemplate(template) if err != nil { return err @@ -370,6 +370,10 @@ func UpdateResources(template *servingv1.RevisionTemplateSpec, resources corev1. container.Resources.Requests[k] = v } + for _, reqToRemove := range requestsToRemove { + delete(container.Resources.Requests, corev1.ResourceName(reqToRemove)) + } + if container.Resources.Limits == nil { container.Resources.Limits = corev1.ResourceList{} } @@ -378,6 +382,10 @@ func UpdateResources(template *servingv1.RevisionTemplateSpec, resources corev1. container.Resources.Limits[k] = v } + for _, limToRemove := range limitsToRemove { + delete(container.Resources.Limits, corev1.ResourceName(limToRemove)) + } + return nil } diff --git a/test/e2e/service_options_test.go b/test/e2e/service_options_test.go index 4e6fae5f06..ab3b1bb128 100644 --- a/test/e2e/service_options_test.go +++ b/test/e2e/service_options_test.go @@ -125,7 +125,7 @@ func TestServiceOptions(t *testing.T) { 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") + serviceCreateWithOptions(r, "svc8", "--limit", "memory=500Mi,cpu=1000m", "--request", "memory=250Mi,cpu=200m") test.ValidateServiceResources(r, "svc8", "250Mi", "200m", "500Mi", "1000m") }