Skip to content

Commit

Permalink
Update flag names to --request and --limit
Browse files Browse the repository at this point in the history 
 - Use singular flag names and support comma separated or repeated flag values
 - Update tests and docs
  • Loading branch information
@navidshaikh
navidshaikh committed Jun 3, 2020
1 parent 3a242c7 commit 8ba2e15
Show file tree
Hide file tree
Showing 9 changed files with 131 additions and 82 deletions.
12 changes: 6 additions & 6 deletions docs/cmd/kn_service_create.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'.
--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.
Expand All @@ -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'.
--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).
Expand Down
12 changes: 6 additions & 6 deletions docs/cmd/kn_service_update.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'.
--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.
Expand All @@ -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'.
--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.
Expand Down
32 changes: 16 additions & 16 deletions pkg/kn/commands/service/configuration_edit_flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,27 +145,27 @@ 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'.")
p.markFlagMakesRevision("limit")
command.Flags().StringSliceVar(&p.Resources.Requests, "request", nil, "The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'.")
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.")
Expand Down Expand Up @@ -352,17 +352,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)
Expand All @@ -378,12 +378,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
}
Expand Down
12 changes: 6 additions & 6 deletions pkg/kn/commands/service/create_mock_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down Expand Up @@ -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()
Expand All @@ -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()
}
Expand Down
10 changes: 5 additions & 5 deletions pkg/kn/commands/service/create_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
71 changes: 43 additions & 28 deletions pkg/kn/flags/resources.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <resourceName1>=<value1>,<resourceName1>=<value2>
// and returns ResourceList.
func populateResourceListV1(spec string) (corev1.ResourceList, error) {
// populateResourceListV1 takes array of strings of form <resourceName1>=<value1>
// 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 <resource>=<value>", 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
}
Loading

0 comments on commit 8ba2e15

Please sign in to comment.