design-proposal: quota and accounting for backup storage - #50
design-proposal: quota and accounting for backup storage#50Timofei Larkin (lllamnyp) wants to merge 3 commits into
Conversation
Cozystack can back up managed applications but cannot limit or report how much backup storage a tenant consumes. The proposal separates the two halves of the problem. Enforcement attaches to BackupJob CREATE, which the tenant initiates; accounting reads Backup, which the strategy controller writes. Gating Backup CREATE instead — the one dimension a CRD gets for free — would reject a run's result after the artifact already exists, orphaning it in object storage. The accounting half is the hard one. Summing sizeBytes over live Backup objects is wrong in both directions: for every driver except Velero, deleting a Backup leaves the archive in place, so a tenant can reset the counter with one command; and driver-side retention prunes archives without deleting the CR, so the ledger overcounts. The proposal promotes per-driver artifact ownership to a declared strategy capability and splits usage into attributed and retained bytes accordingly. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
The first draft used a dedicated BackupQuota CRD on the assumption that ResourceQuota.status could not be safely co-written by a third controller. Reading upstream at v1.35 shows the assumption was wrong. syncResourceQuota seeds status.used from what is already published, overwrites only the keys its own evaluators computed, and masks the result to the key set of spec.hard — so a dimension present in spec.hard with no evaluator is carried through untouched, and CalculateUsage skips unrecognised names without erroring. The admission plugin adds a delta to the existing status.used rather than recomputing it, so foreign keys survive there too. hasUsageStats only inspects resources the incoming object's own evaluator matches, so a lagging backup figure cannot 403 a Pod. Two consequences the design now leans on: kube-controller-manager mirrors spec.hard into status.hard, so only status.used needs writing; and the dimensions are inert to kube-apiserver, which makes visibility and enforcement independently switchable and the phased rollout real. Records the residual risk — this is emergent behaviour, not a documented guarantee — as an integration test that must fail loudly on a Kubernetes bump. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
#48 moves tenant quota accounting off ResourceQuota.status.used and onto declared reservations; this proposal adds a consumer of status.used. They are complementary — a VM's memory is declared before it exists, a backup's size is only known after — but the resulting split between declared and measured accounting should be an explicit decision rather than the accidental outcome of two proposals landing separately. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
What this PR does
Adds a design proposal under
design-proposals/backup-storage-quota/forlimiting and reporting how much backup storage a tenant consumes.
Cozystack can back up managed applications, but nothing caps the resulting
storage and nothing reports it. A tenant with a cron
Planand a large databasefills the operator's object storage without ever meeting a limit or seeing a
number.
The shape of the problem
Enforcement is the easy half. The accounting is not, and that is where the
proposal spends its length.
Gate the request, account the artifact. A CRD gets one quota dimension for
free —
count/backups.backups.cozystack.io— and using it is a trap: it fireson
BackupCREATE, which the strategy controller issues when a run finishes,not the tenant. A tenant at their limit would watch jobs run to completion and
then fail to record the result, orphaning artifacts in object storage with no CR
referencing them. Enforcement therefore attaches to
BackupJobCREATE;accounting reads
Backup.Summing
sizeBytesover liveBackupobjects is wrong in both directions.backup_controller.go's cleanup switch is explicit, per driver: the Altinitybranch notes deletion "does NOT purge the upstream clickhouse-backup archive in
object storage"; MariaDB "does not own the archive, so it does not delete it on
CR removal"; FoundationDB says the same. Those branches exist specifically to
avoid falling through to the Velero default — which does clean up, and is the
one driver where deleting a
Backupgenuinely releases bytes.So on every driver but one,
kubectl delete backup --allresets the counterwhile the bytes remain. That is a one-command bypass, not a documentation
footnote. Inversely, driver-side retention prunes archives without deleting the
CR, so the ledger overcounts.
The proposal promotes per-driver artifact ownership — knowledge the code already
has, as prose in a switch statement — to a declared strategy capability, and
splits usage into attributed (released when the CR goes) and retained
(deleted CR, driver still holds the bytes).
used = attributed + retained, sodeleting CRs moves bytes between buckets and frees nothing.
Where the numbers live
corev1.ResourceQuota, via two extended dimensions(
backups.cozystack.io/size,backups.cozystack.io/count) — not a new CRD.An earlier draft used a dedicated CRD on the assumption that
ResourceQuota.statuscould not be safely co-written by a third controller.Reading upstream at
v1.35showed the assumption was wrong, and the proposalquotes the relevant code:
syncResourceQuotaseedsusedfrom what is already published, overwritesonly the keys its own evaluators computed, and masks to the key set of
spec.hard. A dimension inspec.hardwith no evaluator is carried throughuntouched;
CalculateUsageskips unrecognised names without erroring.Status.Usedrather thanrecomputing it, so foreign keys survive there too.
hasUsageStatsonly inspects resources the incoming object's evaluatormatches, so a lagging backup figure can never 403 a Pod.
Two consequences the design leans on: kube-controller-manager mirrors
spec.Hardintostatus.Hard, so onlystatus.usedneeds writing; and thedimensions are inert to kube-apiserver, which makes visibility and enforcement
independently switchable — an operator can turn on measurement, size a real
limit against observed data, and only then enable the webhook.
This is emergent behaviour rather than a documented API guarantee, so the
Testing section calls for an integration test against a real
kube-controller-manager that asserts a synthetic dimension survives a resync —
gating the release, not just the merge.
Relationship to #48
#48 moves tenant quota
accounting off
status.usedand onto declared reservations; this proposaladds a consumer of
status.used. They are complementary, because the resourcesdiffer in the property that decides the mechanism: a VM's memory is declared
before it exists, while a backup's size is only known after. If both are
accepted the rule becomes "reservation-based where size is declared, measured
where it is discovered" — which is coherent, but ought to be decided explicitly
rather than emerge from two proposals landing separately. Reviewers of either
should say whether they accept it.
Open questions I would most like input on
attributed/retainedsplit is the right model, or heavier thanthe problem deserves.
StrategyCRs, onBackupClass, or in a controller-side registry.countshould use the native object-count dimension after all,accepting the
Backup-CREATE failure mode to avoid a duplicate mechanism.Verified against
cozystack/cozystackat04b742670and Kubernetesv1.35.2.