Merge pull request #364 from mmaslankaprv/raft-config-update-fix

Raft config update fix
redpanda-data · Jan 11, 2021 · 441c8e7 · 441c8e7
2 parents c149716 + 69cee3d
commit 441c8e7
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 11 deletions.
diff --git a/src/v/raft/consensus.cc b/src/v/raft/consensus.cc
@@ -293,8 +293,15 @@ consensus::success_reply consensus::update_follower_index(
 
 void consensus::maybe_promote_to_voter(model::node_id id) {
     (void)ss::with_gate(_bg, [this, id] {
+        const auto& latest_cfg = _configuration_manager.get_latest();
+
+        // node is no longer part of current configuration, skip promotion
+        if (!latest_cfg.current_config().contains(id)) {
+            return ss::now();
+        }
+
         // is voter already
-        if (config().is_voter(id)) {
+        if (latest_cfg.is_voter(id)) {
             return ss::now();
         }
         auto it = _fstats.find(id);
@@ -493,8 +500,6 @@ bool consensus::should_skip_vote(bool ignore_heartbeat) {
     }
 
     skip_vote |= _vstate == vote_state::leader; // already a leader
-    skip_vote |= !_configuration_manager.get_latest().is_voter(
-      _self); // not a voter
 
     return skip_vote;
 }
@@ -543,6 +548,12 @@ void consensus::dispatch_vote(bool leadership_transfer) {
     // update target priority
     _target_priority = next_target_priority();
 
+    // skip sending vote request if current node is not a voter
+    if (!_configuration_manager.get_latest().is_voter(_self)) {
+        arm_vote_timeout();
+        return;
+    }
+
     // if priority is to low, skip dispatching votes, do not take priority into
     // account when we transfer leadership
     if (current_priority_to_low && !leadership_transfer) {

diff --git a/src/v/raft/tests/membership_test.cc b/src/v/raft/tests/membership_test.cc
@@ -188,8 +188,6 @@ FIXTURE_TEST(try_remove_all_voters, raft_test_fixture) {
     BOOST_REQUIRE_EQUAL(result, raft::errc::invalid_configuration_update);
 }
 
-// TODO: Fix failing test. For more details see: #342
-#if 0
 FIXTURE_TEST(replace_whole_group, raft_test_fixture) {
     raft_group gr = raft_group(raft::group_id(0), 3);
     gr.enable_all();
@@ -251,11 +249,19 @@ FIXTURE_TEST(replace_whole_group, raft_test_fixture) {
       },
       "new nodes are up to date");
 
-    info("waiting for new group leader");
-    auto new_leader_id = wait_for_group_leader(gr);
-    auto& new_leader = gr.get_member(new_leader_id);
+    wait_for(
+      5s,
+      [&gr]() {
+          info("waiting for new group leader");
+          auto new_leader_id = wait_for_group_leader(gr);
 
-    BOOST_REQUIRE_GT(new_leader_id, model::node_id(4));
-    BOOST_REQUIRE_EQUAL(new_leader.consensus->config().brokers().size(), 3);
+          return new_leader_id >= model::node_id(4);
+      },
+      "one of new nodes is a leader");
+
+    auto new_leader_id = gr.get_leader_id();
+    if (new_leader_id) {
+        auto& new_leader = gr.get_member(*new_leader_id);
+        BOOST_REQUIRE_EQUAL(new_leader.consensus->config().brokers().size(), 3);
+    }
 }
-#endif