diff --git a/cmd/plugins/topology-aware/policy/e2eish-gen-n4c16_test.go b/cmd/plugins/topology-aware/policy/e2eish-gen-n4c16_test.go index 3f08fe906..8f5a7fe6a 100644 --- a/cmd/plugins/topology-aware/policy/e2eish-gen-n4c16_test.go +++ b/cmd/plugins/topology-aware/policy/e2eish-gen-n4c16_test.go @@ -712,7 +712,7 @@ func TestE2eishGenN4c16Test14Burstable(t *testing.T) { require.Len(t, s.Nodes("pod0c0"), 2) require.Len(t, s.Nodes("pod1c0"), 4) require.Len(t, s.Nodes("pod2c0"), 2) - require.Len(t, s.Nodes("pod3c0"), 4) + require.Len(t, s.Nodes("pod3c0"), 2) s.DeleteAllPodsIn("default") s.Terminate() s.Launch(genN4c16, genN4c16BaseConfig) diff --git a/cmd/plugins/topology-aware/policy/pools.go b/cmd/plugins/topology-aware/policy/pools.go index dc7070093..3efab7af4 100644 --- a/cmd/plugins/topology-aware/policy/pools.go +++ b/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,7 +1137,7 @@ 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 @@ -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() diff --git a/cmd/plugins/topology-aware/policy/pools_test.go b/cmd/plugins/topology-aware/policy/pools_test.go index 644ddca07..344702bb1 100644 --- a/cmd/plugins/topology-aware/policy/pools_test.go +++ b/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) + } + }) + } +} diff --git a/test/e2e/policies.test-suite/topology-aware/n4c16/test14-burstable/code.var.sh b/test/e2e/policies.test-suite/topology-aware/n4c16/test14-burstable/code.var.sh index bc7e61168..328e3d59b 100644 --- a/test/e2e/policies.test-suite/topology-aware/n4c16/test14-burstable/code.var.sh +++ b/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"