fix(rabbitmq): right-size the default resources_preset to s1.nano - #34
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
a96c2f4 to
ec650c9
Compare
europrinter (yankawai)
changed the title
|
Updated to track cozystack/cozystack#3936, where review asked to either report the peak memory that motivates a 1Gi default or right-size it. The observed working set peaked at 125.8Mi against the 128Mi Schema default, model tests, schema test, acceptance test and CHANGELOG all carry |
scooby87
left a comment
scooby87
left a comment
There was a problem hiding this comment.
NOT LGTM — one process blocker (independent cozy-review pass). The code itself is correct, covered and mutation-verified; only the sign-off is missing.
[MAJOR / blocker] DCO. The HEAD commit b3d1148 ("fix(rabbitmq): right-size...") has no Signed-off-by line, while the two earlier commits do, so the required DCO check is red. Fix is trivial: git commit --amend -s (or git rebase --signoff) on that commit + force-push. This is the only reason for NOT LGTM.
Verified and clean: the default target s1.nano matches the chart, s1.nano is already a valid enum in the released API v1.6.1, the generated docs need no regeneration (preset value not embedded, drift 0), the existing-state impact is intentionally documented in the CHANGELOG with a resources_preset = "t1.nano" escape hatch, and the new schema test is mutation-verified (reverting to t1.nano turns it RED).
[MINOR, non-blocking] The paired chart PR cozystack#3936 (which raises the chart default) is still open — runtime does not break (the provider always sends a resolved value), but it is worth tracking the two together.
b3d1148 to
ea7a429
Compare
|
Signed off in ea7a429 with the same tree, and the description now says |
ea7a429 to
2c39493
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM. The changelog entry lands in a version that already shipped, and the provider default now disagrees with the platform while the chart change is still open.
The entry belongs in the unreleased section. It sits under v1.6.1, and that tag is on the merge base, so the release went out with the old preset. git show v1.6.1:internal/provider/rabbitmq_schema.go still reads presetAttribute("t1.nano"). Anyone checking whether the upgrade from v1.6.1 rolls their RabbitMQ pods now reads that the move already happened, and that is the one upgrade where it does. There is a v1.6.2 section at the top already, it only needs a Breaking changes heading.
The other one is merge order. Every other kind keeps its schema default equal to the +kubebuilder:default of the pinned API module, and RabbitMQ is now the only one that does not. The module still carries t1.nano. So does values.yaml on cozystack main, and the chart PR that changes it is still open. That PR has moved its own value once already, from u1.nano. Merging this first means a RabbitMQ created through Terraform is sized differently from one created any other way, and another move upstream would ship a wrong default in v1.6.2. I would wait for cozystack/cozystack#3936, or say in the description why the provider goes first.
Tests are fine. The schema default, the round trip, the explicit t1.nano override and the acceptance check cover both halves of the contract.
Three smaller things:
- The description still lists the exhaustruct_v5 bullet. That fix reached master on its own and the commit went away in the rebase.
2a28976sets u1.nano and the commit after it overrides that. Master takes merge commits, so it would land as history setting a value nobody wants. Squashing the two drops it.- redis, vpn, qdrant and openbao declare
resources_presetinline instead of callingpresetAttribute(), and their descriptions have drifted apart. Pre-existing, but a grep on the helper misses them next time a default changes.
Aleksei Sviridkin (lexfrei)
added
the
target/1.7
) ## What this PR does Raises the RabbitMQ default resource preset from `t1.nano` (128Mi memory) to `s1.nano` (250m CPU / 512Mi memory). A default-sized RabbitMQ 4.2 broker on Cozystack 1.6.1 was OOMKilled with exit 137, reached 10 restarts, and never became Ready. A canary on a larger preset reached 3/3 Ready with zero restarts; its working set peaked at 125.8Mi. Against the 128Mi `t1.nano` ceiling that is 98% of the limit in steady state, and cgroup accounting (working set plus page cache) crosses it under load. The canary and its storage were removed after validation. ### Sizing `s1.nano` keeps the CPU of `t1.nano` (250m) and quadruples memory to 512Mi — a 4x margin over the observed 125.8Mi peak. An earlier revision of this PR proposed `u1.nano` (1Gi). Review correctly objected that 1Gi reserves 8x the observed working set with no measured peak to justify it, and asked to either report that peak or right-size the default. The peak is reported above, and the default is right-sized to the smallest preset that clears the ceiling with a real margin rather than to the first one that is comfortably large. ### Upgrade impact cozystack-api applies OpenAPI schema defaults on the read path and stores what the client sent: `Get` runs `applySpecDefaults` on its way out (`pkg/registry/apps/application/rest.go:1232`) and the write puts `app.Spec` into `HelmRelease.spec.values` (`rest.go:1652`). That splits existing brokers in two. **Brokers with no stored `resourcesPreset`** pick up the new chart default on the next Helm reconcile. For them this upgrade is: - a rolling restart of every replica, and - a rise in guaranteed memory reservation from 128Mi to 512Mi per replica, because the sanitize helper sets memory request equal to limit — roughly 1.1Gi extra reserved per 3-replica instance. **Brokers whose values went through a read-modify-write keep `t1.nano` and are not resized.** `Update` opens with `r.Get` (`rest.go:489`), so a GET-then-PUT — `kubectl edit` is the unambiguous case, and any client that reads before writing behaves the same — freezes the schema default of the moment into the stored values, and a stored value beats the chart default. Those brokers are not restarted, not resized, and keep hitting the ceiling this PR raises. Nothing downstream can separate a deliberate `t1.nano` from one a round trip baked in, so this is documented rather than migrated. After the upgrade the affected brokers are the ones that still report the old preset: ``` kubectl get rabbitmq -A -o custom-columns=NS:.metadata.namespace,NAME:.metadata.name,PRESET:.spec.resourcesPreset ``` The same command before the upgrade tells you nothing: the cozyrd default is still `t1.nano` then, so the read path fills it in for both groups. Afterwards, anything reading `t1.nano` carries a stored value; setting `resourcesPreset: s1.nano` explicitly on the ones that should move is the fix. PVCs are untouched, so there is no data loss, but node headroom should be checked before upgrading. Also worth knowing when reading the parameter table: the preset does not only apply to a fully omitted `resources`. `cozy-lib.resources.defaultingSanitize` merges it key by key, so a broker pinned to `resources: {cpu: 700m}` keeps its CPU and moves from 128Mi to 512Mi with everyone else. The field descriptions now say so, and a test pins it. The values, generated schema, ApplicationDefinition, typed API, and README all carry the same default. Tests cover the new default, an explicit legacy override, explicit resources, and a partially set `resources` map inheriting the preset for what it leaves out. Validation: - `helm template` with the chart's `cozy-lib` dependency renders `limits 250m / 512Mi` and `requests 0.025 / 536870912` for the default; an explicit `t1.nano` renders `128Mi / 134217728`; explicit `700m / 2Gi` renders `0.07 / 2147483648`. The unit test asserts exactly these values. - Provider counterpart: cozystack/terraform-provider-cozystack#34. ### Screenshots Not a UI change. ### Downstream repositories The default is restated by the Terraform provider, so the linked provider PR updates it and documents the state-plan implication. No other trigger-map entry matches this diff. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: cozystack/terraform-provider-cozystack#34 - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: ### Release note ```release-note fix(rabbitmq): raise the default resource preset from t1.nano to s1.nano so default-sized brokers are no longer OOMKilled. Brokers with no stored `resourcesPreset` are restarted on upgrade and their guaranteed memory reservation grows from 128Mi to 512Mi per replica; check node headroom before upgrading. Brokers whose values already pin `resourcesPreset`, including ones where a read-modify-write through the API froze the old default into them, keep their old size: list them after the upgrade with `kubectl get rabbitmq -A -o custom-columns=NAME:.metadata.name,PRESET:.spec.resourcesPreset` and set `resourcesPreset: s1.nano` explicitly on the ones that should move. The preset also fills any resource a partially set `resources` leaves out, so a broker pinned to cpu alone moves too. A tenant that opted into `resourceQuotas` and sits near its memory ceiling has the extra reservation rejected by quota admission, which shows up as a StatefulSet that never scales rather than as pods left Pending. ```
2c39493 to
37163d7
Compare
|
Moved the entry to v1.6.2 under Breaking changes, dropped the exhaustruct_v5 bullet, squashed the u1.nano commit and rebased on master. cozystack/cozystack#3936 is merged now, so the description says the default should ship with a pin that includes it. |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
europrinter (@yankawai) NOT LGTM. The code is right, but the changelog entry landed in a release that already shipped, and the commit message retells another PR's review.
An existing user sees an in-place update on the first plan after the upgrade. resources_preset is Optional and Computed with a static default and no plan modifier, so a cozystack_rabbitmq that never set it plans t1.nano → s1.nano and the pods roll on apply. Nothing in the RabbitMQ schema requires replacement for this attribute, so there is no replace. I read this from the schema flags in presetAttribute; the repo has no plan-level test against a mocked API to run it. The changelog text describes exactly this, and the t1.nano escape hatch is correct.
Business context: keep the provider's RabbitMQ default equal to the chart default that cozystack/cozystack#3936 moved to s1.nano, so brokers created through Terraform are not sized at the 128Mi that got them OOMKilled.
The first blocker is the changelog. After the rebase the new Breaking changes heading sits under ## v1.6.3. That tag is the merge base of this PR and was released on 2026-09-21. git show v1.6.3:CHANGELOG.md has the same section without this entry, and v1.6.3:internal/provider/rabbitmq_schema.go still reads presetAttribute("t1.nano"). So a user upgrading to v1.6.3 would read that their brokers get resized, and they don't. The section header also says it tracks the API at v1.6.3, and the pinned api/apps/v1alpha1 v1.6.3 still has +kubebuilder:default:="t1.nano". No API module tag contains #3936 yet. Its release-1.6 backports are still open. The entry needs a new unreleased section above v1.6.3, the one that ships with a pin containing #3936. The description is stale too: it says the pin is v1.6.1 and the entry sits under an unreleased v1.6.2.
The second one is the commit message. It opens with "Follows cozystack/cozystack#3936, where review asked to either report the peak memory that motivates a 1Gi default or right-size it." This repository never had a 1Gi default, since the u1.nano commit was squashed out, and "review asked" is the history of another PR, not the reason for this one. The rest of the body is the actual reason (the 125.8Mi peak and the watermark math) and can stay. Just "Follows cozystack/cozystack#3936." as the first sentence is enough.
Everything else from my previous review is addressed. One non-blocking thing: the explicit t1.nano test covers only expand. If I replace the flatten at rabbitmq_model.go:87 with a hardcoded types.StringValue("s1.nano"), go test ./internal/provider/ stays green, because the round-trip test feeds s1.nano. That flatten is what keeps an explicit t1.nano from showing a diff after every refresh, so a flatten case with t1.nano in the spec would pin it.
…s preset to s1.nano (#4393) ## What this PR does Manual backport of #3936 to `release-1.6`. The automated backport, #4372, pushed conflict markers in `packages/apps/rabbitmq/README.md` and `packages/system/rabbitmq-rd/cozyrds/rabbitmq.yaml` as its only commit, so it would ship them if merged. Both are generated files, and the conflict is the `tls` block that landed on main after 1.6 was cut. Everything else applies unchanged, so two differences follow: - the README parameter table and the cozyrd schema are regenerated from the 1.6 values with cozyvalues-gen v1.6.0, so they carry the new default and descriptions without `tls`; - the 1.6 rabbitmq `Makefile` has no `test` target, so the helm-unittest sweep would never enter the package and the new preset suite would not run. The second commit adds the same two lines main already has. Verified on this branch: the rabbitmq suite passes 4/4, `make -n test` answers the sweep probe, and a second `make generate` produces no drift. Change summary, unchanged from #3936: the default `resourcesPreset` moves from `t1.nano` (128Mi) to `s1.nano` (250m CPU, 512Mi), because a default-sized RabbitMQ 4.2 broker was OOMKilled on `t1.nano` and never became Ready. The `resources` and `resourcesPreset` descriptions say the preset fills every resource `resources` leaves unset. Close #4372 in favour of this one, or tell me to take a different route and I will. ### Screenshots Not a UI change. ### Downstream repositories Walked the trigger map against the diff: one app chart with a changed default. The provider follow-up is cozystack/terraform-provider-cozystack#34, labelled `target/1.7`. - [ ] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [x] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: cozystack/terraform-provider-cozystack#34 - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(rabbitmq): raise the default resources preset from t1.nano to s1.nano so a default-sized broker is not OOMKilled. Existing brokers that never set resourcesPreset move to s1.nano on upgrade and roll their pods. ```
Follows cozystack/cozystack#3936. The observed working set peaked at 125.8Mi. With the operator's 20% headroom and the broker's 0.6 memory watermark, s1.nano (250m CPU / 512Mi) blocks publishers at about 246Mi, roughly twice that peak, while t1.nano stops at about 61Mi, below it. u1.nano would reserve twice the memory again, and s1.nano leaves the CPU reservation unchanged. Signed-off-by: Yan Bondarenko <202671653+yankawai@users.noreply.github.com>
37163d7 to
8aea80b
Compare
|
Moved the entry to Unreleased above v1.6.3 and corrected the API pin and release references in the PR description. The commit message now explains the sizing reason. Added a flatten case with explicit |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
europrinter (@yankawai) LGTM, the changelog and the commit message are fine now.
s1.nano is in the chart's resourcesPreset enum already at v1.6.3, so the provider can send it with the current API pin. I ran go test ./internal/provider/ and it passes. Putting the schema default back to t1.nano breaks only TestRabbitmqSchemaResourcesPresetDefault, and hardcoding the flatten to s1.nano breaks TestRabbitmqFlatten_ExplicitLegacyPreset, so both paths are pinned.
Not blocking: the description ticks "Generated resource documentation reflects s1.nano" and "Registry resource documentation updated", but the diff doesn't touch docs/. docs/resources/rabbitmq.md shows no default preset at all, because the attribute description has no value in it. I'd untick those two.
cafd43e
into
cozystack:master
3 participants
Pull Request
Summary
Aligns the RabbitMQ
resources_presetdefault with cozystack/cozystack#3936:s1.nano(512Mi) replacest1.nano(128Mi), which could OOM before the broker became ready.Changes
s1.nanoand preserve explicitt1.nanothrough expand and flatten.resources_preset = "t1.nano".Testing
go test ./internal/providerpasses.s1.nano.Documentation
Checklist
Additional Notes
The chart change is merged into
mainandrelease-1.6(cozystack/cozystack#4393). The provider still pinsapi/apps/v1alpha1v1.6.3, which predates that change; release this with an API pin that includes the new chart default.