[Quantity] Fix scaledValue to round negative Quantity values away from zero

Project: kubernetes/kubernetes | Language: Go | Concepts: error-handling, testing-strategy, algorithmic-optimization | Difficulty: mid
Link: https://github.com/kubernetes/kubernetes/pull/138510

The problem

The resource.Quantity.Value() and MilliValue() methods in Kubernetes are documented to round values to the nearest integer away from zero. However, for small negative values, the implementation rounded toward positive infinity (i.e., toward zero), causing incorrect results such as returning +1 instead of -1 for negative inputs like ParseQuantity("-.484785E-7466").

Why it's non-trivial

A naive fix might only adjust the rounding logic for negative numbers, but would miss:

• The difference in rounding semantics between Go's integer division (which truncates toward zero) and the documented behavior (which should round away from zero).
• The existence of two code paths: a fast path for values fitting in int64, and a big.Int path for larger values or exponents, each with different rounding behaviors.
• The need to handle edge cases where values are at or beyond the limits of int64, which could otherwise cause out-of-range errors or incorrect results.

The approach

1. Update the int64 fast path to round away from zero for negatives

Previously, the code only checked if the remainder (mod) was non-zero and always added 1, which was incorrect for negatives:

-		if mod != 0 {
-			return result + 1
-		}

Now, it checks the sign of mod:

if mod > 0 {
    return result + 1
}
if mod < 0 {
    return result - 1
}
return result

Annotation:
This ensures that for positive numbers with a remainder, the result is nudged up, and for negative numbers with a negative remainder, the result is nudged down—both moving away from zero as required.

2. Correct the fast path guard to avoid out-of-range errors

Previously, the fast path was taken for any value less than maxInt64, including huge negatives:

-	if unscaled.Cmp(maxInt64) < 0 && dif < log10MaxInt64 {
+	if unscaled.Cmp(maxInt64) < 0 && unscaled.Cmp(minInt64) > 0 && dif < log10MaxInt64 {

Annotation:
By also checking unscaled.Cmp(minInt64) > 0, the code avoids calling unscaled.Int64() on values outside the int64 range, which would be undefined.

3. Fix rounding in the big.Int path

Previously, the code always incremented the result if there was a remainder, regardless of sign:

-	if remainder.Sign() != 0 {
-		return result.Int64() + 1
-	}

Now, it only increments for positive dividends:

if remainder.Sign() != 0 && unscaled.Sign() > 0 {
    return result.Int64() + 1
}

Annotation:
This leverages the fact that big.Int.DivMod already floors the quotient for negative dividends, which matches the away-from-zero behavior.

4. Update and expand tests

• Added and expanded test cases to cover negative values, both in the int64 and big.Int paths, ensuring correct rounding away from zero.
• Updated baseline tests to assert the new, correct behavior.

Example test addition:

func TestNegativeValueRoundsAwayFromZero(t *testing.T) {
    // ... test table includes negative and positive values, with expected Value and MilliValue
}

From the review thread

thc1006:
I came at this from the other side: I was adding a MinMilliQuantity to apimachinery and could not make its bound hold, because MilliValue does not round negatives the way it documents. ...

• The guard admitted every negative, not just math.MinInt64. unscaled.Cmp(maxInt64) < 0 is signed, so it is true for any negative big.Int however large, and the fast path then called unscaled.Int64(), which Go leaves undefined out of range.
• The sign flip reaches ordinary input. MustParse("-9.5Gi").Value() returns +8246196746 on master, positive, for a negative quantity. ...
• The representations agree now. -1.0001 and -0.1m gave a different MilliValue depending on whether the quantity was int64-backed or Dec-backed (-1001 vs -999, and -1 vs +1); after this, both match. ...
  One small thing, take it or leave it: unscaled.Cmp(maxInt64) < 0 && unscaled.Cmp(minInt64) > 0 excludes both endpoints, so MinInt64 and MaxInt64 fall through to the big.Int path even though Int64() represents them exactly. unscaled.IsInt64() is the same guard, says the intent directly, and keeps the fast path for those two. Correct either way.

Takeaways

• Always match documented rounding semantics, especially with signed numbers:
  Division and modulo operations can behave differently for negatives; test both signs to ensure correctness.

• Guard fast paths with precise range checks:
  When converting between numeric types, ensure that values are within the representable range to avoid undefined behavior.

• Comprehensive tests should cover both positive and negative edge cases:
  Especially for core math logic, tests must include negative values, zero, and large/small magnitudes to catch subtle bugs.