Repository navigation
fix(kafka): set entity-operator resources to avoid OOM under tenant LimitRange - #2934
Conversation
|
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 (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThe Strimzi Kafka Helm template adds explicit CPU/memory requests and a 512Mi memory limit to both ChangesKafka entityOperator Resource Constraints
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Suggested reviewers
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 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 |
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 an issue where the Kafka entity-operator containers would crash due to insufficient memory when running in tenant namespaces with default LimitRanges. By explicitly defining resource requirements, the entity-operator can now start reliably, ensuring that KafkaTopic and KafkaUser resources are properly reconciled. Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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
|
There was a problem hiding this comment.
Code Review
This pull request configures explicit resource requests and limits for the topicOperator and userOperator in the Kafka entity operator template to prevent OOMKill issues caused by default LimitRanges in tenant namespaces. It also adds a corresponding test case to verify the memory limits. The reviewer suggests expanding the test assertions to also verify the CPU and memory requests for both operators.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| - equal: | ||
| path: spec.entityOperator.topicOperator.resources.limits.memory | ||
| value: 512Mi | ||
| - equal: | ||
| path: spec.entityOperator.userOperator.resources.limits.memory | ||
| value: 512Mi |
There was a problem hiding this comment.
To ensure comprehensive test coverage and prevent regressions, consider also asserting the CPU and memory requests for both the topicOperator and userOperator.
- equal:
path: spec.entityOperator.topicOperator.resources.requests.cpu
value: 100m
- equal:
path: spec.entityOperator.topicOperator.resources.requests.memory
value: 256Mi
- equal:
path: spec.entityOperator.topicOperator.resources.limits.memory
value: 512Mi
- equal:
path: spec.entityOperator.userOperator.resources.requests.cpu
value: 100m
- equal:
path: spec.entityOperator.userOperator.resources.requests.memory
value: 256Mi
- equal:
path: spec.entityOperator.userOperator.resources.limits.memory
value: 512MiThere was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/apps/kafka/tests/entityoperator_template_test.yaml (1)
24-34: ⚡ Quick winExpand assertions to cover requested resources too.
The test currently validates only
limits.memory, so it can miss regressions inrequests.cpuandrequests.memoryeven though those are part of the explicit resource contract.Suggested test extension
- it: topic-operator and user-operator set explicit resources (avoid 128Mi LimitRange OOM) release: name: test-kafka namespace: tenant-test asserts: + - equal: + path: spec.entityOperator.topicOperator.resources.requests.cpu + value: 100m + - equal: + path: spec.entityOperator.topicOperator.resources.requests.memory + value: 256Mi - equal: path: spec.entityOperator.topicOperator.resources.limits.memory value: 512Mi + - equal: + path: spec.entityOperator.userOperator.resources.requests.cpu + value: 100m + - equal: + path: spec.entityOperator.userOperator.resources.requests.memory + value: 256Mi - equal: path: spec.entityOperator.userOperator.resources.limits.memory value: 512Mi🤖 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 `@packages/apps/kafka/tests/entityoperator_template_test.yaml` around lines 24 - 34, The test case "topic-operator and user-operator set explicit resources (avoid 128Mi LimitRange OOM)" currently only validates limits.memory for both topicOperator and userOperator, but does not verify the requested resources (requests.cpu and requests.memory). Add four additional equal assertions to the asserts section to validate spec.entityOperator.topicOperator.resources.requests.cpu, spec.entityOperator.topicOperator.resources.requests.memory, spec.entityOperator.userOperator.resources.requests.cpu, and spec.entityOperator.userOperator.resources.requests.memory with their expected values to ensure complete coverage of the explicit resource contract and prevent regressions.
🤖 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.
Nitpick comments:
In `@packages/apps/kafka/tests/entityoperator_template_test.yaml`:
- Around line 24-34: The test case "topic-operator and user-operator set
explicit resources (avoid 128Mi LimitRange OOM)" currently only validates
limits.memory for both topicOperator and userOperator, but does not verify the
requested resources (requests.cpu and requests.memory). Add four additional
equal assertions to the asserts section to validate
spec.entityOperator.topicOperator.resources.requests.cpu,
spec.entityOperator.topicOperator.resources.requests.memory,
spec.entityOperator.userOperator.resources.requests.cpu, and
spec.entityOperator.userOperator.resources.requests.memory with their expected
values to ensure complete coverage of the explicit resource contract and prevent
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d983303-5a66-4164-ad8a-85f58b84883e
📒 Files selected for processing (2)
packages/apps/kafka/templates/kafka.yamlpackages/apps/kafka/tests/entityoperator_template_test.yaml
…imitRange entityOperator.topicOperator/userOperator carried no resources, so in tenant namespaces (the LimitRange defaults containers to 128Mi) the JVM topic-operator and user-operator are OOMKilled on startup. The entity-operator stays in CrashLoopBackOff, KafkaTopics/KafkaUsers never reconcile, and on teardown topics get stuck on the strimzi.io/topic-operator finalizer. Set explicit requests/limits (256Mi/512Mi) so they no longer inherit the namespace default; add a template test. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
3ee03da to
1ab5f17
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM (APPROVE)
Business context: entityOperator.topicOperator/userOperator rendered without resources, so under the tenant LimitRange default (memory: 128Mi) the topic-operator and user-operator JVM containers OOMKill and never reconcile KafkaTopic/KafkaUser. This PR sets explicit resources so they no longer inherit that default.
Blockers: none.
The problem is real and the fix is plumbed correctly:
- The tenant LimitRange applies
default.memory: 128Mito every container, which the JVM operator containers inherit and OOM on. Evidence: packages/apps/tenant/templates/quota.yaml:18-26. resourcesis a valid field under bothtopicOperatoranduserOperatorinkafka.strimzi.io/v1beta2, so the values reach the operator containers. Evidence: packages/system/kafka-operator/charts/strimzi-kafka-operator/crds/040-Crd-kafka.yaml:3463,3653.- The chosen limit cannot break scheduling: that LimitRange defines only
default/defaultRequest— nomax,min, ormaxLimitRequestRatio— so 256Mi/512Mi has no upper bound to violate, and 512Mi sits well above the 128Mi that OOMs (and in line with the cluster-operator's own 384Mi). Evidence: packages/apps/tenant/templates/quota.yaml:16-26. - helm-unittest passes (35/35), including the new assertion.
Non-blocking:
- The test asserts only
limits.memoryfor each operator. The fix is the wholeresourcesblock, sorequests.cpu/requests.memoryare part of the contract and currently unguarded — a later edit could drop them without failing the suite. Addequalassertions forresources.requests.cpu(100m) andresources.requests.memory(256Mi) on both operators. Evidence: packages/apps/kafka/tests/entityoperator_template_test.yaml:31-34. - The resources are hardcoded with no values knob. That matches the platform's sensible-default approach and is fine here; exposing entity-operator sizing the way
kafka/zookeeperexposeresourcesPresetwould be a reasonable follow-up if tuning is ever needed. Evidence: packages/apps/kafka/values.yaml (noentityOperatorsection).
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-1.4
git worktree add -d .worktree/backport-2934-to-release-1.4 origin/release-1.4
cd .worktree/backport-2934-to-release-1.4
git switch --create backport-2934-to-release-1.4
git cherry-pick -x 1ab5f17caca0a950596f6dfd6e64eda318739e1e |
|
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
packages/apps/kafkarendered the StrimziKafkaCR withentityOperator.topicOperator: {}/userOperator: {}and noresources. Tenant namespaces ship aLimitRangethat defaults containers tomemory: 128Mi, so the topic-operator and user-operator JVM containers (Kafka 3.9.1 + fabric8/vertx) are OOMKilled (exit 137) on startup. The<cluster>-entity-operatorDeployment never becomes Ready (CrashLoopBackOff), so:KafkaTopic/KafkaUserCRs never reconcile — topic creation effectively fails;Exceeded timeout of 300000ms while waiting for Deployment ...-entity-operator ... to be ready;KafkaTopics get stuck permanently with thestrimzi.io/topic-operatorfinalizer (no operator left to clear them), blocking namespace deletion.The Kafka broker + ZooKeeper come up fine — only the entity-operator dies. The
apps.cozystack.io/Kafkaschema exposes no field for entity-operator resources, so there is no user-side workaround.Fix
Set explicit
resources(requests256Mi/100m, limit512Mimemory) onentityOperator.topicOperatoranduserOperatorso they no longer inherit the 128Mi namespace default. Adds ahelm-unittestassertion for the rendered limits.Testing
Reproduced on a clean cluster (Cozystack v1.4.3 and v1.5.0-rc.2; Strimzi 0.45.1, Kafka 3.9.1): both entity-operator containers
OOMKilledat 128Mi → CrashLoopBackOff →KafkaTopicneverREADY. After bumping to 512Mi the entity-operator rolls cleanly (both containers Ready, 0 restarts) and theKafkaTopicreconciles toREADY=True.Release note
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests