From 42631d5ba2a649b73f3773019179b4fca82ede59 Mon Sep 17 00:00:00 2001
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Tue, 1 Aug 2023 22:40:13 +0200
Subject: [PATCH] election: fix box.ctl.demote() nop in off-mode
box.ctl.demote() used not to do anything with election_mode='off'
if the synchro queue didn't belong to the caller in the same term
as the election state.
The reason could be that if the synchro queue term is "outdated",
there is no guarantee that some other instance doesn't own it in
the latest term right now.
The "problem" is that this could be workarounded easily by just
calling promote + demote together.
There isn't much sense in fixing it for the off-mode because the
only reasons off-mode exists are 1) for people who don't use
synchro at all, 2) who did use it and want to stop. Hence they
need demote just to disown the queue.
The patch "legalizes" the mentioned workaround by allowing to
perform demote in off-mode even if the synchro queue term is old.
Closes #6860
NO_DOC=bugfix
(cherry picked from commit 1afe2274d9155fc19d228237d04d1a1ddfa17270)
---
.../unreleased/gh-6860-demote-in-off-mode.md | 4 +
src/box/box.cc | 34 ++-
.../gh_6842_qsync_applier_order_test.lua | 5 +-
.../gh_6860_election_off_demote_test.lua | 196 ++++++++++++++++++
4 files changed, 231 insertions(+), 8 deletions(-)
create mode 100644 changelogs/unreleased/gh-6860-demote-in-off-mode.md
create mode 100644 test/replication-luatest/gh_6860_election_off_demote_test.lua
diff --git a/changelogs/unreleased/gh-6860-demote-in-off-mode.md b/changelogs/unreleased/gh-6860-demote-in-off-mode.md
new file mode 100644
index 0000000000..5c67543e0d
--- /dev/null
+++ b/changelogs/unreleased/gh-6860-demote-in-off-mode.md
@@ -0,0 +1,4 @@
+## bugfix/replication
+
+* Fixed a bug when `box.ctl.demote()` with `box.cfg{election_mode = 'off'}`
+ and an owned synchro queue could simply not do anything (gh-6860).
diff --git a/src/box/box.cc b/src/box/box.cc
index fbe5114ce5..189013e77a 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2747,13 +2747,33 @@ box_demote(void)
if (!is_box_configured)
return 0;
- /* Currently active leader is the only one who can issue a DEMOTE. */
- bool is_leader = txn_limbo_replica_term(&txn_limbo, instance_id) ==
- box_raft()->term && txn_limbo.owner_id == instance_id;
- if (box_election_mode != ELECTION_MODE_OFF)
- is_leader = is_leader && box_raft()->state == RAFT_STATE_LEADER;
- if (!is_leader)
- return 0;
+ const struct raft *raft = box_raft();
+ if (box_election_mode == ELECTION_MODE_OFF) {
+ assert(raft->state == RAFT_STATE_FOLLOWER);
+ if (raft->leader != REPLICA_ID_NIL) {
+ diag_set(ClientError, ER_NOT_LEADER, raft->leader);
+ return -1;
+ }
+ if (txn_limbo.owner_id == REPLICA_ID_NIL)
+ return 0;
+ /*
+ * If the limbo term is up to date with Raft, then it might have
+ * a valid owner right now. Demotion would disrupt it. In this
+ * case the user has to explicitly overthrow the old owner with
+ * local promote(), or call demote() on the actual owner.
+ */
+ if (txn_limbo.promote_greatest_term == raft->term &&
+ txn_limbo.owner_id != instance_id)
+ return 0;
+ } else {
+ if (txn_limbo_replica_term(&txn_limbo, instance_id) !=
+ raft->term)
+ return 0;
+ if (txn_limbo.owner_id != instance_id)
+ return 0;
+ if (raft->state != RAFT_STATE_LEADER)
+ return 0;
+ }
if (box_trigger_elections() != 0)
return -1;
if (box_election_mode != ELECTION_MODE_OFF)
diff --git a/test/replication-luatest/gh_6842_qsync_applier_order_test.lua b/test/replication-luatest/gh_6842_qsync_applier_order_test.lua
index b3dfa82dab..4b0299a0f4 100644
--- a/test/replication-luatest/gh_6842_qsync_applier_order_test.lua
+++ b/test/replication-luatest/gh_6842_qsync_applier_order_test.lua
@@ -52,6 +52,10 @@ g.after_each(function(g)
}
box.ctl.demote()
end)
+ -- If server-1 started demote but it is not delivered to server-2 yet, then
+ -- server-2 might start a concurrent one and fail to finish it due to term
+ -- clash. Need to wait.
+ g.server2:wait_for_vclock_of(g.server1)
g.server2:exec(function()
box.cfg{
replication_synchro_quorum = 2,
@@ -63,7 +67,6 @@ g.after_each(function(g)
end
end)
g.server1:wait_for_vclock_of(g.server2)
- g.server2:wait_for_vclock_of(g.server1)
end)
--
diff --git a/test/replication-luatest/gh_6860_election_off_demote_test.lua b/test/replication-luatest/gh_6860_election_off_demote_test.lua
new file mode 100644
index 0000000000..791ab27ef4
--- /dev/null
+++ b/test/replication-luatest/gh_6860_election_off_demote_test.lua
@@ -0,0 +1,196 @@
+local t = require('luatest')
+local server = require('luatest.server')
+local replicaset = require('luatest.replica_set')
+
+local g = t.group('gh-6860')
+
+local function replicaset_create(g)
+ g.replicaset = replicaset:new({})
+ local box_cfg = {
+ replication_timeout = 0.1,
+ replication_synchro_quorum = 2,
+ replication_synchro_timeout = 1000,
+ replication = {
+ server.build_listen_uri('server1', g.replicaset.id),
+ server.build_listen_uri('server2', g.replicaset.id),
+ },
+ }
+ g.server1 = g.replicaset:build_and_add_server({
+ alias = 'server1', box_cfg = box_cfg
+ })
+ -- For stability. To guarantee server-1 is first, server-2 is second.
+ box_cfg.read_only = true
+ g.server2 = g.replicaset:build_and_add_server({
+ alias = 'server2', box_cfg = box_cfg
+ })
+ g.replicaset:start()
+ g.server2:update_box_cfg{read_only = false}
+end
+
+local function replicaset_drop(g)
+ g.replicaset:drop()
+ g.server1 = nil
+ g.server2 = nil
+end
+
+g.before_all(replicaset_create)
+g.after_all(replicaset_drop)
+
+g.after_each(function(g)
+ local function restore()
+ box.cfg{
+ replication_synchro_quorum = 2,
+ replication_synchro_timeout = 1000,
+ election_mode = box.NULL,
+ }
+ box.ctl.demote()
+ end
+ g.server1:exec(restore)
+ -- If server-1 started demote but it is not delivered to server-2 yet, then
+ -- server-2 might start a concurrent one and fail to finish it due to term
+ -- clash. Need to wait.
+ g.server2:wait_for_vclock_of(g.server1)
+ g.server2:exec(restore)
+ g.server1:wait_for_vclock_of(g.server2)
+end)
+
+local function check_synchro_owner(server, owner_id)
+ server:exec(function(owner_id)
+ t.assert_equals(box.info.synchro.queue.owner, owner_id)
+ end, {owner_id})
+end
+
+local function check_is_ro(server, value)
+ server:exec(function(value)
+ t.assert_equals(box.info.ro, value)
+ end, {value})
+end
+
+--
+-- Demote in off-mode disowns the synchro queue if it belongs to this instance.
+-- Regardless of the queue term.
+--
+g.test_election_off_demote_self_no_leader = function(g)
+ g.server2:update_box_cfg{election_mode = 'manual'}
+ g.server1:exec(function()
+ box.cfg{election_mode = 'manual'}
+ box.ctl.promote()
+ end)
+ -- Wait the queue ownership to be persisted to check it below reliably.
+ g.server1:wait_for_synchro_queue_term(g.server1:get_election_term())
+ g.server1:exec(function()
+ box.ctl.demote()
+ box.cfg{election_mode = 'off'}
+ local info = box.info
+ -- Demote in the manual mode doesn't disown the queue. It would make no
+ -- sense because the instance won't be writable unless it is the leader
+ -- anyway.
+ t.assert_lt(info.synchro.queue.owner, info.election.term)
+ -- In off-mode the ownership is dropped. The idea is exactly to become
+ -- writable. Election state won't interfere if there is no leader.
+ box.ctl.demote()
+ end)
+ g.server2:wait_for_synchro_queue_term(g.server1:get_election_term())
+ check_synchro_owner(g.server1, 0)
+ check_is_ro(g.server1, false)
+ check_synchro_owner(g.server2, 0)
+ -- Server-2 is still in the manual mode. Hence read-only.
+ check_is_ro(g.server2, true)
+end
+
+--
+-- Demote in off-mode disowns the synchro queue even if it belongs to another
+-- instance in a term < current one. And there can't be an election leader in
+-- sight.
+--
+g.test_election_off_demote_other_no_leader = function(g)
+ g.server1:update_box_cfg{election_mode = 'manual'}
+ g.server2:update_box_cfg{election_mode = 'manual'}
+ g.server1:exec(function()
+ box.ctl.promote()
+ end)
+ -- Server-2 sees that the queue is owned by server-1.
+ g.server2:wait_for_synchro_queue_term(g.server1:get_election_term())
+ g.server1:exec(function()
+ box.ctl.demote()
+ box.cfg{election_mode = 'off'}
+ end)
+ -- Server-2 sees that server-1 is no longer a leader. But the queue still
+ -- belongs to the latter.
+ g.server2:wait_for_election_term(g.server1:get_election_term())
+ g.server2:exec(function(owner_id)
+ t.assert_equals(box.info.synchro.queue.owner, owner_id)
+ box.cfg{election_mode = 'off'}
+ box.ctl.demote()
+ end, {g.server1:get_instance_id()})
+ g.server1:wait_for_synchro_queue_term(g.server2:get_election_term())
+ check_synchro_owner(g.server1, 0)
+ check_is_ro(g.server1, false)
+ check_synchro_owner(g.server2, 0)
+ check_is_ro(g.server2, false)
+end
+
+--
+-- Demote in off-mode won't do anything if the queue is owned by another
+-- instance in the current term.
+--
+g.test_election_off_demote_other_same_term = function(g)
+ g.server1:update_box_cfg{election_mode = 'manual'}
+ g.server2:update_box_cfg{election_mode = 'manual'}
+ g.server1:exec(function()
+ box.ctl.promote()
+ end)
+ -- Server-2 sees that the queue is owned by server-1.
+ g.server2:wait_for_synchro_queue_term(g.server1:get_election_term())
+ g.server1:exec(function()
+ box.cfg{election_mode = 'off'}
+ end)
+ -- Server-2 sees that server-1 is no longer a leader. But the queue still
+ -- belongs to the latter in the current term.
+ t.helpers.retrying({}, g.server2.exec, g.server2, function()
+ if box.info.election.leader ~= 0 then
+ error('Leader did not resign')
+ end
+ end)
+ t.assert_equals(g.server2:get_election_term(),
+ g.server1:get_election_term())
+ local owner_id = g.server1:get_instance_id()
+ g.server2:exec(function(owner_id)
+ local info = box.info
+ t.assert_equals(info.synchro.queue.owner, owner_id)
+ t.assert_equals(info.synchro.queue.term, info.election.term)
+ box.cfg{election_mode = 'off'}
+ box.ctl.demote()
+ end, {owner_id})
+ check_synchro_owner(g.server1, owner_id)
+ check_is_ro(g.server1, false)
+ check_synchro_owner(g.server2, owner_id)
+ check_is_ro(g.server2, true)
+end
+
+--
+-- Demote in off-mode fails if there is an election leader in sight. Off-mode
+-- only makes sense if all the instances in the replicaset use it. If there is a
+-- leader, then apparently someone is still in non-off-mode.
+--
+g.test_election_off_demote_other_leader = function(g)
+ g.server1:update_box_cfg{election_mode = 'manual'}
+ g.server2:update_box_cfg{election_mode = 'manual'}
+ g.server1:exec(function()
+ box.ctl.promote()
+ end)
+ local election_term = g.server1:get_election_term()
+ g.server2:wait_for_synchro_queue_term(election_term)
+ g.server2:exec(function()
+ box.cfg{election_mode = 'off'}
+ t.assert_error_msg_contains('The instance is not a leader',
+ box.ctl.demote)
+ end)
+ local leader_id = g.server1:get_instance_id()
+ -- Term wasn't bumped.
+ t.assert_equals(election_term, g.server1:get_election_term())
+ check_synchro_owner(g.server1, leader_id)
+ check_is_ro(g.server1, false)
+ check_synchro_owner(g.server2, leader_id)
+ check_is_ro(g.server2, true)
+end
--
GitLab