Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
sanposhiho opened this issue May 13, 2024 · 11 comments
Closed

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

sanposhiho opened this issue May 13, 2024 · 11 comments
Labels
area/vertical-pod-autoscaler kind/support Categorizes issue or PR as a support question.

Comments

@sanposhiho
Copy link
Member

Which component are you using?:

vertical-pod-autoscaler

What version of the component are you using?:

Component version: 0.14.0 (provided within GKE clusters)

What k8s version are you using (kubectl version)?:

kubectl version Output
$ kubectl version
WARNING: This version information is deprecated and will be replaced with the output from kubectl version --short.  Use --output=yaml|json to get the full version.
Client Version: version.Info{Major:"1", Minor:"27", GitVersion:"v1.27.1", GitCommit:"4c9411232e10168d7b050c49a1b59f6df9d7ea4b", GitTreeState:"clean", BuildDate:"2023-04-14T13:14:41Z", GoVersion:"go1.20.3", Compiler:"gc", Platform:"darwin/arm64"}
Kustomize Version: v5.0.1
Server Version: version.Info{Major:"1", Minor:"27", GitVersion:"v1.27.12-gke.1115000", GitCommit:"885327b7f1bebce409c843425b4688e3eeed33f4", GitTreeState:"clean", BuildDate:"2024-03-28T09:16:53Z", GoVersion:"go1.21.8 X:boringcrypto", Compiler:"gc", Platform:"linux/amd64"}

What environment is this in?:

GKE

What did you expect to happen?:

VPA accepts 1.2Gi in .spec.ResourcePolicy.MinAllowed

What happened instead?:

It doesn't.

$ kubectl edit vpa awesome-app -n mercari

error: verticalpodautoscalers.autoscaling.k8s.io "awesome-app" could not be patched: admission webhook "gke-vpa.k8s.io" denied the request: MinAllowed: Memory [{{0 0} {0xc070473f80}  BinarySI}] must be a whole number of bytes

How to reproduce it (as minimally and precisely as possible):

Create VPA with MinAllowed with 1.2Gi

Anything else we need to know?:

@sanposhiho sanposhiho added the kind/bug Categorizes issue or PR as related to a bug. label May 13, 2024
@sanposhiho
Copy link
Member Author

Also, I don't understand why minAllowed in this test case is "bad minAllowed memory value".

name: "bad minAllowed memory value",
vpa: vpa_types.VerticalPodAutoscaler{
Spec: vpa_types.VerticalPodAutoscalerSpec{
ResourcePolicy: &vpa_types.PodResourcePolicy{
ContainerPolicies: []vpa_types.ContainerResourcePolicy{
{
ContainerName: "loot box",
MinAllowed: apiv1.ResourceList{
cpu: resource.MustParse("1m"),
memory: resource.MustParse("100m"),
},
MaxAllowed: apiv1.ResourceList{
cpu: resource.MustParse("275m"),
memory: resource.MustParse("500M"),
},
},
},
},
},
},
expectError: fmt.Errorf("MinAllowed: Memory [%v] must be a whole number of bytes", resource.MustParse("100m")),

@sanposhiho
Copy link
Member Author

/area vertical-pod-autoscaler

@adrianmoisey
Copy link
Member

Also, I don't understand why minAllowed in this test case is "bad minAllowed memory value".

I think it's because the input is 100m, where it should be 100M

@adrianmoisey
Copy link
Member

I can confirm that this bug exists in VPA 1.1.1.

@adrianmoisey
Copy link
Member

It seems as though this issue is related to #4798

@sanposhiho
Copy link
Member Author

I think it's because the input is 100m, where it should be 100M

100m and 100M are different and both are valid memory value.

@voelzmo
Copy link
Contributor

voelzmo commented May 15, 2024

100m is 100 millibytes, which is 0.1 bytes, and the VPA wants (as the error message points out) a whole number of bytes.

The case for 1.2Gi is quite interesting, because if you convert this binary format into (decimal) bytes, you also end up with a fractional amount of bytes

1.2*1024*1024*1024=1288490188.8

My understanding is that internally k8s will also end up converting memory into decimal bytes and can only deal with whole bytes, therefore this validation ensures this. See #4774 for some context.

@voelzmo
Copy link
Contributor

voelzmo commented Jul 15, 2024

/close
/kind consulting

@k8s-ci-robot
Copy link
Contributor

@voelzmo: The label(s) kind/consulting cannot be applied, because the repository doesn't have them.

In response to this:

/close
/kind consulting

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-sigs/prow repository.

@k8s-ci-robot
Copy link
Contributor

@voelzmo: Closing this issue.

In response to this:

/close
/kind consulting

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-sigs/prow repository.

@voelzmo
Copy link
Contributor

voelzmo commented Jul 15, 2024

/kind support
/remove-kind bug

@k8s-ci-robot k8s-ci-robot added kind/support Categorizes issue or PR as a support question. and removed kind/bug Categorizes issue or PR as related to a bug. labels Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vertical-pod-autoscaler kind/support Categorizes issue or PR as a support question.
Projects
None yet
Development

No branches or pull requests

4 participants