Repository navigation
fix(backups): give the barman-cloud sidecar its own resources - #4220
Aleksei Sviridkin (lexfrei) merged 2 commits into
Conversation
github-actions
Bot
added
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change adds CPU and memory requirements to barman-cloud sidecar configurations. Controller types, generated deep-copy code, Helm templates, and PostgreSQL and Keycloak tests validate the settings. ChangesBarman sidecar resources
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Sidecars receive the intended CPU and memory settings across controller, PostgreSQL, and Keycloak paths; no actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 5 files. (3 skipped: 3 unsupported.) ✨ 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 |
|
Would a maintainer consider adding The sidecar is injected without resources on the 1.6 line as well, and the chart path there already carries the checksum pin since v1.6.2, so without a backport the two halves would land in different releases. I don't have permission to set the label myself. |
scooby87
left a comment
scooby87
left a comment
There was a problem hiding this comment.
LGTM (independent cozy-review pass).
Clean, narrowly-scoped bugfix for the silent barman-cloud sidecar OOMKill (default 128Mi from the tenant LimitRange) while Cluster and ObjectStore stayed Ready. The cozy-lib.barman helper rename is clean (grep shows zero dangling references, all three include sites migrated), zz_generated.deepcopy.go is correctly regenerated, and the plugin-barman-cloud CRD accepts instanceSidecarConfiguration.resources (structural schema, not pruned) so the fix actually applies.
Tests are non-vacuous: three separate mutations each turn the right assertion RED. postgres 69/69, keycloak 58/58.
Non-blocking: the 1Gi limit is hardcoded in the helper (not per-chart configurable) — fine as a large improvement over 128Mi; and omitempty on the non-pointer Resources struct is a no-op (harmless here).
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM. The numbers are fine, the reason given for them is wrong in one of the two charts that use the helper, and that text becomes the merge commit.
Business context: the barman-cloud plugin injects its sidecar with no resources, so where a namespace default caps it at 128Mi the sidecar is OOMKilled mid-backup while the ObjectStore and the Cluster stay healthy.
On the numbers. packages/apps/tenant/templates/quota.yaml sets only default (cpu 250m, memory 128Mi, ephemeral-storage 2Gi) and defaultRequest (cpu 25m, memory 128Mi, ephemeral-storage 50Mi) on the LimitRange. No min, no max, no maxLimitRequestRatio, so it can only fill in absent fields and has nothing to reject against: 100m/256Mi requests and a 1Gi limit go in verbatim. The ResourceQuota next to it is a namespace aggregate, not a per-container ceiling, so 1Gi alone cannot make a pod unschedulable. It does change the bill: cozy-lib.resources.sanitize expands a tenant's memory: 4Gi into hard limits.memory and requests.memory, and each sidecar now charges 1Gi against the limits budget instead of 128Mi. A quarter of a 4Gi tenant per instance pod, and the PR does not mention it.
Blockers
B1: the LimitRange reason does not apply to the keycloak caller
packages/system/keycloak/templates/db.yaml:54 says the sidecar is sized off the tenant LimitRange default, and the shared helper's doc comment says the same. The Keycloak database is not in a tenant namespace.
Evidence: kind: LimitRange appears exactly once in the tree, in packages/apps/tenant/templates/quota.yaml, and packages/apps/tenant/templates/_helpers.tpl fails the render unless the release namespace starts with tenant-. So that LimitRange can only exist in a tenant-* namespace. Keycloak runs in cozy-keycloak (packages/core/platform/sources/keycloak.yaml, and the suite pins the same namespace). A tenant that leaves resourceQuotas at its {} default is in the same position: quota.yaml gates the ResourceQuota and the LimitRange on that value, so neither object is rendered.
In those namespaces the sidecar had no requests and no limits at all, and this gives it a hard 1Gi ceiling where there was none. Probably fine, 254 MiB measured is well under it. But it is a different change from raising a 128Mi ceiling, nothing argues it, and the PR body lands in git log as the merge commit message.
Fix: say per caller what applies, in the Keycloak comment and in the body. Tenant LimitRange on the tenant path; for a namespace without one, the reason is that an uncapped sidecar should still declare requests. One sentence on why 1Gi is the right ceiling for a caller that had none closes it.
Non-blocking follow-ups
- 1Gi is four times the single measurement (254 MiB high-water on a 38 MB database) and the helper hard-codes it with no per-chart knob. Fine as a default, worth another look when someone backs up a large database this way.
- The workflows carrying the unit and controller tests are
action_requiredon this head. Nothing red, but the suites this PR adds have never run in CI. - I cannot rerun the reported failure (OOMKilled, exit 137, four restarts, 254 MiB) and took those numbers as given.
The rest checks out. Three include sites, all migrated, no dangling reference to the old helper name. The plugin CRD carries instanceSidecarConfiguration.resources, so nothing is pruned. The chart helper and barmanSidecarConfiguration() agree field for field, including the missing CPU limit. The entityOperator block in the kafka chart says what the body claims. The tests bite: dropping resources from either builder, changing the CPU request, adding a CPU limit, and dropping the deepcopy line each turn exactly the expected assertion red.
| # Pin the barman-cloud sidecar's S3 request checksum to when_required | ||
| # (rationale in cozy-lib.barman.checksumSidecarConfiguration). | ||
| {{- include "cozy-lib.barman.checksumSidecarConfiguration" . | nindent 2 }} | ||
| # Size the barman-cloud sidecar off the tenant LimitRange default and pin its |
There was a problem hiding this comment.
The Keycloak database runs in cozy-keycloak. The only LimitRange in the tree is the one in the tenant chart, and tenant.name refuses to render outside a tenant-* namespace, so nothing defaults this sidecar to 128Mi here. Before this change it had no limit at all; now it has 1Gi.
There was a problem hiding this comment.
Right. cozy-keycloak has no LimitRange, and a tenant with resourceQuotas: {} has none either, so there the sidecar had no requests and no limit, and this gives it a 1Gi ceiling where there was none. The comment and the helper doc now say so per caller, and the body carries the quota accounting: 1Gi per instance pod against limits.memory.
36f9bd1 to
24828b3
Compare
Aleksei Sviridkin (lexfrei)
added
the
kind/backport
|
Wording fixed in the Keycloak comment, the Postgres comment, the cozy-lib doc block and the Go doc, and the body now says per caller what applies and what the 1Gi limit costs a tenant. No rendered object changes; keycloak 58/58 and postgres 69/69 locally. |
The barman-cloud plugin injects its sidecar without resource requests or limits. Tenant namespaces ship a LimitRange that defaults containers to 128Mi, so the sidecar inherits that default and is OOMKilled mid-backup: the ObjectStore stays healthy, the Cluster stays Ready, and only the backup fails. A cgroup high-water mark of 254 MiB on a 38 MB database shows the default cannot hold it. The shared cozy-lib helper and the Go path that builds the platform's own ObjectStore now request 100m/256Mi and cap memory at 1Gi. No CPU limit is set, matching the entityOperator precedent in the kafka chart: a throttled sidecar stalls WAL archiving instead of failing it. The helper is renamed from checksumSidecarConfiguration to sidecarConfiguration, since it no longer carries only the checksum pin. Signed-off-by: Yan Bondarenko <[email protected]>
The Keycloak comment and the helper doc claimed the tenant LimitRange as the reason for the resources, but cozy-keycloak has no LimitRange and neither does a tenant that leaves resourceQuotas empty: there the sidecar had no requests and no limit at all, and the change gives it a 1Gi ceiling where there was none. The comments and both doc blocks now say what applies to each caller. No rendered object changes. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]>
0bd6ae0 to
4dc6065
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. B1 is closed: the Keycloak comment and the Go doc each say what applies on their own path, and the body carries the quota cost.
I re-checked the facts the argument rests on at this head, since the branch moved onto a newer main: the only LimitRange in the tree is still the tenant chart's, still gated on resourceQuotas and still without min/max/maxLimitRequestRatio, and the kafka entityOperator precedent still sets no CPU limit. The new commit is comments only, so "no rendered object changes" holds.
One thing about the postgres suite. Remove the whole resources: block from the helper and the notExists on limits.cpu stays green, because it cannot tell an absent input from a rendered absence. It catches the CPU-limit case only because the equality assert next to it pins the block.
The red E2E is not yours. That run has 55 passes and one failure, redis-2-backup-roundtrip, and postgres-2-backup-roundtrip is among the passes. The failure is in that test's own read helper and is tracked separately. "Unit & controller tests" is green here, which closes my earlier note about those suites never having run.
3219f7e
into
cozystack:main
|
Successfully created backport PR for |
3 participants
What this PR does
The barman-cloud plugin injects its sidecar with no resource requests or limits. What that means depends on the namespace. A tenant with
resourceQuotasset ships aLimitRangethat defaults containers to 128Mi (packages/apps/tenant/templates/quota.yaml), so there the sidecar inherits that default and is OOMKilled mid-backup. A namespace without aLimitRange, which iscozy-keycloakand any tenant that leavesresourceQuotasempty, gave the sidecar no requests and no limit at all, so nothing reserved memory for it and nothing bounded it. The failure on the tenant path is quiet from the control plane: theObjectStorestays healthy, theClusterstaysReady, and only the backup fails.Observed on a 1.6 cluster while backing up a 38 MB database:
lastState.terminated.reason: OOMKilled,exit 137, four restarts on the sidecar, and a cgroup high-water mark of 254 MiB.container_memory_working_set_bytespeaked at only 64 MiB over the same window — it is sampled, and it misses the spike that actually triggers the kill, which is why the working-set number does not explain the failure on its own.Both paths that build an ObjectStore now carry resources:
cozy-lib.barman.sidecarConfiguration, used bypackages/apps/postgresfor the backup and recovery ObjectStores and bypackages/system/keycloak;barmanSidecarConfiguration()ininternal/backupcontroller/cnpgstrategy_controller.go, which builds the platform's own ObjectStore on theuseSystemBucket=truepath.Requests are 100m/256Mi with a 1Gi memory limit on every caller: 256Mi holds the measured working set and 1Gi is four times the measurement, which is also the ceiling a caller without a LimitRange gets where it had none. No CPU limit is set, following the
entityOperatorprecedent inpackages/apps/kafka/templates/kafka.yaml: a throttled sidecar stalls WAL archiving instead of failing it, which is harder to notice than an outright failure. Measured CPU peak was 50 mCPU. Inside a tenant the limit is charged against theResourceQuotalimits.memorybudget, 1Gi per instance pod; a 4Gi tenant spends a quarter of it per Postgres instance.The helper is renamed from
checksumSidecarConfigurationtosidecarConfiguration, because it no longer carries only the checksum pin and its name and doc comment would otherwise be wrong.Tests cover both paths and both render sites: a new helm suite in
packages/apps/postgres, extra assertions in the existing keycloak suite, an assertion in the controller test, and a deepcopy test for the new field. Each of them fails against the unfixed sources.One thing this PR deliberately does not do. This is the fifth time the 128Mi tenant default has been worked around per component — kafka and zookeeper presets (#2537), the kafka entity-operator (#2934), the cert-manager cainjector (#3199), and the note carried in the etcd-operator values. Whether the default itself should move looks like your call rather than something to fold into a fix for one sidecar, so it is left alone here.
Screenshots
Not a UI change.
Downstream repositories
Walked the trigger map in
docs/agents/contributing.mdagainst the diff, entry by entry. The change touches one cozy-lib helper, the two chart templates that include it, one Go function, and tests. It adds, renames or removes no package; changes novalues.yaml,values.schema.json,Chart.yamlorREADME.md, so no version enum, default or generated artifact moves; changes noApplicationDefinitionsemantics, norelease.prefix, no output Secret or Service name; touches no CRD, no namespace, nohack/file, no telemetry metric or label, nocozy-proxyannotation, no node or network requirement, and no commit or PR convention. No entry matches.Release note
Summary by CodeRabbit
New Features
Bug Fixes