Add service initial scale annotation and validation #7678

taragu · 2020-04-22T14:43:24Z

Add new autoscaling annotation initialScale and validation.

/lint
/hold
for Part 1

Part 2 of #7682

Release Note

NONE

/assign @dprotaso @markusthoemmes @vagababov

knative-prow-robot

@taragu: 2 warnings.

In response to this:

Add new autoscaling annotation initialScale and validation.

/lint
/hold
for Part 1

Part 2 of #4098

Release Note
NONE
/assign @dprotaso @markusthoemmes @vagababov

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.

pkg/apis/autoscaling/annotation_validation.go

pkg/apis/autoscaling/register.go

taragu · 2020-04-24T19:42:40Z

/hold cancel

markusthoemmes · 2020-04-27T14:07:04Z

pkg/apis/autoscaling/annotation_validation.go

 	if len(anns) == 0 {
 		return nil
 	}
-	return validateMinMaxScale(anns).Also(validateFloats(anns)).Also(validateWindows(anns).Also(validateMetric(anns)))
+	return validateMinMaxScale(anns).Also(validateFloats(anns)).Also(validateWindows(anns).Also(validateMetric(anns).Also(validateInitialScale(allowInitScaleZero, anns))))


Please break this line.

markusthoemmes · 2020-04-27T14:07:45Z

pkg/apis/autoscaling/annotation_validation.go

@@ -37,11 +37,12 @@ func getIntGE0(m map[string]string, k string) (int64, *apis.FieldError) {
 	return i, nil
 }

-func ValidateAnnotations(anns map[string]string) *apis.FieldError {
+// ValidateAnnotations verifies the autoscaling annotations.
+func ValidateAnnotations(allowInitScaleZero bool, anns map[string]string) *apis.FieldError {


Should the context get passed in here? With more switches, we'd otherwise grow the signature.

@markusthoemmes there's a cyclic package import problem if we do so because we have to import knative.dev/serving/pkg/apis/config in knative.dev/serving/pkg/apis/autoscaling to get the configmap.

can't load package: import cycle not allowed package knative.dev/serving/pkg/apis/autoscaling imports knative.dev/serving/pkg/apis/config imports knative.dev/serving/pkg/autoscaler/config imports knative.dev/serving/pkg/apis/autoscaling

Is there any workaround for this problem?

Hmm, probably not an easy one 🤔. Maybe it's fine for now 🤷‍♂️

markusthoemmes · 2020-04-27T14:08:23Z

pkg/apis/autoscaling/register.go

+	// InitialScaleAnnotationKey is the annotation to specify the initial scale of
+	// a revision when a service is initially deployed. This number can be set to 0 iff
+	// allow-zero-initial-scale of config-autoscaler is true.
+	InitialScaleAnnotationKey = GroupName + "/initialScale"


nit: Put this next to min and maxscale.

markusthoemmes · 2020-04-27T14:08:41Z

pkg/apis/serving/metadata_validation.go

+	allowZeroInitialScale := false
+	if config.FromContext(ctx) != nil {
+		autoscalerConfig := config.FromContext(ctx).Autoscaler
+		if autoscalerConfig != nil {


Why would this ever be nil? Just for tests?

Yes this is just for the tests.

Probably annotate tests instead? If we have cruft, I'd rather have it in tests than in production code.

markusthoemmes · 2020-04-27T14:10:22Z

pkg/apis/serving/metadata_validation_test.go

@@ -160,11 +162,40 @@ func TestValidateObjectMetadata(t *testing.T) {
 			},
 		},
 		expectErr: (*apis.FieldError)(nil),
+	}, {
+		name: "cluster allows zero revision initial scale",
+		ctx: config.ToContext(context.Background(), asCfg(map[string]string{


Why not manually construct the config here?

markusthoemmes · 2020-04-27T14:10:53Z

pkg/apis/serving/v1/revision_validation.go

+	allowZeroInitialScale := false
+	if apisconfig.FromContext(ctx) != nil {
+		autoscalerConfig := apisconfig.FromContext(ctx).Autoscaler
+		if autoscalerConfig != nil {


Same as above: When would this ever be nil? Same for the config in general.

This is for the tests. Would it be better if we make sure all of the test contexts has the config instead of making this change?

I think so, yeah. This feels a little weird to reason about in production code.

markusthoemmes · 2020-04-27T14:11:35Z

pkg/apis/serving/v1alpha1/revision_validation.go

@@ -57,8 +58,15 @@ func (r *Revision) Validate(ctx context.Context) *apis.FieldError {

 // Validate ensures RevisionTemplateSpec is properly configured.
 func (rt *RevisionTemplateSpec) Validate(ctx context.Context) *apis.FieldError {
+	allowZeroInitialScale := false
+	if apisconfig.FromContext(ctx) != nil {


pkg/apis/autoscaling/annotation_validation.go

vagababov · 2020-04-27T16:01:02Z

pkg/apis/serving/metadata_validation.go

+	allowZeroInitialScale := false
+	if config.FromContext(ctx) != nil {
+		autoscalerConfig := config.FromContext(ctx).Autoscaler
+		if autoscalerConfig != nil {


Probably annotate tests instead? If we have cruft, I'd rather have it in tests than in production code.

pkg/apis/serving/v1/revision_validation_test.go

pkg/apis/autoscaling/annotation_validation.go

pkg/apis/serving/v1alpha1/revision_validation_test.go

taragu · 2020-04-28T17:44:16Z

@vagababov @markusthoemmes this is ready for another look

vagababov · 2020-04-28T17:58:11Z

pkg/apis/autoscaling/annotation_validation.go

+	return validateMinMaxScale(anns).Also(validateFloats(anns)).Also(validateWindows(anns).Also(validateMetric(anns).
+		Also(validateInitialScale(allowInitScaleZero, anns))))


Suggested change

return validateMinMaxScale(anns).Also(validateFloats(anns)).Also(validateWindows(anns).Also(validateMetric(anns).

Also(validateInitialScale(allowInitScaleZero, anns))))

return validateMinMaxScale(anns).Also(validateFloats(anns))

.Also(validateWindows(anns).Also(validateMetric(anns)

.Also(validateInitialScale(allowInitScaleZero, anns))))

I can make it three lines, but I'm getting compiler errors if I put the dot at the beginning of the new line

pkg/apis/autoscaling/v1alpha1/metric_validation.go

pkg/apis/autoscaling/v1alpha1/pa_validation.go

vagababov · 2020-04-28T18:34:16Z

pkg/apis/serving/metadata_validation_test.go

+		expectErr: (&apis.FieldError{Message: "", Paths: []string(nil), Details: ""}).Also(
+			(&apis.FieldError{Message: "", Paths: []string(nil), Details: ""}).Also(


Why do we have those empty errors? We should be able to delete them.

This is a result of the chained .Alsos

serving/pkg/apis/autoscaling/annotation_validation.go

Line 45 in 81781b5

return validateMinMaxScale(anns).Also(validateFloats(anns)).Also(validateWindows(anns).Also(validateMetric(anns).

If we have a test case for error in validating metric annotation and class annotation, we will also see the empty errors (but one less).

I just removed the empty errors here: #7752
They are completely redundant and unnecessary. FieldError code removes them anyway.

I still see the empty errors. Did you push?

🤦‍♀️ I missed those. they are updated now

Add new autoscaling annotation initialScale and validation.

markusthoemmes · 2020-04-29T13:40:19Z

pkg/apis/serving/v1alpha1/revision_validation.go

+	allowZeroInitialScale := false
+	autoscalerConfig := apisconfig.FromContextOrDefaults(ctx).Autoscaler
+	allowZeroInitialScale = autoscalerConfig.AllowZeroInitialScale


Suggested change

allowZeroInitialScale := false

autoscalerConfig := apisconfig.FromContextOrDefaults(ctx).Autoscaler

allowZeroInitialScale = autoscalerConfig.AllowZeroInitialScale

allowZeroInitialScale = apisconfig.FromContextOrDefaults(ctx).Autoscaler.AllowZeroInitialScale

For all other occasions too.

markusthoemmes · 2020-04-29T14:05:52Z

pkg/apis/serving/metadata_validation_test.go

@@ -147,11 +150,62 @@ func TestValidateObjectMetadata(t *testing.T) {
 				"testAnnotation": "testValue",
 			},
 		},
+	}, {
+		name: "revision initial scale not parseable",


Shouldn't all the negative test live close to the implementation, i.e. in the autoscaling validation?

I agree. Should we remove these negative test cases here then?

taragu · 2020-04-30T18:12:04Z

@markusthoemmes @vagababov would you please take another look?

vagababov

Mostly fine by me.
There seem some unresolved comments with Markus, so I'll let him approve.
/lgtm

vagababov · 2020-04-30T19:51:49Z

pkg/apis/serving/v1/revision_validation_test.go

+
+func autoscalerConfigCtx(allowInitialScaleZero bool, initialScale int) context.Context {
+	testConfigs := &config.Config{}
+	testConfigs.Autoscaler, _ = autoscalerconfig.NewConfigFromMap(map[string]string{})


Why not just populate the map?

knative-metrics-robot · 2020-04-30T20:27:57Z

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

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/metadata_validation.go	98.2%	98.3%	0.0
pkg/apis/serving/v1/revision_validation.go	94.4%	94.6%	0.2
pkg/apis/serving/v1alpha1/revision_validation.go	96.1%	96.2%	0.1

knative-test-reporter-robot · 2020-04-30T22:04:54Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-integration-tests	pull-knative-serving-integration-tests pull-knative-serving-integration-tests	2/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-integration-tests

dprotaso · 2020-05-04T19:58:18Z

/lgtm

dprotaso · 2020-05-04T20:04:02Z

/lgtm cancel
/approve

This should setup Markus to lgtm it and it should merge

markusthoemmes

/lgtm
/approve

knative-prow-robot · 2020-05-07T10:56:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, markusthoemmes, taragu

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/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-robot assigned dprotaso and markusthoemmes Apr 22, 2020

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2020

knative-prow-robot assigned vagababov Apr 22, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 22, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 22, 2020

knative-prow-robot reviewed Apr 22, 2020

View reviewed changes

pkg/apis/autoscaling/annotation_validation.go Show resolved Hide resolved

pkg/apis/autoscaling/register.go Outdated Show resolved Hide resolved

knative-prow-robot requested review from markusthoemmes and vagababov April 22, 2020 14:43

knative-prow-robot added area/API API objects and controllers area/autoscale labels Apr 22, 2020

taragu force-pushed the tara-init-scale-0-validations branch 2 times, most recently from fae5677 to 2cb3a98 Compare April 24, 2020 19:19

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 24, 2020

markusthoemmes reviewed Apr 27, 2020

View reviewed changes

vagababov reviewed Apr 27, 2020

View reviewed changes

markusthoemmes reviewed Apr 27, 2020

View reviewed changes

pkg/apis/autoscaling/annotation_validation.go Outdated Show resolved Hide resolved

vagababov reviewed Apr 27, 2020

View reviewed changes

pkg/apis/autoscaling/annotation_validation.go Outdated Show resolved Hide resolved

vagababov reviewed Apr 27, 2020

View reviewed changes

pkg/apis/serving/v1alpha1/revision_validation_test.go Outdated Show resolved Hide resolved

vagababov reviewed Apr 28, 2020

View reviewed changes

Tara Gu added 6 commits April 29, 2020 08:53

Add service initial scale annotation and validation

ea5e71b

Add new autoscaling annotation initialScale and validation.

Review

ce19fbe

Review

4eb0184

Review

6cf9370

Review

c197b66

Review

1596cc9

taragu force-pushed the tara-init-scale-0-validations branch from 37bfa31 to 1596cc9 Compare April 29, 2020 12:54

markusthoemmes reviewed Apr 29, 2020

View reviewed changes

Review

dc8589f

Missed updates

dcc8b5a

vagababov reviewed Apr 30, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2020

Review: simplify test CM creation

aae5382

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 4, 2020

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels May 4, 2020

markusthoemmes approved these changes May 7, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2020

knative-prow-robot merged commit fb75f94 into knative:master May 7, 2020

taragu deleted the tara-init-scale-0-validations branch July 29, 2020 11:44

		return validateMinMaxScale(anns).Also(validateFloats(anns)).Also(validateWindows(anns).Also(validateMetric(anns).
		Also(validateInitialScale(allowInitScaleZero, anns))))

		expectErr: (&apis.FieldError{Message: "", Paths: []string(nil), Details: ""}).Also(
		(&apis.FieldError{Message: "", Paths: []string(nil), Details: ""}).Also(

Add service initial scale annotation and validation #7678

Add service initial scale annotation and validation #7678

Conversation

taragu commented Apr 22, 2020 • edited Loading

knative-prow-robot left a comment

Choose a reason for hiding this comment

taragu commented Apr 24, 2020

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

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

taragu commented Apr 28, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taragu commented Apr 30, 2020

vagababov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Apr 30, 2020

knative-test-reporter-robot commented Apr 30, 2020

dprotaso commented May 4, 2020

dprotaso commented May 4, 2020

markusthoemmes left a comment

Choose a reason for hiding this comment

knative-prow-robot commented May 7, 2020

taragu commented Apr 22, 2020 •

edited

Loading