-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add example values for initial scale cluster flags #8846
Add example values for initial scale cluster flags #8846
Conversation
Can you please create (and link) a docs PR in the same go? (cc @abrennan89) |
The following jobs failed:
Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:
|
@markusthoemmes sure! @abrennan89 where would the doc belong? I was checking the docs repo, and it seems there are only high level concepts below https://github.com/knative/docs/tree/master/docs/serving/autoscaling |
4f525f2
to
588f015
Compare
# This value cannot be set to 0 while allow-zero-initial-scale is false. | ||
# If annotation "autoscaling.knative.dev/initialScale" is not set, | ||
# or "autoscaling.knative.dev/initialScale" is set to 0 while | ||
# allow-zero-initial-scale is false, the cluster wide initial-scale value will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we permit to create a revision with autoscaling.knative.dev/initialScale==0 when feature is off, rather than just default with cluster wide value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no we don't permit creating a new revision with autoscaling.knative.dev/initialScale==0, but if there's an existing revision with initialScale 0, we will default with cluster wide value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't make sense :-) Existing revision is exempt from these policies, since they only apply at creation :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh yeah that's true. so then we will just block new revision creation, and there won't be a problem with existing revisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we are saying cluster wide value to override autoscaling.knative.dev/initialScale of a revision if autoscaling.knative.dev/initialScale==0 when allow-zero-initial-scale is false? But int allow-zero-initial-scale
section we are saying new revision creation with autoscaling.knative.dev/initialScale==0 will not succeed if allow-zero-initial-scale is false. They are conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yanweiguo yes you are correct. i haven't made the update to the PR yet, but override autoscaling.knative.dev/initialScale of a revision if autoscaling.knative.dev/initialScale==0 when allow-zero-initial-scale is false
should be removed.
# allow-zero-initial-scale is the cluster-wide flag to indicate whether | ||
# initial-scale is allowed to be 0. | ||
# If this is set to true, both the cluster-wide flag initial-scale and | ||
# the annotation "autoscaling.knative.dev/initialScale" are allowed | ||
# to be set to 0. | ||
# If this is set to false, the cluster-wide initial-scale value is not | ||
# allowed to be set to 0. If user attemps to deploy a new revision with | ||
# "autoscaling.knative.dev/initialScale" annotation set to 0, | ||
# the new revision will not be successfully created. If there's an existing | ||
# revision with "autoscaling.knative.dev/initialScale" annotation | ||
# set to 0, while allow-zero-initial-scale is changed from true to false, | ||
# the default value 1 will be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this isn't quite true. If allow-zero-initial-scale
is false and the annotation value is zero, the cluster-wide default is used, which may be >1 if it was changed. (I'm not really sure how big an issue any of this really is since initial scale only applies once per revision anyway so "changing" initial scale for an existing revision only means something in some pretty small edge cases, but 🤷).
Also, stating both that true allows and false disallows seems redundant and this is already quite a lot of text. Maybe we could lose the "If this is set to true" paragraph and tighten the wording a bit, something like this (unless there's a nuance I missed)?
# allow-zero-initial-scale is the cluster-wide flag to indicate whether | |
# initial-scale is allowed to be 0. | |
# If this is set to true, both the cluster-wide flag initial-scale and | |
# the annotation "autoscaling.knative.dev/initialScale" are allowed | |
# to be set to 0. | |
# If this is set to false, the cluster-wide initial-scale value is not | |
# allowed to be set to 0. If user attemps to deploy a new revision with | |
# "autoscaling.knative.dev/initialScale" annotation set to 0, | |
# the new revision will not be successfully created. If there's an existing | |
# revision with "autoscaling.knative.dev/initialScale" annotation | |
# set to 0, while allow-zero-initial-scale is changed from true to false, | |
# the default value 1 will be used. | |
# allow-zero-initial-scale controls whether either the cluster-wide initial-scale flag, | |
# or the "autoscaling.knative.dev/initialScale" annotation, can be set to 0. | |
# If this value is changed from true to false, existing "autoscaling.knative.dev/initialScale" | |
# annotations with value 0 will be ignored. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, technically the cluster value will be used in this case of the cluster value changing from true to false, but since the initial scale only applies for new revisions. Yes sounds good, the true
case is pretty self-explanatory so we can lose it.
# or "autoscaling.knative.dev/initialScale" is set to 0 while | ||
# allow-zero-initial-scale is false, the cluster wide initial-scale value will | ||
# be used for the initial revision deployment size. | ||
# If annotation "autoscaling.knative.dev/initialScale" is set to > 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading through the rest of this configmap, I don't see anywhere else where we've talked about the overriding annotation in this much detail (or at all, in fact :-)), even though pretty much every value in the config map can be overridden in the same way. Would we miss anything important if we just said:
initial-scale is the cluster-wide default value for the initial target scale of a revision
after creation, unless overridden by the "autoscaling.knative.dev/initialScale" annotation.
This value must be greater than 0 unless allow-zero-initial-scale is true.
(and leave the existing description of allow-zero-initial-scale to cover the edge case where that changes true->false).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is much simpler and I don't think we would be missing anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
I think this is fine and if needed we can iterate.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: taragu, vagababov 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:
Approvers can indicate their approval by writing |
Fixes #7682
Proposed Changes
Release Note