Repository navigation
Conversation
Runner and forwarder probe blocks are rendered only when set, so the default install is byte-identical to before. Liveness is documented on /metrics rather than /healthz. /healthz reflects sink health, so a transient sink outage on a liveness probe restarts the runner while nothing is wrong with it - the reason the 2023 probes were reverted in 2b2292c. /healthz is documented as a readiness option instead. Closes robusta-dev#1158
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe Helm chart adds optional startup, liveness, and readiness probe settings for the runner and forwarder. The templates declare named container ports and render configured probes. Setup documentation describes endpoints, configuration examples, and endpoint selection. ChangesHealth probe configuration
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description check |  |
The description clearly explains the configurable probes, named ports, documentation, rationale, upgrade behavior, and testing for this changeset. |
| Title check | |
The title clearly summarizes the main Helm changes: configurable health probes and named container ports. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (4 skipped: 4 unsupported.)
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
Comment @coderabbitai help to get the list of available commands.
Declares http/5000 on the runner and metrics/2112 on the forwarder so probes can reference the port by name, and documents why that is preferable. Service targetPort stays numeric on purpose. A named targetPort resolves per pod, so upgrading from a chart version whose pods had no named port leaves the old pod Ready with its endpoint published with no port at all - verified on kind, the EndpointSlice comes back with ports: null - and the Service black-holes traffic until the rollout completes.
DrFaust92
changed the title
What
Adds
livenessProbe,readinessProbeandstartupProbevalues for the runner and the forwarder, rendered only when set, and names the container ports so probes can reference them by name (http/5000 on the runner,metrics/2112 on the forwarder).New docs page at
docs/setup-robusta/health-probes.rstcovering which endpoints each component actually serves, which to point probes at, and why to reference ports by name.The only change to a default render is the two
ports:blocks, so existing installs get one pod restart on upgrade and no other behavior change.Why
Follows the endpoint/history writeup and proposal in #1158, which @adamantal 👍'd.
/healthzreflects sink health, so a liveness probe on it restarts the runner whenever a sink flaps — nothing is wrong with the runner. That is what the 2023 probes did (add liveness probe to the runner (check discovery health) #825, add startup probe for long boot time #845) before2b2292c5reverted them, and chore(helm): define pod probes #1960 stalled after re-adding pod probes. Liveness on/metricsrestarts only a genuinely wedged process;/healthzis documented as a readiness option for anyone who wants the sink-aware signal./healthz— only/metricson:2112— so that is what the docs point at.port: httpinstead of repeating5000, survives a listen-port change, and resolves in NetworkPolicy and Prometheus Operator configs too. A name that no container declares also fails with an explicitport "..." not foundrather than a generic connection refused.Service
targetPortdeliberately stays numeric. A namedtargetPortresolves per pod, so upgrading from a chart version whose pods had no named port leaves the old podReadywith its endpoint published with no port at all — I checked on kind and the EndpointSlice comes backports: null— and the Service black-holes traffic until the rollout finishes. Numeric resolves either way. It can move to the name in a later version once no supported upgrade path starts from unnamed pods.Note on the original report:
automountServiceAccountToken: falseno longer produces a running-but-unready pod. On 0.49.0 the runner exits on its first API call and the forwarder exitsfatalon the missing kubeconfig, so both land inCrashLoopBackOff— Kubernetes already reports that. Probes here are for wedged processes, slow starts, and rollout/Service gating; the docs say so rather than implying they detect misconfiguration.Testing
Installed on kind twice — once with numeric probe ports, once with all probes referencing the names (
port: http,port: metrics). Both pods reached1/1 Readywith 0 restarts, and EndpointSlices kept real ports. Confirmedhelm templateis byte-identical to master apart from the twoports:blocks, and rendered clean withrunner.hardenedFs=trueandgrafanaRenderer.enableContainer=true.