From 76ee2c2300d15f713a5275886326f01641dcb807 Mon Sep 17 00:00:00 2001 From: Antti Kervinen Date: Mon, 21 Oct 2024 15:49:03 +0300 Subject: [PATCH 1/4] balloons: add a balloon instance filter function and refactor Signed-off-by: Antti Kervinen --- .../balloons/policy/balloons-policy.go | 55 +++++++++---------- 1 file changed, 25 insertions(+), 30 deletions(-) diff --git a/cmd/plugins/balloons/policy/balloons-policy.go b/cmd/plugins/balloons/policy/balloons-policy.go index 4a85e4932..6f7183d98 100644 --- a/cmd/plugins/balloons/policy/balloons-policy.go +++ b/cmd/plugins/balloons/policy/balloons-policy.go @@ -337,62 +337,57 @@ func (p *balloons) balloonByContainer(c cache.Container) *Balloon { return nil } -// balloonsByGroup returns balloons that contain containers on which -// balloon's GroupBy expression evaluates given group. -func (p *balloons) balloonsByGroup(group string) []*Balloon { +// balloonsByFunc returns balloons for which the callback function +// returns true. +func balloonsByFunc(balloons []*Balloon, f func(*Balloon) bool) []*Balloon { blns := []*Balloon{} - for _, bln := range p.balloons { - if bln.Groups[group] > 0 { + for _, bln := range balloons { + if f(bln) { blns = append(blns, bln) } } return blns } +// balloonsByGroup returns balloons that contain containers on which +// balloon's GroupBy expression evaluates given group. +func (p *balloons) balloonsByGroup(group string) []*Balloon { + return balloonsByFunc(p.balloons, func(bln *Balloon) bool { + return bln.Groups[group] > 0 + }) +} + // balloonsByNamespace returns balloons that contain containers in a // namespace. func (p *balloons) balloonsByNamespace(namespace string) []*Balloon { - blns := []*Balloon{} - for _, bln := range p.balloons { + return balloonsByFunc(p.balloons, func(bln *Balloon) bool { for podID, ctrIDs := range bln.PodIDs { if len(ctrIDs) == 0 { continue } - pod, ok := p.cch.LookupPod(podID) - if !ok { - continue - } - if pod.GetNamespace() == namespace { - blns = append(blns, bln) - break + if pod, ok := p.cch.LookupPod(podID); ok && pod.GetNamespace() == namespace { + return true } } - } - return blns + return false + }) } // balloonsByPod returns balloons that contain any container of a pod. func (p *balloons) balloonsByPod(pod cache.Pod) []*Balloon { podID := pod.GetID() - blns := []*Balloon{} - for _, bln := range p.balloons { - if _, ok := bln.PodIDs[podID]; ok { - blns = append(blns, bln) - } - } - return blns + return balloonsByFunc(p.balloons, func(bln *Balloon) bool { + _, ok := bln.PodIDs[podID] + return ok + }) } // balloonsByDef returns list of balloons instantiated from a balloon // definition. func (p *balloons) balloonsByDef(blnDef *BalloonDef) []*Balloon { - balloons := []*Balloon{} - for _, balloon := range p.balloons { - if balloon.Def == blnDef { - balloons = append(balloons, balloon) - } - } - return balloons + return balloonsByFunc(p.balloons, func(bln *Balloon) bool { + return bln.Def == blnDef + }) } // balloonDefByName returns a balloon definition with a name. From 5ba846d80f9c77a7b06db039db0e0b70dfe449f5 Mon Sep 17 00:00:00 2001 From: Antti Kervinen Date: Mon, 21 Oct 2024 17:01:30 +0300 Subject: [PATCH 2/4] balloons: consider container count when choosing instance - Change chooseBalloonInstance() to fillableBalloonInstances() returning multiple balloon instances that can be chosen. - Add a separate logic for choosing a balloon instance among fillable ones. This changes current container-to-balloon assigning behavior. Instead of choosing the first balloon instance where a container fits, now choose the one with most room in it, and secondly, least containers in it. This fixes the issue in the earlier logic that caused containers without CPU requests to always pile up in the same balloons. Now they are spread evenly to several balloons that have the same amount of free CPUs. Signed-off-by: Antti Kervinen --- .../balloons/policy/balloons-policy.go | 110 +++++++++++------- 1 file changed, 65 insertions(+), 45 deletions(-) diff --git a/cmd/plugins/balloons/policy/balloons-policy.go b/cmd/plugins/balloons/policy/balloons-policy.go index 6f7183d98..9536770d6 100644 --- a/cmd/plugins/balloons/policy/balloons-policy.go +++ b/cmd/plugins/balloons/policy/balloons-policy.go @@ -16,6 +16,7 @@ package balloons import ( "fmt" + "math" "path/filepath" "strconv" @@ -477,20 +478,28 @@ func (p *balloons) maxFreeMilliCpus(bln *Balloon) int { return bln.MaxAvailMilliCpus(p.freeCpus) - p.requestedMilliCpus(bln) } -// largest helps finding the largest element and value in a slice. -// Input the length of a slice and a function that returns the +// largest helps finding largest elements and the largest value in a +// slice. Input the length of a slice and a function that returns the // magnitude of given element in the slice as int. -func largest(sliceLen int, valueOf func(i int) int) (int, int) { - largestIndex := -1 - largestValue := 0 +func largest(sliceLen int, valueOf func(i int) int) ([]int, int) { + largestIndices := []int{} + // the largest value found so far is the smallest number that + // can be presented with int: + largestValue := math.MinInt for index := 0; index < sliceLen; index++ { value := valueOf(index) - if largestIndex == -1 || value > largestValue { - largestIndex = index + switch { + case len(largestIndices) == 0: + largestIndices = append(largestIndices, index) + largestValue = value + case value == largestValue: + largestIndices = append(largestIndices, index) + case value > largestValue: + largestIndices = []int{index} largestValue = value } } - return largestIndex, largestValue + return largestIndices, largestValue } // resetCpuClass resets CPU configurations globally. All balloons can @@ -646,7 +655,7 @@ func (p *balloons) freeBalloon(bln *Balloon) { } } -func (p *balloons) chooseBalloonInstance(blnDef *BalloonDef, fm FillMethod, c cache.Container) (*Balloon, error) { +func (p *balloons) fillableBalloonInstances(blnDef *BalloonDef, fm FillMethod, c cache.Container) ([]*Balloon, error) { reqMilliCpus := p.containerRequestedMilliCpus(c.GetID()) switch fm { case FillNewBalloon, FillNewBalloonMust: @@ -654,7 +663,7 @@ func (p *balloons) chooseBalloonInstance(blnDef *BalloonDef, fm FillMethod, c ca // preferred over instantiating a new balloon. for _, bln := range p.balloonsByDef(blnDef) { if len(bln.PodIDs) == 0 { - return bln, nil + return []*Balloon{bln}, nil } } newBln, err := p.newBalloon(blnDef, false) @@ -712,34 +721,30 @@ func (p *balloons) chooseBalloonInstance(blnDef *BalloonDef, fm FillMethod, c ca // more idle. p.updatePinning(p.shareIdleCpus(p.freeCpus, newBln.Cpus)...) } - return newBln, nil + return []*Balloon{newBln}, nil case FillSameGroup: group, err := c.Expand(blnDef.GroupBy, true) if err != nil { log.Errorf("error choosing balloon for container %q based on groupBy: %s", c.PrettyName(), err) return nil, nil } - for _, bln := range p.balloonsByGroup(group) { - if bln.Def == blnDef && p.maxFreeMilliCpus(bln) >= reqMilliCpus { - return bln, nil - } - } - return nil, nil + return balloonsByFunc(p.balloons, + func(bln *Balloon) bool { + return bln.Groups[group] > 0 && + bln.Def == blnDef && + p.maxFreeMilliCpus(bln) >= reqMilliCpus + }), nil case FillSameNamespace: - for _, bln := range p.balloonsByNamespace(c.GetNamespace()) { - if bln.Def == blnDef && p.maxFreeMilliCpus(bln) >= reqMilliCpus { - return bln, nil - } - } - return nil, nil + return balloonsByFunc(p.balloonsByNamespace(c.GetNamespace()), + func(bln *Balloon) bool { + return bln.Def == blnDef && p.maxFreeMilliCpus(bln) >= reqMilliCpus + }), nil case FillSamePod: if pod, ok := c.GetPod(); ok { - for _, bln := range p.balloonsByPod(pod) { - if p.maxFreeMilliCpus(bln) >= reqMilliCpus { - return bln, nil - } - } - return nil, nil + return balloonsByFunc(p.balloonsByPod(pod), + func(bln *Balloon) bool { + return bln.Def == blnDef && p.maxFreeMilliCpus(bln) >= reqMilliCpus + }), nil } else { return nil, balloonsError("fill method %s failed: cannot find pod for container %s", fm, c.PrettyName()) } @@ -754,21 +759,15 @@ func (p *balloons) chooseBalloonInstance(blnDef *BalloonDef, fm FillMethod, c ca case FillBalanced: // Are there balloons where the container would fit // without inflating the balloon? - blnIdx, freeMilliCpus := largest(len(balloons), func(i int) int { - return p.freeMilliCpus(balloons[i]) - }) - if freeMilliCpus >= reqMilliCpus { - return balloons[blnIdx], nil - } + return balloonsByFunc(balloons, func(bln *Balloon) bool { + return p.freeMilliCpus(bln) >= reqMilliCpus + }), nil case FillBalancedInflate: // Are there balloons where the container would fit // after inflating the balloon? - blnIdx, maxFreeMilliCpus := largest(len(balloons), func(i int) int { - return p.maxFreeMilliCpus(balloons[i]) - }) - if maxFreeMilliCpus >= reqMilliCpus { - return balloons[blnIdx], nil - } + return balloonsByFunc(balloons, func(bln *Balloon) bool { + return p.maxFreeMilliCpus(bln) >= reqMilliCpus + }), nil default: return nil, balloonsError("balloon type fill method not implemented: %s", fm) } @@ -825,17 +824,38 @@ func (p *balloons) allocateBalloonOfDef(blnDef *BalloonDef, c cache.Container) ( fillChain = append(fillChain, FillBalanced, FillBalancedInflate, FillNewBalloon) } for _, fillMethod := range fillChain { - bln, err := p.chooseBalloonInstance(blnDef, fillMethod, c) + blns, err := p.fillableBalloonInstances(blnDef, fillMethod, c) if err != nil { log.Debugf("fill method %q prevents allocation: %w", fillMethod, err) return nil, err } - if bln == nil { + if len(blns) == 0 { log.Debugf("fill method %q not applicable", fillMethod) continue } - log.Debugf("fill method %q suggests balloon instance %v", fillMethod, bln) - return bln, nil + log.Debugf("fill method %q suggests any of balloon instances %v", fillMethod, blns) + + // TODO: Consider: in case of a best effort container, + // choose the balloon with the least number of + // containers assigned to it. This avoids piling up + // all best efforts to a balloon that has least CPU + // reservations on it. + + // Choose the balloon with the most free CPUs. If + // there are equally good candidates, choose the one + // with the lowest number of containers assigned. + largestBy := p.freeMilliCpus + if fillMethod == FillBalancedInflate { + largestBy = p.maxFreeMilliCpus + } + mostRoom, _ := largest(len(blns), func(i int) int { + return largestBy(blns[i]) + }) + leastContainers, _ := largest(len(mostRoom), func(i int) int { + return -blns[mostRoom[i]].ContainerCount() + }) + bestBln := blns[mostRoom[leastContainers[0]]] + return bestBln, nil } return nil, nil } From 4ebebd67c43c9614b276f5cdfaaa3f4b0dbcc162 Mon Sep 17 00:00:00 2001 From: Antti Kervinen Date: Tue, 22 Oct 2024 14:56:27 +0300 Subject: [PATCH 3/4] balloons: refuse to create a new balloon without sufficient CPUs If a new balloon is created for a container that has no CPU requests, CPU allocation of "requested" 0 CPUs succeeds without allocating anything. That would lead to a balloon with empty cpuset, that is, no CPU pinning for containers in it. Signed-off-by: Antti Kervinen --- cmd/plugins/balloons/policy/balloons-policy.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cmd/plugins/balloons/policy/balloons-policy.go b/cmd/plugins/balloons/policy/balloons-policy.go index 9536770d6..5fd2ea3ba 100644 --- a/cmd/plugins/balloons/policy/balloons-policy.go +++ b/cmd/plugins/balloons/policy/balloons-policy.go @@ -666,6 +666,16 @@ func (p *balloons) fillableBalloonInstances(blnDef *BalloonDef, fm FillMethod, c return []*Balloon{bln}, nil } } + // Creating a new balloon and placing a container + // (even a best effort one) to it always requires at + // least one CPU. Make sure this is doable. + if p.freeCpus.Size() == 0 || p.freeCpus.Size() < blnDef.MinCpus { + if fm == FillNewBalloonMust { + return nil, balloonsError("not enough CPUs to create new balloon for container %s requesting %s mCPU. free CPUs: %s", + c.PrettyName(), reqMilliCpus, p.freeCpus.Size()) + } + return nil, nil + } newBln, err := p.newBalloon(blnDef, false) if err != nil { if fm == FillNewBalloonMust { From d9f33f8709a079f28a1133b51379df0950189d55 Mon Sep 17 00:00:00 2001 From: Antti Kervinen Date: Tue, 22 Oct 2024 15:06:04 +0300 Subject: [PATCH 4/4] e2e: add a test for spreading best effort containers to balloons Signed-off-by: Antti Kervinen --- .../n4c16/test01-basic-placement/code.var.sh | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/test/e2e/policies.test-suite/balloons/n4c16/test01-basic-placement/code.var.sh b/test/e2e/policies.test-suite/balloons/n4c16/test01-basic-placement/code.var.sh index d8b2b7129..297da9440 100644 --- a/test/e2e/policies.test-suite/balloons/n4c16/test01-basic-placement/code.var.sh +++ b/test/e2e/policies.test-suite/balloons/n4c16/test01-basic-placement/code.var.sh @@ -70,6 +70,37 @@ verify 'disjoint_sets(cpus["pod5c0"], cpus["pod5c1"], cpus["pod5c2"])' \ 'len(cpus["pod5c1"]) == 2' \ 'len(cpus["pod5c2"]) == 2' +# pod6: ensure that new balloons are created to best effort containers +# when the balloon type prefers new balloons. First, each container +# gets its own balloon, even if there is only one best effort +# container running in an existing balloon. +CPUREQ="" MEMREQ="" CPULIM="" MEMLIM="" +POD_ANNOTATION="balloon.balloons.resource-policy.nri.io: five-cpu" CONTCOUNT=4 create balloons-busybox +report allowed + +verify 'set([len(cpus[ctr]) for ctr in cpus if ctr.startswith("pod6c")]) == {1}' \ + 'disjoint_sets(*[cpus[ctr] for ctr in cpus if ctr.startswith("pod6c")])' \ + 'disjoint_sets( + set.union(*[cpus[ctr] for ctr in cpus if ctr.startswith("pod6c")]), + set.union(*[cpus[ctr] for ctr in cpus if ctr.startswith("pod5c")]))' + +# pod7: ...continuing the previous: when it's not possible to create +# anymore new balloons to new best effort containers. Start filling +# existing balloons. Balloons of pod5c* containers have only 750 mCPU +# free, so they should not be used. Balloons of pod6c* containers have +# 1000 mCPU free, so pod7c* containers should be spread evenly into +# them. +CPUREQ="" MEMREQ="" CPULIM="" MEMLIM="" +POD_ANNOTATION="balloon.balloons.resource-policy.nri.io: five-cpu" CONTCOUNT=4 create balloons-busybox +report allowed + +verify 'set([len(cpus[ctr]) for ctr in cpus if ctr.startswith("pod7c")]) == {1}' \ + 'disjoint_sets(cpus["pod7c0"], cpus["pod7c1"], cpus["pod7c2"], cpus["pod7c3"])' \ + 'max([len(cpuset) for pod, cpuset in cpus.items()]) == 2' \ + 'disjoint_sets( + set.union(*[cpus[ctr] for ctr in cpus if ctr.startswith("pod7c")]), + set.union(*[cpus[ctr] for ctr in cpus if ctr.startswith("pod5c")]))' + cleanup helm-terminate