From adaa58b6cb04c10f5c046ae7cf443b5576636397 Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Sun, 2 Feb 2020 17:35:28 +0000 Subject: [PATCH] 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 | 3 +-- .../cm/topologymanager/policy_best_effort.go | 12 +++-------- .../policy_best_effort_test.go | 4 ++-- pkg/kubelet/cm/topologymanager/policy_none.go | 12 +++-------- .../cm/topologymanager/policy_none_test.go | 15 +++++++------- .../cm/topologymanager/policy_restricted.go | 18 ++++------------- .../topologymanager/policy_restricted_test.go | 13 ++---------- .../policy_single_numa_node.go | 15 ++++---------- .../policy_single_numa_node_test.go | 13 ++---------- .../cm/topologymanager/topology_manager.go | 20 ++++++++++++------- .../topologymanager/topology_manager_test.go | 4 ++-- 11 files changed, 43 insertions(+), 86 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy.go b/pkg/kubelet/cm/topologymanager/policy.go index 8b845578253..3921bcc24aa 100644 --- a/pkg/kubelet/cm/topologymanager/policy.go +++ b/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 diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index 1f02094a045..651f3a76572 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/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 { - return lifecycle.PodAdmitResult{ - Admit: true, - } +func (p *bestEffortPolicy) canAdmitPodResult(hint *TopologyHint) bool { + return 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) diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go index b661516be01..3b23197920e 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go +++ b/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 { - t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit) + if result != tc.expected { + t.Errorf("Expected result to be %t, got %t", tc.expected, result) } } } diff --git a/pkg/kubelet/cm/topologymanager/policy_none.go b/pkg/kubelet/cm/topologymanager/policy_none.go index 7e5010a765d..271f9dde79b 100644 --- a/pkg/kubelet/cm/topologymanager/policy_none.go +++ b/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 { - return lifecycle.PodAdmitResult{ - Admit: true, - } +func (p *nonePolicy) canAdmitPodResult(hint *TopologyHint) bool { + return 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) } diff --git a/pkg/kubelet/cm/topologymanager/policy_none_test.go b/pkg/kubelet/cm/topologymanager/policy_none_test.go index 2c3263cec06..5ce33039bd2 100644 --- a/pkg/kubelet/cm/topologymanager/policy_none_test.go +++ b/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 { - t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit) + if result != tc.expected { + 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) } } diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted.go b/pkg/kubelet/cm/topologymanager/policy_restricted.go index e7882083b98..c3321c2030e 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted.go +++ b/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{ - Admit: false, - Reason: "Topology Affinity Error", - Message: "Resources cannot be allocated with Topology Locality", - } - } - return lifecycle.PodAdmitResult{ - Admit: true, + return false } + 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) diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go index d9463ea08a3..b8174f3cbbd 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go +++ b/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 { - t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit) - } - - 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") - } + if result != tc.expected { + t.Errorf("Expected result to be %t, got %t", tc.expected, result) } } } diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index 224d4686ac7..0e94c282d48 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/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{ - Admit: false, - Reason: "Topology Affinity Error", - Message: "Resources cannot be allocated with Topology Locality", - } - } - return lifecycle.PodAdmitResult{ - Admit: true, + return false } + 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) diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go index d201adcafbd..c6e86dba409 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go +++ b/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 { - t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit) - } - - 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") - } + if result != tc.expected { + t.Errorf("Expected result to be %t, got %t", tc.expected, result) } } } diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index 8ae311b471c..be421b97e51 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/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) - if !admitPod.Admit { - return admitPod + result, admit := m.calculateAffinity(*pod, container) + if !admit { + return lifecycle.PodAdmitResult{ + Message: "Resources cannot be allocated with Topology locality", + Reason: "TopologyAffinityError", + Admit: false, + } } - c[container.Name] = result + hints[container.Name] = result } - m.podTopologyHints[string(pod.UID)] = c + + 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{ diff --git a/pkg/kubelet/cm/topologymanager/topology_manager_test.go b/pkg/kubelet/cm/topologymanager/topology_manager_test.go index cd5b4aaf56f..93abb8765c6 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager_test.go +++ b/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) {