raft: Forward-port improvements to campaign on restart#1589
Conversation
Current coverage is 54.08% (diff: 100%)@@ master #1589 diff @@
==========================================
Files 84 84
Lines 13939 13848 -91
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 7509 7490 -19
+ Misses 5422 5357 -65
+ Partials 1008 1001 -7
|
|
Passes Docker integration tests (except the ones that are normally flaky for me) |
|
@aaronlehmann I don't quite understand. Does this code needed at all in master? |
|
I think it's more correct than the code that's on master right now. It may call |
| // Node ID should be in the progress list to Campaign | ||
| if len(n.cluster.Members()) <= 1 { | ||
| if n.campaignWhenAble && n.addedSelf { | ||
| if len(n.cluster.Members()) == 1 { |
There was a problem hiding this comment.
I think this should just get n.cluster.Members() and check that sole member is self. Then you don't need addedSelf variable.
There was a problem hiding this comment.
Hm. For some reason I thought there were situations where we could call n.cluster.AddMember() without actually applying a conf change in raft, but examining the code, I can't see any.
Updated. I'll update the 1.12 version as well.
aaronlehmann
force-pushed
the
fix-campaign-on-restart-master
branch
from
d0844fa to
50921a9
Compare
| // Node ID should be in the progress list to Campaign | ||
| if len(n.cluster.Members()) <= 1 { | ||
| if n.campaignWhenAble { | ||
| members := n.cluster.Members() |
There was a problem hiding this comment.
I wonder why not just campaign once if you're sole member?
There was a problem hiding this comment.
Actually, I see. Can you pls change comment, so it'll reflect that we start campaign also on inital cluster start?
There was a problem hiding this comment.
Comment updated, thanks for noticing that.
This brings the code on master in sync with the 1.12 changes in moby#1588. The code on master does not have the same problem (since a newer etcd/raft library lets it skip the problematic check), but the version being adopted by 1.12 should be a bit more robust. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
aaronlehmann
force-pushed
the
fix-campaign-on-restart-master
branch
from
50921a9 to
be79cc4
Compare
|
LGTM |
5 participants
This brings the code on master in sync with the 1.12 changes in #1588. The code on master does not have the same problem (since a newer etcd/raft library lets it skip the problematic check), but the version being adopted by 1.12 should be a bit more robust.
This is a separate PR against master instead of cherry-picking from master to bump_v1.12.2 because the code has diverged and needs to be a bit different on master anyway (different etcd/raft version).
cc @LK4D4 @mrjana