Skip to content

fix(quota): enforce explicit zero limit in ResourceQuota #1904

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.

Sign up for GitHub

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

Open
iasthc wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(quota): enforce explicit zero limit in ResourceQuota #1904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dec1579
fix(quota): enforce explicit zero limit in ResourceQuota
iasthc May 25, 2026
f8790de
fix(quota): preserve LimitSet flag in GetResourceQuota
iasthc May 25, 2026
05fc36b
fix(quota): update metric collection to use limit label for ResourceQ…
iasthc May 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions cmd/scheduler/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,18 +298,22 @@ func (cc ClusterManagerCollector) Collect(ch chan<- prometheus.Metric) {
)
for ns, val := range sher.GetQuotaManager().GetResourceQuota() {
for quotaname, q := range *val {
limitLabel := "unlimited"

@mesutoezdil mesutoezdil Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this changes the prometheus label value from "0" to "unlimited" for namespaces that have usage but no configured quota (LimitSet=false). existing dashboards or alerts that filter on limit="0" for the no-quota case will break silently. the new label is semantically correct, but the change should be called out explicitly for operators.

if q.LimitSet {
limitLabel = fmt.Sprint(q.Limit)
}
ch <- prometheus.MustNewConstMetric(
quotaUsedDesc,
prometheus.GaugeValue,
float64(q.Used),
ns, quotaname, fmt.Sprint(q.Limit),
ns, quotaname, limitLabel,
)
if legacy {
ch <- prometheus.MustNewConstMetric(
legacyQuotaUsed,
prometheus.GaugeValue,
float64(q.Used),
ns, quotaname, fmt.Sprint(q.Limit),
ns, quotaname, limitLabel,
)
}
}
Expand Down
18 changes: 11 additions & 7 deletions pkg/device/quota.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ import (
)

type Quota struct {
Used int64
Limit int64
Used int64
Limit int64
LimitSet bool
Comment thread
iasthc marked this conversation as resolved.

@DSFans2014 DSFans2014 May 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can set the default value of limit to MaxInt64 to avoid introducing a new variable. The information from ResourceQuota will be used in metrics as below.

HAMi/cmd/scheduler/metrics.go

Lines 299 to 315 in f852784

for ns, val := range sher.GetQuotaManager().GetResourceQuota() {
for quotaname, q := range *val {
ch <- prometheus.MustNewConstMetric(
quotaUsedDesc,
prometheus.GaugeValue,
float64(q.Used),
ns, quotaname, fmt.Sprint(q.Limit),
)
if legacy {
ch <- prometheus.MustNewConstMetric(
legacyQuotaUsed,
prometheus.GaugeValue,
float64(q.Used),
ns, quotaname, fmt.Sprint(q.Limit),
)
}
}

@iasthc iasthc May 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I think LimitSet is still the safer choice here:

  1. FitQuota would overflow. pkg/device/quota.go#L75-L78 multiplies Limit by memoryFactor. With Limit == MaxInt64 and memoryFactor > 1 this overflows to a negative number, and legitimate requests get rejected. Guarding against it requires an if Limit == MaxInt64 check — another sentinel flag in disguise.

  2. Metric label. Fair point that the rendering needs work, but limit="9223372036854775807" isn't friendlier than "0". I've pushed a follow-up that uses LimitSet to render "unlimited". This case is real today: AddUsage creates LimitSet=false entries for namespaces with usage but no ResourceQuota, and fix: rollback quota usage when filter annotation patch fails #1898 / fix(quota): roll back usage when Filter evicts a stale pod entry #1905 exercise those paths more often now.

  3. Sentinel semantics would spread. AddUsage and DelQuota would both need to use MaxInt64, and every reader has to remember 0 and MaxInt64 carry special meaning. A bool is one byte and explicit.

The metrics commit is on top of the quota fixes, rebased onto current master — PTAL.

}

type DeviceQuota map[string]*Quota
Expand Down Expand Up @@ -69,19 +70,19 @@ func (q *QuotaManager) FitQuota(ns string, memreq int64, memoryFactor int32, cor
return true
}
memQuota, ok := (*dq)[memResourceName]
if ok {
if ok && memQuota.LimitSet {
klog.V(4).InfoS("resourceMem quota judging", "quota limit", memQuota.Limit, "used", memQuota.Used, "alloc", memreq, "memoryFactor", memoryFactor)
limit := memQuota.Limit
if memoryFactor > 1 {
limit = limit * int64(memoryFactor)
}
if limit != 0 && memQuota.Used+memreq > limit {
if memQuota.Used+memreq > limit {
klog.V(4).InfoS("resourceMem quota not fitted", "limit", limit, "used", memQuota.Used, "alloc", memreq)
return false
}
}
coreQuota, ok := (*dq)[coreResourceName]
if ok && coreQuota.Limit != 0 && coreQuota.Used+coresreq > coreQuota.Limit {
if ok && coreQuota.LimitSet && coreQuota.Used+coresreq > coreQuota.Limit {
klog.V(4).InfoS("resourceCores quota not fitted", "limit", coreQuota.Limit, "used", coreQuota.Used, "alloc", coresreq)
return false
}
Expand Down Expand Up @@ -206,6 +207,7 @@ func (q *QuotaManager) AddQuota(quota *corev1.ResourceQuota) {
}
}
(*dp)[dn].Limit = value
(*dp)[dn].LimitSet = true
klog.V(4).InfoS("quota set:", "idx=", idx, "val", value)
}
}
Expand Down Expand Up @@ -235,6 +237,7 @@ func (q *QuotaManager) DelQuota(quota *corev1.ResourceQuota) {
if dq, ok := q.Quotas[quota.Namespace]; ok {
if quotaInfo, ok := (*dq)[dn]; ok {
quotaInfo.Limit = 0
quotaInfo.LimitSet = false
}
}
}
Expand All @@ -257,8 +260,9 @@ func (q *QuotaManager) GetResourceQuota() map[string]*DeviceQuota {
curDQ := &DeviceQuota{}
for name, quota := range *dq {
(*curDQ)[name] = &Quota{
Used: quota.Used,
Limit: quota.Limit,
Used: quota.Used,
Limit: quota.Limit,
LimitSet: quota.LimitSet,
}
}
quotasCopy[ns] = curDQ
Expand Down
105 changes: 103 additions & 2 deletions pkg/device/quota_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,8 @@ func TestFitQuota(t *testing.T) {
coreName := "nvidia.com/gpucore"

qm.Quotas[ns] = &DeviceQuota{
memName: &Quota{Used: 1000, Limit: 2000},
coreName: &Quota{Used: 200, Limit: 400},
memName: &Quota{Used: 1000, Limit: 2000, LimitSet: true},
coreName: &Quota{Used: 200, Limit: 400, LimitSet: true},
}

// Should fit
Expand Down Expand Up @@ -160,6 +160,41 @@ func TestFitQuota(t *testing.T) {
}
}

func TestFitQuotaZeroLimitEnforced(t *testing.T) {
initTest()
qm := NewQuotaManager()
ns := "zero-ns"
deviceName := "NVIDIA"
memName := "nvidia.com/gpumem"
coreName := "nvidia.com/gpucore"

// Explicit zero limit must reject any non-zero request.
qm.Quotas[ns] = &DeviceQuota{
memName: &Quota{Used: 0, Limit: 0, LimitSet: true},
coreName: &Quota{Used: 0, Limit: 0, LimitSet: true},
}
if qm.FitQuota(ns, 1, 1, 0, deviceName) {
t.Error("FitQuota should reject any memory when limit is explicitly 0")
}
if qm.FitQuota(ns, 0, 1, 1, deviceName) {
t.Error("FitQuota should reject any core when limit is explicitly 0")
}
// Zero request against zero limit is still allowed.
if !qm.FitQuota(ns, 0, 1, 0, deviceName) {
t.Error("FitQuota should allow zero request against zero limit")
}

// LimitSet=false (e.g. only AddUsage created the entry) means no limit configured.
nsUnset := "unset-ns"
qm.Quotas[nsUnset] = &DeviceQuota{
memName: &Quota{Used: 0, Limit: 0, LimitSet: false},
coreName: &Quota{Used: 0, Limit: 0, LimitSet: false},
}
if !qm.FitQuota(nsUnset, 9999, 1, 9999, deviceName) {
t.Error("FitQuota should allow requests when no limit is configured")
}
}

func TestAddUsageAndRmUsage(t *testing.T) {
initTest()
qm := NewQuotaManager()
Expand Down Expand Up @@ -231,15 +266,81 @@ func TestAddQuotaAndDelQuota(t *testing.T) {
if (*qm.Quotas[ns])[memName].Limit != 100 {
t.Errorf("AddQuota: expected memory limit 100, got %d", (*qm.Quotas[ns])[memName].Limit)
}
if !(*qm.Quotas[ns])[memName].LimitSet {
t.Error("AddQuota: expected LimitSet=true for memory quota")
}
if (*qm.Quotas[ns])[coreName].Limit != 10 {
t.Errorf("AddQuota: expected core limit 10, got %d", (*qm.Quotas[ns])[coreName].Limit)
}
if !(*qm.Quotas[ns])[coreName].LimitSet {
t.Error("AddQuota: expected LimitSet=true for core quota")
}

qm.DelQuota(rq)
if (*qm.Quotas[ns])[memName].Limit != 0 {
t.Errorf("DelQuota: expected memory limit 0, got %d", (*qm.Quotas[ns])[memName].Limit)
}
if (*qm.Quotas[ns])[memName].LimitSet {
t.Error("DelQuota: expected LimitSet=false for memory quota")
}
if (*qm.Quotas[ns])[coreName].Limit != 0 {
t.Errorf("DelQuota: expected core limit 0, got %d", (*qm.Quotas[ns])[coreName].Limit)
}
if (*qm.Quotas[ns])[coreName].LimitSet {
t.Error("DelQuota: expected LimitSet=false for core quota")
}
}

func TestAddQuotaExplicitZero(t *testing.T) {
initTest()
qm := NewQuotaManager()
ns := "zero-quota-ns"
memName := "nvidia.com/gpumem"

rq := &corev1.ResourceQuota{}
rq.Namespace = ns
rq.Spec.Hard = corev1.ResourceList{
corev1.ResourceName("limits." + memName): *resource.NewQuantity(0, resource.DecimalSI),
}

qm.AddQuota(rq)
q := (*qm.Quotas[ns])[memName]
if q == nil {
t.Fatal("AddQuota: expected entry for explicit zero limit")
}
if q.Limit != 0 {
t.Errorf("AddQuota: expected limit 0, got %d", q.Limit)
}
if !q.LimitSet {
t.Error("AddQuota: expected LimitSet=true even when value is 0")
}
// Verify FitQuota now rejects any non-zero request.
if qm.FitQuota(ns, 1, 1, 0, "NVIDIA") {
t.Error("FitQuota should deny request when explicit zero limit is set")
}
}

func TestGetResourceQuotaPreservesLimitSet(t *testing.T) {
initTest()
qm := NewQuotaManager()
ns := "copy-ns"
memName := "nvidia.com/gpumem"
coreName := "nvidia.com/gpucore"

qm.Quotas[ns] = &DeviceQuota{
memName: &Quota{Used: 100, Limit: 0, LimitSet: true},
coreName: &Quota{Used: 50, Limit: 200, LimitSet: false},
}

snapshot := qm.GetResourceQuota()
dq, ok := snapshot[ns]
if !ok {
t.Fatalf("GetResourceQuota: missing namespace %q", ns)
}
if !(*dq)[memName].LimitSet {
t.Error("GetResourceQuota: expected LimitSet=true to be preserved")
}
if (*dq)[coreName].LimitSet {
t.Error("GetResourceQuota: expected LimitSet=false to be preserved")
}
}
4 changes: 2 additions & 2 deletions pkg/scheduler/webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,8 @@ func TestFitResourceQuota(t *testing.T) {
coreName := "nvidia.com/gpucores"

qm.Quotas[ns] = &device.DeviceQuota{
memName: &device.Quota{Used: 1000, Limit: 2000},
coreName: &device.Quota{Used: 200, Limit: 400},
memName: &device.Quota{Used: 1000, Limit: 2000, LimitSet: true},
coreName: &device.Quota{Used: 200, Limit: 400, LimitSet: true},
}

testCases := []struct {
Expand Down
Loading