From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Vladimir Davydov Subject: [PATCH] replication: allow to change instance id during join Date: Tue, 9 Apr 2019 13:05:21 +0300 Message-Id: <3d5d53552fd48a03b1a3381fbe1a83f095d7689a.1554803888.git.vdavydov.dev@gmail.com> To: kostja@tarantool.org Cc: tarantool-patches@freelists.org List-ID: Before rebootstrapping a replica, the admin may delete it from the _cluster space on the master. If he doesn't make a checkpoint after that, rebootstrap will fail with E> ER_LOCAL_INSTANCE_ID_IS_READ_ONLY: The local instance id 2 is read-only This is sort of unexpected. Let's fix this issue by allowing replicas to change their id during join. A note about replication/misc test. The test used this error to check that a master instance doesn't crash in case a replica fails to bootstrap. However, we can simply set mismatching replicaset UUIDs to get the same effect. Closes #4107 --- https://github.com/tarantool/tarantool/issues/4106 https://github.com/tarantool/tarantool/commits/dv/gh-4107-replica-rebootstrap-fix src/box/replication.cc | 25 +++++++++++-- src/box/replication.h | 5 +++ test/replication/er_load.lua | 3 +- test/replication/misc.result | 4 +-- test/replication/misc.test.lua | 4 +-- test/replication/replica_rejoin.result | 60 ++++++++++++++++++++++++++++++++ test/replication/replica_rejoin.test.lua | 20 +++++++++++ 7 files changed, 114 insertions(+), 7 deletions(-) diff --git a/src/box/replication.cc b/src/box/replication.cc index e3ceb009..a1a2a9eb 100644 --- a/src/box/replication.cc +++ b/src/box/replication.cc @@ -115,7 +115,17 @@ replica_check_id(uint32_t replica_id) if (replica_id >= VCLOCK_MAX) tnt_raise(LoggedError, ER_REPLICA_MAX, (unsigned) replica_id); - if (replica_id == ::instance_id) + /* + * It's okay to update the instance id while it is joining to + * a cluster as long as the id is set by the time bootstrap is + * complete, which is checked in box_cfg() anyway. + * + * For example, the replica could be deleted from the _cluster + * space on the master manually before rebootstrap, in which + * case it will replay this operation during the final join + * stage. + */ + if (!replicaset.is_joining && replica_id == instance_id) tnt_raise(ClientError, ER_LOCAL_INSTANCE_ID_IS_READ_ONLY, (unsigned) replica_id); } @@ -205,7 +215,7 @@ replica_set_id(struct replica *replica, uint32_t replica_id) void replica_clear_id(struct replica *replica) { - assert(replica->id != REPLICA_ID_NIL && replica->id != instance_id); + assert(replica->id != REPLICA_ID_NIL); /* * Don't remove replicas from vclock here. * The vclock_sum() must always grow, it is a core invariant of @@ -216,6 +226,11 @@ replica_clear_id(struct replica *replica) * replication. */ replicaset.replica_by_id[replica->id] = NULL; + if (replica->id == instance_id) { + /* See replica_check_id(). */ + assert(replicaset.is_joining); + instance_id = REPLICA_ID_NIL; + } replica->id = REPLICA_ID_NIL; say_info("removed replica %s", tt_uuid_str(&replica->uuid)); @@ -385,6 +400,12 @@ replica_on_applier_state_f(struct trigger *trigger, void *event) struct replica *replica = container_of(trigger, struct replica, on_applier_state); switch (replica->applier->state) { + case APPLIER_INITIAL_JOIN: + replicaset.is_joining = true; + break; + case APPLIER_JOINED: + replicaset.is_joining = false; + break; case APPLIER_CONNECTED: if (tt_uuid_is_nil(&replica->uuid)) replica_on_applier_connect(replica); diff --git a/src/box/replication.h b/src/box/replication.h index 925af8bd..8c8a9927 100644 --- a/src/box/replication.h +++ b/src/box/replication.h @@ -192,6 +192,11 @@ struct replicaset { * of the cluster as maintained by appliers. */ struct vclock vclock; + /** + * This flag is set while the instance is bootstrapping + * from a remote master. + */ + bool is_joining; /** Applier state. */ struct { /** diff --git a/test/replication/er_load.lua b/test/replication/er_load.lua index 071fc7b0..947e6069 100644 --- a/test/replication/er_load.lua +++ b/test/replication/er_load.lua @@ -17,9 +17,10 @@ box.cfg{ instance_uri(INSTANCE_ID % 2 + 1) }, replication_timeout = 0.01, + -- Mismatching UUIDs to trigger bootstrap failure. + replicaset_uuid = tostring(require('uuid').new()), read_only = INSTANCE_ID == '2' } box.once('bootstrap', function() box.schema.user.grant('guest', 'replication') - box.space._cluster:delete(2) end) diff --git a/test/replication/misc.result b/test/replication/misc.result index ab827c50..84d4425e 100644 --- a/test/replication/misc.result +++ b/test/replication/misc.result @@ -349,8 +349,8 @@ test_run:cmd('start server er_load1 with wait=False, wait_load=False') --- - true ... --- instance er_load2 will fail with error ER_READONLY. this is ok. --- We only test here that er_load1 doesn't assert. +-- Instance er_load2 will fail with error ER_REPLICASET_UUID_MISMATCH. +-- This is OK since we only test here that er_load1 doesn't assert. test_run:cmd('start server er_load2 with wait=True, wait_load=True, crash_expected = True') --- - false diff --git a/test/replication/misc.test.lua b/test/replication/misc.test.lua index eda5310b..1b69454c 100644 --- a/test/replication/misc.test.lua +++ b/test/replication/misc.test.lua @@ -143,8 +143,8 @@ box.schema.user.revoke('guest', 'replication') test_run:cmd('create server er_load1 with script="replication/er_load1.lua"') test_run:cmd('create server er_load2 with script="replication/er_load2.lua"') test_run:cmd('start server er_load1 with wait=False, wait_load=False') --- instance er_load2 will fail with error ER_READONLY. this is ok. --- We only test here that er_load1 doesn't assert. +-- Instance er_load2 will fail with error ER_REPLICASET_UUID_MISMATCH. +-- This is OK since we only test here that er_load1 doesn't assert. test_run:cmd('start server er_load2 with wait=True, wait_load=True, crash_expected = True') test_run:cmd('stop server er_load1') -- er_load2 exits automatically. diff --git a/test/replication/replica_rejoin.result b/test/replication/replica_rejoin.result index 87d626e2..c7633281 100644 --- a/test/replication/replica_rejoin.result +++ b/test/replication/replica_rejoin.result @@ -392,3 +392,63 @@ box.space.test:drop() box.schema.user.revoke('guest', 'replication') --- ... +-- +-- gh-4107: rebootstrap fails if the replica was deleted from +-- the cluster on the master. +-- +box.schema.user.grant('guest', 'replication') +--- +... +test_run:cmd("create server replica with rpl_master=default, script='replication/replica_uuid.lua'") +--- +- true +... +start_cmd = string.format("start server replica with args='%s'", require('uuid').new()) +--- +... +box.space._cluster:get(2) == nil +--- +- true +... +test_run:cmd(start_cmd) +--- +- true +... +test_run:cmd("stop server replica") +--- +- true +... +test_run:cmd("cleanup server replica") +--- +- true +... +box.space._cluster:delete(2) ~= nil +--- +- true +... +test_run:cmd(start_cmd) +--- +- true +... +box.space._cluster:get(2) ~= nil +--- +- true +... +test_run:cmd("stop server replica") +--- +- true +... +test_run:cmd("cleanup server replica") +--- +- true +... +test_run:cmd("delete server replica") +--- +- true +... +box.schema.user.revoke('guest', 'replication') +--- +... +test_run:cleanup_cluster() +--- +... diff --git a/test/replication/replica_rejoin.test.lua b/test/replication/replica_rejoin.test.lua index 9bf43eff..ca5fc538 100644 --- a/test/replication/replica_rejoin.test.lua +++ b/test/replication/replica_rejoin.test.lua @@ -142,3 +142,23 @@ test_run:cmd("delete server replica") test_run:cleanup_cluster() box.space.test:drop() box.schema.user.revoke('guest', 'replication') + +-- +-- gh-4107: rebootstrap fails if the replica was deleted from +-- the cluster on the master. +-- +box.schema.user.grant('guest', 'replication') +test_run:cmd("create server replica with rpl_master=default, script='replication/replica_uuid.lua'") +start_cmd = string.format("start server replica with args='%s'", require('uuid').new()) +box.space._cluster:get(2) == nil +test_run:cmd(start_cmd) +test_run:cmd("stop server replica") +test_run:cmd("cleanup server replica") +box.space._cluster:delete(2) ~= nil +test_run:cmd(start_cmd) +box.space._cluster:get(2) ~= nil +test_run:cmd("stop server replica") +test_run:cmd("cleanup server replica") +test_run:cmd("delete server replica") +box.schema.user.revoke('guest', 'replication') +test_run:cleanup_cluster() -- 2.11.0