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

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

Open
tsutsarin opened this issue Sep 26, 2022 · 6 comments
Open
Labels
enhancement Adding additional functionality or improvements pinned Prevents stalebot from removing priority: could Future work depending on bandwidth and availability

Comments

@tsutsarin
Copy link

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

@tsutsarin tsutsarin added the triage This bug needs triage label Sep 26, 2022
@sudermanjr sudermanjr added enhancement Adding additional functionality or improvements priority: could Future work depending on bandwidth and availability and removed triage This bug needs triage labels Nov 17, 2022
@sudermanjr
Copy link
Member

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.

@sudermanjr sudermanjr added the pinned Prevents stalebot from removing label Nov 17, 2022
@tbondarchuk
Copy link

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
Copy link

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
Copy link

jetersen commented Nov 3, 2023

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
Copy link

jetersen commented Nov 3, 2023

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 jetersen mentioned this issue Nov 3, 2023
2 tasks
@jetersen
Copy link

jetersen commented Nov 3, 2023

Created #667

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding additional functionality or improvements pinned Prevents stalebot from removing priority: could Future work depending on bandwidth and availability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants