Re: [Tarantool-patches] [PATCH 2/4] raft: initialize raft completely at runtime

[Serge Petrenko <sergepetrenko@tarantool.org>]

08.11.2020 21:03, Vladislav Shpilevoy пишет:
> Global raft object was created mostly at compile time with
> constant values assigned to all raft fields. But it won't work
> soon because raft will be moved to a separate library, and it
> will provide methods for raft creation and destruction.
>
> This patch makes raft not rely on compile time initialization,
> and moves it entirely to raft_init(). Also there are added some
> validations via new raft_validate() function, which should ensure
> raft is not used before creation.
>
> This may be hard to support, since raft is used by recovery and
> replication, even before instance has an ID. The validation should
> help to catch bugs related to the initialization order of these
> subsystems.
>
> Part of #5303

Hi! Thanks for the patch!

LGTM.

> ---
>   src/box/raft.c | 37 ++++++++++++++++++++++---------------
>   src/box/raft.h | 14 ++++++++++++++
>   2 files changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/src/box/raft.c b/src/box/raft.c
> index 8f8b59ba6..a6a893373 100644
> --- a/src/box/raft.c
> +++ b/src/box/raft.c
> @@ -46,21 +46,11 @@
>   
>   /** Raft state of this instance. */
>   struct raft raft = {
> -	.leader = 0,
> -	.state = RAFT_STATE_FOLLOWER,
> -	.volatile_term = 1,
> -	.volatile_vote = 0,
> -	.is_enabled = false,
> -	.is_candidate = false,
> -	.is_cfg_candidate = false,
> -	.is_write_in_progress = false,
> -	.is_broadcast_scheduled = false,
> -	.term = 1,
> -	.vote = 0,
> -	.vote_mask = 0,
> -	.vote_count = 0,
> -	.worker = NULL,
> -	.election_timeout = 5,
> +	/*
> +	 * Set an invalid state to validate in all raft functions they are not
> +	 * used before raft initialization.
> +	 */
> +	.state = 0,
>   };
>   
>   /**
> @@ -325,6 +315,7 @@ raft_request_to_string(const struct raft_request *req)
>   void
>   raft_process_recovery(const struct raft_request *req)
>   {
> +	raft_validate();
>   	say_verbose("RAFT: recover %s", raft_request_to_string(req));
>   	if (req->term != 0) {
>   		raft.term = req->term;
> @@ -353,6 +344,7 @@ raft_process_recovery(const struct raft_request *req)
>   int
>   raft_process_msg(const struct raft_request *req, uint32_t source)
>   {
> +	raft_validate();
>   	say_info("RAFT: message %s from %u", raft_request_to_string(req),
>   		 source);
>   	assert(source > 0);
> @@ -496,6 +488,7 @@ raft_process_msg(const struct raft_request *req, uint32_t source)
>   void
>   raft_process_heartbeat(uint32_t source)
>   {
> +	raft_validate();
>   	/*
>   	 * Raft handles heartbeats from all instances, including anon instances
>   	 * which don't participate in Raft.
> @@ -905,6 +898,7 @@ raft_sm_stop(void)
>   void
>   raft_serialize_for_network(struct raft_request *req, struct vclock *vclock)
>   {
> +	raft_validate();
>   	memset(req, 0, sizeof(*req));
>   	/*
>   	 * Volatile state is never used for any communications.
> @@ -926,6 +920,7 @@ raft_serialize_for_network(struct raft_request *req, struct vclock *vclock)
>   void
>   raft_serialize_for_disk(struct raft_request *req)
>   {
> +	raft_validate();
>   	memset(req, 0, sizeof(*req));
>   	req->term = raft.term;
>   	req->vote = raft.vote;
> @@ -934,12 +929,14 @@ raft_serialize_for_disk(struct raft_request *req)
>   void
>   raft_on_update(struct trigger *trigger)
>   {
> +	raft_validate();
>   	trigger_add(&raft.on_update, trigger);
>   }
>   
>   void
>   raft_cfg_is_enabled(bool is_enabled)
>   {
> +	raft_validate();
>   	if (is_enabled == raft.is_enabled)
>   		return;
>   
> @@ -952,6 +949,7 @@ raft_cfg_is_enabled(bool is_enabled)
>   void
>   raft_cfg_is_candidate(bool is_candidate)
>   {
> +	raft_validate();
>   	bool old_is_candidate = raft.is_candidate;
>   	raft.is_cfg_candidate = is_candidate;
>   	raft.is_candidate = is_candidate && raft.is_enabled;
> @@ -991,6 +989,7 @@ raft_cfg_is_candidate(bool is_candidate)
>   void
>   raft_cfg_election_timeout(double timeout)
>   {
> +	raft_validate();
>   	if (timeout == raft.election_timeout)
>   		return;
>   
> @@ -1008,6 +1007,7 @@ raft_cfg_election_timeout(double timeout)
>   void
>   raft_cfg_election_quorum(void)
>   {
> +	raft_validate();
>   	if (raft.state != RAFT_STATE_CANDIDATE ||
>   	    raft.state == RAFT_STATE_LEADER)
>   		return;
> @@ -1019,6 +1019,7 @@ raft_cfg_election_quorum(void)
>   void
>   raft_cfg_death_timeout(void)
>   {
> +	raft_validate();
>   	if (raft.state == RAFT_STATE_FOLLOWER && raft.is_candidate &&
>   	    raft.leader != 0) {
>   		assert(ev_is_active(&raft.timer));
> @@ -1034,6 +1035,7 @@ raft_cfg_death_timeout(void)
>   void
>   raft_new_term(void)
>   {
> +	raft_validate();
>   	if (raft.is_enabled)
>   		raft_sm_schedule_new_term(raft.volatile_term + 1);
>   }
> @@ -1079,6 +1081,11 @@ raft_schedule_broadcast(void)
>   void
>   raft_init(void)
>   {
> +	memset(&raft, 0, sizeof(raft));
> +	raft.state = RAFT_STATE_FOLLOWER;
> +	raft.volatile_term = 1;
> +	raft.term = 1;
> +	raft.election_timeout = 5;
>   	ev_timer_init(&raft.timer, raft_sm_schedule_new_election_cb, 0, 0);
>   	rlist_create(&raft.on_update);
>   }
> diff --git a/src/box/raft.h b/src/box/raft.h
> index 8293d7410..0c60eccdf 100644
> --- a/src/box/raft.h
> +++ b/src/box/raft.h
> @@ -31,6 +31,7 @@
>    */
>   #include <stdint.h>
>   #include <stdbool.h>
> +#include <assert.h>
>   #include "tarantool_ev.h"
>   #include "trigger.h"
>   
> @@ -164,6 +165,16 @@ struct raft {
>   
>   extern struct raft raft;
>   
> +/**
> + * Ensure the raft node can be used. I.e. that it is properly initialized.
> + * Entirely for debug purposes.
> + */
> +static inline void
> +raft_validate(void)
> +{
> +	assert(raft.state != 0);
> +}
> +
>   /**
>    * A flag whether the instance is read-only according to Raft. Even if Raft
>    * allows writes though, it does not mean the instance is writable. It can be
> @@ -172,6 +183,7 @@ extern struct raft raft;
>   static inline bool
>   raft_is_ro(void)
>   {
> +	raft_validate();
>   	return raft.is_enabled && raft.state != RAFT_STATE_LEADER;
>   }
>   
> @@ -179,6 +191,7 @@ raft_is_ro(void)
>   static inline bool
>   raft_is_source_allowed(uint32_t source_id)
>   {
> +	raft_validate();
>   	return !raft.is_enabled || raft.leader == source_id;
>   }
>   
> @@ -186,6 +199,7 @@ raft_is_source_allowed(uint32_t source_id)
>   static inline bool
>   raft_is_enabled(void)
>   {
> +	raft_validate();
>   	return raft.is_enabled;
>   }
>   

-- 
Serge Petrenko