etcdserver: apply() sets consistIndex for any entry type #7856
Conversation
|
bunch of CI failures. Is your git branch out-of-date? |
|
@gyuho my branch is synced today. let me see what those failures are. |
etcdserver/server.go
Outdated
| @@ -1292,8 +1294,6 @@ func (s *EtcdServer) apply(es []raftpb.Entry, confState *raftpb.ConfState) (appl | |||
| func (s *EtcdServer) applyEntryNormal(e *raftpb.Entry) { | |||
| shouldApplyV3 := false | |||
| if e.Index > s.consistIndex.ConsistentIndex() { | |||
| // set the consistent index of current executing entry | |||
| s.consistIndex.setConsistentIndex(e.Index) | |||
There was a problem hiding this comment.
Currently applyEntryNormal sets the consistent index before it begins modifying store, now the consistent index is set after completing the modification and updating apply index. That will admit cases where applied index > consistent index, meaning apply is ahead of raft, which should never happen.
There was a problem hiding this comment.
How is consistent index used in the store and v3 applier? Initially, I thought consistent index is used to keep track of latest applied entry, and it is used to guard v3 applier so that we don't re-apply applied entries.
There was a problem hiding this comment.
The ConsistentIndex tracks the raft index, the AppliedIndex tracks the last index that has been completely applied to the store. The applied index is necessarily <= the raft index since raft is where the data for the apply is coming from.
etcdserver/server.go
Outdated
| @@ -1284,6 +1284,8 @@ func (s *EtcdServer) apply(es []raftpb.Entry, confState *raftpb.ConfState) (appl | |||
| atomic.StoreUint64(&s.r.term, e.Term) | |||
| appliedt = e.Term | |||
| appliedi = e.Index | |||
| // set the consistent index of current executing entry | |||
| s.consistIndex.setConsistentIndex(appliedi) | |||
There was a problem hiding this comment.
this probably belongs at the beginning of applyConfChange, similar to the setConsistentIndex/setAppliedIndex path that's already in applyEntryNormal
fanminshi
force-pushed
the
fix_consistent_index_update
branch
2 times, most recently
from
52f093e
to
0d1cdf1
Compare
|
all fixed. PTAL |
| } | ||
|
|
||
| // create EntryConfChange entry | ||
| now := time.Now() |
There was a problem hiding this comment.
oh nvm. it's used in NewMember
| // set the consistent index of current executing entry | ||
| if e.Index > s.consistIndex.ConsistentIndex() { | ||
| s.consistIndex.setConsistentIndex(e.Index) | ||
| } | ||
| var cc raftpb.ConfChange | ||
| pbutil.MustUnmarshal(&cc, e.Data) | ||
| removedSelf, err := s.applyConfChange(cc, confState) |
There was a problem hiding this comment.
s.setAppliedIndex(e.Index)?
previously, apply() doesn't set consistIndex for EntryConfChange type. this causes a misalignment between consistIndex and applied index where EntryConfChange entry results setting applied index but not consistIndex. suppose that addMember() is called and leader reflects that change. 1. applied index and consistIndex is now misaligned. 2. a new follower node joined. 3. leader sends the snapshot to follower where the applied index is the snapshot metadata index. 4. follower node saves the snapshot and database(includes consistIndex) from leader. 5. restarting follower loads snapshot and database. 6. follower checks snapshot metadata index(same as applied index) and database consistIndex, finds them don't match, and then panic. FIXES etcd-io#7834
fanminshi
force-pushed
the
fix_consistent_index_update
branch
2 times, most recently
from
2495eee
to
89d7bb9
Compare
|
all fixed. PTAL |
fanminshi
force-pushed
the
fix_consistent_index_update
branch
from
89d7bb9
to
e33b10a
Compare
|
merging when green. |
Codecov Report
@@ Coverage Diff @@
## master #7856 +/- ##
==========================================
+ Coverage 75.81% 76.11% +0.29%
==========================================
Files 332 332
Lines 26183 26186 +3
==========================================
+ Hits 19851 19931 +80
+ Misses 4909 4842 -67
+ Partials 1423 1413 -10
Continue to review full report at Codecov.
|
previously, apply() doesn't set consistIndex for EntryConfChange type.
this causes a misalignment between consistIndex and applied index
where EntryConfChange entry results setting applied index but not consistIndex.
suppose that addMember() is called and leader reflects that change.
1. applied index and consistIndex is now misaligned.
2. a new follower node joined.
3. leader sends the snapshot to follower
where the applied index is the snapshot metadata index.
4. follower node saves the snapshot and database(includes consistIndex) from leader.
5. restarting follower loads snapshot and database.
6. follower checks snapshot metadata index(same as applied index) and database consistIndex,
finds them don't match, and then panic.
FIXES #7834