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 <kklues@nvidia.com>
Co-authored-by: Swati Sehgal <swsehgal@redhat.com>
Signed-off-by: Francesco Romani <fromani@redhat.com>

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,
+	}
+}

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)
+		}
+	}
+}

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{
+			return admission.GetPodAdmitResult(err)
-				Message: fmt.Sprintf("Allocate failed due to %v, which is unexpected", err),
-				Reason:  "UnexpectedAdmissionError",
-				Admit:   false,
-			}
 		}
 
 		if m.cpuManager != nil {
 			err = m.cpuManager.Allocate(pod, &container)
 			if err != nil {
-				return lifecycle.PodAdmitResult{
+				return admission.GetPodAdmitResult(err)
-					Message: fmt.Sprintf("Allocate failed due to %v, which is unexpected", err),
-					Reason:  "UnexpectedAdmissionError",
-					Admit:   false,
-				}
 			}
 		}
 
 		if m.memoryManager != nil {
 			err = m.memoryManager.Allocate(pod, &container)
 			if err != nil {
-				return lifecycle.PodAdmitResult{
+				return admission.GetPodAdmitResult(err)
-					Message: fmt.Sprintf("Allocate failed due to %v, which is unexpected", err),
-					Reason:  "UnexpectedAdmissionError",
-					Admit:   false,
-				}
 			}
 		}
 	}
 
-	return lifecycle.PodAdmitResult{Admit: true}
+	return admission.GetPodAdmitResult(nil)
 }
 
 func (cm *containerManagerImpl) SystemCgroupsLimit() v1.ResourceList {

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{
+	return admission.GetPodAdmitResult(nil)
-		Admit: true,
-	}
 }
 
 func (cm *containerManagerImpl) Start(node *v1.Node,

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{
+	return admission.GetPodAdmitResult(nil)
-		Admit: true,
-	}
 }

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}
-}

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 {

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 {

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

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)
+		}
 	}
 }