memory manager: improve the reserved memory validation logic

We will have two layers of the validation.

- the first part of the validation logic will be implemented under the
`ValidateKubeletConfiguration` method
- the second one that requires knowledge about machine topology and
node allocatable resources will be implemented under the memory manager.

Signed-off-by: Artyom Lukianov <alukiano@redhat.com>

pkg/kubelet/apis/config/validation/BUILD

@@ -11,14 +11,17 @@ go_library(
     srcs = [
         "validation.go",
         "validation_others.go",
+        "validation_reserved_memory.go",
         "validation_windows.go",
     ],
     importpath = "k8s.io/kubernetes/pkg/kubelet/apis/config/validation",
     deps = [
+        "//pkg/apis/core/v1/helper:go_default_library",
         "//pkg/features:go_default_library",
         "//pkg/kubelet/apis/config:go_default_library",
         "//pkg/kubelet/cm/cpuset:go_default_library",
         "//pkg/kubelet/types:go_default_library",
+        "//staging/src/k8s.io/api/core/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/errors:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/validation:go_default_library",
@@ -43,10 +46,15 @@ filegroup(
 
 go_test(
     name = "go_default_test",
-    srcs = ["validation_test.go"],
+    srcs = [
+        "validation_reserved_memory_test.go",
+        "validation_test.go",
+    ],
     embed = [":go_default_library"],
     deps = [
         "//pkg/kubelet/apis/config:go_default_library",
+        "//staging/src/k8s.io/api/core/v1:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/errors:go_default_library",
     ],

pkg/kubelet/apis/config/validation/validation.go

@@ -193,6 +193,8 @@ func ValidateKubeletConfiguration(kc *kubeletconfig.KubeletConfiguration) error
 		}
 	}
 
+	allErrors = append(allErrors, validateReservedMemoryConfiguration(kc)...)
+
 	if err := validateKubeletOSConfiguration(kc); err != nil {
 		allErrors = append(allErrors, err)
 	}

pkg/kubelet/apis/config/validation/validation_reserved_memory.go

@@ -0,0 +1,64 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package validation
+
+import (
+	"fmt"
+
+	v1 "k8s.io/api/core/v1"
+	corev1helper "k8s.io/kubernetes/pkg/apis/core/v1/helper"
+	kubeletconfig "k8s.io/kubernetes/pkg/kubelet/apis/config"
+)
+
+// validateReservedMemory validates the reserved memory configuration and returns an error if it is invalid.
+func validateReservedMemoryConfiguration(kc *kubeletconfig.KubeletConfiguration) []error {
+	if len(kc.ReservedMemory) == 0 {
+		return nil
+	}
+
+	var errors []error
+
+	numaTypeDuplicates := map[int32]map[v1.ResourceName]bool{}
+	for _, reservedMemory := range kc.ReservedMemory {
+		numaNode := reservedMemory.NumaNode
+		if _, ok := numaTypeDuplicates[numaNode]; !ok {
+			numaTypeDuplicates[numaNode] = map[v1.ResourceName]bool{}
+		}
+
+		for resourceName, q := range reservedMemory.Limits {
+			if !reservedMemorySupportedLimit(resourceName) {
+				errors = append(errors, fmt.Errorf("the limit type %q for NUMA node %d is not supported, only %v is accepted", resourceName, numaNode, []v1.ResourceName{v1.ResourceMemory, v1.ResourceHugePagesPrefix + "<HugePageSize>"}))
+			}
+
+			// validates that the limit has non-zero value
+			if q.IsZero() {
+				errors = append(errors, fmt.Errorf("reserved memory may not be zero for NUMA node %d and resource %q", numaNode, resourceName))
+			}
+
+			// validates that no duplication for NUMA node and limit type occurred
+			if _, ok := numaTypeDuplicates[numaNode][resourceName]; ok {
+				errors = append(errors, fmt.Errorf("the reserved memory has a duplicate value for NUMA node %d and resource %q", numaNode, resourceName))
+			}
+			numaTypeDuplicates[numaNode][resourceName] = true
+		}
+	}
+	return errors
+}
+
+func reservedMemorySupportedLimit(resourceName v1.ResourceName) bool {
+	return corev1helper.IsHugePageResourceName(resourceName) || resourceName == v1.ResourceMemory
+}

pkg/kubelet/apis/config/validation/validation_reserved_memory_test.go

@@ -0,0 +1,120 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package validation
+
+import (
+	"fmt"
+	"testing"
+
+	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
+	kubeletconfig "k8s.io/kubernetes/pkg/kubelet/apis/config"
+)
+
+func TestValidateReservedMemoryConfiguration(t *testing.T) {
+	testCases := []struct {
+		description          string
+		kubeletConfiguration *kubeletconfig.KubeletConfiguration
+		expectedError        error
+	}{
+		{
+			description:          "The kubelet configuration does not have reserved memory parameter",
+			kubeletConfiguration: &kubeletconfig.KubeletConfiguration{},
+			expectedError:        nil,
+		},
+		{
+			description: "The kubelet configuration has valid reserved memory parameter",
+			kubeletConfiguration: &kubeletconfig.KubeletConfiguration{
+				ReservedMemory: []kubeletconfig.MemoryReservation{
+					{
+						NumaNode: 0,
+						Limits: v1.ResourceList{
+							v1.ResourceMemory: *resource.NewQuantity(128, resource.DecimalSI),
+						},
+					},
+				},
+			},
+			expectedError: nil,
+		},
+		{
+			description: "The reserved memory has duplications for the NUMA node and limit type",
+			kubeletConfiguration: &kubeletconfig.KubeletConfiguration{
+				ReservedMemory: []kubeletconfig.MemoryReservation{
+					{
+						NumaNode: 0,
+						Limits: v1.ResourceList{
+							v1.ResourceMemory: *resource.NewQuantity(128, resource.DecimalSI),
+						},
+					},
+					{
+						NumaNode: 0,
+						Limits: v1.ResourceList{
+							v1.ResourceMemory: *resource.NewQuantity(64, resource.DecimalSI),
+						},
+					},
+				},
+			},
+			expectedError: fmt.Errorf("the reserved memory has a duplicate value for NUMA node %d and resource %q", 0, v1.ResourceMemory),
+		},
+		{
+			description: "The reserved memory has unsupported limit type",
+			kubeletConfiguration: &kubeletconfig.KubeletConfiguration{
+				ReservedMemory: []kubeletconfig.MemoryReservation{
+					{
+						NumaNode: 0,
+						Limits: v1.ResourceList{
+							"blabla": *resource.NewQuantity(128, resource.DecimalSI),
+						},
+					},
+				},
+			},
+			expectedError: fmt.Errorf("the limit type %q for NUMA node %d is not supported, only [memory hugepages-<HugePageSize>] is accepted", "blabla", 0),
+		},
+		{
+			description: "The reserved memory has limit type with zero value",
+			kubeletConfiguration: &kubeletconfig.KubeletConfiguration{
+				ReservedMemory: []kubeletconfig.MemoryReservation{
+					{
+						NumaNode: 0,
+						Limits: v1.ResourceList{
+							v1.ResourceMemory: *resource.NewQuantity(0, resource.DecimalSI),
+						},
+					},
+				},
+			},
+			expectedError: fmt.Errorf("reserved memory may not be zero for NUMA node %d and resource %q", 0, v1.ResourceMemory),
+		},
+	}
+
+	for _, testCase := range testCases {
+		errors := validateReservedMemoryConfiguration(testCase.kubeletConfiguration)
+
+		if len(errors) != 0 && testCase.expectedError == nil {
+			t.Errorf("expected errors %v, got %v", errors, testCase.expectedError)
+		}
+
+		if testCase.expectedError != nil {
+			if len(errors) == 0 {
+				t.Errorf("expected error %v, got %v", testCase.expectedError, errors)
+			}
+
+			if errors[0].Error() != testCase.expectedError.Error() {
+				t.Errorf("expected error %v, got %v", testCase.expectedError, errors[0])
+			}
+		}
+	}
+}

pkg/kubelet/cm/memorymanager/memory_manager.go

@@ -321,7 +321,7 @@ func (m *manager) policyRemoveContainerByRef(podUID string, containerName string
 	return err
 }
 
-func getTotalMemoryTypeReserved(machineInfo *cadvisorapi.MachineInfo, reservedMemory []kubeletconfig.MemoryReservation) map[v1.ResourceName]resource.Quantity {
+func getTotalMemoryTypeReserved(machineInfo *cadvisorapi.MachineInfo, reservedMemory []kubeletconfig.MemoryReservation) (map[v1.ResourceName]resource.Quantity, error) {
 	totalMemoryType := map[v1.ResourceName]resource.Quantity{}
 
 	numaNodes := map[int]bool{}
@@ -331,8 +331,7 @@ func getTotalMemoryTypeReserved(machineInfo *cadvisorapi.MachineInfo, reservedMe
 
 	for _, reservation := range reservedMemory {
 		if !numaNodes[int(reservation.NumaNode)] {
-			klog.Warningf("The NUMA node %d specified under --reserved-memory does not exist on the machine", reservation.NumaNode)
-			continue
+			return nil, fmt.Errorf("the reserved memory configuration references a NUMA node %d that does not exist on this machine", reservation.NumaNode)
 		}
 
 		for resourceName, q := range reservation.Limits {
@@ -343,19 +342,20 @@ func getTotalMemoryTypeReserved(machineInfo *cadvisorapi.MachineInfo, reservedMe
 		}
 	}
 
-	return totalMemoryType
+	return totalMemoryType, nil
 }
 
 func validateReservedMemory(machineInfo *cadvisorapi.MachineInfo, nodeAllocatableReservation v1.ResourceList, reservedMemory []kubeletconfig.MemoryReservation) error {
-	totalMemoryType := getTotalMemoryTypeReserved(machineInfo, reservedMemory)
+	totalMemoryType, err := getTotalMemoryTypeReserved(machineInfo, reservedMemory)
+	if err != nil {
+		return err
+	}
 
 	commonMemoryTypeSet := make(map[v1.ResourceName]bool)
 	for resourceType := range totalMemoryType {
-		if !(corev1helper.IsHugePageResourceName(resourceType) || resourceType == v1.ResourceMemory) {
-			continue
-		}
 		commonMemoryTypeSet[resourceType] = true
 	}
+
 	for resourceType := range nodeAllocatableReservation {
 		if !(corev1helper.IsHugePageResourceName(resourceType) || resourceType == v1.ResourceMemory) {
 			continue
@@ -375,7 +375,7 @@ func validateReservedMemory(machineInfo *cadvisorapi.MachineInfo, nodeAllocatabl
 		}
 
 		if !(*nodeAllocatableMemory).Equal(*reservedMemory) {
-			return fmt.Errorf("the total amount of memory of type \"%s\" is not equal to the value determined by Node Allocatable feature", resourceType)
+			return fmt.Errorf("the total amount %q of type %q is not equal to the value %q determined by Node Allocatable feature", reservedMemory.String(), resourceType, nodeAllocatableMemory.String())
 		}
 	}
 

pkg/kubelet/cm/memorymanager/memory_manager_test.go

@@ -152,7 +152,7 @@ func TestValidateReservedMemory(t *testing.T) {
 			{Id: 1},
 		},
 	}
-	const msgNotEqual = "the total amount of memory of type \"%s\" is not equal to the value determined by Node Allocatable feature"
+	const msgNotEqual = "the total amount %q of type %q is not equal to the value %q determined by Node Allocatable feature"
 	testCases := []struct {
 		description                string
 		nodeAllocatableReservation v1.ResourceList
@@ -193,14 +193,14 @@ func TestValidateReservedMemory(t *testing.T) {
 					},
 				},
 			},
-			fmt.Sprintf(msgNotEqual, v1.ResourceMemory),
+			fmt.Sprintf(msgNotEqual, "12", v1.ResourceMemory, "0"),
 		},
 		{
 			"Node Allocatable set, reserved not set",
 			v1.ResourceList{hugepages2M: *resource.NewQuantity(5, resource.DecimalSI)},
 			machineInfo,
 			[]kubeletconfig.MemoryReservation{},
-			fmt.Sprintf(msgNotEqual, hugepages2M),
+			fmt.Sprintf(msgNotEqual, "0", hugepages2M, "5"),
 		},
 		{
 			"Reserved not equal to Node Allocatable",
@@ -214,7 +214,7 @@ func TestValidateReservedMemory(t *testing.T) {
 					},
 				},
 			},
-			fmt.Sprintf(msgNotEqual, v1.ResourceMemory),
+			fmt.Sprintf(msgNotEqual, "12", v1.ResourceMemory, "5"),
 		},
 		{
 			"Reserved contains the NUMA node that does not exist under the machine",
@@ -234,7 +234,7 @@ func TestValidateReservedMemory(t *testing.T) {
 					},
 				},
 			},
-			fmt.Sprintf(msgNotEqual, v1.ResourceMemory),
+			"the reserved memory configuration references a NUMA node 2 that does not exist on this machine",
 		},
 		{
 			"Reserved total equal to Node Allocatable",
@@ -285,7 +285,7 @@ func TestValidateReservedMemory(t *testing.T) {
 				},
 			},
 
-			fmt.Sprintf(msgNotEqual, hugepages2M),
+			fmt.Sprintf(msgNotEqual, "77", hugepages2M, "14"),
 		},
 	}
 
@@ -404,7 +404,7 @@ func TestGetSystemReservedMemory(t *testing.T) {
 			machineInfo:   machineInfo,
 		},
 		{
-			description:                "Should return error when Allocatable reservation is not equal pre reserved memory",
+			description:                "Should return error when Allocatable reservation is not equal to the reserved memory",
 			nodeAllocatableReservation: v1.ResourceList{},
 			systemReservedMemory: []kubeletconfig.MemoryReservation{
 				{
@@ -415,7 +415,7 @@ func TestGetSystemReservedMemory(t *testing.T) {
 				},
 			},
 			expectedReserved: nil,
-			expectedError:    fmt.Errorf("the total amount of memory of type \"memory\" is not equal to the value determined by Node Allocatable feature"),
+			expectedError:    fmt.Errorf("the total amount \"1Gi\" of type \"memory\" is not equal to the value \"0\" determined by Node Allocatable feature"),
 			machineInfo:      machineInfo,
 		},
 		{
@@ -2171,7 +2171,7 @@ func TestNewManager(t *testing.T) {
 				},
 			},
 			affinity:         topologymanager.NewFakeManager(),
-			expectedError:    fmt.Errorf("the total amount of memory of type %q is not equal to the value determined by Node Allocatable feature", v1.ResourceMemory),
+			expectedError:    fmt.Errorf("the total amount \"3Gi\" of type %q is not equal to the value \"2Gi\" determined by Node Allocatable feature", v1.ResourceMemory),
 			expectedReserved: expectedReserved,
 		},
 		{