strelok89 opened a new pull request, #3854: URL: https://github.com/apache/celeborn/pull/3854
### What changes were proposed in this pull request? Follow-up to [CELEBORN-2447](https://issues.apache.org/jira/browse/CELEBORN-2447) / #3832, which added `GET /healthz` to master and worker. The chart defined no `startupProbe`, `livenessProbe` or `readinessProbe` for either role, so nothing consumed the endpoint. This adds values-driven probes for both roles. Each value is a complete Kubernetes probe object, so it can be tuned freely or set to `null` to drop the probe: | | master | worker | |---|---|---| | `startupProbe` | `/healthz`, 30 x 10s | disabled | | `livenessProbe` | `/healthz`, 6 x 10s | disabled | | `readinessProbe` | disabled | `/healthz`, 3 x 10s | The defaults are asymmetric because the two checks mean different things: - **Worker readiness is on.** This is the reason the endpoint exists: it reports healthy only once the worker has finished initializing and registered with the master, which is the gate a rolling update needs. - **Worker liveness is off.** Worker `/healthz` returns `503` while the worker is unregistered, so a liveness probe on it would restart every worker in the cluster during a master outage or failover, discarding in-flight shuffle data. The README points at a `tcpSocket` check on the RPC port for deployments that want one. - **Master startup and liveness are on.** The master check is shallow, so it is safe to restart on. The chart's master container blocks on `nslookup` of every master peer before invoking `start-master.sh`, so the HTTP port is not open during that wait; the startup probe covers that window and keeps liveness from firing when a replica is slow to be scheduled. - **Master readiness is off** — see below. Also adds `master.podManagementPolicy` and `worker.podManagementPolicy`, both unset by default, and sets `publishNotReadyAddresses: true` on both headless services. ### Why are the changes needed? Without a readiness probe a StatefulSet marks a worker pod Ready as soon as its container process starts and immediately proceeds to the next ordinal, before the restarted worker has re-registered with the master. On a large cluster a routine rolling update can remove a meaningful fraction of workers from service before any of them rejoin. Two design points worth calling out for review: **Master readiness deadlocks a cold start under `OrderedReady`.** A master does not start serving until the DNS records of all master replicas resolve, and with the default pod management policy the controller does not create `master-1` until `master-0` is Ready — so a ready-gated `master-0` waits for a pod that will never be created. Enabling `master.readinessProbe` therefore requires `master.podManagementPolicy: Parallel`. The policy defaults are left unset rather than changed to `Parallel`, because the field is immutable and changing it would break `helm upgrade` on every existing release. **`publishNotReadyAddresses` on the master service is required, not cosmetic.** A headless service publishes a pod's per-pod DNS record only once that pod is ready, and both the startup wait and `celeborn.master.endpoints` address masters by those per-pod names — so a ready-gated master would withhold the very record its peers are waiting for. The worker service sets it to keep DNS and endpoint behaviour as it was before readiness was gated. One operational consequence is documented rather than worked around, matching what was agreed on CELEBORN-2447: a worker parked with `DecommissionThenIdle` reports `Idle`, which `/healthz` treats as not serving because the master does not schedule to it either, so a rolling update stalls at that ordinal until the worker is recommissioned or `worker.readinessProbe` is unset for that rollout. ### Does this PR resolve a correctness bug? - [ ] Yes ### Does this PR introduce _any_ user-facing change? - [x] Yes Master and worker pods now carry probes by default, and both headless services publish not-ready addresses. New values: `master.startupProbe`, `master.livenessProbe`, `master.readinessProbe`, `master.podManagementPolicy` and the matching `worker.*`. Documented in `charts/celeborn/README.md`. ### How was this patch tested? New `helm unittest` cases for both roles covering the defaults, an override, probe removal via `null`, and the pod management policy; plus the `publishNotReadyAddresses` assertion on both services. `helm unittest charts/celeborn --file "tests/**/*_test.yaml" --strict` passes (92 tests, 12 suites) and `helm lint charts/celeborn` is clean. Rendered output inspected with `helm template`. Checked against the CI values used by the minikube job: `charts/celeborn/ci/values.yaml` sets `celeborn.metrics.enabled: false`, and the Jetty HTTP server starts independently of the metrics system (`Service.scala` gates only `metricsSystem.start()`), so `/healthz` is still served there. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
