Merge pull request #108154 from klueska/fix-topology-manager

Update TopologyManager algorithm for selecting "best" non-preferred hint

pkg/kubelet/cm/topologymanager/policy.go

@@ -94,51 +94,196 @@ func filterProvidersHints(providersHints []map[string][]TopologyHint) [][]Topolo
 	return allProviderHints
 }
 
-func mergeFilteredHints(numaNodes []int, filteredHints [][]TopologyHint) TopologyHint {
+func narrowestHint(hints []TopologyHint) *TopologyHint {
-	// Set the default affinity as an any-numa affinity containing the list
+	if len(hints) == 0 {
-	// of NUMA Nodes available on this machine.
+		return nil
-	defaultAffinity, _ := bitmask.NewBitMask(numaNodes...)
+	}
+	var narrowestHint *TopologyHint
+	for i := range hints {
+		if hints[i].NUMANodeAffinity == nil {
+			continue
+		}
+		if narrowestHint == nil {
+			narrowestHint = &hints[i]
+		}
+		if hints[i].NUMANodeAffinity.IsNarrowerThan(narrowestHint.NUMANodeAffinity) {
+			narrowestHint = &hints[i]
+		}
+	}
+	return narrowestHint
+}
 
-	// Set the bestHint to return from this function as {nil false}.
+func maxOfMinAffinityCounts(filteredHints [][]TopologyHint) int {
-	// This will only be returned if no better hint can be found when
+	maxOfMinCount := 0
-	// merging hints from each hint provider.
+	for _, resourceHints := range filteredHints {
-	bestHint := TopologyHint{defaultAffinity, false}
+		narrowestHint := narrowestHint(resourceHints)
+		if narrowestHint == nil {
+			continue
+		}
+		if narrowestHint.NUMANodeAffinity.Count() > maxOfMinCount {
+			maxOfMinCount = narrowestHint.NUMANodeAffinity.Count()
+		}
+	}
+	return maxOfMinCount
+}
+
+func compareHints(bestNonPreferredAffinityCount int, current *TopologyHint, candidate *TopologyHint) *TopologyHint {
+	// Only consider candidates that result in a NUMANodeAffinity > 0 to
+	// replace the current bestHint.
+	if candidate.NUMANodeAffinity.Count() == 0 {
+		return current
+	}
+
+	// If no current bestHint is set, return the candidate as the bestHint.
+	if current == nil {
+		return candidate
+	}
+
+	// If the current bestHint is non-preferred and the candidate hint is
+	// preferred, always choose the preferred hint over the non-preferred one.
+	if !current.Preferred && candidate.Preferred {
+		return candidate
+	}
+
+	// If the current bestHint is preferred and the candidate hint is
+	// non-preferred, never update the bestHint, regardless of the
+	// candidate hint's narowness.
+	if current.Preferred && !candidate.Preferred {
+		return current
+	}
+
+	// If the current bestHint and the candidate hint are both preferred,
+	// then only consider candidate hints that have a narrower
+	// NUMANodeAffinity than the NUMANodeAffinity in the current bestHint.
+	if current.Preferred && candidate.Preferred {
+		if candidate.NUMANodeAffinity.IsNarrowerThan(current.NUMANodeAffinity) {
+			return candidate
+		}
+		return current
+	}
+
+	// The only case left is if the current best bestHint and the candidate
+	// hint are both non-preferred. In this case, try and find a hint whose
+	// affinity count is as close to (but not higher than) the
+	// bestNonPreferredAffinityCount as possible. To do this we need to
+	// consider the following cases and react accordingly:
+	//
+	//   1. current.NUMANodeAffinity.Count() >  bestNonPreferredAffinityCount
+	//   2. current.NUMANodeAffinity.Count() == bestNonPreferredAffinityCount
+	//   3. current.NUMANodeAffinity.Count() <  bestNonPreferredAffinityCount
+	//
+	// For case (1), the current bestHint is larger than the
+	// bestNonPreferredAffinityCount, so updating to any narrower mergeHint
+	// is preferred over staying where we are.
+	//
+	// For case (2), the current bestHint is equal to the
+	// bestNonPreferredAffinityCount, so we would like to stick with what
+	// we have *unless* the candidate hint is also equal to
+	// bestNonPreferredAffinityCount and it is narrower.
+	//
+	// For case (3), the current bestHint is less than
+	// bestNonPreferredAffinityCount, so we would like to creep back up to
+	// bestNonPreferredAffinityCount as close as we can. There are three
+	// cases to consider here:
+	//
+	//   3a. candidate.NUMANodeAffinity.Count() >  bestNonPreferredAffinityCount
+	//   3b. candidate.NUMANodeAffinity.Count() == bestNonPreferredAffinityCount
+	//   3c. candidate.NUMANodeAffinity.Count() <  bestNonPreferredAffinityCount
+	//
+	// For case (3a), we just want to stick with the current bestHint
+	// because choosing a new hint that is greater than
+	// bestNonPreferredAffinityCount would be counter-productive.
+	//
+	// For case (3b), we want to immediately update bestHint to the
+	// candidate hint, making it now equal to bestNonPreferredAffinityCount.
+	//
+	// For case (3c), we know that *both* the current bestHint and the
+	// candidate hint are less than bestNonPreferredAffinityCount, so we
+	// want to choose one that brings us back up as close to
+	// bestNonPreferredAffinityCount as possible. There are three cases to
+	// consider here:
+	//
+	//   3ca. candidate.NUMANodeAffinity.Count() >  current.NUMANodeAffinity.Count()
+	//   3cb. candidate.NUMANodeAffinity.Count() <  current.NUMANodeAffinity.Count()
+	//   3cc. candidate.NUMANodeAffinity.Count() == current.NUMANodeAffinity.Count()
+	//
+	// For case (3ca), we want to immediately update bestHint to the
+	// candidate hint because that will bring us closer to the (higher)
+	// value of bestNonPreferredAffinityCount.
+	//
+	// For case (3cb), we want to stick with the current bestHint because
+	// choosing the candidate hint would strictly move us further away from
+	// the bestNonPreferredAffinityCount.
+	//
+	// Finally, for case (3cc), we know that the current bestHint and the
+	// candidate hint are equal, so we simply choose the narrower of the 2.
+
+	// Case 1
+	if current.NUMANodeAffinity.Count() > bestNonPreferredAffinityCount {
+		if candidate.NUMANodeAffinity.IsNarrowerThan(current.NUMANodeAffinity) {
+			return candidate
+		}
+		return current
+	}
+	// Case 2
+	if current.NUMANodeAffinity.Count() == bestNonPreferredAffinityCount {
+		if candidate.NUMANodeAffinity.Count() != bestNonPreferredAffinityCount {
+			return current
+		}
+		if candidate.NUMANodeAffinity.IsNarrowerThan(current.NUMANodeAffinity) {
+			return candidate
+		}
+		return current
+	}
+	// Case 3a
+	if candidate.NUMANodeAffinity.Count() > bestNonPreferredAffinityCount {
+		return current
+	}
+	// Case 3b
+	if candidate.NUMANodeAffinity.Count() == bestNonPreferredAffinityCount {
+		return candidate
+	}
+	// Case 3ca
+	if candidate.NUMANodeAffinity.Count() > current.NUMANodeAffinity.Count() {
+		return candidate
+	}
+	// Case 3cb
+	if candidate.NUMANodeAffinity.Count() < current.NUMANodeAffinity.Count() {
+		return current
+	}
+	// Case 3cc
+	if candidate.NUMANodeAffinity.IsNarrowerThan(current.NUMANodeAffinity) {
+		return candidate
+	}
+	return current
+}
+
+func mergeFilteredHints(numaNodes []int, filteredHints [][]TopologyHint) TopologyHint {
+	// Set bestNonPreferredAffinityCount to help decide which affinity mask is
+	// preferred amongst all non-preferred hints. We calculate this value as
+	// the maximum of the minimum affinity counts supplied for any given hint
+	// provider. In other words, prefer a hint that has an affinity mask that
+	// includes all of the NUMA nodes from the provider that requires the most
+	// NUMA nodes to satisfy its allocation.
+	bestNonPreferredAffinityCount := maxOfMinAffinityCounts(filteredHints)
+
+	var bestHint *TopologyHint
 	iterateAllProviderTopologyHints(filteredHints, func(permutation []TopologyHint) {
 		// Get the NUMANodeAffinity from each hint in the permutation and see if any
 		// of them encode unpreferred allocations.
 		mergedHint := mergePermutation(numaNodes, permutation)
-		// Only consider mergedHints that result in a NUMANodeAffinity > 0 to
-		// replace the current bestHint.
-		if mergedHint.NUMANodeAffinity.Count() == 0 {
-			return
-		}
 
-		// If the current bestHint is non-preferred and the new mergedHint is
+		// Compare the current bestHint with the candidate mergedHint and
-		// preferred, always choose the preferred hint over the non-preferred one.
+		// update bestHint if appropriate.
-		if mergedHint.Preferred && !bestHint.Preferred {
+		bestHint = compareHints(bestNonPreferredAffinityCount, bestHint, &mergedHint)
-			bestHint = mergedHint
-			return
-		}
-
-		// If the current bestHint is preferred and the new mergedHint is
-		// non-preferred, never update bestHint, regardless of mergedHint's
-		// narowness.
-		if !mergedHint.Preferred && bestHint.Preferred {
-			return
-		}
-
-		// If mergedHint and bestHint has the same preference, only consider
-		// mergedHints that have a narrower NUMANodeAffinity than the
-		// NUMANodeAffinity in the current bestHint.
-		if !mergedHint.NUMANodeAffinity.IsNarrowerThan(bestHint.NUMANodeAffinity) {
-			return
-		}
-
-		// In all other cases, update bestHint to the current mergedHint
-		bestHint = mergedHint
 	})
 
-	return bestHint
+	if bestHint == nil {
+		defaultAffinity, _ := bitmask.NewBitMask(numaNodes...)
+		bestHint = &TopologyHint{defaultAffinity, false}
+	}
+
+	return *bestHint
 }
 
 // Iterate over all permutations of hints in 'allProviderHints [][]TopologyHint'.

pkg/kubelet/cm/topologymanager/policy_test.go

@@ -21,6 +21,7 @@ import (
 	"testing"
 
 	"k8s.io/api/core/v1"
+	"k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/bitmask"
 )
 
 type policyMergeTestCase struct {
@@ -615,6 +616,220 @@ func (p *bestEffortPolicy) mergeTestCases(numaNodes []int) []policyMergeTestCase
 				Preferred:        false,
 			},
 		},
+		{
+			name: "bestNonPreferredAffinityCount (1)",
+			hp: []HintProvider{
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource1": {
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1, 2, 3),
+								Preferred:        false,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource2": {
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+			},
+			expected: TopologyHint{
+				NUMANodeAffinity: NewTestBitMask(0, 1),
+				Preferred:        false,
+			},
+		},
+		{
+			name: "bestNonPreferredAffinityCount (2)",
+			hp: []HintProvider{
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource1": {
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1, 2, 3),
+								Preferred:        false,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource2": {
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 3),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+			},
+			expected: TopologyHint{
+				NUMANodeAffinity: NewTestBitMask(0, 3),
+				Preferred:        false,
+			},
+		},
+		{
+			name: "bestNonPreferredAffinityCount (3)",
+			hp: []HintProvider{
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource1": {
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1, 2, 3),
+								Preferred:        false,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource2": {
+							{
+								NUMANodeAffinity: NewTestBitMask(1, 2),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+			},
+			expected: TopologyHint{
+				NUMANodeAffinity: NewTestBitMask(1, 2),
+				Preferred:        false,
+			},
+		},
+		{
+			name: "bestNonPreferredAffinityCount (4)",
+			hp: []HintProvider{
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource1": {
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1, 2, 3),
+								Preferred:        false,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource2": {
+							{
+								NUMANodeAffinity: NewTestBitMask(2, 3),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+			},
+			expected: TopologyHint{
+				NUMANodeAffinity: NewTestBitMask(2, 3),
+				Preferred:        false,
+			},
+		},
+		{
+			name: "bestNonPreferredAffinityCount (5)",
+			hp: []HintProvider{
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource1": {
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1, 2, 3),
+								Preferred:        false,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource2": {
+							{
+								NUMANodeAffinity: NewTestBitMask(1, 2),
+								Preferred:        false,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(2, 3),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+			},
+			expected: TopologyHint{
+				NUMANodeAffinity: NewTestBitMask(1, 2),
+				Preferred:        false,
+			},
+		},
+		{
+			name: "bestNonPreferredAffinityCount (6)",
+			hp: []HintProvider{
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource1": {
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1, 2, 3),
+								Preferred:        false,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource2": {
+							{
+								NUMANodeAffinity: NewTestBitMask(1, 2, 3),
+								Preferred:        false,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(1, 2),
+								Preferred:        false,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(1, 3),
+								Preferred:        false,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(2, 3),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+			},
+			expected: TopologyHint{
+				NUMANodeAffinity: NewTestBitMask(1, 2),
+				Preferred:        false,
+			},
+		},
 	}
 }
 
@@ -905,3 +1120,283 @@ func testPolicyMerge(policy Policy, tcases []policyMergeTestCase, t *testing.T)
 		}
 	}
 }
+
+func TestMaxOfMinAffinityCounts(t *testing.T) {
+	tcases := []struct {
+		hints    [][]TopologyHint
+		expected int
+	}{
+		{
+			[][]TopologyHint{},
+			0,
+		},
+		{
+			[][]TopologyHint{
+				{
+					TopologyHint{NewTestBitMask(), true},
+				},
+			},
+			0,
+		},
+		{
+			[][]TopologyHint{
+				{
+					TopologyHint{NewTestBitMask(0), true},
+				},
+			},
+			1,
+		},
+		{
+			[][]TopologyHint{
+				{
+					TopologyHint{NewTestBitMask(0, 1), true},
+				},
+			},
+			2,
+		},
+		{
+			[][]TopologyHint{
+				{
+					TopologyHint{NewTestBitMask(0, 1), true},
+					TopologyHint{NewTestBitMask(0, 1, 2), true},
+				},
+			},
+			2,
+		},
+		{
+			[][]TopologyHint{
+				{
+					TopologyHint{NewTestBitMask(0, 1), true},
+					TopologyHint{NewTestBitMask(0, 1, 2), true},
+				},
+				{
+					TopologyHint{NewTestBitMask(0, 1, 2), true},
+				},
+			},
+			3,
+		},
+		{
+			[][]TopologyHint{
+				{
+					TopologyHint{NewTestBitMask(0, 1), true},
+					TopologyHint{NewTestBitMask(0, 1, 2), true},
+				},
+				{
+					TopologyHint{NewTestBitMask(0, 1, 2), true},
+					TopologyHint{NewTestBitMask(0, 1, 2, 3), true},
+				},
+			},
+			3,
+		},
+	}
+
+	for _, tc := range tcases {
+		t.Run("", func(t *testing.T) {
+			result := maxOfMinAffinityCounts(tc.hints)
+			if result != tc.expected {
+				t.Errorf("Expected result to be %v, got %v", tc.expected, result)
+			}
+		})
+	}
+}
+
+func TestCompareHints(t *testing.T) {
+	tcases := []struct {
+		description                   string
+		bestNonPreferredAffinityCount int
+		current                       *TopologyHint
+		candidate                     *TopologyHint
+		expected                      string
+	}{
+		{
+			"candidate.NUMANodeAffinity.Count() == 0 (1)",
+			-1,
+			nil,
+			&TopologyHint{bitmask.NewEmptyBitMask(), false},
+			"current",
+		},
+		{
+			"candidate.NUMANodeAffinity.Count() == 0 (2)",
+			-1,
+			&TopologyHint{NewTestBitMask(), true},
+			&TopologyHint{NewTestBitMask(), false},
+			"current",
+		},
+		{
+			"current == nil (1)",
+			-1,
+			nil,
+			&TopologyHint{NewTestBitMask(0), true},
+			"candidate",
+		},
+		{
+			"current == nil (2)",
+			-1,
+			nil,
+			&TopologyHint{NewTestBitMask(0), false},
+			"candidate",
+		},
+		{
+			"!current.Preferred && candidate.Preferred",
+			-1,
+			&TopologyHint{NewTestBitMask(0), false},
+			&TopologyHint{NewTestBitMask(0), true},
+			"candidate",
+		},
+		{
+			"current.Preferred && !candidate.Preferred",
+			-1,
+			&TopologyHint{NewTestBitMask(0), true},
+			&TopologyHint{NewTestBitMask(0), false},
+			"current",
+		},
+		{
+			"current.Preferred && candidate.Preferred (1)",
+			-1,
+			&TopologyHint{NewTestBitMask(0), true},
+			&TopologyHint{NewTestBitMask(0), true},
+			"current",
+		},
+		{
+			"current.Preferred && candidate.Preferred (2)",
+			-1,
+			&TopologyHint{NewTestBitMask(0, 1), true},
+			&TopologyHint{NewTestBitMask(0), true},
+			"candidate",
+		},
+		{
+			"current.Preferred && candidate.Preferred (3)",
+			-1,
+			&TopologyHint{NewTestBitMask(0), true},
+			&TopologyHint{NewTestBitMask(0, 1), true},
+			"current",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (1.1)",
+			1,
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			"current",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (1.2)",
+			1,
+			&TopologyHint{NewTestBitMask(1, 2), false},
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			"candidate",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (1.3)",
+			1,
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			&TopologyHint{NewTestBitMask(1, 2), false},
+			"current",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (2.1)",
+			2,
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			&TopologyHint{NewTestBitMask(0), false},
+			"current",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (2.2)",
+			2,
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			"current",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (2.3)",
+			2,
+			&TopologyHint{NewTestBitMask(1, 2), false},
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			"candidate",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (2.4)",
+			2,
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			&TopologyHint{NewTestBitMask(1, 2), false},
+			"current",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (3a)",
+			2,
+			&TopologyHint{NewTestBitMask(0), false},
+			&TopologyHint{NewTestBitMask(0, 1, 2), false},
+			"current",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (3b)",
+			2,
+			&TopologyHint{NewTestBitMask(0), false},
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			"candidate",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (3ca.1)",
+			3,
+			&TopologyHint{NewTestBitMask(0), false},
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			"candidate",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (3ca.2)",
+			3,
+			&TopologyHint{NewTestBitMask(0), false},
+			&TopologyHint{NewTestBitMask(1, 2), false},
+			"candidate",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (3ca.3)",
+			4,
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			&TopologyHint{NewTestBitMask(1, 2, 3), false},
+			"candidate",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (3cb)",
+			4,
+			&TopologyHint{NewTestBitMask(1, 2, 3), false},
+			&TopologyHint{NewTestBitMask(0, 1), false},
+			"current",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (3cc.1)",
+			4,
+			&TopologyHint{NewTestBitMask(0, 1, 2), false},
+			&TopologyHint{NewTestBitMask(0, 1, 2), false},
+			"current",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (3cc.2)",
+			4,
+			&TopologyHint{NewTestBitMask(0, 1, 2), false},
+			&TopologyHint{NewTestBitMask(1, 2, 3), false},
+			"current",
+		},
+		{
+			"!current.Preferred && !candidate.Preferred (3cc.3)",
+			4,
+			&TopologyHint{NewTestBitMask(1, 2, 3), false},
+			&TopologyHint{NewTestBitMask(0, 1, 2), false},
+			"candidate",
+		},
+	}
+
+	for _, tc := range tcases {
+		t.Run(tc.description, func(t *testing.T) {
+			result := compareHints(tc.bestNonPreferredAffinityCount, tc.current, tc.candidate)
+			if result != tc.current && result != tc.candidate {
+				t.Errorf("Expected result to be either 'current' or 'candidate' hint")
+			}
+			if tc.expected == "current" && result != tc.current {
+				t.Errorf("Expected result to be %v, got %v", tc.current, result)
+			}
+			if tc.expected == "candidate" && result != tc.candidate {
+				t.Errorf("Expected result to be %v, got %v", tc.candidate, result)
+			}
+		})
+	}
+}