Skip to content

Commit 63abbc8

Browse files
authored
fix(api): read pool usage under the quota key the controller counts (#5184)
## What this PR does Tenant quota admission does not see how much of an extended resource or hugepages the parent pool already uses, so a child tenant can be granted a share the parent has already spent. Admission reads that usage from `limits.<key>`, but Kubernetes reports it only under `requests.<key>`, so the value is always zero. Now each key is read where the quota controller counts it. cpu, memory and ephemeral-storage still read `limits.*`. The new tests fail on main. ### Screenshots ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(api): tenant quota admission now counts the parent pool's usage of extended resources and hugepages, so a child tenant can no longer get a share the parent has already used. After upgrading, updates to existing tenants may be rejected if their quota for these resources exceeds the parent's remaining budget. This includes updates that leave the quota unchanged. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Quota checks now account for existing usage of extended resources, preventing child requests from exceeding the parent’s remaining quota. * Requests are allowed when current usage leaves enough quota available, and denied when usage has consumed the parent’s full quota. * Resource usage is matched against the appropriate quota category, including requests for extended resources and hugepages. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents 1f35c26 + 54695fc commit 63abbc8

2 files changed

Lines changed: 54 additions & 19 deletions

File tree

‎pkg/registry/apps/application/quota.go‎

Lines changed: 29 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ func (r *REST) validateTenantResourceQuotas(ctx context.Context, app *appsv1alph
233233
}
234234
parentLimit := parentQuota[res]
235235
allocated := siblings[res] // zero value when no sibling bounds this resource
236-
used := poolUsed[renderedLimitKey(res)]
236+
used := poolUsed[usageKey(res)]
237237
remaining := parentLimit.DeepCopy()
238238
remaining.Sub(allocated)
239239
remaining.Sub(used)
@@ -285,25 +285,39 @@ var rawQuotaKeys = map[string]struct{}{
285285
"resourcequotas": {},
286286
}
287287

288-
// renderedLimitKey maps a shorthand resourceQuotas key (the vocabulary admission
289-
// works in) to the rendered ResourceQuota key that carries its limit-side
290-
// budget, mirroring cozy-lib.resources.flatten. The mapping is allocation-ratio
291-
// independent: ratios only scale the requests.* side that flatten derives from
292-
// the same shorthand value, so the limit-side key always holds the operator's
293-
// declared amount. This lets admission read a tenant's current usage from its
294-
// rendered ResourceQuota.status.used without replicating the chart's ratio math.
288+
// limitCountedKeys are the shorthand keys whose usage the ResourceQuota pod
289+
// evaluator reports under limits.*. For every other resource it counts only
290+
// requests.*, so limits.<extended resource> or limits.hugepages-* never carries
291+
// usage even though the chart renders it.
292+
var limitCountedKeys = map[string]struct{}{
293+
"cpu": {},
294+
"memory": {},
295+
"ephemeral-storage": {},
296+
}
297+
298+
// usageKey maps a shorthand resourceQuotas key (the vocabulary admission works
299+
// in) to the rendered ResourceQuota key whose status.used carries its usage.
300+
// cozy-lib.resources.flatten renders a shorthand key both as limits.<key> and as
301+
// requests.<key>; usage is read from whichever of the two the quota controller
302+
// counts. For cpu, memory and ephemeral-storage that is the limit side, which
303+
// holds the operator's declared amount whatever allocation ratio scales the
304+
// request side. Every other resource is counted only on the request side, where
305+
// flatten applies no ratio, so requests.<key> holds the declared amount too.
306+
// This lets admission read a tenant's current usage from its rendered
307+
// ResourceQuota.status.used without replicating the chart's ratio math.
295308
//
296-
// cpu, memory, ephemeral-storage, custom resources -> "limits.<key>"
309+
// cpu, memory, ephemeral-storage -> "limits.<key>"
297310
// storage -> "requests.storage" (no hard limits.storage)
298311
// pods, services, services.loadbalancers, ... -> "<key>" (verbatim)
299-
func renderedLimitKey(raw string) string {
300-
if raw == "storage" {
301-
return "requests.storage"
302-
}
312+
// extended resources, hugepages-<size>, ... -> "requests.<key>"
313+
func usageKey(raw string) string {
303314
if _, ok := rawQuotaKeys[raw]; ok {
304315
return raw
305316
}
306-
return "limits." + raw
317+
if _, ok := limitCountedKeys[raw]; ok {
318+
return "limits." + raw
319+
}
320+
return "requests." + raw
307321
}
308322

309323
// parentNamespaceOf returns the namespace owned by the parent of the tenant that
@@ -341,7 +355,7 @@ func (r *REST) poolRootOf(ns string, bounded map[string]bool) string {
341355
// created/updated). Usage is read from each member's ResourceQuota.status.used,
342356
// taking a per-resource max within a namespace to avoid double-counting the
343357
// chart-rendered and controller-allocated quotas, and is keyed in the rendered
344-
// quota key space (see renderedLimitKey). Boundedness is decided by declared
358+
// quota key space (see usageKey). Boundedness is decided by declared
345359
// resourceQuotas so the result does not depend on whether Flux has rendered a
346360
// freshly-created tenant's quota yet.
347361
func (r *REST) parentPoolUsage(ctx context.Context, parentNS, excludeNS string) (map[string]resource.Quantity, error) {

‎pkg/registry/apps/application/quota_test.go‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -136,19 +136,20 @@ func usageQuota(t *testing.T, namespace, name string, used map[string]string) *c
136136
}
137137
}
138138

139-
func TestRenderedLimitKey(t *testing.T) {
139+
func TestUsageKey(t *testing.T) {
140140
cases := map[string]string{
141141
"cpu": "limits.cpu",
142142
"memory": "limits.memory",
143143
"ephemeral-storage": "limits.ephemeral-storage",
144-
"devices.com/nvidia": "limits.devices.com/nvidia",
144+
"devices.com/nvidia": "requests.devices.com/nvidia",
145+
"hugepages-2Mi": "requests.hugepages-2Mi",
145146
"storage": "requests.storage",
146147
"pods": "pods",
147148
"services.loadbalancers": "services.loadbalancers",
148149
}
149150
for raw, want := range cases {
150-
if got := renderedLimitKey(raw); got != want {
151-
t.Errorf("renderedLimitKey(%q) = %q, want %q", raw, got, want)
151+
if got := usageKey(raw); got != want {
152+
t.Errorf("usageKey(%q) = %q, want %q", raw, got, want)
152153
}
153154
}
154155
}
@@ -339,6 +340,26 @@ func TestValidateTenantResourceQuotas_PoolUsage(t *testing.T) {
339340
}
340341
})
341342

343+
t.Run("parent usage of an extended resource shrinks remaining", func(t *testing.T) {
344+
parent := tenantHelmRelease(t, "foo", "tenant-root", map[string]string{"example.com/device": "2"})
345+
used := usageQuota(t, "tenant-foo", "tenant-quota", map[string]string{"requests.example.com/device": "2"})
346+
r := newTenantREST(t, parent, used)
347+
348+
if errs := r.validateTenantResourceQuotas(context.Background(), childApplication(t, "bar", "tenant-foo", map[string]string{"example.com/device": "1"})); len(errs) == 0 {
349+
t.Fatalf("child example.com/device=1 should be denied when the parent pool already uses 2 of 2")
350+
}
351+
})
352+
353+
t.Run("parent usage of an extended resource leaves the rest grantable", func(t *testing.T) {
354+
parent := tenantHelmRelease(t, "foo", "tenant-root", map[string]string{"example.com/device": "2"})
355+
used := usageQuota(t, "tenant-foo", "tenant-quota", map[string]string{"requests.example.com/device": "1"})
356+
r := newTenantREST(t, parent, used)
357+
358+
if errs := r.validateTenantResourceQuotas(context.Background(), childApplication(t, "bar", "tenant-foo", map[string]string{"example.com/device": "1"})); len(errs) > 0 {
359+
t.Fatalf("child example.com/device=1 should be allowed when the parent pool uses 1 of 2, got: %v", errs)
360+
}
361+
})
362+
342363
// No rendered usage yet (freshly-created parent, Flux has not rendered the
343364
// quota): usage degrades to zero and behaviour matches the pre-usage gate.
344365
t.Run("absent usage degrades to budget-only check", func(t *testing.T) {

0 commit comments

Comments
 (0)