From 95a3ac447f95fbcf8ff7242d0e6993cf72365f1a Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Sun, 2 Feb 2020 17:22:06 +0000 Subject: [PATCH 1/4] Fix bug in TopologManager RemoveContainer() Previously, we unconditionally removed *all* topology hints from a pod whenever just one container was being removed. This commit makes it so we only remove the hints for the single container being removed, and then conditionally remove the pod from the podTopologyHints[podUID] when no containers left in it. --- pkg/kubelet/cm/topologymanager/topology_manager.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index 4e452d71a00..8ae311b471c 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager.go @@ -194,10 +194,17 @@ func (m *manager) AddContainer(pod *v1.Pod, containerID string) error { } func (m *manager) RemoveContainer(containerID string) error { + klog.Infof("[topologymanager] RemoveContainer - Container ID: %v", containerID) + podUIDString := m.podMap[containerID] - delete(m.podTopologyHints, podUIDString) delete(m.podMap, containerID) - klog.Infof("[topologymanager] RemoveContainer - Container ID: %v podTopologyHints: %v", containerID, m.podTopologyHints) + if _, exists := m.podTopologyHints[podUIDString]; exists { + delete(m.podTopologyHints[podUIDString], containerID) + if len(m.podTopologyHints[podUIDString]) == 0 { + delete(m.podTopologyHints, podUIDString) + } + } + return nil } From adaa58b6cb04c10f5c046ae7cf443b5576636397 Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Sun, 2 Feb 2020 17:35:28 +0000 Subject: [PATCH 2/4] 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) { From bc686ea27b04fe29fce43cfe93457a7a9822dba6 Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Sun, 2 Feb 2020 18:16:07 +0000 Subject: [PATCH 3/4] Update TopologyManager.GetTopologyHints() to take pointers Previously, this function was taking full Pod and Container objects unnecessarily. This commit updates this so that they will take pointers instead. --- pkg/kubelet/cm/cpumanager/cpu_manager.go | 4 ++-- pkg/kubelet/cm/cpumanager/cpu_manager_test.go | 2 +- pkg/kubelet/cm/cpumanager/fake_cpu_manager.go | 2 +- pkg/kubelet/cm/cpumanager/policy.go | 2 +- pkg/kubelet/cm/cpumanager/policy_none.go | 2 +- pkg/kubelet/cm/cpumanager/policy_static.go | 4 ++-- pkg/kubelet/cm/cpumanager/topology_hints_test.go | 2 +- pkg/kubelet/cm/devicemanager/manager_stub.go | 2 +- pkg/kubelet/cm/devicemanager/topology_hints.go | 2 +- pkg/kubelet/cm/devicemanager/topology_hints_test.go | 2 +- pkg/kubelet/cm/devicemanager/types.go | 2 +- pkg/kubelet/cm/topologymanager/policy_test.go | 2 +- pkg/kubelet/cm/topologymanager/topology_manager.go | 8 ++++---- pkg/kubelet/cm/topologymanager/topology_manager_test.go | 6 +++--- 14 files changed, 21 insertions(+), 21 deletions(-) diff --git a/pkg/kubelet/cm/cpumanager/cpu_manager.go b/pkg/kubelet/cm/cpumanager/cpu_manager.go index 972813d9df5..3a86292133a 100644 --- a/pkg/kubelet/cm/cpumanager/cpu_manager.go +++ b/pkg/kubelet/cm/cpumanager/cpu_manager.go @@ -71,7 +71,7 @@ type Manager interface { // GetTopologyHints implements the topologymanager.HintProvider Interface // and is consulted to achieve NUMA aware resource alignment among this // and other resource controllers. - GetTopologyHints(v1.Pod, v1.Container) map[string][]topologymanager.TopologyHint + GetTopologyHints(*v1.Pod, *v1.Container) map[string][]topologymanager.TopologyHint } type manager struct { @@ -298,7 +298,7 @@ func (m *manager) State() state.Reader { return m.state } -func (m *manager) GetTopologyHints(pod v1.Pod, container v1.Container) map[string][]topologymanager.TopologyHint { +func (m *manager) GetTopologyHints(pod *v1.Pod, container *v1.Container) map[string][]topologymanager.TopologyHint { // Garbage collect any stranded resources before providing TopologyHints m.removeStaleState() // Delegate to active policy diff --git a/pkg/kubelet/cm/cpumanager/cpu_manager_test.go b/pkg/kubelet/cm/cpumanager/cpu_manager_test.go index 2b21ace5463..451943e6d4b 100644 --- a/pkg/kubelet/cm/cpumanager/cpu_manager_test.go +++ b/pkg/kubelet/cm/cpumanager/cpu_manager_test.go @@ -112,7 +112,7 @@ func (p *mockPolicy) RemoveContainer(s state.State, podUID string, containerName return p.err } -func (p *mockPolicy) GetTopologyHints(s state.State, pod v1.Pod, container v1.Container) map[string][]topologymanager.TopologyHint { +func (p *mockPolicy) GetTopologyHints(s state.State, pod *v1.Pod, container *v1.Container) map[string][]topologymanager.TopologyHint { return nil } diff --git a/pkg/kubelet/cm/cpumanager/fake_cpu_manager.go b/pkg/kubelet/cm/cpumanager/fake_cpu_manager.go index d3b3eca6c1c..d958935db62 100644 --- a/pkg/kubelet/cm/cpumanager/fake_cpu_manager.go +++ b/pkg/kubelet/cm/cpumanager/fake_cpu_manager.go @@ -50,7 +50,7 @@ func (m *fakeManager) RemoveContainer(containerID string) error { return nil } -func (m *fakeManager) GetTopologyHints(pod v1.Pod, container v1.Container) map[string][]topologymanager.TopologyHint { +func (m *fakeManager) GetTopologyHints(pod *v1.Pod, container *v1.Container) map[string][]topologymanager.TopologyHint { klog.Infof("[fake cpumanager] Get Topology Hints") return map[string][]topologymanager.TopologyHint{} } diff --git a/pkg/kubelet/cm/cpumanager/policy.go b/pkg/kubelet/cm/cpumanager/policy.go index 63e031b4a95..2031612be91 100644 --- a/pkg/kubelet/cm/cpumanager/policy.go +++ b/pkg/kubelet/cm/cpumanager/policy.go @@ -33,5 +33,5 @@ type Policy interface { // GetTopologyHints implements the topologymanager.HintProvider Interface // and is consulted to achieve NUMA aware resource alignment among this // and other resource controllers. - GetTopologyHints(s state.State, pod v1.Pod, container v1.Container) map[string][]topologymanager.TopologyHint + GetTopologyHints(s state.State, pod *v1.Pod, container *v1.Container) map[string][]topologymanager.TopologyHint } diff --git a/pkg/kubelet/cm/cpumanager/policy_none.go b/pkg/kubelet/cm/cpumanager/policy_none.go index be46369836b..77cd367b642 100644 --- a/pkg/kubelet/cm/cpumanager/policy_none.go +++ b/pkg/kubelet/cm/cpumanager/policy_none.go @@ -52,6 +52,6 @@ func (p *nonePolicy) RemoveContainer(s state.State, podUID string, containerName return nil } -func (p *nonePolicy) GetTopologyHints(s state.State, pod v1.Pod, container v1.Container) map[string][]topologymanager.TopologyHint { +func (p *nonePolicy) GetTopologyHints(s state.State, pod *v1.Pod, container *v1.Container) map[string][]topologymanager.TopologyHint { return nil } diff --git a/pkg/kubelet/cm/cpumanager/policy_static.go b/pkg/kubelet/cm/cpumanager/policy_static.go index 8502c1f2f0b..c088df866eb 100644 --- a/pkg/kubelet/cm/cpumanager/policy_static.go +++ b/pkg/kubelet/cm/cpumanager/policy_static.go @@ -276,7 +276,7 @@ func (p *staticPolicy) guaranteedCPUs(pod *v1.Pod, container *v1.Container) int return int(cpuQuantity.Value()) } -func (p *staticPolicy) GetTopologyHints(s state.State, pod v1.Pod, container v1.Container) map[string][]topologymanager.TopologyHint { +func (p *staticPolicy) GetTopologyHints(s state.State, pod *v1.Pod, container *v1.Container) map[string][]topologymanager.TopologyHint { // If there are no CPU resources requested for this container, we do not // generate any topology hints. if _, ok := container.Resources.Requests[v1.ResourceCPU]; !ok { @@ -284,7 +284,7 @@ func (p *staticPolicy) GetTopologyHints(s state.State, pod v1.Pod, container v1. } // Get a count of how many guaranteed CPUs have been requested. - requested := p.guaranteedCPUs(&pod, &container) + requested := p.guaranteedCPUs(pod, container) // If there are no guaranteed CPUs being requested, we do not generate // any topology hints. This can happen, for example, because init diff --git a/pkg/kubelet/cm/cpumanager/topology_hints_test.go b/pkg/kubelet/cm/cpumanager/topology_hints_test.go index aaf28f49a44..16761157aaf 100644 --- a/pkg/kubelet/cm/cpumanager/topology_hints_test.go +++ b/pkg/kubelet/cm/cpumanager/topology_hints_test.go @@ -265,7 +265,7 @@ func TestGetTopologyHints(t *testing.T) { sourcesReady: &sourcesReadyStub{}, } - hints := m.GetTopologyHints(tc.pod, tc.container)[string(v1.ResourceCPU)] + hints := m.GetTopologyHints(&tc.pod, &tc.container)[string(v1.ResourceCPU)] if len(tc.expectedHints) == 0 && len(hints) == 0 { continue } diff --git a/pkg/kubelet/cm/devicemanager/manager_stub.go b/pkg/kubelet/cm/devicemanager/manager_stub.go index a22e01b29fa..f35e0c74a4b 100644 --- a/pkg/kubelet/cm/devicemanager/manager_stub.go +++ b/pkg/kubelet/cm/devicemanager/manager_stub.go @@ -65,7 +65,7 @@ func (h *ManagerStub) GetWatcherHandler() cache.PluginHandler { } // GetTopologyHints returns an empty TopologyHint map -func (h *ManagerStub) GetTopologyHints(pod v1.Pod, container v1.Container) map[string][]topologymanager.TopologyHint { +func (h *ManagerStub) GetTopologyHints(pod *v1.Pod, container *v1.Container) map[string][]topologymanager.TopologyHint { return map[string][]topologymanager.TopologyHint{} } diff --git a/pkg/kubelet/cm/devicemanager/topology_hints.go b/pkg/kubelet/cm/devicemanager/topology_hints.go index 7a8d1925896..b8ea54ef87e 100644 --- a/pkg/kubelet/cm/devicemanager/topology_hints.go +++ b/pkg/kubelet/cm/devicemanager/topology_hints.go @@ -27,7 +27,7 @@ import ( // GetTopologyHints implements the TopologyManager HintProvider Interface which // ensures the Device Manager is consulted when Topology Aware Hints for each // container are created. -func (m *ManagerImpl) GetTopologyHints(pod v1.Pod, container v1.Container) map[string][]topologymanager.TopologyHint { +func (m *ManagerImpl) GetTopologyHints(pod *v1.Pod, container *v1.Container) map[string][]topologymanager.TopologyHint { // Garbage collect any stranded device resources before providing TopologyHints m.updateAllocatedDevices(m.activePods()) diff --git a/pkg/kubelet/cm/devicemanager/topology_hints_test.go b/pkg/kubelet/cm/devicemanager/topology_hints_test.go index b69a7e3c62d..02bc63eb55f 100644 --- a/pkg/kubelet/cm/devicemanager/topology_hints_test.go +++ b/pkg/kubelet/cm/devicemanager/topology_hints_test.go @@ -413,7 +413,7 @@ func TestGetTopologyHints(t *testing.T) { } } - hints := m.GetTopologyHints(*pod, pod.Spec.Containers[0]) + hints := m.GetTopologyHints(pod, &pod.Spec.Containers[0]) for r := range tc.expectedHints { sort.SliceStable(hints[r], func(i, j int) bool { diff --git a/pkg/kubelet/cm/devicemanager/types.go b/pkg/kubelet/cm/devicemanager/types.go index 4d7c9b8af9e..4f6351410bb 100644 --- a/pkg/kubelet/cm/devicemanager/types.go +++ b/pkg/kubelet/cm/devicemanager/types.go @@ -67,7 +67,7 @@ type Manager interface { // TopologyManager HintProvider provider indicates the Device Manager implements the Topology Manager Interface // and is consulted to make Topology aware resource alignments - GetTopologyHints(pod v1.Pod, container v1.Container) map[string][]topologymanager.TopologyHint + GetTopologyHints(pod *v1.Pod, container *v1.Container) map[string][]topologymanager.TopologyHint } // DeviceRunContainerOptions contains the combined container runtime settings to consume its allocated devices. diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index c8aa12bbaf9..d106e062092 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -858,7 +858,7 @@ func testPolicyMerge(policy Policy, tcases []policyMergeTestCase, t *testing.T) for _, tc := range tcases { var providersHints []map[string][]TopologyHint for _, provider := range tc.hp { - hints := provider.GetTopologyHints(v1.Pod{}, v1.Container{}) + hints := provider.GetTopologyHints(&v1.Pod{}, &v1.Container{}) providersHints = append(providersHints, hints) } diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index be421b97e51..7c65d4d1840 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager.go @@ -76,7 +76,7 @@ type HintProvider interface { // this function for each hint provider, and merges the hints to produce // a consensus "best" hint. The hint providers may subsequently query the // topology manager to influence actual resource assignment. - GetTopologyHints(pod v1.Pod, container v1.Container) map[string][]TopologyHint + GetTopologyHints(pod *v1.Pod, container *v1.Container) map[string][]TopologyHint } //Store interface is to allow Hint Providers to retrieve pod affinity @@ -164,7 +164,7 @@ func (m *manager) GetAffinity(podUID string, containerName string) TopologyHint return m.podTopologyHints[podUID][containerName] } -func (m *manager) accumulateProvidersHints(pod v1.Pod, container v1.Container) (providersHints []map[string][]TopologyHint) { +func (m *manager) accumulateProvidersHints(pod *v1.Pod, container *v1.Container) (providersHints []map[string][]TopologyHint) { // Loop through all hint providers and save an accumulated list of the // hints returned by each hint provider. for _, provider := range m.hintProviders { @@ -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, bool) { +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) @@ -221,7 +221,7 @@ func (m *manager) Admit(attrs *lifecycle.PodAdmitAttributes) lifecycle.PodAdmitR hints := make(map[string]TopologyHint) for _, container := range append(pod.Spec.InitContainers, pod.Spec.Containers...) { - result, admit := m.calculateAffinity(*pod, container) + result, admit := m.calculateAffinity(pod, &container) if !admit { return lifecycle.PodAdmitResult{ Message: "Resources cannot be allocated with Topology locality", diff --git a/pkg/kubelet/cm/topologymanager/topology_manager_test.go b/pkg/kubelet/cm/topologymanager/topology_manager_test.go index 93abb8765c6..2176306c99e 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager_test.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager_test.go @@ -77,7 +77,7 @@ type mockHintProvider struct { th map[string][]TopologyHint } -func (m *mockHintProvider) GetTopologyHints(pod v1.Pod, container v1.Container) map[string][]TopologyHint { +func (m *mockHintProvider) GetTopologyHints(pod *v1.Pod, container *v1.Container) map[string][]TopologyHint { return m.th } @@ -223,7 +223,7 @@ func TestAccumulateProvidersHints(t *testing.T) { mngr := manager{ hintProviders: tc.hp, } - actual := mngr.accumulateProvidersHints(v1.Pod{}, v1.Container{}) + actual := mngr.accumulateProvidersHints(&v1.Pod{}, &v1.Container{}) if !reflect.DeepEqual(actual, tc.expected) { t.Errorf("Test Case %s: Expected NUMANodeAffinity in result to be %v, got %v", tc.name, tc.expected, actual) } @@ -342,7 +342,7 @@ func TestCalculateAffinity(t *testing.T) { mngr := manager{} mngr.policy = &mockPolicy{} mngr.hintProviders = tc.hp - mngr.calculateAffinity(v1.Pod{}, v1.Container{}) + mngr.calculateAffinity(&v1.Pod{}, &v1.Container{}) actual := mngr.policy.(*mockPolicy).ph if !reflect.DeepEqual(tc.expected, actual) { t.Errorf("Test Case: %s", tc.name) From d5addb4090f7a631dccafabfddcb1d51ae00ff8d Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Sun, 2 Feb 2020 18:36:01 +0000 Subject: [PATCH 4/4] Cleanup logging and creation logic of TopologyManager in prep for beta --- .../cm/topologymanager/topology_manager.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index 7c65d4d1840..dab59aa8aa6 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager.go @@ -124,7 +124,7 @@ func NewManager(numaNodeInfo cputopology.NUMANodeInfo, topologyPolicyName string numaNodes = append(numaNodes, node) } - if len(numaNodes) > maxAllowableNUMANodes { + if topologyPolicyName != PolicyNone && len(numaNodes) > maxAllowableNUMANodes { return nil, fmt.Errorf("unsupported on machines with more than %v NUMA Nodes", maxAllowableNUMANodes) } @@ -209,14 +209,12 @@ func (m *manager) RemoveContainer(containerID string) error { } func (m *manager) Admit(attrs *lifecycle.PodAdmitAttributes) lifecycle.PodAdmitResult { - klog.Infof("[topologymanager] Topology Admit Handler") - if m.policy.Name() == "none" { - klog.Infof("[topologymanager] Skipping calculate topology affinity as policy: none") - return lifecycle.PodAdmitResult{ - Admit: true, - } + // Unconditionally admit the pod if we are running with the 'none' policy. + if m.policy.Name() == PolicyNone { + return lifecycle.PodAdmitResult{Admit: true} } + klog.Infof("[topologymanager] Topology Admit Handler") pod := attrs.Pod hints := make(map[string]TopologyHint) @@ -235,7 +233,5 @@ func (m *manager) Admit(attrs *lifecycle.PodAdmitAttributes) lifecycle.PodAdmitR 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{ - Admit: true, - } + return lifecycle.PodAdmitResult{Admit: true} }