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

[Alex Khatskevich <avkhatskevich@tarantool.org>]

On 26.06.2018 14:11, Vladislav Shpilevoy wrote:
> Hello. Thanks for the fixes! See 4 comments below.
>
>>> 2. Same. And try to test here bucket_count instead of shard_index.
>> fixed
>
> 1. Not fixed. I do not see a test for bucket_count.
>
Fixed
>>
>> 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
>
> 2. This does not look like an error message. What is wrong here? Please,
> rephrase to make it clear what is wrong.
new msg
```
Non-dynamic option bucket_count cannot be reconfigured
```
>
>> +...
>>   _ = test_run:cmd("switch 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.
>
> 3. Typo.
>
>> +--
>> +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
>
> 4. See point 8 from the previous review. All the same.
fixed

Full diff:


commit fdc7fd274844ea5e7bae1e0a212154b29c909504
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..ab1808d 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 cannot be reconfigured
+...
  _ = 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..5685ccf 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.bucket_count = 
require('vshard.consts').DEFAULT_BUCKET_COUNT + 1
+---
+...
+util.check_error(vshard.storage.cfg, non_dynamic_cfg, names.storage_1_a)
+---
+- Non-dynamic option bucket_count cannot be reconfigured
+...
  _ = test_run:cmd("switch default")
  ---
  ...
diff --git a/test/storage/storage.test.lua b/test/storage/storage.test.lua
index f4bbf0e..72564e1 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.bucket_count = 
require('vshard.consts').DEFAULT_BUCKET_COUNT + 1
+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..904380b 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 cannot be reconfigured
+...
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..012060d 100644
--- a/vshard/cfg.lua
+++ b/vshard/cfg.lua
@@ -220,16 +220,34 @@ local cfg_template = {
      }},
  }

+--
+-- Names of options which cannot 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 shard_cfg[f_name] ~= old_cfg[f_name] then
+           error(string.format('Non-dynamic option %s ' ..
+                               'cannot be reconfigured', 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()