Hi, Vlad

Thanks for the fix a lot. In general it looks good to me, but I've got few
questions. Find them below.

Best regards
Yaroslav Dynnikov


On Wed, 5 Aug 2020 at 00:04, Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
wrote:

> Before the patch a replica couldn't call vshard.storage.cfg()
> before the master is bootstrapped. That could lead to an exception
> in the middle of vshard.storage.cfg().
>
> It sounds not so bad and even looks unreachable, because replica
> bootstrap anyway is blocked in box.cfg() until a master node is
> started, otherwise the replica instance is terminated.
> But the box.cfg bootstrap != vshard bootstrap.
>
> It could be that both replica and master nodes are already
> bootstrapped and working, but vshard.storage.cfg() wasn't called
> yet. In that case vshard.storage.cfg() on the replica wouldn't
> block in box.cfg(), and would happily fail a few lines later in
> vshard.storage.cfg() at attempt to touch not existing
> box.space._bucket.
>
> That was the case for cartridge. The framework configures
> instances in its own way before vshard.storage.cfg(). Then it
> calls vshard.storage.cfg() on them in arbitrary order, and
> sometimes replicas were configured earlier than the master.
>
> The fail is fixed by simply skipping the _bucket's trigger
> installation on the replica node. Because so far it is not needed
> here for anything. The trigger's sole purpose is to increment
> bucket generation used only for DML on _bucket on the master node.
>
> Now vshard.storage.cfg() on replica is able to finish before
> master does the same. However the replica still won't be able to
> handle vshard.storage.* requests until receives vshard schema
> from master.
>
> Closes #237
> ---
> Branch:
> http://github.com/tarantool/vshard/tree/gerold103/gh-237-boot-replica-first
> Issue: https://github.com/tarantool/vshard/issues/237
>
>  test/lua_libs/storage_template.lua      |  55 +++++++++++-
>  test/reload_evolution/storage.result    |  10 +++
>  test/reload_evolution/storage.test.lua  |   8 ++
>  test/router/boot_replica_first.result   | 112 ++++++++++++++++++++++++
>  test/router/boot_replica_first.test.lua |  42 +++++++++
>  vshard/storage/init.lua                 |  18 +++-
>  vshard/storage/reload_evolution.lua     |  10 ++-
>  7 files changed, 251 insertions(+), 4 deletions(-)
>  create mode 100644 test/router/boot_replica_first.result
>  create mode 100644 test/router/boot_replica_first.test.lua
>
> diff --git a/test/lua_libs/storage_template.lua
> b/test/lua_libs/storage_template.lua
> index 29a753d..84e4180 100644
> --- a/test/lua_libs/storage_template.lua
> +++ b/test/lua_libs/storage_template.lua
> @@ -1,5 +1,7 @@
>  #!/usr/bin/env tarantool
>
> +local luri = require('uri')
> +
>  NAME = require('fio').basename(arg[0], '.lua')
>  fiber = require('fiber')
>  test_run = require('test_run').new()
> @@ -10,12 +12,63 @@ log = require('log')
>  if not cfg.shard_index then
>      cfg.shard_index = 'bucket_id'
>  end
> +instance_uuid = util.name_to_uuid[NAME]
> +
> +--
> +-- Bootstrap the instance exactly like vshard does. But don't
> +-- initialize any vshard-specific code.
> +--
> +local function boot_like_vshard()
> +    assert(type(box.cfg) == 'function')
> +    for rs_uuid, rs in pairs(cfg.sharding) do
> +        for replica_uuid, replica in pairs(rs.replicas) do
> +            if replica_uuid == instance_uuid then
> +                local box_cfg = {replication = {}}
> +                box_cfg.instance_uuid = replica_uuid
> +                box_cfg.replicaset_uuid = rs_uuid
> +                box_cfg.listen = replica.uri
> +                box_cfg.read_only = not replica.master
> +                box_cfg.replication_connect_quorum = 0
> +                box_cfg.replication_timeout = 0.1
> +                for _, replica in pairs(rs.replicas) do
> +                    table.insert(box_cfg.replication, replica.uri)
> +                end
> +                box.cfg(box_cfg)
> +                if not replica.master then
> +                    return
> +                end
> +                local uri = luri.parse(replica.uri)
> +                box.schema.user.create(uri.login, {
> +                    password = uri.password, if_not_exists = true,
> +                })
> +                box.schema.user.grant(uri.login, 'super')
> +                return
> +            end
> +        end
> +    end
> +    assert(false)
>

Should there be some meaningful message?


> +end
> +
> +local omit_cfg = false
> +local i = 1
> +while arg[i] ~= nil do
> +    local key = arg[i]
> +    i = i + 1
> +    if key == 'boot_before_cfg' then
> +        boot_like_vshard()
> +        omit_cfg = true
> +    end
> +end
>
>  vshard = require('vshard')
>  echo_count = 0
>  cfg.replication_connect_timeout = 3
>  cfg.replication_timeout = 0.1
> -vshard.storage.cfg(cfg, util.name_to_uuid[NAME])
> +
> +if not omit_cfg then
> +    vshard.storage.cfg(cfg, instance_uuid)
> +end
> +
>  function bootstrap_storage(engine)
>      box.once("testapp:schema:1", function()
>          if rawget(_G, 'CHANGE_SPACE_IDS') then
> diff --git a/test/reload_evolution/storage.result
> b/test/reload_evolution/storage.result
> index ebf4fdc..4652c4f 100644
> --- a/test/reload_evolution/storage.result
> +++ b/test/reload_evolution/storage.result
> @@ -92,6 +92,16 @@ test_run:grep_log('storage_2_a',
> 'vshard.storage.reload_evolution: upgraded to')
>  ...
>  vshard.storage.internal.reload_version
>  ---
> +- 2
> +...
> +--
> +-- gh-237: should be only one trigger. During gh-237 the trigger
> installation
> +-- became conditional and therefore required to remember the current
> trigger
> +-- somewhere. When reloaded from the old version, the trigger needed to be
> +-- fetched from _bucket:on_replace().
> +--
> +#box.space._bucket:on_replace()
> +---
>  - 1
>  ...
>  -- Make sure storage operates well.
> diff --git a/test/reload_evolution/storage.test.lua
> b/test/reload_evolution/storage.test.lua
> index 56c1693..06f7117 100644
> --- a/test/reload_evolution/storage.test.lua
> +++ b/test/reload_evolution/storage.test.lua
> @@ -37,6 +37,14 @@ package.loaded['vshard.storage'] = nil
>  vshard.storage = require("vshard.storage")
>  test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution:
> upgraded to') ~= nil
>  vshard.storage.internal.reload_version
> +--
> +-- gh-237: should be only one trigger. During gh-237 the trigger
> installation
> +-- became conditional and therefore required to remember the current
> trigger
> +-- somewhere. When reloaded from the old version, the trigger needed to be
> +-- fetched from _bucket:on_replace().
> +--
> +#box.space._bucket:on_replace()
> +
>  -- Make sure storage operates well.
>  vshard.storage.bucket_force_drop(2000)
>  vshard.storage.bucket_force_create(2000)
> diff --git a/test/router/boot_replica_first.result
> b/test/router/boot_replica_first.result
> new file mode 100644
> index 0000000..1705230
> --- /dev/null
> +++ b/test/router/boot_replica_first.result
> @@ -0,0 +1,112 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +REPLICASET_1 = { 'box_1_a', 'box_1_b', 'box_1_c' }
> + | ---
> + | ...
> +test_run:create_cluster(REPLICASET_1, 'router', {args =
> 'boot_before_cfg'})
> + | ---
> + | ...
> +util = require('util')
> + | ---
> + | ...
> +util.wait_master(test_run, REPLICASET_1, 'box_1_a')
> + | ---
> + | ...
> +_ = test_run:cmd("create server router with script='router/router_2.lua'")
> + | ---
> + | ...
> +_ = test_run:cmd("start server router")
> + | ---
> + | ...
> +
> +--
> +-- gh-237: replica should be able to boot before master. Before the issue
> was
> +-- fixed, replica always tried to install a trigger on _bucket space even
> when
> +-- it was not created on a master yet - that lead to an exception in
> +-- storage.cfg. Now it should not install the trigger at all, because
> anyway it
> +-- is not needed on replica for anything.
> +--
> +
> +test_run:switch('box_1_b')
> + | ---
> + | - true
> + | ...
> +vshard.storage.cfg(cfg, instance_uuid)
> + | ---
> + | ...
> +-- _bucket is not created yet. Will fail.
> +util.check_error(vshard.storage.call, 1, 'read', 'echo', {100})
> + | ---
> + | - attempt to index field '_bucket' (a nil value)
> + | ...
> +
> +test_run:switch('default')
> + | ---
> + | - true
> + | ...
> +util.map_evals(test_run, {REPLICASET_1}, 'bootstrap_storage(\'memtx\')')
> + | ---
> + | ...
> +
> +test_run:switch('box_1_a')
> + | ---
> + | - true
> + | ...
> +vshard.storage.cfg(cfg, instance_uuid)
> + | ---
> + | ...
> +
> +test_run:switch('box_1_b')
> + | ---
> + | - true
> + | ...
> +test_run:wait_lsn('box_1_b', 'box_1_a')
> + | ---
> + | ...
> +-- Fails, but gracefully. Because the bucket is not found here.
> +vshard.storage.call(1, 'read', 'echo', {100})
> + | ---
> + | - null
> + | - bucket_id: 1
> + |   reason: Not found
> + |   code: 1
> + |   type: ShardingError
> + |   message: 'Cannot perform action with bucket 1, reason: Not found'
> + |   name: WRONG_BUCKET
> + | ...
> +-- Should not have triggers.
> +#box.space._bucket:on_replace()
> + | ---
> + | - 0
> + | ...
> +
> +test_run:switch('router')
> + | ---
> + | - true
> + | ...
> +vshard.router.bootstrap()
> + | ---
> + | - true
> + | ...
> +vshard.router.callro(1, 'echo', {100})
> + | ---
> + | - 100
> + | ...
> +
> +test_run:switch("default")
> + | ---
> + | - true
> + | ...
> +test_run:cmd('stop server router')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('delete server router')
> + | ---
> + | - true
> + | ...
> +test_run:drop_cluster(REPLICASET_1)
> + | ---
> + | ...
> diff --git a/test/router/boot_replica_first.test.lua
> b/test/router/boot_replica_first.test.lua
> new file mode 100644
> index 0000000..7b1b3fd
> --- /dev/null
> +++ b/test/router/boot_replica_first.test.lua
> @@ -0,0 +1,42 @@
> +test_run = require('test_run').new()
> +REPLICASET_1 = { 'box_1_a', 'box_1_b', 'box_1_c' }
> +test_run:create_cluster(REPLICASET_1, 'router', {args =
> 'boot_before_cfg'})
> +util = require('util')
> +util.wait_master(test_run, REPLICASET_1, 'box_1_a')
> +_ = test_run:cmd("create server router with script='router/router_2.lua'")
> +_ = test_run:cmd("start server router")
> +
> +--
> +-- gh-237: replica should be able to boot before master. Before the issue
> was
> +-- fixed, replica always tried to install a trigger on _bucket space even
> when
> +-- it was not created on a master yet - that lead to an exception in
> +-- storage.cfg. Now it should not install the trigger at all, because
> anyway it
> +-- is not needed on replica for anything.
> +--
> +
> +test_run:switch('box_1_b')
> +vshard.storage.cfg(cfg, instance_uuid)
> +-- _bucket is not created yet. Will fail.
> +util.check_error(vshard.storage.call, 1, 'read', 'echo', {100})
> +
> +test_run:switch('default')
> +util.map_evals(test_run, {REPLICASET_1}, 'bootstrap_storage(\'memtx\')')
> +
> +test_run:switch('box_1_a')
> +vshard.storage.cfg(cfg, instance_uuid)
> +
> +test_run:switch('box_1_b')
> +test_run:wait_lsn('box_1_b', 'box_1_a')
> +-- Fails, but gracefully. Because the bucket is not found here.
> +vshard.storage.call(1, 'read', 'echo', {100})
> +-- Should not have triggers.
> +#box.space._bucket:on_replace()
> +
> +test_run:switch('router')
> +vshard.router.bootstrap()
> +vshard.router.callro(1, 'echo', {100})
> +
> +test_run:switch("default")
> +test_run:cmd('stop server router')
> +test_run:cmd('delete server router')
> +test_run:drop_cluster(REPLICASET_1)
> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
> index c6a78fe..ed577f9 100644
> --- a/vshard/storage/init.lua
> +++ b/vshard/storage/init.lua
> @@ -94,6 +94,17 @@ if not M then
>          -- detect that _bucket was not changed between yields.
>          --
>          bucket_generation = 0,
> +        --
> +        -- Reference to the function used as on_replace trigger on
> +        -- _bucket space. It is used to replace the trigger with
> +        -- a new function when reload happens. It is kept
> +        -- explicitly because the old function is deleted on
> +        -- reload from the global namespace. On the other hand, it
> +        -- is still stored in _bucket:on_replace() somewhere, but
> +        -- it is not known where. The only 100% way to be able to
> +        -- replace the old function is to keep its reference.
> +        --
> +        bucket_on_replace = nil,
>
>          ------------------- Garbage collection -------------------
>          -- Fiber to remove garbage buckets data.
> @@ -2435,8 +2446,11 @@ local function storage_cfg(cfg, this_replica_uuid,
> is_reload)
>      local uri = luri.parse(this_replica.uri)
>      schema_upgrade(is_master, uri.login, uri.password)
>
> -    local old_trigger = box.space._bucket:on_replace()[1]
> -    box.space._bucket:on_replace(bucket_generation_increment, old_trigger)
> +    if is_master then
> +        local old_trigger = M.bucket_on_replace
> +        box.space._bucket:on_replace(bucket_generation_increment,
> old_trigger)
> +        M.bucket_on_replace = bucket_generation_increment
> +    end
>
>      lreplicaset.rebind_replicasets(new_replicasets, M.replicasets)
>      lreplicaset.outdate_replicasets(M.replicasets)
> diff --git a/vshard/storage/reload_evolution.lua
> b/vshard/storage/reload_evolution.lua
> index 5d09b11..f38af74 100644
> --- a/vshard/storage/reload_evolution.lua
> +++ b/vshard/storage/reload_evolution.lua
> @@ -17,6 +17,14 @@ migrations[#migrations + 1] = function(M)
>      -- Code to update Lua objects.
>  end
>
> +migrations[#migrations + 1] = function(M)
> +    local bucket = box.space._bucket
> +    if bucket then
> +        assert(M.bucket_on_replace == nil)
> +        M.bucket_on_replace = bucket:on_replace()[1]
> +    end
> +end
> +
>  --
>  -- Perform an update based on a version stored in `M` (internals).
>  -- @param M Old module internals which should be updated.
> @@ -33,7 +41,7 @@ local function upgrade(M)
>          log.error(err_msg)
>          error(err_msg)
>      end
> -    for i = start_version, #migrations  do
> +    for i = start_version + 1, #migrations do
>

Is it already tested somewhere else (migrations I mean)? This change looks
like a fix for some other problem.


>          local ok, err = pcall(migrations[i], M)
>          if ok then
>              log.info('vshard.storage.reload_evolution: upgraded to %d
> version',
> --
> 2.21.1 (Apple Git-122.3)
>
>