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

@@ -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]

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).

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.

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)
+}

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
 	}

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

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{