From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6455F469710 for ; Sun, 10 May 2020 22:43:17 +0300 (MSK) References: <20200420131115.42047-1-sergepetrenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: <4faaad6d-ffb6-5f3d-04fa-c87206899adb@tarantool.org> Date: Sun, 10 May 2020 21:43:13 +0200 MIME-Version: 1.0 In-Reply-To: <20200420131115.42047-1-sergepetrenko@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] replication: remove unnecessary errors on replicating from an anonymous instance List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org LGTM. On 20/04/2020 15:11, Serge Petrenko wrote: > Since the anonymous replica implementation, it was forbidden to > replicate (join/subscribe/register) from anonymous instances. > Actually, only joining and register should be banned. Anonymous replica > isn't able to register its peer in _cluster anyway. > Allow subscribing to anonymous instances and remove all the errors > mentioning unsupported replication from anonymous replica. Join and > register are covered by ER_READONLY errors, since anonymous replicas > must be read-only. > > Closes #4696 > --- > https://github.com/tarantool/tarantool/tree/sp/gh-4696-anon-replica-follow > https://github.com/tarantool/tarantool/issues/4696 > > There's one open question: should we allow 'normal' instances to follow > anonymous instances? The current patch implementation allows this. > I see one possible use for allowing normal instances to follow anonymous ones: > in case such an anonymous instance can be failed over to with its transition to > master. Then it would be better if all the cluster members followed it in > advance, to instantly start receiving changes from it when it becomes 'normal' > and read-write. > > @ChangeLog: > - Allow anonymous replicas to be followed by other ones (gh-4696). > > src/box/applier.cc | 3 +- > src/box/box.cc | 18 --- > src/box/replication.cc | 5 - > test/replication/anon.result | 229 +++++++++++++++++++++++---------- > test/replication/anon.test.lua | 88 +++++++++---- > 5 files changed, 226 insertions(+), 117 deletions(-) > > diff --git a/src/box/applier.cc b/src/box/applier.cc > index 68de3c08c..96068f098 100644 > --- a/src/box/applier.cc > +++ b/src/box/applier.cc > @@ -636,8 +636,7 @@ applier_read_tx(struct applier *applier, struct stailq *rows) > xrow_decode_error_xc(row); > > /* Replication request. */ > - if (row->replica_id == REPLICA_ID_NIL || > - row->replica_id >= VCLOCK_MAX) { > + if (row->replica_id >= VCLOCK_MAX) { > /* > * A safety net, this can only occur > * if we're fed a strangely broken xlog. > diff --git a/src/box/box.cc b/src/box/box.cc > index f4541b577..7a272900c 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1577,12 +1577,6 @@ box_process_register(struct ev_io *io, struct xrow_header *header) > if (tt_uuid_is_equal(&instance_uuid, &INSTANCE_UUID)) > tnt_raise(ClientError, ER_CONNECTION_TO_SELF); > > - /* Forbid replication from an anonymous instance. */ > - if (replication_anon) { > - tnt_raise(ClientError, ER_UNSUPPORTED, "Replication", > - "replicating from an anonymous instance."); > - } > - > access_check_universe_xc(PRIV_R); > /* We only get register requests from anonymous instances. */ > struct replica *replica = replica_by_uuid(&instance_uuid); > @@ -1711,12 +1705,6 @@ box_process_join(struct ev_io *io, struct xrow_header *header) > if (tt_uuid_is_equal(&instance_uuid, &INSTANCE_UUID)) > tnt_raise(ClientError, ER_CONNECTION_TO_SELF); > > - /* Forbid replication from an anonymous instance. */ > - if (replication_anon) { > - tnt_raise(ClientError, ER_UNSUPPORTED, "Replication", > - "replicating from an anonymous instance."); > - } > - > /* Check permissions */ > access_check_universe_xc(PRIV_R); > > @@ -1825,12 +1813,6 @@ box_process_subscribe(struct ev_io *io, struct xrow_header *header) > if (tt_uuid_is_equal(&replica_uuid, &INSTANCE_UUID)) > tnt_raise(ClientError, ER_CONNECTION_TO_SELF); > > - /* Forbid replication from an anonymous instance. */ > - if (replication_anon) { > - tnt_raise(ClientError, ER_UNSUPPORTED, "Replication", > - "replicating from an anonymous instance."); > - } > - > /* Check permissions */ > access_check_universe_xc(PRIV_R); > > diff --git a/src/box/replication.cc b/src/box/replication.cc > index 7c10fb6f2..0c6bce9f5 100644 > --- a/src/box/replication.cc > +++ b/src/box/replication.cc > @@ -904,11 +904,6 @@ replica_on_relay_stop(struct replica *replica) > replica->gc = NULL; > } else { > assert(replica->gc == NULL); > - /* > - * We do not replicate from anonymous > - * replicas. > - */ > - assert(replica->applier == NULL); > } > } > if (replica_is_orphan(replica)) { > diff --git a/test/replication/anon.result b/test/replication/anon.result > index cbbeeef09..997112c84 100644 > --- a/test/replication/anon.result > +++ b/test/replication/anon.result > @@ -157,75 +157,10 @@ test_run:cmd('switch default') > | - 1 > | ... > > --- Test that replication (even anonymous) from an anonymous > --- instance is forbidden. An anonymous replica will fetch > --- a snapshot though. > -test_run:cmd([[create server replica_anon2 with rpl_master=replica_anon,\ > - script="replication/anon2.lua"]]) > - | --- > - | - true > - | ... > -test_run:cmd('start server replica_anon2') > - | --- > - | - true > - | ... > -test_run:wait_log('replica_anon2',\ > - 'Replication does not support replicating from an anonymous instance',\ > - nil, 10) > - | --- > - | - Replication does not support replicating from an anonymous instance > - | ... > -test_run:cmd('switch replica_anon2') > - | --- > - | - true > - | ... > -a = box.info.vclock[1] > - | --- > - | ... > --- The instance did fetch a snapshot. > -a > 0 > - | --- > - | - true > - | ... > --- 0-th vclock component isn't propagated across the cluster. > -box.info.vclock[0] > - | --- > - | - null > - | ... > -test_run:cmd('switch default') > - | --- > - | - true > - | ... > -box.space.test:insert{2} > - | --- > - | - [2] > - | ... > -test_run:cmd("switch replica_anon2") > - | --- > - | - true > - | ... > --- Second replica doesn't follow master through the > --- 1st one. Replication from an anonymous instance > --- is forbidden indeed. > -box.info.vclock[1] == a or box.info.vclock[1] > - | --- > - | - true > - | ... > - > test_run:cmd('switch replica_anon') > | --- > | - true > | ... > - > -test_run:cmd('stop server replica_anon2') > - | --- > - | - true > - | ... > -test_run:cmd('delete server replica_anon2') > - | --- > - | - true > - | ... > - > -- Promote anonymous replica. > box.cfg{replication_anon=false} > | --- > @@ -388,3 +323,167 @@ test_run:cmd('delete server replica_anon') > | --- > | - true > | ... > + > +-- > +-- gh-4696. Following an anonymous replica. > +-- > +box.schema.user.grant('guest', 'replication') > + | --- > + | ... > + > +test_run:cmd([[create server replica_anon1 with rpl_master=default,\ > + script="replication/anon1.lua"]]) > + | --- > + | - true > + | ... > +test_run:cmd([[create server replica_anon2 with rpl_master=replica_anon1,\ > + script="replication/anon2.lua"]]) > + | --- > + | - true > + | ... > +test_run:cmd('start server replica_anon1') > + | --- > + | - true > + | ... > +test_run:cmd('start server replica_anon2') > + | --- > + | - true > + | ... > +_ = box.schema.space.create('test') > + | --- > + | ... > +_ = box.space.test:create_index('pk') > + | --- > + | ... > +box.space.test:insert{1} > + | --- > + | - [1] > + | ... > + > +-- Check that master's changes are propagated to replica2, > +-- following an anonymous replica1. > +test_run:cmd('switch replica_anon2') > + | --- > + | - true > + | ... > +box.space.test:select{} > + | --- > + | - - [1] > + | ... > + > +test_run:cmd('switch default') > + | --- > + | - true > + | ... > +test_run:cmd('stop server replica_anon2') > + | --- > + | - true > + | ... > +test_run:cmd('delete server replica_anon2') > + | --- > + | - true > + | ... > + > +-- Check that joining to an anonymous replica is prohibited. > +test_run:cmd([[create server replica with rpl_master=replica_anon1,\ > + script="replication/replica.lua"]]) > + | --- > + | - true > + | ... > +test_run:cmd('start server replica with crash_expected=True') > + | --- > + | - false > + | ... > +test_run:cmd('delete server replica') > + | --- > + | - true > + | ... > + > +-- A normal instance (already joined) can follow an anonymous > +-- replica. > +test_run:cmd([[create server replica with rpl_master=default,\ > + script="replication/replica.lua"]]) > + | --- > + | - true > + | ... > +test_run:cmd('start server replica') > + | --- > + | - true > + | ... > +test_run:cmd('switch replica') > + | --- > + | - true > + | ... > +test_run:wait_upstream(1, {status='follow'}) > + | --- > + | - true > + | ... > +box.info.id > + | --- > + | - 2 > + | ... > +test_run:cmd('set variable repl_source to "replica_anon1.listen"') > + | --- > + | - true > + | ... > +box.cfg{replication=repl_source} > + | --- > + | ... > + > +test_run:cmd('switch default') > + | --- > + | - true > + | ... > +box.space.test:insert{2} > + | --- > + | - [2] > + | ... > + > +test_run:cmd('switch replica_anon1') > + | --- > + | - true > + | ... > +-- Check that the new replica follows an anonymous one. > +test_run:wait_downstream(2, {status='follow'}) > + | --- > + | - true > + | ... > + > +test_run:cmd('switch replica') > + | --- > + | - true > + | ... > +box.space.test:select{} > + | --- > + | - - [1] > + | - [2] > + | ... > + > +test_run:cmd('switch default') > + | --- > + | - true > + | ... > + > +-- Cleanup. > +test_run:cmd('stop server replica') > + | --- > + | - true > + | ... > +test_run:cmd('stop server replica_anon1') > + | --- > + | - true > + | ... > +test_run:cmd('delete server replica') > + | --- > + | - true > + | ... > +test_run:cmd('delete server replica_anon1') > + | --- > + | - true > + | ... > +box.space.test:drop() > + | --- > + | ... > +box.schema.user.revoke('guest', 'replication') > + | --- > + | ... > diff --git a/test/replication/anon.test.lua b/test/replication/anon.test.lua > index 627dc5c8e..cdbb8c7bb 100644 > --- a/test/replication/anon.test.lua > +++ b/test/replication/anon.test.lua > @@ -53,34 +53,7 @@ test_run:cmd('switch default') > -- Replica isn't visible on master. > #box.info.replication > > --- Test that replication (even anonymous) from an anonymous > --- instance is forbidden. An anonymous replica will fetch > --- a snapshot though. > -test_run:cmd([[create server replica_anon2 with rpl_master=replica_anon,\ > - script="replication/anon2.lua"]]) > -test_run:cmd('start server replica_anon2') > -test_run:wait_log('replica_anon2',\ > - 'Replication does not support replicating from an anonymous instance',\ > - nil, 10) > -test_run:cmd('switch replica_anon2') > -a = box.info.vclock[1] > --- The instance did fetch a snapshot. > -a > 0 > --- 0-th vclock component isn't propagated across the cluster. > -box.info.vclock[0] > -test_run:cmd('switch default') > -box.space.test:insert{2} > -test_run:cmd("switch replica_anon2") > --- Second replica doesn't follow master through the > --- 1st one. Replication from an anonymous instance > --- is forbidden indeed. > -box.info.vclock[1] == a or box.info.vclock[1] > - > test_run:cmd('switch replica_anon') > - > -test_run:cmd('stop server replica_anon2') > -test_run:cmd('delete server replica_anon2') > - > -- Promote anonymous replica. > box.cfg{replication_anon=false} > -- Cannot switch back after becoming "normal". > @@ -137,3 +110,64 @@ box.cfg{replication_anon = false} > test_run:switch('default') > test_run:cmd('stop server replica_anon') > test_run:cmd('delete server replica_anon') > + > +-- > +-- gh-4696. Following an anonymous replica. > +-- > +box.schema.user.grant('guest', 'replication') > + > +test_run:cmd([[create server replica_anon1 with rpl_master=default,\ > + script="replication/anon1.lua"]]) > +test_run:cmd([[create server replica_anon2 with rpl_master=replica_anon1,\ > + script="replication/anon2.lua"]]) > +test_run:cmd('start server replica_anon1') > +test_run:cmd('start server replica_anon2') > +_ = box.schema.space.create('test') > +_ = box.space.test:create_index('pk') > +box.space.test:insert{1} > + > +-- Check that master's changes are propagated to replica2, > +-- following an anonymous replica1. > +test_run:cmd('switch replica_anon2') > +box.space.test:select{} > + > +test_run:cmd('switch default') > +test_run:cmd('stop server replica_anon2') > +test_run:cmd('delete server replica_anon2') > + > +-- Check that joining to an anonymous replica is prohibited. > +test_run:cmd([[create server replica with rpl_master=replica_anon1,\ > + script="replication/replica.lua"]]) > +test_run:cmd('start server replica with crash_expected=True') > +test_run:cmd('delete server replica') > + > +-- A normal instance (already joined) can follow an anonymous > +-- replica. > +test_run:cmd([[create server replica with rpl_master=default,\ > + script="replication/replica.lua"]]) > +test_run:cmd('start server replica') > +test_run:cmd('switch replica') > +test_run:wait_upstream(1, {status='follow'}) > +box.info.id > +test_run:cmd('set variable repl_source to "replica_anon1.listen"') > +box.cfg{replication=repl_source} > + > +test_run:cmd('switch default') > +box.space.test:insert{2} > + > +test_run:cmd('switch replica_anon1') > +-- Check that the new replica follows an anonymous one. > +test_run:wait_downstream(2, {status='follow'}) > + > +test_run:cmd('switch replica') > +box.space.test:select{} > + > +test_run:cmd('switch default') > + > +-- Cleanup. > +test_run:cmd('stop server replica') > +test_run:cmd('stop server replica_anon1') > +test_run:cmd('delete server replica') > +test_run:cmd('delete server replica_anon1') > +box.space.test:drop() > +box.schema.user.revoke('guest', 'replication') >