Unverified Commit d4d15e04 authored by Shaya Potter's avatar Shaya Potter Committed by GitHub
Browse files

remove REMOVE_NODE log type and include its semantics in DEMOTE (#25)

this commit changes libraft's from using the DEMOTE_NODE log entry which would then initiate a REMOVE_NODE log entry on the DEMOTE's commit, to only using a REMOVE.  

* add the removal of the active node flag on append of REMOVE and (add back if reverted)
* modify is_voting to check for active state as well

this enables simplifying appending and reverting nodes, as we shouldn't be touching voting state, (as a removing node might not be voting yet), only its active state.  and as the is_voting check also now checks for active state, simplifies a lot of code that manually tested both.

* add a test for reversion of a non voting node removal

previously, the code could not handle demoting/removing a non voting node, as demote would want to unset the voting flag (which a non voting node obviously doesn't have), and would therefore fail an assertion.

with the new code, we no longer unset the voting flag in removal, and this test make sure both the is_voting and is_active return as expected through a process of add/removal/revert
parent 0d2189c8
......@@ -62,16 +62,11 @@ typedef enum {
RAFT_LOGTYPE_ADD_NODE,
/**
* Membership change.
* Nodes become demoted when we want to remove them from the cluster.
* Nodes become demoted when we want to remove them from the cluster
* Nodes become demoted upon appending of the log message
* Demoted nodes can't take part in voting or start elections.
* Demoted nodes become inactive, as per raft_node_is_active.
*/
RAFT_LOGTYPE_DEMOTE_NODE,
/**
* Membership change.
* The node is removed from the cluster.
* This happens after the node has been demoted.
* Removing nodes is a 2 step process: first demote, then remove.
* Demoted nodes are removed from cluster when the log message is committed and applied
*/
RAFT_LOGTYPE_REMOVE_NODE,
/**
......
......@@ -129,7 +129,7 @@ void raft_node_set_voting(raft_node_t* me_, int voting)
int raft_node_is_voting(raft_node_t* me_)
{
raft_node_private_t* me = (raft_node_private_t*)me_;
return (me->flags & RAFT_NODE_VOTING) != 0;
return (me->flags & RAFT_NODE_VOTING && !(me->flags & RAFT_NODE_INACTIVE));
}
int raft_node_has_sufficient_logs(raft_node_t* me_)
......
......@@ -223,9 +223,7 @@ int raft_become_candidate(raft_server_t* me_)
{
raft_node_t* node = me->nodes[i];
if (me->node != node &&
raft_node_is_active(node) &&
raft_node_is_voting(node))
if (me->node != node && raft_node_is_voting(node))
{
raft_send_requestvote(me_, node);
}
......@@ -394,7 +392,6 @@ int raft_recv_appendentries_response(raft_server_t* me_,
{
raft_node_t* node = me->nodes[i];
if (me->node != node &&
raft_node_is_active(node) &&
raft_node_is_voting(node) &&
point <= raft_node_get_match_idx(node))
{
......@@ -909,13 +906,10 @@ int raft_apply_entry(raft_server_t* me_)
case RAFT_LOGTYPE_ADD_NONVOTING_NODE:
raft_node_set_addition_committed(node, 1);
break;
case RAFT_LOGTYPE_DEMOTE_NODE:
if (node)
raft_node_set_voting_committed(node, 0);
break;
case RAFT_LOGTYPE_REMOVE_NODE:
if (node)
if (node) {
raft_remove_node(me_, node);
}
break;
default:
break;
......@@ -1136,7 +1130,6 @@ int raft_get_nvotes_for_me(raft_server_t* me_)
for (i = 0, votes = 0; i < me->num_nodes; i++)
{
if (me->node != me->nodes[i] &&
raft_node_is_active(me->nodes[i]) &&
raft_node_is_voting(me->nodes[i]) &&
raft_node_has_vote_for_me(me->nodes[i]))
{
......@@ -1201,7 +1194,7 @@ int raft_apply_all(raft_server_t* me_)
int raft_entry_is_voting_cfg_change(raft_entry_t* ety)
{
return RAFT_LOGTYPE_ADD_NODE == ety->type ||
RAFT_LOGTYPE_DEMOTE_NODE == ety->type;
RAFT_LOGTYPE_REMOVE_NODE == ety->type;
}
int raft_entry_is_cfg_change(raft_entry_t* ety)
......@@ -1209,7 +1202,6 @@ int raft_entry_is_cfg_change(raft_entry_t* ety)
return (
RAFT_LOGTYPE_ADD_NODE == ety->type ||
RAFT_LOGTYPE_ADD_NONVOTING_NODE == ety->type ||
RAFT_LOGTYPE_DEMOTE_NODE == ety->type ||
RAFT_LOGTYPE_REMOVE_NODE == ety->type);
}
......@@ -1250,14 +1242,11 @@ void raft_handle_append_cfg_change(raft_server_t* me_, raft_entry_t* ety, raft_i
assert(raft_node_is_voting(node));
break;
case RAFT_LOGTYPE_DEMOTE_NODE:
if (node)
raft_node_set_voting(node, 0);
break;
case RAFT_LOGTYPE_REMOVE_NODE:
if (node)
if (node) {
raft_node_set_active(node, 0);
}
break;
default:
......@@ -1279,13 +1268,6 @@ void raft_handle_remove_cfg_change(raft_server_t* me_, raft_entry_t* ety, const
switch (ety->type)
{
case RAFT_LOGTYPE_DEMOTE_NODE:
{
raft_node_t* node = raft_get_node(me_, node_id);
raft_node_set_voting(node, 1);
}
break;
case RAFT_LOGTYPE_REMOVE_NODE:
{
raft_node_t* node = raft_get_node(me_, node_id);
......@@ -1602,7 +1584,7 @@ raft_msg_id_t quorum_msg_id(raft_server_t* me_)
for (i = 0; i < me->num_nodes; i++) {
raft_node_t* node = me->nodes[i];
if (!raft_node_is_active(node) || !raft_node_is_voting(node))
if (!raft_node_is_voting(node))
continue;
if (me->node == node) {
......
......@@ -3612,6 +3612,55 @@ void T_estRaft_leader_recv_requestvote_responds_with_granting_if_term_is_higher(
}
#endif
int raft_delete_entry_from_idx(raft_server_t* me_, raft_index_t idx);
void TestRaft_leader_recv_entry_add_nonvoting_node_remove_and_revert(CuTest *tc)
{
raft_cbs_t funcs = {
.applylog = __raft_applylog,
.persist_term = __raft_persist_term,
.node_has_sufficient_logs = __raft_node_has_sufficient_logs,
.log_get_node_id = __raft_log_get_node_id
};
raft_log_cbs_t log_funcs = {
.log_offer = __raft_log_offer
};
void *r = raft_new();
raft_add_node(r, NULL, 1, 1);
raft_add_node(r, NULL, 2, 0);
int has_sufficient_logs_flag = 0;
raft_set_callbacks(r, &funcs, &has_sufficient_logs_flag);
log_set_callbacks(raft_get_log(r), &log_funcs, r);
/* I'm the leader */
raft_set_state(r, RAFT_STATE_LEADER);
raft_set_current_term(r, 1);
raft_set_commit_idx(r, 0);
raft_set_last_applied_idx(r, 0);
/* Add the non-voting node */
raft_entry_t *ety = __MAKE_ENTRY(1, 1, "3");
ety->type = RAFT_LOGTYPE_ADD_NONVOTING_NODE;
msg_entry_response_t etyr;
CuAssertIntEquals(tc, 0, raft_recv_entry(r, ety, &etyr));
CuAssertIntEquals(tc, 1, raft_node_is_active(raft_get_node(r, 3)));
CuAssertIntEquals(tc, 0, raft_node_is_voting(raft_get_node(r, 3)));
/* append a removal log entry for the non-voting node we just added */
ety = __MAKE_ENTRY(1, 1, "3");
ety->type = RAFT_LOGTYPE_REMOVE_NODE;
CuAssertIntEquals(tc, 0, raft_recv_entry(r, ety, &etyr));
CuAssertIntEquals(tc, 0, raft_node_is_active(raft_get_node(r, 3)));
CuAssertIntEquals(tc, 0, raft_node_is_voting(raft_get_node(r, 3)));
/* revert the log entry for the removal we just appended */
CuAssertIntEquals(tc, 0, raft_delete_entry_from_idx(r, etyr.idx));
CuAssertIntEquals(tc, 1, raft_node_is_active(raft_get_node(r, 3)));
CuAssertIntEquals(tc, 0, raft_node_is_voting(raft_get_node(r, 3)));
}
void TestRaft_leader_recv_appendentries_response_set_has_sufficient_logs_after_voting_committed(
CuTest * tc)
{
......
......@@ -83,8 +83,6 @@ class ServerDoesNotExist(Exception):
def logtype2str(log_type):
if log_type == lib.RAFT_LOGTYPE_NORMAL:
return 'normal'
elif log_type == lib.RAFT_LOGTYPE_DEMOTE_NODE:
return 'demote'
elif log_type == lib.RAFT_LOGTYPE_REMOVE_NODE:
return 'remove'
elif log_type == lib.RAFT_LOGTYPE_ADD_NONVOTING_NODE:
......@@ -599,7 +597,7 @@ class Network(object):
return
# Create a new configuration entry to be processed by the leader
ety = self.add_entry(lib.RAFT_LOGTYPE_DEMOTE_NODE,
ety = self.add_entry(lib.RAFT_LOGTYPE_REMOVE_NODE,
ChangeRaftEntry(server.id))
assert server.connection_status == NODE_CONNECTED
assert(lib.raft_entry_is_cfg_change(ety))
......@@ -858,24 +856,6 @@ class RaftServer(object):
if ety.type == lib.RAFT_LOGTYPE_NORMAL:
self.fsm_dict[change.key] = change.val
elif ety.type == lib.RAFT_LOGTYPE_DEMOTE_NODE:
# if we are being demoted, commit suicide on demotion being applied
if change.node_id == lib.raft_get_nodeid(self.raft):
logger.info("{} shutting down because of demotion".format(self))
return lib.RAFT_ERR_SHUTDOWN
# the leader issues a removal of the node after its demotion is applied
elif lib.raft_is_leader(self.raft):
logger.info(f"submitting a remove node entry for {change.node_id} on node {self.id}")
new_ety = self.network.add_entry(
lib.RAFT_LOGTYPE_REMOVE_NODE,
change)
assert(lib.raft_entry_is_cfg_change(new_ety))
e = self.recv_entry(new_ety)
assert e == 0
else:
logger.info(f"not doing anything for demote of {change.node_id} on {self.id} as {lib.raft_get_current_leader(self.raft)} is current leader")
elif ety.type == lib.RAFT_LOGTYPE_REMOVE_NODE:
if change.node_id == lib.raft_get_nodeid(self.raft):
logger.info("{} shutting down because of removal".format(self))
......@@ -1080,8 +1060,7 @@ class RaftServer(object):
if ety.type == lib.RAFT_LOGTYPE_NO_OP:
return 0
if (ety.type == lib.RAFT_LOGTYPE_DEMOTE_NODE or
ety.type == lib.RAFT_LOGTYPE_REMOVE_NODE or
if (ety.type == lib.RAFT_LOGTYPE_REMOVE_NODE or
ety.type == lib.RAFT_LOGTYPE_ADD_NONVOTING_NODE or
ety.type == lib.RAFT_LOGTYPE_ADD_NODE):
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment