WIP: fixed humanize memory bug

Signed-off-by: Omer Aplatony <[email protected]>

vertical-pod-autoscaler/docs/features.md

@@ -24,12 +24,12 @@ VPA can present memory recommendations in human-readable binary units (KiB, MiB,
 
 When enabled, memory values in recommendations will be:
 - Converted to the most appropriate binary unit (KiB, MiB, GiB, or TiB)
-- Displayed with up to 2 decimal places for precision
+- Rounded to the nearest whole number
 - Applied to target, lower bound, and upper bound recommendations
 
-For example, instead of seeing a memory recommendation of `262144000` bytes, you would see `250.00Mi`.
+For example, instead of seeing a memory recommendation of `262144000` bytes, you would see `250Mi`.
 
-Note: Due to the conversion to binary units and decimal place rounding, the humanized values may be slightly higher than the raw byte recommendations. For example, 1537 bytes would be shown as "1.50Ki" (1536 bytes). Consider this small difference when doing precise capacity planning.
+Note: Due to the conversion to binary units and rounding to the nearest whole number, the humanized values may differ slightly from the raw byte recommendations. For example, 1537 bytes would be shown as "2Ki" (2048 bytes). Consider this difference when doing precise capacity planning.
 
 To enable this feature, set the `--humanize-memory` flag to true when running the VPA recommender:
 ```bash

vertical-pod-autoscaler/docs/flags.md

@@ -67,7 +67,7 @@ This document is auto-generated from the flag definitions in the VPA recommender
 | `--external-metrics-memory-metric` |  |                  ALPHA.  Metric to use with external metrics provider for memory usage. |
 | `--history-length` | "8d" |                                  How much time back prometheus have to be queried to get historical metrics |
 | `--history-resolution` | "1h" |                              Resolution at which Prometheus is queried for historical metrics |
-| `--humanize-memory` |  |                                        Convert memory values in recommendations to the highest appropriate SI unit with up to 2 decimal places for better readability. |
+| `--humanize-memory` |  |                                        Convert memory values in recommendations to the highest appropriate binary unit (KiB, MiB, GiB, TiB), rounded to the nearest whole number for better readability. |
 | `--ignored-vpa-object-namespaces` |  |                   A comma-separated list of namespaces to ignore when searching for VPA objects. Leave empty to avoid ignoring any namespaces. These namespaces will not be cleaned by the garbage collector. |
 | `--kube-api-burst` | 10 |                                   QPS burst limit when making requests to Kubernetes apiserver |
 | `--kube-api-qps` | 5 |                                     QPS limit when making requests to Kubernetes apiserver |

vertical-pod-autoscaler/pkg/recommender/logic/recommender.go

@@ -37,7 +37,7 @@ var (
 	lowerBoundMemoryPercentile = flag.Float64("recommendation-lower-bound-memory-percentile", 0.5, `Memory usage percentile that will be used for the lower bound on memory recommendation.`)
 	upperBoundMemoryPercentile = flag.Float64("recommendation-upper-bound-memory-percentile", 0.95, `Memory usage percentile that will be used for the upper bound on memory recommendation.`)
 	confidenceIntervalMemory   = flag.Duration("confidence-interval-memory", time.Hour*24, "The time interval used for computing the confidence multiplier for the memory lower and upper bound. Default: 24h")
-	humanizeMemory             = flag.Bool("humanize-memory", false, "Convert memory values in recommendations to the highest appropriate SI unit with up to 2 decimal places for better readability.")
+	humanizeMemory             = flag.Bool("humanize-memory", false, "Convert memory values in recommendations to the highest appropriate binary unit (KiB, MiB, GiB, TiB), rounded to the nearest whole number for better readability.")
 	roundCPUMillicores         = flag.Int("round-cpu-millicores", 1, `CPU recommendation rounding factor in millicores. The CPU value will always be rounded up to the nearest multiple of this factor.`)
 )
 

vertical-pod-autoscaler/pkg/recommender/model/types.go

@@ -102,12 +102,13 @@ func ResourcesAsResourceList(resources Resources, humanizeMemory bool, roundCPUM
 			}
 		case ResourceMemory:
 			newKey = apiv1.ResourceMemory
-			quantity = QuantityFromMemoryAmount(resourceAmount)
-			if humanizeMemory && !quantity.IsZero() {
-				rawValues := quantity.Value()
-				humanizedValue := HumanizeMemoryQuantity(rawValues)
-				klog.V(4).InfoS("Converting raw value to humanized value", "rawValue", rawValues, "humanizedValue", humanizedValue)
+			if humanizeMemory && resourceAmount > 0 {
+				humanizedValue := HumanizeMemoryQuantity(resourceAmount)
+				klog.V(4).InfoS("Converting raw value to humanized value", "resourceAmount", resourceAmount, "humanizedValue", humanizedValue)
 				quantity = resource.MustParse(humanizedValue)
+				klog.V(4).InfoS("after quantity creation", "quantity", quantity.String())
+			} else {
+				quantity = QuantityFromMemoryAmount(resourceAmount)
 			}
 		default:
 			klog.ErrorS(nil, "Cannot translate resource name", "resourceName", key)
@@ -153,24 +154,24 @@ func resourceAmountFromFloat(amount float64) ResourceAmount {
 	}
 }
 
-// HumanizeMemoryQuantity converts raw bytes to human-readable string using binary units (KiB, MiB, GiB, TiB) with two decimal places.
-func HumanizeMemoryQuantity(bytes int64) string {
+// HumanizeMemoryQuantity converts bytes to a human-readable string using binary units (KiB, MiB, GiB, TiB), rounding to the nearest whole number.
+func HumanizeMemoryQuantity(bytes ResourceAmount) string {
 	const (
-		KiB = 1024
-		MiB = 1024 * KiB
-		GiB = 1024 * MiB
-		TiB = 1024 * GiB
+		Ki ResourceAmount = 1024
+		Mi                = 1024 * Ki
+		Gi                = 1024 * Mi
+		Ti                = 1024 * Gi
 	)
 
 	switch {
-	case bytes >= TiB:
-		return fmt.Sprintf("%.2fTi", float64(bytes)/float64(TiB))
-	case bytes >= GiB:
-		return fmt.Sprintf("%.2fGi", float64(bytes)/float64(GiB))
-	case bytes >= MiB:
-		return fmt.Sprintf("%.2fMi", float64(bytes)/float64(MiB))
-	case bytes >= KiB:
-		return fmt.Sprintf("%.2fKi", float64(bytes)/float64(KiB))
+	case bytes >= Ti:
+		return fmt.Sprintf("%.0fTi", float64(bytes)/float64(Ti))
+	case bytes >= Gi:
+		return fmt.Sprintf("%.0fGi", float64(bytes)/float64(Gi))
+	case bytes >= Mi:
+		return fmt.Sprintf("%.0fMi", float64(bytes)/float64(Mi))
+	case bytes >= Ki:
+		return fmt.Sprintf("%.0fKi", float64(bytes)/float64(Ki))
 	default:
 		return fmt.Sprintf("%d", bytes)
 	}

vertical-pod-autoscaler/pkg/recommender/model/types_test.go

@@ -57,20 +57,20 @@ func TestResourcesAsResourceList(t *testing.T) {
 			roundCPU: 1,
 			resourceList: apiv1.ResourceList{
 				apiv1.ResourceCPU:    *resource.NewMilliQuantity(1000, resource.DecimalSI),
-				apiv1.ResourceMemory: resource.MustParse("250.00Mi"),
+				apiv1.ResourceMemory: resource.MustParse("250Mi"),
 			},
 		},
 		{
 			name: "large memory value with humanize and cpu rounding to 3",
 			resources: Resources{
 				ResourceCPU:    1000,
-				ResourceMemory: 839500000, // 800.61Mi
+				ResourceMemory: 839500000, // 801Mi (rounded up from 800.61Mi)
 			},
 			humanize: true,
 			roundCPU: 3,
 			resourceList: apiv1.ResourceList{
 				apiv1.ResourceCPU:    *resource.NewMilliQuantity(1002, resource.DecimalSI),
-				apiv1.ResourceMemory: resource.MustParse("800.61Mi"),
+				apiv1.ResourceMemory: resource.MustParse("801Mi"),
 			},
 		},
 		{
@@ -115,85 +115,85 @@ func TestResourcesAsResourceList(t *testing.T) {
 
 type HumanizeMemoryQuantityTestCase struct {
 	name   string
-	value  int64
+	value  ResourceAmount
 	wanted string
 }
 
 func TestHumanizeMemoryQuantity(t *testing.T) {
 	testCases := []HumanizeMemoryQuantityTestCase{
 		{
-			name:   "1.00Ki",
-			value:  1024,
-			wanted: "1.00Ki",
+			name:   "1Ki",
+			value:  ResourceAmount(1024),
+			wanted: "1Ki",
 		},
 		{
-			name:   "1.00Mi",
-			value:  1024 * 1024,
-			wanted: "1.00Mi",
+			name:   "1Mi",
+			value:  ResourceAmount(1024 * 1024),
+			wanted: "1Mi",
 		},
 		{
-			name:   "1.00Gi",
-			value:  1024 * 1024 * 1024,
-			wanted: "1.00Gi",
+			name:   "1Gi",
+			value:  ResourceAmount(1024 * 1024 * 1024),
+			wanted: "1Gi",
 		},
 		{
-			name:   "1.00Ti",
-			value:  1024 * 1024 * 1024 * 1024,
-			wanted: "1.00Ti",
+			name:   "1Ti",
+			value:  ResourceAmount(1024 * 1024 * 1024 * 1024),
+			wanted: "1Ti",
 		},
 		{
-			name:   "256.00Mi",
-			value:  256 * 1024 * 1024,
-			wanted: "256.00Mi",
+			name:   "256Mi",
+			value:  ResourceAmount(256 * 1024 * 1024),
+			wanted: "256Mi",
 		},
 		{
-			name:   "1.50Gi",
-			value:  1.5 * 1024 * 1024 * 1024,
-			wanted: "1.50Gi",
+			name:   "2Gi",
+			value:  ResourceAmount(1.5 * 1024 * 1024 * 1024),
+			wanted: "2Gi",
 		},
 		{
 			name:   "1Mi in bytes",
-			value:  1050000,
-			wanted: "1.00Mi",
+			value:  ResourceAmount(1050000),
+			wanted: "1Mi",
 		},
 		{
-			name:   "1.5Ki in bytes",
-			value:  1537,
-			wanted: "1.50Ki",
+			name:   "2Ki in bytes",
+			value:  ResourceAmount(1537),
+			wanted: "2Ki",
 		},
 		{
-			name:   "4.65Gi",
-			value:  4992073454,
-			wanted: "4.65Gi",
+			name:   "5Gi",
+			value:  ResourceAmount(4992073454),
+			wanted: "5Gi",
 		},
 		{
-			name:   "6.05Gi",
-			value:  6499152537,
-			wanted: "6.05Gi",
+			name:   "6Gi",
+			value:  ResourceAmount(6499152537),
+			wanted: "6Gi",
 		},
 		{
-			name:   "15.23Gi",
-			value:  16357476492,
-			wanted: "15.23Gi",
+			name:   "15Gi",
+			value:  ResourceAmount(16357476492),
+			wanted: "15Gi",
 		},
 		{
-			name:   "3.75Gi",
-			value:  4022251530,
-			wanted: "3.75Gi",
+			name:   "4Gi",
+			value:  ResourceAmount(4022251530),
+			wanted: "4Gi",
 		},
 		{
-			name:   "12.65Gi",
-			value:  13580968030,
-			wanted: "12.65Gi",
+			name:   "13Gi",
+			value:  ResourceAmount(13580968030),
+			wanted: "13Gi",
 		},
 		{
-			name:   "14.46Gi",
-			value:  15530468536,
-			wanted: "14.46Gi",
+			name:   "14Gi",
+			value:  ResourceAmount(15530468536),
+			wanted: "14Gi",
 		},
 	}
 	for _, tc := range testCases {
-		t.Run(tc.name, func(*testing.T) {
+		t.Run(tc.name, func(t *testing.T) {
 			result := HumanizeMemoryQuantity(tc.value)
 			assert.Equal(t, tc.wanted, result)
 		})