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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes!

The tests hangs on Travis (and locally too).

On 26/06/2018 18:24, Alex Khatskevich wrote:
> 
> 
> 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()
>