design-proposal: tenant quotas as reservation limits - #48
design-proposal: tenant quotas as reservation limits#48mattia-eleuteri wants to merge 3 commits into
Conversation
Tenant quotas are declared in instance-type units but enforced in pod units, and the gap between the two is KubeVirt's per-VM virtualization overhead: additive, not proportional. That is why --tenant-quota-buffer-percent cannot be set correctly, the required buffer ranging from +6% to +183% with VM granularity alone. The proposal makes reservation the accounting authority: the sum of the sizes declared in a tenant's apps.cozystack.io resources, evaluated at admission via a declarative spec.reservation block on ApplicationDefinition. The v1.6.0 hierarchical pool machinery is kept; only its usage oracle changes, which also takes ResourceQuota.status.used off the admission path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
…es split Rechecked against upstream/main (10554f9dc). The implicit md0 default is still live: #2936 made it removable and migration 47 pins it, but the kubernetes.nodeGroups helper still emits it when no pool is declared. What changes the design is that kubernetes-nodes-split phase 1 has already landed: KubernetesNodes is a registered kind whose sizing sits at the top level of its own values, so worker-pool reservation is a flat block with no iteration and no chart-computed default. Phase 2 (#3315) removes spec.nodeGroups and the implicit md0 with it. The proposal therefore no longer introduces a helper-resolution escape hatch. The evaluator stays a pure function of stored values, md0 is a transitional exception, and phase 4 of the rollout is preferably ordered after #3315. Also notes that reserving minReplicas cannot work for the cluster-autoscaler, which scales the MachineDeployment without touching the CR: the strongest argument for reserving maxReplicas. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
Per design-proposals/template.md the status transitions Draft -> Review when the PR is opened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Mattia Eleuteri <mattia@hidora.io>
📝 WalkthroughWalkthroughChangesThe proposal describes reservation-based tenant quota accounting. It defines the reservation contract, resource evaluation and aggregation APIs, admission checks, controller reporting, operational semantics, rollout phases, and validation plans. Tenant quota reservation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ApplicationDefinition
participant Admission
participant Evaluate
participant Resolver
participant Aggregator
participant TenantQuotaController
ApplicationDefinition->>Admission: submit declared reservation
Admission->>Evaluate: evaluate spec and values
Evaluate->>Resolver: resolve instance type or preset
Resolver-->>Evaluate: return resource values
Evaluate-->>Admission: return reservation
Admission->>Admission: check update reservation delta
Aggregator->>TenantQuotaController: report namespace reservations
TenantQuotaController->>TenantQuotaController: report reserved capacity and overcommit status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design-proposals/tenant-quota-reservation/README.md`:
- Line 442: Update the concurrent-admission behavior in the tenant quota
reservation proposal to avoid claiming the overshoot is limited to one
application. Specify an atomic reservation mechanism with per-pool serialization
or optimistic concurrency, or explicitly document best-effort admission
semantics and define the resulting billing, reporting, and tenant-guarantee
treatment.
- Around line 309-315: Delay enabling kubernetes reservation enforcement until
the implicit md0 default is covered: either materialize the default in
evaluation, resolve the kubernetes.nodeGroups chart helper, or exclude
kubernetes from phase-3 gating. Update the rollout and reservation gate
references so the kubernetes block cannot become enforceable while empty
nodeGroups still produce an uncharged ten-worker pool.
- Around line 152-168: Make the reservation contract versioned or immutable
after an application first uses it: preserve the selected spec.reservation shape
and referenced presets/instance types for existing applications, or persist a
reservation version/snapshot with each application and migrate changes
explicitly. Update the ApplicationDefinitionSpec reservation handling and
related accounting evaluation so later definition or preset changes cannot
silently alter an existing application’s charge.
- Around line 383-391: Clarify the coexistence behavior so the controller
disables EnforcedHard and the tenant-quota-allocated object whenever the
reservation oracle is active, while retaining both only for the legacy path.
Update the surrounding rollout description to make this gating explicit and
prevent reservation-unit values from being applied as pod-unit ResourceQuota
clamps.
- Around line 355-364: The proposed admission accounting must not use cached
HelmRelease objects or evaluated values as its authoritative reservation source.
Update parentPoolUsage and snapshot to read the committed apps.cozystack.io
custom resources—or an atomic reservation record—the admission gate evaluates,
using the same stored representation and preserving alignment between
reservation semantics and accounting.
- Line 445: Update the autoscaler behavior statement in the quota reservation
proposal to distinguish cluster-autoscaler from DHA: describe cluster-autoscaler
scale-ups as covered by the pre-reserved maxReplicas capacity if that is the
intended design, or document the admission/controller enforcement path for live
MachineDeployment scaling instead of claiming the Update gate rejects it.
- Around line 331-352: Expand the reservation contract around Aggregator and
Resolver to define how each application maps to its ApplicationDefinition and
kind, which ApplicationDefinition storage version Evaluate consumes, and how
definitions are resolved for out-of-tree kinds. Specify that ForNamespaces
aggregates only applications while excluding namespace-scoped HelmRelease
objects, so evaluation is deterministic across definition versions and resource
types.
- Around line 426-432: The reservation admission design must fail closed for
existing unevaluable or missing reservations, including out-of-tree
ApplicationDefinitions. Update the reservation evaluation and admission behavior
so missing reservations require an explicit reviewed zero-reservation exemption,
while unevaluable existing applications either block pending reservations or
retain their last known reservation until evaluation succeeds; do not rely
solely on pool conditions or in-tree completeness tests.
- Around line 393-404: Update the proposal’s capacity-guard design around
ResourceQuota, LimitRange, and AutoResourceLimitsGate so the loose operational
guard is explicitly separate from the exact tenant reservation contract. Ensure
the documented admission behavior does not claim that a tenant charged exactly
to its quota can always start workloads when Kubernetes ResourceQuota may reject
pods based on status.used; either retain the guard solely as an independent
operational limit or describe independently wired LimitRange and
AutoResourceLimitsGate behavior.
- Around line 434-440: Align the evaluator and vm-instance rendering for
applications with both instanceType and explicit resources. Update the relevant
vm-instance chart logic to omit the instance-type reference when resources take
precedence, or reject the conflicting configuration before reservation
enforcement; ensure admitted VMs do not render both fields while reservation
sizing remains based on explicit resources.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e47530bb-86d0-41a1-8c4c-0475d5344d77
📒 Files selected for processing (1)
design-proposals/tenant-quota-reservation/README.md
| ### 2. `spec.reservation` on `ApplicationDefinition` | ||
|
|
||
| Each application declares how to read its own size, next to the `openAPISchema` the definition already carries. The apiserver contains no per-kind knowledge. | ||
|
|
||
| This is the same move [PR #39](https://github.com/cozystack/community/pull/39) makes for visibility, cardinality and sharing: behavior that varies per kind becomes data on the definition rather than a branch in Go. It is also what [PR #43](https://github.com/cozystack/community/pull/43) forces, since an out-of-tree catalog cannot ship a patch to the aggregated apiserver. | ||
|
|
||
| ```go | ||
| // api/v1alpha1/applicationdefinitions_types.go | ||
|
|
||
| type ApplicationDefinitionSpec struct { | ||
| Application ApplicationDefinitionApplication `json:"application"` | ||
| Release ApplicationDefinitionRelease `json:"release"` | ||
| // Reservation declares how much this application charges against its | ||
| // tenant's quota, read from the application's own values. Absent means the | ||
| // kind reserves nothing. | ||
| // +optional | ||
| Reservation *ApplicationDefinitionReservation `json:"reservation,omitempty"` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '130,200p' design-proposals/tenant-quota-reservation/README.md
echo
echo "== reservation mentions =="
rg -n "Reservation|reservation|ApplicationDefinitionReservation|re-evaluated|re-?evaluate|snapshot|version" design-proposals/tenant-quota-reservation/README.mdRepository: cozystack/community
Length of output: 19516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== related proposal files =="
git ls-files | rg 'proposal/|application-definition-versioning|tenant-quota-reservation'
echo
echo "== application-definition-versioning mentions in repo =="
rg -n "application-definition-versioning|ApplicationSchema|storage version|spec\.reservation|reservation" . | head -200Repository: cozystack/community
Length of output: 17606
Version the reservation contract for existing applications.
Update ApplicationDefinition currently changes each application’s accounting shape. If spec.reservation is removed, changed, or its referenced presets change, existing applications can be re-evaluated with a different charge without the tenant changing app.spec. Treat the reservation contract as immutable after first use, or persist a reservation version/snapshot with each application and migrate it explicitly. Apply the same rule to referenced presets and instance types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/tenant-quota-reservation/README.md` around lines 152 - 168,
Make the reservation contract versioned or immutable after an application first
uses it: preserve the selected spec.reservation shape and referenced
presets/instance types for existing applications, or persist a reservation
version/snapshot with each application and migrate changes explicitly. Update
the ApplicationDefinitionSpec reservation handling and related accounting
evaluation so later definition or preset changes cannot silently alter an
existing application’s charge.
| **Where a values path is not enough, and why that is temporary.** The evaluator reads values, so a workload a chart synthesizes without mentioning it in the values is invisible to it. Exactly one such case exists in tree, and it is on its way out. | ||
|
|
||
| On `main` today, `kubernetes.nodeGroups` (`packages/apps/kubernetes/templates/_helpers.tpl`) emits a default `md0` pool with `maxReplicas: 10` whenever `.Values.nodeGroups` is empty. [cozystack/cozystack#2936](https://github.com/cozystack/cozystack/pull/2936) made that default *removable*, so it now applies only when no pool is declared and migration 47 pins it explicitly on existing clusters, but it is still emitted for a cluster that declares none. A literal read of `nodeGroups` therefore charges nothing for a cluster that can autoscale to ten workers, which is an under-charge and so a quota hole. | ||
|
|
||
| Two ways to close it. The narrow one is to resolve the named chart helper rather than the raw path for this single kind, which works but puts a slice of chart logic into the reservation contract. The better one is to let [`kubernetes-nodes-split`](#scope-and-related-proposals) close it: [#3315](https://github.com/cozystack/cozystack/pull/3315) removes `spec.nodeGroups` and the implicit `md0` with it, and worker pools become `KubernetesNodes` resources whose sizing sits at the top level of their own values, with nothing implicit left to resolve. | ||
|
|
||
| This proposal therefore does **not** introduce a general escape hatch. It keeps the evaluator a pure function of stored values, treats the `md0` default as a transitional exception covered by the per-kind unit test, and prefers ordering phase 4 of the [rollout](#rollout) after #3315 so the exception is never written. If #3315 slips, the narrow helper resolution is the fallback, scoped to one kind and one path. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not activate the kubernetes reservation before the md0 default is covered.
The exception becomes active in phase 3, not phase 4. Phase 2 already ships a reservation block for kubernetes (Line 465), and phase 3 enables the gate for kinds with a block. With empty nodeGroups, the chart creates md0 with ten workers while the evaluator charges zero.
Materialize the default, resolve the helper, or exclude kubernetes from reservation enforcement until kubernetes-nodes-split completes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/tenant-quota-reservation/README.md` around lines 309 - 315,
Delay enabling kubernetes reservation enforcement until the implicit md0 default
is covered: either materialize the default in evaluation, resolve the
kubernetes.nodeGroups chart helper, or exclude kubernetes from phase-3 gating.
Update the rollout and reservation gate references so the kubernetes block
cannot become enforceable while empty nodeGroups still produce an uncharged
ten-worker pool.
| `pkg/reservation` exposes three pieces, split so that the pure arithmetic is testable without a cluster: | ||
|
|
||
| ```go | ||
| // Resolver turns a size name into a resource list. | ||
| type Resolver interface { | ||
| InstanceType(ctx context.Context, name string) (corev1.ResourceList, error) | ||
| Preset(name string) (corev1.ResourceList, error) | ||
| } | ||
|
|
||
| // Evaluate applies a kind's reservation spec to one application's values. | ||
| // Pure apart from Resolver; no client, no cluster state. | ||
| func Evaluate( | ||
| ctx context.Context, | ||
| spec *v1alpha1.ApplicationDefinitionReservation, | ||
| values map[string]any, | ||
| r Resolver, | ||
| ) (corev1.ResourceList, error) | ||
|
|
||
| // Aggregator sums the reservations of every application in a set of namespaces. | ||
| type Aggregator interface { | ||
| ForNamespaces(ctx context.Context, namespaces []string) (map[string]corev1.ResourceList, error) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,200p'
echo "== target context =="
if [ -f design-proposals/tenant-quota-reservation/README.md ]; then
wc -l design-proposals/tenant-quota-reservation/README.md
sed -n '300,370p' design-proposals/tenant-quota-reservation/README.md
fi
echo "== related references =="
rg -n "ApplicationDefinitionReservation|ForNamespaces|Resolver|Evaluate\\(" design-proposals/tenant-quota-reservation/README.md pkg/reservation -S || true
echo "== search relevant types =="
rg -n "type .*ApplicationDefinitionReservation|ApplicationDefinitionReservation|HelmRelease|Application|storage version|storageVersion|values|valuesPath|ApplicationDefinition" -S --glob '!vendor/**' --glob '!node_modules/**' | head -n 200Repository: cozystack/community
Length of output: 7953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact proposal section and nearby definitions if present.
[ -f design-proposals/tenant-quota-reservation/README.md ] && {
echo "== lines 1-80 =="
sed -n '1,80p' design-proposals/tenant-quota-reservation/README.md
echo "== lines 200-320 =="
sed -n '200,320p' design-proposals/tenant-quota-reservation/README.md
}
# Programmatic check: ensure the described interfaces exist and no mapping fields are present in the snippet.
python3 - <<'PY'
from pathlib import Path
import re
p = Path("design-proposals/tenant-quota-reservation/README.md")
if not p.exists():
raise SystemExit("README missing")
text = p.read_text()
start = text.find("func Evaluate(")
end = text.find("###")
if start < 0:
raise SystemExit("Evaluate signature missing")
snippet = text[max(0, start-1200): end if end > 0 else start+1200]
print("has Evaluate signature:", "func Evaluate(" in snippet)
print("has Aggregator signature:", "type Aggregator interface" in snippet)
print("has Resolver signature:", "type Resolver interface" in snippet)
for name in ["ApplicationDefinition", "HelmRelease", "storageVersion", "values", "kind", "set"]:
print(f"{name}: {text.count(name)}")
PYRepository: cozystack/community
Length of output: 16677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target section =="
sed -n '130,190p' design-proposals/tenant-quota-reservation/README.md
sed -n '328,380p' design-proposals/tenant-quota-reservation/README.md
echo "== deterministic contract parse =="
python3 - <<'PY'
from pathlib import Path
import re
text = Path("design-proposals/tenant-quota-reservation/README.md").read_text()
interfaces = {}
for m in re.finditer(r'(?m)^type\s+(\w+)\s+interface\s*\{(.*?)\n\}', text, re.S):
name = m.group(1)
body = m.group(2)
interfaces[name] = {
"methods": re.findall(r'(?m)^\s*(\w+)\([^)]*\)', body),
"args_strings": " ".join(body.split()),
}
print("interfaces:", list(interfaces.keys()))
for name, data in interfaces.items():
if name in {"Resolver", "Evaluate", "Aggregator"}:
print(f"== {name} ==")
for line in data["args_strings"].split()[:100]:
print(line)
section = text[text.find("# 4. Reservation as the usage oracle"):text.find("### 5. Generalizing")]
required_terms = {
"storage version": [],
"ApplicationSchema": [],
"ApplicationDefinition": [],
"version lookup": [],
"values mapping": [],
"valuesPath": [],
"HelmRelease kind": [],
"exclude HelmRelease": [],
"out-of-tree": [],
}
for term, found in required_terms.items():
hit = any(term.lower() in section.lower() or (term in text) for _ in [1])
print(f"{term}: {hit}")
PYRepository: cozystack/community
Length of output: 7414
Complete the Aggregator contract before implementation.
Evaluate needs an ApplicationDefinitionReservation, values, and a resolver. Aggregator.ForNamespaces receives only namespace names, and Resolver does not define how application/kind definitions are looked up. Define the application-to-definition mapping, which ApplicationDefinition storage version is used for reservation, and how the aggregator excludes only applications and keeps namespace-scoped HelmRelease objects out of the summation.
Without that mapping, out-of-tree kinds and application-definition versioning cannot be evaluated deterministically.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/tenant-quota-reservation/README.md` around lines 331 - 352,
Expand the reservation contract around Aggregator and Resolver to define how
each application maps to its ApplicationDefinition and kind, which
ApplicationDefinition storage version Evaluate consumes, and how definitions are
resolved for out-of-tree kinds. Specify that ForNamespaces aggregates only
applications while excluding namespace-scoped HelmRelease objects, so evaluation
is deterministic across definition versions and resource types.
| The two call sites change source, not shape: | ||
|
|
||
| | Call site | Today | After | | ||
| |---|---|---| | ||
| | `quota.go` `parentPoolUsage` | lists `ResourceQuota` per member namespace, sums `status.used`, keys via `renderedLimitKey` | lists HelmReleases per member namespace, evaluates each, sums in shorthand keys | | ||
| | `reconciler.go` `snapshot` | lists all `ResourceQuota`, builds `usedByNS` from `status.used` | builds `usedByNS` from the aggregator | | ||
|
|
||
| `renderedLimitKey` and its `rawQuotaKeys` companion are deleted: with both sides in shorthand there is nothing to bridge. | ||
|
|
||
| This also removes uncached reads from the admission path. `parentPoolUsage` today deliberately uses the direct watch client `r.w` for ResourceQuotas, with the comment that the aggregated apiserver "must not spin up a cluster-wide ResourceQuota informer just for admission". HelmReleases already have an informer, since `siblingDeclaredQuotas` uses the cached client `r.c` for them, so the new oracle reads from cache where the old one could not. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files matching README:"
fd -a 'README.md' . | sed 's#^\./##' | rg 'design-proposals/tenant-quota-reservation/README.md|tenant-quota-reservation' || true
echo
echo "Relevant proposal section:"
sed -n '1,420p' design-proposals/tenant-quota-reservation/README.md | nl -ba | sed -n '300,375p'
echo
echo "Search reservation goal/source terms:"
rg -n "reserved|deallocate|reservation|HelmRelease|ResourceQuota|snapshot|parentPoolUsage|declared" design-proposals/tenant-quota-reservation/README.mdRepository: cozystack/community
Length of output: 324
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant proposal section:"
sed -n '300,375p' design-proposals/tenant-quota-reservation/README.md | awk '{printf "%7d %s\n", NR+299, $0}'
echo
echo "Search reservation goal/source terms:"
rg -n "reserved|deallocate|reservation|HelmRelease|ResourceQuota|snapshot|parentPoolUsage|declared|uses|source|api server|inform" design-proposals/tenant-quota-reservation/README.mdRepository: cozystack/community
Length of output: 34916
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "HelmRelease-related references in repository:"
rg -n "HelmRelease|helm release|helm releases|applications|apps\." -S . --glob '!**/.git/**' | head -200Repository: cozystack/community
Length of output: 38962
Use an authoritative reservation source instead of caching HelmReleases.
The proposal makes apps.cozystack.io custom resources the admission source, but the call-site change reads cached HelmRelease objects and evaluates their values. HelmRelease objects are not the committed source record; Flux reconciliation or conversion can lag admission, and cached reads can hide a missing or stale values projection. Use the same stored source the gate evaluates, or an atomic reservation record, so admission accounting and reservation semantics stay aligned.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/tenant-quota-reservation/README.md` around lines 355 - 364,
The proposed admission accounting must not use cached HelmRelease objects or
evaluated values as its authoritative reservation source. Update parentPoolUsage
and snapshot to read the committed apps.cozystack.io custom resources—or an
atomic reservation record—the admission gate evaluates, using the same stored
representation and preserving alignment between reservation semantics and
accounting.
| ### 6. What the controller becomes | ||
|
|
||
| Feeding `usedByNS` in instance-type units while `EnforcedHard` still writes a `ResourceQuota` enforced against pods would reintroduce the same unit mismatch one level up: the clamp would be computed from reservations and applied to launcher requests. So the controller must stop being an enforcement point. | ||
|
|
||
| It can. Once the gate covers every kind, pool sharing between unbounded siblings is already enforced at admission, because every application create in every member namespace is checked against the pool's reservation. `EnforcedHard`, `upsertAllocatedQuota`, `gcAllocatedQuotas` and the `tenant-quota-allocated` object become redundant and are removed. | ||
|
|
||
| The controller becomes an observer. It publishes `reserved` against `budget` per pool, and keeps reporting `Overcommitted`, the one case no admission check can prevent, since it arises when a parent lowers its quota after children have already carved out slices. | ||
|
|
||
| During the flag-gated coexistence period `EnforcedHard` stays in place so the legacy path is not left without a runtime net; its removal is the last rollout step. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files matching README:"
git ls-files | rg '(^|/)design-proposals/tenant-quota-reservation/README\.md$|tenant-quota|quota|EnforcedHard|tenant-quota-allocated' || true
echo
echo "Section around proposed lines:"
sed -n '350,410p' design-proposals/tenant-quota-reservation/README.md
echo
echo "Search for EnforcedHard and allocated quota:"
rg -n "EnforcedHard|tenant-quota-allocated|overcommitted|reservation|usedByNS|ResourceQuota|pod[s]?" design-proposals/tenant-quota-reservation/README.md . -S --glob '!node_modules' --glob '!dist' --glob '!build' | head -n 200Repository: cozystack/community
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find candidate implementations:"
git ls-files | rg '(^|/)(reconciler|quota|quot)' | head -n 100 || true
echo
echo "Search EnforcedHard in tracked Go/controller files only:"
rg -n "EnforcedHard|tenant-quota-allocated|upsertAllocatedQuota|gcAllocatedQuotas|usedByNS|resourceQuotas|Enforced|Hard" internal pkg --glob '*.go' -S || true
echo
echo "Search tests mentioning legacy/flag/allocated/enforced:"
rg -n "legacy|EnforcedHard|tenant-quota-allocated|flag|reservation" internal pkg --glob '*_test.go' * --glob '*.go' -S | head -n 200 || trueRepository: cozystack/community
Length of output: 506
Keep EnforcedHard on the legacy path only.
The proposal says EnforcedHard stays during the flag-gated coexistence period so the legacy path keeps a runtime net. Make that explicit: turn off EnforcedHard and tenant-quota-allocated when the reservation oracle is active. Otherwise reservation units become pod-unit ResourceQuota clamps and the mismatch reappears.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/tenant-quota-reservation/README.md` around lines 383 - 391,
Clarify the coexistence behavior so the controller disables EnforcedHard and the
tenant-quota-allocated object whenever the reservation oracle is active, while
retaining both only for the legacy path. Update the surrounding rollout
description to make this gating explicit and prevent reservation-unit values
from being applied as pod-unit ResourceQuota clamps.
| ### 7. The `ResourceQuota` becomes a capacity guard | ||
|
|
||
| The chart-rendered `tenant-quota` is kept, deliberately loose, for two reasons that have nothing to do with the tenant contract: | ||
|
|
||
| - The `LimitRange` providing default container requests is rendered under the same `if .Values.resourceQuotas` guard. Dropping the quota would drop the defaults. | ||
| - `AutoResourceLimitsGate` only sets limits on virt-launcher pods when the namespace has a quota constraining `limits.*`. Dropping the quota would silently change the QoS class of every VM on the platform. | ||
|
|
||
| It cannot be made exact. An exact overhead allowance requires knowing how many VMs the tenant runs, and the quota is rendered by a Helm chart that does not know. A second quota object cannot compensate either, because Kubernetes applies the **most restrictive** quota in a namespace: an additional object can only tighten, never loosen. | ||
|
|
||
| So the guard is rendered from `resourceQuotas` multiplied by a wide, platform-configurable factor, and documented as a guard rather than a contract. Its imprecision is the point: it exists to stop an unbounded runaway and to keep `AutoResourceLimitsGate` armed, not to decide what a tenant may claim. | ||
|
|
||
| The inflation factor therefore changes role rather than disappearing. It moves from "the tenant contract must be falsified", where no value is correct, to "the guard is deliberately slack", where imprecision is the desired property. That requalification is what makes `--tenant-quota-buffer-percent` obsolete, not its literal deletion. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="design-proposals/tenant-quota-reservation/README.md"
if [ ! -f "$file" ]; then
echo "file not found: $file"
git ls-files | rg 'tenant-quota-reservation|README\.md|quota|ResourceQuota|AutoResourceLimitsGate' || true
else
echo "--- file exists: $file ($(wc -l < "$file") lines) ---"
echo "--- around lines 80-120 ---"
sed -n '80,120p' "$file" | nl -ba -v80
echo "--- around lines 360-420 ---"
sed -n '360,420p' "$file" | nl -ba -v360
echo "--- key terms "---
rg -n "ResourceQuota|tenant-quota|AutoResourceLimitsGate|--tenant-quota-buffer-percent|reservation contract|exact|status.used|quota" "$file" -C 2
fiRepository: cozystack/community
Length of output: 311
🌐 Web query:
Kubernetes ResourceQuota admission status.used rejects pod after update quota exceeded
💡 Result:
In Kubernetes, the ResourceQuota admission controller prevents resource consumption from exceeding defined namespace limits by rejecting requests that would violate these constraints [1][2]. If a creation or update request would cause a namespace to exceed its ResourceQuota, the Kubernetes API server will reject that request with an HTTP 403 Forbidden error and a message detailing the violated constraint [1]. The following key points summarize how ResourceQuota interacts with pod lifecycle events and updates: Admission and Rejection: The ResourceQuota plugin introspects incoming requests at the API server level [2]. If accepting a new pod or an update to an existing pod (such as a resource resize) would cause the total usage to exceed the hard limit set in the ResourceQuota, the admission controller blocks the request before it is committed to etcd [2][3]. Updates and Resizing: With the support for in-place pod resource updates, the ResourceQuota admission controller is designed to validate that the requested change (e.g., increasing CPU/memory requests) does not violate the namespace's current quota [3][4]. If an update request exceeds the available quota, the admission controller will reject the update [4]. Existing Resources and Quota Changes: Changes to ResourceQuota limits or the deletion of pods are handled differently. If you reduce the quota limits such that existing pods already exceed the new, lower limit, the existing pods are not affected or evicted; ResourceQuota admission control only enforces limits on new creations or updates to resources [5]. Consistency: ResourceQuota is designed to be eventually consistent [6]. The API server performs atomic quota reservation during the admission phase, while a background ResourceQuota controller periodically synchronizes the status field to reflect actual usage, which is necessary for reconciling changes after deletions [2][7]. Because of this design, there can be brief periods where observed usage and quota status might slightly diverge, but the admission controller remains the primary mechanism for preventing new violations [6][7]. Top results: [1][2][3][4]
Citations:
- 1: https://kubernetes.io/docs/concepts/policy/resource-quotas/
- 2: https://github.com/kubernetes/design-proposals-archive/blob/main/resource-management/admission_control_resource_quota.md
- 3: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/1287-in-place-update-pod-resources/README.md
- 4: https://github.com/kubernetes/enhancements/blob/8bb838caac1251be03785c8974ad72a3e73af629/keps/sig-node/20181106-in-place-update-of-pod-resources.md
- 5: https://kubernetes.ltd/docs/concepts/policy/resource-quotas/
- 6: resource quota may exceed it's limit while deployment rollingupdate kubernetes/kubernetes#123434
- 7: Attempting to resubmit Pod causes incorrect ResourceQuota usage kubernetes/kubernetes#76787
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="design-proposals/tenant-quota-reservation/README.md"
sed -n '100,115p' "$file" > /tmp/section_guarantees.txt
sed -n '386,404p' "$file" > /tmp/section_guard.txt
echo "--- lines 100-115 ---"
awk '{printf "%4d %s\n", NR+100, $0}' /tmp/section_guarantees.txt
echo "--- lines 386-404 ---"
awk '{printf "%4d %s\n", NR+386, $0}' /tmp/section_guard.txt
echo "--- terms around quota admission/status/guard/contract ---"
grep -nEi "status\.used|ResourceQuota|quota|AutoResourceLimitsGate|reservation contract|exact|garant|reject|rejects|deny|block|contract" "$file"Repository: cozystack/community
Length of output: 33433
🌐 Web query:
Kubernetes ResourceQuota does not support in place update request changes update resource limit quota check on status.used
💡 Result:
In Kubernetes, ResourceQuota status.used is managed as an eventually consistent system [1][2], and it does not always update instantaneously in response to resource changes [1][3]. The system relies on a combination of admission controllers and a background reconciliation loop to maintain these values [2][4]. 1. Admission Control vs. Controller Reconciliation: When a resource (like a Pod) is created or updated, the ResourceQuota admission controller attempts to intercept the request and increment the used quota values immediately [1][4]. However, because this is an eventually consistent system, there are scenarios where the status.used may not perfectly reflect the actual state of the cluster at every microsecond [1][2]. 2. Reconciliation Loop: The ResourceQuota controller periodically performs a full re-calculation (reconciliation) of usage across the namespace to correct any mismatches that occur [1][2]. If a change happens that the admission controller fails to account for—or if a transient error causes a discrepancy—the background controller eventually syncs the status.used to match the actual resource usage in the cluster [2][3]. 3. In-Place Updates: When features like In-Place Pod Vertical Scaling are used, the quota controller is designed to monitor pod status updates (specifically when container resource status is populated or changed) to ensure that the ResourceQuota.status.used remains accurate [5]. Without this tracking, there could be delays or inaccuracies in reporting usage as pods resize [5]. 4. Performance Considerations: While users might desire perfectly synchronous updates, the design of the quota system avoids complex distributed locking or atomic synchronous updates to every involved component to maintain performance [1]. Consequently, if you observe a temporary mismatch between your actual resources and the status.used field, it is typically resolved by the controller during its next sync period [1][2]. If you are seeing persistent inaccuracies that do not resolve after a reasonable amount of time, it is generally considered a drift in the controller's observed state, which the background synchronization loop is explicitly designed to rectify [2][3].
Citations:
- 1: resource quota may exceed it's limit while deployment rollingupdate kubernetes/kubernetes#123434
- 2: Rollback ResourceQuota changes in admission if persisting to etcd fails kubernetes/kubernetes#110080
- 3: resource quota update does not take effect until create/delete another ResourceQuota kubernetes/kubernetes#6845
- 4: Attempting to resubmit Pod causes incorrect ResourceQuota usage kubernetes/kubernetes#76787
- 5: Fix ResourceQuota update filter missing initial container status resource population kubernetes/kubernetes#140155
Make the capacity guard explicit and avoid contradicting the tenant contract.
The current goal states a tenant charged exactly to its quota can start all workloads, but tenant-quota is still a Kubernetes ResourceQuota that can reject pods via status.used. Keep the loose guard as a separate operational limit, or wire LimitRange and AutoResourceLimitsGate independently, so the exact reservation admission result is not mixed with Kubernetes pod quota admission.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/tenant-quota-reservation/README.md` around lines 393 - 404,
Update the proposal’s capacity-guard design around ResourceQuota, LimitRange,
and AutoResourceLimitsGate so the loose operational guard is explicitly separate
from the exact tenant reservation contract. Ensure the documented admission
behavior does not claim that a tenant charged exactly to its quota can always
start workloads when Kubernetes ResourceQuota may reject pods based on
status.used; either retain the guard solely as an independent operational limit
or describe independently wired LimitRange and AutoResourceLimitsGate behavior.
| ## Security | ||
|
|
||
| - **No new tenant-supplied input.** The reservation is computed from values that already pass the kind's OpenAPI schema. Tenants gain no new field. | ||
| - **`spec.reservation` is platform-authored.** It lives on a cluster-scoped `ApplicationDefinition`, which tenants cannot write. | ||
| - **A missing or wrong reservation block under-charges a tenant**, which is a quota-escalation vector: an application kind that reserves nothing is free. This is the main new risk. It is mitigated by a completeness test asserting that every in-tree application kind has a `reservation` block, gating each rollout step, and by the fact that an absent block is a visible `0` in the pool's `reserved` report rather than a silent pass. | ||
| - **The gate must fail closed on the object being written.** Today `siblingDeclaredQuotas` deliberately skips siblings whose values do not parse, with a warning, so that "a malformed sibling must not block an unrelated tenant write". That fail-open choice is right for a sibling and wrong for the object under admission: applied to reservation it would make an unparseable custom resource free. The proposed behavior is to reject when the object being written cannot be evaluated, and to warn plus set a pool condition when another object cannot be, so the under-count is surfaced instead of hidden. | ||
| - **RBAC surface is unchanged.** The gate reads HelmReleases with the apiserver's existing service account, as it already does for siblings and pool usage. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate README and related proposal files =="
git ls-files | rg 'design-proposals/tenant-quota-reservation|tenant-quota-reservation|quota.*reservation|reservation'
echo
echo "== relevant README excerpt =="
sed -n '380,450p' design-proposals/tenant-quota-reservation/README.md 2>/dev/null || true
echo
echo "== search for reservation handling / siblingDeclaredQuotas / in-tree tests =="
rg -n "spec\.reservation|reservation|siblingDeclaredQuotas|completeness|in-tree|in tree|out-of-tree|unevaluable|skip|warning|pool condition|reserved" design-proposals/tenant-quota-reservation -S || true
echo
echo "== list all files under proposal =="
git ls-files design-proposals/tenant-quota-reservation 2>/dev/null | sortRepository: cozystack/community
Length of output: 31737
🌐 Web query:
tenant quota reservation out-of-tree application kinds spec.reservation in-tree completeness test proposal
💡 Result:
In the context of Kubernetes and broader distributed system architecture, there is no single unified standard named spec.reservation for tenant quota reservations, nor is there a widely documented "completeness test" specifically tied to this phrase. The terminology likely originates from one of two contexts: 1. Distributed Systems Design Patterns: In complex systems (such as agent loops or OpenStack Neutron), "reservations" are frequently used to handle pre-commit accounting—where a system reserves resources (e.g., tokens, CPU, or memory) against a tenant quota before an operation is dispatched [1][2]. A "completeness test" in this context typically refers to the validation logic required to ensure that a reservation request, once reconciled or fulfilled, does not result in a state that violates total tenant capacity or exceeds the requested "worst-case" budget [1][2]. 2. Kubernetes Out-of-Tree Integrations: The Kubernetes ecosystem (notably via KEP-3521 and projects like Kueue) has long grappled with the limitations of the in-tree ResourceQuota system [3][4][5]. Efforts to support "out-of-tree" quota solutions involve creating extensible, generic quota APIs that allow external controllers (like batch schedulers) to manage resources that the core Kubernetes scheduler cannot natively track (e.g., licenses or external cluster-scoped assets) [4][6][5]. In these designs, "reservation" concepts are often introduced to bridge the gap between namespace-scoped in-tree ResourceQuotas and external, tenant-wide policy enforcement [4]. If you are encountering these terms in a specific proposal or codebase, it is likely an internal architectural requirement for ensuring that an out-of-tree admission controller or quota manager correctly accounts for resource claims across both standard in-tree resources and custom application kinds [4][7]. Top results: [1], [2], [4], [6], [5]
Citations:
- 1: https://static.opendev.org/docs/neutron/8.3.0/devref/quota.html
- 2: https://tianpan.co/blog/2026-04-23-rate-limit-hierarchy-collapse-agent-self-dos
- 3: https://kubernetes.io/docs/concepts/policy/resource-quotas/
- 4: [KEP-3521]: Integrate well with out-of-tree resource quota solutions kubernetes/kubernetes#131835
- 5: Support specifying resource requests via annotation for non-schedulable resources kubernetes-sigs/kueue#6160
- 6: Support cluster scoped ResourceQuota equivalent kubernetes/kubernetes#134720
- 7: https://github.com/kubernetes/design-proposals-archive/blob/main/api-machinery/admission_control_extension.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== outline for all files =="
git ls-files | sort
echo
echo "== reservation references across repo =="
rg -n "siblingDeclaredQuotas|parseDeclaredQuotas|spec\.reservation|reservation|unparseable|malformed|pool condition|complete|completeness" -S \
--glob '!design-proposals/tenant-quota-reservation/README.md' \
--glob '!**/vendor/**' \
--glob '!**/.git/**' . \
| head -n 200
echo
echo "== relevant docs for deployment/package model =="
git ls-files | rg 'packages|internal|pkg|README|manifest|openapi|application-definition|ApplicationDefinition' | head -n 200Repository: cozystack/community
Length of output: 3439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== full relevant reservation safety section =="
sed -n '152,312p' design-proposals/tenant-quota-reservation/README.md
echo
echo "== full rollout, failing and edge cases, testing sections =="
sed -n '428,456p' design-proposals/tenant-quota-reservation/README.md
sed -n '338,400p' design-proposals/tenant-quota-reservation/README.mdRepository: cozystack/community
Length of output: 17035
Fail closed for missing or unevaluable existing reservations.
The completeness test pins only in-tree kinds. An out-of-tree ApplicationDefinition can omit spec.reservation and reserve zero, and a pool condition does not prevent another write from consuming falsely available headroom. Require an explicit, reviewed zero-reservation exemption. For unevaluable existing applications, block pending reservations or retain the last known reservation until evaluation succeeds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/tenant-quota-reservation/README.md` around lines 426 - 432,
The reservation admission design must fail closed for existing unevaluable or
missing reservations, including out-of-tree ApplicationDefinitions. Update the
reservation evaluation and admission behavior so missing reservations require an
explicit reviewed zero-reservation exemption, while unevaluable existing
applications either block pending reservations or retain their last known
reservation until evaluation succeeds; do not rely solely on pool conditions or
in-tree completeness tests.
| ## Failure and edge cases | ||
|
|
||
| - Application kind with no `reservation` block → reserves nothing; the completeness test prevents this shipping for in-tree kinds. | ||
| - `instanceType` names a `VirtualMachineClusterInstancetype` that does not exist → rejected at admission with the resolver's error, instead of later by the chart's `lookup` failure in `templates/vm.yaml`. | ||
| - Both `instanceType` and `resources` set → the evaluator charges `resources`, matching the precedence the `kubernetes` chart documents on `nodeGroups[].resources`. Note that `packages/apps/vm-instance` does not currently omit the instancetype in that case the way the `kubernetes` chart does, so the rendered `VirtualMachine` carries both and KubeVirt cannot reconcile them; tightening that chart is out of scope here but worth a separate fix, and the reservation charges the same number either way. | ||
| - Explicit `resources` with no `instanceType` → charged from `resources`, matching `virtual-machine.domainResources` in `packages/apps/vm-instance/templates/_helpers.tpl`. | ||
| - **`nodeGroups: {}` on a `kubernetes` application, while the parent chart still owns pools** → the chart emits a default `md0` group with `maxReplicas: 10`, so a literal read charges nothing for a cluster that can autoscale to ten workers. Covered by the per-kind unit test and resolved for good by [#3315](https://github.com/cozystack/cozystack/pull/3315); see [§2](#2-specreservation-on-applicationdefinition). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Repository files matching vm/instance/quota names =="
git ls-files | rg -n 'design-proposals/tenant-quota-reservation/README.md|vm-instance|VirtualMachine|instancetype|reservation|nodeGroups|resources' | head -200
echo
echo "== Proposal relevant lines =="
sed -n '410,455p' design-proposals/tenant-quota-reservation/README.md
echo
echo "== Search evaluator/chart precedence handling =="
rg -n "resources|instanceType|nodeGroups|VirtualMachineClusterInstancetype|virtual-machine.domainResources|kubernetes chart|omit the instancetype|reservation|charges|precedence" design-proposals/tenant-quota-reservation README.md packages/apps -S | head -250Repository: cozystack/community
Length of output: 31792
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Remote package files =="
curl -fsSL 'https://raw.githubusercontent.com/cozystack/cozystack/main/packages/apps/vm-instance/templates/vm.yaml' -o /tmp/vm.yaml
curl -fsSL 'https://raw.githubusercontent.com/cozystack/cozystack/main/packages/apps/vm-instance/templates/_helpers.tpl' -o /tmp/helpers.tpl
curl -fsSL 'https://raw.githubusercontent.com/cozystack/cozystack/main/packages/apps/vm-instance/values.yaml' -o /tmp/defaults.yaml
echo "== vm.yaml vm block =="
python3 - <<'PY'
from pathlib import Path
p=Path('/tmp/vm.yaml')
for i,line in enumerate(p.read_text().splitlines(),1):
if 'VirtualMachine' in line or 'instanceType' in line or 'domain' in line:
lo=max(1,i-8); hi=min(i+16,p.read_text().count('\n')+1)
print(f'--- {p}:{lo}-{hi}')
for j,l in enumerate(p.read_text().splitlines()[lo-1:hi],lo):
print(f'{j}: {l}')
PY
echo "== helpers relevant vm/resources sections =="
python3 - <<'PY'
from pathlib import Path
text=Path('/tmp/helpers.tpl').read_text()
for term in ['virtual-machine.domainResources', 'virtual-machine.instanceType', 'virtual-machine.resources', 'VmResources', 'domainResources', 'instanceType', 'nodeGroups']:
idx=text.find(term)
if idx!=-1:
start=max(0,text.rfind('\n',0,idx-200)); end=min(len(text),text.find('\n',idx+500))
print(f'--- around {term}')
print(text[start:end])
PY
echo "== vm defaults =="
python3 - <<'PY'
from pathlib import Path
import yaml, re
text=Path('/tmp/defaults.yaml').read_text()
lines=text.splitlines()
for i,line in enumerate(lines,1):
if re.search(r'instanceType:|resources:|namePreset:|resourcesPreset:', line):
print(f'{i}: {line}')
PY
echo
echo "== Check if KubeVirt source docs this specific precedence/rejection in repo =="
tmpdir=$(mktemp -d)
git clone --depth 1 --no-single-branch https://github.com/kubevirt/kubevirt.git "$tmpdir" >/tmp/clone.log 2>&1
rg -n "instanceType|VirtualMachine.*instanceType|cannot reconcile|rejects|instanceType.*resources|domain resources|domainResources|resourcesPreset|resourcesPresetMap" "$tmpdir" -S \
--glob '*.go' --glob '*.md' | head -300 || trueRepository: cozystack/community
Length of output: 16135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Fetch KubeVirt source snippets =="
curl -fsSL 'https://raw.githubusercontent.com/kubevirt/kubevirt/main/pkg/virt-api/webhooks/mutating-webhook/webhooks/virtualmachine/virtualmachine_create_admit.go' -o /tmp/vm_create_admit.go
curl -fsSL 'https://raw.githubusercontent.com/kubevirt/kubevirt/main/pkg/virt-controller/services/renderresources/renderresources.go' -o /tmp/renderresources.go
echo
echo "== Search KubeVirt instanceType/resources precedence/rejection =="
rg -n "instanceType|resourcesPreset|resourcesPresetMap|domainResources|explicit resources|resources.*memory|spec\.domain\.resources|instancetype" /tmp/vm_create_admit.go /tmp/renderresources.go -S
echo
echo "== Fetch renderresources around relevant lines =="
python3 - <<'PY'
from pathlib import Path
for name in ['/tmp/vm_create_admit.go', '/tmp/renderresources.go']:
text=Path(name).read_text()
lines=text.splitlines()
matches=[]
q='resourcesPreset'
for i,l in enumerate(lines,1):
if q in l:
lo=max(1,i-18); hi=min(len(lines),i+35)
print(f'--- {name}:{lo}-{hi}')
for j,line in enumerate(lines[lo-1:hi],lo):
print(f'{j}: {line}')
PYRepository: cozystack/community
Length of output: 245
🌐 Web query:
KubeVirt VirtualMachine instanceType resourcesPreset explicit domain resources precedence
💡 Result:
In KubeVirt, the precedence and interaction of resources are determined by whether you are using the modern InstanceType/Preference framework or the older, deprecated VirtualMachineInstancePreset mechanism [1]. InstanceType Precedence: When using VirtualMachineInstancetype (or VirtualMachineClusterInstancetype), the resources defined within the instance type are mandatory [2][3]. Anything defined in the instance type cannot be overridden within the VirtualMachine spec [2][1]. Specifically, because CPU and memory are required attributes of an instance type, if you attempt to define these resources within the underlying VirtualMachine, a conflict will occur, and the creation of the VM will be rejected [2][4]. VirtualMachinePreference Precedence: In contrast, VirtualMachinePreference (or VirtualMachineClusterPreference) defines preferred values for attributes [1][3]. Unlike instance types, preferences only provide defaults and can be explicitly overridden by values provided by the user in the VirtualMachine spec [1][3]. Deprecated VirtualMachineInstancePreset: The VirtualMachineInstancePreset CRD is deprecated [1]. It operated using a conflict resolution strategy similar to Kubernetes PodPresets [5]. If a setting (such as CPU or memory) was present in both the VirtualMachineInstance and the Preset, KubeVirt would check for identity [5]. If the values differed, it would result in a conflict error, and the VirtualMachineInstance would fail to enter a running state [5]. General Resource Precedence: When defining resources directly in the VirtualMachine spec (outside of the InstanceType framework), if both spec.domain.memory.guest and spec.domain.resources.requests.memory are set, the memory requests take precedence [6].
Citations:
- 1: https://kubevirt.io/2022/KubeVirt-Introduction-of-instancetypes.html
- 2: https://kubevirt.io/user-guide/user_workloads/instancetypes/
- 3: https://www.bookstack.cn/read/kubevirt-1.5-en/14ae817759cd9ea5.md
- 4: https://www.bookstack.cn/read/kubevirt-1.1-en/f0b70c85cd710b74.md
- 5: https://kubevirt.io/user-guide/user_workloads/presets/
- 6: https://kubevirt.io/user-guide/compute/resources_requests_and_limits/
🌐 Web query:
VirtualMachine resourcesPreset CPU memory conflicts with custom spec.domain.resources instanceType KubeVirt
💡 Result:
In KubeVirt, conflicts between resource definitions arise primarily when using Instance Types, which are the recommended mechanism for defining VM resources [1][2]. If you reference an Instance Type (via VirtualMachineInstancetype or VirtualMachineClusterInstancetype), any CPU or Memory resources defined within that Instance Type are mandatory [3][2]. Consequently, if you attempt to define conflicting CPU or Memory requests within the VirtualMachine's own spec.domain.resources, the request will be rejected by the KubeVirt API [3][1][4]. You cannot override resources encapsulated by an Instance Type [3][2]. Regarding older mechanisms: - VirtualMachineInstancePresets: These are deprecated in favor of Instance Types [1]. When used, if a Preset and the VirtualMachine define different settings for the same domain spec elements, it results in a merge conflict [5][6]. KubeVirt will typically generate a warning event, and the VirtualMachine may enter a Failed state rather than applying the conflicting configuration [5][7]. For standard resource management without Instance Types or Presets: - KubeVirt requires memory to be specified in either spec.domain.memory.guest or spec.domain.resources.requests.memory (with requests taking precedence) [8]. - For CPU, you should not specify both spec.domain.cpu and spec.domain.resources.[requests/limits].cpu; consistent definitions are required, or the configuration will be rejected [9]. To resolve conflicts when using Instance Types, remove the manual resource definitions from your VirtualMachine's spec.domain.resources, as the Instance Type is intended to be the single source of truth for these attributes [3][2]. If you require different resource profiles, you should reference a different Instance Type [2].
Citations:
- 1: https://kubevirt.io/2022/KubeVirt-Introduction-of-instancetypes.html
- 2: https://blog.yarwood.me.uk/presentations/kubevirt/summit/2023/presentation.html
- 3: https://kubevirt.io/user-guide/user_workloads/instancetypes/
- 4: https://blog.yarwood.me.uk/2022/08/03/kubevirt_instancetype_demo_2/
- 5: https://kubevirt.io/user-guide/user_workloads/presets/
- 6: https://github.com/kubevirt/kubevirt/blob/master/docs/vm-presets.md
- 7: https://master--kubevirt-user-guide.netlify.app/virtual_machines/presets/
- 8: https://kubevirt.io/user-guide/compute/resources_requests_and_limits/
- 9: https://kubevirt.io/user-guide/compute/dedicated_cpu_resources/
Align evaluator and vm-instance precedence for conflicting fields.
When instanceType and explicit resources are both set, the evaluator charges explicit resources; vm-instance still renders both the instance-type ref and explicit domain.resources. KubeVirt treats explicit resources as conflicting against an instance type, so the VM is rejected after admission and the workload does not run. Reconcile reservation sizing with chart rendering by rejecting the conflict first or omitting instance type when explicit resources take precedence before enabling reservation enforcement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/tenant-quota-reservation/README.md` around lines 434 - 440,
Align the evaluator and vm-instance rendering for applications with both
instanceType and explicit resources. Update the relevant vm-instance chart logic
to omit the instance-type reference when resources take precedence, or reject
the conflicting configuration before reservation enforcement; ensure admitted
VMs do not render both fields while reservation sizing remains based on explicit
resources.
| - Explicit `resources` with no `instanceType` → charged from `resources`, matching `virtual-machine.domainResources` in `packages/apps/vm-instance/templates/_helpers.tpl`. | ||
| - **`nodeGroups: {}` on a `kubernetes` application, while the parent chart still owns pools** → the chart emits a default `md0` group with `maxReplicas: 10`, so a literal read charges nothing for a cluster that can autoscale to ten workers. Covered by the per-kind unit test and resolved for good by [#3315](https://github.com/cozystack/cozystack/pull/3315); see [§2](#2-specreservation-on-applicationdefinition). | ||
| - **A `KubernetesNodes` pool whose name collides with a `nodeGroup` still owned by the parent** → the render already fails on ownership conflict, as its own schema documents. Reservation would have charged the pool twice, once per owner, so failing early is the correct outcome and no special case is needed. | ||
| - Concurrent creates in different member namespaces of one pool → both may pass, overshooting by at most one application. The controller reports the overshoot; nothing is evicted. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Correct the concurrent-admission overshoot bound.
Without per-pool serialization or optimistic concurrency, any number of requests can observe the same reservation snapshot and pass. Overshoot can equal the sum of all accepted reservations, not one application. Updates can also introduce large deltas.
Use an atomic reservation mechanism, or document best-effort semantics and define how overshoot affects billing, reporting, and tenant guarantees.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/tenant-quota-reservation/README.md` at line 442, Update the
concurrent-admission behavior in the tenant quota reservation proposal to avoid
claiming the overshoot is limited to one application. Specify an atomic
reservation mechanism with per-pool serialization or optimistic concurrency, or
explicitly document best-effort admission semantics and define the resulting
billing, reporting, and tenant-guarantee treatment.
| - Concurrent creates in different member namespaces of one pool → both may pass, overshooting by at most one application. The controller reports the overshoot; nothing is evicted. | ||
| - Parent lowers its quota below existing carve-outs → `Overcommitted` reports it, as today. No retroactive enforcement. | ||
| - `vm-disk` resized upward → the `Update` path charges the delta; a downward resize releases it. | ||
| - Autoscaler (DHA or cluster-autoscaler) raises `replicas` past the pool budget → the `Update` is rejected and the autoscaler surfaces the failure, rather than the scale-up silently failing at pod admission. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate README =="
fd -a 'README.md' . | sed 's#^\./##' | rg 'tenant|quota|reservation|design-proposals' || true
echo "== Relevant lines =="
if [ -f design-proposals/tenant-quota-reservation/README.md ]; then
sed -n '420,485p' design-proposals/tenant-quota-reservation/README.md | nl -ba -v420
fi
echo "== Search for cluster-autoscaler, autoscaler, MachineDeployment text =="
rg -n "cluster-autoscaler|autoscaler|MachineDeployment|maxReplicas|pre-reserved|Update|admission" design-proposals/tenant-q* tenant* -S 2>/dev/null | head -200Repository: cozystack/community
Length of output: 843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant lines =="
sed -n '420,485p' design-proposals/tenant-quota-reservation/README.md
echo "== Search for autoscaler/MachineDeployment wording =="
rg -n "cluster-autoscaler|autoscaler|MachineDeployment|maxReplicas|pre-reserved|Update|admission" design-proposals/tenant-quota-reservation/README.md design-proposals -S | head -240Repository: cozystack/community
Length of output: 50375
🌐 Web query:
CAPI cluster-autoscaler MachineDeployment replicas spec minMaxReplicas changes live MachineDeployment without updating Application CR
💡 Result:
When using the Kubernetes Cluster Autoscaler with Cluster API (CAPI) MachineDeployments, how the replicas field is managed depends on whether you are using ClusterClass and Managed Topologies [1][2]. If you are using ClusterClass/Managed Topologies, you must leave the replicas field unset in the Cluster definition (specifically under spec.topology.workers.machineDeployments[].replicas) to allow the Cluster Autoscaler to take control [1][2]. If this field is set in the Cluster definition, the CAPI topology controller will enforce that value on the MachineDeployment, which can interfere with the autoscaler and lead to unpredictable scaling behavior [3][4][5]. When the replicas field is left unset in the Cluster topology: 1. The topology controller does not enforce a value on the MachineDeployment replicas field [5]. 2. The Cluster Autoscaler is then permitted to modify the MachineDeployment replicas field directly [5]. 3. The MachineDeployment uses specific defaulting logic for its replicas field when it is created or updated, which considers the autoscaler's min/max size annotations [6][7]. For example, if the autoscaler annotations (e.g., cluster-autoscaler.kubernetes.io/cluster-api-machine-deployment-name-min-size and max-size) are present, the system attempts to maintain the replica count within that range during creation or updates [6][3][7]. In summary, you should not update the Application CR (Cluster object) replicas field to scale the cluster; instead, you should leave it unset in the Cluster definition and ensure the required autoscaler annotations are applied to the MachineDeployment [1][2][5]. The Cluster Autoscaler will then manage the live MachineDeployment replicas field automatically [5].
Citations:
- 1: https://cluster-api.sigs.k8s.io/tasks/automated-machine-management/autoscaling
- 2: https://github.com/kubernetes/autoscaler/blob/master/cluster-autoscaler/cloudprovider/clusterapi/README.md
- 3: MachineDeployment.spec.replicas defaulting should take into account autoscaler min/max size if defined kubernetes-sigs/cluster-api#7293
- 4: https://release-1-5.cluster-api.sigs.k8s.io/tasks/automated-machine-management/autoscaling
- 5: Unexpected behaviour using kubectl scale with a topology managed cluster kubernetes-sigs/cluster-api#7395
- 6: https://github.com/kubernetes-sigs/cluster-api/blob/main/config/crd/bases/cluster.x-k8s.io_machinedeployments.yaml
- 7: 🌱 MD: improve replica defaulting for autoscaler kubernetes-sigs/cluster-api#7990
Split the cluster-autoscaler scale-up behavior from DHA.
Line 445 says cluster-autoscaler scale-ups are rejected by the Update gate, but line 474 says cluster-autoscaler scales the live MachineDeployment without updating the application CR. If maxReplicas is pre-reserved, state that coverage instead of claiming Update rejection. Otherwise, document the required admission/controller path for live MachineDeployment scaling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/tenant-quota-reservation/README.md` at line 445, Update the
autoscaler behavior statement in the quota reservation proposal to distinguish
cluster-autoscaler from DHA: describe cluster-autoscaler scale-ups as covered by
the pre-reserved maxReplicas capacity if that is the intended design, or
document the admission/controller enforcement path for live MachineDeployment
scaling instead of claiming the Update gate rejects it.
1 participant
What this PR does
Adds a design proposal under
design-proposals/tenant-quota-reservation/proposing that tenant quotas become reservation limits: consumption is the sum of the sizes declared in a tenant'sapps.cozystack.ioresources, evaluated at admission, rather than the requests and limits of the pods those resources produce.Verified against
mainat10554f9dc.The problem, in one number
A tenant quota is declared in instance-type units (
memory: 16Gi, what the tenant buys and the dashboard shows) but enforced in pod units, because it is ultimately compared againstResourceQuota.status.used. A virt-launcher pod requests the guest memory plus KubeVirt's virtualization overhead, which is additive per VM rather than proportional to the amount reserved. So a tenant whose quota is fully allocated to declared VMs cannot start the last one.--tenant-quota-buffer-percentexists to absorb that, and cannot be set correctly, because the required buffer depends on VM granularity rather than volume. For a 16Gi quota, at roughly 468Mi of overhead per launcher:Any single value is simultaneously too tight for tenants running many small VMs and too generous for tenants running few large ones. The knob is not mistuned, it is the wrong shape for the error it corrects.
There is a second symptom with the same root cause:
ResourceQuota.status.useddrifts permanently and is never recomputed. Since v1.6.0 reads it from the admission path (parentPoolUsage), a stale counter in any pool-member namespace can now forbid the creation of a legitimate sub-tenant, with an error blaming the parent's quota. A platform bug in a leaf namespace has become an onboarding failure.What it proposes
The hierarchical pool machinery in
internal/controller/tenantquotais kept as-is, and only its usage oracle changes. The declaration gate already speaks the reservation vocabulary —parseDeclaredQuotassays theflattenexpansion is "intentionally not applied here" — so the arithmetic is already right; pod units enter at exactly two call sites.spec.reservationblock onApplicationDefinition, so no per-kind Go lives in the aggregated apiserver.parentPoolUsageandsnapshotsum declared reservations instead ofstatus.used;renderedLimitKeyis deleted. This also takes uncached reads off the admission path, since HelmReleases already have an informer where ResourceQuotas deliberately do not.Tenantto every kind, onCreateandUpdate, charging only the delta on update.ResourceQuotais kept, deliberately loose, as a capacity guard. It has to stay: theLimitRangesits under the sameif .Values.resourceQuotasguard, andAutoResourceLimitsGateonly sets limits on virt-launcher when the namespace has a quota constraininglimits.*.--tenant-quota-buffer-percentbecomes unnecessary and is deprecated in the last rollout phase.What it deliberately does not do
It does not fix the kube-controller-manager counter staleness, introduce usage-based quotas, or evict already-admitted workloads when a quota is lowered. It takes the stale counter out of the tenant contract and off the admission path; fixing it remains separate work.
Ordering against in-flight work
The proposal is written to compose with what is already in review, and two of those change it materially:
switchover kinds in the apiserver cannot describe an app whose package lives in another repository.extraintoapps) sets the precedent this follows, and removes one of the open questions outright: once tenant modules are applications they carry their own reservation block instead of being special-cased from the Tenant's boolean flags.kubernetes-nodes-split: phase 1 has landed, so worker-pool reservation is already a flat block onKubernetesNodes. Phase 2 (feat(kubernetes)!: manage worker pools as KubernetesNodes, adopt on upgrade (Phase 2, breaking) cozystack#3315) removesspec.nodeGroupsand the implicitmd0with it, which retires the one place where reading values is not sufficient. Ordering phase 4 of the rollout after #3315 is preferable.external: trueboolean theobjects:block counts today.proposal/application-definition-versioningdecides where the block lives: values are converted to a single storage version, so reservation is declared once against the storage form.Where reviewer input is most useful
maxReplicasfor an autoscaled pool charges idle tenants for headroom. ReservingminReplicascannot work, though: the cluster-autoscaler scales the MachineDeployment without touching the CR, so no admission gate ever sees it. Is charging the maximum the right trade?md0instance, and it would equally help the versioning proposal._resourcepresets.tpl, or export it as a ConfigMap from the platform chart?LimitRangeandAutoResourceLimitsGatebe wired independently ofresourceQuotas?Summary by CodeRabbit