strelok89 opened a new pull request, #3832:
URL: https://github.com/apache/celeborn/pull/3832
### What changes were proposed in this pull request?
Add a `GET /health` endpoint served by both master and worker, whose HTTP
status code reflects whether the service is able to serve: `200` when healthy
and `503` when not.
- `HttpService#healthCheck` defaults to a shallow check that reports healthy
once the HTTP service is available. This is what the master uses.
- `Worker` overrides it to report healthy only when the worker is registered
with the master and its state is `Normal`.
- `/health` is added to the default HTTP authentication bypass paths.
The endpoint is mounted at the root rather than under `/api/v1` so that a
probe is not coupled to the API version, consistent with the other operational
paths (`/ping`, `/metrics/prometheus`). The checked-in OpenAPI spec covers only
`/api/v1`, so the generated client is unaffected.
Example:
```
$ curl -s -o /dev/null -w '%{http_code}\n' http://worker:9096/health
200
$ curl -s http://worker:9096/health
{"service":"worker","healthy":false,"reason":"worker is not registered with
master"}
```
### Why are the changes needed?
Celeborn currently exposes no endpoint whose status code reflects whether
the process can serve, so a meaningful Kubernetes probe cannot be written.
`/ping` always returns `200` regardless of state, and `/api/v1/workers` returns
`200` even when `isRegistered` is false, so operators have to fall back to an
`exec` probe that shells out and greps the JSON body.
The practical cost is rolling updates. Without a readiness signal, 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 this can remove a meaningful
fraction of workers from service before any of them rejoin.
Two design points worth calling out for review:
**The worker check mirrors the master's own definition.**
`AbstractMetaManager#isWorkerAvailable` accepts only `Normal`, so a worker that
is idle, decommissioning or exiting is already excluded from `availableWorkers`
and never selected in `offerSlots`. Reporting such a worker as healthy would
put `/health` at odds with the master. Whether `Idle` specifically should fail
readiness is discussed on CELEBORN-2447 — it is reachable only through an
explicit `DecommissionThenIdle` event, so the alternative (200 for `Idle`, 503
only for the decommission/exit states) is a reasonable variation if reviewers
prefer it.
**The master check is deliberately shallow.** Masters are fronted by a
headless Service that does not set `publishNotReadyAddresses`. A quorum-aware
or leader-aware check would report every master as unhealthy during a cold
start, withholding their DNS records and preventing them from discovering each
other to form quorum. A follower is also a healthy replica. Quorum and
leadership remain observable through `/api/v1/ratis` and `/api/v1/masters`.
`/health` bypasses authentication by default because a kubelet cannot
present credentials, and `celeborn.http.auth.bypass.api.paths` defaults to
empty (CELEBORN-2278).
Wiring probes into the Helm chart, which currently defines no
`livenessProbe`, `readinessProbe` or `startupProbe` for either role, is
intended as a follow-up so the two changes can be reviewed independently.
### Does this PR resolve a correctness bug?
- [ ] Yes
### Does this PR introduce _any_ user-facing change?
- [x] Yes
A new `/health` endpoint on master and worker, documented in
`docs/restapi.md`.
### How was this patch tested?
New tests:
- `ApiBaseResourceSuite`: `/health` returns `200` and reports the correct
service name, for both master and worker.
- `ApiBaseResourceAuthenticationSuite`: `/health` is reachable without
credentials.
- `ApiWorkerResourceSuite`: `/health` returns `503` when the worker is not
registered.
Verified locally on JDK 11: `celeborn-service/Test/compile` and
`celeborn-worker/Test/compile` pass, `ApiMasterResourceSuite` (19/19) and
`ApiMasterResourceAuthenticationSuite` (9/9) pass, and `spotless:check` passes
for the `service` and `worker` modules.
The worker-side suites were not run locally: they were developed on Windows,
where any `MiniClusterFeature` test fails during setup because
`CelebornConf#workerBaseDirs` splits each storage dir on `:` and rejects
`C:\...` paths. That is pre-existing and unrelated to this change, and those
suites are covered by CI.
--
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]
