Re: [Tarantool-patches] [PATCH v2 7/7] replication: do not ignore replica vclock on register

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

27.03.2021 23:13, Serge Petrenko via Tarantool-patches пишет:
>
>
> 26.03.2021 23:50, Vladislav Shpilevoy пишет:
>> Thanks for the patch!
>>
>> See 2 comments below.
>>
>> On 24.03.2021 13:24, Serge Petrenko wrote:
>>> There was a bug in box_process_register. It decoded replica's vclock 
>>> but
>>> never used it when sending the registration stream. So the replica 
>>> might
>>> lose the data in range (replica_vclock, start_vclock).
>> 1. Could you please add a test?
>
> Yes, sure.
>
>>
>>> Follow-up #5566
>>> ---
>>>   src/box/box.cc | 14 ++++++--------
>>>   1 file changed, 6 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/box/box.cc b/src/box/box.cc
>>> index 292a54213..0bcb505a8 100644
>>> --- a/src/box/box.cc
>>> +++ b/src/box/box.cc
>>> @@ -2154,7 +2154,8 @@ box_process_register(struct ev_io *io, struct 
>>> xrow_header *header)
>>>                 "wal_mode = 'none'");
>>>       }
>>>   -    struct gc_consumer *gc = 
>>> gc_consumer_register(&replicaset.vclock,
>>> +    vclock_reset(&replica_vclock, 0, vclock_get(&replicaset.vclock, 
>>> 0));
>> 2. xrow_decode_register_xc() already returns the vclock with empty 0
>> part. Why do you need this reset?
>
> It sets vclock[0] to master's vclock[0]. But it's not needed here, 
> indeed.
> Thanks for noticing!
>
> I thought having vclock[0] = 0 would harm gc somehow, but that's not 
> true.
>

I'm returning this vclock_reset, and here's why.
It doesn't set vclock[0] to 0. It sets it to match master's vclock[0].

This is needed, because recovery tries to find xlog with vclock strictly
less or equal to the vclock provided.
If we leave vclock[0] = 0, recovery will try to open the last xlog which
had vclock[0] = 0. This log may be already deleted, which was the
reason for test failures in CI. Strangely enough, I couldn't reproduce the
issue locally. Anyway, here you go:

================================================
diff --git a/src/box/box.cc b/src/box/box.cc
index ecec8df27..4da274976 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2154,6 +2154,8 @@ box_process_register(struct ev_io *io, struct 
xrow_header *header)
                           "wal_mode = 'none'");
         }

+       /* @sa box_process_subscribe(). */
+       vclock_reset(&replica_vclock, 0, vclock_get(&replicaset.vclock, 0));
         struct gc_consumer *gc = gc_consumer_register(&replica_vclock,
                                 "replica %s", tt_uuid_str(&instance_uuid));
         if (gc == NULL)


================================================

>>
>>> +    struct gc_consumer *gc = gc_consumer_register(&replica_vclock,
>>>                   "replica %s", tt_uuid_str(&instance_uuid));
>>>       if (gc == NULL)
>>>           diag_raise();
>
> =================================
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 0bcb505a8..ecec8df27 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -2154,7 +2154,6 @@ box_process_register(struct ev_io *io, struct 
> xrow_header *header)
>                "wal_mode = 'none'");
>      }
>
> -    vclock_reset(&replica_vclock, 0, vclock_get(&replicaset.vclock, 0));
>      struct gc_consumer *gc = gc_consumer_register(&replica_vclock,
>                  "replica %s", tt_uuid_str(&instance_uuid));
>      if (gc == NULL)
> diff --git a/test/replication/anon_register_gap.result 
> b/test/replication/anon_register_gap.result
> new file mode 100644
> index 000000000..24a3548c8
> --- /dev/null
> +++ b/test/replication/anon_register_gap.result
> @@ -0,0 +1,116 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +
> +--
> +-- When master's registering an anonymous replica, it might ignore 
> the replica's
> +-- current vclock, and skip the data in range (replica_clock, 
> master_clock).
> +--
> +box.schema.user.grant('guest', 'replication')
> + | ---
> + | ...
> +_ = box.schema.space.create('test')
> + | ---
> + | ...
> +_ = box.space.test:create_index('pk')
> + | ---
> + | ...
> +
> +test_run:cmd('create server replica with rpl_master=default,\
> +              script="replication/anon1.lua"')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server replica')
> + | ---
> + | - true
> + | ...
> +
> +test_run:wait_lsn('replica', 'default')
> + | ---
> + | ...
> +box.error.injection.set('ERRINJ_RELAY_SEND_DELAY', true)
> + | ---
> + | - ok
> + | ...
> +
> +box.space.test:insert{1}
> + | ---
> + | - [1]
> + | ...
> +
> +test_run:switch('replica')
> + | ---
> + | - true
> + | ...
> +
> +test_run:wait_upstream(1, {status='disconnected'})
> + | ---
> + | - true
> + | ...
> +box.space.test:select{}
> + | ---
> + | - []
> + | ...
> +
> +fiber = require('fiber')
> + | ---
> + | ...
> +f = fiber.new(function() box.cfg{replication_anon=false} end)
> + | ---
> + | ...
> +test_run:wait_upstream(1, {status='register'})
> + | ---
> + | - true
> + | ...
> +
> +test_run:switch('default')
> + | ---
> + | - true
> + | ...
> +box.error.injection.set('ERRINJ_RELAY_SEND_DELAY', false)
> + | ---
> + | - ok
> + | ...
> +box.space.test:insert{2}
> + | ---
> + | - [2]
> + | ...
> +
> +test_run:switch('replica')
> + | ---
> + | - true
> + | ...
> +test_run:wait_lsn('replica', 'default')
> + | ---
> + | ...
> +f:status()
> + | ---
> + | - dead
> + | ...
> +box.space.test:select{}
> + | ---
> + | - - [1]
> + |   - [2]
> + | ...
> +
> +-- Cleanup
> +test_run:switch('default')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('stop server replica')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('delete server replica')
> + | ---
> + | - true
> + | ...
> +box.space.test:drop()
> + | ---
> + | ...
> +box.schema.user.revoke('guest', 'replication')
> + | ---
> + | ...
> diff --git a/test/replication/anon_register_gap.test.lua 
> b/test/replication/anon_register_gap.test.lua
> new file mode 100644
> index 000000000..c71576a23
> --- /dev/null
> +++ b/test/replication/anon_register_gap.test.lua
> @@ -0,0 +1,43 @@
> +test_run = require('test_run').new()
> +
> +--
> +-- When master's registering an anonymous replica, it might ignore 
> the replica's
> +-- current vclock, and skip the data in range (replica_clock, 
> master_clock).
> +--
> +box.schema.user.grant('guest', 'replication')
> +_ = box.schema.space.create('test')
> +_ = box.space.test:create_index('pk')
> +
> +test_run:cmd('create server replica with rpl_master=default,\
> +              script="replication/anon1.lua"')
> +test_run:cmd('start server replica')
> +
> +test_run:wait_lsn('replica', 'default')
> +box.error.injection.set('ERRINJ_RELAY_SEND_DELAY', true)
> +
> +box.space.test:insert{1}
> +
> +test_run:switch('replica')
> +
> +test_run:wait_upstream(1, {status='disconnected'})
> +box.space.test:select{}
> +
> +fiber = require('fiber')
> +f = fiber.new(function() box.cfg{replication_anon=false} end)
> +test_run:wait_upstream(1, {status='register'})
> +
> +test_run:switch('default')
> +box.error.injection.set('ERRINJ_RELAY_SEND_DELAY', false)
> +box.space.test:insert{2}
> +
> +test_run:switch('replica')
> +test_run:wait_lsn('replica', 'default')
> +f:status()
> +box.space.test:select{}
> +
> +-- Cleanup
> +test_run:switch('default')
> +test_run:cmd('stop server replica')
> +test_run:cmd('delete server replica')
> +box.space.test:drop()
> +box.schema.user.revoke('guest', 'replication')
> diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg
> index aea8b6157..aff5fda26 100644
> --- a/test/replication/suite.cfg
> +++ b/test/replication/suite.cfg
> @@ -1,5 +1,6 @@
>  {
>      "anon.test.lua": {},
> +    "anon_register_gap.test.lua": {},
>      "gh-2991-misc-asserts-on-update.test.lua": {},
>      "gh-3111-misc-rebootstrap-from-ro-master.test.lua": {},
>      "gh-3160-misc-heartbeats-on-master-changes.test.lua": {},
> diff --git a/test/replication/suite.ini b/test/replication/suite.ini
> index fc161700a..a9e44e8cf 100644
> --- a/test/replication/suite.ini
> +++ b/test/replication/suite.ini
> @@ -3,7 +3,7 @@ core = tarantool
>  script =  master.lua
>  description = tarantool/box, replication
>  disabled = consistent.test.lua
> -release_disabled = catch.test.lua errinj.test.lua gc.test.lua 
> gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua 
> qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua 
> sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua 
> gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua 
> gh-5144-qsync-dup-confirm.test.lua 
> gh-5167-qsync-rollback-snap.test.lua gh-5506-election-on-off.test.lua 
> gh-5536-wal-limit.test.lua hang_on_synchro_fail.test.lua
> +release_disabled = catch.test.lua errinj.test.lua gc.test.lua 
> gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua 
> qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua 
> sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua 
> gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua 
> gh-5144-qsync-dup-confirm.test.lua 
> gh-5167-qsync-rollback-snap.test.lua gh-5506-election-on-off.test.lua 
> gh-5536-wal-limit.test.lua hang_on_synchro_fail.test.lua 
> anon_register_gap.test.lua
>  config = suite.cfg
>  lua_libs = lua/fast_replica.lua lua/rlimit.lua
>  use_unix_sockets = True
>

-- 
Serge Petrenko