Add default readiness/liveness probes to the RabbitMQ StatefulSet - #53
Open
henzigo wants to merge 1 commit into
Open
Add default readiness/liveness probes to the RabbitMQ StatefulSet#53henzigo wants to merge 1 commit into
henzigo wants to merge 1 commit into
Conversation
The StatefulSet rendered probes only from values (`rabbitmq.livenessProbe` /
`readinessProbe`, both `{}` by default), so no probes reached the cluster: the
broker pod counted as Ready the moment the container process started, not when
it accepted AMQP connections. The whole hook ordering rests on the premise that
the infra release installed with `wait: true` is really ready before the app
release's migration hook runs — for RabbitMQ that premise was hollow.
Defaults follow the official RabbitMQ guidance
(https://www.rabbitmq.com/docs/monitoring#health-checks):
- readiness: TCP check on the AMQP port 5672. The AMQP listener opens as one of
the last boot steps, so this is exactly "the broker accepts connections"; it
is the upstream Kubernetes Operator's default and the docs explicitly call
CLI-based checks unsuitable as readiness probes.
- liveness: the stage-1 `rabbitmq-diagnostics -q ping` with a generous budget
(60 s initial delay, 3 x 30 s, 15 s timeout) because it spawns an Erlang VM
and a false positive costs a full broker restart. Heavier checks
(check_running, check_local_alarms) are deliberately not used for liveness.
Probes are rendered with the redis `if/else` pattern instead of the
`shopsys.containerSettings` helper so that setting a probe in values replaces
the default wholesale — map deep-merge would otherwise make defaults
impossible to remove. The container `securityContext` is rendered inline for
the same reason (it was the helper's only other output here).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #6
What
Gives the RabbitMQ StatefulSet default readiness/liveness probes (the probes bullet of #6).
The other tasks of #6 (port names/5672 declaration,
startupProbe, preStoprabbitmqctl stop_app+ 90 s grace, memory request/limit defaults, explicitupdateStrategy) are not in this PR — henceRefs, notCloses.Why
statefulset-rabbitmq.yamlrendered probes only from values viashopsys.containerSettings,and
charts/shopsys-infra/values.yamldefaults them tolivenessProbe: {}/readinessProbe: {}— so no probes reached the cluster. The broker pod counted as Readythe moment the container process started, not when it accepted AMQP connections on 5672.
The hook ordering rests on the premise that the infra release (installed with
wait: true)is really ready before the app release's migration hook runs — see the comment in
helmfile.yaml.gotmpland the #44 discussion referenced from #6. For RabbitMQ that premisewas hollow; with a readinessProbe it holds. Redis in the same chart already had template-side
default probes.
Chosen defaults and sources
checks states verbatim: "RabbitMQ
Kubernetes Operator configures a TCP port check on the AMQP port as the
readinessProbeanddefines no
livenessProbeat all. This should be considered the best practice." The samepage explains CLI-based checks are unsuitable as readiness probes (performance overhead,
they assume a fully booted node). The AMQP listener opens as one of the last boot steps, so
the TCP check is precisely the signal we need — and it is cheap. The timings match the
Cluster Operator's defaults (10/10/5, failureThreshold 3),
cf. cluster-operator#413.
rabbitmq-diagnostics -q ping. Stage 1 in the docs' health-checkladder: validates the runtime is operating and CLI authentication works, with "nearly zero
false positives outside maintenance windows". It spawns an Erlang VM, so it is not cheap and
the budget is deliberately generous (60 s initial delay + 3 × 30 s ⇒ ~150 s before a restart,
15 s timeout because a loaded node needs it). Heavier stage-3+ checks (
check_running,check_local_alarms) are intentionally not used for liveness — an alarmed-but-alivebroker must not be killed. Upstream even recommends no liveness probe at all; a stage-1
ping is the conservative middle ground and matches what
shopsys/deployment#75 proposed.
Divergence from #6 (please review)
#6 (and deployment#75) specifies readiness as
rabbitmq-diagnostics -q check_running. I wentwith the TCP check on 5672 instead, because the official docs explicitly discourage CLI checks
as readiness probes and name the TCP-on-AMQP check the best practice (quote above). It is also
strictly cheaper and a later boot signal than
check_running. Liveness follows #6/#75 withthe softened budget from the issue comment (
failureThreshold: 3instead of 5), keepingtimeoutSeconds: 15— a too-short timeout is itself a false-positive source for a check thatboots an Erlang VM.
No
startupProbehere: it is a separate #6 task item, and without it the liveness probe's60 s initial delay + 90 s failure budget already covers a slow boot. Adding a
startupProbekey would also introduce a rabbitmq-only value key, inconsistent with the standard component
keys — better done together with the rest of #6.
Changes
charts/shopsys-infra/templates/statefulset-rabbitmq.yaml: probes rendered with theexisting redis
if/elsepattern (deployment-redis.yaml) rather thanshopsys.containerSettings, so a probe set in values replaces the default wholesale —map deep-merge would otherwise make the defaults impossible to remove. The container
securityContext(the helper's only other output here) is rendered inline; verified thereis exactly one
livenessProbe/readinessProbeblock in the rendered output, no duplicatevia the helper. Rationale lives in Go-template comments, so nothing leaks into the manifest.
charts/shopsys-infra/values.yaml:rabbitmq.livenessProbe/readinessProbestay{}andare now documented (empty = chart defaults, set = wholesale replace). No new keys ⇒ no
values.schema.jsonchange.probes fully;
securityContextstill renders next to the probes) — 62 tests pass.docs/migrating-from-shopsys-deployment.md; the standardcomponent keys table in
docs/values.mdnow says probes replace wholesale.line the empty
containerSettingsinclude used to leave behind).Notes
helm unittest62 passed · golden 15/15 ·helm lintboth charts clean · kubeconform531/531 valid (k8s 1.31.0) · shellcheck clean.
drop-sleep-and-bound-hooksrewords deviation 9 with"RabbitMQ has no default readinessProbe, so the wait guarantees a Running broker container,
not yet an accepting broker". Once both land, that clause is stale and should be dropped
(the [infra] RabbitMQ hardening: ports, probes, graceful restart #6 task item about entry 9) — whichever merges second should fix it. Golden
expected/*.yamlconflicts with that PR are expected and are pure snapshot regenerations.🤖 Generated with Claude Code