fix(pd): report fatal raft errors through health probes

docker/README.md

@@ -172,6 +172,20 @@ curl -fsS http://localhost:8088/about
 
 </details>
 
+### PD health and readiness
+
+PD's `/v1/ready` returns HTTP 503 while it cannot serve through a Raft quorum.
+`/v1/health` remains HTTP 200 during an election or temporary quorum loss, but
+returns HTTP 503 after a fatal Raft error or state-machine shutdown. Both health
+responses retain the empty text body. Probes read cached state without waiting
+for the Raft node lock.
+
+A failed snapshot, for example after the PD disk fills, leaves JRaft in a terminal
+error state. Free disk space before restarting the affected PD; freeing space
+alone does not reinitialize its Raft node. Configure liveness against `/v1/health`
+with an image containing this fix and gate traffic on `/v1/ready`. Older images
+still need the single-PD readiness-based liveness workaround supplied by the Helm chart.
+
 ### Stop or remove a deployment
 
 `stop` keeps containers and data; `down` removes containers and the network but keeps data. **`down -v` also deletes all topology data.**

helm/hugegraph/values.yaml

@@ -177,6 +177,8 @@ pd:
   # election to lose and would otherwise pass /v1/health forever after losing
   # leadership for good (apache/hugegraph#3222). Startup follows liveness so
   # the boot-to-ready window sits inside the startup budget.
+  # TODO: drop the single-PD readiness derivation once the minimum supported image
+  # includes the /v1/health fatal Raft error fix for apache/hugegraph#3222.
   livenessPath: ""
   probes:
     startup:

hugegraph-pd/docs/api-reference.md

@@ -809,7 +809,7 @@ Two unauthenticated endpoints are meant for probes and startup gates:
 
 | Endpoint | Meaning | Status |
 |----------|---------|--------|
-| `GET /v1/health` | Liveness: the REST listener is up. Does not consult raft. | always `200` |
+| `GET /v1/health` | Liveness: the REST listener is up and Raft has no terminal error. Empty text body. | `503` for `STATE_ERROR` or `STATE_SHUTDOWN`, `200` otherwise |
 | `GET /v1/ready` | Readiness: the raft node is active and sees a leader, so this PD is inside a quorum. | `200` when ready, `503` otherwise |
 
 ```bash
@@ -829,6 +829,11 @@ A follower reports `"state": "STATE_FOLLOWER"` with `"isLeader": false`. When th
 
 The answer is served from state the raft callbacks maintain rather than from the raft node, so it stays prompt while an election is running and never waits on the node lock. `state` is therefore the last change raft announced. A PD reports `STATE_UNINITIALIZED` with `"ready": false` from process start until its first raft callback, which is the ordinary startup window before a quorum first forms, and jraft emits no callback for candidacy or leadership transfer, so a candidate reports `STATE_FOLLOWER` with `"ready": false`.
 
+After a fatal snapshot error, `/v1/ready` retains `STATE_ERROR` even after the leader
+steps down, and `/v1/health` returns `503`. Fix the underlying disk or storage
+problem and restart the affected PD: freeing disk space alone does not restart
+JRaft's terminal state machine.
+
 Point Kubernetes readiness probes, `depends_on` healthchecks and any "wait for PD" script at `/v1/ready`; keep liveness probes on `/v1/health` so a PD that merely lost its leader is not restarted.
 
 Match on the body rather than on the status code alone. A PD that predates this endpoint does not reliably answer `404` for it: `RestAuthentication` refuses a request it does not exclude by writing an error envelope, and as of 1.7.0 it does so without setting a status, so an unknown path answers `200` with `{"status":-1,"error":"Unauthorized!"}`. A status-only probe therefore reads such a PD as ready. The body match holds whichever status a refusal carries: a shell gate should use `curl -fsS http://<pd-host>:8620/v1/ready | grep -q '"ready":true'`, and a Kubernetes `httpGet` probe should be paired with a PD image that carries the endpoint.

hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftStateMachine.java

@@ -214,13 +214,10 @@ public void onLeaderStart(final long term) {
     @Override
     public void onLeaderStop(final Status status) {
         this.leaderTerm.set(-1);
-        // TODO: keep STATE_ERROR set by onError instead of overwriting it with STATE_FOLLOWER, so
-        // the probe view, and with it /v1/ready, reports a PD that stepped down for good after a
-        // snapshot failure. /v1/health does not read this view; making it report the state is the
-        // separate TODO at StoreAPI.checkHealthy. The Helm chart (helm/hugegraph) works around both
-        // by deriving a single PD's startup and liveness probes to /v1/ready.
-        // https://github.com/apache/hugegraph/issues/3222
-        this.probeView = new ProbeView(State.STATE_FOLLOWER, false);
+        // JRaft steps down after reporting a fatal error. That callback does not recover the node.
+        if (this.probeView.state != State.STATE_ERROR) {
+            this.probeView = new ProbeView(State.STATE_FOLLOWER, false);
+        }
         super.onLeaderStop(status);
         log.info("Raft  lost leader ");
     }

hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/StoreAPI.java

@@ -17,7 +17,6 @@
 
 package org.apache.hugegraph.pd.rest;
 
-import java.io.Serializable;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.HashMap;
@@ -49,6 +48,7 @@
 import org.springframework.web.bind.annotation.ResponseBody;
 import org.springframework.web.bind.annotation.RestController;
 
+import com.alipay.sofa.jraft.core.State;
 import com.google.protobuf.util.JsonFormat;
 
 import lombok.Data;
@@ -388,22 +388,20 @@ class StoreStatistics {
      * This interface is used to check the health status of the service by accessing the /health
      * path via a GET request.
      * <p>
-     * This is a liveness signal only: it answers 200 as soon as the REST listener is up and
-     * does not consult the raft state. Use {@link #checkReady()} to find out whether this PD
-     * can actually serve.
+     * This liveness signal answers 503 for a fatal raft error or a stopped state machine.
+     * Elections and temporary quorum loss remain healthy. Use {@link #checkReady()} to find
+     * out whether this PD can actually serve.
      *
      * @return Returns a string indicating the service's health status. Typically, an empty
      * string indicates the service is healthy.
      */
-    // TODO: answer non-200 when the raft state machine is in STATE_ERROR (a failed snapshot) or
-    // the KV store never opened (a held RocksDB LOCK): both leave this PD unable to recover while
-    // /health stays 200 forever. The Helm chart (helm/hugegraph) derives a single PD's startup
-    // and liveness probes to /v1/ready for this reason; drop that derivation once this reports it.
-    // https://github.com/apache/hugegraph/issues/3222
-    // https://github.com/apache/hugegraph/issues/3226
     @GetMapping(value = "/health", produces = MediaType.TEXT_PLAIN_VALUE)
-    public Serializable checkHealthy() {
-        return "";
+    public ResponseEntity<String> checkHealthy() {
+        String state = RaftEngine.getInstance().getRaftStatus().getState();
+        boolean failed = State.STATE_ERROR.name().equals(state) ||
+                         State.STATE_SHUTDOWN.name().equals(state);
+        return ResponseEntity.status(failed ? HttpStatus.SERVICE_UNAVAILABLE : HttpStatus.OK)
+                             .body("");
     }
 
     /**

hugegraph-pd/hg-pd-service/src/test/java/org/apache/hugegraph/pd/rest/StoreAPIReadyTest.java

@@ -35,6 +35,7 @@
 import com.alipay.sofa.jraft.core.State;
 import com.alipay.sofa.jraft.entity.LeaderChangeContext;
 import com.alipay.sofa.jraft.entity.PeerId;
+import com.alipay.sofa.jraft.error.RaftException;
 
 /**
  * Pins the status a readiness probe actually receives from {@code GET /v1/ready}: 503 while
@@ -120,6 +121,44 @@ private static LeaderChangeContext context() {
         return new LeaderChangeContext(LEADER, 5, Status.OK());
     }
 
+    @Test
+    public void testFatalErrorSurvivesLeaderStopAndAnswers503() {
+        Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", untouchableNode());
+        this.stateMachine.onLeaderStart(5);
+        this.stateMachine.onError(new RaftException());
+        this.stateMachine.onLeaderStop(new Status(-1, "snapshot failed"));
+
+        Assert.assertEquals(HttpStatus.SERVICE_UNAVAILABLE,
+                            this.api.checkHealthy().getStatusCode());
+        Assert.assertEquals("", this.api.checkHealthy().getBody());
+        ResponseEntity<Map<String, Object>> response = this.api.checkReady();
+        Assert.assertEquals(HttpStatus.SERVICE_UNAVAILABLE, response.getStatusCode());
+        Assert.assertEquals(State.STATE_ERROR.name(), response.getBody().get("state"));
+        Assert.assertEquals(Boolean.FALSE, response.getBody().get("isLeader"));
+    }
+
+    @Test
+    public void testQuorumLossRemainsLive() {
+        Whitebox.setInternalState(RaftEngine.getInstance(), "raftNode", untouchableNode());
+        this.stateMachine.onLeaderStart(5);
+        Assert.assertEquals(HttpStatus.OK, this.api.checkHealthy().getStatusCode());
+        this.stateMachine.onLeaderStop(Status.OK());
+        Assert.assertEquals(HttpStatus.OK, this.api.checkHealthy().getStatusCode());
+        this.stateMachine.onStartFollowing(context());
+        Assert.assertEquals(HttpStatus.OK, this.api.checkHealthy().getStatusCode());
+        this.stateMachine.onStopFollowing(context());
+        Assert.assertEquals(HttpStatus.OK, this.api.checkHealthy().getStatusCode());
+        Assert.assertEquals(HttpStatus.SERVICE_UNAVAILABLE,
+                            this.api.checkReady().getStatusCode());
+    }
+
+    @Test
+    public void testShutdownIsNotLive() {
+        this.stateMachine.onShutdown();
+        Assert.assertEquals(HttpStatus.SERVICE_UNAVAILABLE,
+                            this.api.checkHealthy().getStatusCode());
+    }
+
     /**
      * A started raft node that fails the test if the endpoint calls it. Readiness is served
      * from the state machine callbacks precisely so that it never waits on the node lock.