<div dir="ltr"><div>Thanks for the patch, LGTM.</div><div><br></div><div><div><div dir="ltr" data-smartmail="gmail_signature"><div dir="ltr"><span><div><div dir="ltr">Best regards</div><div>Yaroslav Dynnikov<br></div></div></span></div></div></div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 6 Aug 2020 at 01:15, Vladislav Shpilevoy <<a href="mailto:v.shpilevoy@tarantool.org" target="_blank">v.shpilevoy@tarantool.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Before the patch a replica couldn't call vshard.storage.cfg()<br>
before the master is bootstrapped. That could lead to an exception<br>
in the middle of vshard.storage.cfg().<br>
<br>
It sounds not so bad and even looks unreachable, because replica<br>
bootstrap anyway is blocked in box.cfg() until a master node is<br>
started, otherwise the replica instance is terminated.<br>
But the box.cfg bootstrap != vshard bootstrap.<br>
<br>
It could be that both replica and master nodes are already<br>
bootstrapped and working, but vshard.storage.cfg() wasn't called<br>
yet. In that case vshard.storage.cfg() on the replica wouldn't<br>
block in box.cfg(), and would happily fail a few lines later in<br>
vshard.storage.cfg() at attempt to touch not existing<br>
box.space._bucket.<br>
<br>
That was the case for cartridge. The framework configures<br>
instances in its own way before vshard.storage.cfg(). Then it<br>
calls vshard.storage.cfg() on them in arbitrary order, and<br>
sometimes replicas were configured earlier than the master.<br>
<br>
The fail is fixed by simply skipping the _bucket's trigger<br>
installation on the replica node. Because so far it is not needed<br>
here for anything. The trigger's sole purpose is to increment<br>
bucket generation used only for DML on _bucket on the master node.<br>
<br>
Now vshard.storage.cfg() on replica is able to finish before<br>
master does the same. However the replica still won't be able to<br>
handle vshard.storage.* requests until receives vshard schema<br>
from master.<br>
<br>
Closes #237<br>
---<br>
 test/lua_libs/storage_template.lua      |  55 +++++++++++-<br>
 test/misc/reconfigure.result            |  33 +++++++<br>
 test/misc/reconfigure.test.lua          |   9 ++<br>
 test/reload_evolution/storage.result    |  16 ++--<br>
 test/reload_evolution/storage.test.lua  |  14 +--<br>
 test/router/boot_replica_first.result   | 112 ++++++++++++++++++++++++<br>
 test/router/boot_replica_first.test.lua |  42 +++++++++<br>
 vshard/storage/init.lua                 |  21 ++++-<br>
 vshard/storage/reload_evolution.lua     |   8 ++<br>
 9 files changed, 297 insertions(+), 13 deletions(-)<br>
 create mode 100644 test/router/boot_replica_first.result<br>
 create mode 100644 test/router/boot_replica_first.test.lua<br>
<br>
diff --git a/test/lua_libs/storage_template.lua b/test/lua_libs/storage_template.lua<br>
index 29a753d..84e4180 100644<br>
--- a/test/lua_libs/storage_template.lua<br>
+++ b/test/lua_libs/storage_template.lua<br>
@@ -1,5 +1,7 @@<br>
 #!/usr/bin/env tarantool<br>
<br>
+local luri = require('uri')<br>
+<br>
 NAME = require('fio').basename(arg[0], '.lua')<br>
 fiber = require('fiber')<br>
 test_run = require('test_run').new()<br>
@@ -10,12 +12,63 @@ log = require('log')<br>
 if not cfg.shard_index then<br>
     cfg.shard_index = 'bucket_id'<br>
 end<br>
+instance_uuid = util.name_to_uuid[NAME]<br>
+<br>
+--<br>
+-- Bootstrap the instance exactly like vshard does. But don't<br>
+-- initialize any vshard-specific code.<br>
+--<br>
+local function boot_like_vshard()<br>
+    assert(type(box.cfg) == 'function')<br>
+    for rs_uuid, rs in pairs(cfg.sharding) do<br>
+        for replica_uuid, replica in pairs(rs.replicas) do<br>
+            if replica_uuid == instance_uuid then<br>
+                local box_cfg = {replication = {}}<br>
+                box_cfg.instance_uuid = replica_uuid<br>
+                box_cfg.replicaset_uuid = rs_uuid<br>
+                box_cfg.listen = replica.uri<br>
+                box_cfg.read_only = not replica.master<br>
+                box_cfg.replication_connect_quorum = 0<br>
+                box_cfg.replication_timeout = 0.1<br>
+                for _, replica in pairs(rs.replicas) do<br>
+                    table.insert(box_cfg.replication, replica.uri)<br>
+                end<br>
+                box.cfg(box_cfg)<br>
+                if not replica.master then<br>
+                    return<br>
+                end<br>
+                local uri = luri.parse(replica.uri)<br>
+                box.schema.user.create(uri.login, {<br>
+                    password = uri.password, if_not_exists = true,<br>
+                })<br>
+                box.schema.user.grant(uri.login, 'super')<br>
+                return<br>
+            end<br>
+        end<br>
+    end<br>
+    assert(false)<br>
+end<br>
+<br>
+local omit_cfg = false<br>
+local i = 1<br>
+while arg[i] ~= nil do<br>
+    local key = arg[i]<br>
+    i = i + 1<br>
+    if key == 'boot_before_cfg' then<br>
+        boot_like_vshard()<br>
+        omit_cfg = true<br>
+    end<br>
+end<br>
<br>
 vshard = require('vshard')<br>
 echo_count = 0<br>
 cfg.replication_connect_timeout = 3<br>
 cfg.replication_timeout = 0.1<br>
-vshard.storage.cfg(cfg, util.name_to_uuid[NAME])<br>
+<br>
+if not omit_cfg then<br>
+    vshard.storage.cfg(cfg, instance_uuid)<br>
+end<br>
+<br>
 function bootstrap_storage(engine)<br>
     box.once("testapp:schema:1", function()<br>
         if rawget(_G, 'CHANGE_SPACE_IDS') then<br>
diff --git a/test/misc/reconfigure.result b/test/misc/reconfigure.result<br>
index be58eca..168be5d 100644<br>
--- a/test/misc/reconfigure.result<br>
+++ b/test/misc/reconfigure.result<br>
@@ -277,6 +277,14 @@ box.cfg.replication<br>
 ---<br>
 - - <a href="http://storage:storage@127.0.0.1:3301" rel="noreferrer" target="_blank">storage:storage@127.0.0.1:3301</a><br>
 ...<br>
+box.cfg.read_only<br>
+---<br>
+- false<br>
+...<br>
+#box.space._bucket:on_replace()<br>
+---<br>
+- 1<br>
+...<br>
 _ = test_run:switch('storage_2_a')<br>
 ---<br>
 ...<br>
@@ -303,6 +311,15 @@ box.cfg.replication<br>
 - - <a href="http://storage:storage@127.0.0.1:3303" rel="noreferrer" target="_blank">storage:storage@127.0.0.1:3303</a><br>
   - <a href="http://storage:storage@127.0.0.1:3304" rel="noreferrer" target="_blank">storage:storage@127.0.0.1:3304</a><br>
 ...<br>
+box.cfg.read_only<br>
+---<br>
+- true<br>
+...<br>
+-- Should be zero on the slave node. Even though earlier the node was a master.<br>
+#box.space._bucket:on_replace()<br>
+---<br>
+- 0<br>
+...<br>
 _ = test_run:switch('storage_2_b')<br>
 ---<br>
 ...<br>
@@ -329,6 +346,14 @@ box.cfg.replication<br>
 - - <a href="http://storage:storage@127.0.0.1:3303" rel="noreferrer" target="_blank">storage:storage@127.0.0.1:3303</a><br>
   - <a href="http://storage:storage@127.0.0.1:3304" rel="noreferrer" target="_blank">storage:storage@127.0.0.1:3304</a><br>
 ...<br>
+box.cfg.read_only<br>
+---<br>
+- false<br>
+...<br>
+#box.space._bucket:on_replace()<br>
+---<br>
+- 1<br>
+...<br>
 _ = test_run:switch('storage_3_a')<br>
 ---<br>
 ...<br>
@@ -354,6 +379,14 @@ box.cfg.replication<br>
 ---<br>
 - - <a href="http://storage:storage@127.0.0.1:3306" rel="noreferrer" target="_blank">storage:storage@127.0.0.1:3306</a><br>
 ...<br>
+box.cfg.read_only<br>
+---<br>
+- false<br>
+...<br>
+#box.space._bucket:on_replace()<br>
+---<br>
+- 1<br>
+...<br>
 _ = test_run:switch('router_1')<br>
 ---<br>
 ...<br>
diff --git a/test/misc/reconfigure.test.lua b/test/misc/reconfigure.test.lua<br>
index 1b894c8..e891010 100644<br>
--- a/test/misc/reconfigure.test.lua<br>
+++ b/test/misc/reconfigure.test.lua<br>
@@ -111,6 +111,8 @@ for k,v in pairs(info.replicasets) do table.insert(uris, v.master.uri) end<br>
 table.sort(uris)<br>
 uris<br>
 box.cfg.replication<br>
+box.cfg.read_only<br>
+#box.space._bucket:on_replace()<br>
<br>
 _ = test_run:switch('storage_2_a')<br>
 info = <a href="http://vshard.storage.info" rel="noreferrer" target="_blank">vshard.storage.info</a>()<br>
@@ -119,6 +121,9 @@ for k,v in pairs(info.replicasets) do table.insert(uris, v.master.uri) end<br>
 table.sort(uris)<br>
 uris<br>
 box.cfg.replication<br>
+box.cfg.read_only<br>
+-- Should be zero on the slave node. Even though earlier the node was a master.<br>
+#box.space._bucket:on_replace()<br>
<br>
 _ = test_run:switch('storage_2_b')<br>
 info = <a href="http://vshard.storage.info" rel="noreferrer" target="_blank">vshard.storage.info</a>()<br>
@@ -127,6 +132,8 @@ for k,v in pairs(info.replicasets) do table.insert(uris, v.master.uri) end<br>
 table.sort(uris)<br>
 uris<br>
 box.cfg.replication<br>
+box.cfg.read_only<br>
+#box.space._bucket:on_replace()<br>
<br>
 _ = test_run:switch('storage_3_a')<br>
 info = <a href="http://vshard.storage.info" rel="noreferrer" target="_blank">vshard.storage.info</a>()<br>
@@ -135,6 +142,8 @@ for k,v in pairs(info.replicasets) do table.insert(uris, v.master.uri) end<br>
 table.sort(uris)<br>
 uris<br>
 box.cfg.replication<br>
+box.cfg.read_only<br>
+#box.space._bucket:on_replace()<br>
<br>
 _ = test_run:switch('router_1')<br>
 info = <a href="http://vshard.router.info" rel="noreferrer" target="_blank">vshard.router.info</a>()<br>
diff --git a/test/reload_evolution/storage.result b/test/reload_evolution/storage.result<br>
index dde0a1f..4652c4f 100644<br>
--- a/test/reload_evolution/storage.result<br>
+++ b/test/reload_evolution/storage.result<br>
@@ -86,16 +86,22 @@ package.loaded['vshard.storage'] = nil<br>
 vshard.storage = require("vshard.storage")<br>
 ---<br>
 ...<br>
--- Should be nil. Because there was a bug that reload always reapplied the last<br>
--- migration callback. When it was fixed, the last callback wasn't called twice.<br>
--- But since the callback was only one, now nothing is called, and nothing is<br>
--- logged.<br>
-test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution: upgraded to') == nil<br>
+test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution: upgraded to') ~= nil<br>
 ---<br>
 - true<br>
 ...<br>
 vshard.storage.internal.reload_version<br>
 ---<br>
+- 2<br>
+...<br>
+--<br>
+-- gh-237: should be only one trigger. During gh-237 the trigger installation<br>
+-- became conditional and therefore required to remember the current trigger<br>
+-- somewhere. When reloaded from the old version, the trigger needed to be<br>
+-- fetched from _bucket:on_replace().<br>
+--<br>
+#box.space._bucket:on_replace()<br>
+---<br>
 - 1<br>
 ...<br>
 -- Make sure storage operates well.<br>
diff --git a/test/reload_evolution/storage.test.lua b/test/reload_evolution/storage.test.lua<br>
index 7858112..06f7117 100644<br>
--- a/test/reload_evolution/storage.test.lua<br>
+++ b/test/reload_evolution/storage.test.lua<br>
@@ -35,12 +35,16 @@ box.space.test:insert({42, bucket_id_to_move})<br>
 package.path = original_package_path<br>
 package.loaded['vshard.storage'] = nil<br>
 vshard.storage = require("vshard.storage")<br>
--- Should be nil. Because there was a bug that reload always reapplied the last<br>
--- migration callback. When it was fixed, the last callback wasn't called twice.<br>
--- But since the callback was only one, now nothing is called, and nothing is<br>
--- logged.<br>
-test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution: upgraded to') == nil<br>
+test_run:grep_log('storage_2_a', 'vshard.storage.reload_evolution: upgraded to') ~= nil<br>
 vshard.storage.internal.reload_version<br>
+--<br>
+-- gh-237: should be only one trigger. During gh-237 the trigger installation<br>
+-- became conditional and therefore required to remember the current trigger<br>
+-- somewhere. When reloaded from the old version, the trigger needed to be<br>
+-- fetched from _bucket:on_replace().<br>
+--<br>
+#box.space._bucket:on_replace()<br>
+<br>
 -- Make sure storage operates well.<br>
 vshard.storage.bucket_force_drop(2000)<br>
 vshard.storage.bucket_force_create(2000)<br>
diff --git a/test/router/boot_replica_first.result b/test/router/boot_replica_first.result<br>
new file mode 100644<br>
index 0000000..1705230<br>
--- /dev/null<br>
+++ b/test/router/boot_replica_first.result<br>
@@ -0,0 +1,112 @@<br>
+-- test-run result file version 2<br>
+test_run = require('test_run').new()<br>
+ | ---<br>
+ | ...<br>
+REPLICASET_1 = { 'box_1_a', 'box_1_b', 'box_1_c' }<br>
+ | ---<br>
+ | ...<br>
+test_run:create_cluster(REPLICASET_1, 'router', {args = 'boot_before_cfg'})<br>
+ | ---<br>
+ | ...<br>
+util = require('util')<br>
+ | ---<br>
+ | ...<br>
+util.wait_master(test_run, REPLICASET_1, 'box_1_a')<br>
+ | ---<br>
+ | ...<br>
+_ = test_run:cmd("create server router with script='router/router_2.lua'")<br>
+ | ---<br>
+ | ...<br>
+_ = test_run:cmd("start server router")<br>
+ | ---<br>
+ | ...<br>
+<br>
+--<br>
+-- gh-237: replica should be able to boot before master. Before the issue was<br>
+-- fixed, replica always tried to install a trigger on _bucket space even when<br>
+-- it was not created on a master yet - that lead to an exception in<br>
+-- storage.cfg. Now it should not install the trigger at all, because anyway it<br>
+-- is not needed on replica for anything.<br>
+--<br>
+<br>
+test_run:switch('box_1_b')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+vshard.storage.cfg(cfg, instance_uuid)<br>
+ | ---<br>
+ | ...<br>
+-- _bucket is not created yet. Will fail.<br>
+util.check_error(vshard.storage.call, 1, 'read', 'echo', {100})<br>
+ | ---<br>
+ | - attempt to index field '_bucket' (a nil value)<br>
+ | ...<br>
+<br>
+test_run:switch('default')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+util.map_evals(test_run, {REPLICASET_1}, 'bootstrap_storage(\'memtx\')')<br>
+ | ---<br>
+ | ...<br>
+<br>
+test_run:switch('box_1_a')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+vshard.storage.cfg(cfg, instance_uuid)<br>
+ | ---<br>
+ | ...<br>
+<br>
+test_run:switch('box_1_b')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+test_run:wait_lsn('box_1_b', 'box_1_a')<br>
+ | ---<br>
+ | ...<br>
+-- Fails, but gracefully. Because the bucket is not found here.<br>
+vshard.storage.call(1, 'read', 'echo', {100})<br>
+ | ---<br>
+ | - null<br>
+ | - bucket_id: 1<br>
+ |   reason: Not found<br>
+ |   code: 1<br>
+ |   type: ShardingError<br>
+ |   message: 'Cannot perform action with bucket 1, reason: Not found'<br>
+ |   name: WRONG_BUCKET<br>
+ | ...<br>
+-- Should not have triggers.<br>
+#box.space._bucket:on_replace()<br>
+ | ---<br>
+ | - 0<br>
+ | ...<br>
+<br>
+test_run:switch('router')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+vshard.router.bootstrap()<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+vshard.router.callro(1, 'echo', {100})<br>
+ | ---<br>
+ | - 100<br>
+ | ...<br>
+<br>
+test_run:switch("default")<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+test_run:cmd('stop server router')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+test_run:cmd('delete server router')<br>
+ | ---<br>
+ | - true<br>
+ | ...<br>
+test_run:drop_cluster(REPLICASET_1)<br>
+ | ---<br>
+ | ...<br>
diff --git a/test/router/boot_replica_first.test.lua b/test/router/boot_replica_first.test.lua<br>
new file mode 100644<br>
index 0000000..7b1b3fd<br>
--- /dev/null<br>
+++ b/test/router/boot_replica_first.test.lua<br>
@@ -0,0 +1,42 @@<br>
+test_run = require('test_run').new()<br>
+REPLICASET_1 = { 'box_1_a', 'box_1_b', 'box_1_c' }<br>
+test_run:create_cluster(REPLICASET_1, 'router', {args = 'boot_before_cfg'})<br>
+util = require('util')<br>
+util.wait_master(test_run, REPLICASET_1, 'box_1_a')<br>
+_ = test_run:cmd("create server router with script='router/router_2.lua'")<br>
+_ = test_run:cmd("start server router")<br>
+<br>
+--<br>
+-- gh-237: replica should be able to boot before master. Before the issue was<br>
+-- fixed, replica always tried to install a trigger on _bucket space even when<br>
+-- it was not created on a master yet - that lead to an exception in<br>
+-- storage.cfg. Now it should not install the trigger at all, because anyway it<br>
+-- is not needed on replica for anything.<br>
+--<br>
+<br>
+test_run:switch('box_1_b')<br>
+vshard.storage.cfg(cfg, instance_uuid)<br>
+-- _bucket is not created yet. Will fail.<br>
+util.check_error(vshard.storage.call, 1, 'read', 'echo', {100})<br>
+<br>
+test_run:switch('default')<br>
+util.map_evals(test_run, {REPLICASET_1}, 'bootstrap_storage(\'memtx\')')<br>
+<br>
+test_run:switch('box_1_a')<br>
+vshard.storage.cfg(cfg, instance_uuid)<br>
+<br>
+test_run:switch('box_1_b')<br>
+test_run:wait_lsn('box_1_b', 'box_1_a')<br>
+-- Fails, but gracefully. Because the bucket is not found here.<br>
+vshard.storage.call(1, 'read', 'echo', {100})<br>
+-- Should not have triggers.<br>
+#box.space._bucket:on_replace()<br>
+<br>
+test_run:switch('router')<br>
+vshard.router.bootstrap()<br>
+vshard.router.callro(1, 'echo', {100})<br>
+<br>
+test_run:switch("default")<br>
+test_run:cmd('stop server router')<br>
+test_run:cmd('delete server router')<br>
+test_run:drop_cluster(REPLICASET_1)<br>
diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua<br>
index c6a78fe..5464824 100644<br>
--- a/vshard/storage/init.lua<br>
+++ b/vshard/storage/init.lua<br>
@@ -94,6 +94,17 @@ if not M then<br>
         -- detect that _bucket was not changed between yields.<br>
         --<br>
         bucket_generation = 0,<br>
+        --<br>
+        -- Reference to the function used as on_replace trigger on<br>
+        -- _bucket space. It is used to replace the trigger with<br>
+        -- a new function when reload happens. It is kept<br>
+        -- explicitly because the old function is deleted on<br>
+        -- reload from the global namespace. On the other hand, it<br>
+        -- is still stored in _bucket:on_replace() somewhere, but<br>
+        -- it is not known where. The only 100% way to be able to<br>
+        -- replace the old function is to keep its reference.<br>
+        --<br>
+        bucket_on_replace = nil,<br>
<br>
         ------------------- Garbage collection -------------------<br>
         -- Fiber to remove garbage buckets data.<br>
@@ -2435,8 +2446,14 @@ local function storage_cfg(cfg, this_replica_uuid, is_reload)<br>
     local uri = luri.parse(this_replica.uri)<br>
     schema_upgrade(is_master, uri.login, uri.password)<br>
<br>
-    local old_trigger = box.space._bucket:on_replace()[1]<br>
-    box.space._bucket:on_replace(bucket_generation_increment, old_trigger)<br>
+    if M.bucket_on_replace then<br>
+        box.space._bucket:on_replace(nil, M.bucket_on_replace)<br>
+        M.bucket_on_replace = nil<br>
+    end<br>
+    if is_master then<br>
+        box.space._bucket:on_replace(bucket_generation_increment)<br>
+        M.bucket_on_replace = bucket_generation_increment<br>
+    end<br>
<br>
     lreplicaset.rebind_replicasets(new_replicasets, M.replicasets)<br>
     lreplicaset.outdate_replicasets(M.replicasets)<br>
diff --git a/vshard/storage/reload_evolution.lua b/vshard/storage/reload_evolution.lua<br>
index 1abc9e2..f38af74 100644<br>
--- a/vshard/storage/reload_evolution.lua<br>
+++ b/vshard/storage/reload_evolution.lua<br>
@@ -17,6 +17,14 @@ migrations[#migrations + 1] = function(M)<br>
     -- Code to update Lua objects.<br>
 end<br>
<br>
+migrations[#migrations + 1] = function(M)<br>
+    local bucket = box.space._bucket<br>
+    if bucket then<br>
+        assert(M.bucket_on_replace == nil)<br>
+        M.bucket_on_replace = bucket:on_replace()[1]<br>
+    end<br>
+end<br>
+<br>
 --<br>
 -- Perform an update based on a version stored in `M` (internals).<br>
 -- @param M Old module internals which should be updated.<br>
-- <br>
2.21.1 (Apple Git-122.3)<br>
<br>
</blockquote></div>