From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D5F21469719 for ; Mon, 9 Nov 2020 15:50:15 +0300 (MSK) References: <5226a32c8783879dbf1b3714730b87147767466f.1604858551.git.v.shpilevoy@tarantool.org> From: Serge Petrenko Message-ID: <98eee088-598e-6437-1f21-fb28d4ca0581@tarantool.org> Date: Mon, 9 Nov 2020 15:50:13 +0300 MIME-Version: 1.0 In-Reply-To: <5226a32c8783879dbf1b3714730b87147767466f.1604858551.git.v.shpilevoy@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH 2/4] raft: initialize raft completely at runtime List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org, gorcunov@gmail.com 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 > #include > +#include > #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