feat(pd): add quorum-aware /v1/ready endpoint and raft gauges - #3185
feat(pd): add quorum-aware /v1/ready endpoint and raft gauges#3185bitflicker64 wants to merge 4 commits into
Conversation
/v1/health answers 200 as soon as the Spring listener is up and never consults the raft state, so a PD that has lost its leader keeps reporting healthy to every consumer that gates on it (compose healthchecks, the Store's wait for PD, Kubernetes probes, wait-storage.sh). Keep /v1/health as pure liveness and add an unauthenticated /v1/ready that answers 200 only while the raft node is active and sees a leader, and 503 otherwise. A follower drops its leader id once heartbeats stop inside the election timeout and a leader steps down when it cannot reach a quorum, so "sees a leader" is the local view of being inside a quorum. Export three gauges next to hg_up so operators can alert on quorum loss: hg_raft_leader (1 on the leader), hg_raft_has_leader (1 while a leader is known) and hg_raft_alive_peers (peers the leader heard from inside the election timeout, NaN on non-leaders). Point the compose PD healthchecks at /v1/ready so Stores are no longer released against a leaderless PD, and document both endpoints. The PD startup CI test now also waits for /v1/ready on the live single-node PD, and the REST suite checks the endpoint and the gauges against it. Fixes apache#3183
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3185 +/- ##
=========================================
Coverage 37.78% 37.78%
- Complexity 6556 6563 +7
=========================================
Files 800 800
Lines 68929 68960 +31
Branches 9157 9166 +9
=========================================
+ Hits 26046 26059 +13
- Misses 39824 39836 +12
- Partials 3059 3065 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The liveness and readiness split is the right fix for #3183, the jraft assumptions behind it hold, and the new unit tests pass locally. Four minor notes: one on field visibility, two on comment and doc accuracy, one on image-version compatibility for the compose healthcheck change. Evidence: ran mvn -o -pl hugegraph-pd/hg-pd-test -am -Dtest=RaftEngineReadinessTest test on JDK 11 (9/9 pass) after building hg-pd-core and hg-pd-service; checked jraft 1.3.13 directly, where State.isActive() is ordinal() < STATE_ERROR, NodeImpl.listAlivePeers() throws IllegalStateException off-leader under a read lock, and getLeaderId() already maps an empty peer to null; confirmed MetricsConfig.metricsCommonTags adds hg="pd", so the hg_raft_*{ assertions in RestApiTest will match the Prometheus rendering. CI at 4dd7e71 was still running, with the pd, store and hstore integration jobs incomplete, so the live-PD assertions are unverified here.
Make RaftEngine.raftNode volatile so /v1/ready and the hg_raft_* gauges, which read it from request and scrape threads, do not rely on the @PostConstruct ordering for safe publication, and let isReady() reuse the node it already snapshotted instead of re-reading the field. Drop the wait-storage.sh mention from the /v1/ready javadoc: that script polls /v1/stores and Stores register over gRPC, so the compose healthcheck and Kubernetes probes are the real consumers. Move the raft gauge table below the existing /actuator/metrics example so the example still reads as that command's response, and note that both quorum-loss expressions are briefly true during a normal election and need a for: clause longer than the election timeout. State in the docker README that /v1/ready first ships in 1.8.0, since an older HUGEGRAPH_VERSION would leave the PD healthcheck failing and the Stores never starting, and drop a doubled blank line.
…ment health vs ready PD's /v1/health answers 200 as soon as the REST listener is up and never consults raft, so every PD and Store probe and the Store init container's PD wait count listeners, not quorum members (apache#3183). The fix, apache#3185, adds /v1/ready from 1.8.0. - pd.readinessPath and store.waitPath, both defaulting to /v1/health, so the switch to /v1/ready is a values change made with the 1.8.0 pin; the schema rejects paths without a leading slash - README: Limitations entries for the liveness-only health endpoint and for the 45 second discovery lease (measured 30 to 35 seconds); the Store wait is described as a PD wait rather than a quorum wait; the Server now registers its Pod IP, not the Service URL - NOTES and the init container messages no longer claim a quorum - tests: pd_readiness_path_test.yaml, five cases
Points at apache#3185 and says the defaults flip with the 1.8.0 image pin, so the change is not lost once that PR merges.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The endpoint, the raft accessors and the gauges are correct and well covered, and the jraft assumptions behind them hold. Two things to fix before merge: the paragraph added at docker/README.md:210-212 states a failure mode that this PR's own CI disproves, and the compose healthchecks it describes do not actually gate on readiness, because PD answers an unauthenticated request with HTTP 200 and an error body. The pre-PR /v1/health probe had the same property, so this is a missed improvement rather than a regression. Evidence: RestAuthentication.preHandle:61-66 writes the error body and returns false without response.setStatus(...); in CI run 33642694186, job build-server (rocksdb, 11), the hstore smoke pulled Docker Hub hugegraph/pd:latest (git grep '/v1/ready' origin/master -- hugegraph-pd is empty) and logged Container ...-pd-1 Healthy 11 seconds after start; PDCoreSuiteTest (101 run, 2 skipped) and PDRestSuiteTest (16 run) pass at 5bd1b96.
PD's auth interceptor rejects a request by writing an error envelope without setting a status, so every path it does not exclude, including a path that does not exist, answers 200. A healthcheck that only inspects the status code therefore reads a PD too old to carry /v1/ready as ready, which is the same "healthy without a quorum" shape this PR set out to fix. The compose files run published images, so revert their PD healthchecks and the manual verification calls to /v1/health and document what switching them over needs: a body match on "ready":true, and an image that carries the endpoint. Build the /v1/ready body from one RaftEngine.getRaftStatus() snapshot, taken from a single Node reference and a single getLeaderId() read, so a step-down midway cannot report a ready node that knows no leader. Drop the leader's raft address from the body. The endpoint is unauthenticated and the address was the one new disclosure; leadership itself is already published by the hg_raft_leader gauge, and the address stays on the authenticated /v1/members. Call the window in the hg_raft_alive_peers description what jraft measures, the leader lease timeout, which it derives as 90% of the election timeout by default, rather than the election timeout. Assert the empty body in testHealthNeedsNoAuth, since a 200 alone cannot tell an anonymous path from a rejected one.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The readiness implementation is correct, but the deployment guide overstates which compose probe is active. Evidence: JDK 11 RaftEngineReadinessTest passed 10/10; the current compose files still use /v1/health, and the Codecov patch failure is non-blocking.
The startup ordering list said PD healthchecks probe /v1/ready, but c8adc85 put both compose files back on /v1/health and this line was missed, so the guide described a quorum gate that does not exist. Name /v1/health, say it is liveness only, and point at docker/README.md for what pointing the healthchecks at /v1/ready would require.
2 participants
Purpose of the PR
/v1/healthon PD reports healthy without a raft quorum. This adds a quorum-aware readiness signal and leaves/v1/healthas pure liveness.Main Changes
RaftEngine: newhasLeader(),isReady(),getNodeState()andgetAlivePeerCount();isLeader()andgetLeader()are now null-safe before the raft node starts.StoreAPI: new unauthenticatedGET /v1/ready. Returns200with{"ready":true,"state":"STATE_LEADER","isLeader":true}while the raft node is active and sees a leader,503with"ready":falseotherwise. The body comes from oneRaftEngine.getRaftStatus()snapshot and carries no cluster addresses. Added to the auth interceptor exclusion list next to/v1/health.PDMetrics: three gauges for alerting on quorum loss:hg_raft_leader,hg_raft_has_leader,hg_raft_alive_peers(leader only,NaNelsewhere)./v1/health: these files run published images, and PD's auth interceptor answers200on any path it does not exclude, so a status-only probe reads a PD without the endpoint as ready. The docker README records what switching them over needs, a body match on"ready":trueand an image that carries the endpoint.Why "sees a leader" is the right local signal: jraft resets a follower's leader id once heartbeats stop arriving inside the election timeout, and a leader steps down when it cannot reach a quorum. So a non-null leader id means this node is inside a quorum from its own point of view, which is what a readiness probe needs. This matches the behaviour measured in the issue, where the survivor logged
Raft lost leaderwithin a second of the fault.Verify the Changes
RaftEngineReadinessTest(added toPDCoreSuiteTest) covers: no raft node, leader, follower with leader, follower without leader, empty leader id, candidate, transferring, inactive states, and the leadership-loss race ingetAlivePeerCount().RestApiTest(runs against the live CI PD) now checks that/v1/healthanswers200with an empty body, that/v1/readyreportsready=trueandSTATE_LEADERon the single-node PD without disclosing an address, and that the three gauges are exported with the expected values.test-start-hugegraph-pd.shwaits for/v1/readyto return200withready=trueafter the health endpoint responds.pd-rest-test16/16 andtest-start-hugegraph-pd.sh13/13 against a source-built PD. Against a live PD,/v1/readyanswers200{"ready":true,...}as leader and503{"ready":false,...}with two unreachable peers, while/v1/healthstays200throughout and the gauges move1/1/1to0/0/NaN.Does this PR potentially affect the following parts?
Notes for reviewers:
/v1/healthand point readiness probes at/v1/ready. Using/v1/readyas a liveness probe would restart a PD that merely lost its leader./v1/healthis unchanged; this PR only covers PD./v1/readymust match the body, not just the status code.RestAuthentication.preHandlerejects by writing an error envelope without callingsetStatus, so any non-excluded path answers200with{"status":-1,"error":"Unauthorized!"}. Fixing that root cause is out of scope here.Documentation Status
Doc - Updated