Add active-min-scale annotation #13136

psschwei · 2022-07-20T19:56:23Z

Signed-off-by: Paul S. Schweigert paulschw@us.ibm.com

Proposed Changes

This PR implements a "min-scale, but with scale to zero" feature. In
essence, when this feature is in use, if the service is handling
requests it will never have less than N number of pods (though it can
still scale to zero).

N is specified on a per-revision basis using an
autoscaling.knative.dev/active-min-scale annotation.

This annotation will not impact initial-scale values, as it will only apply on
subsequent scales from zero.

Fixes #11308

/assign @nader-ziada @dprotaso

Signed-off-by: Paul S. Schweigert <paulschw@us.ibm.com> This PR implements a "min-scale, but with scale to zero" feature. In essence, when this feature is in use, if the service is handling requests it will never have less than N number of pods (though it can still scale to zero). N is specified on a per-revision basis using an `autoscaling.knative.dev/min-non-zero-replicas` annotation. If min-non-zero-replicas > initial-scale, the service will initially scale to the min-non-zero-replicas value.

psschwei · 2022-07-20T19:57:49Z

@vagababov I think I've seen you pop in a bit recently, so if you have bandwidth to take a look that'd be great (and if not, no worries at all)

/approve cancel

codecov · 2022-07-20T20:04:44Z

Codecov Report

Merging #13136 (ba7e34b) into main (d366049) will decrease coverage by 0.08%.
The diff coverage is 64.70%.

@@            Coverage Diff             @@
##             main   #13136      +/-   ##
==========================================
- Coverage   86.78%   86.69%   -0.09%     
==========================================
  Files         196      196              
  Lines       14410    14450      +40     
==========================================
+ Hits        12506    12528      +22     
- Misses       1609     1626      +17     
- Partials      295      296       +1

Impacted Files	Coverage Δ
pkg/autoscaler/scaling/multiscaler.go	`87.24% <ø> (-1.35%)`	⬇️
pkg/apis/autoscaling/annotation_validation.go	`95.23% <52.63%> (-4.77%)`	⬇️
pkg/autoscaler/scaling/autoscaler.go	`96.84% <62.50%> (-1.51%)`	⬇️
pkg/apis/autoscaling/v1alpha1/pa_lifecycle.go	`100.00% <100.00%> (ø)`
...kg/reconciler/autoscaling/kpa/resources/decider.go	`100.00% <100.00%> (ø)`
cmd/queue/main.go
pkg/queue/sharedmain/main.go	`0.64% <0.00%> (ø)`
pkg/autoscaler/statforwarder/leases.go	`73.95% <0.00%> (+1.56%)`	⬆️
pkg/reconciler/revision/controller.go	`92.95% <0.00%> (+3.62%)`	⬆️

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 d366049...ba7e34b. Read the comment docs.

dprotaso

I think the annotation name min-non-zero-replicas is confusing. The only alternative I have to offer is min-activate-scale - where active (to me) means the revision is handling a request(s)

dprotaso · 2022-07-22T19:08:17Z

pkg/apis/autoscaling/annotation_validation.go

+		} else if nzMin < 2 {
+			errs = errs.Also(&apis.FieldError{
+				Message: fmt.Sprintf("min-non-zero-replicas=%d must be greater than 1", nzMin),
+				Paths:   []string{MinNonZeroReplicasKey},
+			})
+		}


Curious if setting a value of 1 would be necessary for the scenario:

min-scale: 0
initial-scale: 0
min-non-zero-replicas: 1

Or is setting it to 1 just not necessary because that's the floor?

My thought here was that if you're going to set this value, it needs to actually do something (setting it equal to one is the same as not setting it at all, and in that case you may as well not use it at all...)

pkg/apis/autoscaling/annotation_validation.go

pkg/apis/autoscaling/v1alpha1/pa_lifecycle_test.go

pkg/reconciler/autoscaling/kpa/resources/decider_test.go

pkg/reconciler/autoscaling/kpa/scaler.go

pkg/autoscaler/scaling/autoscaler.go

Signed-off-by: Paul S. Schweigert <paulschw@us.ibm.com>

pkg/autoscaler/scaling/multiscaler.go

pkg/autoscaler/scaling/autoscaler.go

dprotaso · 2022-07-27T15:22:58Z

Also - I would do a sanity check on the annotation name say with someone from Docs/UX - in case they have a better suggestion. Worst case we change the name again in a future before (but before the release)

dprotaso · 2022-07-27T15:51:03Z

/lgtm
/approve
/hold in case you plan on doing the sanity check - otherwise feel free to /unhold and do a follow up PR if the name changes

dprotaso · 2022-07-27T15:51:36Z

I guess one last thing (in a follow up PR) - having an e2e test would be great

Signed-off-by: Paul S. Schweigert <paulschw@us.ibm.com>

psschwei · 2022-07-27T16:53:51Z

We chatted in slack on the name change
/hold cancel

dprotaso · 2022-07-27T17:48:35Z

/lgtm
/approve

let's do an e2e test in a followup

knative-prow · 2022-07-27T17:48:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso

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:

~~pkg/OWNERS~~ [dprotaso]
~~pkg/apis/OWNERS~~ [dprotaso]

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

knative-prow bot assigned dprotaso and nader-ziada Jul 20, 2022

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers area/autoscale labels Jul 20, 2022

dprotaso reviewed Jul 22, 2022

View reviewed changes

psschwei added 2 commits July 26, 2022 18:22

review fixes

ee4f805

Signed-off-by: Paul S. Schweigert <paulschw@us.ibm.com>

fix unit test

0231ec5

Signed-off-by: Paul S. Schweigert <paulschw@us.ibm.com>

psschwei changed the title ~~Add min-non-zero-replicas annotation~~ Add min-activate-scale annotation Jul 27, 2022

dprotaso reviewed Jul 27, 2022

View reviewed changes

pkg/autoscaler/scaling/multiscaler.go Outdated Show resolved Hide resolved

pkg/autoscaler/scaling/autoscaler.go Outdated Show resolved Hide resolved

knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 27, 2022

renaming to active-min-scale

ba7e34b

Signed-off-by: Paul S. Schweigert <paulschw@us.ibm.com>

psschwei changed the title ~~Add min-activate-scale annotation~~ Add active-min-scale annotation Jul 27, 2022

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2022

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2022

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2022

knative-prow bot merged commit addd55a into knative:main Jul 27, 2022

psschwei mentioned this pull request Jul 27, 2022

Add documentation for active-min-scale knative/docs#5142

Merged

dprotaso mentioned this pull request Jul 27, 2022

Updating name of activation-scale #13161

Merged

dsimansk mentioned this pull request Jul 29, 2022

Add --scale-activation flag to match new Serving option knative/client#1713

Closed

psschwei mentioned this pull request Aug 9, 2022

Adds e2e test for activation-scale #13197

Merged

psschwei deleted the scale-up-non-one branch August 10, 2022 15:47

daraghlowe mentioned this pull request Apr 22, 2024

Allow scale to zero to work when min-scale is greater than 0 #15154

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add active-min-scale annotation #13136

Add active-min-scale annotation #13136

psschwei commented Jul 20, 2022 •

edited

psschwei commented Jul 20, 2022

codecov bot commented Jul 20, 2022 •

edited

dprotaso left a comment

dprotaso Jul 22, 2022

psschwei Jul 25, 2022

dprotaso commented Jul 27, 2022

dprotaso commented Jul 27, 2022

dprotaso commented Jul 27, 2022 •

edited

psschwei commented Jul 27, 2022 •

edited

dprotaso commented Jul 27, 2022

knative-prow bot commented Jul 27, 2022

Add active-min-scale annotation #13136

Add active-min-scale annotation #13136

Conversation

psschwei commented Jul 20, 2022 • edited

Proposed Changes

psschwei commented Jul 20, 2022

codecov bot commented Jul 20, 2022 • edited

Codecov Report

dprotaso left a comment

Choose a reason for hiding this comment

dprotaso Jul 22, 2022

Choose a reason for hiding this comment

psschwei Jul 25, 2022

Choose a reason for hiding this comment

dprotaso commented Jul 27, 2022

dprotaso commented Jul 27, 2022

dprotaso commented Jul 27, 2022 • edited

psschwei commented Jul 27, 2022 • edited

dprotaso commented Jul 27, 2022

knative-prow bot commented Jul 27, 2022

psschwei commented Jul 20, 2022 •

edited

codecov bot commented Jul 20, 2022 •

edited

dprotaso commented Jul 27, 2022 •

edited

psschwei commented Jul 27, 2022 •

edited