apache / apache/kvrocks

Possible split brain on migration race condition

Open
#3,147 7 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
C++
Stars
4.4k
Forks
658
Avg merge
1d 20h
Merged PRs (30d)
10

Description

### Search before asking

- [x] I had searched in the [issues](https://github.com/apache/kvrocks/issues) and found no similar issues.

### Version

https://github.com/apache/kvrocks/blob/9c9ffb82de7fd70e02ae0244faf05677d27f53b0/src/cluster/slot_migrate.cc#L424-L445
In `finishSuccessfulMigration`, the code first set SUCCESS on dest node then the local cluster marks the slot range on itself as migrated.

However, if the dest node has some network issue and we have an error for `setImportStatusOnDstNode`, then it is possible the dest node actually successfully processed `setImportStatusOnDstNode` and put the slot in `imported_slots_`.However, the local cluster does not mark the slot as migrated.

In this case, the old node would be able to serve read. And the new node would be able to server read and write: https://github.com/apache/kvrocks/blob/9c9ffb82de7fd70e02ae0244faf05677d27f53b0/src/cluster/cluster.cc#L926-L930

And if clients are sending traffic to the two nodes at the same time (e.g. because some clients have old slot mapping), then it is possible the clients hitting old node may see stale value. In the worst situation, if controller/operator sets the node slots to old value, all writes to the dest node will be lost?

In https://github.com/apache/kvrocks/blob/9c9ffb82de7fd70e02ae0244faf05677d27f53b0/src/cluster/cluster.cc#L926-L930, should we only return OK if the connection is in ASKING mode?

### Minimal reproduce step

N/A

### What did you expect to see?

N/A

### What did you see instead?

N/A

### Anything Else?

_No response_

### Are you willing to submit a PR?

- [x] I'm willing to submit a PR!

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by tracing finishSuccessfulMigration in src/cluster/slot_migrate.cc, especially setImportStatusOnDstNode, then inspect the serving decision in src/cluster/cluster.cc around lines 926-930. Reproduce or model the destination network-error race and determine how migration state should prevent both nodes from serving conflicting traffic. Done means the migration state transition and ASKING behavior are made safe and the race is covered by appropriate tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
databases, distributed-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
32/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.