Start autoscaler in panic mode. #4795
Conversation
When autoscaler restarts it has no memory of the previous stats and immediately scales the deployment down. Under my load test conditions, Autoscaler goes from 7 to 1 then to 2, only then to 6 pods, before finally reaching 7 after a few cycles. This is obviously not a desired property, so this change fixes that behavior, by starting Autoscaler in panic mode with panic pod count equal to the current pod count if we have more than 1 serving pods right now. Obviously if the deployment is scaled to 0, there's no reason to change logic and if it is 1, then we won't scale below 1 anyway for the next stable window, so no need to panic either. /assign @mattmoor @markusthoemmes This is the GA scope of knative#2930
knative-prow-robot
added
the
do-not-merge/work-in-progress
knative-prow-robot
added
size/S
There was a problem hiding this comment.
@vagababov: 0 warnings.
In response to this:
When autoscaler restarts it has no memory of the previous stats and immediately scales the deployment down. Under my load test conditions,
Autoscaler goes from 7 to 1 then to 2, only then to 6 pods, before finally reaching 7 after a few cycles.
This is obviously not a desired property, so this change fixes that behavior, by
starting Autoscaler in panic mode with panic pod count equal to the current pod count
if we have more than 1 serving pods right now.Obviously if the deployment is scaled to 0, there's no reason to change logic
and if it is 1, then we won't scale below 1 anyway for the next stable window, so no need to panic either./assign @mattmoor @markusthoemmes
This is the GA scope of #2930
/lint
This starts autoscaler in panic mode and does not scale down deployment after autoscaler restart.
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.
pkg/autoscaler/autoscaler.go
Outdated
| @@ -24,6 +24,7 @@ import ( | |||
| "sync" | |||
| "time" | |||
|
|
|||
| "github.com/wacul/ptr" | |||
There was a problem hiding this comment.
🤦♂ Can we just add this to knative/pkg rather than juggling packages for this?
There was a problem hiding this comment.
I don't think we need it at all. A local variable will do as well.
There was a problem hiding this comment.
https://github.com/knative/pkg/pull/519/files
Well, as soon as matt does this!
knative-prow-robot
added
size/L
knative-prow-robot
removed
the
do-not-merge/work-in-progress
knative-prow-robot
added
size/XL
knative-prow-robot
added
size/L
|
This is ready |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor, 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
|
/test pull-knative-serving-integration-tests hmmm.. 🚀 |
When autoscaler restarts it has no memory of the previous stats and immediately scales the deployment down. Under my load test conditions,
Autoscaler goes from 7 to 1 then to 2, only then to 6 pods, before finally reaching 7 after a few cycles.
This is obviously not a desired property, so this change fixes that behavior, by
starting Autoscaler in panic mode with panic pod count equal to the current pod count
if we have more than 1 serving pods right now.
Obviously if the deployment is scaled to 0, there's no reason to change logic
and if it is 1, then we won't scale below 1 anyway for the next stable window, so no need to panic either.
/assign @mattmoor @markusthoemmes
This is the GA scope of #2930
/lint