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

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

[Евгений Заикин]

Adds prohibition of simultaneous router/storage configuration from
multiple fibers.
Fixes: #140

---
Ticket:https://github.com/tarantool/vshard/issues/140
Branch:https://github.com/tarantool/vshard/tree/rave4life/gh-140-prohibit-multiple-cfg

Changes in v2:
  - separated test for router and storage
  - locker wrapper function moved to vshard.utils
 test/router/multi_fiber_cfg.result    | 38 +++++++++++++++++++++++++
 test/router/multi_fiber_cfg.test.lua  | 12 ++++++++
 test/storage/multi_fiber_cfg.result   | 41 +++++++++++++++++++++++++++
 test/storage/multi_fiber_cfg.test.lua | 13 +++++++++
 vshard/router/init.lua                |  6 ++--
 vshard/storage/init.lua               |  4 +--
 vshard/util.lua                       | 17 +++++++++++
 7 files changed, 126 insertions(+), 5 deletions(-)
 create mode 100644 test/router/multi_fiber_cfg.result
 create mode 100644 test/router/multi_fiber_cfg.test.lua
 create mode 100644 test/storage/multi_fiber_cfg.result
 create mode 100644 test/storage/multi_fiber_cfg.test.lua

diff --git a/test/router/multi_fiber_cfg.result
b/test/router/multi_fiber_cfg.result
new file mode 100644
index 0000000..42a4055
--- /dev/null
+++ b/test/router/multi_fiber_cfg.result
@@ -0,0 +1,38 @@
+test_run = require('test_run').new()
+---
+...
+cfg = require('localcfg')
+---
+...
+cfg.memtx_memory = nil
+---
+...
+vshard = require('vshard')
+---
+...
+fiber = require('fiber')
+---
+...
+vshard.router.cfg(cfg)
+---
+...
+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()
+---
+- true
+...
+c:get()
+---
+- true
+...
diff --git a/test/router/multi_fiber_cfg.test.lua
b/test/router/multi_fiber_cfg.test.lua
new file mode 100644
index 0000000..2d0a0c9
--- /dev/null
+++ b/test/router/multi_fiber_cfg.test.lua
@@ -0,0 +1,12 @@
+test_run = require('test_run').new()
+cfg = require('localcfg')
+cfg.memtx_memory = nil
+vshard = require('vshard')
+fiber = require('fiber')
+vshard.router.cfg(cfg)
+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()
diff --git a/test/storage/multi_fiber_cfg.result
b/test/storage/multi_fiber_cfg.result
new file mode 100644
index 0000000..92fd06c
--- /dev/null
+++ b/test/storage/multi_fiber_cfg.result
@@ -0,0 +1,41 @@
+test_run = require('test_run').new()
+---
+...
+cfg = {memtx_memory = nil, 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')
+---
+...
+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
+...
+cfg = nil
+---
+...
diff --git a/test/storage/multi_fiber_cfg.test.lua
b/test/storage/multi_fiber_cfg.test.lua
new file mode 100644
index 0000000..1a6cd39
--- /dev/null
+++ b/test/storage/multi_fiber_cfg.test.lua
@@ -0,0 +1,13 @@
+test_run = require('test_run').new()
+cfg = {memtx_memory = nil, 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')
+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()
+cfg = nil
diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index 60ceafa..6b7a2c5 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -860,7 +860,7 @@ end

 local router_mt = {
     __index = {
-        cfg = function(router, cfg) return router_cfg(router, cfg, false) end,
+        cfg = function(router, cfg) return
util.locker_func(router_cfg, router, cfg, false) end,
         info = router_info;
         buckets_info = router_buckets_info;
         call = router_call;
@@ -934,7 +934,7 @@ end
 local function legacy_cfg(cfg)
     if M.static_router then
         -- Reconfigure.
-        router_cfg(M.static_router, cfg, false)
+        util.locker_func(router_cfg, M.static_router, cfg, false)
     else
         -- Create new static instance.
         local router, err = router_new(STATIC_ROUTER_NAME, cfg)
@@ -959,7 +959,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)
+        util.locker_func(router_cfg, 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 f9b8000..83f7343 100644
--- a/vshard/storage/init.lua
+++ b/vshard/storage/init.lua
@@ -1871,7 +1871,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)
+ util.locker_func(storage_cfg, M.current_cfg, M.this_replica.uuid, true)
     M.module_version = M.module_version + 1
 end

@@ -1908,7 +1908,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 util.locker_func(storage_cfg,
cfg, uuid, false) end,
     info = storage_info,
     buckets_info = storage_buckets_info,
     buckets_count = storage_buckets_count,
diff --git a/vshard/util.lua b/vshard/util.lua
index 3afaa61..d52598b 100644
--- a/vshard/util.lua
+++ b/vshard/util.lua
@@ -11,6 +11,8 @@ if not M then
     M = {
         -- Latest versions of functions.
         reloadable_fiber_main_loop = nil,
+        cfg_counter = 0,
+        lock = fiber.channel(1),
     }
     rawset(_G, MODULE_INTERNALS, M)
 end
@@ -134,10 +136,25 @@ local function async_task(delay, task, ...)
     fiber.create(sync_task, delay, task, ...)
 end

+local function locker_func(f, ...)
+    M.lock:put(true)
+    if M.cfg_counter > 0 then
+        error('Error: cfg_counter > 0')
+    end
+    M.cfg_counter = M.cfg_counter + 1
+    local status, err = pcall(f, ...)
+    M.cfg_counter = M.cfg_counter - 1
+    M.lock:get()
+    if not status then
+        error(err)
+    end
+end
+
 return {
     tuple_extract_key = tuple_extract_key,
     reloadable_fiber_create = reloadable_fiber_create,
     generate_self_checker = generate_self_checker,
     async_task = async_task,
+    locker_func = locker_func,
     internal = M,
 }
-- 
2.17.1.windows.1

[tarantool-patches] Re: [PATCH v2] Prohibit router/storage configuration from different fibers

[Vladislav Shpilevoy]

Hi! Thanks for the fixes. See 9 comments below.

1. Please, put your fixes inlined right after my
remarks in a response email. It is hard sometimes
so track which of my remarks are not fixed without
this.

2. Write all your questions about a patch into
tarantool-patches. I paste here your question
from a private email:

> Hi! I have a question about writing test module. The test should fail
> if patch (or any part of this patch) is absent. But it is difficult to
> make it possible without any architectural distortion (e.g. without
> making nested function calls, without redundant constructions and code
> injection, without any overkills in working code). There are no
> questions If no test needed, but...
> So which way is correct:
> 1. to make one more nested (redundant) call in router/storage code;
> 2. to inject additional counters in 'vshard.util' module, witch
> contains implementation of my path ?
> Current PATCH v2, witch was already sent, implements 2nd way (test
> fails if M.lock:put(true) / M.lock:get() is absent or disabled).
> https://www.freelists.org/post/tarantool-patches/PATCH-v2-Prohibit-routerstorage-configuration-from-different-fibers
> 
> Thank you for reviewing my patch.

Answer is the following: neither of those things. Use M.errinj
table for such error injections, as I said in the previous review.
Look at its current content and how is it used to make the similar
test for concurrent cfgs. Or dig into errinj.h in Tarantool to
find which of existing ones can be used.

On 21/08/2018 06:27, Евгений Заикин wrote:
> Adds prohibition of simultaneous router/storage configuration from
> multiple fibers.
> Fixes: #140
> 
> ---
> Ticket:https://github.com/tarantool/vshard/issues/140
> Branch:https://github.com/tarantool/vshard/tree/rave4life/gh-140-prohibit-multiple-cfg
> 
> Changes in v2:
>    - separated test for router and storage
>    - locker wrapper function moved to vshard.utils
>   test/router/multi_fiber_cfg.result    | 38 +++++++++++++++++++++++++
>   test/router/multi_fiber_cfg.test.lua  | 12 ++++++++
>   test/storage/multi_fiber_cfg.result   | 41 +++++++++++++++++++++++++++
>   test/storage/multi_fiber_cfg.test.lua | 13 +++++++++
>   vshard/router/init.lua                |  6 ++--
>   vshard/storage/init.lua               |  4 +--
>   vshard/util.lua                       | 17 +++++++++++
>   7 files changed, 126 insertions(+), 5 deletions(-)
>   create mode 100644 test/router/multi_fiber_cfg.result
>   create mode 100644 test/router/multi_fiber_cfg.test.lua
>   create mode 100644 test/storage/multi_fiber_cfg.result
>   create mode 100644 test/storage/multi_fiber_cfg.test.lua
> 
> diff --git a/test/router/multi_fiber_cfg.result
> b/test/router/multi_fiber_cfg.result
> new file mode 100644
> index 0000000..42a4055
> --- /dev/null
> +++ b/test/router/multi_fiber_cfg.result
> @@ -0,0 +1,38 @@
> +test_run = require('test_run').new()
> +---
> +...
> +cfg = require('localcfg')
> +---
> +...
> +cfg.memtx_memory = nil

3. Why do you modify cfg?
The same question for storage test.

> +---
> +...
> +vshard = require('vshard')

4. As I said in the previous review, please,
put this test into router/router.test.lua. It
is not worth a separate file.

The same remark for storage test.

> +---
> +...
> +fiber = require('fiber')
> +---
> +...
> +vshard.router.cfg(cfg)
> +---
> +...
> +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()
> +---
> +- true
> +...
> +c:get()
> +---
> +- true
> +...
> diff --git a/vshard/router/init.lua b/vshard/router/init.lua
> index 60ceafa..6b7a2c5 100644
> --- a/vshard/router/init.lua
> +++ b/vshard/router/init.lua
> @@ -860,7 +860,7 @@ end
> 
>   local router_mt = {
>       __index = {
> -        cfg = function(router, cfg) return router_cfg(router, cfg, false) end,
> +        cfg = function(router, cfg) return
> util.locker_func(router_cfg, router, cfg, false) end,

5. Out of 80 symbols per line.

>           info = router_info;
>           buckets_info = router_buckets_info;
>           call = router_call;
> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
> index f9b8000..83f7343 100644
> --- a/vshard/storage/init.lua
> +++ b/vshard/storage/init.lua
> @@ -1871,7 +1871,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)
> + util.locker_func(storage_cfg, M.current_cfg, M.this_replica.uuid, true)

6. Do not use tabs in Lua files for indentation.

>       M.module_version = M.module_version + 1
>   end
> 
> @@ -1908,7 +1908,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 util.locker_func(storage_cfg,
> cfg, uuid, false) end,

7. Out of 80.

>       info = storage_info,
>       buckets_info = storage_buckets_info,
>       buckets_count = storage_buckets_count,
> diff --git a/vshard/util.lua b/vshard/util.lua
> index 3afaa61..d52598b 100644
> --- a/vshard/util.lua
> +++ b/vshard/util.lua
> @@ -134,10 +136,25 @@ local function async_task(delay, task, ...)
>       fiber.create(sync_task, delay, task, ...)
>   end
> 
> +local function locker_func(f, ...)
> +    M.lock:put(true)
> +    if M.cfg_counter > 0 then
> +        error('Error: cfg_counter > 0')
> +    end
> +    M.cfg_counter = M.cfg_counter + 1
> +    local status, err = pcall(f, ...)
> +    M.cfg_counter = M.cfg_counter - 1
> +    M.lock:get()
> +    if not status then
> +        error(err)
> +    end
> +end

8. Since you started working on this patch, multiple
routers per process feature was pushed, so M now can
not be used in router for the counter and the lock.
Please, rebase. Now each router should have its own
non-global lock.

9. Please, write a comment to this function like it
is done for other ones.

> +
>   return {
>       tuple_extract_key = tuple_extract_key,
>       reloadable_fiber_create = reloadable_fiber_create,
>       generate_self_checker = generate_self_checker,
>       async_task = async_task,
> +    locker_func = locker_func,
>       internal = M,
>   }
>