Add new ConfigMap fields for service initial scale #7674
Conversation
Adding two new ConfigMap fields `allow-zero-initial-scale` and `default-initial-scale` to config-autoscaler.
knative-prow-robot
added
the
size/M
There was a problem hiding this comment.
@taragu: 0 warnings.
In response to this:
Adding two new ConfigMap fields
allow-zero-initial-scaleanddefault-initial-scaleto config-autoscaler./lint
Part 1 of #4098
Release Note
NONE
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.
|
/assign @dprotaso @markusthoemmes @vagababov |
|
should we add the new fields to the |
|
@julz I think we should hold off from an _example entry until the feature is implemented. |
|
/retest |
|
/lgtm |
pkg/autoscaler/config/config.go
Outdated
| if lc.InitialScale < 0 { | ||
| return nil, fmt.Errorf("initial-scale = %v, must be at least 0 (or at least 1 when allow-zero-initial-scale is false)", lc.InitialScale) | ||
| } | ||
| if lc.InitialScale == 0 && !lc.AllowZeroInitialScale { | ||
| return nil, fmt.Errorf("initial-scale = %v, must be at least 1 since allow-zero-initial-scale is false", lc.InitialScale) | ||
| } |
There was a problem hiding this comment.
If we re-arrange in the reverse order, then we can go back to the previous errors messages, since they'd be correct, I think. But probably isn't important much.
There was a problem hiding this comment.
I'm not sure I understand. What do you mean by "we can go back to the previous errors messages"?
There was a problem hiding this comment.
eh, basically
if x < 0 || x==0 && !allow {
return "must be non-negative or positive if allow is unset"
}
There was a problem hiding this comment.
How about:
minInitialScale := 1
if lc.AllowZeroInitialScale {
minInitialScale = 0
}
if lc.InitialScale < minInitialScale {
return nil, fmt.Errorf("initial-scale = %d, must be at least %d", lc.InitialScale, minInitialScale)
}There was a problem hiding this comment.
Well we kind of had that before :-)
It's bikeshedding at this point. I like mine, since it has single if :)
|
/test pull-knative-serving-integration-tests |
|
/lgtm |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, 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 |
knative-prow-robot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
The following jobs failed:
Job pull-knative-serving-integration-tests expended all 3 retries without success. |
Adding two new ConfigMap fields
allow-zero-initial-scaleanddefault-initial-scaleto config-autoscaler./lint
Part 1 of #7682
Release Note