Fix bug in TopologyManager with merging hints when NUM_NUMA > 2

Before this fix, hint permutations such as:

	permutation: [{11 true} {0101 true}]

Could result in merged hints of:

	mergedHint: {01 true}

This was possible because both hints in the permutation container a "preferred"
allocation (i.e. the full set of NUMA nodes set in the affinity bitmask are
*required* to satisfy the allocation). With this in place, the simplified logic
we had simply kept the merged hint as preferred as well.

However, what we really want is to ensure that the merged hint is only
preferred if *true* alignment of all resources is possible (i.e. if all hints
in the permutation are preferred AND their affinities are exactly equal).

The only exception to this is if *no* topology information is provided by a
given hint provider. In this case, we assume alignment doesn't matter and only
consider the resources that actually have hints provided for them.

This changes the semantics of permutations of the form:

	permutation: [{111 true} {011 true}]

To now result in the merged hint of:

	mergedHint: {011 false}

Instead of:

	mergedHint: {011 true}

This is arguably how it should always have been though (because a hint should
not be preferred if true alignment isn't possible), and two tests have had to
change to accomodate these new semantics.

This commit changes the merge function to implement the updated logic, adds a
test to verify it is functioning correctly, and updates the two tests mentioned
above to adjust to the new semantics.

Signed-off-by: Kevin Klues <kklues@nvidia.com>

pkg/kubelet/cm/topologymanager/policy.go

@@ -41,12 +41,14 @@ func mergePermutation(numaNodes []int, permutation []TopologyHint) TopologyHint
 	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 {
+		if hint.NUMANodeAffinity != nil {
 			numaAffinities = append(numaAffinities, hint.NUMANodeAffinity)
+			// Only mark preferred if all affinities are equal.
+			if !hint.NUMANodeAffinity.IsEqual(numaAffinities[0]) {
+				preferred = false
+			}
 		}
-
+		// Only mark preferred if all affinities are preferred.
 		if !hint.Preferred {
 			preferred = false
 		}
@@ -54,8 +56,8 @@ func mergePermutation(numaNodes []int, permutation []TopologyHint) TopologyHint
 
 	// Merge the affinities using a bitwise-and operation.
 	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.
+	// Build a mergedHint from the merged affinity mask, setting preferred as
+	// appropriate based on the logic above.
 	return TopologyHint{mergedAffinity, preferred}
 }
 

pkg/kubelet/cm/topologymanager/policy_best_effort_test.go

@@ -50,7 +50,7 @@ func TestPolicyBestEffortCanAdmitPodResult(t *testing.T) {
 }
 
 func TestPolicyBestEffortMerge(t *testing.T) {
-	numaNodes := []int{0, 1}
+	numaNodes := []int{0, 1, 2, 3}
 	policy := NewBestEffortPolicy(numaNodes)
 
 	tcases := commonPolicyMergeTestCases(numaNodes)

pkg/kubelet/cm/topologymanager/policy_restricted_test.go

@@ -68,7 +68,7 @@ func TestPolicyRestrictedCanAdmitPodResult(t *testing.T) {
 }
 
 func TestPolicyRestrictedMerge(t *testing.T) {
-	numaNodes := []int{0, 1}
+	numaNodes := []int{0, 1, 2, 3}
 	policy := NewRestrictedPolicy(numaNodes)
 
 	tcases := commonPolicyMergeTestCases(numaNodes)

pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go

@@ -156,7 +156,7 @@ func TestPolicySingleNumaNodeFilterHints(t *testing.T) {
 }
 
 func TestPolicySingleNumaNodeMerge(t *testing.T) {
-	numaNodes := []int{0, 1}
+	numaNodes := []int{0, 1, 2, 3}
 	policy := NewSingleNumaNodePolicy(numaNodes)
 
 	tcases := commonPolicyMergeTestCases(numaNodes)

pkg/kubelet/cm/topologymanager/policy_test.go

@@ -315,6 +315,43 @@ func commonPolicyMergeTestCases(numaNodes []int) []policyMergeTestCase {
 
 func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase {
 	return []policyMergeTestCase{
+		{
+			name: "Two providers, 2 hints each, same mask (some with different bits), same preferred",
+			hp: []HintProvider{
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource1": {
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1),
+								Preferred:        true,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 2),
+								Preferred:        true,
+							},
+						},
+					},
+				},
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource2": {
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1),
+								Preferred:        true,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 2),
+								Preferred:        true,
+							},
+						},
+					},
+				},
+			},
+			expected: TopologyHint{
+				NUMANodeAffinity: NewTestBitMask(0, 1),
+				Preferred:        true,
+			},
+		},
 		{
 			name: "TopologyHint not set",
 			hp:   []HintProvider{},
@@ -513,7 +550,7 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase
 			},
 			expected: TopologyHint{
 				NUMANodeAffinity: NewTestBitMask(0),
-				Preferred:        true,
+				Preferred:        false,
 			},
 		},
 		{
@@ -550,7 +587,7 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase
 			},
 		},
 		{
-			name: "Two providers, 1 hint each, 1 wider mask, both preferred 1/2",
+			name: "Two providers, 1 hint each, 1 wider mask, both preferred 2/2",
 			hp: []HintProvider{
 				&mockHintProvider{
 					map[string][]TopologyHint{
@@ -575,7 +612,7 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase
 			},
 			expected: TopologyHint{
 				NUMANodeAffinity: NewTestBitMask(1),
-				Preferred:        true,
+				Preferred:        false,
 			},
 		},
 	}