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.
2025-07-23 19:56:01 +00:00 · 2019-12-17 08:36:08 +00:00 · 2019-12-17 08:36:08 +00:00 · 4cc5b9e46c
commit 4cc5b9e46c
parent e3d0c9397f
3 changed files with 195 additions and 114 deletions
--- 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
 			}

--- 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) {
--- 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)
 		}
 	}
 }