-
Notifications
You must be signed in to change notification settings - Fork 9.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
etcdserver: apply() sets consistIndex for any entry type #7856
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably belongs at the beginning of applyConfChange, similar to the setConsistentIndex/setAppliedIndex path that's already in applyEntryNormal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed. will change.
52f093e
to
0d1cdf1
Compare
all fixed. PTAL |
} | ||
|
||
// create EntryConfChange entry | ||
now := time.Now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now
is not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
2495eee
to
89d7bb9
Compare
all fixed. PTAL |
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