From 4cc5b9e46c3c8b741466e80fbc3b23de33dcf3ea Mon Sep 17 00:00:00 2001 From: nolancon Date: Tue, 17 Dec 2019 08:36:08 +0000 Subject: [PATCH] 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) } } }