From 6dcec345dfb43bff9e978cb9e7a992e1e34c9507 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Wed, 5 May 2021 16:50:28 +0200 Subject: [PATCH] smtalign: cm: factor out admission response Introduce a new `admission` subpackage to factor out the responsability to create `PodAdmitResult` objects. This enables resource manager to report specific errors in Allocate() and to bubble up them in the relevant fields of the `PodAdmitResult`. To demonstrate the approach we refactor TopologyAffinityError as a proper error. Co-authored-by: Kevin Klues Co-authored-by: Swati Sehgal Signed-off-by: Francesco Romani --- pkg/kubelet/cm/admission/errors.go | 62 +++++++++++++ pkg/kubelet/cm/admission/errors_test.go | 87 +++++++++++++++++++ pkg/kubelet/cm/container_manager_linux.go | 21 ++--- pkg/kubelet/cm/container_manager_windows.go | 5 +- .../topologymanager/fake_topology_manager.go | 5 +- pkg/kubelet/cm/topologymanager/scope.go | 27 +----- .../cm/topologymanager/scope_container.go | 7 +- pkg/kubelet/cm/topologymanager/scope_pod.go | 7 +- .../cm/topologymanager/topology_manager.go | 13 +++ .../topologymanager/topology_manager_test.go | 6 ++ 10 files changed, 188 insertions(+), 52 deletions(-) create mode 100644 pkg/kubelet/cm/admission/errors.go create mode 100644 pkg/kubelet/cm/admission/errors_test.go diff --git a/pkg/kubelet/cm/admission/errors.go b/pkg/kubelet/cm/admission/errors.go new file mode 100644 index 00000000000..5e549220394 --- /dev/null +++ b/pkg/kubelet/cm/admission/errors.go @@ -0,0 +1,62 @@ +/* +Copyright 2021 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 admission + +import ( + "errors" + "fmt" + + "k8s.io/kubernetes/pkg/kubelet/lifecycle" +) + +const ( + ErrorReasonUnexpected = "UnexpectedAdmissionError" +) + +type Error interface { + Error() string + Type() string +} + +type unexpectedAdmissionError struct{ Err error } + +var _ Error = (*unexpectedAdmissionError)(nil) + +func (e *unexpectedAdmissionError) Error() string { + return fmt.Sprintf("Allocate failed due to %v, which is unexpected", e.Err) +} + +func (e *unexpectedAdmissionError) Type() string { + return ErrorReasonUnexpected +} + +func GetPodAdmitResult(err error) lifecycle.PodAdmitResult { + if err == nil { + return lifecycle.PodAdmitResult{Admit: true} + } + + var admissionErr Error + if !errors.As(err, &admissionErr) { + admissionErr = &unexpectedAdmissionError{err} + } + + return lifecycle.PodAdmitResult{ + Message: admissionErr.Error(), + Reason: admissionErr.Type(), + Admit: false, + } +} diff --git a/pkg/kubelet/cm/admission/errors_test.go b/pkg/kubelet/cm/admission/errors_test.go new file mode 100644 index 00000000000..d18eb4a8ed4 --- /dev/null +++ b/pkg/kubelet/cm/admission/errors_test.go @@ -0,0 +1,87 @@ +/* +Copyright 2021 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 admission + +import ( + "errors" + "testing" +) + +type TestAdmissionError struct { + message string + reason string +} + +func (e *TestAdmissionError) Error() string { + return e.message +} + +func (e *TestAdmissionError) Type() string { + return e.reason +} + +func TestAdmissionErrors(t *testing.T) { + testCases := []struct { + Error error + expectedAdmissionError bool + }{ + { + nil, + false, + }, + { + errors.New("Not an AdmissionError error"), + false, + }, + { + &TestAdmissionError{ + "Is an AdmissionError error", + "TestAdmissionError", + }, + true, + }, + } + + for _, tc := range testCases { + h := GetPodAdmitResult(tc.Error) + if tc.Error == nil { + if !h.Admit { + t.Errorf("expected PodAdmitResult.Admit = true") + } + continue + } + + if h.Admit { + t.Errorf("expected PodAdmitResult.Admit = false") + } + + if tc.expectedAdmissionError { + err, ok := tc.Error.(*TestAdmissionError) + if !ok { + t.Errorf("expected TestAdmissionError") + } + if h.Reason != err.reason { + t.Errorf("expected PodAdmitResult.Reason = %v, got %v", err.reason, h.Reason) + } + continue + } + + if h.Reason != ErrorReasonUnexpected { + t.Errorf("expected PodAdmitResult.Reason = %v, got %v", ErrorReasonUnexpected, h.Reason) + } + } +} diff --git a/pkg/kubelet/cm/container_manager_linux.go b/pkg/kubelet/cm/container_manager_linux.go index 14b62159acd..31a728b7291 100644 --- a/pkg/kubelet/cm/container_manager_linux.go +++ b/pkg/kubelet/cm/container_manager_linux.go @@ -52,6 +52,7 @@ import ( podresourcesapi "k8s.io/kubelet/pkg/apis/podresources/v1" kubefeatures "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/kubelet/cadvisor" + "k8s.io/kubernetes/pkg/kubelet/cm/admission" "k8s.io/kubernetes/pkg/kubelet/cm/containermap" "k8s.io/kubernetes/pkg/kubelet/cm/cpumanager" "k8s.io/kubernetes/pkg/kubelet/cm/devicemanager" @@ -758,37 +759,25 @@ func (m *resourceAllocator) Admit(attrs *lifecycle.PodAdmitAttributes) lifecycle for _, container := range append(pod.Spec.InitContainers, pod.Spec.Containers...) { err := m.deviceManager.Allocate(pod, &container) if err != nil { - return lifecycle.PodAdmitResult{ - Message: fmt.Sprintf("Allocate failed due to %v, which is unexpected", err), - Reason: "UnexpectedAdmissionError", - Admit: false, - } + return admission.GetPodAdmitResult(err) } if m.cpuManager != nil { err = m.cpuManager.Allocate(pod, &container) if err != nil { - return lifecycle.PodAdmitResult{ - Message: fmt.Sprintf("Allocate failed due to %v, which is unexpected", err), - Reason: "UnexpectedAdmissionError", - Admit: false, - } + return admission.GetPodAdmitResult(err) } } if m.memoryManager != nil { err = m.memoryManager.Allocate(pod, &container) if err != nil { - return lifecycle.PodAdmitResult{ - Message: fmt.Sprintf("Allocate failed due to %v, which is unexpected", err), - Reason: "UnexpectedAdmissionError", - Admit: false, - } + return admission.GetPodAdmitResult(err) } } } - return lifecycle.PodAdmitResult{Admit: true} + return admission.GetPodAdmitResult(nil) } func (cm *containerManagerImpl) SystemCgroupsLimit() v1.ResourceList { diff --git a/pkg/kubelet/cm/container_manager_windows.go b/pkg/kubelet/cm/container_manager_windows.go index 1bc246c94b1..17fa16cdd9f 100644 --- a/pkg/kubelet/cm/container_manager_windows.go +++ b/pkg/kubelet/cm/container_manager_windows.go @@ -35,6 +35,7 @@ import ( podresourcesapi "k8s.io/kubelet/pkg/apis/podresources/v1" kubefeatures "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/kubelet/cadvisor" + "k8s.io/kubernetes/pkg/kubelet/cm/admission" "k8s.io/kubernetes/pkg/kubelet/cm/cpumanager" "k8s.io/kubernetes/pkg/kubelet/cm/devicemanager" "k8s.io/kubernetes/pkg/kubelet/cm/memorymanager" @@ -63,9 +64,7 @@ type containerManagerImpl struct { type noopWindowsResourceAllocator struct{} func (ra *noopWindowsResourceAllocator) Admit(attrs *lifecycle.PodAdmitAttributes) lifecycle.PodAdmitResult { - return lifecycle.PodAdmitResult{ - Admit: true, - } + return admission.GetPodAdmitResult(nil) } func (cm *containerManagerImpl) Start(node *v1.Node, diff --git a/pkg/kubelet/cm/topologymanager/fake_topology_manager.go b/pkg/kubelet/cm/topologymanager/fake_topology_manager.go index 407691e98f0..8a60aa23347 100644 --- a/pkg/kubelet/cm/topologymanager/fake_topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/fake_topology_manager.go @@ -19,6 +19,7 @@ package topologymanager import ( "k8s.io/api/core/v1" "k8s.io/klog/v2" + "k8s.io/kubernetes/pkg/kubelet/cm/admission" "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) @@ -64,7 +65,5 @@ func (m *fakeManager) RemoveContainer(containerID string) error { func (m *fakeManager) Admit(attrs *lifecycle.PodAdmitAttributes) lifecycle.PodAdmitResult { klog.InfoS("Topology Admit Handler") - return lifecycle.PodAdmitResult{ - Admit: true, - } + return admission.GetPodAdmitResult(nil) } diff --git a/pkg/kubelet/cm/topologymanager/scope.go b/pkg/kubelet/cm/topologymanager/scope.go index c5c6f36be97..912aba3fde0 100644 --- a/pkg/kubelet/cm/topologymanager/scope.go +++ b/pkg/kubelet/cm/topologymanager/scope.go @@ -17,12 +17,11 @@ limitations under the License. package topologymanager import ( - "fmt" - "sync" "k8s.io/api/core/v1" "k8s.io/klog/v2" + "k8s.io/kubernetes/pkg/kubelet/cm/admission" "k8s.io/kubernetes/pkg/kubelet/cm/containermap" "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) @@ -133,10 +132,10 @@ func (s *scope) admitPolicyNone(pod *v1.Pod) lifecycle.PodAdmitResult { for _, container := range append(pod.Spec.InitContainers, pod.Spec.Containers...) { err := s.allocateAlignedResources(pod, &container) if err != nil { - return unexpectedAdmissionError(err) + return admission.GetPodAdmitResult(err) } } - return admitPod() + return admission.GetPodAdmitResult(nil) } // It would be better to implement this function in topologymanager instead of scope @@ -150,23 +149,3 @@ func (s *scope) allocateAlignedResources(pod *v1.Pod, container *v1.Container) e } return nil } - -func topologyAffinityError() lifecycle.PodAdmitResult { - return lifecycle.PodAdmitResult{ - Message: "Resources cannot be allocated with Topology locality", - Reason: "TopologyAffinityError", - Admit: false, - } -} - -func unexpectedAdmissionError(err error) lifecycle.PodAdmitResult { - return lifecycle.PodAdmitResult{ - Message: fmt.Sprintf("Allocate failed due to %v, which is unexpected", err), - Reason: "UnexpectedAdmissionError", - Admit: false, - } -} - -func admitPod() lifecycle.PodAdmitResult { - return lifecycle.PodAdmitResult{Admit: true} -} diff --git a/pkg/kubelet/cm/topologymanager/scope_container.go b/pkg/kubelet/cm/topologymanager/scope_container.go index de45209625a..1e4e2f58fc0 100644 --- a/pkg/kubelet/cm/topologymanager/scope_container.go +++ b/pkg/kubelet/cm/topologymanager/scope_container.go @@ -19,6 +19,7 @@ package topologymanager import ( "k8s.io/api/core/v1" "k8s.io/klog/v2" + "k8s.io/kubernetes/pkg/kubelet/cm/admission" "k8s.io/kubernetes/pkg/kubelet/cm/containermap" "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) @@ -53,17 +54,17 @@ func (s *containerScope) Admit(pod *v1.Pod) lifecycle.PodAdmitResult { klog.InfoS("Best TopologyHint", "bestHint", bestHint, "pod", klog.KObj(pod), "containerName", container.Name) if !admit { - return topologyAffinityError() + return admission.GetPodAdmitResult(&TopologyAffinityError{}) } klog.InfoS("Topology Affinity", "bestHint", bestHint, "pod", klog.KObj(pod), "containerName", container.Name) s.setTopologyHints(string(pod.UID), container.Name, bestHint) err := s.allocateAlignedResources(pod, &container) if err != nil { - return unexpectedAdmissionError(err) + return admission.GetPodAdmitResult(err) } } - return admitPod() + return admission.GetPodAdmitResult(nil) } func (s *containerScope) accumulateProvidersHints(pod *v1.Pod, container *v1.Container) []map[string][]TopologyHint { diff --git a/pkg/kubelet/cm/topologymanager/scope_pod.go b/pkg/kubelet/cm/topologymanager/scope_pod.go index 9ccc6414dd9..b77682597b8 100644 --- a/pkg/kubelet/cm/topologymanager/scope_pod.go +++ b/pkg/kubelet/cm/topologymanager/scope_pod.go @@ -19,6 +19,7 @@ package topologymanager import ( "k8s.io/api/core/v1" "k8s.io/klog/v2" + "k8s.io/kubernetes/pkg/kubelet/cm/admission" "k8s.io/kubernetes/pkg/kubelet/cm/containermap" "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) @@ -51,7 +52,7 @@ func (s *podScope) Admit(pod *v1.Pod) lifecycle.PodAdmitResult { bestHint, admit := s.calculateAffinity(pod) klog.InfoS("Best TopologyHint", "bestHint", bestHint, "pod", klog.KObj(pod)) if !admit { - return topologyAffinityError() + return admission.GetPodAdmitResult(&TopologyAffinityError{}) } for _, container := range append(pod.Spec.InitContainers, pod.Spec.Containers...) { @@ -60,10 +61,10 @@ func (s *podScope) Admit(pod *v1.Pod) lifecycle.PodAdmitResult { err := s.allocateAlignedResources(pod, &container) if err != nil { - return unexpectedAdmissionError(err) + return admission.GetPodAdmitResult(err) } } - return admitPod() + return admission.GetPodAdmitResult(nil) } func (s *podScope) accumulateProvidersHints(pod *v1.Pod) []map[string][]TopologyHint { diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index 4f327e6efc0..7cd67d1aa60 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager.go @@ -36,8 +36,21 @@ const ( // present on a machine and the TopologyManager is enabled, an error will // be returned and the TopologyManager will not be loaded. maxAllowableNUMANodes = 8 + // ErrorTopologyAffinity represents the type for a TopologyAffinityError + ErrorTopologyAffinity = "TopologyAffinityError" ) +// TopologyAffinityError represents an resource alignment error +type TopologyAffinityError struct{} + +func (e TopologyAffinityError) Error() string { + return "Resources cannot be allocated with Topology locality" +} + +func (e TopologyAffinityError) Type() string { + return ErrorTopologyAffinity +} + // Manager interface provides methods for Kubelet to manage pod topology hints type Manager interface { // PodAdmitHandler is implemented by Manager diff --git a/pkg/kubelet/cm/topologymanager/topology_manager_test.go b/pkg/kubelet/cm/topologymanager/topology_manager_test.go index 6c84411c397..c0cc0980c65 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager_test.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager_test.go @@ -477,11 +477,17 @@ func TestAdmit(t *testing.T) { if ctnActual.Admit != tc.expected { t.Errorf("Error occurred, expected Admit in result to be %v got %v", tc.expected, ctnActual.Admit) } + if !ctnActual.Admit && ctnActual.Reason != ErrorTopologyAffinity { + t.Errorf("Error occurred, expected Reason in result to be %v got %v", ErrorTopologyAffinity, ctnActual.Reason) + } // Pod scope Admit podActual := podScopeManager.Admit(&podAttr) if podActual.Admit != tc.expected { t.Errorf("Error occurred, expected Admit in result to be %v got %v", tc.expected, podActual.Admit) } + if !ctnActual.Admit && ctnActual.Reason != ErrorTopologyAffinity { + t.Errorf("Error occurred, expected Reason in result to be %v got %v", ErrorTopologyAffinity, ctnActual.Reason) + } } }