[tarantool-patches] [PATCH V4] Prohibit router/storage configuration from different fibers

[Evgeniy Zaikin <evgen4rave@gmail.com>]

Subject: [PATCH V4] Prohibit router/storage configuration from different fibers
Fixes: #140
---
Ticket:https://github.com/tarantool/vshard/issues/140
Branch:https://github.com/tarantool/vshard/tree/rave4life/gh-140-prohibit-multiple-cfg

Hi! Thank you for review. This is my next patch version - hope it is
correct. You'll find patch below.

On Thu, 20 Sep 2018 at 22:59, Vladislav Shpilevoy
<v.shpilevoy@tarantool.org> wrote:
>
> Hi! Sorry for the last response - I was on vacation last
> 2 weeks. See 8 comments below.
>
> On 11/09/2018 12:32, Evgeniy Zaikin wrote:
> >  From 5ce83d5db595455a14a58aedf9e6411f5adf1d7c Mon Sep 17 00:00:00 2001
> > From: rave4life <evgen4rave@gmail.com>
> > Date: Fri, 31 Aug 2018 16:41:11 +0300
> > Subject: [PATCH V3] Prohibit router/storage configuration from different fibers
> >
> > Fixes: #140
> > ---
> > Ticket:https://github.com/tarantool/vshard/issues/140
> > Branch:https://github.com/tarantool/vshard/tree/rave4life/gh-140-prohibit-multiple-cfg
> >
>
> > diff --git a/test/router/router.result b/test/router/router.result
> > index 000307d..ada8309 100644
> > --- a/test/router/router.result
> > +++ b/test/router/router.result
> > @@ -1140,6 +1140,45 @@ test_run:drop_cluster(REPLICASET_2)
> >   while test_run:grep_log('router_1', 'disconnected from ') == nil do
> > fiber.sleep(0.1) end
> >   ---
> >   ...
> > +-- gh-140: prohibit vshard.router/storage.cfg from multiple fibers.
>

Fixed.

> 1. Sorry, still out of 80 symbols. It is a strict rule. Please,
> inspect your patch carefully for such code style things before
> sending to a review.
>
> > +cfg = {sharding = require('localcfg').sharding}
> > +---
> > +...
> > +vshard = require('vshard')
> > +---
> > +...
> > +fiber = require('fiber')
>

OK. Switched to 'storage_1_a' for storage and to 'router_1' for router.

> 2. Please, do not use 'default' server for testing
> storage or router. Router/router.test.lua starts multiple
> Tarantool instances: storage_1_a - storage_2_b and router_1
> (see the very beginning of the test file) and switches
> between them to test router or storage things. 'Default'
> server shall not be used for this. Please, use test_run:switch
> and router_1 server for a test. In such a case you will not
> have require('vshard') and require('fiber') since they are
> already loaded on router_1 instance.
>
> There is a reason why 'default' can not be used - once a
> router is started on an instance, it can not be stopped
> without restart (its internal fibers, I mean). For the storage
> things are worse - it creates new spaces, users, functions
> and we have no way to cleanup those things. So once 'default'
> is used for testing, it will affect next tests.
>
> For this problem we have an issue: https://github.com/tarantool/vshard/issues/121
> You can try it next after this patch is finished if you want.
>
> > +---
> > +...
> > +vshard.router.cfg(cfg)
> > +---
> > +...
> > +vshard.router.internal.errinj.ERRINJ_MULTIPLE_CFG = false
>

Set to 'nil' according to your suggestion (8-th comment)

> 3. It is false by default already (as I see in router/init.lua),
> so what is a point of this line? I removed it and the test
> passed ok. I guess it is a typo. Please, fix.
>
> > +---
> > +...
> > +c = fiber.channel(2)
> > +---
> > +...
> > diff --git a/test/storage/storage.result b/test/storage/storage.result
> > index e3e9e7f..0228445 100644
> > --- a/test/storage/storage.result
> > +++ b/test/storage/storage.result
> > @@ -688,6 +688,48 @@ rs:callro('echo', {'some_data'})
> >   - null
> >   - null
> >   ...
> > +-- gh-140: prohibit vshard.router/storage.cfg from multiple fibers.
> > +cfg = {sharding = {}}
> > +---
> > +...
> > +cfg.sharding[box.info.cluster.uuid] = {replicas = {}}
> > +---
> > +...
> > +cfg.sharding[box.info.cluster.uuid].replicas[box.info.uuid] = {uri =
> > 'storage:storage@127.0.0.1:3309', name = 'test_storage', master =
> > true}
> > +---
> > +...
> > +vshard = require('vshard')
> > +---
> > +...
> > +fiber = require('fiber')
>

OK, removed.

> 4. For the same reason I explained above, you do not
> need 'require' here. This code is executed on 'storage_1_a' where
> both fiber and vshard are loaded already
> (see line storage.test.lua:137 - this is the latest switch before
> your test).
>
> > +---
> > +...
> > +vshard.storage.internal.errinj.ERRINJ_MULTIPLE_CFG = false
>
> 5. Same as 3.
>
> > +---
> > +...
> > +c = fiber.channel(2)
> > +---
> > +...
> > diff --git a/vshard/router/init.lua b/vshard/router/init.lua
> > index 7573801..446b1a0 100644
> > --- a/vshard/router/init.lua
> > +++ b/vshard/router/init.lua
> > @@ -28,6 +28,7 @@ if not M then
> >           ---------------- Common module attributes ----------------
> >           errinj = {
> >               ERRINJ_CFG = false,
> > +            ERRINJ_MULTIPLE_CFG = false,
> >               ERRINJ_FAILOVER_CHANGE_CFG = false,
> >               ERRINJ_RELOAD = false,
> >               ERRINJ_LONG_DISCOVERY = false,
> > @@ -53,6 +54,10 @@ local ROUTER_TEMPLATE = {
> >           name = nil,
> >           -- The last passed configuration.
> >           current_cfg = nil,
> > +        -- Fiber channel as the locker to prevent multiple config.
> > +        cfg_lock = nil,
> > +        -- Configuration counter to control and verify locker.
> > +        cfg_counter = 0,
>

OK, 'cfg_counter' is also removed.

> 6. Please, remove cfg_counter. Its value > 0 is unreachable
> during normal work of router/storage. If you need a counter for
> testing lock, you can use erring.ERRINJ_MULTIPLE_CFG as a counter.
> It is not necessary to make each errinj as a strict boolean. See
> my proposal below in the comment 8.
>
> >           -- Time to outdate old objects on reload.
> >           connection_outdate_delay = nil,
> >           -- Bucket map cache.
> > @@ -594,6 +599,29 @@ local function router_cfg(router, cfg, is_reload)
> >       end
> >   end
> >
> > +--------------------------------------------------------------------------------
> > +-- Configuration wrapper
> > +--------------------------------------------------------------------------------
>
> 7. Function comment should look like this:
>
>      --
>      -- My function description wrapped by 66
>      -- symbols.
>      -- @param name1 Parameter 1 description.
>      -- @param name2 Parameter 2 description.
>      --
>      -- [if returns anything]
>      -- @retval Returned value description.
>      -- @retval Next possible return value description.
>      --
>

OK, comment was added.

> > +
> > +local function cfg_wrapper(router, ...)
> > +    local enable_lock = not M.errinj.ERRINJ_MULTIPLE_CFG
> > +    if enable_lock then
> > +        router.cfg_lock:put(true)
> > +    end
> > +    if router.cfg_counter > 0 then
> > +        error('Error: cfg_counter > 0')
> > +    end
> > +    router.cfg_counter = router.cfg_counter + 1
> > +    local status, err = pcall(router_cfg, router, ...)
> > +    router.cfg_counter = router.cfg_counter - 1
> > +    if enable_lock then
> > +        router.cfg_lock:get()
> > +    end
> > +    if not status then
> > +        error(err)
> > +    end
> > +end
>
> 8. Too complex. Lets use errinj.ERRINJ_MULTIPLE_CFG as
> an integer and set it to nil by default
>
> M = {
>   ...
>   errinj = {
>      ...
>      ERRINJ_MULTIPLE_CFG = nil
>   }
>   ...
> }
>
> ...
>
> function cfg_wrapper()
>      if ERRINJ_MULTIPLE_CFG then
>           ERRINJ_MULTIPLE_CFG = ERRINJ_MULTIPLE_CFG + 1
>           if ERRINJ_MULTIPLE_CFG > 1 then
>               error('Multiple cfgs at the same time')
>           end
>           status, err = pcall(router_cfg, router, ...)
>           ERRINJ_MULTIPLE_CFG = ERRINJ_MULTIPLE_CFG - 1
>      else
>          lock:put(true)
>          status, err = pcall(router_cfg, router, ...)
>          lock:get()
>      end
>      if not status then
>          error(err)
>      end
>      return
> end
>
>
> ERRINJ_MULTIPLE_CFG is nil by default and to active it
> you can do this in a test:
>
>      ERRINJ_MULTIPLE_CFG = 0
>
> This will trigger counter in my pseudo code above since '0'
> in Lua is true.
>

Yes, placed new code instead.

So, my fixed patch is below.

Changes in v4:
  - Checked code line length for 80 symbols
  - Switched to appropriate Tarantool instances in test
  - Error injection works now as a triggered counter
  - Added comment to wrapper function

---
 test/router/router.result     | 38 +++++++++++++++++++++++++++++++++++
 test/router/router.test.lua   | 16 +++++++++++++++
 test/storage/storage.result   | 35 ++++++++++++++++++++++++++++++++
 test/storage/storage.test.lua | 16 +++++++++++++++
 vshard/router/init.lua        | 38 ++++++++++++++++++++++++++++++++---
 vshard/storage/init.lua       | 34 +++++++++++++++++++++++++++++--
 6 files changed, 172 insertions(+), 5 deletions(-)

diff --git a/test/router/router.result b/test/router/router.result
index 000307d..5ce48b1 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -1130,6 +1130,44 @@ vshard.router.static:route(1):callro('echo',
{'some_data'})
 - null
 - null
 ...
+-- gh-140: prohibit vshard.router/storage.cfg from multiple fibers.
+cfg = {sharding = require('localcfg').sharding}
+---
+...
+vshard.router.cfg(cfg)
+---
+...
+c = fiber.channel(2)
+---
+...
+vshard.router.internal.errinj.ERRINJ_MULTIPLE_CFG = 0
+---
+...
+f = function()
+    fiber.create(function()
+        local status, err = pcall(vshard.router.cfg, cfg)
+        c:put(status and true or false)
+    end)
+end
+---
+...
+f()
+---
+...
+f()
+---
+...
+c:get()
+---
+- true
+...
+c:get()
+---
+- true
+...
+vshard.router.internal.errinj.ERRINJ_MULTIPLE_CFG = nil
+---
+...
 _ = test_run:switch("default")
 ---
 ...
diff --git a/test/router/router.test.lua b/test/router/router.test.lua
index 9f32d3e..5590f95 100644
--- a/test/router/router.test.lua
+++ b/test/router/router.test.lua
@@ -434,6 +434,22 @@ vshard.router.route(1):callro('echo', {'some_data'})
 -- object.
 vshard.router.static:route(1):callro('echo', {'some_data'})

+-- gh-140: prohibit vshard.router/storage.cfg from multiple fibers.
+cfg = {sharding = require('localcfg').sharding}
+vshard.router.cfg(cfg)
+vshard.router.internal.errinj.ERRINJ_MULTIPLE_CFG = 0
+c = fiber.channel(2)
+f = function()
+    fiber.create(function()
+        local status, err = pcall(vshard.router.cfg, cfg)
+        c:put(status and true or false)
+    end)
+end
+f()
+f()
+c:get()
+c:get()
+vshard.router.internal.errinj.ERRINJ_MULTIPLE_CFG = nil
 _ = test_run:switch("default")
 test_run:drop_cluster(REPLICASET_2)

diff --git a/test/storage/storage.result b/test/storage/storage.result
index e3e9e7f..b528b36 100644
--- a/test/storage/storage.result
+++ b/test/storage/storage.result
@@ -688,6 +688,41 @@ rs:callro('echo', {'some_data'})
 - null
 - null
 ...
+-- gh-140: prohibit storage.cfg from multiple fibers.
+_ = test_run:switch("storage_1_a")
+---
+...
+vshard.storage.internal.errinj.ERRINJ_MULTIPLE_CFG = 0
+---
+...
+c = fiber.channel(2)
+---
+...
+f = function()
+    fiber.create(function()
+        local status, err = pcall(vshard.storage.cfg, cfg, box.info.uuid)
+        c:put(status and true or false)
+    end)
+end
+---
+...
+f()
+---
+...
+f()
+---
+...
+c:get()
+---
+- true
+...
+c:get()
+---
+- true
+...
+vshard.storage.internal.errinj.ERRINJ_MULTIPLE_CFG = nil
+---
+...
 _ = test_run:switch("default")
 ---
 ...
diff --git a/test/storage/storage.test.lua b/test/storage/storage.test.lua
index c07730b..12fff36 100644
--- a/test/storage/storage.test.lua
+++ b/test/storage/storage.test.lua
@@ -182,6 +182,22 @@ util.has_same_fields(old_internal, vshard.storage.internal)
 _, rs = next(vshard.storage.internal.replicasets)
 rs:callro('echo', {'some_data'})

+-- gh-140: prohibit vshard.router/storage.cfg from multiple fibers.
+_ = test_run:switch("storage_1_a")
+vshard.storage.internal.errinj.ERRINJ_MULTIPLE_CFG = 0
+c = fiber.channel(2)
+f = function()
+    fiber.create(function()
+        local status, err = pcall(vshard.storage.cfg, cfg, box.info.uuid)
+        c:put(status and true or false)
+    end)
+end
+f()
+f()
+c:get()
+c:get()
+vshard.storage.internal.errinj.ERRINJ_MULTIPLE_CFG = nil
+
 _ = test_run:switch("default")
 test_run:drop_cluster(REPLICASET_2)
 test_run:drop_cluster(REPLICASET_1)
diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index 7573801..93bef77 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -28,6 +28,7 @@ if not M then
         ---------------- Common module attributes ----------------
         errinj = {
             ERRINJ_CFG = false,
+            ERRINJ_MULTIPLE_CFG = nil,
             ERRINJ_FAILOVER_CHANGE_CFG = false,
             ERRINJ_RELOAD = false,
             ERRINJ_LONG_DISCOVERY = false,
@@ -53,6 +54,8 @@ local ROUTER_TEMPLATE = {
         name = nil,
         -- The last passed configuration.
         current_cfg = nil,
+        -- Fiber channel as the locker to prevent multiple config.
+        cfg_lock = nil,
         -- Time to outdate old objects on reload.
         connection_outdate_delay = nil,
         -- Bucket map cache.
@@ -594,6 +597,34 @@ local function router_cfg(router, cfg, is_reload)
     end
 end

+--
+-- Wrapper around 'router_cfg' function to provide
+-- locking beafore / unlocking after pcall(router_cfg, router, ...),
+-- so it prevents 'router_cfg' been called from diffenrent fibers.
+-- @param router Router instance to be configured.
+-- @param ... Arguments which would be passed to the `router_cfg`.
+-- @retval true, ... on success
+--
+local function cfg_wrapper(router, ...)
+    local status, err
+    if M.errinj.ERRINJ_MULTIPLE_CFG then
+        M.errinj.ERRINJ_MULTIPLE_CFG = M.errinj.ERRINJ_MULTIPLE_CFG + 1
+        if M.errinj.ERRINJ_MULTIPLE_CFG > 1 then
+            error('Multiple cfgs at the same time')
+        end
+        status, err = pcall(router_cfg, router, ...)
+        M.errinj.ERRINJ_MULTIPLE_CFG = M.errinj.ERRINJ_MULTIPLE_CFG - 1
+    else
+        router.cfg_lock:put(true)
+        status, err = pcall(router_cfg, router, ...)
+        router.cfg_lock:get()
+    end
+    if not status then
+        error(err)
+    end
+    return true
+end
+
 --------------------------------------------------------------------------------
 -- Bootstrap
 --------------------------------------------------------------------------------
@@ -862,7 +893,7 @@ end

 local router_mt = {
     __index = {
-        cfg = function(router, cfg) return router_cfg(router, cfg, false) end,
+        cfg = function(router, cfg) return cfg_wrapper(router, cfg) end,
         info = router_info;
         buckets_info = router_buckets_info;
         call = router_call;
@@ -920,6 +951,7 @@ local function router_new(name, cfg)
     local router = table.deepcopy(ROUTER_TEMPLATE)
     setmetatable(router, router_mt)
     router.name = name
+    router.cfg_lock = lfiber.channel(1)
     M.routers[name] = router
     local ok, err = pcall(router_cfg, router, cfg)
     if not ok then
@@ -936,7 +968,7 @@ end
 local function legacy_cfg(cfg)
     if M.static_router then
         -- Reconfigure.
-        router_cfg(M.static_router, cfg, false)
+        cfg_wrapper(M.static_router, cfg, false)
     else
         -- Create new static instance.
         local router, err = router_new(STATIC_ROUTER_NAME, cfg)
@@ -961,7 +993,7 @@ if not rawget(_G, MODULE_INTERNALS) then
     rawset(_G, MODULE_INTERNALS, M)
 else
     for _, router in pairs(M.routers) do
-        router_cfg(router, router.current_cfg, true)
+        cfg_wrapper(router, router.current_cfg, true)
         setmetatable(router, router_mt)
     end
     if M.static_router then
diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
index dc059ba..3b1b525 100644
--- a/vshard/storage/init.lua
+++ b/vshard/storage/init.lua
@@ -48,6 +48,8 @@ if not M then
         ---------------- Common module attributes ----------------
         -- The last passed configuration.
         current_cfg = nil,
+        -- Fiber channel as the locker to prevent multiple config.
+        cfg_lock = lfiber.channel(1),
         --
         -- All known replicasets used for bucket re-balancing.
         -- See format in replicaset.lua.
@@ -66,6 +68,7 @@ if not M then
         total_bucket_count = 0,
         errinj = {
             ERRINJ_CFG = false,
+            ERRINJ_MULTIPLE_CFG = nil,
             ERRINJ_BUCKET_FIND_GARBAGE_DELAY = false,
             ERRINJ_RELOAD = false,
             ERRINJ_CFG_DELAY = false,
@@ -1843,6 +1846,33 @@ local function storage_cfg(cfg,
this_replica_uuid, is_reload)
     collectgarbage()
 end

+--
+-- Wrapper around 'storage_cfg' function to provide
+-- locking beafore / unlocking after pcall(storage_cfg, ...),
+-- so it prevents 'storage_cfg' been called from diffenrent fibers.
+-- @param ... Arguments which would be passed to the `storage_cfg`.
+-- @retval true, ... on success
+--
+local function storage_cfg_wrapper(...)
+    local status, err
+    if M.errinj.ERRINJ_MULTIPLE_CFG then
+        M.errinj.ERRINJ_MULTIPLE_CFG = M.errinj.ERRINJ_MULTIPLE_CFG + 1
+        if M.errinj.ERRINJ_MULTIPLE_CFG > 1 then
+            error('Multiple cfgs at the same time')
+        end
+        status, err = pcall(storage_cfg, ...)
+        M.errinj.ERRINJ_MULTIPLE_CFG = M.errinj.ERRINJ_MULTIPLE_CFG - 1
+    else
+        M.cfg_lock:put(true)
+        status, err = pcall(storage_cfg, ...)
+        M.cfg_lock:get()
+    end
+    if not status then
+        error(err)
+    end
+    return true
+end
+
 --------------------------------------------------------------------------------
 -- Monitoring
 --------------------------------------------------------------------------------
@@ -2060,7 +2090,7 @@ if not rawget(_G, MODULE_INTERNALS) then
     rawset(_G, MODULE_INTERNALS, M)
 else
     reload_evolution.upgrade(M)
-    storage_cfg(M.current_cfg, M.this_replica.uuid, true)
+    storage_cfg_wrapper(M.current_cfg, M.this_replica.uuid, true)
     M.module_version = M.module_version + 1
 end

@@ -2103,7 +2133,7 @@ return {
     rebalancing_is_in_progress = rebalancing_is_in_progress,
     recovery_wakeup = recovery_wakeup,
     call = storage_call,
-    cfg = function(cfg, uuid) return storage_cfg(cfg, uuid, false) end,
+    cfg = function(cfg, uuid) return storage_cfg_wrapper(cfg, uuid, false) end,
     info = storage_info,
     buckets_info = storage_buckets_info,
     buckets_count = storage_buckets_count,
-- 
2.17.1