wI2L / kubectl-vpa-recommendation

kubectl plugin to compare VPA recommendations to actual resources requests
MIT License
35 stars 3 forks source link

Division by zero in v0.6.0 #3

Open EraYaN opened 1 year ago

EraYaN commented 1 year ago

Running: kubectl vpa-recommendation --show-stats --output wide --sort-columns mem-diff,cpu-diff

I get the following error:

``` NAME MODE TARGET CPU REQUEST CPU TARGET % CPU DIFF MEMORY REQUEST MEMORY TARGET % MEMORY DIFF panic: division by zero goroutine 1 [running]: math/big.nat.div({0x0?, 0x1?, 0x0?}, {0x0?, 0x0?, 0xc00012f0d8?}, {0x0?, 0xc000708450?, 0x1000000000001?}, {0x0, ...}) /opt/hostedtoolcache/go/1.19.2/x64/src/math/big/natdiv.go:507 +0x34b math/big.(*Int).Quo(0xc0005c7410, 0xc0005c73b0, 0xc0005c73e0) /opt/hostedtoolcache/go/1.19.2/x64/src/math/big/int.go:211 +0x78 gopkg.in/inf%2ev0.(*Dec).quoRem(0xc0005c7410, 0xc0005c73b0, 0xc0005c73e0, 0x0, 0x0, 0x0, 0x0) /home/runner/go/pkg/mod/gopkg.in/inf.v0@v0.9.1/dec.go:331 +0x654 gopkg.in/inf%2ev0.(*Dec).quo(0xc000422c80?, 0x0?, 0x28?, {0x1b350e0?, 0x269f140?}, {0x1b3bbf8, 0xc0000b3610}) /home/runner/go/pkg/mod/gopkg.in/inf.v0@v0.9.1/dec.go:267 +0x14b gopkg.in/inf%2ev0.(*Dec).QuoRound(...) /home/runner/go/pkg/mod/gopkg.in/inf.v0@v0.9.1/dec.go:257 github.com/wI2L/kubectl-vpa-recommendation/cli.table.meanQuantities({0x0?, 0x0, 0x176d820?}, 0xc0005c7201?) /home/runner/work/kubectl-vpa-recommendation/kubectl-vpa-recommendation/cli/table.go:299 +0xc5 github.com/wI2L/kubectl-vpa-recommendation/cli.table.printStats({0x0?, 0x0, 0x0}, {0x1b33fc0?, 0xc000012018?}) /home/runner/work/kubectl-vpa-recommendation/kubectl-vpa-recommendation/cli/table.go:243 +0x604 github.com/wI2L/kubectl-vpa-recommendation/cli.table.Print({0x0, 0x0, 0x0}, {0x1b33fc0, 0xc000012018}, 0xc0001f0360) /home/runner/work/kubectl-vpa-recommendation/kubectl-vpa-recommendation/cli/table.go:200 +0x618 github.com/wI2L/kubectl-vpa-recommendation/cli.(*CommandOptions).Execute(0xc000424000) /home/runner/work/kubectl-vpa-recommendation/kubectl-vpa-recommendation/cli/command.go:204 +0x66b github.com/wI2L/kubectl-vpa-recommendation/cli.(*CommandOptions).Run(0x128aee0?, 0xc0000f9c00?, {0xc0003fc5b0, 0x0, 0x7}) /home/runner/work/kubectl-vpa-recommendation/kubectl-vpa-recommendation/cli/command.go:103 +0x7b github.com/spf13/cobra.(*Command).execute(0xc000357400, {0xc00003e090, 0x7, 0x7}) /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.3.0/command.go:860 +0x663 github.com/spf13/cobra.(*Command).ExecuteC(0xc000357400) /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.3.0/command.go:974 +0x3bd github.com/spf13/cobra.(*Command).Execute(...) /home/runner/go/pkg/mod/github.com/spf13/cobra@v1.3.0/command.go:902 k8s.io/component-base/cli.run(0xc000357400) /home/runner/go/pkg/mod/k8s.io/component-base@v0.23.4/cli/run.go:146 +0x317 k8s.io/component-base/cli.Run(0xc0000e7d00?) /home/runner/go/pkg/mod/k8s.io/component-base@v0.23.4/cli/run.go:46 +0x1d main.main() /home/runner/work/kubectl-vpa-recommendation/kubectl-vpa-recommendation/cmd/kubectl-vpa-recommendation/kubectl-vpa-recommendation.go:44 +0x145 ```

It seems like meanQuantities chokes on some of our vpa's recommandations. I have dumped the vpas to yaml but there is a ton of internal info in those, so I'm not sure if I can share those.

Most of them have one container with something like this:

- containerName: webservices
  lowerBound:
    cpu: 15m
    memory: "248036781"
  target:
    cpu: 15m
    memory: "272061154"
  uncappedTarget:
    cpu: 15m
    memory: "272061154"
  upperBound:
    cpu: 15m
    memory: "336054630"

and a couple will have two containers.

EDIT: Alright kubectl vpa-recommendation doesn't actually return any VPAs. All our VPAs target a custom CRD, might that be the issue? Seems like it doesn't realize the number of matched VPAs is actually 0 because they are all targeting some unsupported kind.

wI2L commented 1 year ago

Hello,

meanQuantities is part of the --show-stats calculations of the "global stats" for all VPAs of a namespace. The division by zero and the fact that kubectl vpa-recommendation that returns no VPA clearly might indeed seems to be the source of the problem.

The doc states in the Limitations section:

Unlike the official VPA recommender, which is fully generic and handle any kind of "scalable" resources, the plugin recognize only some well-known controllers such as: CronJob, DaemonSet, Deployment, Job, ReplicaSet, ReplicationController, StatefulSet.

https://github.com/wI2L/kubectl-vpa-recommendation/blob/8f936b6b4630eae6d913fcaaaec7696b597450d8/cli/command.go#L229

Did you get logs about the VPA target kind being unsupported ? When the plugin cannot bind a VPA with it's target, it doesn't append it to the "table". And the len of the yable is used for the states computations: https://github.com/wI2L/kubectl-vpa-recommendation/blob/8f936b6b4630eae6d913fcaaaec7696b597450d8/cli/table.go#L299

I'll fix the panic.

Regarding your use case, the plugin would need to be able to fetch any kind, tho this might cause other issues (a CRD with the /scale subresource will probably not have the same spec for resource requests/limits, which are used for the computation of the stats):

https://github.com/wI2L/kubectl-vpa-recommendation/blob/master/vpa/target.go#L73

Do you mind sharing some information about the spec of your custom resources?

EraYaN commented 1 year ago
kubectl vpa-recommendation -v5

gives a ton of I0823 13:18:59.714861 13838 command.go:231] couldn't get target for vpa default/<name> lines.

Our CRD does have a scale subresource, so the newer versions of the vpa recommender force us to update our targets from the Deployments to the CRD.

Our CRD has this in its Spec:

Resources *corev1.ResourceRequirements `json:"resources"`

So that is not the same like it is for deployments where resources are per container.

For us it would be acceptable to let us add a command line option like --kind="KindName" --resourcePath=".spec.resources". Or some way of helping it resolve to the pod selectors themselves like what the recommender does.

Maybe you can even depend on the recommender to steal this piece of code to get the selectors from the scale resource: https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/pkg/target/fetcher.go#L119-L146 then you can map to the pods and grab the current resources from that.

We define those as follows:

+kubebuilder:subresource:scale:specpath=.spec.replicas,statuspath=.status.deployment.replicas,selectorpath=.status.scale.selector
wI2L commented 1 year ago

@EraYaN I'll fix the panic first, but to support custom target kinds, this will require a bit of refactor, and I won't have time to work on it immediately. If you are willing to work on it, let me know.

For us it would be acceptable to let us add a command line option like --kind="KindName" --resourcePath=".spec.resources". Or some way of helping it resolve to the pod selectors themselves like what the recommender does.

I don't think a --kind="KindName" flag is necessary, the target reference in the VPA should have enough information to fetch the custom resource, as long as it has a /scale subresource (same way the VPA recommender does it).

Regarding the second flag, --resourcePath=".spec.resources", I agree. By default the plugin should try to look into the common path .spec.template.spec which works for well-known controllers, otherwise this should be specified manually for custom paths.

EraYaN commented 1 year ago

Alright I gave it a go in #4