From cf8b098dda5212cc9de9b84cd5b6522e48120148 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Thu, 5 Dec 2019 04:15:08 +0000 Subject: [PATCH 01/33] Refactor policy-best-effort - Modularize code with mergePermutation method --- .../cm/topologymanager/policy_best_effort.go | 65 +++++++++++-------- 1 file changed, 38 insertions(+), 27 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index 2a0d9f1d927..6379daa81a9 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort.go @@ -91,6 +91,43 @@ func iterateAllProviderTopologyHints(allProviderHints [][]TopologyHint, callback iterate(0, []TopologyHint{}) } +// Merge a TopologyHints permutation to a single hint by performing a bitwise-AND +// of their affinity masks. The hint shall be preferred if all hits in the permutation +// are preferred. +func mergePermutation(policy Policy, numaNodes []int, permutation []TopologyHint) TopologyHint { + // Get the NUMANodeAffinity from each hint in the permutation and see if any + // of them encode unpreferred allocations. + defaultAffinity, _ := bitmask.NewBitMask(numaNodes...) + preferred := true + var numaAffinities []bitmask.BitMask + for _, hint := range permutation { + // Only consider hints that have an actual NUMANodeAffinity set. + 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 mergedHint from the merged affinity mask, indicating if an + // preferred allocation was used to generate the affinity mask or not. + return TopologyHint{mergedAffinity, preferred} +} + // 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 @@ -136,33 +173,7 @@ func mergeProvidersHints(policy Policy, numaNodes []int, providersHints []map[st 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} + mergedHint := mergePermutation(policy, numaNodes, permutation) // Only consider mergedHints that result in a NUMANodeAffinity > 0 to // replace the current bestHint. From dc36924c373f9ca54ebee6dbc4a264058646c6ea Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Thu, 5 Dec 2019 06:15:20 +0000 Subject: [PATCH 02/33] Update policy_none removing canAdmitPodResult Update unit tests for none_policy Add Name test for policy_restricted --- pkg/kubelet/cm/topologymanager/policy_none.go | 8 +--- .../cm/topologymanager/policy_none_test.go | 47 +++++++++++++------ .../topologymanager/policy_restricted_test.go | 20 +++++++- 3 files changed, 52 insertions(+), 23 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_none.go b/pkg/kubelet/cm/topologymanager/policy_none.go index 7e5010a765d..2b79fc0c4ef 100644 --- a/pkg/kubelet/cm/topologymanager/policy_none.go +++ b/pkg/kubelet/cm/topologymanager/policy_none.go @@ -36,12 +36,6 @@ func (p *nonePolicy) Name() string { return PolicyNone } -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) + return TopologyHint{}, lifecycle.PodAdmitResult{Admit: true} } diff --git a/pkg/kubelet/cm/topologymanager/policy_none_test.go b/pkg/kubelet/cm/topologymanager/policy_none_test.go index cafcaf044b6..e65d9317467 100644 --- a/pkg/kubelet/cm/topologymanager/policy_none_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_none_test.go @@ -17,10 +17,11 @@ limitations under the License. package topologymanager import ( + "k8s.io/kubernetes/pkg/kubelet/lifecycle" "testing" ) -func TestName(t *testing.T) { +func TestPolicyNoneName(t *testing.T) { tcases := []struct { name string expected string @@ -38,30 +39,46 @@ func TestName(t *testing.T) { } } -func TestPolicyNoneCanAdmitPodResult(t *testing.T) { +func TestPolicyNoneMerge(t *testing.T) { tcases := []struct { - name string - hint TopologyHint - expected bool + name string + providersHints []map[string][]TopologyHint + expectedHint TopologyHint + expectedAdmit lifecycle.PodAdmitResult }{ { - name: "Preferred is set to false in topology hints", - hint: TopologyHint{nil, false}, - expected: true, + name: "merged empty providers hints", + providersHints: []map[string][]TopologyHint{}, + expectedHint: TopologyHint{}, + expectedAdmit: lifecycle.PodAdmitResult{Admit: true}, }, { - name: "Preferred is set to true in topology hints", - hint: TopologyHint{nil, true}, - expected: true, + name: "merge with a single provider with a single preferred resource", + providersHints: []map[string][]TopologyHint{ + { + "resource": {{NUMANodeAffinity: NewTestBitMask(0, 1), Preferred: true}}, + }, + }, + expectedHint: TopologyHint{}, + expectedAdmit: lifecycle.PodAdmitResult{Admit: true}, + }, + { + name: "merge with a single provider with a single non-preferred resource", + providersHints: []map[string][]TopologyHint{ + { + "resource": {{NUMANodeAffinity: NewTestBitMask(0, 1), Preferred: false}}, + }, + }, + expectedHint: TopologyHint{}, + expectedAdmit: lifecycle.PodAdmitResult{Admit: true}, }, } for _, tc := range tcases { policy := NewNonePolicy() - result := policy.(*nonePolicy).canAdmitPodResult(&tc.hint) - - if result.Admit != tc.expected { - t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit) + result, admit := policy.Merge(tc.providersHints) + if !result.IsEqual(tc.expectedHint) || admit.Admit != tc.expectedAdmit.Admit { + t.Errorf("Test Case: %s: Expected merge hint to be %v, got %v", tc.name, tc.expectedHint, result) } } } diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go index f9cf840708c..d9463ea08a3 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go @@ -20,6 +20,24 @@ import ( "testing" ) +func TestPolicyRestrictedName(t *testing.T) { + tcases := []struct { + name string + expected string + }{ + { + name: "New Restricted Policy", + expected: "restricted", + }, + } + for _, tc := range tcases { + policy := NewRestrictedPolicy([]int{0, 1}) + if policy.Name() != tc.expected { + t.Errorf("Expected Policy Name to be %s, got %s", tc.expected, policy.Name()) + } + } +} + func TestPolicyRestrictedCanAdmitPodResult(t *testing.T) { tcases := []struct { name string @@ -58,7 +76,7 @@ func TestPolicyRestrictedCanAdmitPodResult(t *testing.T) { } } -func TestRestrictedPolicyMerge(t *testing.T) { +func TestPolicyRestrictedMerge(t *testing.T) { numaNodes := []int{0, 1} policy := NewRestrictedPolicy(numaNodes) From eda15215621f450a9a79b411217dcd15b0cb6581 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Thu, 5 Dec 2019 08:10:27 +0000 Subject: [PATCH 03/33] Make mergeProviderHints policy-specific: - Remove need to pass policy and numaNodes as arguments - Remove PolicySingleNUMANode special case check in policy_best_effort - Add mergeProviderHints base to policy_single_numa_node for upcoming commit --- .../cm/topologymanager/policy_best_effort.go | 25 +++++++------------ .../cm/topologymanager/policy_restricted.go | 2 +- .../policy_single_numa_node.go | 12 ++++++++- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index 6379daa81a9..26c7163bd83 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort.go @@ -48,7 +48,7 @@ func (p *bestEffortPolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAd } func (p *bestEffortPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { - hint := mergeProvidersHints(p, p.numaNodes, providersHints) + hint := p.mergeProvidersHints(providersHints) admit := p.canAdmitPodResult(&hint) return hint, admit } @@ -72,7 +72,7 @@ func (p *bestEffortPolicy) Merge(providersHints []map[string][]TopologyHint) (To // providerHints[-1][z] // } // callback(permutation) -func iterateAllProviderTopologyHints(allProviderHints [][]TopologyHint, callback func([]TopologyHint)) { +func (p *bestEffortPolicy) 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) { @@ -94,10 +94,10 @@ func iterateAllProviderTopologyHints(allProviderHints [][]TopologyHint, callback // Merge a TopologyHints permutation to a single hint by performing a bitwise-AND // of their affinity masks. The hint shall be preferred if all hits in the permutation // are preferred. -func mergePermutation(policy Policy, numaNodes []int, permutation []TopologyHint) TopologyHint { +func (p *bestEffortPolicy) mergePermutation(permutation []TopologyHint) TopologyHint { // Get the NUMANodeAffinity from each hint in the permutation and see if any // of them encode unpreferred allocations. - defaultAffinity, _ := bitmask.NewBitMask(numaNodes...) + defaultAffinity, _ := bitmask.NewBitMask(p.numaNodes...) preferred := true var numaAffinities []bitmask.BitMask for _, hint := range permutation { @@ -111,17 +111,10 @@ func mergePermutation(policy Policy, numaNodes []int, permutation []TopologyHint 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, _ := bitmask.NewBitMask(p.numaNodes...) mergedAffinity.And(numaAffinities...) // Build a mergedHint from the merged affinity mask, indicating if an // preferred allocation was used to generate the affinity mask or not. @@ -129,10 +122,10 @@ func mergePermutation(policy Policy, numaNodes []int, permutation []TopologyHint } // Merge the hints from all hint providers to find the best one. -func mergeProvidersHints(policy Policy, numaNodes []int, providersHints []map[string][]TopologyHint) TopologyHint { +func (p *bestEffortPolicy) 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(numaNodes...) + defaultAffinity, _ := bitmask.NewBitMask(p.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 @@ -170,10 +163,10 @@ func mergeProvidersHints(policy Policy, numaNodes []int, providersHints []map[st // 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) { + p.iterateAllProviderTopologyHints(allProviderHints, func(permutation []TopologyHint) { // Get the NUMANodeAffinity from each hint in the permutation and see if any // of them encode unpreferred allocations. - mergedHint := mergePermutation(policy, numaNodes, permutation) + mergedHint := p.mergePermutation(permutation) // Only consider mergedHints that result in a NUMANodeAffinity > 0 to // replace the current bestHint. diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted.go b/pkg/kubelet/cm/topologymanager/policy_restricted.go index cc522980c0e..34e3fb3c247 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted.go +++ b/pkg/kubelet/cm/topologymanager/policy_restricted.go @@ -52,7 +52,7 @@ func (p *restrictedPolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.PodAd } func (p *restrictedPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { - hint := mergeProvidersHints(p, p.numaNodes, providersHints) + hint := p.mergeProvidersHints(providersHints) admit := p.canAdmitPodResult(&hint) return hint, admit } diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index 6f29e1c27b7..41b02c3ee8d 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -17,6 +17,7 @@ limitations under the License. package topologymanager import ( + "k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask" "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) @@ -52,8 +53,17 @@ func (p *singleNumaNodePolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.P } } +func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][]TopologyHint) TopologyHint { + // Set the default hint to return from this function as an any-NUMANode + // affinity with an unpreferred allocation. This will only be returned if + // no better hint can be found when merging hints from each hint provider. + defaultAffinity, _ := bitmask.NewBitMask(p.numaNodes...) + defaultHint := TopologyHint{defaultAffinity, false} + return defaultHint +} + func (p *singleNumaNodePolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { - hint := mergeProvidersHints(p, p.numaNodes, providersHints) + hint := p.mergeProvidersHints(providersHints) admit := p.canAdmitPodResult(&hint) return hint, admit } From 5ce2ea277342a51e06cfd27894ff2b466f1c0a96 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Thu, 5 Dec 2019 08:31:29 +0000 Subject: [PATCH 04/33] Return defaultAffinity from PolicyBestEffort: Now that PolicySingleNUMANode is not considered here, return defaultAffinity as was the original case before previous bug fix --- pkg/kubelet/cm/topologymanager/policy_best_effort.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index 26c7163bd83..5b1528c9740 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort.go @@ -135,7 +135,7 @@ func (p *bestEffortPolicy) mergeProvidersHints(providersHints []map[string][]Top // 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}}) + allProviderHints = append(allProviderHints, []TopologyHint{{defaultAffinity, true}}) continue } @@ -143,13 +143,13 @@ func (p *bestEffortPolicy) mergeProvidersHints(providersHints []map[string][]Top 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}}) + allProviderHints = append(allProviderHints, []TopologyHint{{defaultAffinity, 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}}) + allProviderHints = append(allProviderHints, []TopologyHint{{defaultAffinity, false}}) continue } From 2825a7be1a665c8276d6b7e74440c7b35e3145d4 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Fri, 6 Dec 2019 06:15:22 +0000 Subject: [PATCH 05/33] Add new functionality for single-numa-node policy: Explanation taken from original commit: - Change the current method of finding the best hint. Instead of going over all permutations, sort the hints and find the narrowest hint common to all resources. - Break out early when merging to a preferred hint is not possible --- .../policy_single_numa_node.go | 124 ++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index 41b02c3ee8d..815e3a52a17 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -17,8 +17,10 @@ limitations under the License. package topologymanager import ( + "k8s.io/klog" "k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask" "k8s.io/kubernetes/pkg/kubelet/lifecycle" + "sort" ) type singleNumaNodePolicy struct { @@ -53,12 +55,134 @@ func (p *singleNumaNodePolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.P } } +// Get the narrowest hint aligned to one NUMA node +// assumption: allResourcesHints are not empty, all hints have NUMAAffinity != nil, +// all hints have exactly one NUMA node set in NUMANodeAffinity +func (p *singleNumaNodePolicy) getHintMatch(allResourcesHints [][]TopologyHint) (bool, TopologyHint) { + // Sort all resources hints + for _, resource := range allResourcesHints { + sort.Slice(resource, func(i, j int) bool { + if resource[i].NUMANodeAffinity.GetBits()[0] < resource[j].NUMANodeAffinity.GetBits()[0] { + return true + } + return false + }) + + } + // find a match by searching a hint of one resource in the rest + var match TopologyHint + var foundMatch bool + + if len(allResourcesHints) == 1 { + match = allResourcesHints[0][0] + match.Preferred = true + return true, match + } + for _, candidate := range allResourcesHints[0] { + foundMatch = true + for _, hints := range allResourcesHints[1:] { + res := sort.Search(len(hints), func(i int) bool { + return hints[i].NUMANodeAffinity.GetBits()[0] >= candidate.NUMANodeAffinity.GetBits()[0] + }) + if res >= len(hints) || + !hints[res].NUMANodeAffinity.IsEqual(candidate.NUMANodeAffinity) { + // hint not found, move to next hint from allResourcesHints[0] + foundMatch = false + break + } + } + if foundMatch { + match = candidate + match.Preferred = true + break + } + } + return foundMatch, match +} + +// Return hints that have valid bitmasks with exactly one bit set +func (p *singleNumaNodePolicy) filterHints(allResourcesHints [][]TopologyHint) [][]TopologyHint { + var filteredResourcesHints [][]TopologyHint + if len(allResourcesHints) > 0 { + for _, oneResourceHints := range allResourcesHints { + var filtered []TopologyHint + if len(oneResourceHints) > 0 { + for _, hint := range oneResourceHints { + if hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() == 1 { + filtered = append(filtered, hint) + } + } + } + filteredResourcesHints = append(filteredResourcesHints, filtered) + } + } + return filteredResourcesHints +} + func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][]TopologyHint) TopologyHint { // Set the default hint to return from this function as an any-NUMANode // affinity with an unpreferred allocation. This will only be returned if // no better hint can be found when merging hints from each hint provider. defaultAffinity, _ := bitmask.NewBitMask(p.numaNodes...) defaultHint := TopologyHint{defaultAffinity, false} + + // Loop through all provider hints 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 allResourcesHints [][]TopologyHint + for _, hints := range providersHints { + if len(hints) == 0 { + klog.Infof("[topologymanager] Hint Provider has no preference for NUMA affinity with any resource, " + + "skipping.") + 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', skipping.", resource) + continue + } + + if len(hints[resource]) == 0 { + klog.Infof("[topologymanager] Hint Provider has no possible NUMA affinities for resource '%s'", + resource) + // return defaultHint which will fail pod admission + return defaultHint + } + klog.Infof("[topologymanager] TopologyHints for resource '%v': %v", resource, hints[resource]) + allResourcesHints = append(allResourcesHints, hints[resource]) + } + } + // In case allProviderHints length is zero it means that we have no + // preference for NUMA affinity. In that case update default hint preferred + // to true to allow scheduling. + if len(allResourcesHints) == 0 { + klog.Infof("[topologymanager] No preference for NUMA affinity from all providers") + defaultHint.Preferred = true + return defaultHint + } + + allResourcesHints = p.filterHints(allResourcesHints) + // if no hints or there is a resource with empty hints after filtering, then policy cannot be satisfied + if len(allResourcesHints) == 0 { + klog.Infof("[topologymanager] No hints that align to a single NUMA node.") + return defaultHint + } + for _, hints := range allResourcesHints { + if len(hints) == 0 { + klog.Infof("[topologymanager] No hints that align to a single NUMA node for resource.") + return defaultHint + } + } + + found, match := p.getHintMatch(allResourcesHints) + if found { + klog.Infof("[topologymanager] single-numa-node policy match: %v", match) + return match + } + klog.Infof("[topologymanager] single-numa-node no match: %v", defaultHint) return defaultHint } From f886d2a8320d2960c3380f4636941eef12f19387 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Fri, 6 Dec 2019 06:22:55 +0000 Subject: [PATCH 06/33] Update single-numa-node policy unit tests --- .../policy_single_numa_node_test.go | 272 +++++++++++++++++- 1 file changed, 271 insertions(+), 1 deletion(-) 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 a85862ba916..7847716f8fa 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go @@ -17,6 +17,7 @@ limitations under the License. package topologymanager import ( + "reflect" "testing" ) @@ -53,7 +54,276 @@ func TestPolicySingleNumaNodeCanAdmitPodResult(t *testing.T) { } } -func TestSingleNumaNodePolicyMerge(t *testing.T) { +func TestPolicySingleNumaNodeFilterHints(t *testing.T) { + tcases := []struct { + name string + allResources [][]TopologyHint + expectedResources [][]TopologyHint + }{ + { + name: "filter empty resources", + allResources: [][]TopologyHint{}, + expectedResources: [][]TopologyHint(nil), + }, + { + name: "filter hints with nil socket mask", + allResources: [][]TopologyHint{ + { + {NUMANodeAffinity: nil, Preferred: false}, + }, + { + {NUMANodeAffinity: nil, Preferred: true}, + }, + }, + expectedResources: [][]TopologyHint{ + []TopologyHint(nil), + []TopologyHint(nil), + }, + }, + { + name: "filter hints with nil socket mask", + allResources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: nil, Preferred: false}, + }, + { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: nil, Preferred: true}, + }, + }, + expectedResources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + }, + { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + }, + }, + }, + { + name: "filter hints with empty resource socket mask", + allResources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: nil, Preferred: false}, + }, + {}, + }, + expectedResources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + }, + []TopologyHint(nil), + }, + }, + { + name: "filter hints with wide sockemask", + allResources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1, 2), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(0, 1, 2), Preferred: false}, + {NUMANodeAffinity: nil, Preferred: false}, + }, + { + {NUMANodeAffinity: NewTestBitMask(1, 2), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(0, 1, 2), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(0, 2), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(3), Preferred: false}, + }, + { + {NUMANodeAffinity: NewTestBitMask(1, 2), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(0, 1, 2), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(0, 2), Preferred: false}, + }, + }, + expectedResources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + }, + { + {NUMANodeAffinity: NewTestBitMask(3), Preferred: false}, + }, + []TopologyHint(nil), + }, + }, + } + + numaNodes := []int{0, 1, 2, 3} + for _, tc := range tcases { + policy := NewSingleNumaNodePolicy(numaNodes) + actual := policy.(*singleNumaNodePolicy).filterHints(tc.allResources) + if !reflect.DeepEqual(tc.expectedResources, actual) { + t.Errorf("Test Case: %s", tc.name) + t.Errorf("Expected result to be %v, got %v", tc.expectedResources, actual) + } + } +} + +func TestPolicySingleNumaNodeGetHintMatch(t *testing.T) { + tcases := []struct { + name string + resources [][]TopologyHint + expectedFound bool + expectedMatch TopologyHint + }{ + { + name: "match single resource single hint", + resources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, + }, + }, + expectedFound: true, + expectedMatch: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(3), + Preferred: true, + }, + }, + { + name: "match single resource multiple hints (Selected hint preferred is true) 1/2", + resources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(5), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + }, + }, + expectedFound: true, + expectedMatch: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + { + name: "match single resource multiple hints (Selected hint preferred is false) 2/2", + resources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(5), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1), Preferred: false}, + }, + }, + expectedFound: true, + expectedMatch: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(1), + Preferred: true, + }, + }, + { + name: "match multiple resource single hint", + resources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + }, + { + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + }, + { + {NUMANodeAffinity: NewTestBitMask(2), Preferred: false}, + }, + }, + expectedFound: true, + expectedMatch: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(2), + Preferred: true, + }, + }, + { + name: "match multiple resource single hint no match", + resources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + }, + { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + }, + { + {NUMANodeAffinity: NewTestBitMask(2), Preferred: false}, + }, + }, + expectedFound: false, + expectedMatch: TopologyHint{}, + }, + { + name: "multiple resources no match", + resources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(4), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1), Preferred: false}, + }, + { + {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(5), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1), Preferred: false}, + }, + { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(5), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(4), Preferred: true}, + }, + }, + expectedFound: false, + expectedMatch: TopologyHint{}, + }, + { + name: "multiple resources with match", + resources: [][]TopologyHint{ + { + {NUMANodeAffinity: NewTestBitMask(3), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(4), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + }, + { + {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(5), Preferred: false}, + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1), Preferred: false}, + }, + { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(5), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(4), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, + }, + }, + expectedFound: true, + expectedMatch: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(3), + Preferred: true, + }, + }, + } + + numaNodes := []int{0, 1, 2, 3, 4, 5} + for _, tc := range tcases { + policy := NewSingleNumaNodePolicy(numaNodes) + found, match := policy.(*singleNumaNodePolicy).getHintMatch(tc.resources) + if found != tc.expectedFound { + t.Errorf("Test Case: %s", tc.name) + t.Errorf("Expected found to be %v, got %v", tc.expectedFound, found) + } + if found { + if !match.IsEqual(tc.expectedMatch) { + t.Errorf("Test Case: %s", tc.name) + t.Errorf("Expected match to be %v, got %v", tc.expectedMatch, match) + } + } + } +} + +func TestPolicySingleNumaNodeMerge(t *testing.T) { numaNodes := []int{0, 1} policy := NewSingleNumaNodePolicy(numaNodes) From 9f21f494930e7c5d375b89f9bed17176837a965e Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Fri, 6 Dec 2019 07:06:10 +0000 Subject: [PATCH 07/33] Additional unit tests for Topology Manager methods --- .../topologymanager/topology_manager_test.go | 247 ++++++++++++++++++ 1 file changed, 247 insertions(+) diff --git a/pkg/kubelet/cm/topologymanager/topology_manager_test.go b/pkg/kubelet/cm/topologymanager/topology_manager_test.go index 9fc760413da..cd5b4aaf56f 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager_test.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager_test.go @@ -104,6 +104,253 @@ func TestGetAffinity(t *testing.T) { } } +func TestAccumulateProvidersHints(t *testing.T) { + tcases := []struct { + name string + hp []HintProvider + expected []map[string][]TopologyHint + }{ + { + name: "TopologyHint not set", + hp: []HintProvider{}, + expected: nil, + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: []map[string][]TopologyHint{ + {}, + }, + }, + { + name: "HintProvider returns - nil map[string][]TopologyHint from provider", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": nil, + }, + }, + }, + expected: []map[string][]TopologyHint{ + { + "resource": nil, + }, + }, + }, + { + name: "2 HintProviders with 1 resource returns hints", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": {TopologyHint{}}, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource2": {TopologyHint{}}, + }, + }, + }, + expected: []map[string][]TopologyHint{ + { + "resource1": {TopologyHint{}}, + }, + { + "resource2": {TopologyHint{}}, + }, + }, + }, + { + name: "2 HintProviders 1 with 1 resource 1 with nil hints", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": {TopologyHint{}}, + }, + }, + &mockHintProvider{nil}, + }, + expected: []map[string][]TopologyHint{ + { + "resource1": {TopologyHint{}}, + }, + nil, + }, + }, + { + name: "2 HintProviders 1 with 1 resource 1 empty hints", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": {TopologyHint{}}, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: []map[string][]TopologyHint{ + { + "resource1": {TopologyHint{}}, + }, + {}, + }, + }, + { + name: "HintProvider with 2 resources returns hints", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource1": {TopologyHint{}}, + "resource2": {TopologyHint{}}, + }, + }, + }, + expected: []map[string][]TopologyHint{ + { + "resource1": {TopologyHint{}}, + "resource2": {TopologyHint{}}, + }, + }, + }, + } + + for _, tc := range tcases { + mngr := manager{ + hintProviders: tc.hp, + } + actual := mngr.accumulateProvidersHints(v1.Pod{}, v1.Container{}) + if !reflect.DeepEqual(actual, tc.expected) { + t.Errorf("Test Case %s: Expected NUMANodeAffinity in result to be %v, got %v", tc.name, tc.expected, actual) + } + } +} + +type mockPolicy struct { + nonePolicy + ph []map[string][]TopologyHint +} + +func (p *mockPolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { + p.ph = providersHints + return TopologyHint{}, lifecycle.PodAdmitResult{} +} + +func TestCalculateAffinity(t *testing.T) { + tcases := []struct { + name string + hp []HintProvider + expected []map[string][]TopologyHint + }{ + { + name: "No hint providers", + hp: []HintProvider{}, + expected: ([]map[string][]TopologyHint)(nil), + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: []map[string][]TopologyHint{ + {}, + }, + }, + { + name: "HintProvider returns -nil map[string][]TopologyHint from provider", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": nil, + }, + }, + }, + expected: []map[string][]TopologyHint{ + { + "resource": nil, + }, + }, + }, + { + name: "Assorted HintProviders", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource-1/A": { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(0, 1), Preferred: false}, + }, + "resource-1/B": { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1, 2), Preferred: false}, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource-2/A": { + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(3, 4), Preferred: false}, + }, + "resource-2/B": { + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(3, 4), Preferred: false}, + }, + }, + }, + &mockHintProvider{ + map[string][]TopologyHint{ + "resource-3": nil, + }, + }, + }, + expected: []map[string][]TopologyHint{ + { + "resource-1/A": { + {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(0, 1), Preferred: false}, + }, + "resource-1/B": { + {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(1, 2), Preferred: false}, + }, + }, + { + "resource-2/A": { + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(3, 4), Preferred: false}, + }, + "resource-2/B": { + {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, + {NUMANodeAffinity: NewTestBitMask(3, 4), Preferred: false}, + }, + }, + { + "resource-3": nil, + }, + }, + }, + } + + for _, tc := range tcases { + mngr := manager{} + mngr.policy = &mockPolicy{} + mngr.hintProviders = tc.hp + mngr.calculateAffinity(v1.Pod{}, v1.Container{}) + actual := mngr.policy.(*mockPolicy).ph + if !reflect.DeepEqual(tc.expected, actual) { + t.Errorf("Test Case: %s", tc.name) + t.Errorf("Expected result to be %v, got %v", tc.expected, actual) + } + } +} + func TestAddContainer(t *testing.T) { testCases := []struct { name string From 17d615bca2b22ea9a41595ce9e91613b2178a130 Mon Sep 17 00:00:00 2001 From: nolancon Date: Mon, 9 Dec 2019 08:22:47 +0000 Subject: [PATCH 08/33] Update filterHints: - Only append valid preferred-true hints to filtered - Return true if allResourceHints only consist of nil-affinity/preferred-true hints: {nil true}, update defaultHint preference accordingly. --- .../policy_single_numa_node.go | 34 +++++++++++++++---- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index 815e3a52a17..6c1184d1cd1 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -100,23 +100,34 @@ func (p *singleNumaNodePolicy) getHintMatch(allResourcesHints [][]TopologyHint) return foundMatch, match } -// Return hints that have valid bitmasks with exactly one bit set -func (p *singleNumaNodePolicy) filterHints(allResourcesHints [][]TopologyHint) [][]TopologyHint { +// Return hints that have valid bitmasks with exactly one bit set. Also return bool +// which indicates whether allResourceHints only consists of {nil true} hints. +func (p *singleNumaNodePolicy) filterHints(allResourcesHints [][]TopologyHint) ([][]TopologyHint, bool) { var filteredResourcesHints [][]TopologyHint + var noAffinityPreferredHints int + var totalHints int if len(allResourcesHints) > 0 { for _, oneResourceHints := range allResourcesHints { var filtered []TopologyHint if len(oneResourceHints) > 0 { for _, hint := range oneResourceHints { - if hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() == 1 { + totalHints++ + if hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() == 1 && hint.Preferred == true { filtered = append(filtered, hint) } + if hint.NUMANodeAffinity == nil && hint.Preferred == true { + noAffinityPreferredHints++ + } } } filteredResourcesHints = append(filteredResourcesHints, filtered) } } - return filteredResourcesHints + // Check if all resource hints only consist of nil-affinity/preferred hint: {nil true}. + if noAffinityPreferredHints == totalHints { + return filteredResourcesHints, true + } + return filteredResourcesHints, false } func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][]TopologyHint) TopologyHint { @@ -164,16 +175,25 @@ func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][ return defaultHint } - allResourcesHints = p.filterHints(allResourcesHints) - // if no hints or there is a resource with empty hints after filtering, then policy cannot be satisfied + var noAffinityPreferredOnly bool + allResourcesHints, noAffinityPreferredOnly = p.filterHints(allResourcesHints) + // If no hints, then policy cannot be satisfied if len(allResourcesHints) == 0 { klog.Infof("[topologymanager] No hints that align to a single NUMA node.") return defaultHint } + // If there is a resource with empty hints after filtering, then policy cannot be satisfied. + // In the event that the only hints that exist are {nil true} update default hint preferred + // to allow scheduling. for _, hints := range allResourcesHints { if len(hints) == 0 { klog.Infof("[topologymanager] No hints that align to a single NUMA node for resource.") - return defaultHint + if !noAffinityPreferredOnly { + return defaultHint + } else if noAffinityPreferredOnly { + defaultHint.Preferred = true + return defaultHint + } } } From b5ca4989e3a691e438696e7e63652817b4791867 Mon Sep 17 00:00:00 2001 From: nolancon Date: Mon, 9 Dec 2019 08:27:38 +0000 Subject: [PATCH 09/33] Update unit tests: - Update filterHints test to reflect changes in previous commit. - Some common test cases achieve differing expected results based on policy due to independent merge strategies. These cases are moved into individual policy based test functions. --- .../policy_single_numa_node_test.go | 76 ++++- pkg/kubelet/cm/topologymanager/policy_test.go | 275 ++++++++++++------ 2 files changed, 254 insertions(+), 97 deletions(-) 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 7847716f8fa..b0173e424f1 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go @@ -59,18 +59,45 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { name string allResources [][]TopologyHint expectedResources [][]TopologyHint + expectedExists bool }{ { name: "filter empty resources", allResources: [][]TopologyHint{}, expectedResources: [][]TopologyHint(nil), + expectedExists: false, }, { - name: "filter hints with nil socket mask", + name: "filter hints with nil socket mask, preferred true", + allResources: [][]TopologyHint{ + { + {NUMANodeAffinity: nil, Preferred: true}, + }, + }, + expectedResources: [][]TopologyHint{ + []TopologyHint(nil), + }, + expectedExists: true, + }, + + { + name: "filter hints with nil socket mask, preferred false", allResources: [][]TopologyHint{ { {NUMANodeAffinity: nil, Preferred: false}, }, + }, + expectedResources: [][]TopologyHint{ + []TopologyHint(nil), + }, + expectedExists: false, + }, + { + name: "filter hints with nil socket mask, preferred both true", + allResources: [][]TopologyHint{ + { + {NUMANodeAffinity: nil, Preferred: true}, + }, { {NUMANodeAffinity: nil, Preferred: true}, }, @@ -79,6 +106,40 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { []TopologyHint(nil), []TopologyHint(nil), }, + expectedExists: true, + }, + { + name: "filter hints with nil socket mask, preferred both false", + allResources: [][]TopologyHint{ + { + {NUMANodeAffinity: nil, Preferred: false}, + }, + { + {NUMANodeAffinity: nil, Preferred: false}, + }, + }, + expectedResources: [][]TopologyHint{ + []TopologyHint(nil), + []TopologyHint(nil), + }, + expectedExists: false, + }, + + { + name: "filter hints with nil socket mask, preferred true and false", + allResources: [][]TopologyHint{ + { + {NUMANodeAffinity: nil, Preferred: true}, + }, + { + {NUMANodeAffinity: nil, Preferred: false}, + }, + }, + expectedResources: [][]TopologyHint{ + []TopologyHint(nil), + []TopologyHint(nil), + }, + expectedExists: false, }, { name: "filter hints with nil socket mask", @@ -100,6 +161,7 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, }, }, + expectedExists: false, }, { name: "filter hints with empty resource socket mask", @@ -118,6 +180,7 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { }, []TopologyHint(nil), }, + expectedExists: false, }, { name: "filter hints with wide sockemask", @@ -146,22 +209,25 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, }, - { - {NUMANodeAffinity: NewTestBitMask(3), Preferred: false}, - }, + []TopologyHint(nil), []TopologyHint(nil), }, + expectedExists: false, }, } numaNodes := []int{0, 1, 2, 3} for _, tc := range tcases { policy := NewSingleNumaNodePolicy(numaNodes) - actual := policy.(*singleNumaNodePolicy).filterHints(tc.allResources) + actual, exists := policy.(*singleNumaNodePolicy).filterHints(tc.allResources) if !reflect.DeepEqual(tc.expectedResources, actual) { t.Errorf("Test Case: %s", tc.name) t.Errorf("Expected result to be %v, got %v", tc.expectedResources, actual) } + if !reflect.DeepEqual(tc.expectedResources, actual) { + t.Errorf("Test Case: %s", tc.name) + t.Errorf("Expected result to be %v, got %v", tc.expectedExists, exists) + } } } diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index 5e8a61b6394..9e32c6989b2 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -203,64 +203,6 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { Preferred: false, }, }, - { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", - 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", - 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", hp: []HintProvider{ @@ -367,39 +309,6 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { Preferred: true, }, }, - { - name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", - 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", hp: []HintProvider{ @@ -578,6 +487,97 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: true, }, }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", + 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", + 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 with 2 hints, 1 with single non-preferred hint matching", + 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, + }, + }, } } @@ -612,7 +612,7 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest }, }, expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), + NUMANodeAffinity: NewTestBitMask(numaNodes...), Preferred: false, }, }, @@ -646,6 +646,97 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest Preferred: true, }, }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", + 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(numaNodes...), + Preferred: false, + }, + }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", + 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(numaNodes...), + Preferred: false, + }, + }, + { + name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", + 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(numaNodes...), + Preferred: false, + }, + }, } } From e3d0c9397f0f8c337559077959d7d9e8c428e00c Mon Sep 17 00:00:00 2001 From: nolancon Date: Tue, 10 Dec 2019 05:05:26 +0000 Subject: [PATCH 10/33] Updates to single-numa-node policy: - Remove getHintMatch method. - Replace with simplified versions of mergePermutation and iterateAllProviderTopologyHints methods - as used in best-effort. - Remove getHintMatch unit tests. --- .../policy_single_numa_node.go | 113 +++++++------ .../policy_single_numa_node_test.go | 158 ------------------ 2 files changed, 66 insertions(+), 205 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index 6c1184d1cd1..37d95166d36 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -20,7 +20,6 @@ import ( "k8s.io/klog" "k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask" "k8s.io/kubernetes/pkg/kubelet/lifecycle" - "sort" ) type singleNumaNodePolicy struct { @@ -55,49 +54,59 @@ func (p *singleNumaNodePolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.P } } -// Get the narrowest hint aligned to one NUMA node -// assumption: allResourcesHints are not empty, all hints have NUMAAffinity != nil, -// all hints have exactly one NUMA node set in NUMANodeAffinity -func (p *singleNumaNodePolicy) getHintMatch(allResourcesHints [][]TopologyHint) (bool, TopologyHint) { - // Sort all resources hints - for _, resource := range allResourcesHints { - sort.Slice(resource, func(i, j int) bool { - if resource[i].NUMANodeAffinity.GetBits()[0] < resource[j].NUMANodeAffinity.GetBits()[0] { - return true - } - return false - }) - - } - // find a match by searching a hint of one resource in the rest - var match TopologyHint - var foundMatch bool - - if len(allResourcesHints) == 1 { - match = allResourcesHints[0][0] - match.Preferred = true - return true, match - } - for _, candidate := range allResourcesHints[0] { - foundMatch = true - for _, hints := range allResourcesHints[1:] { - res := sort.Search(len(hints), func(i int) bool { - return hints[i].NUMANodeAffinity.GetBits()[0] >= candidate.NUMANodeAffinity.GetBits()[0] - }) - if res >= len(hints) || - !hints[res].NUMANodeAffinity.IsEqual(candidate.NUMANodeAffinity) { - // hint not found, move to next hint from allResourcesHints[0] - foundMatch = false - break - } +// 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], +// ... +// providerHints[-1][z] +// } +// callback(permutation) +func (p *singleNumaNodePolicy) 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 } - if foundMatch { - match = candidate - match.Preferred = true - break + + // 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])) } } - return foundMatch, match + iterate(0, []TopologyHint{}) +} + +// Merge a TopologyHints permutation to a single hint by performing a bitwise-AND +// of their affinity masks. At this point, all hints have single numa node affinity +// and preferred-true. +func (p *singleNumaNodePolicy) mergePermutation(permutation []TopologyHint) TopologyHint { + preferred := true + var numaAffinities []bitmask.BitMask + for _, hint := range permutation { + numaAffinities = append(numaAffinities, hint.NUMANodeAffinity) + } + + // Merge the affinities using a bitwise-and operation. + mergedAffinity, _ := bitmask.NewBitMask(p.numaNodes...) + mergedAffinity.And(numaAffinities...) + // Build a mergedHint from the merged affinity mask. + return TopologyHint{mergedAffinity, preferred} } // Return hints that have valid bitmasks with exactly one bit set. Also return bool @@ -197,12 +206,22 @@ func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][ } } - found, match := p.getHintMatch(allResourcesHints) - if found { - klog.Infof("[topologymanager] single-numa-node policy match: %v", match) - return match - } - klog.Infof("[topologymanager] single-numa-node no match: %v", defaultHint) + p.iterateAllProviderTopologyHints(allResourcesHints, func(permutation []TopologyHint) { + mergedHint := p.mergePermutation(permutation) + // Only consider mergedHints that result in a NUMANodeAffinity == 1 to + // replace the current defaultHint. + if mergedHint.NUMANodeAffinity.Count() != 1 { + return + } + + // If the current defaultHint is the same size as the new mergedHint, + // do not update defaultHint + if mergedHint.NUMANodeAffinity.Count() == defaultHint.NUMANodeAffinity.Count() { + return + } + // In all other cases, update defaultHint to the current mergedHint + defaultHint = mergedHint + }) return defaultHint } 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 b0173e424f1..4999265ac99 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go @@ -231,164 +231,6 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { } } -func TestPolicySingleNumaNodeGetHintMatch(t *testing.T) { - tcases := []struct { - name string - resources [][]TopologyHint - expectedFound bool - expectedMatch TopologyHint - }{ - { - name: "match single resource single hint", - resources: [][]TopologyHint{ - { - {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, - }, - }, - expectedFound: true, - expectedMatch: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(3), - Preferred: true, - }, - }, - { - name: "match single resource multiple hints (Selected hint preferred is true) 1/2", - resources: [][]TopologyHint{ - { - {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(5), Preferred: false}, - {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, - }, - }, - expectedFound: true, - expectedMatch: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - { - name: "match single resource multiple hints (Selected hint preferred is false) 2/2", - resources: [][]TopologyHint{ - { - {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(5), Preferred: false}, - {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(1), Preferred: false}, - }, - }, - expectedFound: true, - expectedMatch: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(1), - Preferred: true, - }, - }, - { - name: "match multiple resource single hint", - resources: [][]TopologyHint{ - { - {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, - }, - { - {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, - }, - { - {NUMANodeAffinity: NewTestBitMask(2), Preferred: false}, - }, - }, - expectedFound: true, - expectedMatch: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(2), - Preferred: true, - }, - }, - { - name: "match multiple resource single hint no match", - resources: [][]TopologyHint{ - { - {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, - }, - { - {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, - }, - { - {NUMANodeAffinity: NewTestBitMask(2), Preferred: false}, - }, - }, - expectedFound: false, - expectedMatch: TopologyHint{}, - }, - { - name: "multiple resources no match", - resources: [][]TopologyHint{ - { - {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(4), Preferred: false}, - {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(1), Preferred: false}, - }, - { - {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(5), Preferred: false}, - {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(1), Preferred: false}, - }, - { - {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(5), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(4), Preferred: true}, - }, - }, - expectedFound: false, - expectedMatch: TopologyHint{}, - }, - { - name: "multiple resources with match", - resources: [][]TopologyHint{ - { - {NUMANodeAffinity: NewTestBitMask(3), Preferred: false}, - {NUMANodeAffinity: NewTestBitMask(4), Preferred: false}, - {NUMANodeAffinity: NewTestBitMask(2), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, - }, - { - {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(5), Preferred: false}, - {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(1), Preferred: false}, - }, - { - {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(5), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(4), Preferred: true}, - {NUMANodeAffinity: NewTestBitMask(3), Preferred: true}, - }, - }, - expectedFound: true, - expectedMatch: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(3), - Preferred: true, - }, - }, - } - - numaNodes := []int{0, 1, 2, 3, 4, 5} - for _, tc := range tcases { - policy := NewSingleNumaNodePolicy(numaNodes) - found, match := policy.(*singleNumaNodePolicy).getHintMatch(tc.resources) - if found != tc.expectedFound { - t.Errorf("Test Case: %s", tc.name) - t.Errorf("Expected found to be %v, got %v", tc.expectedFound, found) - } - if found { - if !match.IsEqual(tc.expectedMatch) { - t.Errorf("Test Case: %s", tc.name) - t.Errorf("Expected match to be %v, got %v", tc.expectedMatch, match) - } - } - } -} - func TestPolicySingleNumaNodeMerge(t *testing.T) { numaNodes := []int{0, 1} policy := NewSingleNumaNodePolicy(numaNodes) From 4cc5b9e46c3c8b741466e80fbc3b23de33dcf3ea Mon Sep 17 00:00:00 2001 From: nolancon Date: Tue, 17 Dec 2019 08:36:08 +0000 Subject: [PATCH 11/33] Edit hints returned from policies and unit tests: - Best Effort Policy: Return hint with nil affinity as opposed to defaultAffinity when provider has no preference for NUMA affinty or no possible NUMA affinities. - Single NUMA Node Policy: Remove defaultHint from mergeProvidersHints. Instead return appropriate TopologyHint where required. - Update unit tests to reflect changes. Some test cases moved into individual policy test functions due to differing returned affinties per policy. --- .../cm/topologymanager/policy_best_effort.go | 6 +- .../policy_single_numa_node.go | 29 +- pkg/kubelet/cm/topologymanager/policy_test.go | 274 ++++++++++++------ 3 files changed, 195 insertions(+), 114 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index 5b1528c9740..26c7163bd83 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort.go @@ -135,7 +135,7 @@ func (p *bestEffortPolicy) mergeProvidersHints(providersHints []map[string][]Top // 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{{defaultAffinity, true}}) + allProviderHints = append(allProviderHints, []TopologyHint{{nil, true}}) continue } @@ -143,13 +143,13 @@ func (p *bestEffortPolicy) mergeProvidersHints(providersHints []map[string][]Top 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{{defaultAffinity, true}}) + 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{{defaultAffinity, false}}) + allProviderHints = append(allProviderHints, []TopologyHint{{nil, false}}) continue } diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index 37d95166d36..dc28a0c6349 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -140,12 +140,6 @@ func (p *singleNumaNodePolicy) filterHints(allResourcesHints [][]TopologyHint) ( } func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][]TopologyHint) TopologyHint { - // Set the default hint to return from this function as an any-NUMANode - // affinity with an unpreferred allocation. This will only be returned if - // no better hint can be found when merging hints from each hint provider. - defaultAffinity, _ := bitmask.NewBitMask(p.numaNodes...) - defaultHint := TopologyHint{defaultAffinity, false} - // Loop through all provider hints 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. @@ -169,7 +163,7 @@ func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][ klog.Infof("[topologymanager] Hint Provider has no possible NUMA affinities for resource '%s'", resource) // return defaultHint which will fail pod admission - return defaultHint + return TopologyHint{} } klog.Infof("[topologymanager] TopologyHints for resource '%v': %v", resource, hints[resource]) allResourcesHints = append(allResourcesHints, hints[resource]) @@ -180,8 +174,7 @@ func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][ // to true to allow scheduling. if len(allResourcesHints) == 0 { klog.Infof("[topologymanager] No preference for NUMA affinity from all providers") - defaultHint.Preferred = true - return defaultHint + return TopologyHint{nil, true} } var noAffinityPreferredOnly bool @@ -189,7 +182,7 @@ func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][ // If no hints, then policy cannot be satisfied if len(allResourcesHints) == 0 { klog.Infof("[topologymanager] No hints that align to a single NUMA node.") - return defaultHint + return TopologyHint{} } // If there is a resource with empty hints after filtering, then policy cannot be satisfied. // In the event that the only hints that exist are {nil true} update default hint preferred @@ -198,14 +191,18 @@ func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][ if len(hints) == 0 { klog.Infof("[topologymanager] No hints that align to a single NUMA node for resource.") if !noAffinityPreferredOnly { - return defaultHint + return TopologyHint{} } else if noAffinityPreferredOnly { - defaultHint.Preferred = true - return defaultHint + return TopologyHint{nil, true} } } } + // Set the bestHint to return from this function as an any-NUMANode + // affinity with an unpreferred allocation. This will only be returned if + // no better hint can be found when merging hints from each hint provider. + defaultAffinity, _ := bitmask.NewBitMask(p.numaNodes...) + bestHint := TopologyHint{defaultAffinity, false} p.iterateAllProviderTopologyHints(allResourcesHints, func(permutation []TopologyHint) { mergedHint := p.mergePermutation(permutation) // Only consider mergedHints that result in a NUMANodeAffinity == 1 to @@ -216,13 +213,13 @@ func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][ // If the current defaultHint is the same size as the new mergedHint, // do not update defaultHint - if mergedHint.NUMANodeAffinity.Count() == defaultHint.NUMANodeAffinity.Count() { + if mergedHint.NUMANodeAffinity.Count() == bestHint.NUMANodeAffinity.Count() { return } // In all other cases, update defaultHint to the current mergedHint - defaultHint = mergedHint + bestHint = mergedHint }) - return defaultHint + return bestHint } func (p *singleNumaNodePolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index 9e32c6989b2..084d9b55790 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -17,6 +17,7 @@ limitations under the License. package topologymanager import ( + "reflect" "testing" "k8s.io/api/core/v1" @@ -30,92 +31,6 @@ type policyMergeTestCase struct { func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ - { - name: "TopologyHint not set", - hp: []HintProvider{}, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, - { - name: "HintProvider returns empty non-nil map[string][]TopologyHint", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{}, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, - { - name: "HintProvider returns -nil map[string][]TopologyHint from provider", - 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", - 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", - 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", - 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", hp: []HintProvider{ @@ -429,6 +344,92 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ + { + name: "TopologyHint not set", + hp: []HintProvider{}, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, + { + name: "HintProvider returns -nil map[string][]TopologyHint from provider", + 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", + 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", + 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", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: false, + }, + }, { name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -583,6 +584,92 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ + { + name: "TopologyHint not set", + hp: []HintProvider{}, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + { + name: "HintProvider returns -nil map[string][]TopologyHint from provider", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": nil, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": {}, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + { + name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + { + name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: false, + }, + }, { name: "Single NUMA hint generation", hp: []HintProvider{ @@ -612,7 +699,7 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest }, }, expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), + NUMANodeAffinity: nil, Preferred: false, }, }, @@ -671,7 +758,7 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest }, }, expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), + NUMANodeAffinity: nil, Preferred: false, }, }, @@ -700,7 +787,7 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest }, }, expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), + NUMANodeAffinity: nil, Preferred: false, }, }, @@ -733,7 +820,7 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest }, }, expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), + NUMANodeAffinity: nil, Preferred: false, }, }, @@ -749,11 +836,8 @@ func testPolicyMerge(policy Policy, tcases []policyMergeTestCase, t *testing.T) } actual, _ := policy.Merge(providersHints) - if !actual.NUMANodeAffinity.IsEqual(tc.expected.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("%v: Expected Affinity preference in result to be %v, got %v", tc.name, tc.expected.Preferred, actual.Preferred) + if !reflect.DeepEqual(actual, tc.expected) { + t.Errorf("%v: Expected Topology Hint to be %v, got %v:", tc.name, tc.expected, actual) } } } From e43f0a52933be0d78abe590beddfdda6d02b4396 Mon Sep 17 00:00:00 2001 From: nolancon Date: Tue, 17 Dec 2019 09:03:19 +0000 Subject: [PATCH 12/33] Reinstate canAdmitPodResult in policy_none: This is to keep consistency with the other policies. This change may be made across all policies in a future PR, but removing it from the scope of this PR for now. --- pkg/kubelet/cm/topologymanager/policy_none.go | 10 ++++++++-- pkg/kubelet/cm/topologymanager/policy_none_test.go | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_none.go b/pkg/kubelet/cm/topologymanager/policy_none.go index 2b79fc0c4ef..7e5010a765d 100644 --- a/pkg/kubelet/cm/topologymanager/policy_none.go +++ b/pkg/kubelet/cm/topologymanager/policy_none.go @@ -36,6 +36,12 @@ func (p *nonePolicy) Name() string { return PolicyNone } -func (p *nonePolicy) Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) { - return TopologyHint{}, lifecycle.PodAdmitResult{Admit: true} +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 e65d9317467..ee674dcf6de 100644 --- a/pkg/kubelet/cm/topologymanager/policy_none_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_none_test.go @@ -39,7 +39,7 @@ func TestPolicyNoneName(t *testing.T) { } } -func TestPolicyNoneMerge(t *testing.T) { +func TestPolicyNoneCanAdmitPodResult(t *testing.T) { tcases := []struct { name string providersHints []map[string][]TopologyHint From adfd11f38f84bb63f5c8988008ece44499fc0c8e Mon Sep 17 00:00:00 2001 From: nolancon Date: Wed, 18 Dec 2019 05:41:04 +0000 Subject: [PATCH 13/33] Make iterateAllProviderTopologyHints generic: - Remove policy parameters to make this function generic. - Move function out of individual policies and into policy.go --- pkg/kubelet/cm/topologymanager/policy.go | 38 ++++++++++++++++++ .../cm/topologymanager/policy_best_effort.go | 40 +------------------ .../policy_single_numa_node.go | 40 +------------------ 3 files changed, 40 insertions(+), 78 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy.go b/pkg/kubelet/cm/topologymanager/policy.go index c93f3d135a0..6d682f83deb 100644 --- a/pkg/kubelet/cm/topologymanager/policy.go +++ b/pkg/kubelet/cm/topologymanager/policy.go @@ -28,3 +28,41 @@ type Policy interface { // and a Pod Admit Handler Response based on hints and policy type Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) } + +// 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], +// ... +// providerHints[-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{}) +} diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index 26c7163bd83..3f4e15fddda 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort.go @@ -53,44 +53,6 @@ func (p *bestEffortPolicy) Merge(providersHints []map[string][]TopologyHint) (To 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], -// ... -// providerHints[-1][z] -// } -// callback(permutation) -func (p *bestEffortPolicy) 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 a TopologyHints permutation to a single hint by performing a bitwise-AND // of their affinity masks. The hint shall be preferred if all hits in the permutation // are preferred. @@ -163,7 +125,7 @@ func (p *bestEffortPolicy) mergeProvidersHints(providersHints []map[string][]Top // 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} - p.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. mergedHint := p.mergePermutation(permutation) diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index dc28a0c6349..b4807db3db7 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -54,44 +54,6 @@ func (p *singleNumaNodePolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.P } } -// 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], -// ... -// providerHints[-1][z] -// } -// callback(permutation) -func (p *singleNumaNodePolicy) 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 a TopologyHints permutation to a single hint by performing a bitwise-AND // of their affinity masks. At this point, all hints have single numa node affinity // and preferred-true. @@ -203,7 +165,7 @@ func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][ // no better hint can be found when merging hints from each hint provider. defaultAffinity, _ := bitmask.NewBitMask(p.numaNodes...) bestHint := TopologyHint{defaultAffinity, false} - p.iterateAllProviderTopologyHints(allResourcesHints, func(permutation []TopologyHint) { + iterateAllProviderTopologyHints(allResourcesHints, func(permutation []TopologyHint) { mergedHint := p.mergePermutation(permutation) // Only consider mergedHints that result in a NUMANodeAffinity == 1 to // replace the current defaultHint. From 5487941485e712a99d53d739dd967db361e3394c Mon Sep 17 00:00:00 2001 From: nolancon Date: Wed, 18 Dec 2019 07:54:00 +0000 Subject: [PATCH 14/33] Refactor filterHints: - Restructure function - Remove bug fix for catching {nil true} - To be fixed in later commit - Restore unit tests to original state for testing filterHints --- .../policy_single_numa_node.go | 49 ++++-------- .../policy_single_numa_node_test.go | 74 +------------------ 2 files changed, 17 insertions(+), 106 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index b4807db3db7..0b30ec584ce 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -71,34 +71,20 @@ func (p *singleNumaNodePolicy) mergePermutation(permutation []TopologyHint) Topo return TopologyHint{mergedAffinity, preferred} } -// Return hints that have valid bitmasks with exactly one bit set. Also return bool -// which indicates whether allResourceHints only consists of {nil true} hints. -func (p *singleNumaNodePolicy) filterHints(allResourcesHints [][]TopologyHint) ([][]TopologyHint, bool) { +// Return hints that have valid bitmasks with exactly one bit set. +func (p *singleNumaNodePolicy) filterHints(allResourcesHints [][]TopologyHint) [][]TopologyHint { var filteredResourcesHints [][]TopologyHint - var noAffinityPreferredHints int - var totalHints int - if len(allResourcesHints) > 0 { - for _, oneResourceHints := range allResourcesHints { - var filtered []TopologyHint - if len(oneResourceHints) > 0 { - for _, hint := range oneResourceHints { - totalHints++ - if hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() == 1 && hint.Preferred == true { - filtered = append(filtered, hint) - } - if hint.NUMANodeAffinity == nil && hint.Preferred == true { - noAffinityPreferredHints++ - } - } + for _, oneResourceHints := range allResourcesHints { + var filtered []TopologyHint + for _, hint := range oneResourceHints { + if hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() == 1 && hint.Preferred == true { + filtered = append(filtered, hint) } - filteredResourcesHints = append(filteredResourcesHints, filtered) } + + filteredResourcesHints = append(filteredResourcesHints, filtered) } - // Check if all resource hints only consist of nil-affinity/preferred hint: {nil true}. - if noAffinityPreferredHints == totalHints { - return filteredResourcesHints, true - } - return filteredResourcesHints, false + return filteredResourcesHints } func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][]TopologyHint) TopologyHint { @@ -139,24 +125,17 @@ func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][ return TopologyHint{nil, true} } - var noAffinityPreferredOnly bool - allResourcesHints, noAffinityPreferredOnly = p.filterHints(allResourcesHints) - // If no hints, then policy cannot be satisfied + allResourcesHints = p.filterHints(allResourcesHints) + // If no hints, or there is a resource with empty hints after filtering, then policy + // cannot be satisfied if len(allResourcesHints) == 0 { klog.Infof("[topologymanager] No hints that align to a single NUMA node.") return TopologyHint{} } - // If there is a resource with empty hints after filtering, then policy cannot be satisfied. - // In the event that the only hints that exist are {nil true} update default hint preferred - // to allow scheduling. for _, hints := range allResourcesHints { if len(hints) == 0 { klog.Infof("[topologymanager] No hints that align to a single NUMA node for resource.") - if !noAffinityPreferredOnly { - return TopologyHint{} - } else if noAffinityPreferredOnly { - return TopologyHint{nil, true} - } + return TopologyHint{} } } 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 4999265ac99..c4a8b669c89 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go @@ -59,45 +59,18 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { name string allResources [][]TopologyHint expectedResources [][]TopologyHint - expectedExists bool }{ { name: "filter empty resources", allResources: [][]TopologyHint{}, expectedResources: [][]TopologyHint(nil), - expectedExists: false, }, { - name: "filter hints with nil socket mask, preferred true", - allResources: [][]TopologyHint{ - { - {NUMANodeAffinity: nil, Preferred: true}, - }, - }, - expectedResources: [][]TopologyHint{ - []TopologyHint(nil), - }, - expectedExists: true, - }, - - { - name: "filter hints with nil socket mask, preferred false", + name: "filter hints with nil socket mask 1/2", allResources: [][]TopologyHint{ { {NUMANodeAffinity: nil, Preferred: false}, }, - }, - expectedResources: [][]TopologyHint{ - []TopologyHint(nil), - }, - expectedExists: false, - }, - { - name: "filter hints with nil socket mask, preferred both true", - allResources: [][]TopologyHint{ - { - {NUMANodeAffinity: nil, Preferred: true}, - }, { {NUMANodeAffinity: nil, Preferred: true}, }, @@ -106,43 +79,9 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { []TopologyHint(nil), []TopologyHint(nil), }, - expectedExists: true, }, { - name: "filter hints with nil socket mask, preferred both false", - allResources: [][]TopologyHint{ - { - {NUMANodeAffinity: nil, Preferred: false}, - }, - { - {NUMANodeAffinity: nil, Preferred: false}, - }, - }, - expectedResources: [][]TopologyHint{ - []TopologyHint(nil), - []TopologyHint(nil), - }, - expectedExists: false, - }, - - { - name: "filter hints with nil socket mask, preferred true and false", - allResources: [][]TopologyHint{ - { - {NUMANodeAffinity: nil, Preferred: true}, - }, - { - {NUMANodeAffinity: nil, Preferred: false}, - }, - }, - expectedResources: [][]TopologyHint{ - []TopologyHint(nil), - []TopologyHint(nil), - }, - expectedExists: false, - }, - { - name: "filter hints with nil socket mask", + name: "filter hints with nil socket mask 2/2", allResources: [][]TopologyHint{ { {NUMANodeAffinity: NewTestBitMask(0), Preferred: true}, @@ -161,7 +100,6 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, }, }, - expectedExists: false, }, { name: "filter hints with empty resource socket mask", @@ -180,7 +118,6 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { }, []TopologyHint(nil), }, - expectedExists: false, }, { name: "filter hints with wide sockemask", @@ -212,22 +149,17 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { []TopologyHint(nil), []TopologyHint(nil), }, - expectedExists: false, }, } numaNodes := []int{0, 1, 2, 3} for _, tc := range tcases { policy := NewSingleNumaNodePolicy(numaNodes) - actual, exists := policy.(*singleNumaNodePolicy).filterHints(tc.allResources) + actual := policy.(*singleNumaNodePolicy).filterHints(tc.allResources) if !reflect.DeepEqual(tc.expectedResources, actual) { t.Errorf("Test Case: %s", tc.name) t.Errorf("Expected result to be %v, got %v", tc.expectedResources, actual) } - if !reflect.DeepEqual(tc.expectedResources, actual) { - t.Errorf("Test Case: %s", tc.name) - t.Errorf("Expected result to be %v, got %v", tc.expectedExists, exists) - } } } From 2d1a535a35c5a7c75c928c021db72a5f01300737 Mon Sep 17 00:00:00 2001 From: nolancon Date: Thu, 19 Dec 2019 08:18:26 +0000 Subject: [PATCH 15/33] Make mergePermutation generic: - Remove policy parameters to make function generic - Move function into top level policy.go --- pkg/kubelet/cm/topologymanager/policy.go | 31 ++++++++++++++++++ .../cm/topologymanager/policy_best_effort.go | 32 +------------------ .../policy_single_numa_node.go | 19 +---------- 3 files changed, 33 insertions(+), 49 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy.go b/pkg/kubelet/cm/topologymanager/policy.go index 6d682f83deb..76d384d14bf 100644 --- a/pkg/kubelet/cm/topologymanager/policy.go +++ b/pkg/kubelet/cm/topologymanager/policy.go @@ -17,6 +17,7 @@ limitations under the License. package topologymanager import ( + "k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask" "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) @@ -29,6 +30,36 @@ type Policy interface { Merge(providersHints []map[string][]TopologyHint) (TopologyHint, lifecycle.PodAdmitResult) } +// Merge a TopologyHints permutation to a single hint by performing a bitwise-AND +// of their affinity masks. The hint shall be preferred if all hits in the permutation +// are preferred. +func mergePermutation(numaNodes []int, permutation []TopologyHint) TopologyHint { + // Get the NUMANodeAffinity from each hint in the permutation and see if any + // of them encode unpreferred allocations. + defaultAffinity, _ := bitmask.NewBitMask(numaNodes...) + preferred := true + var numaAffinities []bitmask.BitMask + for _, hint := range permutation { + // Only consider hints that have an actual NUMANodeAffinity set. + if hint.NUMANodeAffinity == nil { + numaAffinities = append(numaAffinities, defaultAffinity) + } else { + numaAffinities = append(numaAffinities, hint.NUMANodeAffinity) + } + + if !hint.Preferred { + preferred = false + } + } + + // Merge the affinities using a bitwise-and operation. + mergedAffinity, _ := bitmask.NewBitMask(numaNodes...) + mergedAffinity.And(numaAffinities...) + // Build a mergedHint from the merged affinity mask, indicating if an + // preferred allocation was used to generate the affinity mask or not. + return TopologyHint{mergedAffinity, preferred} +} + // Iterate over all permutations of hints in 'allProviderHints [][]TopologyHint'. // // This procedure is implemented as a recursive function over the set of hints diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort.go b/pkg/kubelet/cm/topologymanager/policy_best_effort.go index 3f4e15fddda..fdcf3a55087 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort.go @@ -53,36 +53,6 @@ func (p *bestEffortPolicy) Merge(providersHints []map[string][]TopologyHint) (To return hint, admit } -// Merge a TopologyHints permutation to a single hint by performing a bitwise-AND -// of their affinity masks. The hint shall be preferred if all hits in the permutation -// are preferred. -func (p *bestEffortPolicy) mergePermutation(permutation []TopologyHint) TopologyHint { - // Get the NUMANodeAffinity from each hint in the permutation and see if any - // of them encode unpreferred allocations. - defaultAffinity, _ := bitmask.NewBitMask(p.numaNodes...) - preferred := true - var numaAffinities []bitmask.BitMask - for _, hint := range permutation { - // Only consider hints that have an actual NUMANodeAffinity set. - if hint.NUMANodeAffinity == nil { - numaAffinities = append(numaAffinities, defaultAffinity) - } else { - numaAffinities = append(numaAffinities, hint.NUMANodeAffinity) - } - - if !hint.Preferred { - preferred = false - } - } - - // Merge the affinities using a bitwise-and operation. - mergedAffinity, _ := bitmask.NewBitMask(p.numaNodes...) - mergedAffinity.And(numaAffinities...) - // Build a mergedHint from the merged affinity mask, indicating if an - // preferred allocation was used to generate the affinity mask or not. - return TopologyHint{mergedAffinity, preferred} -} - // Merge the hints from all hint providers to find the best one. func (p *bestEffortPolicy) mergeProvidersHints(providersHints []map[string][]TopologyHint) TopologyHint { // Set the default affinity as an any-numa affinity containing the list @@ -128,7 +98,7 @@ func (p *bestEffortPolicy) mergeProvidersHints(providersHints []map[string][]Top iterateAllProviderTopologyHints(allProviderHints, func(permutation []TopologyHint) { // Get the NUMANodeAffinity from each hint in the permutation and see if any // of them encode unpreferred allocations. - mergedHint := p.mergePermutation(permutation) + mergedHint := mergePermutation(p.numaNodes, permutation) // Only consider mergedHints that result in a NUMANodeAffinity > 0 to // replace the current bestHint. diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index 0b30ec584ce..7c79d8f6dcd 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -54,23 +54,6 @@ func (p *singleNumaNodePolicy) canAdmitPodResult(hint *TopologyHint) lifecycle.P } } -// Merge a TopologyHints permutation to a single hint by performing a bitwise-AND -// of their affinity masks. At this point, all hints have single numa node affinity -// and preferred-true. -func (p *singleNumaNodePolicy) mergePermutation(permutation []TopologyHint) TopologyHint { - preferred := true - var numaAffinities []bitmask.BitMask - for _, hint := range permutation { - numaAffinities = append(numaAffinities, hint.NUMANodeAffinity) - } - - // Merge the affinities using a bitwise-and operation. - mergedAffinity, _ := bitmask.NewBitMask(p.numaNodes...) - mergedAffinity.And(numaAffinities...) - // Build a mergedHint from the merged affinity mask. - return TopologyHint{mergedAffinity, preferred} -} - // Return hints that have valid bitmasks with exactly one bit set. func (p *singleNumaNodePolicy) filterHints(allResourcesHints [][]TopologyHint) [][]TopologyHint { var filteredResourcesHints [][]TopologyHint @@ -145,7 +128,7 @@ func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][ defaultAffinity, _ := bitmask.NewBitMask(p.numaNodes...) bestHint := TopologyHint{defaultAffinity, false} iterateAllProviderTopologyHints(allResourcesHints, func(permutation []TopologyHint) { - mergedHint := p.mergePermutation(permutation) + mergedHint := mergePermutation(p.numaNodes, permutation) // Only consider mergedHints that result in a NUMANodeAffinity == 1 to // replace the current defaultHint. if mergedHint.NUMANodeAffinity.Count() != 1 { From 6758f95117b29d8e786640f8c2843cd1f752b19c Mon Sep 17 00:00:00 2001 From: nolancon Date: Thu, 19 Dec 2019 08:36:18 +0000 Subject: [PATCH 16/33] Restore original policy none test cases: Mistakenly overwritten in earlier commit --- .../cm/topologymanager/policy_none_test.go | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/pkg/kubelet/cm/topologymanager/policy_none_test.go b/pkg/kubelet/cm/topologymanager/policy_none_test.go index ee674dcf6de..2c3263cec06 100644 --- a/pkg/kubelet/cm/topologymanager/policy_none_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_none_test.go @@ -40,6 +40,34 @@ func TestPolicyNoneName(t *testing.T) { } func TestPolicyNoneCanAdmitPodResult(t *testing.T) { + tcases := []struct { + name string + hint TopologyHint + expected bool + }{ + { + name: "Preferred is set to false in topology hints", + hint: TopologyHint{nil, false}, + expected: true, + }, + { + name: "Preferred is set to true in topology hints", + hint: TopologyHint{nil, true}, + expected: true, + }, + } + + for _, tc := range tcases { + policy := NewNonePolicy() + result := policy.(*nonePolicy).canAdmitPodResult(&tc.hint) + + if result.Admit != tc.expected { + t.Errorf("Expected Admit field in result to be %t, got %t", tc.expected, result.Admit) + } + } +} + +func TestPolicyNoneMerge(t *testing.T) { tcases := []struct { name string providersHints []map[string][]TopologyHint From 59bb6c4d6f4caed71f47cb536260e3d230ebd1d5 Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 05:24:57 +0000 Subject: [PATCH 17/33] Update checks in mergeProvidersHints: - Initialize best Hint to TopologyHint{} - Update checks. - Move generic unit test case into policy specific tests and updated expected outcome to reflect changes. --- .../policy_single_numa_node.go | 25 +++--- pkg/kubelet/cm/topologymanager/policy_test.go | 87 ++++++++++++------- 2 files changed, 73 insertions(+), 39 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index 7c79d8f6dcd..eb8cbd1e12a 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -18,7 +18,6 @@ package topologymanager import ( "k8s.io/klog" - "k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask" "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) @@ -122,11 +121,10 @@ func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][ } } - // Set the bestHint to return from this function as an any-NUMANode - // affinity with an unpreferred allocation. This will only be returned if - // no better hint can be found when merging hints from each hint provider. - defaultAffinity, _ := bitmask.NewBitMask(p.numaNodes...) - bestHint := TopologyHint{defaultAffinity, false} + // Set the bestHint to return from this function as {nil false}. + // This will only be returned if no better hint can be found when + // merging hints from each hint provider. + bestHint := TopologyHint{} iterateAllProviderTopologyHints(allResourcesHints, func(permutation []TopologyHint) { mergedHint := mergePermutation(p.numaNodes, permutation) // Only consider mergedHints that result in a NUMANodeAffinity == 1 to @@ -135,12 +133,19 @@ func (p *singleNumaNodePolicy) mergeProvidersHints(providersHints []map[string][ return } - // If the current defaultHint is the same size as the new mergedHint, - // do not update defaultHint - if mergedHint.NUMANodeAffinity.Count() == bestHint.NUMANodeAffinity.Count() { + // If the current bestHint NUMANodeAffinity is nil, update bestHint + // to the current mergedHint. + if bestHint.NUMANodeAffinity == nil { + bestHint = mergedHint return } - // In all other cases, update defaultHint to the current mergedHint + + // 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/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index 084d9b55790..61ca7d0496f 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -89,35 +89,6 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { Preferred: true, }, }, - { - name: "Two providers, 1 hint each, no common mask", - 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 no hints, 1 single hint preferred 1/2", hp: []HintProvider{ @@ -430,6 +401,35 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: false, }, }, + { + name: "Two providers, 1 hint each, no common mask", + 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, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -733,6 +733,35 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest Preferred: true, }, }, + { + name: "Two providers, 1 hint each, no common mask", + 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: nil, + Preferred: false, + }, + }, { name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", hp: []HintProvider{ From 8466a5852af7e8ef1ded0c61194fac7ffe3c66a0 Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 06:43:06 +0000 Subject: [PATCH 18/33] Restore policy_test.go to upstream Following commits will contain incremental changes to this file to ease review process and ensure all tests are accounted for. --- pkg/kubelet/cm/topologymanager/policy_test.go | 628 ++++++------------ 1 file changed, 212 insertions(+), 416 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index 61ca7d0496f..5e8a61b6394 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -17,7 +17,6 @@ limitations under the License. package topologymanager import ( - "reflect" "testing" "k8s.io/api/core/v1" @@ -31,6 +30,92 @@ type policyMergeTestCase struct { func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ + { + name: "TopologyHint not set", + hp: []HintProvider{}, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, + { + name: "HintProvider returns -nil map[string][]TopologyHint from provider", + 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", + 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", + 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", + 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", hp: []HintProvider{ @@ -89,6 +174,93 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { Preferred: true, }, }, + { + name: "Two providers, 1 hint each, no common mask", + 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", + 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", + 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", hp: []HintProvider{ @@ -195,6 +367,39 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { Preferred: true, }, }, + { + name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", + 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", hp: []HintProvider{ @@ -315,121 +520,6 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ - { - name: "TopologyHint not set", - hp: []HintProvider{}, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, - { - name: "HintProvider returns empty non-nil map[string][]TopologyHint", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{}, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, - { - name: "HintProvider returns -nil map[string][]TopologyHint from provider", - 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", - 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", - 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", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": { - { - NUMANodeAffinity: nil, - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: false, - }, - }, - { - name: "Two providers, 1 hint each, no common mask", - 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, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -488,188 +578,11 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: true, }, }, - { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", - 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", - 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 with 2 hints, 1 with single non-preferred hint matching", - 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, - }, - }, } } func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ - { - name: "TopologyHint not set", - hp: []HintProvider{}, - expected: TopologyHint{ - NUMANodeAffinity: nil, - Preferred: true, - }, - }, - { - name: "HintProvider returns empty non-nil map[string][]TopologyHint", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{}, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: nil, - Preferred: true, - }, - }, - { - name: "HintProvider returns -nil map[string][]TopologyHint from provider", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": nil, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: nil, - Preferred: true, - }, - }, - { - name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": {}, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: nil, - Preferred: false, - }, - }, - { - name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": { - { - NUMANodeAffinity: nil, - Preferred: true, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: nil, - Preferred: true, - }, - }, - { - name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{ - "resource": { - { - NUMANodeAffinity: nil, - Preferred: false, - }, - }, - }, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: nil, - Preferred: false, - }, - }, { name: "Single NUMA hint generation", hp: []HintProvider{ @@ -699,7 +612,7 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest }, }, expected: TopologyHint{ - NUMANodeAffinity: nil, + NUMANodeAffinity: NewTestBitMask(0), Preferred: false, }, }, @@ -733,126 +646,6 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest Preferred: true, }, }, - { - name: "Two providers, 1 hint each, no common mask", - 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: nil, - Preferred: false, - }, - }, - { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", - 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: nil, - Preferred: false, - }, - }, - { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", - 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: nil, - Preferred: false, - }, - }, - { - name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", - 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: nil, - Preferred: false, - }, - }, } } @@ -865,8 +658,11 @@ func testPolicyMerge(policy Policy, tcases []policyMergeTestCase, t *testing.T) } actual, _ := policy.Merge(providersHints) - if !reflect.DeepEqual(actual, tc.expected) { - t.Errorf("%v: Expected Topology Hint to be %v, got %v:", tc.name, tc.expected, actual) + if !actual.NUMANodeAffinity.IsEqual(tc.expected.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("%v: Expected Affinity preference in result to be %v, got %v", tc.name, tc.expected.Preferred, actual.Preferred) } } } From 51f1af03958c4494b4775ca0494375d362dbd944 Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 06:48:04 +0000 Subject: [PATCH 19/33] Move test case 'TopologyHint not set' into individual policy tests --- pkg/kubelet/cm/topologymanager/policy_test.go | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index 5e8a61b6394..d00ff1ca851 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -30,14 +30,6 @@ type policyMergeTestCase struct { func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ - { - name: "TopologyHint not set", - hp: []HintProvider{}, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, { name: "HintProvider returns empty non-nil map[string][]TopologyHint", hp: []HintProvider{ @@ -520,6 +512,14 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ + { + name: "TopologyHint not set", + hp: []HintProvider{}, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, { name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -583,6 +583,14 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ + { + name: "TopologyHint not set", + hp: []HintProvider{}, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: true, + }, + }, { name: "Single NUMA hint generation", hp: []HintProvider{ From 57661ee94677129af2ac5fa3caf0e9d3ed6f0306 Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 06:51:47 +0000 Subject: [PATCH 20/33] Move test case 'HintProvider returns empty non-nil map[string][]TopologyHint' into specific policy tests. --- pkg/kubelet/cm/topologymanager/policy_test.go | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index d00ff1ca851..e26b47358df 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -30,18 +30,6 @@ type policyMergeTestCase struct { func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ - { - name: "HintProvider returns empty non-nil map[string][]TopologyHint", - hp: []HintProvider{ - &mockHintProvider{ - map[string][]TopologyHint{}, - }, - }, - expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(numaNodes...), - Preferred: true, - }, - }, { name: "HintProvider returns -nil map[string][]TopologyHint from provider", hp: []HintProvider{ @@ -520,6 +508,18 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: true, }, }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, { name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -591,6 +591,18 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest Preferred: true, }, }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{}, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: true, + }, + }, { name: "Single NUMA hint generation", hp: []HintProvider{ From 599217d48283339c51dcbd37708691bce5905e4a Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 06:55:50 +0000 Subject: [PATCH 21/33] Move test case "HintProvider returns -nil map[string][]TopologyHint from provider" into specific policy tests --- pkg/kubelet/cm/topologymanager/policy_test.go | 42 ++++++++++++------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index e26b47358df..279adbfcd33 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -30,20 +30,6 @@ type policyMergeTestCase struct { func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ - { - name: "HintProvider returns -nil map[string][]TopologyHint from provider", - 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", hp: []HintProvider{ @@ -520,6 +506,20 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: true, }, }, + { + name: "HintProvider returns -nil map[string][]TopologyHint from provider", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": nil, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, { name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -603,6 +603,20 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest Preferred: true, }, }, + { + name: "HintProvider returns -nil map[string][]TopologyHint from provider", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": nil, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: true, + }, + }, { name: "Single NUMA hint generation", hp: []HintProvider{ From baeff9ec5d78a6262ca533436a20572b70c8b4c6 Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 06:59:54 +0000 Subject: [PATCH 22/33] Move test case "HintProvider returns empty non-nil map[string][]TopologyHint from provider" into specific policy tests. --- pkg/kubelet/cm/topologymanager/policy_test.go | 40 ++++++++++++------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index 279adbfcd33..ca81c6c7e93 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -30,20 +30,6 @@ type policyMergeTestCase struct { func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ - { - name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", - 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", hp: []HintProvider{ @@ -520,6 +506,19 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: true, }, }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": {}, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: false, + }, + }, { name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -617,6 +616,19 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest Preferred: true, }, }, + { + name: "HintProvider returns empty non-nil map[string][]TopologyHint from provider", hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": {}, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: false, + }, + }, { name: "Single NUMA hint generation", hp: []HintProvider{ From 6460ef63929fe6171708ab7d2c2ec08e8e177517 Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 07:07:46 +0000 Subject: [PATCH 23/33] Move test case "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil" into specific policy tests. --- pkg/kubelet/cm/topologymanager/policy_test.go | 57 ++++++++++++------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index ca81c6c7e93..878e8997545 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -30,25 +30,6 @@ type policyMergeTestCase struct { func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ - { - name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", - 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", hp: []HintProvider{ @@ -519,6 +500,25 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: false, }, }, + { + name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: true, + }, + }, { name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -629,6 +629,25 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest Preferred: false, }, }, + { + name: "Single TopologyHint with Preferred as true and NUMANodeAffinity as nil", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: true, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: true, + }, + }, { name: "Single NUMA hint generation", hp: []HintProvider{ From 401a2bb285a79413986361e904da826eaa2d17d5 Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 07:12:04 +0000 Subject: [PATCH 24/33] Move test case "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil" into specific policy tests. --- pkg/kubelet/cm/topologymanager/policy_test.go | 57 ++++++++++++------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index 878e8997545..5517e4c7b0c 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -30,25 +30,6 @@ type policyMergeTestCase struct { func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { return []policyMergeTestCase{ - { - name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", - 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", hp: []HintProvider{ @@ -519,6 +500,25 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: true, }, }, + { + name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: NewTestBitMask(numaNodes...), + Preferred: false, + }, + }, { name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -648,6 +648,25 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest Preferred: true, }, }, + { + name: "Single TopologyHint with Preferred as false and NUMANodeAffinity as nil", + hp: []HintProvider{ + &mockHintProvider{ + map[string][]TopologyHint{ + "resource": { + { + NUMANodeAffinity: nil, + Preferred: false, + }, + }, + }, + }, + }, + expected: TopologyHint{ + NUMANodeAffinity: nil, + Preferred: false, + }, + }, { name: "Single NUMA hint generation", hp: []HintProvider{ From f639da7637be1379d8c3757d1f31c98a72e0c9fc Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 07:22:30 +0000 Subject: [PATCH 25/33] Move test case "Two providers, 1 hint each, no common mask" into specific policy tests. --- pkg/kubelet/cm/topologymanager/policy_test.go | 87 ++++++++++++------- 1 file changed, 58 insertions(+), 29 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index 5517e4c7b0c..2dad9bb4f25 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -88,35 +88,6 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { Preferred: true, }, }, - { - name: "Two providers, 1 hint each, no common mask", - 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", hp: []HintProvider{ @@ -519,6 +490,35 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: false, }, }, + { + name: "Two providers, 1 hint each, no common mask", + 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, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -667,6 +667,35 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest Preferred: false, }, }, + { + name: "Two providers, 1 hint each, no common mask", + 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: nil, + Preferred: false, + }, + }, { name: "Single NUMA hint generation", hp: []HintProvider{ From a38a2562b2f2b2208069ed14f6372410f2731f0d Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 07:27:28 +0000 Subject: [PATCH 26/33] Move test case "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2" into specific policy test. --- pkg/kubelet/cm/topologymanager/policy_test.go | 87 ++++++++++++------- 1 file changed, 58 insertions(+), 29 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index 2dad9bb4f25..0cd7902c81b 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -88,35 +88,6 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { Preferred: true, }, }, - { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", - 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", hp: []HintProvider{ @@ -519,6 +490,35 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: false, }, }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", + 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, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -696,6 +696,35 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest Preferred: false, }, }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 1/2", + 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: nil, + Preferred: false, + }, + }, { name: "Single NUMA hint generation", hp: []HintProvider{ From 681c42bfc2575a08330cfec663ac87affea63ee7 Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 07:31:18 +0000 Subject: [PATCH 27/33] Move test case "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2" into specific policy tests --- pkg/kubelet/cm/topologymanager/policy_test.go | 87 ++++++++++++------- 1 file changed, 58 insertions(+), 29 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index 0cd7902c81b..e1f1bd86eb8 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -88,35 +88,6 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { Preferred: true, }, }, - { - name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", - 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", hp: []HintProvider{ @@ -519,6 +490,35 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: false, }, }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", + 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 hint each, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -725,6 +725,35 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest Preferred: false, }, }, + { + name: "Two providers, 1 hint each, same mask, 1 preferred, 1 not 2/2", + 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: nil, + Preferred: false, + }, + }, { name: "Single NUMA hint generation", hp: []HintProvider{ From 8b3f6e61a22fa0364c1ab3f70a3e57ee2d5cd4c4 Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 07:45:28 +0000 Subject: [PATCH 28/33] Move test case "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching" into specific policy tests --- pkg/kubelet/cm/topologymanager/policy_test.go | 99 ++++++++++++------- 1 file changed, 66 insertions(+), 33 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index e1f1bd86eb8..edc485a4bd5 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -194,39 +194,6 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase { Preferred: true, }, }, - { - name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", - 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", hp: []HintProvider{ @@ -548,6 +515,39 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase Preferred: true, }, }, + { + name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", + 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, 1 hint each, 1 wider mask, both preferred 1/2", hp: []HintProvider{ @@ -754,6 +754,39 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest Preferred: false, }, }, + { + name: "Two providers, 1 with 2 hints, 1 with single non-preferred hint matching", + 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: nil, + Preferred: false, + }, + }, { name: "Single NUMA hint generation", hp: []HintProvider{ From 92eb7cd601122c9ffc8c00a9f80f43682c28f02e Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 07:47:46 +0000 Subject: [PATCH 29/33] Update "Single NUMA hint generation" expected affinity to nil --- pkg/kubelet/cm/topologymanager/policy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index edc485a4bd5..de2c4b4b46b 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -816,7 +816,7 @@ func (p *singleNumaNodePolicy) mergeTestCases(numaNodes []int) []policyMergeTest }, }, expected: TopologyHint{ - NUMANodeAffinity: NewTestBitMask(0), + NUMANodeAffinity: nil, Preferred: false, }, }, From 5e23517ebf1c1163287affef0c63608e53feda1a Mon Sep 17 00:00:00 2001 From: nolancon Date: Fri, 20 Dec 2019 07:51:24 +0000 Subject: [PATCH 30/33] Use reflect.DeepEqual check in policy_test.go --- pkg/kubelet/cm/topologymanager/policy_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index de2c4b4b46b..c8aa12bbaf9 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_test.go @@ -17,6 +17,7 @@ limitations under the License. package topologymanager import ( + "reflect" "testing" "k8s.io/api/core/v1" @@ -862,11 +863,8 @@ func testPolicyMerge(policy Policy, tcases []policyMergeTestCase, t *testing.T) } actual, _ := policy.Merge(providersHints) - if !actual.NUMANodeAffinity.IsEqual(tc.expected.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("%v: Expected Affinity preference in result to be %v, got %v", tc.name, tc.expected.Preferred, actual.Preferred) + if !reflect.DeepEqual(actual, tc.expected) { + t.Errorf("%v: Expected Topology Hint to be %v, got %v:", tc.name, tc.expected, actual) } } } From 94489c137ca88b1b7fb186c649b4b370caf9bbd5 Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Thu, 16 Jan 2020 15:52:16 +0100 Subject: [PATCH 31/33] Cleanup use of defaultAffinity in mergePermutation of TopologyManager --- pkg/kubelet/cm/topologymanager/policy.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy.go b/pkg/kubelet/cm/topologymanager/policy.go index 76d384d14bf..f2df6f29b27 100644 --- a/pkg/kubelet/cm/topologymanager/policy.go +++ b/pkg/kubelet/cm/topologymanager/policy.go @@ -36,8 +36,8 @@ type Policy interface { func mergePermutation(numaNodes []int, permutation []TopologyHint) TopologyHint { // Get the NUMANodeAffinity from each hint in the permutation and see if any // of them encode unpreferred allocations. - defaultAffinity, _ := bitmask.NewBitMask(numaNodes...) preferred := true + defaultAffinity, _ := bitmask.NewBitMask(numaNodes...) var numaAffinities []bitmask.BitMask for _, hint := range permutation { // Only consider hints that have an actual NUMANodeAffinity set. @@ -53,8 +53,7 @@ func mergePermutation(numaNodes []int, permutation []TopologyHint) TopologyHint } // Merge the affinities using a bitwise-and operation. - mergedAffinity, _ := bitmask.NewBitMask(numaNodes...) - mergedAffinity.And(numaAffinities...) + mergedAffinity := bitmask.And(defaultAffinity, numaAffinities...) // Build a mergedHint from the merged affinity mask, indicating if an // preferred allocation was used to generate the affinity mask or not. return TopologyHint{mergedAffinity, preferred} From 2905ffffa7649baa65c954b6a93a57711cf3127b Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Thu, 16 Jan 2020 15:53:26 +0100 Subject: [PATCH 32/33] Rename TopologyManager test TestPolicyBestEffortMerge for consistency --- pkg/kubelet/cm/topologymanager/policy_best_effort_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go index 2ae7de0ecd8..b661516be01 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go @@ -49,7 +49,7 @@ func TestPolicyBestEffortCanAdmitPodResult(t *testing.T) { } } -func TestBestEffortPolicyMerge(t *testing.T) { +func TestPolicyBestEffortMerge(t *testing.T) { numaNodes := []int{0, 1} policy := NewBestEffortPolicy(numaNodes) From 7069b1d6e8bedc9f9307b99e473b27465653bee4 Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Thu, 16 Jan 2020 15:54:17 +0100 Subject: [PATCH 33/33] Update TopologyManager single-numa-node logic to handle "don't cares" The logic has been updated to match the logic of the best-effort policy except in two places: 1) The hint filtering frunction has been updated to allow "don't care" hints encoded with a `nil` affinity mask, to pass through the filter in addition to hints that have just a single NUMA bit set. 2) After calculating the `bestHint` we transform "don't care" affinities encoded as having all NUMA bits set in their affinity masks into "don't care" affinities encoded as `nil`. --- .../policy_single_numa_node.go | 90 ++++++++++--------- .../policy_single_numa_node_test.go | 5 +- 2 files changed, 52 insertions(+), 43 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go index eb8cbd1e12a..ccbbe213aec 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node.go @@ -18,6 +18,7 @@ package topologymanager import ( "k8s.io/klog" + "k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask" "k8s.io/kubernetes/pkg/kubelet/lifecycle" ) @@ -59,95 +60,100 @@ func (p *singleNumaNodePolicy) filterHints(allResourcesHints [][]TopologyHint) [ for _, oneResourceHints := range allResourcesHints { var filtered []TopologyHint for _, hint := range oneResourceHints { + if hint.NUMANodeAffinity == nil && hint.Preferred == true { + filtered = append(filtered, hint) + } if hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() == 1 && hint.Preferred == true { filtered = append(filtered, hint) } } - filteredResourcesHints = append(filteredResourcesHints, filtered) } return filteredResourcesHints } func (p *singleNumaNodePolicy) 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(p.numaNodes...) + // Loop through all provider hints 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 allResourcesHints [][]TopologyHint + 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, " + - "skipping.") + 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', skipping.", resource) + 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) - // return defaultHint which will fail pod admission - return TopologyHint{} + klog.Infof("[topologymanager] Hint Provider has no possible NUMA affinities for resource '%s'", resource) + allProviderHints = append(allProviderHints, []TopologyHint{{nil, false}}) + continue } - klog.Infof("[topologymanager] TopologyHints for resource '%v': %v", resource, hints[resource]) - allResourcesHints = append(allResourcesHints, hints[resource]) + + allProviderHints = append(allProviderHints, hints[resource]) } } - // In case allProviderHints length is zero it means that we have no - // preference for NUMA affinity. In that case update default hint preferred - // to true to allow scheduling. - if len(allResourcesHints) == 0 { - klog.Infof("[topologymanager] No preference for NUMA affinity from all providers") - return TopologyHint{nil, true} - } - allResourcesHints = p.filterHints(allResourcesHints) - // If no hints, or there is a resource with empty hints after filtering, then policy - // cannot be satisfied - if len(allResourcesHints) == 0 { - klog.Infof("[topologymanager] No hints that align to a single NUMA node.") - return TopologyHint{} - } - for _, hints := range allResourcesHints { - if len(hints) == 0 { - klog.Infof("[topologymanager] No hints that align to a single NUMA node for resource.") - return TopologyHint{} - } - } + // Filter to only include don't cares and hints with a single NUMA node. + allProviderHints = p.filterHints(allProviderHints) // Set the bestHint to return from this function as {nil false}. // This will only be returned if no better hint can be found when // merging hints from each hint provider. - bestHint := TopologyHint{} - iterateAllProviderTopologyHints(allResourcesHints, func(permutation []TopologyHint) { + 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. mergedHint := mergePermutation(p.numaNodes, permutation) - // Only consider mergedHints that result in a NUMANodeAffinity == 1 to - // replace the current defaultHint. - if mergedHint.NUMANodeAffinity.Count() != 1 { + + // Only consider mergedHints that result in a NUMANodeAffinity > 0 to + // replace the current bestHint. + if mergedHint.NUMANodeAffinity.Count() == 0 { return } - // If the current bestHint NUMANodeAffinity is nil, update bestHint - // to the current mergedHint. - if bestHint.NUMANodeAffinity == nil { + // 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 } - // Only consider mergedHints that have a narrower NUMANodeAffinity - // than the NUMANodeAffinity in the current bestHint. + // 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 }) + + if bestHint.NUMANodeAffinity.IsEqual(defaultAffinity) { + bestHint = TopologyHint{nil, bestHint.Preferred} + } + return bestHint } 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 c4a8b669c89..d8664e46b5c 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go +++ b/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go @@ -77,7 +77,9 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { }, expectedResources: [][]TopologyHint{ []TopologyHint(nil), - []TopologyHint(nil), + { + {NUMANodeAffinity: nil, Preferred: true}, + }, }, }, { @@ -98,6 +100,7 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) { }, { {NUMANodeAffinity: NewTestBitMask(1), Preferred: true}, + {NUMANodeAffinity: nil, Preferred: true}, }, }, },