Add resource validation #4798

mowangdk · 2022-04-07T12:19:43Z

Which component this PR applies to?

vertical-pod-autoscaler

What type of PR is this?

/kind bug

What this PR does / why we need it:

Add Validation for VPA

Which issue(s) this PR fixes:

Fixes #4790

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

linux-foundation-easycla · 2022-04-07T12:19:45Z

The committers listed above are authorized under a signed CLA.

✅ login: mowangdk / name: geyingqi (68d7593)

k8s-ci-robot · 2022-04-07T12:19:50Z

Welcome @mowangdk!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

mowangdk · 2022-04-07T12:47:02Z

@jbartosik PTAL

jbartosik

Thank you for the change. I left a few comments. (Also I'll respond faster in the future)

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler_test.go

mowangdk · 2022-04-19T02:49:03Z

@jbartosik comments fixed, ptal

jbartosik

Thank you. I think this is close to being ready for submit.

There is one change needed (validate resolution of max even if there is no corresponding min). I also left a few requests for smaller improvements.

jbartosik · 2022-04-19T07:10:47Z

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler_test.go

+			expectError: fmt.Errorf("Memory [%v] must be a whole number of bytes", resource.MustParse("100m")),
+		},
+		{
+			name: "bad manAllowed cpu value",


typo: s/manAllowed/maxAllowed/

jbartosik · 2022-04-19T07:13:56Z

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler_test.go

@@ -36,6 +37,7 @@ func TestValidateVPA(t *testing.T) {
 	badMinReplicas := int32(0)
 	validMinReplicas := int32(1)
 	badScalingMode := vpa_types.ContainerScalingMode("bad")
+	badCPUValue := resource.MustParse("187500u")


Please use resource.MustParse("187500u") directly, like with other values.

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler_test.go

jbartosik · 2022-04-19T07:50:07Z

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go

-				if found && max.Cmp(min) < 0 {
-					return fmt.Errorf("max resource for %v is lower than min", resource)
+				if found {
+					if err := validate(resource, max); err != nil {


We should check if max has correct resolution in a loop:

for resource, min := range policy.MaxAllowed { if err := validate(resource, max); err != nil { //...

current implementation doesn't validate max when min is not specified.

jbartosik · 2022-04-19T07:52:18Z

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go

@@ -133,9 +136,17 @@ func validateVPA(vpa *vpa_types.VerticalPodAutoscaler, isCreate bool) error {
 				}
 			}
 			for resource, min := range policy.MinAllowed {
+				if err := validate(resource, min); err != nil {
+					return err


It would be nice to mention in the error message which field has improper value.

You can add this information here:

return fmt.Errorf("MinAllowed: %s", err)

or pass the name of the field to validate so it can include it in the error message it returns.

jbartosik · 2022-04-19T07:53:02Z

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go

@@ -157,3 +168,27 @@ func validateVPA(vpa *vpa_types.VerticalPodAutoscaler, isCreate bool) error {

 	return nil
 }
+
+func validate(name corev1.ResourceName, val apires.Quantity) error {


Please make the name more specific. For example validateResourceResolution

mowangdk · 2022-04-22T10:07:04Z

@jbartosik ptal, There is no way to call string() directly on resource.MustParse("187500u") return value , so i kept badCPUResource

jbartosik · 2022-04-22T15:34:59Z

Thanks, just a couple small comments.

Please squash commits.

Joachim ptal, There is no way to call string() directly on resource.MustParse("187500u") return value , so i kept badCPUResource

I think you can do this the same way you're handling other resource amounts.

jbartosik

Looks good, only a few small comments left:

No need to change if
Consistent approach to amounts in unit tests
Please squash commits

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go

mowangdk · 2022-04-25T03:33:38Z

@jbartosik make some changes according to your comments, ptal

jbartosik

Thank you

/lgtm
/approve

k8s-ci-robot · 2022-04-25T06:23:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jbartosik, mowangdk

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:

~~vertical-pod-autoscaler/OWNERS~~ [jbartosik]

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

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 7, 2022

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 7, 2022

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 7, 2022

k8s-ci-robot requested review from jbartosik and krzysied April 7, 2022 12:20

mowangdk force-pushed the fix/add_resource_validation branch from 1ed2ae4 to 68d7593 Compare April 7, 2022 12:21

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 7, 2022

mowangdk force-pushed the fix/add_resource_validation branch from 68d7593 to 4ce04dd Compare April 7, 2022 12:26

jbartosik added the area/vertical-pod-autoscaler label Apr 14, 2022

jbartosik requested changes Apr 14, 2022

View reviewed changes

mowangdk force-pushed the fix/add_resource_validation branch from 4ce04dd to 93febe6 Compare April 16, 2022 03:58

k8s-ci-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 Apr 16, 2022

mowangdk force-pushed the fix/add_resource_validation branch from 93febe6 to 48b4e45 Compare April 17, 2022 06:22

jbartosik requested changes Apr 19, 2022

View reviewed changes

mowangdk force-pushed the fix/add_resource_validation branch from dfc6601 to 9a80be0 Compare April 22, 2022 10:04

jbartosik approved these changes Apr 22, 2022

View reviewed changes

vertical-pod-autoscaler/pkg/admission-controller/resource/vpa/handler.go Outdated Show resolved Hide resolved

jbartosik self-requested a review April 22, 2022 15:36

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2022

mowangdk force-pushed the fix/add_resource_validation branch from 9a80be0 to c85142b Compare April 23, 2022 06:24

Add resource resolution check for VPA

82f6707

mowangdk force-pushed the fix/add_resource_validation branch from c85142b to 82f6707 Compare April 23, 2022 06:30

jbartosik approved these changes Apr 25, 2022

View reviewed changes

k8s-ci-robot assigned jbartosik Apr 25, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2022

k8s-ci-robot merged commit 59ef519 into kubernetes:master Apr 25, 2022

dixneuf19 mentioned this pull request Oct 9, 2022

[stable/vpa] Update vpa to 0.11.0 FairwindsOps/charts#960

Merged

4 tasks

mowangdk mentioned this pull request Apr 2, 2024

REQUEST: New membership for mowangdk kubernetes/org#4860

Closed

9 tasks

adrianmoisey mentioned this pull request May 13, 2024

VPA doesn't accept 1.2Gi in .spec.ResourcePolicy.MinAllowed #6819

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resource validation #4798

Add resource validation #4798

mowangdk commented Apr 7, 2022

linux-foundation-easycla bot commented Apr 7, 2022 •

edited

Loading

k8s-ci-robot commented Apr 7, 2022

mowangdk commented Apr 7, 2022

jbartosik left a comment

mowangdk commented Apr 19, 2022

jbartosik left a comment

jbartosik Apr 19, 2022

jbartosik Apr 19, 2022

jbartosik Apr 19, 2022

jbartosik Apr 19, 2022

jbartosik Apr 19, 2022

mowangdk commented Apr 22, 2022

jbartosik commented Apr 22, 2022

jbartosik left a comment

mowangdk commented Apr 25, 2022

jbartosik left a comment

k8s-ci-robot commented Apr 25, 2022

Add resource validation #4798

Add resource validation #4798

Conversation

mowangdk commented Apr 7, 2022

Which component this PR applies to?

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla bot commented Apr 7, 2022 • edited Loading

k8s-ci-robot commented Apr 7, 2022

mowangdk commented Apr 7, 2022

jbartosik left a comment

Choose a reason for hiding this comment

mowangdk commented Apr 19, 2022

jbartosik left a comment

Choose a reason for hiding this comment

jbartosik Apr 19, 2022

Choose a reason for hiding this comment

jbartosik Apr 19, 2022

Choose a reason for hiding this comment

jbartosik Apr 19, 2022

Choose a reason for hiding this comment

jbartosik Apr 19, 2022

Choose a reason for hiding this comment

jbartosik Apr 19, 2022

Choose a reason for hiding this comment

mowangdk commented Apr 22, 2022

jbartosik commented Apr 22, 2022

jbartosik left a comment

Choose a reason for hiding this comment

mowangdk commented Apr 25, 2022

jbartosik left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 25, 2022

linux-foundation-easycla bot commented Apr 7, 2022 •

edited

Loading