Scale bounds annotations #1980

imikushin · 2018-09-06T22:52:51Z

Proposed Changes

Propagate annotation changes from Revision to KPA
Introduce autoscaling.knative.dev/minScale and autoscaling.knative.dev/maxScale annotations on Revision
Constrain scale changes to the bounds set by these annotations

TODO

Tests

Release Note

Operators can now use `autoscaling.knative.dev/minScale` and 
`autoscaling.knative.dev/maxScale` annotations on Revision and KPA objects 
to set lower/upper bounds on the number of pods for the revision.

Implements knative#1656

imikushin · 2018-09-07T00:56:11Z

/assign @mattmoor

markusthoemmes · 2018-09-07T06:20:07Z

/cc @josephburnett
/assign @josephburnett

pkg/reconciler/v1alpha1/autoscaling/kpa_scaler.go

Make sure explicitly specified scale bounds are greater than 0.

Plus unit tests

Add scale bounds to spec.md

pkg/apis/serving/v1alpha1/metadata_validation_test.go

mattmoor · 2018-09-08T02:04:15Z

docs/spec/spec.md

+        # +optional. When not specified, the revision can scale down to 0 pods
+        autoscaling.knative.dev/minReplicaCount: "2"  
+        # +optional. When not specified, there's no upper scale bound
+        autoscaling.knative.dev/maxReplicaCount: "10"  


This is a feature of the KPA implementation that we bundle today (once you implement it?), so I'd rather not document this here because it is not within the scope of conformance (other KPA implementations may choose to ignore these).

This feels like it belongs in documentation for our KPA plugin. cc @josephburnett

Ack that this feels a bit crazy until we can actually swap in different autoscalers :)

It is (as implemented in this PR) a feature of the bundled KPA, and it's something that (as a user) I'd like other KPA implementations to have.

They can, of course, ignore these annotations, but I'd like Knative to recommend them to minimize user confusion.

As implemented here, these annotations are applicable to both KPA (directly and propagating from Revision) and Revision (directly and via revisionTemplate).

As a user I don't want to know how auto-scaling works. All I care about is that it is present and I can set bounds. It is great to be able to set these bounds declaratively (via revisionTemplate) and very inconvenient to wait until KPA object magically appears once the Revision is created and set them there.

Addressed this above in https://github.com/knative/serving/pull/1980/files#r218251531.

I think we should leave this out of the spec. We propagate all annotation from Service -> Configuration -> Revision so you can use this knob at whatever level you want. But the entities along that path should not necessarily be aware of, or couple with, those annotations. Min/max is an implementation specific knob.

mattmoor · 2018-09-08T02:04:56Z

pkg/apis/autoscaling/register.go

+	ClassAnnotationKey = GroupName + "/class"
+
+	MinScaleAnnotationKey = GroupName + "/minReplicaCount"
+	MaxScaleAnnotationKey = GroupName + "/maxReplicaCount"


I feel like we should define these in pkg/autoscaler, not alongside our types (continuation of the comment on spec.md)

👍 to @mattmoor.

IMHO honoring ResourceQuota specs for the given namespace of the ReplicaSet wrt maxReplicaCount might be very wise a design.

RFC @mattmoor @imikushin 🙏

@mattmoor The issue this PR is addressing (#1656) suggests the placement and rationale for these annotations:

I would like to add support for two KPA annotations (here) to constrain the min and max number of pods.

autoscaling.knative.dev/minReplicaCount autoscaling.knative.dev/maxReplicaCount

The annotations would be in the user-facing domain (not autoscaling.internal.knative.dev) because they are intended to be set by the Operator to place bounds on resource consumption.

They would also apply to other autoscaling implementations specified by autoscaling.knative.dev/class (if the implementation supports min/max constraints.)

@nrshrivatsan ResourceQuotas address a similar but different concern IMO and they apply to the whole namespace. Scale bounds only apply to specific revisions and enable both upper and lower bounds (kinda opposite to quotas' purpose).

nrshrivatsan

Suggestions on abstracting specifics into pkg/autoscaler

nrshrivatsan · 2018-09-08T22:28:36Z

pkg/apis/autoscaling/register.go

+	ClassAnnotationKey = GroupName + "/class"
+
+	MinScaleAnnotationKey = GroupName + "/minReplicaCount"
+	MaxScaleAnnotationKey = GroupName + "/maxReplicaCount"


👍 to @mattmoor.

IMHO honoring ResourceQuota specs for the given namespace of the ReplicaSet wrt maxReplicaCount might be very wise a design.

RFC @mattmoor @imikushin 🙏

imikushin · 2018-09-11T17:37:01Z

@josephburnett Can you please review and/or mark as ok to test?

josephburnett

/lgtm

josephburnett · 2018-09-12T18:02:10Z

docs/spec/spec.md

        knative.dev/type: "function"  # One of "function" or "app"
+      annotations:
+        # +optional. When not specified, the revision can scale down to 0 pods
+        autoscaling.knative.dev/minReplicaCount: "2"  


@cooperneil, what do you think about these annotations for providing Knative autoscaling bounds?

Thanks @imikushin for doing this! I think my suggestion to use the word "replica" is flawed. Knative doesn't talk about "replicas", "instances", "clones" etc.. It only talks about "containers". This was the reason I eventually chose the field name "ContainerConcurrency" in the Revision spec.

I would like to change these annotations to:

autoscaling.knative.dev/minContainerCount autoscaling.knative.dev/maxContainerCount

What do you think?

I didn't like the choice of names in the issue either, but thought I'd try to implement and see where it leads. I'd like to suggest:

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

If you insist on (min|max)ContainerCount I don't mind either. Let me know what you like more and I'll update the PR accordingly.

@josephburnett The reasons behind minScale/maxScale:

shorter

don't talk about the actual things being scaled (rumors are: pods might replace containers in revisionTemplate)

Ping @josephburnett !

🐸 @josephburnett @mattmoor

I'd like to update this with whatever you guys decide

@imikushin, yes, I think that autoscaling.knative.dev/minScale and autoscaling.knative.dev/maxScale are better names for the same reasons.

@mattmoor and @cooperneil, if you agree with these names, let's go with it!

I've chatted offline with @mattmoor about how to use these annotations, and I'll restate my understanding here.

The purpose of annotations (over the strongly-typed spec) is to allow users to reach under the covers and tune a component they know is there. In this case, we want to tune the specific implementation of the KPA interface that comes with Knative. The autoscaling.knative.dev/class identifies the particular KPA implementation being used and will remain with the KPA. However other implementation-specific annotations such as autoscaling.knative.dev/minScale and autoscaling.knative.dev/maxScale will remain with the implementation, even if it is separated (at some later time.)

As such, my recommendation is to go ahead with the autoscaling.knative.dev/minScale and autoscaling.knative.dev/maxScale annotation names. But do not document them in the Revision spec. Document them somewhere else (maybe in the https://github.com/knative/serving/blob/master/pkg/autoscaler/README.md file).

imikushin · 2018-09-20T02:11:07Z

/meow

knative-prow-robot · 2018-09-20T02:11:08Z

@imikushin:

Details

In response to this:

/meow

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.

# Conflicts: # pkg/reconciler/v1alpha1/autoscaling/kpa_scaler.go

imikushin · 2018-09-22T00:18:16Z

Resolved merge conflict for the 4th time!

/shrug

josephburnett · 2018-09-26T16:44:05Z

/approve
/lgtm
/meow

knative-prow-robot · 2018-09-26T16:44:07Z

@josephburnett:

Details

In response to this:

/approve
/lgtm
/meow

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.

josephburnett · 2018-09-26T16:59:26Z

/assign @mattmoor

vdemeester · 2018-09-26T17:21:59Z

SGTM (but not yet familiar enough with the code base 😅)

mattmoor

So I think with the requested changes, the only thing that should need my approval is the revision controller stuff, which LGTM.

I sent #2098 to correct the autoscaling API OWNERS.

mattmoor · 2018-09-27T21:40:36Z

pkg/apis/serving/v1alpha1/metadata_validation.go

 		}
 	}
+
+	if err := validateScaleBoundAnnotations(meta.GetAnnotations()); err != nil {


This is KPA specific, so can we add it to the KPA validation?

mattmoor · 2018-09-27T22:01:38Z

pkg/reconciler/v1alpha1/autoscaling/kpa_scaler.go

+	return 0
+}
+
+func getScaleBounds(ctx context.Context, kpa *kpa.PodAutoscaler) (int32, int32) {


Should this be an accessor on the KPA resource itself?

* Validate metadata on KPA (including scale bounds) * Add kpa.ScaleBounds() accessor method

# Conflicts: # pkg/reconciler/v1alpha1/autoscaling/kpa_scaler.go

knative-metrics-robot · 2018-10-02T18:37:27Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/autoscaling/v1alpha1/kpa_types.go	100.0%	63.2%	-36.8
pkg/reconciler/v1alpha1/autoscaling/kpa_scaler.go	63.6%	68.0%	4.4
pkg/reconciler/v1alpha1/revision/cruds.go	97.4%	97.4%	0.1

imikushin · 2018-10-02T18:42:10Z

/unshrug

knative-prow-robot · 2018-10-02T18:42:11Z

@imikushin: ¯\_(ツ)_/¯

Details

In response to this:

/unshrug

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.

imikushin · 2018-10-02T19:15:42Z

This needs another /lgtm

@mattmoor I believe I've addressed your feedback (thanks!)

josephburnett · 2018-10-02T19:55:55Z

/lgtm

josephburnett · 2018-10-02T19:58:32Z

/approve

josephburnett · 2018-10-02T20:22:47Z

/assign @mattmoor

mattmoor · 2018-10-03T03:37:18Z

/approve

knative-prow-robot · 2018-10-03T03:37:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: imikushin, josephburnett, mattmoor

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~docs/scaling/OWNERS~~ [josephburnett,mattmoor]
~~pkg/apis/OWNERS~~ [mattmoor]
~~pkg/reconciler/OWNERS~~ [mattmoor]

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

Scale bounds annotations

252686d

Implements knative#1656

knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 6, 2018

knative-prow-robot requested review from mattmoor and tcnghia September 6, 2018 22:52

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 6, 2018

Add kpa scaler tests for min/max scale bounds

bcf3196

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 7, 2018

imikushin changed the title ~~[WIP] Scale bounds annotations~~ Scale bounds annotations Sep 7, 2018

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 7, 2018

knative-prow-robot assigned mattmoor Sep 7, 2018

knative-prow-robot assigned josephburnett Sep 7, 2018

knative-prow-robot requested a review from josephburnett September 7, 2018 06:20

glyn reviewed Sep 7, 2018

View reviewed changes

pkg/reconciler/v1alpha1/autoscaling/kpa_scaler.go Outdated Show resolved Hide resolved

glyn reviewed Sep 7, 2018

View reviewed changes

pkg/reconciler/v1alpha1/autoscaling/kpa_scaler.go Outdated Show resolved Hide resolved

glyn reviewed Sep 7, 2018

View reviewed changes

pkg/reconciler/v1alpha1/autoscaling/kpa_scaler.go Outdated Show resolved Hide resolved

Ivan Mikushin added 3 commits September 7, 2018 10:27

Address review comments

7e762b8

Make sure explicitly specified scale bounds are greater than 0.

Add scaleBounds annotations validation

f7c2ab5

Plus unit tests

Update docs

7b4ea2c

Add scale bounds to spec.md

mattmoor reviewed Sep 8, 2018

View reviewed changes

nrshrivatsan reviewed Sep 8, 2018

View reviewed changes

Fix copyright header and formatting

4eaccec

josephburnett reviewed Sep 12, 2018

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2018

Improve coverage

49e7e58

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2018

Merge remote-tracking branch 'upstream/master' into scale-bounds

58a0088

Ivan Mikushin added 2 commits September 21, 2018 17:05

Merge remote-tracking branch 'upstream/master' into scale-bounds

153789e

# Conflicts: # pkg/reconciler/v1alpha1/autoscaling/kpa_scaler.go

i didn't break that test!

77e4d11

knative-prow-robot added the ¯\_(ツ)_/¯ label Sep 22, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2018

mattmoor reviewed Sep 27, 2018

View reviewed changes

Address more comments (thanks @mattmoor)

7e74103

* Validate metadata on KPA (including scale bounds) * Add kpa.ScaleBounds() accessor method

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 2, 2018

Merge branch 'master' into scale-bounds

fc17f24

# Conflicts: # pkg/reconciler/v1alpha1/autoscaling/kpa_scaler.go

knative-prow-robot removed the ¯\_(ツ)_/¯ label Oct 2, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 2, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2018

knative-prow-robot merged commit 1fa7f09 into knative:master Oct 3, 2018

imikushin deleted the scale-bounds branch October 9, 2018 17:00

Scale bounds annotations #1980

Scale bounds annotations #1980

Uh oh!

Conversation

imikushin commented Sep 6, 2018 • edited by yanweiguo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

TODO

Uh oh!

imikushin commented Sep 7, 2018

Uh oh!

markusthoemmes commented Sep 7, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrshrivatsan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imikushin commented Sep 11, 2018

Uh oh!

josephburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imikushin commented Sep 20, 2018

Uh oh!

knative-prow-robot commented Sep 20, 2018

Uh oh!

imikushin commented Sep 22, 2018

Uh oh!

josephburnett commented Sep 26, 2018

Uh oh!

knative-prow-robot commented Sep 26, 2018

Uh oh!

josephburnett commented Sep 26, 2018

Uh oh!

vdemeester commented Sep 26, 2018

Uh oh!

mattmoor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

imikushin commented Sep 6, 2018 •

edited by yanweiguo

Loading