raftstore: gc abnormal snapshots and destroy peer if failed to apply snapshots. #16992

LykxSassinator · 2024-05-10T07:54:28Z

What is changed and how it works?

Issue Number: Close #15292

What's Changed:

Previously, there were pending tasks to address the scenario where TiKV would panic if applying snapshots failed due to abnormal conditions such as IO errors or unexpected issues.

This pull request resolves the issue by introducing additional traits tombstone: bool to SnapshotApplied, indicating whether the failure occurred due to abnormal snapshots.
Additionally, the abnormal peer will send ExtraMessageType::MsgForcelyRemovePeerRequest to related leader of this region, trigger a new ConfChange with RemoveNode to remove the associated peer. Finally, this peer will be destroyed later to ensure the cluster will add one new peer by sending a fresh snapshot to the affected node.

Replace `SnapshotApplied` with `SnapshotApplied { peer_id: u64, tombstone: bool}`. And if `tombstone` == true, the
relative peer will be automatically GCed.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Release note

None.

…a snapshot. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

ti-chi-bot · 2024-05-10T07:54:31Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2024-05-10T07:54:31Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

glorv · 2024-05-14T08:08:04Z

I have 2 questions about this PR:

Is it possible to directly retry applying the snapshot if the failure is caused by unexpected(maybe some IO layer) error?
Why need to tombstone the peer after applying snapshot failed? Why not just switch the peer status to normal and so the leader should start a new snapshot automatically.

LykxSassinator · 2024-05-14T08:20:46Z

I have 2 questions about this PR:

Is it possible to directly retry applying the snapshot if the failure is caused by unexpected(maybe some IO layer) error?

Why need to tombstone the peer after applying snapshot failed? Why not just switch the peer status to normal and so the leader should start a new snapshot automatically.

Nope, it's caused by loading abnormal blocks. According to the logs in the issue tikv panic repeatedly after this tikv recover from io hang #15292 and tikv panic repeatedly with “\"[region 16697056] 19604003 applying snapshot failed\"” after down this tikv for 20mins and recover #16958, it shows that the given snapshot has abnormal file blocks, causing the failure of the applying.
For safety. Tombstone the peer and destroy it will assure that this TiKV node do not have any other remains data / meta data about this peer.

LykxSassinator · 2024-05-14T08:22:36Z

By the way, as for the point 2, I agree what u mentioned before. But for safety, this pr takes current implementation.

Why not just switch the peer status to normal and so the leader should start a new snapshot automatically.

I'll have a try for the point 2 u mentioned and do some extra tests on it to find the more appropriate way.

overvenus · 2024-05-14T08:43:06Z

I don't think we can directly mark it as Tombstone or Normal, because both options violate the current raft state machine protocol.

Tombstone indicates that the peer has been fully removed, including its Raft membership and data. However, in this case, the peer remains a valid member.
Normal means that the peer has all data up to its last commit index. But, in this case, it does not, as its last commit index has been updated to the snapshot index.

There are two ways to fix the panic:

Have PD remove this peer via a confchange, which I believe is the simplest solution.
Introduce a new RPC to instruct the leader to resend the snapshot, which may change lots of code.

glorv · 2024-05-14T09:21:06Z

Introduce a new RPC to instruct the leader to resend the snapshot, which may change lots of code.

Why need this extra RPC? At the leader side, it will switch the peer's state to normal after finishing send the snapshot. At the follower side, when apply snapshot failed, it is also doable to restore the raft state to its previous state before this snapshot. Thus, If I understand correctly, the leader should start another snapshot without any extra operation?

overvenus · 2024-05-14T09:58:17Z

At the follower side, when apply snapshot failed, it is also doable to restore the raft state to its previous state before this snapshot.

Do you mean persisting the previous state so it can be restored even after restarting TiKV?

That's doable (without introducing a new RPC), but it does add extra complexity to raftstore (and we'll need to review every code path related to snapshot handling).

components/raftstore/src/store/fsm/peer.rs

hbisheng · 2024-05-24T10:25:29Z

Out of curiosity and just trying to learn, what are the typical scenarios where applying snapshots may fail? In those cases, would it help if we retry a few times? If we destroy the peer and add a new one, it's possible that the new peer may hit the same issue again, right?

LykxSassinator · 2024-05-24T12:53:59Z

Out of curiosity and just trying to learn, what are the typical scenarios where applying snapshots may fail? In those cases, would it help if we retry a few times? If we destroy the peer and add a new one, it's possible that the new peer may hit the same issue again, right?

Yep. One typical case is that loading snapshots encounters IO errors. In this case, the issue can be attributed to physical errors on the disk or system errors, causing the TiKV panic.
And as for whether the retrying is success or not, it depends on the root cause of the IO error. If it's due to physical errors on the disk, retries may not always be successful. But if it's a system error or bugs, retries can have a higher chance of success.

components/raftstore/src/store/fsm/peer.rs

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

LykxSassinator · 2024-05-29T04:35:51Z

Cargo.toml

@@ -244,6 +244,8 @@ procinfo = { git = "https://github.com/tikv/procinfo-rs", rev = "7693954bd1dd86e
 # After the PR to rust-rocksdb is merged, remember to comment this out and run `cargo update -p rocksdb`.
 # [patch.'https://github.com/tikv/rust-rocksdb']
 # rocksdb = { git = "https://github.com/your_github_id/rust-rocksdb", branch = "your_branch" }
+[patch.'https://github.com/pingcap/kvproto']
+kvproto = { git = "https://github.com/LykxSassinator/kvproto", branch = "tombstone_peer" }


Will be updated with latest if this pr is approved.

LykxSassinator · 2024-05-29T06:19:42Z

deny.toml

@@ -99,3 +99,7 @@ exceptions = [
 unknown-git = "deny"
 unknown-registry = "deny"
 allow-org = { github = ["tikv", "pingcap", "rust-lang"] }
+# Allow list.
+allow-git = [


Will be cleared when this pr is approved.

components/raftstore/src/store/peer_storage.rs

components/raftstore/src/store/fsm/peer.rs

Connor1996 · 2024-06-03T13:12:10Z

components/raftstore/src/store/fsm/peer.rs

                self.fsm.has_ready = true;
+                // If failed on applying snapshot, it should tomebstone the peer.
+                self.fsm.peer.should_tombstone =


why not send msg here?

To prevent the loss of messages in case of sending failures due to unexpected issues. Furthermore, in the event of message loss, this abnormal peer will be left in a dangled state and unable to process any messages from the leader.

The flag should_tombstone is used to ensure that the abnormal peer continues to send the TombStone message to the leader until it is removed from the raft group.

still can send it here

FYI, the abnormal peer will stop sending messages once it is destroyed, which is triggered by receiving a RaftMessage with the flag RaftMessage::is_tombstone set to true.

Mark `unavailable == true` if the abnormal peer failed to apply snapshot and could not be normarlly removed by ConfChange. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

components/raftstore/src/store/fsm/peer.rs

Connor1996 · 2024-06-04T09:35:32Z

components/raftstore/src/store/fsm/peer.rs

+            // If failed on applying snapshot, send ConfChange to the leader to make the
+            // region tombstone the peer.
+            if self.fsm.peer.should_tombstone {
+                self.fsm.peer.send_tombstone_peer_msg(self.ctx);


how to avoid sending too many msgs?

After the abnormal peer is destoryed, it will not send message anymore.

Connor1996 · 2024-06-04T09:45:22Z

components/raftstore/src/store/fsm/peer.rs

                self.fsm.has_ready = true;
+                // If failed on applying snapshot, it should tomebstone the peer.
+                self.fsm.peer.should_tombstone =


still can send it here

components/raftstore/src/store/fsm/peer.rs

This reverts commit cad382d. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

LykxSassinator · 2024-06-06T11:59:27Z

This pr needs extra support from PD side to make the implementation more valid.

Hold until tikv/pd#8266 is merged.

LykxSassinator added 2 commits May 10, 2024 15:37

raftstore: gc abnormal snapshots and destroy peer if failed to apply …

d786f8a

…a snapshot. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

Add logs.

963bcb5

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress do-not-merge/release-note-label-needed labels May 10, 2024

Merge branch 'master' into clear_if_apply_snap_failed

6623dfa

ti-chi-bot bot added the size/M label May 10, 2024

LykxSassinator mentioned this pull request May 13, 2024

tikv panic repeatedly with “\"[region 16697056] 19604003 applying snapshot failed\"” after down this tikv for 20mins and recover #16958

Closed

LykxSassinator added 2 commits May 14, 2024 15:21

Replace ForceDestroyPeer with Applied {..}

271310a

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

Merge branch 'master' into clear_if_apply_snap_failed

ea31ab2

ti-chi-bot bot added release-note-none do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue do-not-merge/release-note-label-needed labels May 14, 2024

LykxSassinator marked this pull request as ready for review May 14, 2024 07:34

ti-chi-bot bot removed the do-not-merge/work-in-progress label May 14, 2024

LykxSassinator requested review from overvenus, glorv, tonyxuqqi and Connor1996 May 14, 2024 07:35

LykxSassinator marked this pull request as draft May 14, 2024 08:24

ti-chi-bot bot added the do-not-merge/work-in-progress label May 14, 2024

glorv reviewed May 23, 2024

View reviewed changes

components/raftstore/src/store/fsm/peer.rs Outdated Show resolved Hide resolved

Merge branch 'master' into clear_if_apply_snap_failed

b34a49f

Merge branch 'master' into clear_if_apply_snap_failed

5227de6

overvenus reviewed May 27, 2024

View reviewed changes

components/raftstore/src/store/fsm/peer.rs Outdated Show resolved Hide resolved

Merge branch 'master' into clear_if_apply_snap_failed

2e006dd

LykxSassinator mentioned this pull request May 29, 2024

raft_serverpb: add extra message MsgTombstonePeerRequest. pingcap/kvproto#1247

Draft

Polish codes and address comments.

7e981aa

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

LykxSassinator commented May 29, 2024

View reviewed changes

LykxSassinator requested review from overvenus, Connor1996 and glorv May 29, 2024 06:20

Merge branch 'master' into clear_if_apply_snap_failed

a5a3cec

Connor1996 reviewed Jun 3, 2024

View reviewed changes

components/raftstore/src/store/peer_storage.rs Outdated Show resolved Hide resolved

Connor1996 reviewed Jun 3, 2024

View reviewed changes

components/raftstore/src/store/fsm/peer.rs Outdated Show resolved Hide resolved

Connor1996 reviewed Jun 3, 2024

View reviewed changes

LykxSassinator added 2 commits June 4, 2024 10:07

Merge branch 'master' into clear_if_apply_snap_failed

5ca18d3

Addd extra unavailable flag to CheckPeerAvailableResponse.

cad382d

Mark `unavailable == true` if the abnormal peer failed to apply snapshot and could not be normarlly removed by ConfChange. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

ti-chi-bot bot added the dco-signoff: no label Jun 4, 2024

Connor1996 reviewed Jun 4, 2024

View reviewed changes

LykxSassinator added 3 commits June 4, 2024 19:21

Revert "Addd extra unavailable flag to CheckPeerAvailableResponse."

cabb4b4

This reverts commit cad382d. Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

Polish annotations.

6a67d17

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

Polish codes and add extra uts.

cc692c8

Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

ti-chi-bot bot added dco-signoff: yes and removed dco-signoff: no labels Jun 4, 2024

LykxSassinator requested a review from Connor1996 June 4, 2024 12:44

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raftstore: gc abnormal snapshots and destroy peer if failed to apply snapshots. #16992

raftstore: gc abnormal snapshots and destroy peer if failed to apply snapshots. #16992

LykxSassinator commented May 10, 2024 •

edited

ti-chi-bot bot commented May 10, 2024

ti-chi-bot bot commented May 10, 2024

glorv commented May 14, 2024

LykxSassinator commented May 14, 2024

LykxSassinator commented May 14, 2024 •

edited

overvenus commented May 14, 2024

glorv commented May 14, 2024

overvenus commented May 14, 2024 •

edited

hbisheng commented May 24, 2024

LykxSassinator commented May 24, 2024 •

edited

LykxSassinator May 29, 2024

LykxSassinator May 29, 2024

Connor1996 Jun 3, 2024

LykxSassinator Jun 4, 2024 •

edited

Connor1996 Jun 4, 2024

LykxSassinator Jun 4, 2024

Connor1996 Jun 4, 2024

LykxSassinator Jun 4, 2024

Connor1996 Jun 4, 2024

LykxSassinator commented Jun 6, 2024

raftstore: gc abnormal snapshots and destroy peer if failed to apply snapshots. #16992

Are you sure you want to change the base?

raftstore: gc abnormal snapshots and destroy peer if failed to apply snapshots. #16992

Conversation

LykxSassinator commented May 10, 2024 • edited

What is changed and how it works?

Related changes

Check List

Release note

ti-chi-bot bot commented May 10, 2024

ti-chi-bot bot commented May 10, 2024

glorv commented May 14, 2024

LykxSassinator commented May 14, 2024

LykxSassinator commented May 14, 2024 • edited

overvenus commented May 14, 2024

glorv commented May 14, 2024

overvenus commented May 14, 2024 • edited

hbisheng commented May 24, 2024

LykxSassinator commented May 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LykxSassinator Jun 4, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LykxSassinator commented Jun 6, 2024

LykxSassinator commented May 10, 2024 •

edited

LykxSassinator commented May 14, 2024 •

edited

overvenus commented May 14, 2024 •

edited

LykxSassinator commented May 24, 2024 •

edited

LykxSassinator Jun 4, 2024 •

edited