From b17706b149436767dfa7abb9e9d369946f90d5b4 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Mon, 4 Nov 2019 07:54:43 +0100 Subject: [PATCH 01/11] Added LessThan() and IsEqual() methods for TopologyHints --- .../cm/cpumanager/topology_hints_test.go | 11 ++-------- .../cm/devicemanager/topology_hints_test.go | 11 ++-------- .../cm/topologymanager/topology_manager.go | 21 +++++++++++++++++++ 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/pkg/kubelet/cm/cpumanager/topology_hints_test.go b/pkg/kubelet/cm/cpumanager/topology_hints_test.go index 3f0be9a6388..f998e734159 100644 --- a/pkg/kubelet/cm/cpumanager/topology_hints_test.go +++ b/pkg/kubelet/cm/cpumanager/topology_hints_test.go @@ -29,13 +29,6 @@ import ( "k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask" ) -func topologyHintLessThan(a topologymanager.TopologyHint, b topologymanager.TopologyHint) bool { - if a.Preferred != b.Preferred { - return a.Preferred == true - } - return a.NUMANodeAffinity.IsNarrowerThan(b.NUMANodeAffinity) -} - func TestGetTopologyHints(t *testing.T) { testPod1 := makePod("2", "2") testContainer1 := &testPod1.Spec.Containers[0] @@ -168,10 +161,10 @@ func TestGetTopologyHints(t *testing.T) { continue } sort.SliceStable(hints, func(i, j int) bool { - return topologyHintLessThan(hints[i], hints[j]) + return hints[i].LessThan(hints[j]) }) sort.SliceStable(tc.expectedHints, func(i, j int) bool { - return topologyHintLessThan(tc.expectedHints[i], tc.expectedHints[j]) + return tc.expectedHints[i].LessThan(tc.expectedHints[j]) }) if !reflect.DeepEqual(tc.expectedHints, hints) { t.Errorf("Expected in result to be %v , got %v", tc.expectedHints, hints) diff --git a/pkg/kubelet/cm/devicemanager/topology_hints_test.go b/pkg/kubelet/cm/devicemanager/topology_hints_test.go index 1222e042e45..e77cf3b4957 100644 --- a/pkg/kubelet/cm/devicemanager/topology_hints_test.go +++ b/pkg/kubelet/cm/devicemanager/topology_hints_test.go @@ -44,13 +44,6 @@ func makeNUMADevice(id string, numa int) pluginapi.Device { } } -func topologyHintLessThan(a topologymanager.TopologyHint, b topologymanager.TopologyHint) bool { - if a.Preferred != b.Preferred { - return a.Preferred == true - } - return a.NUMANodeAffinity.IsNarrowerThan(b.NUMANodeAffinity) -} - func makeSocketMask(sockets ...int) bitmask.BitMask { mask, _ := bitmask.NewBitMask(sockets...) return mask @@ -292,10 +285,10 @@ func TestGetTopologyHints(t *testing.T) { for r := range tc.expectedHints { sort.SliceStable(hints[r], func(i, j int) bool { - return topologyHintLessThan(hints[r][i], hints[r][j]) + return hints[r][i].LessThan(hints[r][j]) }) sort.SliceStable(tc.expectedHints[r], func(i, j int) bool { - return topologyHintLessThan(tc.expectedHints[r][i], tc.expectedHints[r][j]) + return tc.expectedHints[r][i].LessThan(tc.expectedHints[r][j]) }) if !reflect.DeepEqual(hints[r], tc.expectedHints[r]) { t.Errorf("%v: Expected result to be %v, got %v", tc.description, tc.expectedHints[r], hints[r]) diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index e8cb592fac5..03243e21701 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager.go @@ -94,6 +94,27 @@ type TopologyHint struct { Preferred bool } +// IsEqual checks if TopologyHint are equal +func (th *TopologyHint) IsEqual(topologyHint TopologyHint) bool { + if th.Preferred == topologyHint.Preferred { + if th.NUMANodeAffinity == nil || topologyHint.NUMANodeAffinity == nil { + return th.NUMANodeAffinity == topologyHint.NUMANodeAffinity + } + return th.NUMANodeAffinity.IsEqual(topologyHint.NUMANodeAffinity) + } + return false +} + +// LessThan checks if TopologyHint `a` is less than TopologyHint `b` +// this means that either `a` is a preferred hint and `b` is not +// or `a` NUMANodeAffinity attribute is narrower than `b` NUMANodeAffinity attribute. +func (th *TopologyHint) LessThan(other TopologyHint) bool { + if th.Preferred != other.Preferred { + return th.Preferred == true + } + return th.NUMANodeAffinity.IsNarrowerThan(other.NUMANodeAffinity) +} + var _ Manager = &manager{} //NewManager creates a new TopologyManager based on provided policy From 3391daeb00fc2ef4a8ec7b1969d11aded60ce7f1 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Mon, 4 Nov 2019 09:21:05 +0100 Subject: [PATCH 02/11] Break TopologyManager.calculateAffinity() into more modular functions This modularization is in preparation for a larger refactoring effort that will add a 'Merge()' API to the TopologyManager policy API. --- .../cm/topologymanager/topology_manager.go | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index 03243e21701..ef78cd0796e 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager.go @@ -167,6 +167,18 @@ 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) { + // Loop through all hint providers and save an accumulated list of the + // hints returned by each hint provider. + for _, provider := range m.hintProviders { + // Get the TopologyHints from a provider. + hints := provider.GetTopologyHints(pod, container) + providersHints = append(providersHints, hints) + klog.Infof("[topologymanager] TopologyHints for pod '%v', container '%v': %v", pod.Name, container.Name, hints) + } + return providersHints +} + // Iterate over all permutations of hints in 'allProviderHints [][]TopologyHint'. // // This procedure is implemented as a recursive function over the set of hints @@ -206,7 +218,7 @@ func (m *manager) iterateAllProviderTopologyHints(allProviderHints [][]TopologyH } // Merge the hints from all hint providers to find the best one. -func (m *manager) calculateAffinity(pod v1.Pod, container v1.Container) TopologyHint { +func (m *manager) mergeProvidersHints(providersHints []map[string][]TopologyHint) TopologyHint { // Set the default affinity as an any-numa affinity containing the list // of NUMA Nodes available on this machine. defaultAffinity, _ := bitmask.NewBitMask(m.numaNodes...) @@ -215,10 +227,7 @@ func (m *manager) calculateAffinity(pod v1.Pod, container v1.Container) Topology // hints returned by each hint provider. If no hints are provided, assume // that provider has no preference for topology-aware allocation. var allProviderHints [][]TopologyHint - for _, provider := range m.hintProviders { - // Get the TopologyHints from a provider. - hints := provider.GetTopologyHints(pod, container) - + for _, hints := range providersHints { // If hints is nil, insert a single, preferred any-numa hint into allProviderHints. if len(hints) == 0 { klog.Infof("[topologymanager] Hint Provider has no preference for NUMA affinity with any resource") @@ -312,8 +321,14 @@ func (m *manager) calculateAffinity(pod v1.Pod, container v1.Container) Topology bestHint = mergedHint }) - klog.Infof("[topologymanager] ContainerTopologyHint: %v", bestHint) + return bestHint +} +// 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 { + providersHints := m.accumulateProvidersHints(pod, container) + bestHint := m.mergeProvidersHints(providersHints) + klog.Infof("[topologymanager] ContainerTopologyHint: %v", bestHint) return bestHint } From 6fd8a6eb691960872de1584c1a17ceda565bb3b6 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Mon, 4 Nov 2019 11:52:22 +0100 Subject: [PATCH 03/11] Make restricted TopologyManager policy inherit from best-effort policy These policies only differ on whether they admit the pod or not when a TopologyHint is preferred or not. As such, the restricted policy should simply inherit whatever it can from the best effort policy and only overwrite what is necessary. This does not matter for now, but will become important when we add a new 'Merge()' abstraction to a Policy later on. --- pkg/kubelet/cm/topologymanager/policy_restricted.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted.go b/pkg/kubelet/cm/topologymanager/policy_restricted.go index 7993675ce35..531fff89550 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted.go +++ b/pkg/kubelet/cm/topologymanager/policy_restricted.go @@ -20,7 +20,9 @@ import ( "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) -type restrictedPolicy struct{} +type restrictedPolicy struct { + bestEffortPolicy +} var _ Policy = &restrictedPolicy{} @@ -29,7 +31,7 @@ const PolicyRestricted string = "restricted" // NewRestrictedPolicy returns restricted policy. func NewRestrictedPolicy() Policy { - return &restrictedPolicy{} + return &restrictedPolicy{bestEffortPolicy{}} } func (p *restrictedPolicy) Name() string { From e72847676fc3781daddc773b9c31671c89da0753 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Mon, 4 Nov 2019 10:10:03 +0100 Subject: [PATCH 04/11] Pass a list of NUMA nodes to the various TopologyManager policies This is in preparation for a larger refactoring effort that will add a 'Merge()' API to the TopologyManager policy API. --- .../cm/topologymanager/policy_best_effort.go | 9 +++-- .../policy_best_effort_test.go | 3 +- .../cm/topologymanager/policy_restricted.go | 4 +- .../topologymanager/policy_restricted_test.go | 3 +- .../policy_single_numa_node.go | 9 +++-- .../policy_single_numa_node_test.go | 3 +- .../cm/topologymanager/topology_manager.go | 38 +++++++++---------- .../topologymanager/topology_manager_test.go | 32 ++++++++-------- 8 files changed, 56 insertions(+), 45 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index b5f81b2fdb4..c0611d31a10 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort.go @@ -20,7 +20,10 @@ import ( "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) -type bestEffortPolicy struct{} +type bestEffortPolicy struct { + //List of NUMA Nodes available on the underlying machine + numaNodes []int +} var _ Policy = &bestEffortPolicy{} @@ -28,8 +31,8 @@ var _ Policy = &bestEffortPolicy{} const PolicyBestEffort string = "best-effort" // NewBestEffortPolicy returns best-effort policy. -func NewBestEffortPolicy() Policy { - return &bestEffortPolicy{} +func NewBestEffortPolicy(numaNodes []int) Policy { + return &bestEffortPolicy{numaNodes: numaNodes} } func (p *bestEffortPolicy) Name() string { diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go index 2fea3df9920..9505c72f89f 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go @@ -39,7 +39,8 @@ func TestPolicyBestEffortCanAdmitPodResult(t *testing.T) { } for _, tc := range tcases { - policy := NewBestEffortPolicy() + numaNodes := []int{0, 1} + policy := NewBestEffortPolicy(numaNodes) result := policy.CanAdmitPodResult(&tc.hint) if result.Admit != tc.expected { diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted.go b/pkg/kubelet/cm/topologymanager/policy_restricted.go index 531fff89550..02e188a35b8 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted.go +++ b/pkg/kubelet/cm/topologymanager/policy_restricted.go @@ -30,8 +30,8 @@ var _ Policy = &restrictedPolicy{} const PolicyRestricted string = "restricted" // NewRestrictedPolicy returns restricted policy. -func NewRestrictedPolicy() Policy { - return &restrictedPolicy{bestEffortPolicy{}} +func NewRestrictedPolicy(numaNodes []int) Policy { + return &restrictedPolicy{bestEffortPolicy{numaNodes: numaNodes}} } func (p *restrictedPolicy) Name() string { diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go index d011971632a..2543d8af0c3 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go @@ -39,7 +39,8 @@ func TestPolicyRestrictedCanAdmitPodResult(t *testing.T) { } for _, tc := range tcases { - policy := NewRestrictedPolicy() + numaNodes := []int{0, 1} + policy := NewRestrictedPolicy(numaNodes) result := policy.CanAdmitPodResult(&tc.hint) if result.Admit != tc.expected { diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index e17e5ca949c..7173ef94c5d 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -20,7 +20,10 @@ import ( "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) -type singleNumaNodePolicy struct{} +type singleNumaNodePolicy struct { + //List of NUMA Nodes available on the underlying machine + numaNodes []int +} var _ Policy = &singleNumaNodePolicy{} @@ -28,8 +31,8 @@ var _ Policy = &singleNumaNodePolicy{} const PolicySingleNumaNode string = "single-numa-node" // NewSingleNumaNodePolicy returns single-numa-node policy. -func NewSingleNumaNodePolicy() Policy { - return &singleNumaNodePolicy{} +func NewSingleNumaNodePolicy(numaNodes []int) Policy { + return &singleNumaNodePolicy{numaNodes: numaNodes} } func (p *singleNumaNodePolicy) Name() string { 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 44ea8370edd..594fef71c6b 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go @@ -34,7 +34,8 @@ func TestPolicySingleNumaNodeCanAdmitPodResult(t *testing.T) { } for _, tc := range tcases { - policy := NewSingleNumaNodePolicy() + numaNodes := []int{0, 1} + policy := NewSingleNumaNodePolicy(numaNodes) result := policy.CanAdmitPodResult(&tc.hint) if result.Admit != tc.expected { diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index ef78cd0796e..129d63dcdb7 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager.go @@ -120,25 +120,6 @@ var _ Manager = &manager{} //NewManager creates a new TopologyManager based on provided policy func NewManager(numaNodeInfo cputopology.NUMANodeInfo, topologyPolicyName string) (Manager, error) { klog.Infof("[topologymanager] Creating topology manager with %s policy", topologyPolicyName) - var policy Policy - - switch topologyPolicyName { - - case PolicyNone: - policy = NewNonePolicy() - - case PolicyBestEffort: - policy = NewBestEffortPolicy() - - case PolicyRestricted: - policy = NewRestrictedPolicy() - - case PolicySingleNumaNode: - policy = NewSingleNumaNodePolicy() - - default: - return nil, fmt.Errorf("unknown policy: \"%s\"", topologyPolicyName) - } var numaNodes []int for node := range numaNodeInfo { @@ -149,6 +130,25 @@ func NewManager(numaNodeInfo cputopology.NUMANodeInfo, topologyPolicyName string return nil, fmt.Errorf("unsupported on machines with more than %v NUMA Nodes", maxAllowableNUMANodes) } + var policy Policy + switch topologyPolicyName { + + case PolicyNone: + policy = NewNonePolicy() + + case PolicyBestEffort: + policy = NewBestEffortPolicy(numaNodes) + + case PolicyRestricted: + policy = NewRestrictedPolicy(numaNodes) + + case PolicySingleNumaNode: + policy = NewSingleNumaNodePolicy(numaNodes) + + default: + return nil, fmt.Errorf("unknown policy: \"%s\"", topologyPolicyName) + } + var hp []HintProvider pth := make(map[string]map[string]TopologyHint) pm := make(map[string]string) diff --git a/pkg/kubelet/cm/topologymanager/topology_manager_test.go b/pkg/kubelet/cm/topologymanager/topology_manager_test.go index 50c122cf460..0a79d10d38f 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager_test.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager_test.go @@ -658,7 +658,7 @@ func TestCalculateAffinity(t *testing.T) { }, { name: "Special cased PolicySingleNumaNode for single NUMA hint generation", - policy: NewSingleNumaNodePolicy(), + policy: NewSingleNumaNodePolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -692,7 +692,7 @@ func TestCalculateAffinity(t *testing.T) { }, { name: "Special cased PolicySingleNumaNode with one no-preference provider", - policy: NewSingleNumaNodePolicy(), + policy: NewSingleNumaNodePolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -835,6 +835,8 @@ func TestAddHintProvider(t *testing.T) { } func TestAdmit(t *testing.T) { + numaNodes := []int{0, 1} + tcases := []struct { name string result lifecycle.PodAdmitResult @@ -860,7 +862,7 @@ func TestAdmit(t *testing.T) { { name: "QOSClass set as BestEffort. single-numa-node Policy. No Hints.", qosClass: v1.PodQOSBestEffort, - policy: NewRestrictedPolicy(), + policy: NewRestrictedPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{}, }, @@ -869,7 +871,7 @@ func TestAdmit(t *testing.T) { { name: "QOSClass set as BestEffort. Restricted Policy. No Hints.", qosClass: v1.PodQOSBestEffort, - policy: NewRestrictedPolicy(), + policy: NewRestrictedPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{}, }, @@ -878,7 +880,7 @@ func TestAdmit(t *testing.T) { { name: "QOSClass set as Guaranteed. BestEffort Policy. Preferred Affinity.", qosClass: v1.PodQOSGuaranteed, - policy: NewBestEffortPolicy(), + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -900,7 +902,7 @@ func TestAdmit(t *testing.T) { { name: "QOSClass set as Guaranteed. BestEffort Policy. More than one Preferred Affinity.", qosClass: v1.PodQOSGuaranteed, - policy: NewBestEffortPolicy(), + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -926,7 +928,7 @@ func TestAdmit(t *testing.T) { { name: "QOSClass set as Burstable. BestEffort Policy. More than one Preferred Affinity.", qosClass: v1.PodQOSBurstable, - policy: NewBestEffortPolicy(), + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -952,7 +954,7 @@ func TestAdmit(t *testing.T) { { name: "QOSClass set as Guaranteed. BestEffort Policy. No Preferred Affinity.", qosClass: v1.PodQOSGuaranteed, - policy: NewBestEffortPolicy(), + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -970,7 +972,7 @@ func TestAdmit(t *testing.T) { { name: "QOSClass set as Guaranteed. Restricted Policy. Preferred Affinity.", qosClass: v1.PodQOSGuaranteed, - policy: NewRestrictedPolicy(), + policy: NewRestrictedPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -992,7 +994,7 @@ func TestAdmit(t *testing.T) { { name: "QOSClass set as Burstable. Restricted Policy. Preferred Affinity.", qosClass: v1.PodQOSBurstable, - policy: NewRestrictedPolicy(), + policy: NewRestrictedPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -1014,7 +1016,7 @@ func TestAdmit(t *testing.T) { { name: "QOSClass set as Guaranteed. Restricted Policy. More than one Preferred affinity.", qosClass: v1.PodQOSGuaranteed, - policy: NewRestrictedPolicy(), + policy: NewRestrictedPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -1040,7 +1042,7 @@ func TestAdmit(t *testing.T) { { name: "QOSClass set as Burstable. Restricted Policy. More than one Preferred affinity.", qosClass: v1.PodQOSBurstable, - policy: NewRestrictedPolicy(), + policy: NewRestrictedPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -1066,7 +1068,7 @@ func TestAdmit(t *testing.T) { { name: "QOSClass set as Guaranteed. Restricted Policy. No Preferred affinity.", qosClass: v1.PodQOSGuaranteed, - policy: NewRestrictedPolicy(), + policy: NewRestrictedPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -1084,7 +1086,7 @@ func TestAdmit(t *testing.T) { { name: "QOSClass set as Burstable. Restricted Policy. No Preferred affinity.", qosClass: v1.PodQOSBurstable, - policy: NewRestrictedPolicy(), + policy: NewRestrictedPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -1105,7 +1107,7 @@ func TestAdmit(t *testing.T) { policy: tc.policy, podTopologyHints: make(map[string]map[string]TopologyHint), hintProviders: tc.hp, - numaNodes: []int{0, 1}, + numaNodes: numaNodes, } pod := &v1.Pod{ From 78d7856288d1df9a57995ea610f5d154a9b458f7 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Mon, 4 Nov 2019 11:38:56 +0100 Subject: [PATCH 05/11] Globalize a few TopologyManager functions This is in preparation for a larger refactoring effort that will add a 'Merge()' API to the TopologyManager policy API. --- pkg/kubelet/cm/topologymanager/topology_manager.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index 129d63dcdb7..f8288d8cef8 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager.go @@ -198,7 +198,7 @@ func (m *manager) accumulateProvidersHints(pod v1.Pod, container v1.Container) ( // provideryHints[-1][z] // } // callback(permutation) -func (m *manager) iterateAllProviderTopologyHints(allProviderHints [][]TopologyHint, callback func([]TopologyHint)) { +func iterateAllProviderTopologyHints(allProviderHints [][]TopologyHint, callback func([]TopologyHint)) { // Internal helper function to accumulate the permutation before calling the callback. var iterate func(i int, accum []TopologyHint) iterate = func(i int, accum []TopologyHint) { @@ -218,10 +218,10 @@ func (m *manager) iterateAllProviderTopologyHints(allProviderHints [][]TopologyH } // Merge the hints from all hint providers to find the best one. -func (m *manager) mergeProvidersHints(providersHints []map[string][]TopologyHint) TopologyHint { +func mergeProvidersHints(policy Policy, numaNodes []int, providersHints []map[string][]TopologyHint) TopologyHint { // Set the default affinity as an any-numa affinity containing the list // of NUMA Nodes available on this machine. - defaultAffinity, _ := bitmask.NewBitMask(m.numaNodes...) + defaultAffinity, _ := bitmask.NewBitMask(numaNodes...) // Loop through all hint providers and save an accumulated list of the // hints returned by each hint provider. If no hints are provided, assume @@ -259,7 +259,7 @@ func (m *manager) mergeProvidersHints(providersHints []map[string][]TopologyHint // permutations that have at least one NUMA ID set. If no merged mask can be // found that has at least one NUMA ID set, return the 'defaultAffinity'. bestHint := TopologyHint{defaultAffinity, false} - m.iterateAllProviderTopologyHints(allProviderHints, func(permutation []TopologyHint) { + iterateAllProviderTopologyHints(allProviderHints, func(permutation []TopologyHint) { // Get the NUMANodeAffinity from each hint in the permutation and see if any // of them encode unpreferred allocations. preferred := true @@ -277,13 +277,13 @@ func (m *manager) mergeProvidersHints(providersHints []map[string][]TopologyHint // Special case PolicySingleNumaNode to only prefer hints where // all providers have a single NUMA affinity set. - if m.policy != nil && m.policy.Name() == PolicySingleNumaNode && hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() > 1 { + if policy != nil && policy.Name() == PolicySingleNumaNode && hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() > 1 { preferred = false } } // Merge the affinities using a bitwise-and operation. - mergedAffinity, _ := bitmask.NewBitMask(m.numaNodes...) + mergedAffinity, _ := bitmask.NewBitMask(numaNodes...) mergedAffinity.And(numaAffinities...) // Build a mergedHintfrom the merged affinity mask, indicating if an @@ -327,7 +327,7 @@ func (m *manager) mergeProvidersHints(providersHints []map[string][]TopologyHint // 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 { providersHints := m.accumulateProvidersHints(pod, container) - bestHint := m.mergeProvidersHints(providersHints) + bestHint := mergeProvidersHints(m.policy, m.numaNodes, providersHints) klog.Infof("[topologymanager] ContainerTopologyHint: %v", bestHint) return bestHint } From d95464645cdad87a104080bb36a0a1e5a833ebf1 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Mon, 4 Nov 2019 12:38:43 +0100 Subject: [PATCH 06/11] Add Merge() API to TopologyManager Policy abstraction This abstraction moves the responsibility of merging topology hints to the individual policies themselves. As part of this, it removes the CanAdmitPodResult() API from the policy abstraction, and rolls it into a second return value from Merge() --- pkg/kubelet/cm/topologymanager/policy.go | 5 +- .../cm/topologymanager/policy_best_effort.go | 8 ++- .../policy_best_effort_test.go | 2 +- pkg/kubelet/cm/topologymanager/policy_none.go | 6 +- .../cm/topologymanager/policy_none_test.go | 2 +- .../cm/topologymanager/policy_restricted.go | 8 ++- .../topologymanager/policy_restricted_test.go | 2 +- .../policy_single_numa_node.go | 8 ++- .../policy_single_numa_node_test.go | 2 +- .../cm/topologymanager/topology_manager.go | 12 ++-- .../topologymanager/topology_manager_test.go | 69 ++++++++++++------- 11 files changed, 81 insertions(+), 43 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy.go b/pkg/kubelet/cm/topologymanager/policy.go index 7ffa0a79f10..05f46714eab 100644 --- a/pkg/kubelet/cm/topologymanager/policy.go +++ b/pkg/kubelet/cm/topologymanager/policy.go @@ -24,6 +24,7 @@ import ( type Policy interface { //Returns Policy Name Name() string - //Returns Pod Admit Handler Response based on hints and policy type - CanAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult + // 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) } diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index c0611d31a10..97c61f031da 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort.go @@ -39,8 +39,14 @@ func (p *bestEffortPolicy) Name() string { return PolicyBestEffort } -func (p *bestEffortPolicy) CanAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { +func (p *bestEffortPolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { return lifecycle.PodAdmitResult{ Admit: true, } } + +func (p *bestEffortPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { + hint := mergeProvidersHints(p, p.numaNodes, providersHints) + admit := p.canAdmitPodResult(&hint) + return hint, admit +} diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go index 9505c72f89f..1459a823bc3 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go @@ -41,7 +41,7 @@ func TestPolicyBestEffortCanAdmitPodResult(t *testing.T) { for _, tc := range tcases { numaNodes := []int{0, 1} policy := NewBestEffortPolicy(numaNodes) - result := policy.CanAdmitPodResult(&tc.hint) + 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) diff --git a/pkg/kubelet/cm/topologymanager/policy_none.go b/pkg/kubelet/cm/topologymanager/policy_none.go index aacdf19aa10..7e5010a765d 100644 --- a/pkg/kubelet/cm/topologymanager/policy_none.go +++ b/pkg/kubelet/cm/topologymanager/policy_none.go @@ -36,8 +36,12 @@ func (p *nonePolicy) Name() string { return PolicyNone } -func (p *nonePolicy) CanAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { +func (p *nonePolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { return lifecycle.PodAdmitResult{ Admit: true, } } + +func (p *nonePolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { + 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 76f46e490b4..cafcaf044b6 100644 --- a/pkg/kubelet/cm/topologymanager/policy_none_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_none_test.go @@ -58,7 +58,7 @@ func TestPolicyNoneCanAdmitPodResult(t *testing.T) { for _, tc := range tcases { policy := NewNonePolicy() - result := policy.CanAdmitPodResult(&tc.hint) + 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) diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted.go b/pkg/kubelet/cm/topologymanager/policy_restricted.go index 02e188a35b8..cc522980c0e 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted.go +++ b/pkg/kubelet/cm/topologymanager/policy_restricted.go @@ -38,7 +38,7 @@ func (p *restrictedPolicy) Name() string { return PolicyRestricted } -func (p *restrictedPolicy) CanAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { +func (p *restrictedPolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { if !hint.Preferred { return lifecycle.PodAdmitResult{ Admit: false, @@ -50,3 +50,9 @@ func (p *restrictedPolicy) CanAdmitPodResult(hint *TopologyHint) lifecycle.PodAd Admit: true, } } + +func (p *restrictedPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { + hint := mergeProvidersHints(p, p.numaNodes, providersHints) + admit := p.canAdmitPodResult(&hint) + return hint, admit +} diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go index 2543d8af0c3..11119a0400c 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go @@ -41,7 +41,7 @@ func TestPolicyRestrictedCanAdmitPodResult(t *testing.T) { for _, tc := range tcases { numaNodes := []int{0, 1} policy := NewRestrictedPolicy(numaNodes) - result := policy.CanAdmitPodResult(&tc.hint) + 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) diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index 7173ef94c5d..6f29e1c27b7 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -39,7 +39,7 @@ func (p *singleNumaNodePolicy) Name() string { return PolicySingleNumaNode } -func (p *singleNumaNodePolicy) CanAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { +func (p *singleNumaNodePolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAdmitResult { if !hint.Preferred { return lifecycle.PodAdmitResult{ Admit: false, @@ -51,3 +51,9 @@ func (p *singleNumaNodePolicy) CanAdmitPodResult(hint *TopologyHint) lifecycle.P Admit: true, } } + +func (p *singleNumaNodePolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { + hint := mergeProvidersHints(p, p.numaNodes, providersHints) + admit := p.canAdmitPodResult(&hint) + return hint, admit +} 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 594fef71c6b..cdc895295fc 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go @@ -36,7 +36,7 @@ func TestPolicySingleNumaNodeCanAdmitPodResult(t *testing.T) { for _, tc := range tcases { numaNodes := []int{0, 1} policy := NewSingleNumaNodePolicy(numaNodes) - result := policy.CanAdmitPodResult(&tc.hint) + 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) diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index f8288d8cef8..3600d3700db 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager.go @@ -63,8 +63,6 @@ type manager struct { podMap map[string]string //Topology Manager Policy policy Policy - //List of NUMA Nodes available on the underlying machine - numaNodes []int } // HintProvider is an interface for components that want to collaborate to @@ -157,7 +155,6 @@ func NewManager(numaNodeInfo cputopology.NUMANodeInfo, topologyPolicyName string podTopologyHints: pth, podMap: pm, policy: policy, - numaNodes: numaNodes, } return manager, nil @@ -325,11 +322,11 @@ func mergeProvidersHints(policy Policy, numaNodes []int, providersHints []map[st } // 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 { +func (m *manager) calculateAffinity(pod v1.Pod, container v1.Container) (TopologyHint, lifecycle.PodAdmitResult) { providersHints := m.accumulateProvidersHints(pod, container) - bestHint := mergeProvidersHints(m.policy, m.numaNodes, providersHints) + bestHint, admit := m.policy.Merge(providersHints) klog.Infof("[topologymanager] ContainerTopologyHint: %v", bestHint) - return bestHint + return bestHint, admit } func (m *manager) AddHintProvider(h HintProvider) { @@ -361,8 +358,7 @@ func (m *manager) Admit(attrs *lifecycle.PodAdmitAttributes) lifecycle.PodAdmitR c := make(map[string]TopologyHint) for _, container := range append(pod.Spec.InitContainers, pod.Spec.Containers...) { - result := m.calculateAffinity(*pod, container) - admitPod := m.policy.CanAdmitPodResult(&result) + result, admitPod := m.calculateAffinity(*pod, container) if !admitPod.Admit { return admitPod } diff --git a/pkg/kubelet/cm/topologymanager/topology_manager_test.go b/pkg/kubelet/cm/topologymanager/topology_manager_test.go index 0a79d10d38f..49c4e1cea88 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager_test.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager_test.go @@ -114,15 +114,17 @@ func TestCalculateAffinity(t *testing.T) { policy Policy }{ { - name: "TopologyHint not set", - hp: []HintProvider{}, + name: "TopologyHint not set", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{}, expected: TopologyHint{ NUMANodeAffinity: NewTestBitMask(numaNodes...), Preferred: true, }, }, { - name: "HintProvider returns empty non-nil map[string][]TopologyHint", + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{}, @@ -134,7 +136,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "HintProvider returns -nil map[string][]TopologyHint from provider", + name: "HintProvider returns -nil map[string][]TopologyHint from provider", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -148,7 +151,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", + name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -162,7 +166,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", + name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -181,7 +186,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", + name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -200,7 +206,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, same mask, both preferred 1/2", + name: "Two providers, 1 hint each, same mask, both preferred 1/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -229,7 +236,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, same mask, both preferred 2/2", + name: "Two providers, 1 hint each, same mask, both preferred 2/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -258,7 +266,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", + name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -287,7 +296,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", + name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -316,7 +326,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, no common mask", + name: "Two providers, 1 hint each, no common mask", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -345,7 +356,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -374,7 +386,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -403,7 +416,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 no hints, 1 single hint preferred 1/2", + name: "Two providers, 1 no hints, 1 single hint preferred 1/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{}, &mockHintProvider{ @@ -423,7 +437,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 no hints, 1 single hint preferred 2/2", + name: "Two providers, 1 no hints, 1 single hint preferred 2/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{}, &mockHintProvider{ @@ -443,7 +458,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 with 2 hints, 1 with single hint matching 1/2", + name: "Two providers, 1 with 2 hints, 1 with single hint matching 1/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -476,7 +492,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 with 2 hints, 1 with single hint matching 2/2", + name: "Two providers, 1 with 2 hints, 1 with single hint matching 2/2", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -509,7 +526,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", + name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -542,7 +560,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Two providers, both with 2 hints, matching narrower preferred hint from both", + name: "Two providers, both with 2 hints, matching narrower preferred hint from both", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -579,7 +598,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Ensure less narrow preferred hints are chosen over narrower non-preferred hints", + name: "Ensure less narrow preferred hints are chosen over narrower non-preferred hints", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -620,7 +640,8 @@ func TestCalculateAffinity(t *testing.T) { }, }, { - name: "Multiple resources, same provider", + name: "Multiple resources, same provider", + policy: NewBestEffortPolicy(numaNodes), hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -727,9 +748,8 @@ func TestCalculateAffinity(t *testing.T) { mngr := manager{ policy: tc.policy, hintProviders: tc.hp, - numaNodes: numaNodes, } - actual := mngr.calculateAffinity(v1.Pod{}, v1.Container{}) + actual, _ := mngr.calculateAffinity(v1.Pod{}, v1.Container{}) if !actual.NUMANodeAffinity.IsEqual(tc.expected.NUMANodeAffinity) { t.Errorf("Expected NUMANodeAffinity in result to be %v, got %v", tc.expected.NUMANodeAffinity, actual.NUMANodeAffinity) } @@ -1107,7 +1127,6 @@ func TestAdmit(t *testing.T) { policy: tc.policy, podTopologyHints: make(map[string]map[string]TopologyHint), hintProviders: tc.hp, - numaNodes: numaNodes, } pod := &v1.Pod{ From 5f7db54d3c4cabed163e94291adb9cfd660dae1b Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Mon, 4 Nov 2019 13:19:52 +0100 Subject: [PATCH 07/11] Move function from top-level TopologyManager to best-effort policy This is in preparation for removing the special-case of the SingleNumaNode policy in mergeProvidersHints() in favor of a custom merging strategy with much less overhead. --- .../cm/topologymanager/policy_best_effort.go | 147 ++++++++++++++++++ .../cm/topologymanager/topology_manager.go | 145 ----------------- 2 files changed, 147 insertions(+), 145 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index 97c61f031da..caf48cb30fe 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort.go @@ -17,6 +17,8 @@ limitations under the License. package topologymanager import ( + "k8s.io/klog" + "k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask" "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) @@ -50,3 +52,148 @@ func (p *bestEffortPolicy) Merge(providersHints []map[string][]TopologyHint) (To admit := p.canAdmitPodResult(&hint) return hint, admit } + +// Iterate over all permutations of hints in 'allProviderHints [][]TopologyHint'. +// +// This procedure is implemented as a recursive function over the set of hints +// in 'allproviderHints[i]'. It applies the function 'callback' to each +// permutation as it is found. It is the equivalent of: +// +// for i := 0; i < len(providerHints[0]); i++ +// for j := 0; j < len(providerHints[1]); j++ +// for k := 0; k < len(providerHints[2]); k++ +// ... +// for z := 0; z < len(providerHints[-1]); z++ +// permutation := []TopologyHint{ +// providerHints[0][i], +// providerHints[1][j], +// providerHints[2][k], +// ... +// provideryHints[-1][z] +// } +// callback(permutation) +func iterateAllProviderTopologyHints(allProviderHints [][]TopologyHint, callback func([]TopologyHint)) { + // Internal helper function to accumulate the permutation before calling the callback. + var iterate func(i int, accum []TopologyHint) + iterate = func(i int, accum []TopologyHint) { + // Base case: we have looped through all providers and have a full permutation. + if i == len(allProviderHints) { + callback(accum) + return + } + + // Loop through all hints for provider 'i', and recurse to build the + // the permutation of this hint with all hints from providers 'i++'. + for j := range allProviderHints[i] { + iterate(i+1, append(accum, allProviderHints[i][j])) + } + } + iterate(0, []TopologyHint{}) +} + +// Merge the hints from all hint providers to find the best one. +func mergeProvidersHints(policy Policy, numaNodes []int, providersHints []map[string][]TopologyHint) TopologyHint { + // Set the default affinity as an any-numa affinity containing the list + // of NUMA Nodes available on this machine. + defaultAffinity, _ := bitmask.NewBitMask(numaNodes...) + + // Loop through all hint providers and save an accumulated list of the + // hints returned by each hint provider. If no hints are provided, assume + // that provider has no preference for topology-aware allocation. + var allProviderHints [][]TopologyHint + for _, hints := range providersHints { + // If hints is nil, insert a single, preferred any-numa hint into allProviderHints. + if len(hints) == 0 { + klog.Infof("[topologymanager] Hint Provider has no preference for NUMA affinity with any resource") + allProviderHints = append(allProviderHints, []TopologyHint{{nil, true}}) + continue + } + + // Otherwise, accumulate the hints for each resource type into allProviderHints. + for resource := range hints { + if hints[resource] == nil { + klog.Infof("[topologymanager] Hint Provider has no preference for NUMA affinity with resource '%s'", resource) + allProviderHints = append(allProviderHints, []TopologyHint{{nil, true}}) + continue + } + + if len(hints[resource]) == 0 { + klog.Infof("[topologymanager] Hint Provider has no possible NUMA affinities for resource '%s'", resource) + allProviderHints = append(allProviderHints, []TopologyHint{{nil, false}}) + continue + } + + allProviderHints = append(allProviderHints, hints[resource]) + } + } + + // Iterate over all permutations of hints in 'allProviderHints'. Merge the + // hints in each permutation by taking the bitwise-and of their affinity masks. + // Return the hint with the narrowest NUMANodeAffinity of all merged + // permutations that have at least one NUMA ID set. If no merged mask can be + // found that has at least one NUMA ID set, return the 'defaultAffinity'. + bestHint := TopologyHint{defaultAffinity, false} + iterateAllProviderTopologyHints(allProviderHints, func(permutation []TopologyHint) { + // Get the NUMANodeAffinity from each hint in the permutation and see if any + // of them encode unpreferred allocations. + preferred := true + var numaAffinities []bitmask.BitMask + for _, hint := range permutation { + if hint.NUMANodeAffinity == nil { + numaAffinities = append(numaAffinities, defaultAffinity) + } else { + numaAffinities = append(numaAffinities, hint.NUMANodeAffinity) + } + + if !hint.Preferred { + preferred = false + } + + // Special case PolicySingleNumaNode to only prefer hints where + // all providers have a single NUMA affinity set. + if policy != nil && policy.Name() == PolicySingleNumaNode && hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() > 1 { + preferred = false + } + } + + // Merge the affinities using a bitwise-and operation. + mergedAffinity, _ := bitmask.NewBitMask(numaNodes...) + mergedAffinity.And(numaAffinities...) + + // Build a mergedHintfrom the merged affinity mask, indicating if an + // preferred allocation was used to generate the affinity mask or not. + mergedHint := TopologyHint{mergedAffinity, preferred} + + // Only consider mergedHints that result in a NUMANodeAffinity > 0 to + // replace the current bestHint. + if mergedHint.NUMANodeAffinity.Count() == 0 { + return + } + + // If the current bestHint is non-preferred and the new mergedHint is + // preferred, always choose the preferred hint over the non-preferred one. + if mergedHint.Preferred && !bestHint.Preferred { + bestHint = mergedHint + return + } + + // If the current bestHint is preferred and the new mergedHint is + // non-preferred, never update bestHint, regardless of mergedHint's + // narowness. + if !mergedHint.Preferred && bestHint.Preferred { + return + } + + // If mergedHint and bestHint has the same preference, only consider + // mergedHints that have a narrower NUMANodeAffinity than the + // NUMANodeAffinity in the current bestHint. + if !mergedHint.NUMANodeAffinity.IsNarrowerThan(bestHint.NUMANodeAffinity) { + return + } + + // In all other cases, update bestHint to the current mergedHint + bestHint = mergedHint + }) + + return bestHint +} diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index 3600d3700db..4e452d71a00 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager.go @@ -176,151 +176,6 @@ func (m *manager) accumulateProvidersHints(pod v1.Pod, container v1.Container) ( return providersHints } -// Iterate over all permutations of hints in 'allProviderHints [][]TopologyHint'. -// -// This procedure is implemented as a recursive function over the set of hints -// in 'allproviderHints[i]'. It applies the function 'callback' to each -// permutation as it is found. It is the equivalent of: -// -// for i := 0; i < len(providerHints[0]); i++ -// for j := 0; j < len(providerHints[1]); j++ -// for k := 0; k < len(providerHints[2]); k++ -// ... -// for z := 0; z < len(providerHints[-1]); z++ -// permutation := []TopologyHint{ -// providerHints[0][i], -// providerHints[1][j], -// providerHints[2][k], -// ... -// provideryHints[-1][z] -// } -// callback(permutation) -func iterateAllProviderTopologyHints(allProviderHints [][]TopologyHint, callback func([]TopologyHint)) { - // Internal helper function to accumulate the permutation before calling the callback. - var iterate func(i int, accum []TopologyHint) - iterate = func(i int, accum []TopologyHint) { - // Base case: we have looped through all providers and have a full permutation. - if i == len(allProviderHints) { - callback(accum) - return - } - - // Loop through all hints for provider 'i', and recurse to build the - // the permutation of this hint with all hints from providers 'i++'. - for j := range allProviderHints[i] { - iterate(i+1, append(accum, allProviderHints[i][j])) - } - } - iterate(0, []TopologyHint{}) -} - -// Merge the hints from all hint providers to find the best one. -func mergeProvidersHints(policy Policy, numaNodes []int, providersHints []map[string][]TopologyHint) TopologyHint { - // Set the default affinity as an any-numa affinity containing the list - // of NUMA Nodes available on this machine. - defaultAffinity, _ := bitmask.NewBitMask(numaNodes...) - - // Loop through all hint providers and save an accumulated list of the - // hints returned by each hint provider. If no hints are provided, assume - // that provider has no preference for topology-aware allocation. - var allProviderHints [][]TopologyHint - for _, hints := range providersHints { - // If hints is nil, insert a single, preferred any-numa hint into allProviderHints. - if len(hints) == 0 { - klog.Infof("[topologymanager] Hint Provider has no preference for NUMA affinity with any resource") - allProviderHints = append(allProviderHints, []TopologyHint{{nil, true}}) - continue - } - - // Otherwise, accumulate the hints for each resource type into allProviderHints. - for resource := range hints { - if hints[resource] == nil { - klog.Infof("[topologymanager] Hint Provider has no preference for NUMA affinity with resource '%s'", resource) - allProviderHints = append(allProviderHints, []TopologyHint{{nil, true}}) - continue - } - - if len(hints[resource]) == 0 { - klog.Infof("[topologymanager] Hint Provider has no possible NUMA affinities for resource '%s'", resource) - allProviderHints = append(allProviderHints, []TopologyHint{{nil, false}}) - continue - } - - allProviderHints = append(allProviderHints, hints[resource]) - } - } - - // Iterate over all permutations of hints in 'allProviderHints'. Merge the - // hints in each permutation by taking the bitwise-and of their affinity masks. - // Return the hint with the narrowest NUMANodeAffinity of all merged - // permutations that have at least one NUMA ID set. If no merged mask can be - // found that has at least one NUMA ID set, return the 'defaultAffinity'. - bestHint := TopologyHint{defaultAffinity, false} - iterateAllProviderTopologyHints(allProviderHints, func(permutation []TopologyHint) { - // Get the NUMANodeAffinity from each hint in the permutation and see if any - // of them encode unpreferred allocations. - preferred := true - var numaAffinities []bitmask.BitMask - for _, hint := range permutation { - if hint.NUMANodeAffinity == nil { - numaAffinities = append(numaAffinities, defaultAffinity) - } else { - numaAffinities = append(numaAffinities, hint.NUMANodeAffinity) - } - - if !hint.Preferred { - preferred = false - } - - // Special case PolicySingleNumaNode to only prefer hints where - // all providers have a single NUMA affinity set. - if policy != nil && policy.Name() == PolicySingleNumaNode && hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() > 1 { - preferred = false - } - } - - // Merge the affinities using a bitwise-and operation. - mergedAffinity, _ := bitmask.NewBitMask(numaNodes...) - mergedAffinity.And(numaAffinities...) - - // Build a mergedHintfrom the merged affinity mask, indicating if an - // preferred allocation was used to generate the affinity mask or not. - mergedHint := TopologyHint{mergedAffinity, preferred} - - // Only consider mergedHints that result in a NUMANodeAffinity > 0 to - // replace the current bestHint. - if mergedHint.NUMANodeAffinity.Count() == 0 { - return - } - - // If the current bestHint is non-preferred and the new mergedHint is - // preferred, always choose the preferred hint over the non-preferred one. - if mergedHint.Preferred && !bestHint.Preferred { - bestHint = mergedHint - return - } - - // If the current bestHint is preferred and the new mergedHint is - // non-preferred, never update bestHint, regardless of mergedHint's - // narowness. - if !mergedHint.Preferred && bestHint.Preferred { - return - } - - // If mergedHint and bestHint has the same preference, only consider - // mergedHints that have a narrower NUMANodeAffinity than the - // NUMANodeAffinity in the current bestHint. - if !mergedHint.NUMANodeAffinity.IsNarrowerThan(bestHint.NUMANodeAffinity) { - return - } - - // In all other cases, update bestHint to the current mergedHint - bestHint = mergedHint - }) - - return bestHint -} - // 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) { providersHints := m.accumulateProvidersHints(pod, container) From dee22d1fbcd136d7ebbd24e150d9d59fb9b9f549 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Mon, 4 Nov 2019 13:27:31 +0100 Subject: [PATCH 08/11] Fix comments in TopologyManager --- pkg/kubelet/cm/topologymanager/policy.go | 4 ++-- pkg/kubelet/cm/topologymanager/policy_best_effort.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy.go b/pkg/kubelet/cm/topologymanager/policy.go index 05f46714eab..c93f3d135a0 100644 --- a/pkg/kubelet/cm/topologymanager/policy.go +++ b/pkg/kubelet/cm/topologymanager/policy.go @@ -20,9 +20,9 @@ import ( "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) -//Policy interface for Topology Manager Pod Admit Result +// Policy interface for Topology Manager Pod Admit Result type Policy interface { - //Returns Policy Name + // Returns Policy Name Name() string // Returns a merged TopologyHint based on input from hint providers // and a Pod Admit Handler Response based on hints and policy type diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index caf48cb30fe..2a0d9f1d927 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort.go @@ -69,7 +69,7 @@ func (p *bestEffortPolicy) Merge(providersHints []map[string][]TopologyHint) (To // providerHints[1][j], // providerHints[2][k], // ... -// provideryHints[-1][z] +// providerHints[-1][z] // } // callback(permutation) func iterateAllProviderTopologyHints(allProviderHints [][]TopologyHint, callback func([]TopologyHint)) { From d7d7bfcda0d35b7f6599292c3050e428df9ef828 Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Mon, 4 Nov 2019 15:11:52 +0100 Subject: [PATCH 09/11] Abstract TopologyManager Policy Merge() tests into their own function --- .../cm/topologymanager/topology_manager_test.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/topology_manager_test.go b/pkg/kubelet/cm/topologymanager/topology_manager_test.go index 49c4e1cea88..e5752885b15 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager_test.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager_test.go @@ -104,7 +104,7 @@ func TestGetAffinity(t *testing.T) { } } -func TestCalculateAffinity(t *testing.T) { +func TestPolicyMerge(t *testing.T) { numaNodes := []int{0, 1} tcases := []struct { @@ -745,11 +745,13 @@ func TestCalculateAffinity(t *testing.T) { } for _, tc := range tcases { - mngr := manager{ - policy: tc.policy, - hintProviders: tc.hp, + var providersHints []map[string][]TopologyHint + for _, provider := range tc.hp { + hints := provider.GetTopologyHints(v1.Pod{}, v1.Container{}) + providersHints = append(providersHints, hints) } - actual, _ := mngr.calculateAffinity(v1.Pod{}, v1.Container{}) + + actual, _ := tc.policy.Merge(providersHints) if !actual.NUMANodeAffinity.IsEqual(tc.expected.NUMANodeAffinity) { t.Errorf("Expected NUMANodeAffinity in result to be %v, got %v", tc.expected.NUMANodeAffinity, actual.NUMANodeAffinity) } From 7ea1fc9be4f927f465594e1fdf484b99ae30b198 Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Mon, 4 Nov 2019 15:34:18 +0100 Subject: [PATCH 10/11] Move TopologyManager TestPolicyMerge() to shared test file --- pkg/kubelet/cm/topologymanager/BUILD | 1 + pkg/kubelet/cm/topologymanager/policy_test.go | 681 ++++++++++++++++++ .../topologymanager/topology_manager_test.go | 657 ----------------- 3 files changed, 682 insertions(+), 657 deletions(-) create mode 100644 pkg/kubelet/cm/topologymanager/policy_test.go diff --git a/pkg/kubelet/cm/topologymanager/BUILD b/pkg/kubelet/cm/topologymanager/BUILD index c0dd4d1249e..5d15446fd2e 100644 --- a/pkg/kubelet/cm/topologymanager/BUILD +++ b/pkg/kubelet/cm/topologymanager/BUILD @@ -47,6 +47,7 @@ go_test( "policy_none_test.go", "policy_restricted_test.go", "policy_single_numa_node_test.go", + "policy_test.go", "topology_manager_test.go", ], embed = [":go_default_library"], diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go new file mode 100644 index 00000000000..2d5da65ad57 --- /dev/null +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -0,0 +1,681 @@ +/* +Copyright 2019 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 topologymanager + +import ( + "testing" + + "k8s.io/api/core/v1" +) + +func TestPolicyMerge(t *testing.T) { + numaNodes := []int{0, 1} + + tcases := []struct { + name string + hp []HintProvider + expected TopologyHint + policy Policy + }{ + { + name: "TopologyHint not set", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{}, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, + { + name: "HintProvider returns -nil map[string][]TopologyHint from provider", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": nil, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": {}, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: false, + }, + }, + { + name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, + { + name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: false, + }, + }, + { + name: "Two providers, 1 hint each, same mask, both preferred 1/2", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + { + name: "Two providers, 1 hint each, same mask, both preferred 2/2", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + { + name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + { + name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + { + name: "Two providers, 1 hint each, no common mask", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: false, + }, + }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(0), + Preferred: false, + }, + }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(1), + Preferred: false, + }, + }, + { + name: "Two providers, 1 no hints, 1 single hint preferred 1/2", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{}, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + { + name: "Two providers, 1 no hints, 1 single hint preferred 2/2", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{}, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + { + name: "Two providers, 1 with 2 hints, 1 with single hint matching 1/2", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + { + name: "Two providers, 1 with 2 hints, 1 with single hint matching 2/2", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + { + name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(0), + Preferred: false, + }, + }, + { + name: "Two providers, both with 2 hints, matching narrower preferred hint from both", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + { + name: "Ensure less narrow preferred hints are chosen over narrower non-preferred hints", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: false, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + { + name: "Multiple resources, same provider", + policy: NewBestEffortPolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: false, + }, + }, + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + { + name: "Special cased PolicySingleNumaNode for single NUMA hint generation", + policy: NewSingleNumaNodePolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: true, + }, + }, + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(0), + Preferred: false, + }, + }, + { + name: "Special cased PolicySingleNumaNode with one no-preference provider", + policy: NewSingleNumaNodePolicy(numaNodes), + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: false, + }, + }, + }, + }, + &mockHintProvider{ + nil, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + } + + for _, tc := range tcases { + var providersHints []map[string][]TopologyHint + for _, provider := range tc.hp { + hints := provider.GetTopologyHints(v1.Pod{}, v1.Container{}) + providersHints = append(providersHints, hints) + } + + actual, _ := tc.policy.Merge(providersHints) + if !actual.NUMANodeAffinity.IsEqual(tc.expected.NUMANodeAffinity) { + t.Errorf("Expected NUMANodeAffinity in result to be %v, got %v", tc.expected.NUMANodeAffinity, actual.NUMANodeAffinity) + } + if actual.Preferred != tc.expected.Preferred { + t.Errorf("Expected Affinity preference in result to be %v, got %v", tc.expected.Preferred, actual.Preferred) + } + } +} + diff --git a/pkg/kubelet/cm/topologymanager/topology_manager_test.go b/pkg/kubelet/cm/topologymanager/topology_manager_test.go index e5752885b15..9fc760413da 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager_test.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager_test.go @@ -104,663 +104,6 @@ func TestGetAffinity(t *testing.T) { } } -func TestPolicyMerge(t *testing.T) { - numaNodes := []int{0, 1} - - tcases := []struct { - name string - hp []HintProvider - expected TopologyHint - policy Policy - }{ - { - name: "TopologyHint not set", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{}, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, - { - name: "HintProvider returns empty non-nil map[string][]TopologyHint", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{}, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, - { - name: "HintProvider returns -nil map[string][]TopologyHint from provider", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": nil, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, - { - name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": {}, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: false, - }, - }, - { - name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": { - { - NUMANodeAffinity: nil, - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, - { - name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": { - { - NUMANodeAffinity: nil, - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: false, - }, - }, - { - name: "Two providers, 1 hint each, same mask, both preferred 1/2", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - { - name: "Two providers, 1 hint each, same mask, both preferred 2/2", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - { - name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - { - name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - { - name: "Two providers, 1 hint each, no common mask", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: false, - }, - }, - { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), - Preferred: false, - }, - }, - { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(1), - Preferred: false, - }, - }, - { - name: "Two providers, 1 no hints, 1 single hint preferred 1/2", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{}, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - { - name: "Two providers, 1 no hints, 1 single hint preferred 2/2", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{}, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - { - name: "Two providers, 1 with 2 hints, 1 with single hint matching 1/2", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - { - name: "Two providers, 1 with 2 hints, 1 with single hint matching 2/2", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - { - name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), - Preferred: false, - }, - }, - { - name: "Two providers, both with 2 hints, matching narrower preferred hint from both", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - { - name: "Ensure less narrow preferred hints are chosen over narrower non-preferred hints", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: false, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - { - name: "Multiple resources, same provider", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: false, - }, - }, - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - { - name: "Special cased PolicySingleNumaNode for single NUMA hint generation", - policy: NewSingleNumaNodePolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: true, - }, - }, - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), - Preferred: false, - }, - }, - { - name: "Special cased PolicySingleNumaNode with one no-preference provider", - policy: NewSingleNumaNodePolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: false, - }, - }, - }, - }, - &mockHintProvider{ - nil, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - } - - for _, tc := range tcases { - var providersHints []map[string][]TopologyHint - for _, provider := range tc.hp { - hints := provider.GetTopologyHints(v1.Pod{}, v1.Container{}) - providersHints = append(providersHints, hints) - } - - actual, _ := tc.policy.Merge(providersHints) - if !actual.NUMANodeAffinity.IsEqual(tc.expected.NUMANodeAffinity) { - t.Errorf("Expected NUMANodeAffinity in result to be %v, got %v", tc.expected.NUMANodeAffinity, actual.NUMANodeAffinity) - } - if actual.Preferred != tc.expected.Preferred { - t.Errorf("Expected Affinity preference in result to be %v, got %v", tc.expected.Preferred, actual.Preferred) - } - } -} - func TestAddContainer(t *testing.T) { testCases := []struct { name string From b5f52e6072fa142bf8f144b4d36178fee6861b9b Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Mon, 4 Nov 2019 16:27:20 +0100 Subject: [PATCH 11/11] Modularize TopologyManager policy Merge() tests These changes make it so that a set of common test cases can be used for all merge strategies, with specific test cases being able to be specified on a policy-by-policy basis. --- .../policy_best_effort_test.go | 10 + .../topologymanager/policy_restricted_test.go | 10 + .../policy_single_numa_node_test.go | 10 + pkg/kubelet/cm/topologymanager/policy_test.go | 217 ++++++++---------- 4 files changed, 132 insertions(+), 115 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go index 1459a823bc3..2ae7de0ecd8 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go @@ -48,3 +48,13 @@ func TestPolicyBestEffortCanAdmitPodResult(t *testing.T) { } } } + +func TestBestEffortPolicyMerge(t *testing.T) { + numaNodes := []int{0, 1} + policy := NewBestEffortPolicy(numaNodes) + + tcases := commonPolicyMergeTestCases(numaNodes) + tcases = append(tcases, policy.(*bestEffortPolicy).mergeTestCases(numaNodes)...) + + testPolicyMerge(policy, tcases, t) +} diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go index 11119a0400c..f9cf840708c 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go @@ -57,3 +57,13 @@ func TestPolicyRestrictedCanAdmitPodResult(t *testing.T) { } } } + +func TestRestrictedPolicyMerge(t *testing.T) { + numaNodes := []int{0, 1} + policy := NewRestrictedPolicy(numaNodes) + + tcases := commonPolicyMergeTestCases(numaNodes) + tcases = append(tcases, policy.(*restrictedPolicy).mergeTestCases(numaNodes)...) + + testPolicyMerge(policy, tcases, t) +} 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 cdc895295fc..a85862ba916 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go @@ -52,3 +52,13 @@ func TestPolicySingleNumaNodeCanAdmitPodResult(t *testing.T) { } } } + +func TestSingleNumaNodePolicyMerge(t *testing.T) { + numaNodes := []int{0, 1} + policy := NewSingleNumaNodePolicy(numaNodes) + + tcases := commonPolicyMergeTestCases(numaNodes) + tcases = append(tcases, policy.(*singleNumaNodePolicy).mergeTestCases(numaNodes)...) + + testPolicyMerge(policy, tcases, t) +} diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index 2d5da65ad57..5e8a61b6394 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -22,27 +22,24 @@ import ( "k8s.io/api/core/v1" ) -func TestPolicyMerge(t *testing.T) { - numaNodes := []int{0, 1} +type policyMergeTestCase struct { + name string + hp []HintProvider + expected TopologyHint +} - tcases := []struct { - name string - hp []HintProvider - expected TopologyHint - policy Policy - }{ +func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { + return []policyMergeTestCase{ { - name: "TopologyHint not set", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{}, + name: "TopologyHint not set", + hp: []HintProvider{}, expected: TopologyHint{ NUMANodeAffinity: NewTestBitMask(numaNodes...), Preferred: true, }, }, { - name: "HintProvider returns empty non-nil map[string][]TopologyHint", - policy: NewBestEffortPolicy(numaNodes), + name: "HintProvider returns empty non-nil map[string][]TopologyHint", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{}, @@ -54,8 +51,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "HintProvider returns -nil map[string][]TopologyHint from provider", - policy: NewBestEffortPolicy(numaNodes), + name: "HintProvider returns -nil map[string][]TopologyHint from provider", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -69,8 +65,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", - policy: NewBestEffortPolicy(numaNodes), + name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -84,8 +79,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", - policy: NewBestEffortPolicy(numaNodes), + name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -104,8 +98,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", - policy: NewBestEffortPolicy(numaNodes), + name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -124,8 +117,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, same mask, both preferred 1/2", - policy: NewBestEffortPolicy(numaNodes), + name: "Two providers, 1 hint each, same mask, both preferred 1/2", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -154,8 +146,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, same mask, both preferred 2/2", - policy: NewBestEffortPolicy(numaNodes), + name: "Two providers, 1 hint each, same mask, both preferred 2/2", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -184,68 +175,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), - Preferred: true, - }, - }, - { - name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", - policy: NewBestEffortPolicy(numaNodes), - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource1": { - { - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - }, - }, - &mockHintProvider{ - map[string][]TopologyHint{ - "resource2": { - { - NUMANodeAffinity: NewTestBitMask(0, 1), - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - { - name: "Two providers, 1 hint each, no common mask", - policy: NewBestEffortPolicy(numaNodes), + name: "Two providers, 1 hint each, no common mask", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -274,8 +204,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", - policy: NewBestEffortPolicy(numaNodes), + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -304,8 +233,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", - policy: NewBestEffortPolicy(numaNodes), + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -334,8 +262,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Two providers, 1 no hints, 1 single hint preferred 1/2", - policy: NewBestEffortPolicy(numaNodes), + name: "Two providers, 1 no hints, 1 single hint preferred 1/2", hp: []HintProvider{ &mockHintProvider{}, &mockHintProvider{ @@ -355,8 +282,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Two providers, 1 no hints, 1 single hint preferred 2/2", - policy: NewBestEffortPolicy(numaNodes), + name: "Two providers, 1 no hints, 1 single hint preferred 2/2", hp: []HintProvider{ &mockHintProvider{}, &mockHintProvider{ @@ -376,8 +302,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Two providers, 1 with 2 hints, 1 with single hint matching 1/2", - policy: NewBestEffortPolicy(numaNodes), + name: "Two providers, 1 with 2 hints, 1 with single hint matching 1/2", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -410,8 +335,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Two providers, 1 with 2 hints, 1 with single hint matching 2/2", - policy: NewBestEffortPolicy(numaNodes), + name: "Two providers, 1 with 2 hints, 1 with single hint matching 2/2", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -444,8 +368,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", - policy: NewBestEffortPolicy(numaNodes), + name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -478,8 +401,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Two providers, both with 2 hints, matching narrower preferred hint from both", - policy: NewBestEffortPolicy(numaNodes), + name: "Two providers, both with 2 hints, matching narrower preferred hint from both", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -516,8 +438,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Ensure less narrow preferred hints are chosen over narrower non-preferred hints", - policy: NewBestEffortPolicy(numaNodes), + name: "Ensure less narrow preferred hints are chosen over narrower non-preferred hints", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -558,8 +479,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Multiple resources, same provider", - policy: NewBestEffortPolicy(numaNodes), + name: "Multiple resources, same provider", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -595,9 +515,76 @@ func TestPolicyMerge(t *testing.T) { Preferred: true, }, }, + } +} + +func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase { + return []policyMergeTestCase{ { - name: "Special cased PolicySingleNumaNode for single NUMA hint generation", - policy: NewSingleNumaNodePolicy(numaNodes), + name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(0), + Preferred: true, + }, + }, + { + name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": { + { + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": { + { + NUMANodeAffinity: NewTestBitMask(0, 1), + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + } +} + +func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase { + return []policyMergeTestCase{ + { + name: "Single NUMA hint generation", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -630,8 +617,7 @@ func TestPolicyMerge(t *testing.T) { }, }, { - name: "Special cased PolicySingleNumaNode with one no-preference provider", - policy: NewSingleNumaNodePolicy(numaNodes), + name: "One no-preference provider", hp: []HintProvider{ &mockHintProvider{ map[string][]TopologyHint{ @@ -661,7 +647,9 @@ func TestPolicyMerge(t *testing.T) { }, }, } +} +func testPolicyMerge(policy Policy, tcases []policyMergeTestCase, t *testing.T) { for _, tc := range tcases { var providersHints []map[string][]TopologyHint for _, provider := range tc.hp { @@ -669,13 +657,12 @@ func TestPolicyMerge(t *testing.T) { providersHints = append(providersHints, hints) } - actual, _ := tc.policy.Merge(providersHints) + actual, _ := policy.Merge(providersHints) if !actual.NUMANodeAffinity.IsEqual(tc.expected.NUMANodeAffinity) { - t.Errorf("Expected NUMANodeAffinity in result to be %v, got %v", tc.expected.NUMANodeAffinity, actual.NUMANodeAffinity) + t.Errorf("%v: Expected NUMANodeAffinity in result to be %v, got %v", tc.name, tc.expected.NUMANodeAffinity, actual.NUMANodeAffinity) } if actual.Preferred != tc.expected.Preferred { - t.Errorf("Expected Affinity preference in result to be %v, got %v", tc.expected.Preferred, actual.Preferred) + t.Errorf("%v: Expected Affinity preference in result to be %v, got %v", tc.name, tc.expected.Preferred, actual.Preferred) } } } -