Validation added for initialScale greater than maxScale (#14153) #14682

yp969803 · 2023-11-29T19:03:58Z

Proposed Changes

Added a validation for initialScale > maxScale
As initialScale > maxScale not making sense at all. As it is not depending on traffic.

Release Note

none

knative-prow · 2023-11-29T19:04:08Z

Hi @yp969803. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

dprotaso · 2023-12-07T20:37:23Z

/ok-to-test

dprotaso · 2023-12-07T20:38:56Z

pkg/apis/autoscaling/annotation_validation.go

+
+func validateMaxInitialScale(config *autoscalerconfig.Config, m map[string]string) *apis.FieldError{
+
+	max,errs := getIntGE0(m,MaxScaleAnnotation)


These annotations could be omitted from the revision so you would need to handle that

see

serving/pkg/apis/autoscaling/annotation_validation.go

Line 267 in aef6c59

if k, v, ok := InitialScaleAnnotation.Get(m); ok {

yp969803 · 2024-01-02T18:02:02Z

@dprotaso i have added the further annotations can you review

codecov · 2024-01-12T19:00:38Z

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (81149da) 86.05% compared to head (2579cc1) 85.81%.
Report is 39 commits behind head on main.

❗ Current head 2579cc1 differs from pull request most recent head 6ef38fa. Consider uploading reports for the commit 6ef38fa to get more accurate results

Files	Patch %	Lines
pkg/apis/autoscaling/annotation_validation.go	0.00%	29 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14682      +/-   ##
==========================================
- Coverage   86.05%   85.81%   -0.24%     
==========================================
  Files         197      197              
  Lines       14937    14974      +37     
==========================================
- Hits        12854    12850       -4     
- Misses       1774     1810      +36     
- Partials      309      314       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dprotaso · 2024-01-12T19:01:36Z

pkg/apis/autoscaling/annotation_validation.go

+     if (initial<0){
+		errs=errs.Also(&apis.FieldError{
+			Message: fmt.Sprintf("initial-scale=%d should be greater than 0", initial),
+			Paths: []string{MaxScaleAnnotationKey,InitialScaleAnnotationKey},
+		})
+	 }else if !config.AllowZeroInitialScale && initial==0 {
+		errs=errs.Also(&apis.FieldError{
+			Message: fmt.Sprintf("initial-scale=%d not allowed by cluster", initial),
+			Paths: []string{MaxScaleAnnotationKey,InitialScaleAnnotationKey},
+		})
+	 }else if max<=0{
+		errs=errs.Also(&apis.FieldError{
+			Message: fmt.Sprintf("max-scale=%d should be greate than 0",max),
+			Paths: []string{MaxScaleAnnotationKey,InitialScaleAnnotationKey},
+		})


I don't think these assertions are needed - we validate InitialScale here

serving/pkg/apis/autoscaling/annotation_validation.go

Line 265 in f1bd929

func validateInitialScale(config *autoscalerconfig.Config, m map[string]string) *apis.FieldError {

We validate max scale here

serving/pkg/apis/autoscaling/annotation_validation.go

Line 178 in f1bd929

func validateMinMaxScale(config *autoscalerconfig.Config, m map[string]string) *apis.FieldError {

We just need to perform validation if initial scale and max scale are present then do the comparison.

See here for examples:

serving/pkg/apis/autoscaling/annotation_validation.go

Line 178 in 2579cc1

func validateMinMaxScale(config *autoscalerconfig.Config, m map[string]string) *apis.FieldError {

pkg/apis/autoscaling/annotation_validation.go

knative-prow · 2024-01-16T14:49:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yp969803
Once this PR has been reviewed and has the lgtm label, please ask for approval from dprotaso. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/apis/OWNERS

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

yp969803 · 2024-01-16T14:56:51Z

@dprotaso i have updated the pr you can review, adding test is left

dprotaso

please fix the CI jobs

code doesn't compile
you'll need to use gofmt to format the code (linter is complaining)

Secondly, please add a unit test

dprotaso · 2024-01-16T15:02:35Z

pkg/apis/autoscaling/annotation_validation.go

+    if initial > max {
+        return &apis.FieldError{
+			Message: fmt.Sprintf("max-scale=%d is less than initial-scale=%d", max, initial),
+			Paths: []string{MaxScaleAnnotationKey,InitialScaleAnnotationKey},
+		}
+	}


This fails when the max scale annotation is not present since max will be equal to zero

…tive#14682)

yp969803 · 2024-01-16T17:02:58Z

@dprotaso updated the pr and added the unit test

…tive#14682) linting error removed from annotation_validation.go

knative-prow · 2024-01-18T11:36:14Z

@yp969803: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
unit-tests_serving_main	`6ef38fa`	link	true	`/test unit-tests`

Your PR dashboard.

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. I understand the commands that are listed here.

yp969803 · 2024-01-18T17:44:44Z

@dprotaso any stuff left?

dprotaso · 2024-01-18T18:38:22Z

CI is failing - see the GitHub checks - I'll wait for them to go green before reviewing again

dprotaso · 2024-02-16T16:19:26Z

Hey @yp969803 the unit tests that are failing is the code you've modified can you take a look?

dprotaso · 2024-03-05T18:19:49Z

Going to close this out due to inactivity.

knative-prow bot requested review from dprotaso and kauana November 29, 2023 19:04

knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 29, 2023

knative-prow bot added area/API API objects and controllers area/autoscale labels Nov 29, 2023

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 7, 2023

dprotaso reviewed Dec 7, 2023

View reviewed changes

yp969803 added a commit to yp969803/serving that referenced this pull request Jan 2, 2024

Annotions related to validateMaxInitialScale configured (#knative#14682)

1f2d995

knative-prow bot 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 Jan 2, 2024

yp969803 added a commit to yp969803/serving that referenced this pull request Jan 2, 2024

Validation added for maxScale > initialScale (knative#14682)

b106efa

yp969803 force-pushed the issue#14153 branch from 1f2d995 to b106efa Compare January 2, 2024 17:58

dprotaso requested changes Jan 12, 2024

View reviewed changes

knative-prow bot assigned dprotaso Jan 12, 2024

yp969803 added a commit to yp969803/serving that referenced this pull request Jan 16, 2024

Validation added for maxScale > initialScale (knative#14682)

657eb55

yp969803 force-pushed the issue#14153 branch from 2579cc1 to 657eb55 Compare January 16, 2024 14:48

yp969803 added a commit to yp969803/serving that referenced this pull request Jan 16, 2024

Validation added for maxScale > initialScale (knative#14682)

84be57f

yp969803 force-pushed the issue#14153 branch from 657eb55 to 84be57f Compare January 16, 2024 14:54

knative-prow bot 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 Jan 16, 2024

dprotaso requested changes Jan 16, 2024

View reviewed changes

yp969803 added a commit to yp969803/serving that referenced this pull request Jan 16, 2024

Validation added for initialScale greater than maxScale (knative#14682)

18c2366

yp969803 added a commit to yp969803/serving that referenced this pull request Jan 16, 2024

Validation and test added for initialScale greater than maxScale (kna…

11703c9

…tive#14682)

yp969803 force-pushed the issue#14153 branch from 84be57f to 11703c9 Compare January 16, 2024 17:01

knative-prow bot 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 Jan 16, 2024

Validation and test added for initialScale greater than maxScale (kna…

6ef38fa

…tive#14682) linting error removed from annotation_validation.go

yp969803 force-pushed the issue#14153 branch from 11703c9 to 6ef38fa Compare January 18, 2024 11:23

dprotaso closed this Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validation added for initialScale greater than maxScale (#14153) #14682

Validation added for initialScale greater than maxScale (#14153) #14682

yp969803 commented Nov 29, 2023

knative-prow bot commented Nov 29, 2023

dprotaso commented Dec 7, 2023

dprotaso Dec 7, 2023

yp969803 commented Jan 2, 2024

codecov bot commented Jan 12, 2024 •

edited

Loading

dprotaso Jan 12, 2024

knative-prow bot commented Jan 16, 2024

yp969803 commented Jan 16, 2024

dprotaso left a comment

dprotaso Jan 16, 2024

yp969803 commented Jan 16, 2024

knative-prow bot commented Jan 18, 2024

yp969803 commented Jan 18, 2024

dprotaso commented Jan 18, 2024

dprotaso commented Feb 16, 2024

dprotaso commented Mar 5, 2024


		func validateMaxInitialScale(config autoscalerconfig.Config, m map[string]string) apis.FieldError{

		max,errs := getIntGE0(m,MaxScaleAnnotation)

Validation added for initialScale greater than maxScale (#14153) #14682

Validation added for initialScale greater than maxScale (#14153) #14682

Conversation

yp969803 commented Nov 29, 2023

Proposed Changes

knative-prow bot commented Nov 29, 2023

dprotaso commented Dec 7, 2023

dprotaso Dec 7, 2023

Choose a reason for hiding this comment

yp969803 commented Jan 2, 2024

codecov bot commented Jan 12, 2024 • edited Loading

Codecov Report

dprotaso Jan 12, 2024

Choose a reason for hiding this comment

knative-prow bot commented Jan 16, 2024

yp969803 commented Jan 16, 2024

dprotaso left a comment

Choose a reason for hiding this comment

dprotaso Jan 16, 2024

Choose a reason for hiding this comment

yp969803 commented Jan 16, 2024

knative-prow bot commented Jan 18, 2024

yp969803 commented Jan 18, 2024

dprotaso commented Jan 18, 2024

dprotaso commented Feb 16, 2024

dprotaso commented Mar 5, 2024

codecov bot commented Jan 12, 2024 •

edited

Loading