feat(service create/update): Add support of minScale/maxScale/target #157

rhuss · 2019-06-02T15:18:09Z

Autoscaler annotations are added to the revision template if
--min-scale / --max-scale / --target are provided to kn service create or
kn service update

Fixes #151

This PR builds on top of #156 but can be back-ported on master if desired.
For review I recommend to look at commit efb23b7 first, which contains the delta.

The PR is now based on upstream master only.

knative-prow-robot · 2019-06-02T15:18:24Z

Hi @rhuss. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rhuss · 2019-06-02T15:18:59Z

/ok-to-test

rhuss · 2019-06-02T15:19:22Z

/assign sixolet

knative-prow-robot · 2019-06-02T15:19:26Z

@rhuss: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cppforlife · 2019-06-03T17:04:23Z

/ok-to-test

rhuss · 2019-06-03T18:03:52Z

Integration test should be fixed if #155 is merged and this PR rebased on top of it

pkg/kn/commands/service_create.go

duglin · 2019-06-04T00:05:58Z

docs/cmd/kn_service_create.md

  -n, --namespace string         List the requested object(s) in given namespace.
      --requests-cpu string      The requested CPU (e.g., 250m).
      --requests-memory string   The requested CPU (e.g., 64Mi).
+      --target int               Number of in-flight requests per pod.


The issues talks about containerConcurrency, so I think this should be named something like that. The "target" config option is different - we can add a flag for that too if people want it, but my original request is for CC.

tbh, I think we can add all autoscaling processing annotations in one go. But I'm fine of course to remove target if we agree that this is not something we want to expose via the CLI (althouhg I believe every part of the public Knative API which is put into annotation should be available as CLI option as this is hard to find out elsewhere as it is not part of the CRD schema and nobody memorizes the full name of annotation keys)

I'm +1 for exposing all of them too - I just didn't want this PR to go thru w/o addressing what the issue was asking for :-)

got you, valid point.

The word target (unless I am missing something or has historical meaning) is vague and not meaningful in this context? Why can't this be no-in-flights or similar?

were you going to add that flag?

These concurrency flags are added to kn service create and kn service update. There is the general problem that for changing these annotations on update, this will trigger a new revision, and don't touch the old revision. This means if you have used minScale then this revision will always spin up that many pods even when its not the latest revision anymore (revisions are immutable). The only thing to get this back to 0 is to fiddle around with the autoscaler CRD. There is some discussions over there to make metadata like annotations mutable on revision instances, but as long as this doesn't happen we have to deal with that (i.e. don't modify the annotation on the running Revision but on the Service/Configuration's Revision template). See knative/serving#3547 (comment) for the discussion over there in serving.

For the naming:

I like --min-scale and --max-scale as it's concise and pretty clear what it means. Alternatively we could call it --min-replicas & --max-replicas to reflect the Deployment fields affected (or even --min-pods and --max-pods). However this suggestion again connects to K8s which we shouldn't do ("replicas" and even more "pods" is a concept occupied by K8s). So I'm fine with --min-scale & --max-scale.

Another option would be --scale-max and --scale-min which would put the options nicely together, e.g. in an alphabetically ordered help options list or documentation. This is important when your option list gets long (ever tried jx create cluster minikube --help ;-P), and I think for create and update it will still grow.

For --target I agree that it is to unspecific as an option name.

My suggestions would be (with preference in that order):

--scale-target 10 (would fit to a --scale-min and --scale-max (but could be misread that it's a target to reach 10, but afaiu its just the number of concurrent requests after that a scale up event happen)

--scale-after-requests 10 (which reads nicely like in scale after 10 request)

--scale-after 10

--concurrency-target 10

--scale-in-flight 10(not sure whether this clear enough)

--scale-after-concurrent-requests 10

For clarity, containerConcurrency and the target annotation are two different things. The first is an enforced limit, the second is a "soft" target for the autoscaler. In the absence of the second, the second is inferred from the first.

In that sense I'd opt for:

--concurrency-limit to define the former

--concurrency-target to define the latter. We should encode concurrency into here as for the KPA the default will be concurrency. If we also want to expose the HPA as an autoscaler, things will get more tricky here though.

I think that nicely encodes the difference in the name. The CLI docs could say that --concurrency-target defaults to --concurrency-limit if not passed.

Ah, I see. There can be different types of autoscaling triggers, which currently is based on concurrent requests (but could be e.g. CPU consumption when using the HPA). But maxScale and minScale are more a global scaling (boundary) option which applies regardless of the type of scaling.

So my suggestion now would be:

--max-scale and --min-scale (as in this PR)

--concurrency-target (renaming the option)

--concurrency-limit (adding to this PR, by setting the proper field in the spec)

@markusthoemmes Just saw that containerConcurrency is part of PodAutoscaler which is not part of the public API. So is it correct, that this datum can't be changed from a client like kn ? Or is there a corresponding field on Service which the get put into the PodAutoscaler resource when a service is created ?

For now, I think for now we have to ommit --concurrency-limit

rhuss · 2019-06-06T09:49:46Z

@cppforlife @sixolet From my POV we are good for review and eventually merge with this PR.

Rebased to latest refactoring
Introduced --min-scale, --max-scale, --concurrency-target for service create and service update
Cleaned up and backported this feature on master only (was based on another PR before)

The introduction of --concurrency-limit is postponed until we know how to set this on a PodAutoscaler resource (if this possible at all).

duglin · 2019-06-06T10:22:24Z

containerConcurrency is part of a KnService so kn should expose the option

…

Sent from my iPad

On Jun 6, 2019, at 5:05 AM, Roland Huß ***@***.***> wrote: @rhuss commented on this pull request. In docs/cmd/kn_service_create.md: > -n, --namespace string List the requested object(s) in given namespace. --requests-cpu string The requested CPU (e.g., 250m). --requests-memory string The requested CPU (e.g., 64Mi). + --target int Number of in-flight requests per pod. @markusthoemmes Just saw that containerConcurrency is part of PodAutoscaler which is not part of the public API. So is it correct, that this datum can't be changed from a client like kn ? Or is there a corresponding field on Service which the get put into the PodAutoscaler resource when a service is created ? For now, I think for now we have to ommit --concurrency-limit — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

rhuss · 2019-06-06T10:30:28Z

@duglin Ah, I see. It's on the RevisionSpec, not ServiceSpec. Thanks for the heads up. So I will add --concurrency-limit now, too.

duglin · 2019-06-06T12:05:04Z

@duglin Ah, I see. It's on the RevisionSpec, not ServiceSpec. Thanks for the heads up. So I will add --concurrency-limit now, too.

Thanks!!

sixolet

Mostly good. A little bit I'd like to see moved to a separate PR.

sixolet · 2019-06-07T00:15:22Z

pkg/kn/commands/service/service_update.go

@@ -36,8 +37,9 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
  kn service update mysvc --requests-cpu 500m --limits-memory 1024Mi`,
 		RunE: func(cmd *cobra.Command, args []string) (err error) {
 			if len(args) != 1 {
-				return errors.New("requires the service name.")
+				return errors.New("'kn update' requires the service name")


This isn't an accurate rendition of the actual command, and these changes seem unrelated to what you're doing in the rest of the PR. Can you make a separate PR?

fair point, will do

cleaned up and rebased

…ency-target/concurrency-limit Autoscaler concurrency annotations are added to the revision template if --min-scale / --max-scale / --concurrency-target/--concurrency-limit are provided to `kn service create` and `kn service update` Fixes knative#151

knative-metrics-robot · 2019-06-07T13:26:29Z

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/configuration_edit_flags.go	80.4%	82.4%	1.9
pkg/serving/config_changes.go	57.1%	66.1%	9.0

sixolet · 2019-06-08T15:27:19Z

/approve
/lgtm

knative-prow-robot · 2019-06-08T15:27:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhuss, sixolet

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sixolet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

- Test PR for knative#157 /hold

Runs cross platform build to ensure all the checks pass

Bonuses: * better handling of missing bazel test artifacts (internal error) * clearly separate summary and detailed logs on failures

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 2, 2019

knative-prow-robot requested review from mattmoor and sixolet June 2, 2019 15:18

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 2, 2019

knative-prow-robot assigned sixolet Jun 2, 2019

rhuss mentioned this pull request Jun 2, 2019

The road to 0.2.0 #158

Closed

rhuss changed the title ~~feature(service create/update): Add support of minScale/maxScale/target~~ feat(service create/update): Add support of minScale/maxScale/target Jun 3, 2019

cppforlife added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 3, 2019

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 3, 2019

maximilien reviewed Jun 3, 2019

View reviewed changes

pkg/kn/commands/service_create.go Outdated Show resolved Hide resolved

rhuss force-pushed the pr/min-max-scale branch from 51d152a to 9c36588 Compare June 3, 2019 21:58

duglin reviewed Jun 4, 2019

View reviewed changes

rhuss force-pushed the pr/min-max-scale branch 4 times, most recently from 031eec8 to 27d167d Compare June 6, 2019 09:45

sixolet suggested changes Jun 7, 2019

View reviewed changes

rhuss force-pushed the pr/min-max-scale branch from 824549f to 143f363 Compare June 7, 2019 13:24

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 8, 2019

sixolet approved these changes Jun 8, 2019

View reviewed changes

knative-prow-robot merged commit b885e41 into knative:master Jun 8, 2019

rhuss added this to the v0.2.0 milestone Jun 9, 2019

navidshaikh added a commit to navidshaikh/client that referenced this pull request Nov 6, 2019

[test] Runs cross platform build to ensure all the checks pass

6147d6c

- Test PR for knative#157 /hold

navidshaikh pushed a commit to navidshaikh/client that referenced this pull request Nov 6, 2019

Merge pull request knative#157 from navidshaikh/pr/build-all

0443bee

Runs cross platform build to ensure all the checks pass

rhuss deleted the pr/min-max-scale branch March 9, 2021 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(service create/update): Add support of minScale/maxScale/target #157

feat(service create/update): Add support of minScale/maxScale/target #157

rhuss commented Jun 2, 2019 •

edited

Loading

knative-prow-robot commented Jun 2, 2019

rhuss commented Jun 2, 2019

rhuss commented Jun 2, 2019

knative-prow-robot commented Jun 2, 2019

cppforlife commented Jun 3, 2019

rhuss commented Jun 3, 2019

duglin Jun 4, 2019

rhuss Jun 4, 2019

duglin Jun 4, 2019

rhuss Jun 4, 2019

maximilien Jun 4, 2019 •

edited

Loading

rhuss Jun 5, 2019

rhuss Jun 5, 2019

markusthoemmes Jun 5, 2019 •

edited

Loading

rhuss Jun 5, 2019

rhuss Jun 6, 2019

rhuss commented Jun 6, 2019

duglin commented Jun 6, 2019 via email

rhuss commented Jun 6, 2019

duglin commented Jun 6, 2019

sixolet left a comment

sixolet Jun 7, 2019

rhuss Jun 7, 2019

rhuss Jun 7, 2019

knative-metrics-robot commented Jun 7, 2019

sixolet commented Jun 8, 2019

knative-prow-robot commented Jun 8, 2019

feat(service create/update): Add support of minScale/maxScale/target #157

feat(service create/update): Add support of minScale/maxScale/target #157

Conversation

rhuss commented Jun 2, 2019 • edited Loading

knative-prow-robot commented Jun 2, 2019

rhuss commented Jun 2, 2019

rhuss commented Jun 2, 2019

knative-prow-robot commented Jun 2, 2019

cppforlife commented Jun 3, 2019

rhuss commented Jun 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilien Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusthoemmes Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss commented Jun 6, 2019

duglin commented Jun 6, 2019 via email

rhuss commented Jun 6, 2019

duglin commented Jun 6, 2019

sixolet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Jun 7, 2019

sixolet commented Jun 8, 2019

knative-prow-robot commented Jun 8, 2019

rhuss commented Jun 2, 2019 •

edited

Loading

maximilien Jun 4, 2019 •

edited

Loading

markusthoemmes Jun 5, 2019 •

edited

Loading