Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Start autoscaler in panic mode. #4795

Merged
merged 6 commits into from
Jul 19, 2019
Merged

Start autoscaler in panic mode. #4795

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
982142a
Start autoscaler in panic mode.
vagababov Jul 18, 2019
7ddbe12
Merge branch 'master' into 2930-panic
vagababov Jul 18, 2019
ae417f7
Merge branch 'master' into 2930-panic
vagababov Jul 18, 2019
fb3522f
unit tests
vagababov Jul 18, 2019
8aa5206
updates to the imports
vagababov Jul 19, 2019
e934cee
merge
vagababov Jul 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 21 additions & 2 deletions pkg/autoscaler/autoscaler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"sync"
"time"

"github.com/wacul/ptr"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂ Can we just add this to knative/pkg rather than juggling packages for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need it at all. A local variable will do as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/knative/pkg/pull/519/files
Well, as soon as matt does this!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be done

"go.uber.org/zap"

"knative.dev/pkg/logging"
Expand Down Expand Up @@ -66,8 +67,23 @@ func New(
return nil, errors.New("stats reporter must not be nil")
}

// A new instance of autoscaler is created without panic mode.
reporter.ReportPanic(0)
// We always start in the panic mode, if the deployment is scaled up over 1 pod.
// If the scale is 0 or 1, normal Autoscaler behavior is fine.
// When Autoscaler restarts we lose metric history, which causes us to
// momentarily scale down, and that is not a desired behaviour.
// Thus, we're keeping the current scale until we accumulate enough data to make conscious decisions.
vagababov marked this conversation as resolved.
Show resolved Hide resolved
curC, err := podCounter.ReadyCount()
if err != nil {
return nil, fmt.Errorf("initial pod count failed: %v", err)
}
var pt *time.Time
if curC > 1 {
pt = ptr.Time(time.Now())
// A new instance of autoscaler is created in panic mode.
reporter.ReportPanic(1)
} else {
reporter.ReportPanic(0)
}

return &Autoscaler{
namespace: namespace,
Expand All @@ -76,6 +92,9 @@ func New(
podCounter: podCounter,
deciderSpec: deciderSpec,
reporter: reporter,

panicTime: pt,
maxPanicPods: int32(curC),
}, nil
}

Expand Down