[tarantool-patches] Re: [PATCH][vshard] Prohibit reconfigure non-dynamic options

[Alex Khatskevich <avkhatskevich@tarantool.org>]

> On 15/06/2018 17:02, AKhatskevich wrote:
>> +err:match('Non%-dynamic.*')
>
> 1. Please, used util.check_error function instead of pcall + match.
>> +_, err = pcall(vshard.storage.cfg, non_dynamic_cfg, names.storage_1_a)
>
> 2. Same. And try to test here bucket_count instead of shard_index.
fixed
>> +non_dynamic_cfg_old = table.copy(cfg)
>
> 3. Why do you need here so many copies? Please, do not copy when
> possible. Tests are too long and big with no copies even.
ok
>> +err:match('Non%-dynamic.*')
>
> 4. Same as 1, 2: use util.check_error.
fixed
>
>>
>> -local function cfg_check(shard_cfg)
>> +local function cfg_check(shard_cfg, old_cfg)
>>       if type(shard_cfg) ~= 'table' then
>>           error('Сonfig must be map of options')
>>       end
>>       shard_cfg = table.deepcopy(shard_cfg)
>>       validate_config(shard_cfg, cfg_template)
>> +    if not old_cfg then
>> +        return shard_cfg
>> +    end
>> +    assert(not old_cfg or type(old_cfg) == 'table')
>
> 5. Useless assert: 3 lines above we have already checked old_cfg to be
> not null. It is like
>
>     if cond then
>         -- ...
>     else
>         assert(not cond)
>     end
deleted
>
> 6. This table will be created/destroyed on each cfg.check call. Please,
> move it out of the function like it is done for other option tables.
fixed
>
>> +    for _, f_name in pairs(non_dynamic) do
>> +        -- New option may be added in new vshard verion.
>
> 7. Typo.
fixed
>
> 8. Now VShard has no non-dynamic options but bucket_count and 
> shard_index.
> And there are no ideas of new non-dynamic options. So please, just check
> old != new. We can add this check for new options when it will be needed.
>
>> +        if old_cfg[f_name] and shard_cfg[f_name] ~= old_cfg[f_name] 
>> then
ok
>> +    cfg = lcfg.check(cfg, M.current_cfg)
>> +    local new_cfg = table.deepcopy(cfg)
>
> 9. Why in tests do you copy, but deepcopy here?
Fixed, but I hope this patch will be applied after gh-116-reload, where 
the same code is written.
I just was not sure if box.cfg changes the passed table and did deepcopy.

Full diff of the patch:

commit 38d504e5d91835916618c743ba3b34ca0fd7ca22
Author: AKhatskevich <avkhatskevich@tarantool.org>
Date:   Fri Jun 15 16:53:06 2018 +0300

     Prohibit reconfigure non-dynamic options

     By now there are two non-dynamic options:
      * bucket_count
      * shard_index

     They cannot be changed by passing a new value to
     router/storage.cfg.

     Closes #114

diff --git a/test/router/router.result b/test/router/router.result
index 2ee1bff..5940e45 100644
--- a/test/router/router.result
+++ b/test/router/router.result
@@ -1057,6 +1057,17 @@ error_messages
  - - Use replica:is_connected(...) instead of replica.is_connected(...)
    - Use replica:safe_uri(...) instead of replica.safe_uri(...)
  ...
+-- gh-114: Check non-dynamic option change during reconfigure.
+non_dynamic_cfg = table.copy(cfg)
+---
+...
+non_dynamic_cfg.shard_index = 'non_default_name'
+---
+...
+util.check_error(vshard.router.cfg, non_dynamic_cfg)
+---
+- Non-dynamic option shard_index is changed in new vshard config
+...
  _ = test_run:cmd("switch default")
  ---
  ...
diff --git a/test/router/router.test.lua b/test/router/router.test.lua
index fae8e24..63616cd 100644
--- a/test/router/router.test.lua
+++ b/test/router/router.test.lua
@@ -389,6 +389,11 @@ end;
  test_run:cmd("setopt delimiter ''");
  error_messages

+-- gh-114: Check non-dynamic option change during reconfigure.
+non_dynamic_cfg = table.copy(cfg)
+non_dynamic_cfg.shard_index = 'non_default_name'
+util.check_error(vshard.router.cfg, non_dynamic_cfg)
+
  _ = test_run:cmd("switch default")
  test_run:drop_cluster(REPLICASET_2)

diff --git a/test/storage/storage.result b/test/storage/storage.result
index d0bf792..c6609f6 100644
--- a/test/storage/storage.result
+++ b/test/storage/storage.result
@@ -720,6 +720,17 @@ test_run:cmd("setopt delimiter ''");
  ---
  - true
  ...
+-- gh-114: Check non-dynamic option change during reconfigure.
+non_dynamic_cfg = table.copy(cfg)
+---
+...
+non_dynamic_cfg.shard_index = 'non_default_name'
+---
+...
+util.check_error(vshard.storage.cfg, non_dynamic_cfg, names.storage_1_a)
+---
+- Non-dynamic option shard_index is changed in new vshard config
+...
  _ = test_run:cmd("switch default")
  ---
  ...
diff --git a/test/storage/storage.test.lua b/test/storage/storage.test.lua
index f4bbf0e..1e1112b 100644
--- a/test/storage/storage.test.lua
+++ b/test/storage/storage.test.lua
@@ -177,6 +177,11 @@ for _, new_replicaset in pairs(new_replicasets) do
  end;
  test_run:cmd("setopt delimiter ''");

+-- gh-114: Check non-dynamic option change during reconfigure.
+non_dynamic_cfg = table.copy(cfg)
+non_dynamic_cfg.shard_index = 'non_default_name'
+util.check_error(vshard.storage.cfg, non_dynamic_cfg, names.storage_1_a)
+
  _ = test_run:cmd("switch default")

  test_run:drop_cluster(REPLICASET_2)
diff --git a/test/unit/config.result b/test/unit/config.result
index 6b4f87b..d97f2b6 100644
--- a/test/unit/config.result
+++ b/test/unit/config.result
@@ -506,3 +506,17 @@ _ = lcfg.check(cfg)
  replica.name = 'storage'
  ---
  ...
+-- gh-114: Check non-dynamic option change during reconfigure.
+cfg_with_non_default = table.copy(cfg)
+---
+...
+cfg.shard_index = nil
+---
+...
+cfg_with_non_default.shard_index = 'non_default_name'
+---
+...
+util.check_error(lcfg.check, cfg, cfg_with_non_default)
+---
+- Non-dynamic option shard_index is changed in new vshard config
+...
diff --git a/test/unit/config.test.lua b/test/unit/config.test.lua
index 0f549d3..e0cd4f4 100644
--- a/test/unit/config.test.lua
+++ b/test/unit/config.test.lua
@@ -206,3 +206,9 @@ _ = lcfg.check(cfg)
  replica.name = nil
  _ = lcfg.check(cfg)
  replica.name = 'storage'
+
+-- gh-114: Check non-dynamic option change during reconfigure.
+cfg_with_non_default = table.copy(cfg)
+cfg.shard_index = nil
+cfg_with_non_default.shard_index = 'non_default_name'
+util.check_error(lcfg.check, cfg, cfg_with_non_default)
diff --git a/vshard/cfg.lua b/vshard/cfg.lua
index f5db4c0..f9d6d52 100644
--- a/vshard/cfg.lua
+++ b/vshard/cfg.lua
@@ -220,16 +220,34 @@ local cfg_template = {
      }},
  }

+--
+-- Names of options which cnnot be changed during reconfigure.
+--
+local non_dynamic_options = {
+    'bucket_count', 'shard_index'
+}
+
  --
  -- Check sharding config on correctness. Check types, name and uri
--- uniqueness, master count (in each replicaset must by <= 1).
+-- uniqueness, master count (in each replicaset must be <= 1).
  --
-local function cfg_check(shard_cfg)
+local function cfg_check(shard_cfg, old_cfg)
      if type(shard_cfg) ~= 'table' then
          error('Сonfig must be map of options')
      end
      shard_cfg = table.deepcopy(shard_cfg)
      validate_config(shard_cfg, cfg_template)
+    if not old_cfg then
+        return shard_cfg
+    end
+    -- Check non-dynamic after default values are added.
+    for _, f_name in pairs(non_dynamic_options) do
+        -- New option may be added in new vshard version.
+        if old_cfg[f_name] and shard_cfg[f_name] ~= old_cfg[f_name] then
+           error(string.format('Non-dynamic option %s ' ..
+                               'is changed in new vshard config', f_name))
+        end
+    end
      return shard_cfg
  end

diff --git a/vshard/router/init.lua b/vshard/router/init.lua
index 21093e5..fc4714d 100644
--- a/vshard/router/init.lua
+++ b/vshard/router/init.lua
@@ -457,7 +457,8 @@ end
  --------------------------------------------------------------------------------

  local function router_cfg(cfg)
-    cfg = lcfg.check(cfg)
+    cfg = lcfg.check(cfg, M.current_cfg)
+    local new_cfg = table.copy(cfg)
      if not M.replicasets then
          log.info('Starting router configuration')
      else
@@ -478,6 +479,7 @@ local function router_cfg(cfg)
      -- TODO: update existing route map in-place
      M.route_map = {}
      M.replicasets = new_replicasets
+    M.current_cfg = new_cfg
      -- Move connections from an old configuration to a new one.
      -- It must be done with no yields to prevent usage both of not
      -- fully moved old replicasets, and not fully built new ones.
diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
index 57076e1..8507a8a 100644
--- a/vshard/storage/init.lua
+++ b/vshard/storage/init.lua
@@ -1448,7 +1448,8 @@ local function storage_cfg(cfg, this_replica_uuid)
      if this_replica_uuid == nil then
          error('Usage: cfg(configuration, this_replica_uuid)')
      end
-    cfg = lcfg.check(cfg)
+    cfg = lcfg.check(cfg, M.current_cfg)
+    local new_cfg = table.copy(cfg)
      if cfg.weights or cfg.zone then
          error('Weights and zone are not allowed for storage 
configuration')
      end
@@ -1574,6 +1575,7 @@ local function storage_cfg(cfg, this_replica_uuid)
      M.shard_index = shard_index
      M.collect_bucket_garbage_interval = collect_bucket_garbage_interval
      M.collect_lua_garbage = collect_lua_garbage
+    M.current_cfg = new_cfg

      if was_master and not is_master then
          local_on_master_disable()