FairwindsOps / goldilocks

Get your resource requests "Just Right"
https://fairwinds.com
Apache License 2.0
2.56k stars 136 forks source link

Use mebibytes (Mi) instead of megabites (M) in memory recommendation #522

Open tsutsarin opened 2 years ago

tsutsarin commented 2 years ago

Goldilocks dashboard shows memory recommendation in megabytes units (M), which is 10^6 of bytes. Mebibytes (Mi) instead use as base powers of 2.

Usually Kubernetes memory requests are specified in Mi and also different tools, like default Prometheus Operator Grafana dashboard show in MiB units.

As a feature option to switch between different types of measurements can be added.

From Kubernetes docs:

Limits and requests for memory are measured in bytes. You can express memory as a plain integer or as a fixed-point integer using one of these suffixes: E, P, T, G, M, K. You can also use the power-of-two equivalents: Ei, Pi, Ti, Gi, Mi, Ki. For example, the following represent roughly the same value:

128974848, 129e6, 129M, 123Mi

sudermanjr commented 1 year ago

Thanks for the request. We would be happy to accept a community contribution for this change. I think being able to change the units would be super cool.

tbondarchuk commented 1 year ago

I've ended up using following to get recommendations in Mi:

kubectl get vpa -o json | jq -r '.items[] | .metadata.name, (.status.recommendation.containerRecommendations[] | ["", .containerName, .target.cpu, (.target.memory | tonumber / 1048576 | round | tostring) + "Mi"] | @tsv)'
thepaulmacca commented 1 year ago

I've ended up using following to get recommendations in Mi:

kubectl get vpa -o json | jq -r '.items[] | .metadata.name, (.status.recommendation.containerRecommendations[] | ["", .containerName, .target.cpu, (.target.memory | tonumber / 1048576 | round | tostring) + "Mi"] | @tsv)'

Thanks @tbondarchuk this is the route I've also gone down for now

jetersen commented 1 year ago

I have a working solution for the dashboard. The whole issue is that resource quantity uses DecimalSI which I am not quite sure where it is coming from 🤔

My only concern is at what level do we want this feature?

I have two approaches for formatting to Mebibytes.

One where 5G would be converted to 4769Mi or one where 5G gets converted to 5Gi but

This is the approach that simply deals with Mi units so 5G gets converted to 4769Mi

const (
    Mebibyte = 1024 * 1024
)

func formatToMebibyte(actual resource.Quantity) resource.Quantity {
    value := actual.Value() / Mebibyte
    if actual.Value()%Mebibyte != 0 {
        value++
    }
    return *resource.NewQuantity(value*Mebibyte, resource.BinarySI)
}

This is the generic approach that can be expanded to more units. Although the approach could have rules like anything below 2 Gibibyte is considered Mebibyte

const (
    Mebibyte = 1024 * 1024
    Gibibyte = Mebibyte * 1024
)

func formatToBinarySi(actual resource.Quantity) resource.Quantity {
    var unit int64
    if actual.Value() >= 2*Gibibyte {
        unit = Gibibyte
    } else {
        unit = Mebibyte
    }
    value := actual.Value() / unit
    if actual.Value()%unit != 0 {
        value++
    }
    return *resource.NewQuantity(value*unit, resource.BinarySI)
}

this code above could quickly expand to support Ti and Pi (although for pods that is debatable) but at the same time at what point do you start using Gibibyte?

At the same time I am not sure if the code is the best approach to get the value converted correctly to BinarySI. I tried to use RoundUp and ScaledValue but I never got the expected output in the ToString method 🤔

jetersen commented 1 year ago

Oh after some digging I found the culprit 👏 FormatResourceList which does not preserve the Mi scales when using RoundUp So VPA actually uses BinarySI so the reason we get DecimalSI is because of RoundUp 🤔

jetersen commented 1 year ago

Created #667