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.

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 default cluster-side min-scale #12290

Merged
merged 1 commit into from
Nov 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion config/core/configmaps/autoscaler.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ metadata:
app.kubernetes.io/version: devel
serving.knative.dev/release: devel
annotations:
knative.dev/example-checksum: "604cb513"
knative.dev/example-checksum: "b4401cf4"
data:
_example: |
################################
Expand Down Expand Up @@ -187,6 +187,10 @@ data:
# or the "autoscaling.knative.dev/initialScale" annotation, can be set to 0.
allow-zero-initial-scale: "false"

# min-scale is the cluster-wide default value for the min scale of a revision,
# unless overridden by the "autoscaling.knative.dev/minScale" annotation.
min-scale: "0"

# max-scale is the cluster-wide default value for the max scale of a revision,
# unless overridden by the "autoscaling.knative.dev/maxScale" annotation.
# If set to 0, the revision has no maximum scale.
Expand Down
5 changes: 4 additions & 1 deletion pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,10 @@ func (pa *PodAutoscaler) annotationFloat64(key string) (float64, bool) {
func (pa *PodAutoscaler) ScaleBounds(asConfig *autoscalerconfig.Config) (int32, int32) {
var min int32
if pa.Spec.Reachability != ReachabilityUnreachable {
min, _ = pa.annotationInt32(autoscaling.MinScaleAnnotationKey)
min = asConfig.MinScale
Copy link
Member

Choose a reason for hiding this comment

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

I'd add a test to the test table for this case (i.e. that when minScale > 1 but unreachable we still return 0) since nothing caught this being one line too high before :)

Copy link
Member Author

Choose a reason for hiding this comment

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

added test

Copy link
Member

Choose a reason for hiding this comment

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

not super important at all, but jfwiw it's a little easier to review if you push new commits rather than force pushing a squashed commit (just because that way the reviewer doesn't need to read back through the whole thing), and prow is set up to auto-squash when merging anyway

if paMin, ok := pa.annotationInt32(autoscaling.MinScaleAnnotationKey); ok {
min = paMin
}
}

max := asConfig.MaxScale
Expand Down
7 changes: 7 additions & 0 deletions pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,13 @@ func TestScaleBounds(t *testing.T) {
max: "sandwich",
wantMin: 0,
wantMax: 0,
}, {
name: "min-scale set but unreachable",
config: autoscalerconfig.Config{
MinScale: 2,
},
reachability: ReachabilityUnreachable,
wantMin: 0,
}}

for _, tc := range cases {
Expand Down
4 changes: 4 additions & 0 deletions pkg/autoscaler/config/autoscalerconfig/autoscalerconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ type Config struct {
// services. This can be set to 0 iff AllowZeroInitialScale is true.
InitialScale int32

// MinScale is the default min scale for any revision created without an
// autoscaling.knative.dev/minScale annotation
MinScale int32

// MaxScale is the default max scale for any revision created without an
// autoscaling.knative.dev/maxScale annotation
MaxScale int32
Expand Down
10 changes: 10 additions & 0 deletions pkg/autoscaler/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ func defaultConfig() *autoscalerconfig.Config {
PodAutoscalerClass: autoscaling.KPA,
AllowZeroInitialScale: false,
InitialScale: 1,
MinScale: 0,
MaxScale: 0,
MaxScaleLimit: 0,
}
Expand All @@ -86,6 +87,7 @@ func NewConfigFromMap(data map[string]string) (*autoscalerconfig.Config, error)
cm.AsFloat64("panic-threshold-percentage", &lc.PanicThresholdPercentage),

cm.AsInt32("initial-scale", &lc.InitialScale),
cm.AsInt32("min-scale", &lc.MinScale),
cm.AsInt32("max-scale", &lc.MaxScale),
cm.AsInt32("max-scale-limit", &lc.MaxScaleLimit),

Expand Down Expand Up @@ -176,6 +178,10 @@ func validate(lc *autoscalerconfig.Config) (*autoscalerconfig.Config, error) {
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.MinScale < 0 {
nader-ziada marked this conversation as resolved.
Show resolved Hide resolved
return nil, fmt.Errorf("min-scale = %v, must be at least 0", lc.MinScale)
}

var minMaxScale int32
if lc.MaxScaleLimit > 0 {
// Default maxScale must be set if maxScaleLimit is set.
Expand All @@ -191,6 +197,10 @@ func validate(lc *autoscalerconfig.Config) (*autoscalerconfig.Config, error) {
return nil, fmt.Errorf("max-scale-limit = %v, must be at least 0", lc.MaxScaleLimit)
}

if lc.MinScale > lc.MaxScale && lc.MaxScale > 0 {
return nil, fmt.Errorf("min-scale (%d) must be less than max-scale (%d)", lc.MinScale, lc.MaxScale)
}

return lc, nil
}

Expand Down
25 changes: 25 additions & 0 deletions pkg/autoscaler/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,22 +133,30 @@ func TestNewConfig(t *testing.T) {
name: "with toggles explicitly flipped",
input: map[string]string{
"enable-scale-to-zero": "false",
"min-scale": "1",
"max-scale": "2",
},
want: func() *autoscalerconfig.Config {
c := defaultConfig()
c.EnableScaleToZero = false
c.MinScale = 1
c.MaxScale = 2
return c
}(),
}, {
name: "with explicit grace period",
input: map[string]string{
"enable-scale-to-zero": "false",
"scale-to-zero-grace-period": "33s",
"min-scale": "1",
"max-scale": "2",
},
want: func() *autoscalerconfig.Config {
c := defaultConfig()
c.EnableScaleToZero = false
c.ScaleToZeroGracePeriod = 33 * time.Second
c.MinScale = 1
c.MaxScale = 2
return c
}(),
}, {
Expand Down Expand Up @@ -350,6 +358,23 @@ func TestNewConfig(t *testing.T) {
"max-scale-limit": "11",
},
wantErr: true,
}, {
name: "with max scale less than min-scale",
input: map[string]string{
"min-scale": "4",
"max-scale": "3",
},
wantErr: true,
}, {
name: "with min scale set but max-scale not set",
input: map[string]string{
"min-scale": "4",
},
want: func() *autoscalerconfig.Config {
c := defaultConfig()
c.MinScale = 4
return c
}(),
}, {
name: "with valid default max scale and max scale limit",
input: map[string]string{
Expand Down
2 changes: 2 additions & 0 deletions pkg/reconciler/autoscaling/kpa/kpa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,15 @@ func defaultConfigMapData() map[string]string {
"panic-window": "10s",
"scale-to-zero-grace-period": gracePeriod.String(),
"tick-interval": "2s",
"min-scale": "0",
}
}

func initialScaleZeroASConfig() *autoscalerconfig.Config {
ac, _ := asconfig.NewConfigFromMap(defaultConfigMapData())
ac.AllowZeroInitialScale = true
ac.InitialScale = 0
ac.MinScale = 0
ac.EnableScaleToZero = true
return ac
}
Expand Down