topology-aware: respect unlimitedBurstable for unlimited memory.

A burstable container with a memory request but no memory limit always
ended up in the root pool. The memory burstability check ran before the
CPU burstability check and, for a zero limit, preferred the pool with
the largest memory capacity. That is always the root, so the
unlimitedBurstable setting (and the per-container annotation) never got
a say. A pod requesting 1 CPU and 1G of memory with no limits was
pinned to the shared cpuset of the whole machine and had its memory
spread over every NUMA node.

Keep the capacity check for containers that have a memory limit. For
unlimited memory, decide after the CPU burstability check: a pool at the
configured unlimitedBurstable topology level wins, and only for a tie do
we prefer the pool with more memory capacity.

Add a unit test for the placement. Change test14-burstable accordingly,
since pod3 (memory limit 0) now gets one socket instead of all four NUMA
nodes by default.

Author: kad

cmd/plugins/topology-aware/policy/e2eish-gen-n4c16_test.go

@@ -1055,13 +1055,14 @@ func (p *policy) compareScores(request Request, pools []Node, scores map[int]Sco
 		}
 		log.Debugf("  - offer memory types are a tie (%s vs %s)", t1, t2)
 
-		if req, lim := request.MemAmountToAllocate(), request.MemoryLimit(); req != lim {
+		// Unlimited memory (lim == 0) is handled below, after CPU burstability.
+		if req, lim := request.MemAmountToAllocate(), request.MemoryLimit(); req != lim && lim != 0 {
 			capa1, capa2 := p.poolZoneCapacity(node1, memType), p.poolZoneCapacity(node2, memType)
-			if (lim != 0 && capa1 >= lim && capa2 < lim) || (lim == 0 && capa1 > capa2) {
+			if capa1 >= lim && capa2 < lim {
 				log.Debugf("   - %s loses on memory offer burstability", node2.Name())
 				return true
 			}
-			if (lim != 0 && capa1 < lim && capa2 >= lim) || (lim == 0 && capa2 > capa1) {
+			if capa1 < lim && capa2 >= lim {
 				log.Debugf("   - %s loses on memory offer burstability", node1.Name())
 				return false
 			}
@@ -1144,6 +1145,38 @@ func (p *policy) compareScores(request Request, pools []Node, scores map[int]Sco
 		}
 	}
 
+	// for unlimited memory, prefer a pool at the unlimited burstable topology
+	// level, then the pool with more memory capacity
+	if req, lim := request.MemAmountToAllocate(), request.MemoryLimit(); lim == 0 && req != lim && o1 != nil && o2 != nil {
+		var (
+			ctr     = request.GetContainer()
+			target  = node1.Policy().unlimitedBurstablePreference(ctr)
+			level1  = node1.Kind().TopologyLevel()
+			level2  = node2.Kind().TopologyLevel()
+			memType = request.MemoryType()
+		)
+
+		switch {
+		case level1 == target && level2 != target:
+			log.Debugf("  - %s WINS on unlimited memory topology level (%s)", node1.Name(), target)
+			return true
+		case level2 == target && level1 != target:
+			log.Debugf("  - %s WINS on unlimited memory topology level (%s)", node2.Name(), target)
+			return false
+		}
+
+		capa1, capa2 := p.poolZoneCapacity(node1, memType), p.poolZoneCapacity(node2, memType)
+		if capa1 > capa2 {
+			log.Debugf("   - %s loses on unlimited memory capacity", node2.Name())
+			return true
+		}
+		if capa2 > capa1 {
+			log.Debugf("   - %s loses on unlimited memory capacity", node1.Name())
+			return false
+		}
+		log.Debugf("  - unlimited memory capacity is a TIE")
+	}
+
 	// tighter memory offer wins (skip if both offers are nil)
 	if o1 != nil && o2 != nil {
 		m1, m2 := o1.NodeMask(), o2.NodeMask()

cmd/plugins/topology-aware/policy/pools.go

@@ -21,9 +21,12 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"strings"
 	"sync"
 	"testing"
 
+	corev1 "k8s.io/api/core/v1"
+
 	cfgapi "github.com/containers/nri-plugins/pkg/apis/config/v1alpha1/resmgr/policy/topologyaware"
 	policyapi "github.com/containers/nri-plugins/pkg/resmgr/policy"
 
@@ -619,3 +622,61 @@ func TestAffinities(t *testing.T) {
 		})
 	}
 }
+
+// Test that a burstable container without memory and CPU limits is placed
+// at the unlimitedBurstable topology level, instead of the largest pool.
+func TestUnlimitedBurstablePlacement(t *testing.T) {
+	dir := testSysfs(t)
+
+	tcases := []struct {
+		name     string
+		level    cfgapi.CPUTopologyLevel
+		expected string
+	}{
+		{name: "package level", level: cfgapi.CPUTopologyLevelPackage, expected: "socket #"},
+		{name: "system level", level: cfgapi.CPUTopologyLevelSystem, expected: "root"},
+	}
+
+	for _, tc := range tcases {
+		t.Run(tc.name, func(t *testing.T) {
+			sys, err := system.DiscoverSystemAt(path.Join(dir, "sysfs", "4-socket-server-nosnc", "sys"))
+			if err != nil {
+				panic(err)
+			}
+
+			policy := New().(*policy)
+			err = policy.Setup(&policyapi.BackendOptions{
+				Cache:  &mockCache{},
+				System: sys,
+				Config: &cfgapi.Config{
+					ReservedResources:  cfgapi.Constraints{cfgapi.CPU: "750m"},
+					UnlimitedBurstable: tc.level,
+				},
+			})
+			if err != nil {
+				log.Warnf("failed to setup test policy: %v", err)
+			}
+
+			req := &request{
+				memReq:   1 << 30,
+				memLim:   0,
+				memType:  memoryDRAM,
+				fraction: 1000,
+				limit:    unlimitedCPU,
+				container: &mockContainer{
+					returnValueForQOSClass: corev1.PodQOSBurstable,
+				},
+			}
+
+			log.EnableDebug(true)
+			_, pools := policy.sortPoolsByScore(req, map[int]int32{})
+			log.EnableDebug(false)
+			if len(pools) < 1 {
+				t.Fatalf("pool scoring failed to find any pools")
+			}
+			if name := pools[0].Name(); !strings.HasPrefix(name, tc.expected) {
+				t.Errorf("expected best pool %q*, got %s", tc.expected, name)
+			}
+		})
+	}
+}

cmd/plugins/topology-aware/policy/pools_test.go

@@ -15,14 +15,14 @@ CPUREQ=1 CPULIM=2 MEMREQ=100M MEMLIM=2.5G create burstable
 CPUREQ=1 CPULIM=2 MEMREQ=200M MEMLIM=0 create burstable
 
 # pod0 and pod2 have limits that require 2 NUMA nodes.
-# pod1 requires all 4 NUMA nodes. pod3 is unlimited, so
-# it should also get all NUMA nodes.
+# pod1 requires all 4 NUMA nodes. pod3 has no memory limit, so
+# it is placed at the unlimitedBurstable level, by default a socket.
 report allowed
 verify \
     'len(nodes["pod0c0"]) == 2' \
     'len(nodes["pod1c0"]) == 4' \
     'len(nodes["pod2c0"]) == 2' \
-    'len(nodes["pod3c0"]) == 4'
+    'len(nodes["pod3c0"]) == 2'
 
 vm-command "kubectl delete pods --all --now"