Re: [Tarantool-patches] [PATCH 1/1] raft: introduce election_role configuration option

[Serge Petrenko <sergepetrenko@tarantool.org>]

06.10.2020 23:36, Vladislav Shpilevoy пишет:
> The new option can be one of 3 values: 'off', 'candidate',
> 'voter'. It replaces 2 old options: election_is_enabled and
> election_is_candidate. These flags looked strange, that it was
> possible to set candidate true, but disable election at the same
> time. Also it would not look good if we would ever decide to
> introduce another mode like a data-less sentinel node, for
> example. Just for voting.
>
> Anyway, the single option approach looks easier to configure and
> to extend.
>
> - 'off' means the election is disabled on the node. It is the same
>    as election_is_enabled = false in the old config;
>
> - 'voter' means the node can vote and is never writable. The same
>    as election_is_enabled = true + election_is_candidate = false in
>    the old config;
>
> - 'candidate' means the node is a full-featured cluster member,
>    which eventually may become a leader. The same as
>    election_is_enabled = true + election_is_candidate = true in the
>    old config.
>
> Part of #1146
Hi! Thanks for the patch!

I propose to either rename `election_role` to `election_mode` or
`election_role="off"` to `election_role="none"`

Other than that, LGTM.
> ---
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-1146-raft-options
> Issue: https://github.com/tarantool/tarantool/issues/1146
>
>   src/box/box.cc                           | 54 +++++++-----------------
>   src/box/box.h                            |  3 +-
>   src/box/lua/cfg.cc                       | 15 ++-----
>   src/box/lua/load_cfg.lua                 | 15 +++----
>   test/app-tap/init_script.result          |  3 +-
>   test/box/admin.result                    |  6 +--
>   test/box/cfg.result                      | 12 ++----
>   test/replication/election_basic.result   | 28 ++++--------
>   test/replication/election_basic.test.lua | 17 +++-----
>   test/replication/election_replica.lua    |  3 +-
>   10 files changed, 48 insertions(+), 108 deletions(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 6ec813c12..079fa8b4b 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -477,26 +477,17 @@ box_check_uri(const char *source, const char *option_name)
>   	}
>   }
>   
> -static int
> -box_check_election_is_enabled(void)
> +static const char *
> +box_check_election_role(void)
>   {
> -	int b = cfg_getb("election_is_enabled");
> -	if (b < 0) {
> -		diag_set(ClientError, ER_CFG, "election_is_enabled",
> -			 "the value must be a boolean");
> +	const char *role = cfg_gets("election_role");
> +	if (role == NULL || (strcmp(role, "off") != 0 &&
> +	    strcmp(role, "voter") != 0 && strcmp(role, "candidate") != 0)) {
> +		diag_set(ClientError, ER_CFG, "election_role", "the value must "
> +			 "be a string 'off' or 'voter' or 'candidate'");
> +		return NULL;
>   	}
> -	return b;
> -}
> -
> -static int
> -box_check_election_is_candidate(void)
> -{
> -	int b = cfg_getb("election_is_candidate");
> -	if (b < 0) {
> -		diag_set(ClientError, ER_CFG, "election_is_candidate",
> -			 "the value must be a boolean");
> -	}
> -	return b;
> +	return role;
>   }
>   
>   static double
> @@ -768,9 +759,7 @@ box_check_config(void)
>   	box_check_uri(cfg_gets("listen"), "listen");
>   	box_check_instance_uuid(&uuid);
>   	box_check_replicaset_uuid(&uuid);
> -	if (box_check_election_is_enabled() < 0)
> -		diag_raise();
> -	if (box_check_election_is_candidate() < 0)
> +	if (box_check_election_role() == NULL)
>   		diag_raise();
>   	if (box_check_election_timeout() < 0)
>   		diag_raise();
> @@ -797,22 +786,13 @@ box_check_config(void)
>   }
>   
>   int
> -box_set_election_is_enabled(void)
> +box_set_election_role(void)
>   {
> -	int b = box_check_election_is_enabled();
> -	if (b < 0)
> +	const char *role = box_check_election_role();
> +	if (role == NULL)
>   		return -1;
> -	raft_cfg_is_enabled(b);
> -	return 0;
> -}
> -
> -int
> -box_set_election_is_candidate(void)
> -{
> -	int b = box_check_election_is_candidate();
> -	if (b < 0)
> -		return -1;
> -	raft_cfg_is_candidate(b);
> +	raft_cfg_is_candidate(strcmp(role, "candidate") == 0);
> +	raft_cfg_is_enabled(strcmp(role, "off") != 0);
>   	return 0;
>   }
>   
> @@ -2786,15 +2766,13 @@ box_cfg_xc(void)
>   	 * election-enabled node without election actually enabled leading to
>   	 * disconnect.
>   	 */
> -	if (box_set_election_is_candidate() != 0)
> -		diag_raise();
>   	if (box_set_election_timeout() != 0)
>   		diag_raise();
>   	/*
>   	 * Election is enabled last. So as all the parameters are installed by
>   	 * that time.
>   	 */
> -	if (box_set_election_is_enabled() != 0)
> +	if (box_set_election_role() != 0)
>   		diag_raise();
>   
>   	title("running");
> diff --git a/src/box/box.h b/src/box/box.h
> index 45ff8bbbf..ca66ecff6 100644
> --- a/src/box/box.h
> +++ b/src/box/box.h
> @@ -245,8 +245,7 @@ void box_set_vinyl_memory(void);
>   void box_set_vinyl_max_tuple_size(void);
>   void box_set_vinyl_cache(void);
>   void box_set_vinyl_timeout(void);
> -int box_set_election_is_enabled(void);
> -int box_set_election_is_candidate(void);
> +int box_set_election_role(void);
>   int box_set_election_timeout(void);
>   void box_set_replication_timeout(void);
>   void box_set_replication_connect_timeout(void);
> diff --git a/src/box/lua/cfg.cc b/src/box/lua/cfg.cc
> index bbb92f038..94a7eea90 100644
> --- a/src/box/lua/cfg.cc
> +++ b/src/box/lua/cfg.cc
> @@ -270,17 +270,9 @@ lbox_cfg_set_worker_pool_threads(struct lua_State *L)
>   }
>   
>   static int
> -lbox_cfg_set_election_is_enabled(struct lua_State *L)
> +lbox_cfg_set_election_role(struct lua_State *L)
>   {
> -	if (box_set_election_is_enabled() != 0)
> -		luaT_error(L);
> -	return 0;
> -}
> -
> -static int
> -lbox_cfg_set_election_is_candidate(struct lua_State *L)
> -{
> -	if (box_set_election_is_candidate() != 0)
> +	if (box_set_election_role() != 0)
>   		luaT_error(L);
>   	return 0;
>   }
> @@ -406,8 +398,7 @@ box_lua_cfg_init(struct lua_State *L)
>   		{"cfg_set_vinyl_max_tuple_size", lbox_cfg_set_vinyl_max_tuple_size},
>   		{"cfg_set_vinyl_cache", lbox_cfg_set_vinyl_cache},
>   		{"cfg_set_vinyl_timeout", lbox_cfg_set_vinyl_timeout},
> -		{"cfg_set_election_is_enabled", lbox_cfg_set_election_is_enabled},
> -		{"cfg_set_election_is_candidate", lbox_cfg_set_election_is_candidate},
> +		{"cfg_set_election_role", lbox_cfg_set_election_role},
>   		{"cfg_set_election_timeout", lbox_cfg_set_election_timeout},
>   		{"cfg_set_replication_timeout", lbox_cfg_set_replication_timeout},
>   		{"cfg_set_replication_connect_quorum", lbox_cfg_set_replication_connect_quorum},
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index d558e7ac9..b2c3ce188 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -87,8 +87,7 @@ local default_cfg = {
>       checkpoint_wal_threshold = 1e18,
>       checkpoint_count    = 2,
>       worker_pool_threads = 4,
> -    election_is_enabled = false,
> -    election_is_candidate = true,
> +    election_role       = 'off',
>       election_timeout    = 5,
>       replication_timeout = 1,
>       replication_sync_lag = 10,
> @@ -168,8 +167,7 @@ local template_cfg = {
>       hot_standby         = 'boolean',
>       memtx_use_mvcc_engine = 'boolean',
>       worker_pool_threads = 'number',
> -    election_is_enabled = 'boolean',
> -    election_is_candidate = 'boolean',
> +    election_role       = 'string',
>       election_timeout    = 'number',
>       replication_timeout = 'number',
>       replication_sync_lag = 'number',
> @@ -287,8 +285,7 @@ local dynamic_cfg = {
>           require('title').update(box.cfg.custom_proc_title)
>       end,
>       force_recovery          = function() end,
> -    election_is_enabled     = private.cfg_set_election_is_enabled,
> -    election_is_candidate   = private.cfg_set_election_is_candidate,
> +    election_role           = private.cfg_set_election_role,
>       election_timeout        = private.cfg_set_election_timeout,
>       replication_timeout     = private.cfg_set_replication_timeout,
>       replication_connect_timeout = private.cfg_set_replication_connect_timeout,
> @@ -344,8 +341,7 @@ local dynamic_cfg_order = {
>       -- the new one. This should be fixed when box.cfg is able to
>       -- apply some parameters together and atomically.
>       replication_anon        = 250,
> -    election_is_enabled     = 300,
> -    election_is_candidate   = 310,
> +    election_role           = 300,
>       election_timeout        = 320,
>   }
>   
> @@ -364,8 +360,7 @@ local dynamic_cfg_skip_at_load = {
>       vinyl_cache             = true,
>       vinyl_timeout           = true,
>       too_long_threshold      = true,
> -    election_is_enabled     = true,
> -    election_is_candidate   = true,
> +    election_role           = true,
>       election_timeout        = true,
>       replication             = true,
>       replication_timeout     = true,
> diff --git a/test/app-tap/init_script.result b/test/app-tap/init_script.result
> index d8969278b..55b739bc6 100644
> --- a/test/app-tap/init_script.result
> +++ b/test/app-tap/init_script.result
> @@ -8,8 +8,7 @@ checkpoint_count:2
>   checkpoint_interval:3600
>   checkpoint_wal_threshold:1e+18
>   coredump:false
> -election_is_candidate:true
> -election_is_enabled:false
> +election_role:off
>   election_timeout:5
>   feedback_enabled:true
>   feedback_host:https://feedback.tarantool.io
> diff --git a/test/box/admin.result b/test/box/admin.result
> index 52b62356f..383ffec83 100644
> --- a/test/box/admin.result
> +++ b/test/box/admin.result
> @@ -37,10 +37,8 @@ cfg_filter(box.cfg)
>       - 1000000000000000000
>     - - coredump
>       - false
> -  - - election_is_candidate
> -    - true
> -  - - election_is_enabled
> -    - false
> +  - - election_role
> +    - off
>     - - election_timeout
>       - 5
>     - - feedback_enabled
> diff --git a/test/box/cfg.result b/test/box/cfg.result
> index f19f4bff7..10c505593 100644
> --- a/test/box/cfg.result
> +++ b/test/box/cfg.result
> @@ -25,10 +25,8 @@ cfg_filter(box.cfg)
>    |     - 1000000000000000000
>    |   - - coredump
>    |     - false
> - |   - - election_is_candidate
> - |     - true
> - |   - - election_is_enabled
> - |     - false
> + |   - - election_role
> + |     - off
>    |   - - election_timeout
>    |     - 5
>    |   - - feedback_enabled
> @@ -140,10 +138,8 @@ cfg_filter(box.cfg)
>    |     - 1000000000000000000
>    |   - - coredump
>    |     - false
> - |   - - election_is_candidate
> - |     - true
> - |   - - election_is_enabled
> - |     - false
> + |   - - election_role
> + |     - off
>    |   - - election_timeout
>    |     - 5
>    |   - - feedback_enabled
> diff --git a/test/replication/election_basic.result b/test/replication/election_basic.result
> index e59386f90..28143124b 100644
> --- a/test/replication/election_basic.result
> +++ b/test/replication/election_basic.result
> @@ -11,25 +11,19 @@ old_election_timeout = box.cfg_election_timeout
>    | ...
>   
>   -- Election is turned off by default.
> -assert(not box.cfg.election_is_enabled)
> - | ---
> - | - true
> - | ...
> --- Is candidate by default. Although it does not matter, until election is
> --- turned on.
> -assert(box.cfg.election_is_candidate)
> +assert(box.cfg.election_role == 'off')
>    | ---
>    | - true
>    | ...
>   -- Ensure election options are validated.
> -box.cfg{election_is_enabled = 100}
> +box.cfg{election_role = 100}
>    | ---
> - | - error: 'Incorrect value for option ''election_is_enabled'': should be of type boolean'
> + | - error: 'Incorrect value for option ''election_role'': should be of type string'
>    | ...
> -box.cfg{election_is_candidate = 100}
> +box.cfg{election_role = '100'}
>    | ---
> - | - error: 'Incorrect value for option ''election_is_candidate'': should be of type
> - |     boolean'
> + | - error: 'Incorrect value for option ''election_role'': the value must be a string
> + |     ''off'' or ''voter'' or ''candidate'''
>    | ...
>   box.cfg{election_timeout = -1}
>    | ---
> @@ -64,10 +58,7 @@ assert(not box.info.ro)
>    | ...
>   
>   -- Turned on election blocks writes until the instance becomes a leader.
> -box.cfg{election_is_candidate = false}
> - | ---
> - | ...
> -box.cfg{election_is_enabled = true}
> +box.cfg{election_role = 'voter'}
>    | ---
>    | ...
>   assert(box.info.election.state == 'follower')
> @@ -97,7 +88,7 @@ assert(box.info.election.leader == 0)
>   box.cfg{election_timeout = 1000}
>    | ---
>    | ...
> -box.cfg{election_is_candidate = true}
> +box.cfg{election_role = 'candidate'}
>    | ---
>    | ...
>   test_run:wait_cond(function() return box.info.election.state == 'leader' end)
> @@ -118,8 +109,7 @@ assert(box.info.election.leader == box.info.id)
>    | ...
>   
>   box.cfg{                                                                        \
> -    election_is_enabled = false,                                                \
> -    election_is_candidate = true,                                               \
> +    election_role = 'off',                                                      \
>       election_timeout = old_election_timeout                                     \
>   }
>    | ---
> diff --git a/test/replication/election_basic.test.lua b/test/replication/election_basic.test.lua
> index 506d5ec4e..81f587866 100644
> --- a/test/replication/election_basic.test.lua
> +++ b/test/replication/election_basic.test.lua
> @@ -6,13 +6,10 @@ test_run = require('test_run').new()
>   old_election_timeout = box.cfg_election_timeout
>   
>   -- Election is turned off by default.
> -assert(not box.cfg.election_is_enabled)
> --- Is candidate by default. Although it does not matter, until election is
> --- turned on.
> -assert(box.cfg.election_is_candidate)
> +assert(box.cfg.election_role == 'off')
>   -- Ensure election options are validated.
> -box.cfg{election_is_enabled = 100}
> -box.cfg{election_is_candidate = 100}
> +box.cfg{election_role = 100}
> +box.cfg{election_role = '100'}
>   box.cfg{election_timeout = -1}
>   box.cfg{election_timeout = 0}
>   
> @@ -25,8 +22,7 @@ assert(box.info.election.leader == 0)
>   assert(not box.info.ro)
>   
>   -- Turned on election blocks writes until the instance becomes a leader.
> -box.cfg{election_is_candidate = false}
> -box.cfg{election_is_enabled = true}
> +box.cfg{election_role = 'voter'}
>   assert(box.info.election.state == 'follower')
>   assert(box.info.ro)
>   -- Term is not changed, because the instance can't be a candidate,
> @@ -37,15 +33,14 @@ assert(box.info.election.leader == 0)
>   
>   -- Candidate instance votes immediately, if sees no leader.
>   box.cfg{election_timeout = 1000}
> -box.cfg{election_is_candidate = true}
> +box.cfg{election_role = 'candidate'}
>   test_run:wait_cond(function() return box.info.election.state == 'leader' end)
>   assert(box.info.election.term > term)
>   assert(box.info.election.vote == box.info.id)
>   assert(box.info.election.leader == box.info.id)
>   
>   box.cfg{                                                                        \
> -    election_is_enabled = false,                                                \
> -    election_is_candidate = true,                                               \
> +    election_role = 'off',                                                      \
>       election_timeout = old_election_timeout                                     \
>   }
>   
> diff --git a/test/replication/election_replica.lua b/test/replication/election_replica.lua
> index b7d1aebe7..96a01245b 100644
> --- a/test/replication/election_replica.lua
> +++ b/test/replication/election_replica.lua
> @@ -19,8 +19,7 @@ box.cfg({
>           instance_uri(3),
>       },
>       replication_timeout = 0.1,
> -    election_is_enabled = true,
> -    election_is_candidate = true,
> +    election_role = 'candidate',
>       election_timeout = ELECTION_TIMEOUT,
>       replication_synchro_quorum = SYNCHRO_QUORUM,
>       replication_synchro_timeout = 0.1,

-- 
Serge Petrenko