From 155562dd2e3e9ad32a77c3df7b86ece7b3499a34 Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Thu, 10 Feb 2022 16:31:05 +0000 Subject: [PATCH] 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 --- pkg/kubelet/cm/topologymanager/policy.go | 14 +++--- .../policy_best_effort_test.go | 2 +- .../topologymanager/policy_restricted_test.go | 2 +- .../policy_single_numa_node_test.go | 2 +- pkg/kubelet/cm/topologymanager/policy_test.go | 43 +++++++++++++++++-- 5 files changed, 51 insertions(+), 12 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/policy.go b/pkg/kubelet/cm/topologymanager/policy.go index b548e4afdbe5..231d9e3f8aaa 100644 --- a/pkg/kubelet/cm/topologymanager/policy.go +++ b/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} } diff --git a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go b/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go index 3b23197920e0..43bcc6ca933d 100644 --- a/pkg/kubelet/cm/topologymanager/policy_best_effort_test.go +++ b/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) diff --git a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go b/pkg/kubelet/cm/topologymanager/policy_restricted_test.go index b8174f3cbbd1..d26231992089 100644 --- a/pkg/kubelet/cm/topologymanager/policy_restricted_test.go +++ b/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) 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 c6e86dba409b..f2303e72a801 100644 --- a/pkg/kubelet/cm/topologymanager/policy_single_numa_node_test.go +++ b/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) diff --git a/pkg/kubelet/cm/topologymanager/policy_test.go b/pkg/kubelet/cm/topologymanager/policy_test.go index d106e0620920..1cc4f21d0cf8 100644 --- a/pkg/kubelet/cm/topologymanager/policy_test.go +++ b/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, }, }, }