Skip to content

Commit e3f09ba

Browse files
scooby87kvaps
authored andcommitted
fix(apiserver): subtract parent pool usage in hierarchical tenant quota admission
The declaration-time gate only subtracted sibling carve-outs from a parent's budget, not the parent pool's current usage. A tenant already consuming most of its budget could therefore carve a full-budget child, letting the sub-tree collectively exceed the root's allocation. Sum the current usage of the parent pool (its own namespace plus its unbounded descendants, read from each member's rendered ResourceQuota.status.used) and subtract it from the remaining budget, mapping each shorthand quota key to its rendered limit key (renderedLimitKey). Boundedness is decided by declared resourceQuotas so the result is independent of whether Flux has rendered a freshly-created tenant's quota yet. This is best-effort at declaration time (a snapshot; concurrent writes can briefly overshoot and running pods are never evicted) — the tenant-quota controller remains the runtime backstop. Assisted-By: Claude <[email protected]> Signed-off-by: Алексей Артамонов <[email protected]>
1 parent b0556cc commit e3f09ba

2 files changed

Lines changed: 262 additions & 2 deletions

File tree

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

Lines changed: 178 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"strings"
2525

2626
helmv2 "github.com/fluxcd/helm-controller/api/v2"
27+
corev1 "k8s.io/api/core/v1"
2728
apierrors "k8s.io/apimachinery/pkg/api/errors"
2829
"k8s.io/apimachinery/pkg/api/resource"
2930
"k8s.io/apimachinery/pkg/labels"
@@ -211,19 +212,35 @@ func (r *REST) validateTenantResourceQuotas(ctx context.Context, app *appsv1alph
211212
return append(allErrs, field.InternalError(fldPath, err))
212213
}
213214

215+
// Current usage of the parent's pool (its own namespace plus its unbounded
216+
// descendants). Counting it keeps the invariant — a sub-tree may never, via
217+
// quota, be promised more than its root owns — holding at declaration time:
218+
// without it a parent already using most of its budget could still carve a
219+
// full-budget child and the two would collectively exceed the root. This is
220+
// best-effort (a snapshot; concurrent writes can briefly overshoot and
221+
// already-running pods are never evicted) — the tenant-quota controller is
222+
// the runtime backstop.
223+
childNamespace := r.computeTenantNamespace(app.Namespace, app.Name)
224+
poolUsed, err := r.parentPoolUsage(ctx, app.Namespace, childNamespace)
225+
if err != nil {
226+
return append(allErrs, field.InternalError(fldPath, err))
227+
}
228+
214229
for _, res := range sortedQuotaKeys(parentQuota) {
215230
want, requested := childQuota[res]
216231
if !requested {
217232
continue
218233
}
219234
parentLimit := parentQuota[res]
220235
allocated := siblings[res] // zero value when no sibling bounds this resource
236+
used := poolUsed[renderedLimitKey(res)]
221237
remaining := parentLimit.DeepCopy()
222238
remaining.Sub(allocated)
239+
remaining.Sub(used)
223240
if want.Cmp(remaining) > 0 {
224241
allErrs = append(allErrs, field.Forbidden(fldPath.Key(res),
225-
fmt.Sprintf("requested %s exceeds the parent tenant's remaining %q quota of %s (parent allows %s, %s already allocated to sibling tenants); a child tenant may not be granted more quota than its parent has left",
226-
want.String(), res, remaining.String(), parentLimit.String(), allocated.String())))
242+
fmt.Sprintf("requested %s exceeds the parent tenant's remaining %q quota of %s (parent allows %s, %s already allocated to sibling tenants, %s currently in use by the parent pool); a child tenant may not be granted more quota than its parent has left",
243+
want.String(), res, remaining.String(), parentLimit.String(), allocated.String(), used.String())))
227244
}
228245
}
229246
return allErrs
@@ -251,3 +268,162 @@ func sortedQuotaKeys(q map[string]resource.Quantity) []string {
251268
sort.Strings(keys)
252269
return keys
253270
}
271+
272+
// rawQuotaKeys are the shorthand resourceQuotas keys that the tenant chart emits
273+
// verbatim as ResourceQuota hard keys (no limits./requests. prefix). Kept in
274+
// sync with cozy-lib.resources.flattenResource ($rawQuotaKeys).
275+
var rawQuotaKeys = map[string]struct{}{
276+
"pods": {},
277+
"services": {},
278+
"services.loadbalancers": {},
279+
"services.nodeports": {},
280+
"services.clusterip": {},
281+
"configmaps": {},
282+
"secrets": {},
283+
"persistentvolumeclaims": {},
284+
"replicationcontrollers": {},
285+
"resourcequotas": {},
286+
}
287+
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.
295+
//
296+
// cpu, memory, ephemeral-storage, custom resources -> "limits.<key>"
297+
// storage -> "requests.storage" (no hard limits.storage)
298+
// pods, services, services.loadbalancers, ... -> "<key>" (verbatim)
299+
func renderedLimitKey(raw string) string {
300+
if raw == "storage" {
301+
return "requests.storage"
302+
}
303+
if _, ok := rawQuotaKeys[raw]; ok {
304+
return raw
305+
}
306+
return "limits." + raw
307+
}
308+
309+
// parentNamespaceOf returns the namespace owned by the parent of the tenant that
310+
// owns ns, or "" for the root tenant and non-tenant namespaces. It mirrors
311+
// tenantquota.parentNamespace / the inverse of computeTenantNamespace: the
312+
// hierarchy is encoded in the namespace name, so the parent is recovered by
313+
// stripping the trailing "-<name>" segment.
314+
func (r *REST) parentNamespaceOf(ns string) string {
315+
if ns == rootTenantNamespace || !strings.HasPrefix(ns, r.releaseConfig.Prefix) {
316+
return ""
317+
}
318+
segments := strings.Split(ns, "-")
319+
if len(segments) == 2 {
320+
return rootTenantNamespace
321+
}
322+
return strings.Join(segments[:len(segments)-1], "-")
323+
}
324+
325+
// poolRootOf returns the nearest ancestor namespace (inclusive of ns) that is
326+
// bounded — declares its own quota — or "" when none is. That ancestor is the
327+
// pool root governing ns.
328+
func (r *REST) poolRootOf(ns string, bounded map[string]bool) string {
329+
for cur := ns; cur != ""; cur = r.parentNamespaceOf(cur) {
330+
if bounded[cur] {
331+
return cur
332+
}
333+
}
334+
return ""
335+
}
336+
337+
// parentPoolUsage sums the current usage of the pool rooted at parentNS: the
338+
// parent's own namespace plus every unbounded descendant whose nearest bounded
339+
// ancestor is parentNS. Bounded sub-tenants are excluded — their reservations
340+
// are already charged as sibling carve-outs — as is excludeNS (the tenant being
341+
// created/updated). Usage is read from each member's ResourceQuota.status.used,
342+
// taking a per-resource max within a namespace to avoid double-counting the
343+
// chart-rendered and controller-allocated quotas, and is keyed in the rendered
344+
// quota key space (see renderedLimitKey). Boundedness is decided by declared
345+
// resourceQuotas so the result does not depend on whether Flux has rendered a
346+
// freshly-created tenant's quota yet.
347+
func (r *REST) parentPoolUsage(ctx context.Context, parentNS, excludeNS string) (map[string]resource.Quantity, error) {
348+
releases := &helmv2.HelmReleaseList{}
349+
selector := labels.SelectorFromSet(labels.Set{
350+
ApplicationKindLabel: r.kindName,
351+
ApplicationGroupLabel: r.gvk.Group,
352+
})
353+
if err := r.c.List(ctx, releases, &client.ListOptions{LabelSelector: selector}); err != nil {
354+
return nil, err
355+
}
356+
357+
bounded := map[string]bool{}
358+
owned := make([]string, 0, len(releases.Items))
359+
for i := range releases.Items {
360+
hr := &releases.Items[i]
361+
name := strings.TrimPrefix(hr.Name, r.releaseConfig.Prefix)
362+
ns := r.computeTenantNamespace(hr.Namespace, name)
363+
owned = append(owned, ns)
364+
quotas, err := declaredQuotasFromHelmRelease(hr)
365+
if err != nil {
366+
klog.Warningf("skipping tenant %s/%s with unparseable quotas while summing pool usage: %v", hr.Namespace, hr.Name, err)
367+
continue
368+
}
369+
if len(quotas) > 0 {
370+
bounded[ns] = true
371+
}
372+
}
373+
374+
members := map[string]bool{parentNS: true}
375+
for _, ns := range owned {
376+
if ns == excludeNS {
377+
continue
378+
}
379+
if r.poolRootOf(ns, bounded) == parentNS {
380+
members[ns] = true
381+
}
382+
}
383+
delete(members, excludeNS)
384+
385+
quotas := &corev1.ResourceQuotaList{}
386+
if err := r.c.List(ctx, quotas); err != nil {
387+
return nil, err
388+
}
389+
perNamespace := map[string]map[string]resource.Quantity{}
390+
for i := range quotas.Items {
391+
rq := &quotas.Items[i]
392+
if !members[rq.Namespace] {
393+
continue
394+
}
395+
perNamespace[rq.Namespace] = maxQuotas(perNamespace[rq.Namespace], resourceListToQuotas(rq.Status.Used))
396+
}
397+
398+
total := map[string]resource.Quantity{}
399+
for _, used := range perNamespace {
400+
addQuotas(total, used)
401+
}
402+
return total, nil
403+
}
404+
405+
// resourceListToQuotas converts a corev1.ResourceList to the string-keyed quota
406+
// map the admission arithmetic uses.
407+
func resourceListToQuotas(rl corev1.ResourceList) map[string]resource.Quantity {
408+
out := make(map[string]resource.Quantity, len(rl))
409+
for k, v := range rl {
410+
out[string(k)] = v
411+
}
412+
return out
413+
}
414+
415+
// maxQuotas returns the per-resource maximum of dst and src as a new map.
416+
// Multiple ResourceQuotas in one namespace each report the same usage, so usage
417+
// is merged with a max (not a sum) to avoid double counting.
418+
func maxQuotas(dst, src map[string]resource.Quantity) map[string]resource.Quantity {
419+
out := map[string]resource.Quantity{}
420+
for k, v := range dst {
421+
out[k] = v.DeepCopy()
422+
}
423+
for k, v := range src {
424+
if cur, ok := out[k]; !ok || v.Cmp(cur) > 0 {
425+
out[k] = v.DeepCopy()
426+
}
427+
}
428+
return out
429+
}

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

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,9 @@ import (
2222
"testing"
2323

2424
helmv2 "github.com/fluxcd/helm-controller/api/v2"
25+
corev1 "k8s.io/api/core/v1"
2526
apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
27+
"k8s.io/apimachinery/pkg/api/resource"
2628
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2729
"k8s.io/apimachinery/pkg/runtime"
2830
"k8s.io/apimachinery/pkg/runtime/schema"
@@ -79,6 +81,9 @@ func newTenantREST(t *testing.T, objects ...client.Object) *REST {
7981
if err := helmv2.AddToScheme(scheme); err != nil {
8082
t.Fatalf("register helmv2 scheme: %v", err)
8183
}
84+
if err := corev1.AddToScheme(scheme); err != nil {
85+
t.Fatalf("register corev1 scheme: %v", err)
86+
}
8287
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build()
8388
return &REST{
8489
c: fakeClient,
@@ -116,6 +121,37 @@ func childApplication(t *testing.T, name, namespace string, quotas map[string]st
116121
}
117122
}
118123

124+
// usageQuota builds a ResourceQuota reporting current usage (status.used) in a
125+
// namespace, keyed in the rendered quota key space (e.g. "limits.cpu").
126+
func usageQuota(t *testing.T, namespace, name string, used map[string]string) *corev1.ResourceQuota {
127+
t.Helper()
128+
rl := corev1.ResourceList{}
129+
for k, v := range used {
130+
rl[corev1.ResourceName(k)] = resource.MustParse(v)
131+
}
132+
return &corev1.ResourceQuota{
133+
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},
134+
Status: corev1.ResourceQuotaStatus{Used: rl},
135+
}
136+
}
137+
138+
func TestRenderedLimitKey(t *testing.T) {
139+
cases := map[string]string{
140+
"cpu": "limits.cpu",
141+
"memory": "limits.memory",
142+
"ephemeral-storage": "limits.ephemeral-storage",
143+
"devices.com/nvidia": "limits.devices.com/nvidia",
144+
"storage": "requests.storage",
145+
"pods": "pods",
146+
"services.loadbalancers": "services.loadbalancers",
147+
}
148+
for raw, want := range cases {
149+
if got := renderedLimitKey(raw); got != want {
150+
t.Errorf("renderedLimitKey(%q) = %q, want %q", raw, got, want)
151+
}
152+
}
153+
}
154+
119155
func TestValidateTenantResourceQuotas(t *testing.T) {
120156
tests := []struct {
121157
name string
@@ -265,6 +301,54 @@ func TestValidateTenantResourceQuotas_UpdateExcludesSelf(t *testing.T) {
265301
}
266302
}
267303

304+
// TestValidateTenantResourceQuotas_PoolUsage verifies that the parent pool's
305+
// current usage is carved out of the budget available to a new child, so the
306+
// sub-tree invariant holds at declaration time.
307+
func TestValidateTenantResourceQuotas_PoolUsage(t *testing.T) {
308+
// foo budget cpu=10, already using 8 in its own namespace. A child asking
309+
// for 10 must be denied (only 2 left); a child asking for 2 is allowed.
310+
t.Run("parent usage shrinks remaining", func(t *testing.T) {
311+
parent := tenantHelmRelease(t, "foo", "tenant-root", map[string]string{"cpu": "10"})
312+
used := usageQuota(t, "tenant-foo", "tenant-quota", map[string]string{"limits.cpu": "8"})
313+
r := newTenantREST(t, parent, used)
314+
315+
denied := r.validateTenantResourceQuotas(context.Background(), childApplication(t, "bar", "tenant-foo", map[string]string{"cpu": "10"}))
316+
if len(denied) == 0 {
317+
t.Fatalf("child cpu=10 should be denied when parent pool already uses 8 of 10")
318+
}
319+
allowed := r.validateTenantResourceQuotas(context.Background(), childApplication(t, "bar", "tenant-foo", map[string]string{"cpu": "2"}))
320+
if len(allowed) > 0 {
321+
t.Fatalf("child cpu=2 should be allowed (2 left), got: %v", allowed)
322+
}
323+
})
324+
325+
// Usage in a bounded child's own namespace (its own pool) must not count
326+
// against the parent pool's remaining budget.
327+
t.Run("bounded child usage excluded from parent pool", func(t *testing.T) {
328+
parent := tenantHelmRelease(t, "foo", "tenant-root", map[string]string{"cpu": "10"})
329+
child := tenantHelmRelease(t, "bar", "tenant-foo", map[string]string{"cpu": "4"})
330+
// bar uses its full 4 in its own namespace.
331+
childUsed := usageQuota(t, "tenant-foo-bar", "tenant-quota", map[string]string{"limits.cpu": "4"})
332+
r := newTenantREST(t, parent, child, childUsed)
333+
334+
// Raising bar to 9 must still be allowed: bar's own usage is in its own
335+
// pool, and as the tenant being updated it is excluded anyway.
336+
if errs := r.validateTenantResourceQuotas(context.Background(), childApplication(t, "bar", "tenant-foo", map[string]string{"cpu": "9"})); len(errs) > 0 {
337+
t.Fatalf("raising own quota within budget must be allowed, got: %v", errs)
338+
}
339+
})
340+
341+
// No rendered usage yet (freshly-created parent, Flux has not rendered the
342+
// quota): usage degrades to zero and behaviour matches the pre-usage gate.
343+
t.Run("absent usage degrades to budget-only check", func(t *testing.T) {
344+
parent := tenantHelmRelease(t, "foo", "tenant-root", map[string]string{"cpu": "10"})
345+
r := newTenantREST(t, parent)
346+
if errs := r.validateTenantResourceQuotas(context.Background(), childApplication(t, "bar", "tenant-foo", map[string]string{"cpu": "10"})); len(errs) > 0 {
347+
t.Fatalf("with no recorded usage a full-budget child must be allowed, got: %v", errs)
348+
}
349+
})
350+
}
351+
268352
func TestParentTenantHelmReleaseRef(t *testing.T) {
269353
tests := []struct {
270354
owned string

0 commit comments

Comments
 (0)