Re: [Tarantool-patches] [PATCH v3 08/10] Support manual elections in `box.ctl.clear_synchro_queue()`

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

16.04.2021 18:38, Serge Petrenko via Tarantool-patches пишет:
>
>
> 16.04.2021 02:30, Vladislav Shpilevoy пишет:
>>> diff --git a/src/box/raft.h b/src/box/raft.h
>>> index 15f4e80d9..8fce423e1 100644
>>> --- a/src/box/raft.h
>>> +++ b/src/box/raft.h
>>> @@ -97,6 +97,9 @@ box_raft_checkpoint_remote(struct raft_request *req);
>>>   int
>>>   box_raft_process(struct raft_request *req, uint32_t source);
>>>   +void
>>> +box_raft_wait_leader_found();
>>> +
>>>   void
>>>   box_raft_init(void);
>>>   diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
>>> index e9ce8cade..7b77e05ea 100644
>>> --- a/src/lib/raft/raft.c
>>> +++ b/src/lib/raft/raft.c
>>> @@ -846,7 +846,7 @@ raft_cfg_is_enabled(struct raft *raft, bool 
>>> is_enabled)
>>>   }
>>>     void
>>> -raft_cfg_is_candidate(struct raft *raft, bool is_candidate)
>>> +raft_cfg_is_candidate(struct raft *raft, bool is_candidate, bool 
>>> demote)
>> 5. I know it might lead to some code duplication, but probably
>> better move that to other functions. For example,
>>
>>     raft_cfg_is_temporary_candidate()
>>
>> or something like that. Otherwise it appears surprisingly hard
>> to follow these 2 flags together. Although I might be wrong and
>> it would look worse. Did you try?
>>
>> Or another option:
>>
>>     raft_cfg_is_candidate(box_raft(), true, false);
>>     raft_cfg_is_candidate(box_raft(), false, false);
>>
>> turns into
>>
>>     raft_start_candidate(box_raft())
>>     raft_stop_candidate(box_raft())
>>
>> Also it would be good to have unit tests for the changes in raft.h
>> and raft.c.
>
> This variant sounds good. I'll implement in in a new commit.

The commit I was talking about:

=====================================================

commit 79940c7b20a4acefaa5984550fee2872a58fef0c
Author: Serge Petrenko <sergepetrenko@tarantool.org>
Date:   Fri Apr 16 18:22:28 2021 +0300

     raft: introduce raft_start/stop_candidate

     Extract raft_start_candidate and raft_stop_candidate functions from
     raft_cfg_is_candidate.

     These functions will be used in manual elections.

     Prerequisite #3055

diff --git a/src/lib/raft/raft.c b/src/lib/raft/raft.c
index e9ce8cade..8deb06eb5 100644
--- a/src/lib/raft/raft.c
+++ b/src/lib/raft/raft.c
@@ -848,38 +848,59 @@ raft_cfg_is_enabled(struct raft *raft, bool 
is_enabled)
  void
  raft_cfg_is_candidate(struct raft *raft, bool is_candidate)
  {
-    bool old_is_candidate = raft->is_candidate;
      raft->is_cfg_candidate = is_candidate;
-    raft->is_candidate = is_candidate && raft->is_enabled;
-    if (raft->is_candidate == old_is_candidate)
-        return;
+    is_candidate = is_candidate && raft->is_enabled;
+    if (is_candidate)
+        raft_start_candidate(raft);
+    else
+        raft_stop_candidate(raft, true);
+}

-    if (raft->is_candidate) {
-        assert(raft->state == RAFT_STATE_FOLLOWER);
-        if (raft->is_write_in_progress) {
-            /*
-             * If there is an on-going WAL write, it means there was
-             * some node who sent newer data to this node. So it is
-             * probably a better candidate. Anyway can't do anything
-             * until the new state is fully persisted.
-             */
-        } else if (raft->leader != 0) {
-            raft_sm_wait_leader_dead(raft);
-        } else {
-            raft_sm_wait_leader_found(raft);
-        }
+void
+raft_start_candidate(struct raft *raft)
+{
+    if (raft->is_candidate)
+        return;
+    raft->is_candidate = true;
+    assert(raft->state == RAFT_STATE_FOLLOWER);
+    if (raft->is_write_in_progress) {
+        /*
+         * If there is an on-going WAL write, it means there was
+         * some node who sent newer data to this node. So it is
+         * probably a better candidate. Anyway can't do anything
+         * until the new state is fully persisted.
+         */
+    } else if (raft->leader != 0) {
+        raft_sm_wait_leader_dead(raft);
      } else {
-        if (raft->state != RAFT_STATE_LEADER) {
-            /* Do not wait for anything while being a voter. */
-            raft_ev_timer_stop(raft_loop(), &raft->timer);
-        }
-        if (raft->state != RAFT_STATE_FOLLOWER) {
-            if (raft->state == RAFT_STATE_LEADER)
-                raft->leader = 0;
-            raft->state = RAFT_STATE_FOLLOWER;
-            /* State is visible and changed - broadcast. */
-            raft_schedule_broadcast(raft);
+        raft_sm_wait_leader_found(raft);
+    }
+}
+
+void
+raft_stop_candidate(struct raft *raft, bool demote)
+{
+    if (!raft->is_candidate)
+        return;
+    raft->is_candidate = false;
+    if (raft->state != RAFT_STATE_LEADER) {
+        /* Do not wait for anything while being a voter. */
+        raft_ev_timer_stop(raft_loop(), &raft->timer);
+    }
+    if (raft->state != RAFT_STATE_FOLLOWER) {
+        if (raft->state == RAFT_STATE_LEADER) {
+            if (!demote) {
+                /*
+                 * Remain leader until someone
+                 * triggers new elections.
+                 */
+                return;
+            }
+            raft->leader = 0;
          }
+        raft->state = RAFT_STATE_FOLLOWER;
+        /* State is visible and changed - broadcast. */
+        raft_schedule_broadcast(raft);
      }
  }

diff --git a/src/lib/raft/raft.h b/src/lib/raft/raft.h
index a5f7e08d9..69dec63c6 100644
--- a/src/lib/raft/raft.h
+++ b/src/lib/raft/raft.h
@@ -327,6 +327,19 @@ raft_cfg_is_enabled(struct raft *raft, bool 
is_enabled);
  void
  raft_cfg_is_candidate(struct raft *raft, bool is_candidate);

+/**
+ * Make the instance a candidate.
+ */
+void
+raft_start_candidate(struct raft *raft);
+
+/**
+ * Make the instance stop taking part in new elections.
+ * @param demote whether to stop being a leader immediately or not.
+ */
+void
+raft_stop_candidate(struct  raft *raft, bool demote);
+
  /** Configure Raft leader election timeout. */
  void
  raft_cfg_election_timeout(struct raft *raft, double timeout);
diff --git a/test/unit/raft.c b/test/unit/raft.c
index 0306cefcd..575886932 100644
--- a/test/unit/raft.c
+++ b/test/unit/raft.c
@@ -1296,15 +1296,43 @@ raft_test_term_filter(void)
      ok(!raft_is_node_outdated(&node.raft, 3), "node doesn't become "
                            "outdated");

-
      raft_node_destroy(&node);
      raft_finish_test();
  }

+static void
+raft_test_start_stop_candidate(void)
+{
+    raft_start_test(4);
+    struct raft_node node;
+    raft_node_create(&node);
+
+    raft_node_cfg_is_candidate(&node, false);
+    raft_node_cfg_election_quorum(&node, 1);
+
+    raft_start_candidate(&node.raft);
+    raft_run_next_event();
+    is(node.raft.state, RAFT_STATE_LEADER, "became leader after "
+                           "start_candidate");
+    raft_stop_candidate(&node.raft, false);
+    raft_run_for(node.cfg_death_timeout);
+    is(node.raft.state, RAFT_STATE_LEADER, "remain leader after "
+                           "stop_candidate");
+
+    is(raft_node_send_vote_request(&node,
+        3 /* Term. */,
+        "{}" /* Vclock. */,
+        2 /* Source. */
+    ), 0, "vote request from 2");
+    is(node.raft.state, RAFT_STATE_FOLLOWER, "demote once new election "
+                         "starts");
+    raft_finish_test();
+}
+
  static int
  main_f(va_list ap)
  {
-    raft_start_test(14);
+    raft_start_test(15);

      (void) ap;
      fakeev_init();
@@ -1323,6 +1351,7 @@ main_f(va_list ap)
      raft_test_enable_disable();
      raft_test_too_long_wal_write();
      raft_test_term_filter();
+    raft_test_start_stop_candidate();

      fakeev_free();

diff --git a/test/unit/raft.result b/test/unit/raft.result
index ecb962e42..bb799936b 100644
--- a/test/unit/raft.result
+++ b/test/unit/raft.result
@@ -1,5 +1,5 @@
      *** main_f ***
-1..14
+1..15
      *** raft_test_leader_election ***
      1..24
      ok 1 - 1 pending message at start
@@ -233,4 +233,12 @@ ok 13 - subtests
      ok 9 - node doesn't become outdated
  ok 14 - subtests
      *** raft_test_term_filter: done ***
+    *** raft_test_start_stop_candidate ***
+    1..4
+    ok 1 - became leader after start_candidate
+    ok 2 - remain leader after stop_candidate
+    ok 3 - vote request from 2
+    ok 4 - demote once new election starts
+ok 15 - subtests
+    *** raft_test_start_stop_candidate: done ***
      *** main_f: done ***

-- 
Serge Petrenko