fix(kafka): bump default resourcesPreset to medium - #2537
Conversation
The `small` preset (512Mi) is not sufficient for the Strimzi 0.45 / Kafka 3.9 image baseline. With the default Strimzi heap formula (Kafka 50%, ZooKeeper 75% of container memory limit), the remaining non-heap budget (Metaspace, code cache, direct buffers, JMX javaagent, embedded Jetty admin server, KRaft migration endpoints, thread stacks) consistently overflows the 512Mi cgroup limit at startup, leading to OOMKilled (exit 137) and CrashLoopBackOff for both Kafka and ZooKeeper pods. Bump the default `resourcesPreset` for kafka and zookeeper from `small` (512Mi) to `medium` (1Gi). The `small` preset stays in the enum so existing deployments that pin it are unaffected; only the default changes. Signed-off-by: Ivan Okhotnikov <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request updates the default Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses stability issues in fresh Kafka deployments caused by insufficient memory resources under the previous default configuration. By increasing the default resource preset, the application now provides adequate headroom for the JVM non-heap requirements in modern Kafka/Strimzi images, preventing startup failures while maintaining backward compatibility for existing deployments. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Clean default change. All generated artifacts (CRD types, JSON schema, cozyrd OpenAPI, README, values.yaml) are consistent, and existing deployments with an explicit resourcesPreset are unaffected since kubebuilder defaults only apply on CREATE. The Strimzi heap-fraction reasoning (50% Kafka, 75% ZooKeeper) matches upstream DYNAMIC_HEAP_FRACTION defaults; at 512Mi the non-heap budget (256Mi for Kafka, 128Mi for ZooKeeper) is too tight for the Java 17 footprint of Strimzi 0.45 / Kafka 3.9, and doubling memory while holding CPU is a conservative, reversible fix. LGTM.
|
Backport failed for |
|
Successfully created backport PR for |
## 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 `resourceQuotas` set ships a `LimitRange` that 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 a `LimitRange`, which is `cozy-keycloak` and any tenant that leaves `resourceQuotas` empty, 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: the `ObjectStore` stays healthy, the `Cluster` stays `Ready`, 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_bytes` peaked 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 by `packages/apps/postgres` for the backup and recovery ObjectStores and by `packages/system/keycloak`; - `barmanSidecarConfiguration()` in `internal/backupcontroller/cnpgstrategy_controller.go`, which builds the platform's own ObjectStore on the `useSystemBucket=true` path. 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 `entityOperator` precedent in `packages/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 the `ResourceQuota` `limits.memory` budget, 1Gi per instance pod; a 4Gi tenant spends a quarter of it per Postgres instance. The helper is renamed from `checksumSidecarConfiguration` to `sidecarConfiguration`, 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.md` against 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 no `values.yaml`, `values.schema.json`, `Chart.yaml` or `README.md`, so no version enum, default or generated artifact moves; changes no `ApplicationDefinition` semantics, no `release.prefix`, no output Secret or Service name; touches no CRD, no namespace, no `hack/` file, no telemetry metric or label, no `cozy-proxy` annotation, no node or network requirement, and no commit or PR convention. No entry matches. - [x] No downstream repository is affected by this change ### Release note ```release-note fix(backups): give the barman-cloud sidecar its own resources The barman-cloud sidecar was injected without resource requests or limits, so in tenant namespaces it inherited the 128Mi `LimitRange` default and was OOMKilled during backups while the `ObjectStore` and the `Cluster` both stayed healthy. It now requests 100m/256Mi and caps memory at 1Gi on the chart path and on the platform's Go path alike; in namespaces without a LimitRange the sidecar had no limit before and gets the same 1Gi ceiling. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added explicit CPU and memory resource settings for barman-cloud backup and recovery sidecars. - Backup sidecars request 100m CPU and 256Mi memory, with a 1Gi memory limit. - Recovery sidecars receive a 1Gi memory limit. - Preserved the S3 checksum configuration. - **Bug Fixes** - Prevented sidecars from inheriting unsuitable tenant resource limits. - Ensured resource settings are preserved when backup configuration is copied. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Changes the default
resourcesPresetfor bothkafkaandzookeeperin the Kafka managed application fromsmall(1 CPU / 512Mi) tomedium(1 CPU / 1Gi).The
smallpreset is no longer sufficient for the current Strimzi 0.45 / Kafka 3.9 image baseline:/v1/readyand KRaft migration endpoints, thread stacks) systematically exceeds those budgets at startup, triggering a cgroup OOM kill (exit 137) before the broker can connect to ZooKeeper.Result on a fresh deployment with the previous default: every Kafka broker and every ZooKeeper pod ends up in
CrashLoopBackOff, withlastState.terminated.reason: OOMKilled. ZooKeeper occasionally hitsOOMKilledonce quorum traffic ramps up, breaking the quorum and causing Kafka brokers to fail withZooKeeperClientTimeoutException.The
mediumpreset (1Gi) is the smallest preset that gives both Kafka (heap 512Mi + ~500Mi non-heap budget) and ZooKeeper (heap 768Mi + 256Mi non-heap budget) enough headroom to start cleanly. Users who need more can still picklarge,xlarge, or2xlarge, or overrideresourcesexplicitly.The
smallpreset is intentionally kept in the enum so existing deployments that pinresourcesPreset: smallare not broken; only the default changes.Release note
Summary by CodeRabbit
Chores
Documentation