From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B8F7323C13 for ; Mon, 25 Jun 2018 07:48:58 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id RAkRwoYu4JW7 for ; Mon, 25 Jun 2018 07:48:58 -0400 (EDT) Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id C23CA23AFB for ; Mon, 25 Jun 2018 07:48:57 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH][vshard] Prohibit reconfigure non-dynamic options References: <20180615140225.2576-1-avkhatskevich@tarantool.org> <92d28edf-8237-c499-3ff0-e9795d8ff845@tarantool.org> From: Alex Khatskevich Message-ID: Date: Mon, 25 Jun 2018 14:48:53 +0300 MIME-Version: 1.0 In-Reply-To: <92d28edf-8237-c499-3ff0-e9795d8ff845@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Vladislav Shpilevoy , tarantool-patches@freelists.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 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()