HDDS-14989. Delay follower SCM DN server start until Ratis log catch-up#10059
HDDS-14989. Delay follower SCM DN server start until Ratis log catch-up#10059xichen01 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Thanks @xichen01 for the optimization. This optimization would allow to reduce the possibility to NO_REPLICA_FOUND when doing startup (e.g. we are doing rolling SCM restarts).
However, I think this is a fundamental design issue of SCM where some information need to be replicated, usually user-facing ones (e.g. creating Container Info and updating Container State), but some information is only updated locally from the heartbeat and updated in best-effort (e.g. container replica, the DN -> List mappings between node and containers). This hybrid combinations of eventually consistent heartbeat mechanism and strongly consistent Ratis potentially can introduce similar subtle bugs. For example
- The container replica might be recorded under the NodeStateManager, but not in ContainerStateManager (e.g. due NotLeaderException).
- If container 1000 has been created in the leader but follower has not seen it, when the datanode sends a report on the replica for container 1000, the follower will reject it with ContainerNotFoundException. If the follower becomes a leader immediately after, container 1000 will not record this replica.
In the future, we need to revisit the SCM heartbeat and Ratis design for container management.
This patch should improve the situation. Nevertheless, after the initial assertion before starting the DN RPC server, there is no guarantee that this issue will not happen under network partition between SCMs (e.g. SCM leader and SCM follower is partitioned, but the SCM follower can still have heartbeats with the DNs.
We might need to discuss for a more long-term solution.
| for (RaftProtos.CommitInfoProto info : division.getCommitInfos()) { | ||
| if (info.getServer().getId().equals(leaderId.toByteString())) { | ||
| long leaderCommit = info.getCommitIndex(); | ||
| boolean caughtUp = lastAppliedIndex >= leaderCommit; |
There was a problem hiding this comment.
Let's apply the fix where we need to take into account the metadata entry.
|
@xichen01 , is this PR ready for review now? |
sumitagrawl
left a comment
sumitagrawl
left a comment
There was a problem hiding this comment.
@xichen01
Actual cause was, if state was different that of persisted at follower node while processing ICR/FCR, it does not allow state change by throwing NotLeaderException. So it does not allow registration of Replica.
Later even leader sync transaction to follower, it just get updated without adding DN as replica. This problem continue after follower becomes leader.
Due to async nature of raft log sync from leader to follower where sync can be delayed due to load and other unknown factor, the problem can not be fixed by just delaying DatanodeProtocolServer startup till leader election.
So As Fix should allow replica registration even state change fails.
Also one more case, we allow moving container from deleted to closed state, if blocks are reported and bcsid is higher. But in this case, still have issue that is not fixed currently in this. We may need allow registeration of DN replica in this case for follower also.
| // start datanode protocol server | ||
| getDatanodeProtocolServer().start(); | ||
| // If HA is enabled, start datanode protocol server once leader is ready. | ||
| if (!scmStorageConfig.isSCMHAEnabled()) { |
There was a problem hiding this comment.
This is not required, as this will delay exit of safemode rule at follower due to delay in DN registration.
| * leader's committed log entries. | ||
| * @return true if {@code lastAppliedIndex >= leaderCommitIndex} | ||
| */ | ||
| private boolean isFollowerCaughtUp() { |
There was a problem hiding this comment.
We can avoid much change to StateMachine based, as this will not resolve problem completely, due to nature that sync from leader may not be immediate, and leader keep trying sync and update
|
FYI.. related issue for Safemode Exit HDDS-15238 |
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
|
Thank you for your contribution. This PR is being closed due to inactivity. Please contact a maintainer if you would like to reopen it. |
4 participants
What changes were proposed in this pull request?
Context
Fixed a bug where reading the key could result in a NO_REPLICA_FOUND error during SCM restart and leader transfer.
For details:
https://issues.apache.org/jira/browse/HDDS-14989
Or can see
testFollowerCatchupAfterContainerClosefor detailed reproduction wayFix
updateContainerState.What is the link to the Apache JIRA'
https://issues.apache.org/jira/browse/HDDS-14989
How was this patch tested?
new test