From a1f73cc247e89089eab49c5785c82f4e7ae462c2 Mon Sep 17 00:00:00 2001
From: Ravindra Thakur <ravindra.nath.thakur@est.tech>
Date: Tue, 14 Dec 2021 11:42:05 +0530
Subject: [PATCH] ReservedMemory Configuration for NUMA

Kubelet throws error when multiple numa nodes are specified for memory
reservation. This is due to "," being used as separator for different
memory types within same numa node as well as for different numa nodes.

This PR fixes the error by using ";" as the separator for
specifying multiple numa node configuration.

Signed-off-by: Ravindra Thakur <ravindra.nath.thakur@est.tech>
---
 pkg/util/flag/flags.go      | 69 ++++++++++++++++++-------------------
 pkg/util/flag/flags_test.go | 19 ++++++++++
 2 files changed, 53 insertions(+), 35 deletions(-)

diff --git a/pkg/util/flag/flags.go b/pkg/util/flag/flags.go
index 63c48e413d6..ff8e40db8e8 100644
--- a/pkg/util/flag/flags.go
+++ b/pkg/util/flag/flags.go
@@ -188,47 +188,46 @@ func (v *ReservedMemoryVar) Set(s string) error {
 		return nil
 	}
 
-	numaNodeReservation := strings.Split(s, ":")
-	if len(numaNodeReservation) != 2 {
-		return fmt.Errorf("the reserved memory has incorrect format, expected numaNodeID:type=quantity[,type=quantity...], got %s", s)
-	}
-
-	memoryTypeReservations := strings.Split(numaNodeReservation[1], ",")
-	if len(memoryTypeReservations) < 1 {
-		return fmt.Errorf("the reserved memory has incorrect format, expected numaNodeID:type=quantity[,type=quantity...], got %s", s)
-	}
-
-	numaNodeID, err := strconv.Atoi(numaNodeReservation[0])
-	if err != nil {
-		return fmt.Errorf("failed to convert the NUMA node ID, exptected integer, got %s", numaNodeReservation[0])
-	}
-
-	memoryReservation := kubeletconfig.MemoryReservation{
-		NumaNode: int32(numaNodeID),
-		Limits:   map[v1.ResourceName]resource.Quantity{},
-	}
-
-	for _, reservation := range memoryTypeReservations {
-		limit := strings.Split(reservation, "=")
-		if len(limit) != 2 {
-			return fmt.Errorf("the reserved limit has incorrect value, expected type=quantatity, got %s", reservation)
+	numaNodeReservations := strings.Split(s, ";")
+	for _, reservation := range numaNodeReservations {
+		numaNodeReservation := strings.Split(reservation, ":")
+		if len(numaNodeReservation) != 2 {
+			return fmt.Errorf("the reserved memory has incorrect format, expected numaNodeID:type=quantity[,type=quantity...], got %s", reservation)
 		}
-
-		resourceName := v1.ResourceName(limit[0])
-		if resourceName != v1.ResourceMemory && !corev1helper.IsHugePageResourceName(resourceName) {
-			return fmt.Errorf("memory type conversion error, unknown type: %q", resourceName)
+		memoryTypeReservations := strings.Split(numaNodeReservation[1], ",")
+		if len(memoryTypeReservations) < 1 {
+			return fmt.Errorf("the reserved memory has incorrect format, expected numaNodeID:type=quantity[,type=quantity...], got %s", reservation)
 		}
-
-		q, err := resource.ParseQuantity(limit[1])
+		numaNodeID, err := strconv.Atoi(numaNodeReservation[0])
 		if err != nil {
-			return fmt.Errorf("failed to parse the quantatity, expected quantatity, got %s", limit[1])
+			return fmt.Errorf("failed to convert the NUMA node ID, exptected integer, got %s", numaNodeReservation[0])
 		}
 
-		memoryReservation.Limits[v1.ResourceName(limit[0])] = q
+		memoryReservation := kubeletconfig.MemoryReservation{
+			NumaNode: int32(numaNodeID),
+			Limits:   map[v1.ResourceName]resource.Quantity{},
+		}
+
+		for _, memoryTypeReservation := range memoryTypeReservations {
+			limit := strings.Split(memoryTypeReservation, "=")
+			if len(limit) != 2 {
+				return fmt.Errorf("the reserved limit has incorrect value, expected type=quantatity, got %s", memoryTypeReservation)
+			}
+
+			resourceName := v1.ResourceName(limit[0])
+			if resourceName != v1.ResourceMemory && !corev1helper.IsHugePageResourceName(resourceName) {
+				return fmt.Errorf("memory type conversion error, unknown type: %q", resourceName)
+			}
+
+			q, err := resource.ParseQuantity(limit[1])
+			if err != nil {
+				return fmt.Errorf("failed to parse the quantatity, expected quantatity, got %s", limit[1])
+			}
+
+			memoryReservation.Limits[v1.ResourceName(limit[0])] = q
+		}
+		*v.Value = append(*v.Value, memoryReservation)
 	}
-
-	*v.Value = append(*v.Value, memoryReservation)
-
 	return nil
 }
 
diff --git a/pkg/util/flag/flags_test.go b/pkg/util/flag/flags_test.go
index 938715b3537..ba76f1f3ef2 100644
--- a/pkg/util/flag/flags_test.go
+++ b/pkg/util/flag/flags_test.go
@@ -224,6 +224,25 @@ func TestReservedMemoryVar(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc: "valid input with ';' as separator for multiple reserved-memory arguments",
+			argc: "blah --reserved-memory=0:memory=1Gi,hugepages-1Gi=1Gi;1:memory=1Gi",
+			expectVal: []kubeletconfig.MemoryReservation{
+				{
+					NumaNode: 0,
+					Limits: v1.ResourceList{
+						v1.ResourceMemory:        memory1Gi,
+						resourceNameHugepages1Gi: memory1Gi,
+					},
+				},
+				{
+					NumaNode: 1,
+					Limits: v1.ResourceList{
+						v1.ResourceMemory: memory1Gi,
+					},
+				},
+			},
+		},
 		{
 			desc:      "invalid input",
 			argc:      "blah --reserved-memory=bad-input",