Update TopologyManager.Policy.Merge() to return a simple bool

Previously, the verious Merge() policies of the TopologyManager all
eturned their own lifecycle.PodAdmitResult result. However, for
consistency in any failed admits, this is better handled in the
top-level Topology manager, with each policy only returning a boolean
about whether or not they would like to admit the pod or not. This
commit changes the semantics to match this logic.

pkg/kubelet/cm/topologymanager/policy.go

@@ -19,7 +19,6 @@ package topologymanager
 import (
 	"k8s.io/klog"
 	"k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask"
-	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
 )
 
 // Policy interface for Topology Manager Pod Admit Result
@@ -28,7 +27,7 @@ type Policy interface {
 	Name() string
 	// Returns a merged TopologyHint based on input from hint providers
 	// and a Pod Admit Handler Response based on hints and policy type
-	Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult)
+	Merge(providersHints []map[string][]TopologyHint) (TopologyHint, bool)
 }
 
 // Merge a TopologyHints permutation to a single hint by performing a bitwise-AND

pkg/kubelet/cm/topologymanager/policy_best_effort.go

@@ -16,10 +16,6 @@ limitations under the License.
 
 package topologymanager
 
-import (
-	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
-)
-
 type bestEffortPolicy struct {
 	//List of NUMA Nodes available on the underlying machine
 	numaNodes []int
@@ -39,13 +35,11 @@ func (p *bestEffortPolicy) Name() string {
 	return PolicyBestEffort
 }
 
-func (p *bestEffortPolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult {
+func (p *bestEffortPolicy) canAdmitPodResult(hint *TopologyHint) bool {
-	return lifecycle.PodAdmitResult{
+	return true
-		Admit: true,
-	}
 }
 
-func (p *bestEffortPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) {
+func (p *bestEffortPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, bool) {
 	filteredProvidersHints := filterProvidersHints(providersHints)
 	bestHint := mergeFilteredHints(p.numaNodes, filteredProvidersHints)
 	admit := p.canAdmitPodResult(&bestHint)

pkg/kubelet/cm/topologymanager/policy_best_effort_test.go

@@ -43,8 +43,8 @@ func TestPolicyBestEffortCanAdmitPodResult(t *testing.T) {
 		policy := NewBestEffortPolicy(numaNodes)
 		result := policy.(*bestEffortPolicy).canAdmitPodResult(&tc.hint)
 
-		if result.Admit != tc.expected {
+		if result != tc.expected {
-			t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit)
+			t.Errorf("Expected result to be %t, got %t", tc.expected, result)
 		}
 	}
 }

pkg/kubelet/cm/topologymanager/policy_none.go

@@ -16,10 +16,6 @@ limitations under the License.
 
 package topologymanager
 
-import (
-	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
-)
-
 type nonePolicy struct{}
 
 var _ Policy = &nonePolicy{}
@@ -36,12 +32,10 @@ func (p *nonePolicy) Name() string {
 	return PolicyNone
 }
 
-func (p *nonePolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult {
+func (p *nonePolicy) canAdmitPodResult(hint *TopologyHint) bool {
-	return lifecycle.PodAdmitResult{
+	return true
-		Admit: true,
-	}
 }
 
-func (p *nonePolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) {
+func (p *nonePolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, bool) {
 	return TopologyHint{}, p.canAdmitPodResult(nil)
 }

pkg/kubelet/cm/topologymanager/policy_none_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package topologymanager
 
 import (
-	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
 	"testing"
 )
 
@@ -61,8 +60,8 @@ func TestPolicyNoneCanAdmitPodResult(t *testing.T) {
 		policy := NewNonePolicy()
 		result := policy.(*nonePolicy).canAdmitPodResult(&tc.hint)
 
-		if result.Admit != tc.expected {
+		if result != tc.expected {
-			t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit)
+			t.Errorf("Expected result to be %t, got %t", tc.expected, result)
 		}
 	}
 }
@@ -72,13 +71,13 @@ func TestPolicyNoneMerge(t *testing.T) {
 		name           string
 		providersHints []map[string][]TopologyHint
 		expectedHint   TopologyHint
-		expectedAdmit  lifecycle.PodAdmitResult
+		expectedAdmit  bool
 	}{
 		{
 			name:           "merged empty providers hints",
 			providersHints: []map[string][]TopologyHint{},
 			expectedHint:   TopologyHint{},
-			expectedAdmit:  lifecycle.PodAdmitResult{Admit: true},
+			expectedAdmit:  true,
 		},
 		{
 			name: "merge with a single provider with a single preferred resource",
@@ -88,7 +87,7 @@ func TestPolicyNoneMerge(t *testing.T) {
 				},
 			},
 			expectedHint:  TopologyHint{},
-			expectedAdmit: lifecycle.PodAdmitResult{Admit: true},
+			expectedAdmit: true,
 		},
 		{
 			name: "merge with a single provider with a single non-preferred resource",
@@ -98,14 +97,14 @@ func TestPolicyNoneMerge(t *testing.T) {
 				},
 			},
 			expectedHint:  TopologyHint{},
-			expectedAdmit: lifecycle.PodAdmitResult{Admit: true},
+			expectedAdmit: true,
 		},
 	}
 
 	for _, tc := range tcases {
 		policy := NewNonePolicy()
 		result, admit := policy.Merge(tc.providersHints)
-		if !result.IsEqual(tc.expectedHint) || admit.Admit != tc.expectedAdmit.Admit {
+		if !result.IsEqual(tc.expectedHint) || admit != tc.expectedAdmit {
 			t.Errorf("Test Case: %s: Expected merge hint to be %v, got %v", tc.name, tc.expectedHint, result)
 		}
 	}

pkg/kubelet/cm/topologymanager/policy_restricted.go

@@ -16,10 +16,6 @@ limitations under the License.
 
 package topologymanager
 
-import (
-	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
-)
-
 type restrictedPolicy struct {
 	bestEffortPolicy
 }
@@ -38,20 +34,14 @@ func (p *restrictedPolicy) Name() string {
 	return PolicyRestricted
 }
 
-func (p *restrictedPolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult {
+func (p *restrictedPolicy) canAdmitPodResult(hint *TopologyHint) bool {
 	if !hint.Preferred {
-		return lifecycle.PodAdmitResult{
+		return false
-			Admit:   false,
-			Reason:  "Topology Affinity Error",
-			Message: "Resources cannot be allocated with Topology Locality",
-		}
-	}
-	return lifecycle.PodAdmitResult{
-		Admit: true,
 	}
+	return true
 }
 
-func (p *restrictedPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) {
+func (p *restrictedPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, bool) {
 	filteredHints := filterProvidersHints(providersHints)
 	hint := mergeFilteredHints(p.numaNodes, filteredHints)
 	admit := p.canAdmitPodResult(&hint)

pkg/kubelet/cm/topologymanager/policy_restricted_test.go

@@ -61,17 +61,8 @@ func TestPolicyRestrictedCanAdmitPodResult(t *testing.T) {
 		policy := NewRestrictedPolicy(numaNodes)
 		result := policy.(*restrictedPolicy).canAdmitPodResult(&tc.hint)
 
-		if result.Admit != tc.expected {
+		if result != tc.expected {
-			t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit)
+			t.Errorf("Expected result to be %t, got %t", tc.expected, result)
-		}
-
-		if tc.expected == false {
-			if len(result.Reason) == 0 {
-				t.Errorf("Expected Reason field to be not empty")
-			}
-			if len(result.Message) == 0 {
-				t.Errorf("Expected Message field to be not empty")
-			}
 		}
 	}
 }

pkg/kubelet/cm/topologymanager/policy_single_numa_node.go

@@ -18,7 +18,6 @@ package topologymanager
 
 import (
 	"k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask"
-	"k8s.io/kubernetes/pkg/kubelet/lifecycle"
 )
 
 type singleNumaNodePolicy struct {
@@ -40,17 +39,11 @@ func (p *singleNumaNodePolicy) Name() string {
 	return PolicySingleNumaNode
 }
 
-func (p *singleNumaNodePolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult {
+func (p *singleNumaNodePolicy) canAdmitPodResult(hint *TopologyHint) bool {
 	if !hint.Preferred {
-		return lifecycle.PodAdmitResult{
+		return false
-			Admit:   false,
-			Reason:  "Topology Affinity Error",
-			Message: "Resources cannot be allocated with Topology Locality",
-		}
-	}
-	return lifecycle.PodAdmitResult{
-		Admit: true,
 	}
+	return true
 }
 
 // Return hints that have valid bitmasks with exactly one bit set.
@@ -71,7 +64,7 @@ func filterSingleNumaHints(allResourcesHints [][]TopologyHint) [][]TopologyHint
 	return filteredResourcesHints
 }
 
-func (p *singleNumaNodePolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) {
+func (p *singleNumaNodePolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, bool) {
 	filteredHints := filterProvidersHints(providersHints)
 	// Filter to only include don't cares and hints with a single NUMA node.
 	singleNumaHints := filterSingleNumaHints(filteredHints)

pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go

@@ -39,17 +39,8 @@ func TestPolicySingleNumaNodeCanAdmitPodResult(t *testing.T) {
 		policy := NewSingleNumaNodePolicy(numaNodes)
 		result := policy.(*singleNumaNodePolicy).canAdmitPodResult(&tc.hint)
 
-		if result.Admit != tc.expected {
+		if result != tc.expected {
-			t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit)
+			t.Errorf("Expected result to be %t, got %t", tc.expected, result)
-		}
-
-		if tc.expected == false {
-			if len(result.Reason) == 0 {
-				t.Errorf("Expected Reason field to be not empty")
-			}
-			if len(result.Message) == 0 {
-				t.Errorf("Expected Message field to be not empty")
-			}
 		}
 	}
 }

pkg/kubelet/cm/topologymanager/topology_manager.go

@@ -177,7 +177,7 @@ func (m *manager) accumulateProvidersHints(pod v1.Pod, container v1.Container) (
 }
 
 // Collect Hints from hint providers and pass to policy to retrieve the best one.
-func (m *manager) calculateAffinity(pod v1.Pod, container v1.Container) (TopologyHint, lifecycle.PodAdmitResult) {
+func (m *manager) calculateAffinity(pod v1.Pod, container v1.Container) (TopologyHint, bool) {
 	providersHints := m.accumulateProvidersHints(pod, container)
 	bestHint, admit := m.policy.Merge(providersHints)
 	klog.Infof("[topologymanager] ContainerTopologyHint: %v", bestHint)
@@ -216,17 +216,23 @@ func (m *manager) Admit(attrs *lifecycle.PodAdmitAttributes) lifecycle.PodAdmitR
 			Admit: true,
 		}
 	}
+
 	pod := attrs.Pod
-	c := make(map[string]TopologyHint)
+	hints := make(map[string]TopologyHint)
 
 	for _, container := range append(pod.Spec.InitContainers, pod.Spec.Containers...) {
-		result, admitPod := m.calculateAffinity(*pod, container)
+		result, admit := m.calculateAffinity(*pod, container)
-		if !admitPod.Admit {
+		if !admit {
-			return admitPod
+			return lifecycle.PodAdmitResult{
+				Message: "Resources cannot be allocated with Topology locality",
+				Reason:  "TopologyAffinityError",
+				Admit:   false,
 			}
-		c[container.Name] = result
 		}
-	m.podTopologyHints[string(pod.UID)] = c
+		hints[container.Name] = result
+	}
+
+	m.podTopologyHints[string(pod.UID)] = hints
 	klog.Infof("[topologymanager] Topology Affinity for Pod: %v are %v", pod.UID, m.podTopologyHints[string(pod.UID)])
 
 	return lifecycle.PodAdmitResult{

pkg/kubelet/cm/topologymanager/topology_manager_test.go

@@ -235,9 +235,9 @@ type mockPolicy struct {
 	ph []map[string][]TopologyHint
 }
 
-func (p *mockPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) {
+func (p *mockPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, bool) {
 	p.ph = providersHints
-	return TopologyHint{}, lifecycle.PodAdmitResult{}
+	return TopologyHint{}, true
 }
 
 func TestCalculateAffinity(t *testing.T) {