-
Notifications
You must be signed in to change notification settings - Fork 802
fix(device): enforce ResourceQuota in Fit() for all backends #2397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -428,6 +428,12 @@ func (neuron *AWSNeuronDevices) Fit(devices []*device.DeviceUsage, request devic | |
| continue | ||
| } | ||
|
|
||
| if !device.FitQuotaForDevice(tmpDevs, allocated, pod.Namespace, int64(k.Memreq), int64(k.Coresreq), AWSNeuronDevice, neuron.GetResourceNames()) { | ||
| reason[common.ResourceQuotaNotFit]++ | ||
| klog.V(3).InfoS(common.ResourceQuotaNotFit, "pod", pod.Name, "memreq", k.Memreq, "coresreq", k.Coresreq) | ||
| continue | ||
| } | ||
|
|
||
|
Comment on lines
+431
to
+436
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Check quota before the multi-device return. The Validate the complete selected allocation before the return. Include temporary and previously allocated devices in that validation. 🤖 Prompt for AI Agents |
||
| if countMaskAvailable(dev.Totalcore)-countMaskAvailable(dev.Usedcores) < k.Coresreq { | ||
| reason[common.CardInsufficientCore]++ | ||
| klog.V(5).InfoS(common.CardInsufficientCore, "pod", klog.KObj(pod), "device", dev.ID, "device index", i, "device total core", dev.Totalcore, "device used core", dev.Usedcores, "request cores", k.Coresreq) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -431,6 +431,11 @@ func (enf *EnflameDevices) Fit(devices []*device.DeviceUsage, request device.Con | |
| klog.V(5).InfoS(common.ExclusiveDeviceAllocateConflict, "pod", klog.KObj(pod), "device", dev.ID, "device index", i, "used", dev.Used) | ||
| continue | ||
| } | ||
| if !device.FitQuotaForDevice(tmpDevs, allocated, pod.Namespace, int64(profileMemoryMiB), int64(profileCorePercent), EnflameVGCUDevice, enf.GetResourceNames()) { | ||
| reason[common.ResourceQuotaNotFit]++ | ||
| klog.V(3).InfoS(common.ResourceQuotaNotFit, "pod", pod.Name, "memreq", profileMemoryMiB, "coresreq", profileCorePercent) | ||
| continue | ||
| } | ||
|
Comment on lines
+434
to
+438
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 5 \
'MemoryFactor|profileMemoryMiB|EnflameResourceNameGCUMemory|FitQuotaForDevice' \
pkg/device/enflame/device.go pkg/device/quota.go pkg/device/quota_test.goRepository: Project-HAMi/HAMi Length of output: 15058 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '--- quota implementation ---\n'
sed -n '1,150p' pkg/device/quota.go
printf '\n--- Enflame device structs and resource names ---\n'
sed -n '70,110p' pkg/device/enflame/device.go
sed -n '478,488p' pkg/device/enflame/device.go
printf '\n--- FitQuota signatures/usages ---\n'
rg -n 'func \([^)]*\) FitQuota|func FitQuota|MemoryFactor|ResourceNames' pkg/device -SRepository: Project-HAMi/HAMi Length of output: 246 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- quota implementation ---'
sed -n '1,150p' pkg/device/quota.go
echo
echo '--- Enflame device structs and resource names ---'
sed -n '70,110p' pkg/device/enflame/device.go
sed -n '478,488p' pkg/device/enflame/device.go
echo
echo '--- FitQuota signatures/usages ---'
rg -n 'func \([^)]*\) FitQuota|func FitQuota|MemoryFactor|ResourceNames' pkg/device -SRepository: Project-HAMi/HAMi Length of output: 17587 Set the Enflame memory conversion factor before enforcing this quota check.
🤖 Prompt for AI AgentsSource: MCP tools |
||
| if dev.Totalmem-dev.Usedmem < profileMemoryMiB { | ||
| reason[common.CardInsufficientMemory]++ | ||
| klog.V(5).InfoS(common.CardInsufficientMemory, "pod", klog.KObj(pod), "device", dev.ID, "device index", i, "device total memory", dev.Totalmem, "device used memory", dev.Usedmem, "request memory", profileMemoryMiB) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -371,6 +371,12 @@ func (mats *MetaxSDevices) Fit(devices []*device.DeviceUsage, request device.Con | |
| memreq = dev.Totalmem * request.MemPercentagereq / 100 | ||
| } | ||
|
|
||
| if !device.FitQuotaForDevice(tmpDevs, allocated, pod.Namespace, int64(memreq), int64(request.Coresreq), MetaxSGPUDevice, mats.GetResourceNames()) { | ||
| reason[common.ResourceQuotaNotFit]++ | ||
| klog.V(3).InfoS(common.ResourceQuotaNotFit, "pod", pod.Name, "memreq", memreq, "coresreq", request.Coresreq) | ||
| continue | ||
| } | ||
|
|
||
|
Comment on lines
+374
to
+379
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift Build the selected allocation before quota validation.
Do not fix this by adding an empty map. This check runs before the final device list is selected, so an empty map cannot aggregate a multi-device allocation. Build the selected 🤖 Prompt for AI Agents |
||
| if dev.Totalmem-dev.Usedmem < memreq { | ||
| reason[common.CardInsufficientMemory]++ | ||
| klog.V(5).InfoS(common.CardInsufficientMemory, "pod", klog.KObj(pod), "device", dev.ID, "device index", i, "device total memory", dev.Totalmem, "device used memory", dev.Usedmem, "request memory", memreq) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,31 @@ func (q *QuotaManager) FitQuota(ns string, memreq int64, memoryFactor int32, cor | |
| return true | ||
| } | ||
|
|
||
| // FitQuotaForDevice checks whether a pod's total device request (current round | ||
| // plus previously allocated containers) fits the namespace ResourceQuota for the | ||
| // given device type. Every backend should call this from its Fit() method to | ||
| // enforce quota at scheduling time. | ||
| func FitQuotaForDevice(tmpDevs map[string]ContainerDevices, allocated *PodDevices, ns string, memreq int64, coresreq int64, deviceName string, resourceNames ResourceNames) bool { | ||
| mem := memreq | ||
| core := coresreq | ||
| for _, val := range tmpDevs[deviceName] { | ||
| mem += int64(val.Usedmem) | ||
| core += int64(val.Usedcores) | ||
| } | ||
| if allocated != nil { | ||
| if podSingleDevice, exists := (*allocated)[deviceName]; exists { | ||
| for _, containerDevices := range podSingleDevice { | ||
| for _, val := range containerDevices { | ||
| mem += int64(val.Usedmem) | ||
| core += int64(val.Usedcores) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| klog.V(4).Infoln("FitQuotaForDevice: device", deviceName, "mem", mem, "cores", core) | ||
| return GetLocalCache().FitQuota(ns, mem, resourceNames.MemoryFactor, core, deviceName) | ||
|
Comment on lines
+95
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline pkg/device/quota.go --items all
rg -n -C 5 '\b(FitQuotaForDevice|FitQuota|AddUsage|RmUsage)\b' pkg
rg -n -C 5 '\.Fit\(|FitQuotaForDevice\(' pkgRepository: Project-HAMi/HAMi Length of output: 50373 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect quota implementation and relevant scheduler paths.
sed -n '50,190p' pkg/device/quota.go
printf '\n--- scheduler relevant sections ---\n'
sed -n '130,175p' pkg/scheduler/scheduler.go
sed -n '910,1005p' pkg/scheduler/scheduler.go
printf '\n--- webhook quota path ---\n'
sed -n '110,170p' pkg/scheduler/webhook.go
printf '\n--- all direct calls to device quota functions outside tests ---\n'
rg -n -C 3 '\.FitQuotaForDevice\(|FitQuotaForDevice\(|GetLocalCache\(\)\.FitQuota\(' `git ls-files '*_test.go' | sed 's/.*//;q'` 2>/dev/null >/tmp/except_tests.txt || true
rg -n -C 3 '\.FitQuotaForDevice\(|FitQuotaForDevice\(|device\.GetLocalCache\(\)\.FitQuota\(' --glob '!**/*_test.go' pkg
printf '\n--- search for quota related methods and lock patterns ---\n'
rg -n -C 3 'FitQuota|QuotaManager|mutex\.RLock|mutex\.Lock|AddUsage|TakeAndDelete' pkg --glob '!**/*_test.go'Repository: Project-HAMi/HAMi Length of output: 10814 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Behavioral probe of the QuotaManager state update semantics with an RLock-only check.
# This model matches: FitQuota reads Quota.Used under RLock; AddUsage records usage under Lock;
# multiple Fits can complete before the winning reservation is written.
cat > /tmp/quota_concurrency_model.go <<'EOF'
package main
import (
"context"
"fmt"
"sync"
"time"
)
type Quota struct {
Limit int64
Used int64
}
type QuotaManager struct {
Quotas map[string]Quota
mu sync.RWMutex
}
func fit(q *QuotaManager, ns string, request int64) bool {
q.mu.RLock()
v := q.Quotas[ns]
accept := v.Used+request <= v.Limit
q.mu.RUnlock()
if !accept {
return false
}
time.Sleep(time.Microsecond)
return true
}
func addUsage(q *QuotaManager, ns string, usage int64) {
q.mu.Lock()
q.Quotas[ns].Used += usage
q.mu.Unlock()
}
func main() {
m := &QuotaManager{Quotas: map[string]Quota{"ns": {Limit: 1000}}}
var done sync.WaitGroup
ctx := context.Background()
for i := 0; i < 5; i++ {
done.Add(1)
go func(id int) {
defer done.Done()
select {
case <-time.After(time.Microsecond):
case <-ctx.Done():
return
}
if fit(m, "ns", 300) {
println("fit", id)
addUsage(m, "ns", 300)
}
}(i)
}
done.Wait()
fmt.Printf("final_used=%d\n", m.Quotas["ns"].Used)
}
EOF
go run /tmp/quota_concurrency_model.goRepository: Project-HAMi/HAMi Length of output: 273 🏁 Script executed: #!/bin/bash
set -euo pipefail
cat > /tmp/quota_concurrency_model.go <<'EOF'
package main
import (
"fmt"
"sync"
"sync/atomic"
"time"
)
type Quota struct {
Limit int64
Used int64
}
type QuotaManager struct {
Quotas map[string]*Quota
mu sync.RWMutex
}
func fit(q *QuotaManager, ns string, request int64) bool {
q.mu.RLock()
v := q.Quotas[ns]
accept := v.Used+request <= v.Limit
q.mu.RUnlock()
if !accept {
return false
}
time.Sleep(time.Microsecond)
return true
}
func addUsage(q *QuotaManager, ns string, usage int64) {
q.mu.Lock()
defer q.mu.Unlock()
if q.Quotas[ns] == nil {
q.Quotas[ns] = &Quota{Limit: 1000}
}
q.Quotas[ns].Used += usage
}
func main() {
m := &QuotaManager{Quotas: map[string]*Quota{"ns": {Limit: 1000}}}
var attempts, successes atomic.Int64
var wg sync.WaitGroup
for i := 0; i < 50; i++ {
wg.Add(1)
go func() {
defer wg.Done()
if fit(m, "ns", 300) {
values := make(chan bool, 1)
go func() { values <- addUsage(m, "ns", 300) }()
time.Sleep(time.Microsecond * 20)
select {
case <-values:
successes.Add(1)
default:
m.mu.RLock()
used := m.Quotas["ns"].Used
m.mu.RUnlock()
if used == 300 {
successes.Add(1)
}
}
attempts.Add(1)
}
}()
}
wg.Wait()
fmt.Printf("attempts=%d successes=%d final_used=%d\n", attempts.Load(), successes.Load(), m.Quotas["ns"].Used)
}
EOF
go run /tmp/quota_concurrency_model.goRepository: Project-HAMi/HAMi Length of output: 267 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Remove the probe from output; it is not needed for a deterministic code-base result because
# the source shows FitQuota only reads while AddUsage later mutates separately.
trueRepository: Project-HAMi/HAMi Length of output: 155 Make quota admission atomic.
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| func countPodDevices(podDev PodDevices) map[string]int64 { | ||
| res := make(map[string]int64) | ||
| for deviceName, podSingle := range podDev { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Pass the converted core usage to the quota check.
Line 315 passes
k.Coresreq, but Lines 326-358 convert that percentage tocoreReqand persistcoreReqinContainerDevice.Usedcores. The quota check can undercount the current allocation whendev.Totalcoreis not 100.Calculate
coreReqbefore this check. PasscoreReqtoFitQuotaForDevice.Based on PR context, quota usage is accumulated from
ContainerDevice.Usedcores.🤖 Prompt for AI Agents