From ebe4cd9bd79b7c724f523935fe80d7bc331594fa Mon Sep 17 00:00:00 2001
From: Astronomax <fgfgfb93@gmail.com>
Date: Fri, 1 Dec 2023 14:04:20 +0300
Subject: [PATCH] box: fix failing assertion in box_promote_qsync
Fixed a bug when the assertion in box_promote_qsync would fail.
The assertion is that at the moment when box_promote_qsync is
executed, no other promote is executed. It turned out that this
assertion is basically incorrect. Now after this patch the newly
elected leader is trying to repeat box_promote_qsync in
box_raft_update_synchro_queue until it fails due to the fact
that some other promotion is currently being executed.
Closes #9263
NO_DOC=bugfix
---
.../gh-9263-assertion-in-box-promote-qsync.md | 8 +
src/box/box.cc | 5 +-
src/box/errcode.h | 1 +
src/box/raft.c | 10 +-
test/box/error.result | 1 +
...63_assertion_in_box_promote_qsync_test.lua | 151 ++++++++++++++++++
6 files changed, 174 insertions(+), 2 deletions(-)
create mode 100644 changelogs/unreleased/gh-9263-assertion-in-box-promote-qsync.md
create mode 100644 test/replication-luatest/gh_9263_assertion_in_box_promote_qsync_test.lua
diff --git a/changelogs/unreleased/gh-9263-assertion-in-box-promote-qsync.md b/changelogs/unreleased/gh-9263-assertion-in-box-promote-qsync.md
new file mode 100644
index 0000000000..d5764e92fd
--- /dev/null
+++ b/changelogs/unreleased/gh-9263-assertion-in-box-promote-qsync.md
@@ -0,0 +1,8 @@
+## bugfix/core
+
+* Fixed a bug when the assertion in `box_promote_qsync` would fail in the
+ debug build mode. The assertion is that at the moment when `box_promote_qsync`
+ is called, no other promote is being executed. It turned out that this
+ assertion is basically incorrect. In the release build mode, this incorrect
+ assumption could potentially lead to writing 2 PROMOTE entries in the same
+ term (gh-9263).
diff --git a/src/box/box.cc b/src/box/box.cc
index b1b2812a8b..74ddaef65b 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2901,7 +2901,10 @@ box_issue_demote(int64_t promote_lsn)
int
box_promote_qsync(void)
{
- assert(!is_in_box_promote);
+ if (is_in_box_promote) {
+ diag_set(ClientError, ER_IN_ANOTHER_PROMOTE);
+ return -1;
+ }
assert(is_box_configured);
struct raft *raft = box_raft();
is_in_box_promote = true;
diff --git a/src/box/errcode.h b/src/box/errcode.h
index b4489691b5..775bddd1e1 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -330,6 +330,7 @@ struct errcode_record {
/*275 */_(ER_CREATE_DEFAULT_FUNC, "Failed to create field default function '%s': %s") \
/*276 */_(ER_DEFAULT_FUNC_FAILED, "Error calling field default function '%s': %s") \
/*277 */_(ER_INVALID_DEC, "Invalid decimal: '%s'") \
+ /*278 */_(ER_IN_ANOTHER_PROMOTE, "box.ctl.promote() is already running") \
/*
* !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/raft.c b/src/box/raft.c
index 2f22b74668..2becd6578d 100644
--- a/src/box/raft.c
+++ b/src/box/raft.c
@@ -123,14 +123,22 @@ box_raft_update_synchro_queue(struct raft *raft)
return;
int rc = 0;
uint32_t errcode = 0;
+ bool try_again;
do {
+ try_again = false;
rc = box_promote_qsync();
if (rc != 0) {
struct error *err = diag_last_error(diag_get());
errcode = box_error_code(err);
diag_log();
+ if (!fiber_is_cancelled() &&
+ (errcode == ER_QUORUM_WAIT ||
+ errcode == ER_IN_ANOTHER_PROMOTE)) {
+ try_again = true;
+ fiber_sleep(0);
+ }
}
- } while (rc != 0 && errcode == ER_QUORUM_WAIT && !fiber_is_cancelled());
+ } while (try_again);
}
static int
diff --git a/test/box/error.result b/test/box/error.result
index c39f3adcc7..40a4102012 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -496,6 +496,7 @@ t;
| 275: box.error.CREATE_DEFAULT_FUNC
| 276: box.error.DEFAULT_FUNC_FAILED
| 277: box.error.INVALID_DEC
+ | 278: box.error.IN_ANOTHER_PROMOTE
| ...
test_run:cmd("setopt delimiter ''");
diff --git a/test/replication-luatest/gh_9263_assertion_in_box_promote_qsync_test.lua b/test/replication-luatest/gh_9263_assertion_in_box_promote_qsync_test.lua
new file mode 100644
index 0000000000..ab527ca221
--- /dev/null
+++ b/test/replication-luatest/gh_9263_assertion_in_box_promote_qsync_test.lua
@@ -0,0 +1,151 @@
+local t = require('luatest')
+local cluster = require('luatest.replica_set')
+local proxy = require('luatest.replica_proxy')
+local server = require('luatest.server')
+
+local g = t.group('assertion-in-box-promote-qsync')
+
+local wait_timeout = 10
+
+local function wait_pair_sync(server1, server2)
+ -- Without retrying it fails sometimes when vclocks are empty and both
+ -- instances are in 'connect' state instead of 'follow'.
+ t.helpers.retrying({timeout = 10}, function()
+ server1:wait_for_vclock_of(server2)
+ server2:wait_for_vclock_of(server1)
+ server1:assert_follows_upstream(server2:get_instance_id())
+ server2:assert_follows_upstream(server1:get_instance_id())
+ end)
+end
+
+local function server_wait_wal_is_blocked(server)
+ server:exec(function()
+ t.helpers.retrying({timeout = 10}, function()
+ t.assert(box.error.injection.get('ERRINJ_WAL_DELAY'))
+ end)
+ end)
+end
+
+local function server_wait_synchro_queue_len_is_equal(server, expected)
+ server:exec(function(expected)
+ t.helpers.retrying({timeout = 10}, function(expected)
+ t.assert_equals(box.info.synchro.queue.len, expected)
+ end, expected)
+ end, {expected})
+end
+
+local function get_wait_quorum_count(server)
+ return server:exec(function()
+ return box.error.injection.get('ERRINJ_WAIT_QUORUM_COUNT')
+ end)
+end
+
+local function server_wait_wait_quorum_count_ge_than(server, threshold)
+ server:exec(function(threshold, wait_timeout)
+ t.helpers.retrying({timeout = wait_timeout}, function(threshold)
+ t.assert_ge(box.error.injection.get('ERRINJ_WAIT_QUORUM_COUNT'),
+ threshold)
+ end, threshold)
+ end, {threshold, wait_timeout})
+end
+
+g.before_each(function(cg)
+ t.tarantool.skip_if_not_debug()
+
+ cg.cluster = cluster:new({})
+
+ local box_cfg = {
+ replication = {
+ server.build_listen_uri('master', cg.cluster.id),
+ server.build_listen_uri('replica_proxy'),
+ },
+ election_mode = 'candidate',
+ replication_synchro_quorum = 2,
+ replication_synchro_timeout = 100000,
+ replication_timeout = 0.1,
+ election_fencing_mode = 'off',
+ }
+ cg.master = cg.cluster:build_and_add_server({
+ alias = 'master',
+ box_cfg = box_cfg
+ })
+ box_cfg.replication = {
+ server.build_listen_uri('replica', cg.cluster.id),
+ server.build_listen_uri('master_proxy'),
+ }
+ box_cfg.election_mode = 'off'
+ cg.replica = cg.cluster:build_and_add_server({
+ alias = 'replica',
+ box_cfg = box_cfg
+ })
+ cg.master_proxy = proxy:new({
+ client_socket_path = server.build_listen_uri('master_proxy'),
+ server_socket_path = server.build_listen_uri('master', cg.cluster.id),
+ })
+ t.assert(cg.master_proxy:start({force = true}))
+ cg.replica_proxy = proxy:new({
+ client_socket_path = server.build_listen_uri('replica_proxy'),
+ server_socket_path = server.build_listen_uri('replica', cg.cluster.id),
+ })
+ t.assert(cg.replica_proxy:start({force = true}))
+ cg.cluster:start()
+ cg.master:wait_until_election_leader_found()
+ cg.replica:wait_until_election_leader_found()
+
+ cg.master:exec(function()
+ box.schema.space.create('test', {is_sync = true})
+ box.space.test:create_index('pk')
+ end)
+ wait_pair_sync(cg.replica, cg.master)
+end)
+
+g.after_each(function(cg)
+ cg.cluster:drop()
+end)
+
+g.test_is_in_box_promote = function(cg)
+ local f = cg.replica:exec(function()
+ box.error.injection.set('ERRINJ_WAL_DELAY_COUNTDOWN', 0)
+ local f = require('fiber').create(function() box.ctl.promote() end)
+ f:set_joinable(true)
+ return f:id()
+ end)
+ server_wait_wal_is_blocked(cg.replica)
+
+ cg.replica_proxy:pause()
+
+ t.helpers.retrying({timeout = 10}, function()
+ cg.master:exec(function()
+ local status = box.info.replication[2].upstream.status
+ t.assert(status ~= 'follow')
+ end)
+ end)
+
+ cg.master:exec(function()
+ require('fiber').create(function() box.space.test:insert{1} end)
+ end)
+ server_wait_synchro_queue_len_is_equal(cg.replica, 1)
+
+ local wait_quorum_count = get_wait_quorum_count(cg.replica)
+ local ff = require('fiber').create(function()
+ cg.replica:exec(function(f)
+ box.error.injection.set('ERRINJ_WAL_DELAY', false)
+ local ok, _ = require('fiber').find(f):join()
+ t.assert(ok)
+ end, {f})
+ end)
+ ff:set_joinable(true)
+ -- We need to be sure we entered the 'box_wait_quorum' call.
+ server_wait_wait_quorum_count_ge_than(cg.replica, wait_quorum_count + 1)
+ cg.replica:exec(function()
+ box.cfg{
+ election_mode = 'candidate',
+ replication_synchro_quorum = 1
+ }
+ end)
+ cg.replica:wait_for_election_state('leader')
+ cg.replica_proxy:resume()
+ local ok, err = ff:join()
+ t.assert_equals(err, nil)
+ t.assert(ok)
+end
--
GitLab