resource.MustParse(str string) returns an empty string for multiples of 8 for Gi

[besir-reddit]

Hey apimachinery team 👋

While using the resource.MustParse(str string) function, I came across a bug where multiples of 8 for quantity values return an empty string. I believe this is a bug, and is not intentional.

For example:

• resource.MustParse("8Gi") should return 8Gi, but returns an empty string.
• resource.MustParse("40Gi") should return 40Gi, but returns an empty string.
• resource.MustParse("192Ti") should return 192Ti, but returns an empty string.

Here's a Go Playground example: https://go.dev/play/p/sEkUXVjcwys

This is the code to reproduce the error:

package main

import (
	"fmt"

	"k8s.io/apimachinery/pkg/api/resource"
)

func main() {
	// Mixed example demonstrating for "Gi"
	fmt.Println(resource.MustParse("2Gi"))
	fmt.Println(resource.MustParse("8Gi"))
	fmt.Println(resource.MustParse("12Gi"))
	fmt.Println(resource.MustParse("40Gi"))
	fmt.Println(resource.MustParse("41Gi"))
	fmt.Println(resource.MustParse("50Gi"))
	fmt.Println(resource.MustParse("51Gi"))
	fmt.Println(resource.MustParse("100Gi"))
	fmt.Println(resource.MustParse("101Gi"))

	// Working example for "Ti"
	fmt.Println(resource.MustParse("2Ti"))

	// Buggy examples for multiple of 8 for "Ti"
	fmt.Println(resource.MustParse("8Ti"))
	fmt.Println(resource.MustParse("16Ti"))
	fmt.Println(resource.MustParse("32Ti"))
	fmt.Println(resource.MustParse("64Ti"))
	fmt.Println(resource.MustParse("128Ti"))
	fmt.Println(resource.MustParse("192Ti"))
}

Should return:

{{2147483648 0} {<nil>} 2Gi BinarySI}
{{8589934592 0} {<nil>}  BinarySI}          <- Empty String (Bug)
{{12884901888 0} {<nil>} 12Gi BinarySI}
{{42949672960 0} {<nil>}  BinarySI}         <- Empty String (Bug)
{{44023414784 0} {<nil>} 41Gi BinarySI}
{{53687091200 0} {<nil>} 50Gi BinarySI}
{{54760833024 0} {<nil>} 51Gi BinarySI}
{{107374182400 0} {<nil>} 100Gi BinarySI}
{{108447924224 0} {<nil>} 101Gi BinarySI}
{{2199023255552 0} {<nil>} 2Ti BinarySI}
{{8796093022208 0} {<nil>}  BinarySI}       <- Empty String (Bug)
{{17592186044416 0} {<nil>}  BinarySI}      <- Empty String (Bug)
{{35184372088832 0} {<nil>}  BinarySI}      <- Empty String (Bug)
{{70368744177664 0} {<nil>}  BinarySI}      <- Empty String (Bug)
{{0 0} {0xc000016660}  BinarySI}            <- Empty String (Bug)
{{0 0} {0xc000016780}  BinarySI}            <- Empty String (Bug)

Program exited.

As it can be seen above, all the values that are multiples of 8 return an empty string.

This bug is most likely originating from resource.ParseQuantity(str string) in:

apimachinery/pkg/api/resource/quantity.go

Lines 331 to 336 in ee6d966

	// if the number is in canonical form, reuse the string
	switch format {
	case BinarySI:
	if exponent%10 == 0 && (value&0x07 != 0) {
	return Quantity{i: int64Amount{value: result, scale: Scale(scale)}, Format: format, s: str}, nil
	}

In the condition above:
The exponent is for the binary unit prefix representation for the unit in The string. (e.g., 30 is used here, because 2^30=1 Gi).
the value is the numerical part of our input (e.g., 40 in 40Gi).
The code checks if the exponent is a multiple of 10 and whether the value isn't a multiple of 8.
Values like 8, 16, 24, 32, 40, etc. (aka. multiples of 8), will fail the condition and will not preserve the original string.

I'm not exactly sure why there's a check for values that aren't a multiple of 8 in the condition.

Extra thanks to @brian-g-w for digging into this bug with me.

Edit: Looks like this package was originally moved from the kubernetes/kubernetes repository. I found the oldest referral to this file in that repo under the v1.5.8 tag:

https://github.com/kubernetes/kubernetes/blob/v1.5.8/pkg/api/resource/quantity.go#L329-L334

Edit2: Found the original commit that added this condition: kubernetes/kubernetes@b131021