kubernetes: resource.Quantity: problematic MarshalJSON with scale <= -10

What happened:

json.Marshal(resource.NewScaledQuantity(1, -9)) // "1n", ok
json.Marshal(resource.NewScaledQuantity(1, -10)) // "100" what!!
json.Marshal(resource.NewScaledQuantity(123, -10)) // "12300" what!!

What you expected to happen:

json.Marshal(resource.NewScaledQuantity(1, -10)) // "0"
json.Marshal(resource.NewScaledQuantity(123, -10)) // "12n"

How to reproduce it (as minimally and precisely as possible): Just run the code above.

Anything else we need to know?: This bug is caused by quantitySuffixer.constructBytes failing to work with an exponent of -12. It wants to return 100pico but there is no such suffix, so it returned 100. I think loss of precision (returning 12n) would be much better (less of a surprise) than not having a suffix at all (returning 12300).

Environment:

Git Commit: 5442980dd98f09566c584482d38e595adfc278bd (2019-02-13)

/kind bug /sig api-machinery

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 1
Comments: 17 (11 by maintainers)

Commits related to this issue

[admission][ASE] ASE Admission Controller [admission][ASE] fix logTailContainer spec problems [admission][ASE] ASE AdmissionController - 添加对Node的validation fix ASE Admission Controller [admission]... — committed to mars1024/kubernetes by deleted user 5 years ago
[admission][ASE] ASE Admission Controller [admission][ASE] fix logTailContainer spec problems [admission][ASE] ASE AdmissionController - 添加对Node的validation fix ASE Admission Controller [admission]... — committed to mars1024/kubernetes by deleted user 5 years ago

Most upvoted comments

@lavalamp @yliaog

It’s not really about the pico unit, I am just pointing out that there is a potential bug, and I believe the solution is not to support pico but to round up to nano as did in #74006 .

My use case: I have a controller that divides a Quantity by a certain number. 100m divided by 2 can produce the correct 50m, but when divided by 3, it would produce 3333333300(pico).

Related code:

newValue := resource.NewScaledQuantity(int64(float64(oldValue.ScaledValue(-10))/ratio), -10)

The code is perhaps a little problematic (changing -10 to -3 will make it work), but getting a Quantity 10^12 the size it should be sometimes is really a surprise.

KevinWang15 on Feb 15, 2019

We do not support pico units. NewScaledQuantity should have rejected the parameters you passed.

wrt @lavalamp 's comment, that weird output you got from serializing quantity is right suggesting that you are losing precision. or rather, an explicit rounding up/down should be done before calling NewScaledQuantity. +1 that we should reject it on on instantiation.

https://github.com/kubernetes/kubernetes/blob/a887ae8344adf9739c1d156a718c05fdf95b73ab/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go#L59-L62

yue9944882 on Feb 19, 2019