Refactor autoscaling parameters

[rhuss]

Since we have collected all concurrency related options with the option prefix --concurrency- and also list them together in kn service describe, the autoscale window parameter should be also in this group. So either move everything to --concurrency- or, move everything (limit, target, utilization, window) to --autoscale.

I'm not sure in which direction to go, moving to --concurrency is easier as most of the options are already --concurrency prefixed, on the other "concurrency" is quite tedious to type, so maybe 'autoscale-' is a better prefix? But we might even consider a total different prefix as well?

[duglin]

Just to add another wrinkle into it... not having the same names as what people would put into the yaml has caused me some mental pain :-) it makes it harder to switch between the two worlds. Thinking/remembering is hard.

[rhuss]

I totally agree that we should align where it is possible, but the concurrency options are a bit 'special', as they are sprinkled all over the YAML (as annotations and different fields). I see the value of the CLI to allow a use to not remember the full annotation.

I think we should use the given name where possible, but deviate where needed. E.g. I really would love to have all concurrency/scaling related parameters together in the 'option namespace' (i.e. appearing together on the help page, so using the same prefix).

[rhuss]

According to Configuring Autoscaling

we should probably better move to --autoscale- prefix and deprecated --concurrency- (so, support it for the next releases, too but move to a uniform prefix). I think that makes sense if we want to support even more autoscale related parameters. The per-revision parameters that possibly make sense to support are:

• --autoscale-class
• --autoscale-metric
• --autoscale-target (deprecate --concurrency-target)
• --autoscale-limit (deprecate --concurrency-limit)
• --autoscale-utilization (deprecate --concurrency-utilization)
• --autoscale-rps (requests-per-second) <--- I think we can skip that as it's not clear how this differs to --autoscale-target
• --autoscale-min (deprecate --min-scale. Does this make sense ?)
• --autoscale-max (deprecate --max-scale. Does this make sense ?)
• --autoscale-window
• --autoscale-panic-window
• --autoscale-panic-threshold

For backwards compatibility reason we could stick to the current main concurrency related config:

• --concurrency-limit
• --concurrency-target
• --min-scale
• --max-scale

then deprecate --autoscale-window and --concurrency-utilization and introduce a --autoscale parameter for configuring less used autoscaling parameters like in --autoscale window=10s,utilization=70,panic-window=10 with the alternative syntax --autoscale window=10s --autoscale utilization=70 --autoscale panic-window=10 (the same what we already support for array-typed options like --env or --annotation). For sake of consistency we then can add also min, max, limit and target to the key --autoscale subconfig keys.

The benefit of this approach:

• We don't have to deprecate too many options, resulting in a better backwards-compatible approach. Only --concurrency-utilization and --autoscale-window would be affected.
• We can easily add new autoscaling parameters without polluting the option namespace
• Shortcuts for the most common options, but allowing all possible autoscaling parameters.

I think, I like this suggestion. wdyt ?

[duglin]

I'd hold off until they're done with that doc and resulting PRs. I'm still hoping they'll rename stuff as a result of that work.

In particular, --autoscale-limit is not a great alternative to container-concurrency. As a user I would guess that --autoscale-limit is about max scaling of instances, not the amount of concurrency allowed per instance

[itsmurugappan]

According to Configuring Autoscaling

@rhuss can I please get access to this doc ?

[rhuss]

We sorted out the issue. FYI you need to subscribe to https://groups.google.com/forum/#!forum/knative-dev in order to access Knative documentations ....

[markusthoemmes]

FWIW: I would vote against --autoscale-limit definitely, it's not even really an autoscaling parameter. It's more of a loadbalancing/enforcement paramter for the revision which is just inferred into target by the autoscaler, because that makes sense from its PoV. I'd not prepend that with autoscale.

[rhuss]

FWIW: I would vote against --autoscale-limit definitely, it's not even really an autoscaling parameter. It's more of a loadbalancing/enforcement paramter for the revision which is just inferred into target by the autoscaler, because that makes sense from its PoV. I'd not prepend that with autoscale.

@markusthoemmes so you are saying that the "limit" and the "target" are not belonging to the same group (like kind of imposed by the syntax "autoscaling.knative.dev/target"), where on is connected to autoscaling and the other not ? If so, then I think the analogy to 'soft' vs 'hard' limit is a bit misleading then, too (limit of what ?)

So its should be --concurrency-limit and --autoscale-target ? (or would be --concurrency-target also ok ?)

[markusthoemmes]

Yeah, it's a little involved.

The autoscaler only knows about the target. If no explicit target is set, it is derived from the concurrency limit (aka containerConcurrency). I agree, the analogy might be a bit weird, it explains the behavior from the user's pov though.

[github-actions[bot]]

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

[rhuss]

/remove-lifecycle-stale /reopen

We definitely still need on this refactoring. E.g we have --autoscale-window but --scale-min. Technically it should be probably all --autoscale but --scale is shorter.

Also we are still missing some parameters, like the configuration options for the panic window.

[knative-prow-robot]

@rhuss: Reopened this issue.

In response to [this](https://github.com/knative/client/issues/816#issuecomment-736402989): >/remove-lifecycle-stale >/reopen > >We definitely still need on this refactoring. E.g we have `--autoscale-window` but `--scale-min`. Technically it should be probably all `--autoscale` but `--scale` is shorter. > >Also we are still missing some parameters, like the configuration options for the panic window. Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[github-actions[bot]]

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

[rhuss]

/remove-lifecycle stale

[abrennan89]

@rhuss any timeline for plans to work on this one? I think refactoring these would give a much nicer UX.

[dsimansk]

:thinking: it seems to be done already.

                                          generation, and {{.Random [n]}} for n random consonants (e.g. {{.Service}}-{{.Random 5}}-{{.Generation}})
      --scale string                      Set the Minimum and Maximum number of replicas. You can use this flag to set both to a single value, or set a range with min/max values, or set either min or max values without specifying the other. Example: --scale 5 (scale-min = 5, scale-max = 5) or --scale
                                          1..5 (scale-min = 1, scale-max = 5) or --scale 1.. (scale-min = 1, scale-max = unchanged) or --scale ..5 (scale-min = unchanged, scale-max = 5)
      --scale-init int                    Initial number of replicas with which a service starts. Can be 0 or a positive integer.
      --scale-max int                     Maximum number of replicas.
      --scale-min int                     Minimum number of replicas.
      --scale-target int                  Recommendation for when to scale up based on the concurrent number of incoming request. Defaults to --concurrency-limit when given.
      --scale-utilization int             Percentage of concurrent requests utilization before scaling up. (default 70)
      --scale-window string               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)

There's a PR to add --scale-metric, but that's current state of scaling flags. I can look up since which release it's changed, if it helps.