Burstable pods with unlimited memory should respect unlimitedBurstable

cmd/plugins/topology-aware/policy/pools.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
 			}
@@ -1136,14 +1137,46 @@ func (p *policy) compareScores(request Request, pools []Node, scores map[int]Sco
 				if level2.Value() > target.Value() && level1.Value() < target.Value() {
 					log.Debugf("  - %s WINS on CPU burstability limit (%s, limit %s)",
 						node2.Name(), level2, target)
-					return true
+					return false
 				}
 				log.Debugf("  - CPU burstability limit is a TIE")
 				return id1 < id2
 			}
 		}
 	}
 
+	// 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_test.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)
+			}
+		})
+	}
+}

test/e2e/policies.test-suite/topology-aware/n4c16/test14-burstable/code.var.sh

@@ -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"