From 102124464a994946e151c976775cf751423b14f7 Mon Sep 17 00:00:00 2001 From: Artyom Lukianov Date: Mon, 14 Dec 2020 20:28:11 +0200 Subject: [PATCH] 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 --- pkg/kubelet/apis/config/validation/BUILD | 10 +- .../apis/config/validation/validation.go | 2 + .../validation/validation_reserved_memory.go | 64 ++++++++++ .../validation_reserved_memory_test.go | 120 ++++++++++++++++++ .../cm/memorymanager/memory_manager.go | 18 +-- .../cm/memorymanager/memory_manager_test.go | 18 +-- 6 files changed, 213 insertions(+), 19 deletions(-) create mode 100644 pkg/kubelet/apis/config/validation/validation_reserved_memory.go create mode 100644 pkg/kubelet/apis/config/validation/validation_reserved_memory_test.go diff --git a/pkg/kubelet/apis/config/validation/BUILD b/pkg/kubelet/apis/config/validation/BUILD index 25ad727a0a0..55d2875d903 100644 --- a/pkg/kubelet/apis/config/validation/BUILD +++ b/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", ], diff --git a/pkg/kubelet/apis/config/validation/validation.go b/pkg/kubelet/apis/config/validation/validation.go index 56069cae10b..69b08a8229d 100644 --- a/pkg/kubelet/apis/config/validation/validation.go +++ b/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) } diff --git a/pkg/kubelet/apis/config/validation/validation_reserved_memory.go b/pkg/kubelet/apis/config/validation/validation_reserved_memory.go new file mode 100644 index 00000000000..ba4d9a289ff --- /dev/null +++ b/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 + ""})) + } + + // 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 +} diff --git a/pkg/kubelet/apis/config/validation/validation_reserved_memory_test.go b/pkg/kubelet/apis/config/validation/validation_reserved_memory_test.go new file mode 100644 index 00000000000..1910ffaa1eb --- /dev/null +++ b/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-] 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]) + } + } + } +} diff --git a/pkg/kubelet/cm/memorymanager/memory_manager.go b/pkg/kubelet/cm/memorymanager/memory_manager.go index 7840d6d0b25..1b00008bfa4 100644 --- a/pkg/kubelet/cm/memorymanager/memory_manager.go +++ b/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()) } } diff --git a/pkg/kubelet/cm/memorymanager/memory_manager_test.go b/pkg/kubelet/cm/memorymanager/memory_manager_test.go index e1d0f20d86d..ff2b3f811c9 100644 --- a/pkg/kubelet/cm/memorymanager/memory_manager_test.go +++ b/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, }, {