Add default cluster-side min-scale #12290

nader-ziada · 2021-11-16T20:58:29Z

based on a slack conversation: https://knative.slack.com/archives/C94SPR60H/p1637002245043400

Proposed Changes

add a cluster-wide config value for min-scale

Release Note

Add cluster wide default min-scale

codecov · 2021-11-16T21:03:55Z

Codecov Report

Merging #12290 (1a13736) into main (a902f25) will decrease coverage by 0.00%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             main   #12290      +/-   ##
==========================================
- Coverage   87.47%   87.46%   -0.01%     
==========================================
  Files         195      195              
  Lines        9595     9599       +4     
==========================================
+ Hits         8393     8396       +3     
+ Misses        923      922       -1     
- Partials      279      281       +2

Impacted Files	Coverage Δ
pkg/autoscaler/config/config.go	`97.91% <66.66%> (-2.09%)`	⬇️
pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go	`100.00% <100.00%> (ø)`
pkg/reconciler/revision/background.go	`90.00% <0.00%> (-1.82%)`	⬇️
pkg/reconciler/configuration/configuration.go	`84.61% <0.00%> (-1.54%)`	⬇️
cmd/queue/main.go	`0.53% <0.00%> (+<0.01%)`	⬆️
pkg/queue/concurrency_state.go	`79.71% <0.00%> (+3.65%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a902f25...1a13736. Read the comment docs.

config/core/configmaps/autoscaler.yaml

pkg/autoscaler/config/autoscalerconfig/autoscalerconfig.go

julz · 2021-11-17T11:05:16Z

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

@@ -84,9 +84,11 @@ func (pa *PodAutoscaler) annotationFloat64(key string) (float64, bool) {
 // not set.
 // Note: min will be ignored if the PA is not reachable
 func (pa *PodAutoscaler) ScaleBounds(asConfig *autoscalerconfig.Config) (int32, int32) {
-	var min int32
+	min := asConfig.MinScale


I think this needs to be in the if block one line down, otherwise we'll keep minScale > 0 even when the revision becomes unreachable, which isn't how specifying min-scale-as-an-annotation acts

julz · 2021-11-17T11:05:45Z

pkg/autoscaler/config/autoscalerconfig/autoscalerconfig.go

@@ -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/maxScale annotation


Suggested change

// autoscaling.knative.dev/maxScale annotation

// autoscaling.knative.dev/minScale annotation

fixed, sorry for the typos, went through and made sure there aren't any more

pkg/autoscaler/config/config.go

julz · 2021-11-17T16:39:29Z

pkg/autoscaler/config/config.go

@@ -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 {


what if lc.MaxScale hasn't been set, so is == 0?

added validation and test

julz · 2021-11-17T16:42:20Z

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

@@ -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


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 :)

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

julz

thanks @nader-ziada!

/lgtm
/approve

julz · 2021-11-18T08:08:58Z

pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go

@@ -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


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

knative-prow-robot · 2021-11-18T08:12:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: julz, nader-ziada

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:

~~config/OWNERS~~ [julz]
~~pkg/apis/autoscaling/OWNERS~~ [julz]
~~pkg/autoscaler/OWNERS~~ [julz]
~~pkg/reconciler/autoscaling/OWNERS~~ [julz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nader-ziada · 2021-11-18T13:59:26Z

@julz sorry about that, didn't know prow was set to squash commits

mattmoor · 2021-11-26T16:51:00Z

@nader-ziada thanks for the quick turnaround on this, I just saw! Looking forward to 1.1 🤩

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 16, 2021

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/API API objects and controllers area/autoscale labels Nov 16, 2021

julz reviewed Nov 16, 2021

View reviewed changes

config/core/configmaps/autoscaler.yaml Outdated Show resolved Hide resolved

pkg/autoscaler/config/autoscalerconfig/autoscalerconfig.go Outdated Show resolved Hide resolved

nader-ziada force-pushed the min-scale-default branch from aaa1dff to 4a5b800 Compare November 16, 2021 21:28

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 16, 2021

julz reviewed Nov 17, 2021

View reviewed changes

nader-ziada force-pushed the min-scale-default branch from 4a5b800 to 7a193f2 Compare November 17, 2021 14:24

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 17, 2021

julz reviewed Nov 17, 2021

View reviewed changes

Add default cluster-side min-scale

1a13736

nader-ziada force-pushed the min-scale-default branch from 7a193f2 to 1a13736 Compare November 17, 2021 17:52

dprotaso added this to the v1.1.0 milestone Nov 17, 2021

julz approved these changes Nov 18, 2021

View reviewed changes

knative-prow-robot assigned julz Nov 18, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 18, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2021

knative-prow-robot merged commit b4768b1 into knative:main Nov 18, 2021

psschwei mentioned this pull request Apr 4, 2022

Global key for min-scale value knative/docs#4897

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default cluster-side min-scale #12290

Add default cluster-side min-scale #12290

nader-ziada commented Nov 16, 2021

codecov bot commented Nov 16, 2021 •

edited

Loading

julz Nov 17, 2021

julz Nov 17, 2021

nader-ziada Nov 17, 2021

julz Nov 17, 2021

nader-ziada Nov 17, 2021

julz Nov 17, 2021

nader-ziada Nov 17, 2021

julz Nov 18, 2021

julz left a comment

julz Nov 18, 2021

knative-prow-robot commented Nov 18, 2021

nader-ziada commented Nov 18, 2021

mattmoor commented Nov 26, 2021

	// autoscaling.knative.dev/maxScale annotation
	// autoscaling.knative.dev/minScale annotation

Add default cluster-side min-scale #12290

Add default cluster-side min-scale #12290

Conversation

nader-ziada commented Nov 16, 2021

Proposed Changes

codecov bot commented Nov 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 18, 2021

nader-ziada commented Nov 18, 2021

mattmoor commented Nov 26, 2021

codecov bot commented Nov 16, 2021 •

edited

Loading