fix(store): rejoin a Store rebuilt empty at its old raft address - #3234
Conversation
A Store whose data volume is lost comes back at the same raft address but registers with PD under a new store id. PD's reallocShards puts the new id into every shard group and fires ChangeShard, but the raft configuration already holds that address, so changePeers has nothing to add: the rebuilt Store never gets a raft node or a snapshot. The group leader still has the old id in its local shard group, and its partition heartbeat writes that id back to PD a few seconds later. Every group then names a store id that no longer exists and runs on two live replicas, while /v1/stores and the cluster state read healthy. When PD's shard list and the raft configuration have the same endpoints but PD names another store id at one of them, the leader now creates the raft node on that endpoint (jraft then installs a snapshot into it), takes PD's store ids into its local shard group, reports the group to PD and waits for the peer to catch up. Resolving a raft endpoint to a store id on a configuration commit also checks PD's current group, so a later leader change does not bring the old id back.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3234 +/- ##
============================================
+ Coverage 41.35% 41.37% +0.02%
- Complexity 7299 7322 +23
============================================
Files 802 802
Lines 69688 69770 +82
Branches 9291 9309 +18
============================================
+ Hits 28816 28867 +51
- Misses 37576 37613 +37
+ Partials 3296 3290 -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 repair path looks correct for the #3227 case. Endpoint equality is checked before any id sync, the leader is never included, createRaftNode is a no-op on a Store that already runs the group, and onLeaderStart and onStartFollowing both resolve through PD's group before the leader's next partition heartbeat, so the old id does not come back after a leader change. One minor retry issue: a failed createRaftNode now returns TASK_CONTINUE, and each retry takes a fresh raft snapshot. Evidence: read the full diff at 9b6bf41 with PartitionEngine.changePeers, doChangeShard, the SYNC_PARTITION_TASK handler, HgCmdClient.tryWithThrowable, HgCmdProcessor.handleCreateRaft, HgStoreEngine.createPartitionEngine and HeartbeatService.partitionHeartbeat. CI is green apart from codecov patch/project (patch coverage 30%) and build-server (hbase), which was still running when this was reviewed.
| if (!status.isOk()) { | ||
| log.info("Raft {} createRaftNode, peer:{}, reason:{}", getGroupId(), peer, | ||
| status.getErrorMsg()); | ||
| return HgRaftError.TASK_CONTINUE.toStatus(); |
There was a problem hiding this comment.
Minor: When createRaftNode fails here, the method returns TASK_CONTINUE, and doChangeShard re-queues the task straight away through addRaftTask(SYNC_PARTITION_TASK). Before that, local ids are not updated yet, so the retry finds the same changed endpoint and calls doSnapshot again on line 474. A single failure takes about 1.5 s: HgCmdClient.tryWithThrowable retries 5 times with a 100/200/300/400/500 ms backoff. Each retry also appends a raft entry, and setSnapshotLogIndexMargin is commented out in PartitionEngine.init, so jraft does not skip these snapshots. If the rebuilt Store goes down again after PD has moved the group to its new id (for example a crash-looping Pod), the leader of every affected group takes a snapshot and appends a log entry about every 1.5 to 2 s until the Store is back. changePeers handles the same createRaftNode failure at line 329 by returning the RPC status (code -1), which is not TASK_CONTINUE and does not re-queue. Please do the same here, or at least call doSnapshot only once per repair (for example after createRaftNode succeeds, or only when getReplicatorState shows the peer is not already installing a snapshot), so a peer that stays down does not trigger a retry loop that snapshots on every pass.
There was a problem hiding this comment.
Fixed in 97f5b01. A failed createRaftNode now returns the RPC status, as changePeers does at line 329, so doChangeShard no longer re-queues it. The task is already stored by the SYNC_PARTITION_TASK handler, and ReplicatorStateListener runs it again when the endpoint comes back online. The snapshot is taken once, after the raft node exists. Rerun on images from 0c2a180: converged 17 s after the Tombstone, all 12 leaders logged snapshot after create raft node, 0 of 1494 writes lost (evidence).
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Timeout retries lose pending repair state, and the required operator documentation is not included.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Fixes Store rejoining after data loss when its Raft address is reused under a new Store ID.
Changes:
- Detects Store identity changes at unchanged Raft endpoints.
- Creates replacement Raft nodes, synchronizes snapshots, and resolves IDs using PD state.
- Adds Store-ID resolution and endpoint-difference tests.
| File | Description |
|---|---|
StoreIdChangeTest.java |
Tests Store-ID and endpoint resolution. |
CoreSuiteTest.java |
Registers the new test suite. |
PartitionEngine.java |
Implements identity synchronization and replication waiting. |
PartitionManager.java |
Resolves current Store IDs against PD metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return waitForReplicate(changed) ? HgRaftError.OK.toStatus() : | ||
| HgRaftError.TASK_CONTINUE.toStatus(); |
There was a problem hiding this comment.
97f5b01: a timeout now logs a warning and returns TASK_ERROR, so it no longer re-queues a pass that finds nothing to do and reports OK. The ids stay taken before the wait on purpose: a rebuilt Store that restarts before PD names it exits in PartitionManager.loadPartitions (System.exit(0) at line 302), so moving them after the wait would crash-loop it during catch-up. Nothing is lost on a timeout: the node exists and its replicator keeps installing the snapshot on its own; the task result only goes to the log. The timeout path needs a live raft group, so it has no unit test; the kind rerun in the PR description covers the normal path.
| * @param shards shard list from PD | ||
| * @return OK when nothing changed or the peers caught up, TASK_CONTINUE to retry | ||
| */ | ||
| private Status syncShardIdentities(List<Metapb.Shard> shards) { |
There was a problem hiding this comment.
0c2a180 adds the procedure to hugegraph-store/docs/operations-guide.md (Single Store Node Failure, step 3): retire the old id with Tombstone, run patrolPartitions, verify, then delete it, with the note that Store versions without this fix never finish it. The website only lists these PD endpoints (quickstart hugegraph-pd), with no replacement procedure to correct. The Helm chart README is not in master yet; it lives in #3218, and its "do not delete a Store volume" warning changes there with a version caveat once this is merged and rerun on master images.
When the rebuilt Store cannot be reached, syncShardIdentities returned TASK_CONTINUE. doChangeShard then re-queued the task as a raft entry at once, and every pass took a new raft snapshot before the RPC failed again, about every 1.5 to 2 seconds for as long as the Store stayed down. Return the createRaftNode status instead, as changePeers does for the same failure. The task stays stored, and the replicator listener runs it again once the endpoint comes online. Take the snapshot only after the raft node exists. A peer that has not caught up after the wait now returns TASK_ERROR instead of TASK_CONTINUE. The re-queued pass found the ids already taken and returned OK without doing anything; the replicator keeps installing the snapshot either way. The ids stay taken before the wait, because a rebuilt Store that restarts before PD names it exits in loadPartitions.
The single-node failure procedure said PD assigns partitions to a new Store by itself. A replacement that reuses the failed node's raft address with an empty data directory registers under a new store id, and the old id keeps its partitions until it is retired. Add the Tombstone, patrol, verify and delete steps, and say that Store versions without the apache#3227 fix never finish them.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no actionable issue found after five independent review lanes and synthesis at 0c2a180. Score: 8.8/10. The earlier retry, timeout-result and recovery-documentation findings are addressed. Exact-head Store CI passed 30 core tests and 9 raft-core tests; live recovery and the >600-second timeout were not independently rerun. This score is not a merge approval; remaining CI and unresolved review threads still need to be cleared.
Reserve the chart-owned checksum/ podAnnotations prefix on every component: user annotations render after the chart's own and the last duplicate key wins, so a fixed value would pin the rotation checksum. Refuse server.securityContext.readOnlyRootFilesystem=true, mirroring the Hubble guard, because the Server wrapper rewrites rest-server.properties inside the image. Reject updateStrategy.rollingUpdate options together with type OnDelete in the schema for PD and Store; Kubernetes refuses that combination at apply time, which would otherwise surface mid-upgrade. Unit tests cover the template guards and the CI reject-invalid-values step covers the schema constraint. Use the get-with-default pattern for the $exposed NetworkPolicy check, matching the rest of validateValues. Remove the constant $pdMeta and $wrapper indirection from the Server Deployment, and remove the unused server.restServer.minFreeMemory / batchMaxWriteThreads knobs end to end (template, values, schema, README); the chart is unreleased, so nothing depends on them. helm template output on the default, single and cluster presets is byte-identical before and after. Refresh the docs: the server.testResources default in the README matches values.yaml again, and the runbook follows apache#3232, apache#3233 and apache#3234 (merged 2026-09-24). The empty-PVC Store retirement was re-proven on images built from master at dbb6663: all 12 groups converged onto the replacement 1 s after Tombstone and patrol, with 0 acknowledged writes lost, so the Disaster Recovery section documents the working procedure with a version caveat instead of a prohibition.
Bring the deployment page level with the chart at 8603cdbb3: the values-cluster preset now ships NetworkPolicy and 5Gi/8Gi Store memory, PD startup and liveness derive from the replica count (pd.livenessPath, single-PD /v1/ready per apache/hugegraph#3222), the Store roll procedure no longer treats Up in PD as the between-pods check, and the Limitations follow the merged fixes, including in-place empty-PVC Store recovery on images carrying apache/hugegraph#3234. Add an operations page (EN and CN, registered in the version route map) rewritten for operators from the chart README: ports and health, scheduling, partition sharding, NetworkPolicy with the per-CNI NodePort client behavior, safe Store rolls, the disaster recovery runbook with the post-#3234 procedure, scaling including the Store drain steps, running Hubble outside the cluster, and the two causes of "Could not rebind". Verified: scripts/hugo.sh build passes and both new pages render with their cross-links in EN and CN.
Purpose of the PR
A Store whose data volume is lost comes back at the same raft address (on Kubernetes the StatefulSet always rebuilds the Pod under the same DNS name; on bare metal, any rebuild that reuses host:port) but registers with PD under a new store id. Retiring the old id per the documented procedure (
Tombstone, thenpatrolPartitions) never repairs the groups: every group ends up naming a store id that no longer exists, the rebuilt Store holds no partitions, every group runs on two live replicas, and/v1/stores, the cluster state, Hubble and Pod readiness all read healthy. #3227 has the full measurement and a diagram of the loop.There were two faults, and both had to go:
reallocShardsputs the new id Y into each group and fires ChangeShard. The group leader maps the ids to raft endpoints (shards2Peers), gets the same endpoint set it already has, sochangePeersadds nothing. The rebuilt Store never gets a raft node for any group, and jraft only installs a snapshot into a node that exists. On the [Bug] A Store rebuilt with an empty volume never rejoins its raft groups; retirement (Tombstone + patrol) cannot repair it and every health surface reads healthy #3227 run the leaders loggedReplicator stateChanged hugegraph-store-2...:8510 OFFLINEfor that address 11,814 times on store-0 alone.ShardGroupstill lists the old id X. Its partition heartbeat (HeartbeatService.partitionHeartbeat, every 5 s) sends that list, andPartitionService.partitionHeartbeatoverwrites PD's corrected group with it. On the [Bug] A Store rebuilt with an empty volume never rejoins its raft groups; retirement (Tombstone + patrol) cannot repair it and every health surface reads healthy #3227 run PD rewrote group 1 with X three seconds afterreallocShardswrote Y (Raft 1 updateShardGroup ... store_id: 1190848017232829764at 17:59:47, PD leader log).onConfigurationCommittedresolves endpoints through the same stale list, so any later leader change would do the same.Main Changes
Store side only:
hg-store-core, its tests, and the Store operations guide. No PD, proto or configuration change.PartitionEngine.doChangeShard: afterchangePeers, call the newsyncShardIdentities. When PD's shard list and the raft configuration have the same endpoints but PD names another store id at one of them (never the leader itself), the leader:createRaftNodeto that endpoint with the current configuration, so the rebuilt Store creates the raft node and jraft installs the leader's snapshot into it (the same RPCchangePeersalready uses for new learners), then takes a snapshot. If the RPC fails, it returns that status aschangePeersdoes, so nothing is re-queued; the stored task runs again when the endpoint's replicator comes online;Replicate(the existing wait loop, moved intowaitForReplicateso both paths share it). A timeout logs a warning and returnsTASK_ERROR; the replicator keeps installing the snapshot. The ids are taken before the wait because a rebuilt Store that restarts before PD names it exits inPartitionManager.loadPartitions.No raft membership change happens: the endpoint is already a voter, and the rebuilt node joins that slot with an empty log.
PartitionManager.getStoreByRaftEndpoint(ShardGroup, Metapb.ShardGroup, String): when the id the local group holds for an endpoint is no longer in PD's current group, use the id PD's group has at that endpoint.onConfigurationCommittedfetches PD's group once and resolves through it, so a leader change after the repair does not put the old id back. The two-argument overload is unchanged.hugegraph-store/docs/operations-guide.md, Single Store Node Failure: the steps for a replacement that reuses the failed node's raft address (retire the old id, patrol, verify, delete), and that Store versions without this fix never finish them.StoreIdChangeTest(inCoreSuiteTest): endpoint resolution when the local id is still valid, retired, or deleted from PD, and the endpoint-diff rule (finds a new id at an unchanged address, ignores real membership changes, never includes the leader itself).Not in this PR: PD retiring the old record by itself when a new id registers at its raft address. The documented
Tombstonestep is still needed to start the repair; with this change it now works. Readiness while a rebuilt Store catches up stays with #3229.Verifying these changes
mvn test -pl hugegraph-store/hg-store-test -am -P store-core-testruns 30 tests, 8 of them new, 0 failures (22 before).store-raftcore-test(9) andpd-core-test(104, 2 skipped) unchanged and green;editorconfig:checkclean. JDK 11, run 2026-09-24.Tombstonethe old id on the PD leader,patrolPartitions, wait up to 25 min. Same script, same chart, same host, only the Store/PD/Server images differ. Control: the:latestimages [Bug] A Store rebuilt with an empty volume never rejoins its raft groups; retirement (Tombstone + patrol) cannot repair it and every health surface reads healthy #3227 was measured on (apache master83ef9f3, storesha256:7bc0cbd81550). Fix: images built from this branch (9b6bf414, revision label checked in the pod).83ef9f3)9b6bf414):8520/v1/partition/{id}DELETE /v1/store/{old}/v1/storesand cluster stateUp; rebuilt StorepartitionCount0;Cluster_OKUp; rebuilt StorepartitionCount12;Cluster_OK(run 2, from 48 s on)Run 2 (same images, fresh cluster) adds a 5-minute health watch after convergence, a second
patrolPartitions, and a restart of the rebuilt Store with its PVC kept. It converged 17 s after the Tombstone. From the 48 s sample on, every 30 s sample readCluster_OK, the rebuilt StoreUpwithpartitionCount12, and all 12 groupsPState_Normal. The second patrol changed nothing. The restarted Store was Ready in 151 s under the same id, with 0 container restarts, nois illegalexit inloadPartitions, and all 12 groups served. The Store-levelpartitionCountfollows the Store heartbeat (30 s), so it lags the repair: run 1 sampled it 1 s after the Tombstone and sawpartitionCount0 andCluster_Warnwhile the snapshots were still installing.Rerun after the review follow-ups (
97f5b013,0c2a1801), images built from0c2a1801: converged 17 s after the Tombstone;Cluster_OK, rebuilt StorepartitionCount12 and all 12 groupsPState_Normalthrough the 5-minute watch; restart with the PVC Ready in 151 s, same id, 12 of 12; 0 of 1494 acknowledged writes lost. All 12 leaders loggedsnapshot after create raft nodethendoChangeShard result is Status[OK]. Unit suites unchanged (30, 9, 104). Evidence: bitflicker64/hugegraphlogs/fix-3227-review.Store leader log on the fixed run, per group (store-0 and store-1 between them cover all 12):
Evidence (scripts, run logs, per-Store partition views, PD and Store logs, write ledgers): bitflicker64/hugegraph
logs/fix-3227(s2b-run.shis the harness,README.mdlists the layout, one folder per run)Does this PR potentially affect the following parts?
hg-store-core)Documentation Status
Doc - TODO: required documentation is pending; complete it before merging.Doc - Done: documentation is included here or linked below.Doc - No Need: no user-visible documentation is affected.Documentation files in this PR or paired hugegraph-doc PR:
hugegraph-store/docs/operations-guide.md(this PR). The website (hugegraph-doc) only lists these PD endpoints, with no replacement procedure, so no paired doc PR.After this merges
For whoever picks this up next. Nothing here blocks the merge.
masterat or after the merge commit of this PR, then build the images with the revision label:SHA=$(git rev-parse HEAD); IMAGE_TAG=m3227 SOURCE_REVISION=$SHA docker buildx bake -f docker/bake.hcl pd store server-hstore --set '*.platform=linux/amd64' --set "*.labels.org.opencontainers.image.revision=$SHA" --load, and tag each ashg3227/{pd,store,server}:master. (Or use the Docker Hub:latestpublished after the merge, and record its digests and revision instead of building.)OBSERVE_S=300 WAIT_S=1500 bin/s2b-run.sh masterwith the harness linked under Evidence. It expects the chart under~/hg-3227/chart/helm/hugegraph, andharness/kind.yaml,writer.shandverify-ledger.shfrom the evidence folder under~/hg-lifecycle(kind.yamlat the top, the two scripts inscripts/). It also needs the imageshg3227/hubble:controlandhg3227/probe:1, the latter built fromharness/probe.Dockerfile.converged=1; 0 groups naming the old id and 12 naming the new one; the rebuilt Store answers 200 on all 12:8520/v1/partition/{id}; after the observation window/v1/storesshows the rebuilt StoreUpwithpartitionCount12 and the clusterCluster_OK; the store-2 restart with its PVC comes back with 12 of 12 and nois illegalline;missing=0in the ledger. Attach thes2b.logandhealth.logto this PR as a comment.feat/hstore-helm-chart-3132, the head of apache PR feat(helm): add HStore deployment chart #3218 and its mirror feat(helm): add HStore deployment chart hugegraph/hugegraph#221, filehelm/hugegraph/README.md). Two places say a Store volume must never be deleted: the paragraph starting "Do not delete a Store's PersistentVolumeClaim on current images." under Disaster Recovery, and the first bullet of "## Limitations" ("A Store cannot be recovered in place after its volume is lost"). Replace them with the retirement procedure that now converges (find the old id at the replaced Pod's address,POST /v1/store/{oldId}with{"storeState":"Tombstone"}on the PD leader,patrolPartitions, verify every group lists onlyUpStores and the replaced Store's:8520/v1/partition/{id}answers 200, thenDELETE /v1/store/{oldId}). Keep a version caveat: images built before this PR's merge commit, including every published 1.7.x image and:latestbefore that merge, still cannot recover in place, and the old warning applies to them word for word. Only make this change after step 1 passes, and cite the step 1 run in the commit message.Upor add a readiness signal. It does change what [Feature] Store-side readiness for rolling replacement: registration marks Up before partition restore, and a stopped Store stays Up in every shard group for keepAlive-timeout #3229 is about: before, a Store rebuilt with an empty volume never received data, so "Up but empty" lasted forever; now it receives a snapshot for every group after the old id is retired, and the remaining gap is the window betweenUpand caught-up (1 s on the 243 MB test data, longer on real data), plus the staleUpof a stopped Store. Post one short comment on [Feature] Store-side readiness for rolling replacement: registration marks Up before partition restore, and a stopped Store stays Up in every shard group for keepAlive-timeout #3229 saying this, linking this PR.s2b-run.shfrom the evidence intoscripts/lifecycle/, add a row for it toscripts/lifecycle/README.md, and add a section toreferences/test-suite.mdunder Lifecycle with the procedure, the pass criteria from step 1, and the expected control result on images older than this merge (12 of 12 groups keep the old id, rebuilt Store 500 on every group,Cluster_OKthroughout). Mark the old S2b finding in that file as fixed by this PR.