node not part of the cluster is not allowed to vote #477

dhiaayachi · 2021-09-23T19:33:55Z

When a follower is removed from the cluster it's removed from the configuration servers list but still run a follower loop (originally a follower node or ShutdownOnRemove set to false for Leader). When that node timeout (because it's not receiving heartbeat from the other nodes in the cluster), it transition to a Candidate state and try to trigger an election. 2 possible cases:

the cluster is stable and have a leader: the vote request will be rejected
the cluster is unstable and don't have a leader: the vote is granted and the removed node will be part of the voting nodes.

This uncover 2 issue:

a removed node should not transition to a Candidate state. This is fixed in Forbid removed node from being a Candidate #476
a node that is not part of the cluster should not be allowed to vote: this a bit trickier to fix because the RequestVoteRequest only have the ServerAddress which do not represent a unique ID of the node, the one stored in the configuration server list. A test that reproduce the issue is in this PR.

To keep track of things in this PR here is a list of what I intend to achieve:

Add ID and Addr field in RPCHeader
Transition from using Candidate and Leader fields in Protocol Version 3 to using Addr (part of RPC header) field in protocol version 4
Use ID to validate that the node that is triggering an election is part of the latest configuration
Expose the Leader ID field as part of the API to be used in consul
Mixed ProtocolVersion Test

dhiaayachi · 2021-10-01T01:37:18Z

@mkeeler I was thinking about this after our conversation today and I noticed few things:

Adding the ID as part of EncodePeer and DecodePeer would break compatibility because in the case of node that is version 4 talking to a node version 3, the node version 3 would not be able to decode the receiver peer correctly and fail, So we will need to make a 2 step upgrade process as discussed
I was thinking that we can add the ID as a separate field with a separate set of encoders decoders like it's currently the case in this PR, this will make the upgrade seamless and we still can survive with nodes running different versions (3 and 4)
I also thought that having the ID of the source node as part of any raft request/response is very useful (at least for 2 know cases that we have today) and instead of adding it as part of RequestVoteRequest only I can add it in RPCHeader so all the request/response benefit from it. We can encode it all the time and only use it with a protocolVersion check in all the needed cases (RequestVoteRequest , AppendEntriesRequest and InstallSnapshotRequest)

Let me know what you think about this?

mkeeler · 2021-10-05T17:28:42Z

So from our original discussions I was thinking the upgrade process would look like:

All nodes are on some existing version at protocol 3. DecodePeer & EncodePeer only handle the raw server address.
New nodes are added with the new version but set to use protocol 3. EncodePeer sends the server address form. DecodePeer can handle either the old form or some new struct that contains the id and address.
Old nodes are removed.
New nodes get a rolling restart and get switched to protocol 4. EncodePeer will now send both the id and address as the peer info instead of just the address. DecodePeer can still handle both forms though.

So with that sort of flow it wouldn't break compatibility. However I think I prefer your approach of adding the id to the RPCHeader as we are likely to want that information in other places in order to track the leaders id and make it available to the application running Raft.

I wonder whether we should add both the address and id of the peer server to the RPCHeader and have Raft proto 4 not encode the Candidate field of the RequestVoteRequest and the Leader field of the AppendEntriesRequest / InstallSnapshotRequest. What do you think?

dhiaayachi · 2021-10-05T17:41:32Z

Thank you for taking a look at this @mkeeler

About the upgrade process, I was thinking the same. I just called it a breaking change from the perspective that it need a special upgrade flow (2 restarts).

About removing the Candidate and Leader fields I need to check if that bring any other implications, but I think the use of it is internal to raft and it's doable, I will update the PR and get back to you for a round of review.

dnephin

I'm not all that familiar with this code but I had some high level questions/suggestion.

commands.go

transport.go

mkeeler

I think you are already going back and adding some mixed protocol version tests which is great.

I suspect they will show some issues. I pointed out one place where I think there is a backwards compat concern but generally I think all the places where there is something like following probably needs to execute the code unconditionally instead of only if the proto isn't v4:

   // encode some older proto 3 specific field

Basically we should always encode the older Leader and Candidate fields even if the proto is v4 so that if we are communicating with v3 nodes they can parse out the information appropiately.

raft.go

transport.go

This reverts commit 316c59a.

…luster before granting a vote

…` and `Leader` in version > 3

mkeeler

All the main code looks good. There are a handful of comment fixups and two logging fixes suggested but nothing that would require further review of.

raft.go

raft_test.go

Co-authored-by: Matt Keeler <mkeeler@users.noreply.github.com>

dnephin

Nice! I think this is looking good.

If I understand correctly, this PR does the following:

Increments to protocol version to 4, to indicate that we have new data available in the RPCHeader
Uses that new ID in the header to check if the voter is actually in the configuration before considering the vote.

That makes sense to me, and in general having the ID and addr of the server sending the message seems like it is useful.

A few questions, a few more style suggestions. I think the only blockers are the API backwards compat, and the one place we don't version check req.Addr.

But I do think if we can keep using req.Leader and req.Candiate when available, that might help keep the code easier to follow by reducing the number of places we check the protocol version.

I did not look at the tests much, hopefully Matt reviewed those more closely. Otherwise I can have a more detailed look.

api.go

commands.go

fuzzy/verifier.go

net_transport.go

raft.go

net_transport_test.go

raft.go

dnephin

Thinking about the Protocol version increase, are you sure we need to increase the version?

Could we not optionally check if that field is set, and if it is do the new logic? If it's not then nothing we can do about it. That would avoid the difficult upgrade scenario, and allow everyone on V3 to get the benefit of this new check.

dhiaayachi

Thank you for reviewing this @dnephin, see my comments in line.

I'm starting to think more that v4 is not necessary for this. Your point about using the new field and fallback to the old one when the new is empty seem reasonable to me and the fact that we always fill the old fields will ensure the compatibility with the nodes running a code version prior to this PR.

When I first did this with v4 version I was thinking that we will remove the Candidate and Leader field (or stop filling them in v4) but we ended up filling them all the time which ensure retro compatibility.

Testing the wire protocol compatibility is trivial and can easily be done the way you described.

What you think @mkeeler ?

api.go

commands.go

fuzzy/verifier.go

net_transport.go

raft.go

… nil to fallback on the old fields.

dhiaayachi · 2021-11-30T20:30:24Z

@dnephin, @mkeeler I did the changes to rely on the addr field being empty instead of incrementing the protocolVersion to 4. Let me know what you think?

dnephin

Looking good! I definitely think this works better.

A couple comments about why we prefer the new field, and to document the existing fields accordingly.

I started to read over the tests a bit. I must admit some of the comments in the tests (which I think predate this PR) seem either misleading or confusing. I suspect I won't be able to review the test changes properly without spending a lot more time reading over Raft source first, but I trust Matt has done that review.

testing.go

replication.go

dnephin · 2021-11-30T21:12:32Z

fuzzy/verifier.go

+	if len(req.RPCHeader.Addr) > 0 {
+		ldr = string(req.RPCHeader.Addr)
+	} else {
+		ldr = string(req.Leader)
+	}


Do we still need to do this? Any reason not to continue to use req.Leader? Why do we prefer the new RPCHeader.Addr ?

If we are trying to phase it out, we should document that on the Leader field.

Same question about the other two places we look at Leader or Candidate.

I think we should aim to phaseout Leader and Candidate parameters, it make more sense to infer those values from the RPCHeader now and removing those will reduce the on the wire packet size sensibly. Also if we keep them it mean we need to document/test special cases behaviour (different values, Addr empty and Candidate not...).
I will add the deprecation comment for both Candidate and Leader

raft_test.go

dhiaayachi · 2021-12-06T14:19:52Z

@dnephin I will leave this open for a day or so otherwise I will merge it. I would like to start working on bringing this changes to Consul, I have a couple of fixes blocked on this.

dnephin

LGTM

…should not have votes granted.

dhiaayachi mentioned this pull request Sep 23, 2021

Forbid removed node from being a Candidate #476

Merged

dhiaayachi force-pushed the noncluster-node-vote branch from 316c59a to 5778b92 Compare September 23, 2021 19:42

dhiaayachi changed the base branch from main to removed-node-vote September 23, 2021 19:43

dhiaayachi force-pushed the noncluster-node-vote branch from 5778b92 to 3907e87 Compare September 29, 2021 14:33

dhiaayachi changed the title ~~add test that check a removed node is not allowed to vote~~ node not part of the cluster is not allowed to vote Sep 29, 2021

Base automatically changed from removed-node-vote to main October 7, 2021 00:31

dhiaayachi requested a review from mkeeler October 7, 2021 15:25

dnephin reviewed Oct 25, 2021

View reviewed changes

commands.go Outdated Show resolved Hide resolved

transport.go Outdated Show resolved Hide resolved

mkeeler reviewed Oct 25, 2021

View reviewed changes

raft.go Outdated Show resolved Hide resolved

raft.go Outdated Show resolved Hide resolved

raft.go Outdated Show resolved Hide resolved

transport.go Outdated Show resolved Hide resolved

dhiaayachi added 18 commits October 27, 2021 23:04

add test to verify that a removed node is not able to vote

8d01054

add some comments to the tests

9ed9594

add wait loop to test

3a2e405

remove test related to removed node voting

8da6d8d

add test that check a removed node is not allowed to vote

b4e2273

add wait loop to make test more robust

47da40c

increase timeout

10c6f7c

Revert "increase timeout"

4aa895c

This reverts commit 316c59a.

add ID to RequestVoteRequest and check if the node is part of the c…

f22dae0

…luster before granting a vote

use request protocol version to ensure we have the right version

822cde2

add ID and Addr as part of RPCHeader and do not fill `Candidate…

e7a1af3

…` and `Leader` in version > 3

return LeaderID as part of the Leader() api

512d338

fix docstring

9e1e9c2

fix retro compatibility with version 3

9e751db

fix string casting

8074fe5

remove EncodeID and DecodeID from transport interface

2768524

add missing leaderID initialization

6ea0aa4

add protocol version 3 to version 4 upgrade test

ecd60c2

dhiaayachi requested review from dnephin and mkeeler October 28, 2021 15:07

mkeeler approved these changes Nov 24, 2021

View reviewed changes

raft.go Outdated Show resolved Hide resolved

raft.go Outdated Show resolved Hide resolved

raft_test.go Outdated Show resolved Hide resolved

raft_test.go Outdated Show resolved Hide resolved

raft_test.go Outdated Show resolved Hide resolved

raft_test.go Outdated Show resolved Hide resolved

dhiaayachi and others added 2 commits November 24, 2021 14:42

Apply suggestions from code review

4620c71

Co-authored-by: Matt Keeler <mkeeler@users.noreply.github.com>

Fix comment

86d22fa

Co-authored-by: Matt Keeler <mkeeler@users.noreply.github.com>

dnephin reviewed Nov 26, 2021

View reviewed changes

dhiaayachi commented Nov 26, 2021

View reviewed changes

dhiaayachi added 3 commits November 26, 2021 15:52

fix review comments

3621401

do not increment protocolVersion to 4 and rely on Addr/ID field being…

8631c97

… nil to fallback on the old fields.

deprecate Leader in favor of LeaderWithID

634a602

dnephin reviewed Nov 30, 2021

View reviewed changes

dhiaayachi added 2 commits December 1, 2021 09:26

remove duplicate test and code clean up

a8af0db

add deprecation for Leader and Candidate fields

d4932d0

dhiaayachi requested a review from dnephin December 1, 2021 15:03

fix deprecation comments

86c4d96

dnephin approved these changes Dec 6, 2021

View reviewed changes

dhiaayachi added 3 commits January 5, 2022 15:57

Merge branch 'main' into noncluster-node-vote

eff27ce

check if node is voter instead if it's part of the cluster, nonVoter …

4fa2eb0

…should not have votes granted.

add deprecation notice for Leader() API.

0e724f0

dhiaayachi merged commit 38cb186 into main Mar 7, 2022

dhiaayachi deleted the noncluster-node-vote branch March 7, 2022 20:29

eculver mentioned this pull request Apr 22, 2022

update raft to v1.3.8 hashicorp/consul#12844

Merged

dhiaayachi mentioned this pull request Apr 25, 2022

Fix a breaking change introduced in #477 and released in raft v1.3.7 #500

Merged

This was referenced Sep 6, 2022

Unstable leadership when running server is demoted/removed without its participation #524

Closed

Non voter with higher term should not cause cluster instability #525

Closed

dhiaayachi mentioned this pull request Sep 14, 2022

check if server is in configuration when receiving a voteRequest #526

Merged

narvikd mentioned this pull request Feb 27, 2023

feat: Add resilience in case the cluster falls bellow 1 node narvikd/nubedb#53

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node not part of the cluster is not allowed to vote #477

node not part of the cluster is not allowed to vote #477

dhiaayachi commented Sep 23, 2021 •

edited

Loading

dhiaayachi commented Oct 1, 2021

mkeeler commented Oct 5, 2021

dhiaayachi commented Oct 5, 2021

dnephin left a comment

mkeeler left a comment

mkeeler left a comment

dnephin left a comment •

edited

Loading

dnephin left a comment

dhiaayachi left a comment

dhiaayachi commented Nov 30, 2021

dnephin left a comment •

edited

Loading

dnephin Nov 30, 2021

dhiaayachi Dec 1, 2021

dhiaayachi commented Dec 6, 2021

dnephin left a comment

node not part of the cluster is not allowed to vote #477

node not part of the cluster is not allowed to vote #477

Conversation

dhiaayachi commented Sep 23, 2021 • edited Loading

dhiaayachi commented Oct 1, 2021

mkeeler commented Oct 5, 2021

dhiaayachi commented Oct 5, 2021

dnephin left a comment

Choose a reason for hiding this comment

mkeeler left a comment

Choose a reason for hiding this comment

mkeeler left a comment

Choose a reason for hiding this comment

dnephin left a comment • edited Loading

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

dhiaayachi left a comment

Choose a reason for hiding this comment

dhiaayachi commented Nov 30, 2021

dnephin left a comment • edited Loading

Choose a reason for hiding this comment

dnephin Nov 30, 2021

Choose a reason for hiding this comment

dhiaayachi Dec 1, 2021

Choose a reason for hiding this comment

dhiaayachi commented Dec 6, 2021

dnephin left a comment

Choose a reason for hiding this comment

dhiaayachi commented Sep 23, 2021 •

edited

Loading

dnephin left a comment •

edited

Loading

dnephin left a comment •

edited

Loading