[PATCH] replication: allow to change instance id during join

[PATCH] replication: allow to change instance id during join

[Vladimir Davydov]

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

[tarantool-patches] Re: [PATCH] replication: allow to change instance id during join

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/04/09 13:09]:
> 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

OK to push.



-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] Re: [PATCH] replication: allow to change instance id during join

[Vladimir Davydov]

On Thu, Apr 11, 2019 at 01:26:50PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/04/09 13:09]:
> > 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
> 
> OK to push.

Pushed to master, 2.1, 1.10.