From 5501f542cddccab3b27196df49f3d30f87e51ca9 Mon Sep 17 00:00:00 2001
From: Kevin Klues <kklues@nvidia.com>
Date: Wed, 9 Oct 2019 15:47:27 -0700
Subject: [PATCH] Fixed bug in TopologyManager with SingleNUMANode Policy

This patch fixes an issue in the TopologyManager that wouldn't allow
pods to be admitted if pods were launched with the SingleNUMANode policy
and any of the hint providers had no NUMA preferences.

This is due to 2 factors:

1) Any hint provider that passes back a `nil` as its hints, has its hint
automatically transformed into a single {11 true} hint before merging

2) We added a special casing for the SingleNumaNodePolicy() in the
TopologyManager that essentially turns these hints into a
{11 false} anytime a {11 true} is seen.

The current patch reworks this logic so the that TopologyManager can
tell the difference between a "don't care" hint and a true "{11 true}"
hint returned by the hint provider. Only true "{11 true}" hints will be
converted by the special casing for the SingleNumaNodePolicy(), while
"don't care" hints will not.

This is a short term fix for this issue until we do a larger refactoring
of this code for the 1.17 release.
---
 .../cm/topologymanager/topology_manager.go    | 29 ++++++++--------
 .../topologymanager/topology_manager_test.go  | 33 ++++++++++++++++++-
 2 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go
index f8e7c662f23..3b1718ef114 100644
--- a/pkg/kubelet/cm/topologymanager/topology_manager.go
+++ b/pkg/kubelet/cm/topologymanager/topology_manager.go
@@ -201,7 +201,7 @@ func (m *manager) calculateAffinity(pod v1.Pod, container v1.Container) Topology
 		// 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
 		}
 
@@ -209,13 +209,13 @@ func (m *manager) calculateAffinity(pod v1.Pod, container v1.Container) Topology
 		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
 			}
 
@@ -235,18 +235,21 @@ func (m *manager) calculateAffinity(pod v1.Pod, container v1.Container) Topology
 		preferred := true
 		var numaAffinities []bitmask.BitMask
 		for _, hint := range permutation {
-			// Only consider hints that have an actual NUMANodeAffinity set.
-			if hint.NUMANodeAffinity != nil {
-				if !hint.Preferred {
-					preferred = false
-				}
-				// Special case PolicySingleNumaNode to only prefer hints where
-				// all providers have a single NUMA affinity set.
-				if m.policy != nil && m.policy.Name() == PolicySingleNumaNode && hint.NUMANodeAffinity.Count() > 1 {
-					preferred = false
-				}
+			if hint.NUMANodeAffinity == nil {
+				numaAffinities = append(numaAffinities, defaultAffinity)
+			} else {
 				numaAffinities = append(numaAffinities, hint.NUMANodeAffinity)
 			}
+
+			if !hint.Preferred {
+				preferred = false
+			}
+
+			// Special case PolicySingleNumaNode to only prefer hints where
+			// all providers have a single NUMA affinity set.
+			if m.policy != nil && m.policy.Name() == PolicySingleNumaNode && hint.NUMANodeAffinity != nil && hint.NUMANodeAffinity.Count() > 1 {
+				preferred = false
+			}
 		}
 
 		// Merge the affinities using a bitwise-and operation.
diff --git a/pkg/kubelet/cm/topologymanager/topology_manager_test.go b/pkg/kubelet/cm/topologymanager/topology_manager_test.go
index ecec3e71b33..826c89c2765 100644
--- a/pkg/kubelet/cm/topologymanager/topology_manager_test.go
+++ b/pkg/kubelet/cm/topologymanager/topology_manager_test.go
@@ -196,7 +196,7 @@ func TestCalculateAffinity(t *testing.T) {
 			},
 			expected: TopologyHint{
 				NUMANodeAffinity: NewTestBitMask(numaNodes...),
-				Preferred:        true,
+				Preferred:        false,
 			},
 		},
 		{
@@ -690,6 +690,37 @@ func TestCalculateAffinity(t *testing.T) {
 				Preferred:        false,
 			},
 		},
+		{
+			name:   "Special cased PolicySingleNumaNode with one no-preference provider",
+			policy: NewSingleNumaNodePolicy(),
+			hp: []HintProvider{
+				&mockHintProvider{
+					map[string][]TopologyHint{
+						"resource1": {
+							{
+								NUMANodeAffinity: NewTestBitMask(0),
+								Preferred:        true,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(1),
+								Preferred:        true,
+							},
+							{
+								NUMANodeAffinity: NewTestBitMask(0, 1),
+								Preferred:        false,
+							},
+						},
+					},
+				},
+				&mockHintProvider{
+					nil,
+				},
+			},
+			expected: TopologyHint{
+				NUMANodeAffinity: NewTestBitMask(0),
+				Preferred:        true,
+			},
+		},
 	}
 
 	for _, tc := range tcases {