From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp31.i.mail.ru (smtp31.i.mail.ru [94.100.177.91]) (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 5EBA6469710 for ; Wed, 6 May 2020 13:08:50 +0300 (MSK) References: <20200420131115.42047-1-sergepetrenko@tarantool.org> <52ac079d-3439-a190-c418-52212a83683c@tarantool.org> From: Serge Petrenko Message-ID: <7cc455cc-185f-b4cb-17bb-13e3528c63c0@tarantool.org> Date: Wed, 6 May 2020 13:08:48 +0300 MIME-Version: 1.0 In-Reply-To: <52ac079d-3439-a190-c418-52212a83683c@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: ru 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: Vladislav Shpilevoy , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org 28.04.2020 23:55, Vladislav Shpilevoy пишет: > Hi! Thanks for the patch! Thanks for the review! > > 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. > Seems like some amount of double whitespaces here and in the patch > (I noticed people with new macbooks have them often, huh). Thanks, fixed.  This keybboard is  killing me. >> 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). >> >> 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 >> @@ -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] >> + | ... > Nit: the test is potentially flaky. The change may not manage > to propagate to replica_anon2 while you switch to it. A more > robust option would be to use test_run:wait_cond() to wait > until the space has this tuple. Added wait_cond() >> + >> +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"]]) > Isn't it dangerous? So the master thinks this replica is dead, > even though it is not, but just connected through an anon proxy? > In case it is true, and the master has a replication quorum, that > may turn him into orphan when the normal replicas are connected > via an anon replica. > > This is related to your question whether we should allow to normal > replicas replicate from anon. I don't have a strong opinion here. > I just was always thinking that anon replicas are some kind of > satellite instances which never become a master, or even a read-only > replica unless everything else is dead. Ok then, let's prohibit normal replicas to follow anonymous ones. I've  updated the test case and commit message The diff is below. diff --git a/src/box/applier.cc b/src/box/applier.cc index 96068f098..c6deeff1b 100644 --- a/src/box/applier.cc +++ b/src/box/applier.cc @@ -640,6 +640,8 @@ applier_read_tx(struct applier *applier, struct stailq *rows)                         /*                          * A safety net, this can only occur                          * if we're fed a strangely broken xlog. +                        * row->replica_id == 0, when reading +                        * heartbeats from an anonymous instance.                          */                         tnt_raise(ClientError, ER_UNKNOWN_REPLICA, int2str(row->replica_id), diff --git a/src/box/box.cc b/src/box/box.cc index 7a272900c..f9b7d5bcf 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -1813,6 +1819,15 @@ 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); +       /* +        * Do not allow non-anonymous followers for anonymous +        * instances. +        */ +       if (replication_anon && !anon) { +               tnt_raise(ClientError, ER_UNSUPPORTED, "Anonymous replica", +                         "non-anonymous followers."); +       } +         /* Check permissions */         access_check_universe_xc(PRIV_R); diff --git a/test/replication/anon.result b/test/replication/anon.result index 997112c84..8782212ca 100644 --- a/test/replication/anon.result +++ b/test/replication/anon.result @@ -330,6 +330,12 @@ test_run:cmd('delete server replica_anon')  box.schema.user.grant('guest', 'replication')   | ---   | ... +_ = box.schema.space.create('test') + | --- + | ... +_ = box.space.test:create_index('pk') + | --- + | ...  test_run:cmd([[create server replica_anon1 with rpl_master=default,\               script="replication/anon1.lua"]]) @@ -349,12 +355,7 @@ 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] @@ -366,9 +367,9 @@ test_run:cmd('switch replica_anon2')   | ---   | - true   | ... -box.space.test:select{} +test_run:wait_cond(function() return box.space.test:get{1} ~= nil  end, 10)   | --- - | - - [1] + | - true   | ...  test_run:cmd('switch default') @@ -399,14 +400,14 @@ test_run:cmd('delete server replica')   | - true   | ... --- A normal instance (already joined) can follow an anonymous +-- A normal instance (already joined) can't 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') +test_run:cmd('start server replica')   | ---   | - true   | ... @@ -429,36 +430,10 @@ test_run:cmd('set variable repl_source to "replica_anon1.listen"')  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'}) +test_run:wait_log('replica', 'ER_UNSUPPORTED: Anonymous replica does not support non.anonymous followers.', nil, 10)   | --- - | - true + | - 'ER_UNSUPPORTED: Anonymous replica does not support non-anonymous followers.'   | ... - -test_run:cmd('switch replica') - | --- - | - true - | ... -box.space.test:select{} - | --- - | - - [1] - |   - [2] - | ... -  test_run:cmd('switch default')   | ---   | - true diff --git a/test/replication/anon.test.lua b/test/replication/anon.test.lua index cdbb8c7bb..39393e24a 100644 --- a/test/replication/anon.test.lua +++ b/test/replication/anon.test.lua @@ -115,6 +115,8 @@ test_run:cmd('delete server replica_anon')  -- gh-4696. Following an anonymous replica.  --  box.schema.user.grant('guest', 'replication') +_ = box.schema.space.create('test') +_ = box.space.test:create_index('pk')  test_run:cmd([[create server replica_anon1 with rpl_master=default,\               script="replication/anon1.lua"]]) @@ -122,14 +124,13 @@ 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:wait_cond(function() return box.space.test:get{1} ~= nil  end, 10)  test_run:cmd('switch default')  test_run:cmd('stop server replica_anon2') @@ -141,27 +142,17 @@ test_run:cmd([[create server replica with rpl_master=replica_anon1,\  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 +-- A normal instance (already joined) can't 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('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:wait_log('replica', 'ER_UNSUPPORTED: Anonymous replica does not support non.anonymous followers.', nil, 10)  test_run:cmd('switch default')  -- Cleanup. -- -- Serge Petrenko