ra:transfer_leadership reduces quorum count by 1 during a leadership transfer

[adrianroe]

ra:transfer_leadership appears to reduce the quorum count by 1 during a leadership transfer. This leads to cluster failure if you ever call transfer_leader on a cluster that is one server away from being in-quorate. The easiest way to reproduce the behaviour is to run a cluster of 2 nodes and attempt to transfer the leadership, but the same behaviour occurs if you are ever on the cusp of being in-quorate (2 servers up out of 3, 3 out of 4, 3 out of 5...)

Repro: note the timeouts calling ra members after the call to transfer_leader and that once the cluster becomes operational again ra1 is still the leader...

Eshell V12.1.3  (abort with ^G)
(runner@macbookM1)1> ErlangNodes = ['ra1@macbookM1', 'ra2@macbookM1'].
[ra1@macbookM1,ra2@macbookM1]
(runner@macbookM1)2> [io:format("Attempting to communicate with node ~s, response: ~s~n", [N, net_adm:ping(N)]) || N <- ErlangNodes].
Attempting to communicate with node ra1@macbookM1, response: pong
Attempting to communicate with node ra2@macbookM1, response: pong
[ok,ok]
(runner@macbookM1)3> [rpc:call(N, ra, start, []) || N <- ErlangNodes].
[ok,ok]
(runner@macbookM1)4> ServerIds = [{quick_start, N} || N <- ErlangNodes].
[{quick_start,ra1@macbookM1},{quick_start,ra2@macbookM1}]
(runner@macbookM1)5> ClusterName = quick_start.
quick_start
(runner@macbookM1)6> Machine = {simple, fun erlang:'+'/2, 0}.
{simple,fun erlang:'+'/2,0}
(runner@macbookM1)7> {ok, ServersStarted, _ServersNotStarted} = ra:start_cluster(default, ClusterName, Machine, ServerIds).
{ok,[{quick_start,ra2@macbookM1},
     {quick_start,ra1@macbookM1}],
    []}
(runner@macbookM1)8> {ok, StateMachineResult, LeaderId} = ra:process_command(hd(ServersStarted), 5).
{ok,5,{quick_start,ra1@macbookM1}}
(runner@macbookM1)9> {ok, 12, LeaderId1} = ra:process_command(LeaderId, 7).
{ok,12,{quick_start,ra1@macbookM1}}
(runner@macbookM1)10> ra:members({quick_start,ra1@macbookM1}).                                                               {ok,[{quick_start,ra1@macbookM1},
     {quick_start,ra2@macbookM1}],
    {quick_start,ra1@macbookM1}}
(runner@macbookM1)11> ra:transfer_leadership({quick_start,ra1@macbookM1},  {quick_start,ra2@macbookM1}).
ok
(runner@macbookM1)12> ra:members({quick_start,ra1@macbookM1}).
{timeout,{quick_start,ra1@macbookM1}}
(runner@macbookM1)13> ra:members({quick_start,ra1@macbookM1}).
{timeout,{quick_start,ra1@macbookM1}}
(runner@macbookM1)14> ra:members({quick_start,ra1@macbookM1}).
{timeout,{quick_start,ra1@macbookM1}}
(runner@macbookM1)15> ra:members({quick_start,ra1@macbookM1}).
{ok,[{quick_start,ra1@macbookM1},
     {quick_start,ra2@macbookM1}],
    {quick_start,ra1@macbookM1}}
(runner@macbookM1)16>

[michaelklishin]

Leadership transfer in a group implies that there is a window of time during which (Raft) cluster availability is not a given. Running on the cusp of losing your quorum is dangerous in many ways. I can't think of a way to avoid this or similar behaviors since no events in a distributed system happen instantaneously, and Raft leader election don't even try to be by design.

[adrianroe]

Thanks for the quick response. I have to confess I guessed the behaviour was by design.

[michaelklishin]

Two nodes clusters are explicitly unsupported by Ra and systems that use it (such as RabbitMQ).

If you can provide evidence that this is an implementation per se and it would be safe to change (without deviating from the Raft design, we avoid that as much as practically possible), you are welcome to submit a PR. Otherwise this is a reasonable limitation since Raft is all about having a clear quorum or losing availability by design.

[adrianroe]

To be clear is't not a feature of a two node cluster - any cluster on the edge of availability exhibits the same behaviour (so if 2 nodes out of three are running you cannot transfer leadership between the running nodes)

[kjnilsson]

As it stands the leader doesn't participate in the pre_vote process which is why 2 node leader transfers don't complete. It probably could do this so if the transfer target member has an appropriately up-to-date log it would at least proceed to candidate state which means the old leader would have to step down. We'd need to handle pre_votes in the await_condition state and most likely keep a record of it's prior state to call the right function but it should be doable.

…

On Mon, 7 Feb 2022 at 15:54, Adrian Roe ***@***.***> wrote: To be clear is't not a feature of a two node cluster - any cluster on the edge of availability exhibits the same behaviour (so if 2 nodes out of three are running you cannot transfer leadership between the running nodes) — Reply to this email directly, view it on GitHub <#251 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJAHFEUPPDSDS7O3IKXDTTUZ7TKXANCNFSM5NXZTF4Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- *Karl Nilsson*

[adrianroe]

That sounds really exciting - and something we'd definitely lend support to if possible. With some initial guidance from yourselves we probably have the skills to assist with a PR. Let me know if you'd be interested.

[lukebakken]

@kjnilsson's comment suggests that re-opening is appropriate.

[adrianroe]

Just exploring the suggestion made by @kjnilsson, we applied the change below and all of our tests pass now :)

I don't in any way claim that it's not a lot more involved than this, but the basic principal seems promising.

diff --git a/src/ra_server.erl b/src/ra_server.erl
index 32e91db..2e6ee3a 100644
--- a/src/ra_server.erl
+++ b/src/ra_server.erl
@@ -1267,8 +1267,13 @@ handle_await_condition(Msg, #{condition := Cond} = State0) ->
         {true, State} ->
             {follower, State, [{next_event, Msg}]};
         {false, State} ->
-            % log_unhandled_msg(await_condition, Msg, State),
-            {await_condition, State, []}
+            case Msg of
+              #pre_vote_rpc{} = PreVote ->
+              process_pre_vote(follower, PreVote, State);
+            _ ->
+              log_unhandled_msg(await_condition, Msg, State),
+              {await_condition, State, []}
+            end
     end.

[kjnilsson]

Looks fine, but I think you need to pass await_condition as the state to process_pre_vote else your leader would exit the condition state before the condition had been achieved.

Would you like to prepare a PR for this? perhaps with a test for the 2 node leader transfer as well.

Cheers

[adrianroe]

With pleasure!

I have a local branch with a passing 2-node test transfer. I will run that branch with my code that exercises the "edge of quorate" case very hard for a couple of days to see if that flushes out anything else (in which case I'll write a test to cover that as well), but looking good so far.