bitflicker64 commented on code in PR #3185:
URL: https://github.com/apache/hugegraph/pull/3185#discussion_r3918294000
##########
docker/docker-compose-hstore.yml:
##########
@@ -44,7 +44,7 @@ services:
volumes:
- pd-data:/hugegraph-pd/pd_data
healthcheck:
- test: ["CMD-SHELL", "curl -fsS http://localhost:8620/v1/health
>/dev/null"]
+ test: ["CMD-SHELL", "curl -fsS http://localhost:8620/v1/ready
>/dev/null"]
Review Comment:
⚠️ This probe cannot tell a ready PD from one that has no `/v1/ready` at
all, so the gate in front of `store` still checks only liveness.
PD's auth interceptor rejects an unauthenticated request by writing a JSON
error body and returning `false` without calling `response.setStatus(...)`
(`hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/interceptor/RestAuthentication.java:61-66`),
so the response commits with the servlet default 200. On a build that lacks
the new `/v1/ready` exclusion, `curl -fsS` exits 0 on
`{"status":-1,"error":"Unauthorized!"}`.
Measured here. `docker/test-compose.sh:270` defaults `HUGEGRAPH_VERSION` to
`latest` and nothing under `.github/` sets it, so `smoke` pulls Docker Hub
`hugegraph/pd:latest`, which predates this PR (`git grep '/v1/ready'
origin/master -- hugegraph-pd` is empty). Run 33642694186, job `build-server
(rocksdb, 11)`: pd `Started` 14:39:17, `Healthy` 14:39:28. Eleven seconds,
against an image with no readiness endpoint, so `Compose smoke passed: hstore`
does not exercise this change.
Not a regression, since the pre-PR `/v1/health` probe was liveness-only too.
But as written the switch buys nothing.
Requested change: check the payload, the way
`test-start-hugegraph-pd.sh:123` already does with `[[ "$body" ==
*'"ready":true'* ]]`. For this line:
test: ["CMD-SHELL", "curl -fsS http://localhost:8620/v1/ready | grep -q
'\"ready\":true'"]
and for `docker-compose-3pd-3store-3server.yml:40`, where the trailing `||
exit 1` has to sit after the pipeline:
test: ["CMD-SHELL", "curl -fsS http://localhost:8620/v1/ready | grep -q
'\"ready\":true' || exit 1"]
Left as plain blocks rather than committable suggestions on purpose:
applying them reds `docker/test-compose.sh smoke` until an image carrying
`/v1/ready` is published. Pin the smoke run to source-built images at the same
time, or keep the compose files on `/v1/health` and land the endpoint, gauges
and docs first.
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/rest/RestApiTest.java:
##########
@@ -62,6 +62,51 @@ public void testQueryClusterInfo() throws
URISyntaxException, IOException, Inter
assert obj.getInt("status") == 0;
}
+ @Test
+ public void testHealthNeedsNoAuth() throws URISyntaxException, IOException,
+ InterruptedException {
+ String url = pdRestAddr + "/v1/health";
+ HttpRequest request = HttpRequest.newBuilder().uri(new
URI(url)).GET().build();
+ HttpResponse<String> response = client.send(request,
HttpResponse.BodyHandlers.ofString());
+ assert response.statusCode() == 200;
Review Comment:
🧹 `statusCode() == 200` cannot tell "no auth required" apart from "auth
rejected": PD's interceptor answers an unauthenticated request with `200` and
an error envelope, per the comment on `docker-compose-hstore.yml:47`. This test
would still pass if `/v1/health` were dropped from `AuthenticationConfigurer`.
`testReadyNeedsNoAuthAndReflectsRaft` is fine, because
`obj.getBoolean("ready")` would throw on that envelope.
Requested change: assert the body too. `checkHealthy()` returns `""`.
```suggestion
assert response.statusCode() == 200;
assert response.body().isEmpty() : "expected an empty body, got " +
response.body();
```
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/StoreAPI.java:
##########
@@ -386,4 +396,30 @@ class StoreStatistics {
public Serializable checkHealthy() {
return "";
}
+
+ /**
+ * Check Service Readiness
+ * Answers 200 only when this PD is part of a raft quorum, that is, the
raft node is active
+ * and knows the current leader. Otherwise answers 503 so that anything
gating on PD
+ * (the compose healthcheck in front of Stores, a Kubernetes readiness
probe)
+ * is held back until the PD can serve. Like /health this endpoint needs
no authentication.
+ *
+ * @return JSON with the readiness flag, the local raft state and the raft
address of the
+ * leader (null when there is none)
+ */
+ @GetMapping(value = "/ready", produces = MediaType.APPLICATION_JSON_VALUE)
+ public ResponseEntity<Map<String, Object>> checkReady() {
+ RaftEngine raft = RaftEngine.getInstance();
+ boolean ready = raft.isReady();
+ State state = raft.getNodeState();
+ PeerId leader = raft.getLeader();
Review Comment:
🧹 The body is built from four independent raft reads. `isReady()` reaches
`getLeaderId()` through `hasLeader(node)`, then `getLeader()` calls it again,
so a step-down between the two yields `"ready": true` with `"leader": null`,
which `api-reference.md:802` promises only for the `503` case.
Requested change: derive all four fields from a single `Node` reference, the
same idiom as the `hasLeader(Node)` overload added in 5bd1b96. A small snapshot
accessor on `RaftEngine` would do it without moving raft logic into the
controller.
##########
docker/README.md:
##########
@@ -202,6 +202,15 @@ done
curl -fsS http://localhost:8088/about
```
+PD answers two unauthenticated probe endpoints. `/v1/health` is liveness only:
+it returns `200` as soon as the REST listener is up, even when the PD has no
+raft leader. `/v1/ready` returns `200` only while the PD sees a leader and
+`503` otherwise, so the compose healthchecks gate Stores on `/v1/ready`. A
+single PD elects itself; three PDs become ready once two can talk to each
+other. `/v1/ready` first ships in 1.8.0: with an older `HUGEGRAPH_VERSION`
+the PD healthcheck never passes and the Stores never start, so pin 1.8.0
+or newer, or build the images from source with `docker-compose.dev.yml`.
Review Comment:
⚠️ This states the opposite of what happens, and my earlier comment on this
paragraph was wrong to predict the hang.
With an older `HUGEGRAPH_VERSION` the PD healthcheck passes on its first
probe and the Stores start straight away, gated on nothing, for the reason in
the comment on `docker-compose-hstore.yml:47`. That is the worse of the two
failure modes: silent, and the same "healthy without a quorum" shape #3183 is
about.
Two smaller things in the same edit:
- Lines 134 and 189 move the manual verification `curl -fsS` calls to
`/v1/ready` as well, and those succeed the same way against an older image.
- "1.8.0" is a guess. The tree builds as 1.7.0 and the next release number
is not settled, so "the next release" is safer unless you have confirmation.
Requested change: harden the healthchecks as described on the compose file,
which makes this paragraph true as written. If they stay status-only, say
instead that an older PD image answers `/v1/ready` with `200` and an auth error
body, so the gate silently does nothing.
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/metrics/PDMetrics.java:
##########
@@ -76,7 +77,28 @@ private void registerMeters() {
Gauge.builder(PREFIX + ".terms", () -> setTerms())
.description("term of partitions in PD")
.register(registry);
+ registerRaftMeters();
+ }
+ /**
+ * Raft membership gauges so operators can alert on quorum loss. They
mirror what
+ * {@code GET /v1/ready} answers: a PD that sees no leader is outside a
quorum.
+ */
+ private void registerRaftMeters() {
+ RaftEngine raft = RaftEngine.getInstance();
+ Gauge.builder(PREFIX + ".raft.leader", () -> raft.isLeader() ? 1 : 0)
+ .description("1 if this PD is the raft leader, 0 otherwise")
+ .register(registry);
+ Gauge.builder(PREFIX + ".raft.has_leader", () -> raft.hasLeader() ? 1
: 0)
+ .description("1 if this PD sees a raft leader, i.e. is part of a
quorum, 0 otherwise")
+ .register(registry);
+ Gauge.builder(PREFIX + ".raft.alive_peers", () -> {
+ int alive = raft.getAlivePeerCount();
+ return alive < 0 ? Double.NaN : alive;
+ })
+ .description("Number of raft peers, itself included, the leader
has heard from " +
+ "within the election timeout; NaN on non-leader
nodes")
Review Comment:
🧹 "within the election timeout" overstates the window. `listAlivePeers()`
delegates to `NodeImpl.getAliveNodes`, which compares against
`leaderLeaseTimeoutMs` (jraft-core 1.3.11, `NodeImpl.java:2247-2260`), and
jraft derives that as `electionTimeoutMs * leaderLeaseTimeRatio / 100` with a
default ratio of 90. `RaftEngine.java:110` does not override the ratio, so a
peer drops out of the count at 90% of the election timeout, not at it.
Small, but operators size alert windows off this string. Same wording in
`RaftEngine.getAlivePeerCount()` (lines 257-258) and
`hugegraph-pd/docs/api-reference.md:837`.
```suggestion
.description("Number of raft peers, itself included, the leader
has heard from " +
"within the leader lease timeout; NaN on
non-leader nodes")
```
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/interceptor/AuthenticationConfigurer.java:
##########
@@ -32,6 +32,7 @@ public class AuthenticationConfigurer implements
WebMvcConfigurer {
public void addInterceptors(InterceptorRegistry registry) {
registry.addInterceptor(restAuthentication)
.addPathPatterns("/**")
- .excludePathPatterns("/actuator/*", "/v1/health",
"/v1/prom/targets/*");
+ .excludePathPatterns("/actuator/*", "/v1/health", "/v1/ready",
+ "/v1/prom/targets/*");
Review Comment:
🧹 `/v1/ready` joins `/v1/health` as an anonymous path, but it answers with
more than a status: the local `state` and the leader's raft address
(`api-reference.md:795`). Everything else under `/v1` is auth-gated, and
`/v1/health` returns an empty string.
Minor rather than important, because `/actuator/*` is already anonymous and
`Authentication`'s own class javadoc says PD must not be reachable outside a
trusted network. Still, a probe only needs the status code, and this is the one
new disclosure in the PR.
Requested change: consider trimming the anonymous response to `ready`, and
leaving `state`, `leader` and `isLeader` on an authenticated path for operators.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
