[tarantool-patches] Re: [tarantool-patches] Re: [PATCH] box: fixed comparison of old and new config options

["Olga Arkhangelskaia" <arkholga@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 7416 bytes --]




29/10/2018 13:21, Vladimir Davydov пишет:
> On Wed, Oct 24, 2018 at 01:14:31PM +0300, Olga Arkhangelskaia wrote:
>> Due to incorrect configuration comparison, some options, especially
>> replication were reseted and restarted, despite that change did not
>> actually happened. The patch fixes it.
>>
>> Closes #3711
>> ---
>> Issue:
>>  https://github.com/tarantool/tarantool/issues/3711
>> Branch:
>>  https://github.com/tarantool/tarantool/tree/OKriw/gh-3711-do-not-restart-replication-if-config-did-not-change-2.1
>>
>>   src/box/lua/load_cfg.lua       |  43 +++++++++++++++--
>>   test/replication/misc.result   | 106 +++++++++++++++++++++++++++++++++++++++++
>>   test/replication/misc.test.lua |  38 +++++++++++++++
>>   3 files changed, 183 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
>> index df0c3c6ae..98d7ff47c 100644
>> --- a/src/box/lua/load_cfg.lua
>> +++ b/src/box/lua/load_cfg.lua
>> @@ -365,6 +365,41 @@ local function apply_default_cfg(cfg, default_cfg)
>>       end
>>   end
>> 
>> +-- Compare given  config with old one.
>> +-- Returns true if new config is similar to old one, false otherwise.
> Nit: empty space between 'given' and 'config'. Articles are missing.
> You should use 'equivalent' instead of 'similar' here.
ok
>
>> +local function compare_cfg(cfg1, cfg2)
>> +    if type(cfg1) ~= "table" and type(cfg2) ~= "table" then
>> +	return cfg1 == cfg2
>> +    end
>> +    if type(cfg1) == "table" and type(cfg2) ~= "table" then
>> +        if table.getn(cfg1) ~= 1 then
> table.getn is deprecated, you should use # instead
ok

>
>> +            return false
>> +        else
>> +            if type(cfg1[1]) ~= "nil" and type(cfg2) ~= "nil" then
>> +                return string.formagt(cfg1[1]) == string.format(cfg2)
>> +            else return cfg1[1] == cfg2 end
>> +        end
>> +    end
>> +    if type(cfg2) == "table" and type(cfg1) ~= "table" then
>> +	if table.getn(cfg2) ~= 1 then
>> +            return false
>> +        else
>> +            if type(cfg2[1]) ~= "nil" and type(cfg1) ~= "nil" then
>> +                return string.format(cfg2[1]) == string.format(cfg1)
>> +            else return cfg2[1] == cfg1 end
>> +        end
>> +    end
> I don't think you should interpret the table content anyhow here, i.e.
> convert values to string or treat a table of one element as a raw value,
> because although this is valid for box.cfg.replication, this may be not
> for other configuration options. This work should be done by modify_cfg.
You mean additionally check in modify_cfg check type of values in 
sub-tables of configuration?
Yes, it is valid only for replication, but as I have seen - there is no 
tables except replication config.
They may have appear in future. But we still need compare value from 
table and number sometimes.

>
>> +    if type(cfg1) == "table" and type(cfg2) == "table" then
>> +        if table.getn(cfg1) ~= table.getn(cfg2) then return false end
>> +        table.sort(cfg1)
>> +        table.sort(cfg2)
>> +        for key in pairs(cfg1) do
>> +	    if not compare_cfg(cfg1[key], cfg2[key]) then return false end
>> +        end
>> +	return true
> Bad indentation.
>
> Anyway, this is a rather peculiar way of comparing tables. What if a
> table contains subtables or string keys? How table.sort() is going to
> work then? Please google a correct way of comparing Lua tables.
At the moment I can't imagine situation when we have table for some 
configuration option, that consists of
subtables. Do you mean that some future changes may lead to such situations?
I used table.sort when we need to compare {1,2} and {2, 1}. Should we 
distinguish this configurations?

>
>> +    end
>> +end
>> +
>>   local function reload_cfg(oldcfg, cfg)
>>       cfg = upgrade_cfg(cfg, translate_cfg)
>>       local newcfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)
>> @@ -375,9 +410,9 @@ local function reload_cfg(oldcfg, cfg)
>>           end
>>       end
>>       for key in pairs(cfg) do
>> -        local val = newcfg[key]
>> -        local oldval = oldcfg[key]
>> -        if oldval ~= val then
>> +	if not compare_cfg(oldcfg[key], newcfg[key]) then
>> +            local val = newcfg[key]
>> +            local oldval = oldcfg[key]
> Please try to keep your diff as small as possible. Here you could simply
>
>           local val = newcfg[key]
>           local oldval = oldcfg[key]
> -        if oldval ~= val then
> +        if not compare_cfg(oldval, val) then
>               rawset(oldcfg, key, val)
>
> Also, you should use compare_cfg() in load_cfg() too.
Sorry, missed it.

>
>>               rawset(oldcfg, key, val)
>>               if not pcall(dynamic_cfg[key]) then
>>                   rawset(oldcfg, key, oldval) -- revert the old value
>> @@ -388,7 +423,7 @@ local function reload_cfg(oldcfg, cfg)
>>               end
>>               log.info("set '%s' configuration option to %s", key,
>>                   json.encode(val))
>> -        end
>> +        else log.info("option '%s' has not change", key) end
> We silently skipped unmodified options before. I don't think we need to
> log them now.
Will remove, it mostly make test easier for me.

>
>>       end
>>       if type(box.on_reload_configuration) == 'function' then
>>           box.on_reload_configuration()
>> diff --git a/test/replication/misc.test.lua b/test/replication/misc.test.lua
>> index 46726b7f4..96274b263 100644
>> --- a/test/replication/misc.test.lua
>> +++ b/test/replication/misc.test.lua
>> @@ -191,3 +191,41 @@ test_run:cmd("stop server replica")
>>   test_run:cmd("cleanup server replica")
>>   test_run:cmd("delete server replica")
>>   box.schema.user.revoke('guest', 'replication')
>> +
>> +--
>> +-- gh-3711 Do not restart replication on box.cfg if the configuration didn't change
>> +--
>> +box.schema.user.grant('guest', 'replication')
>> +
>> +test_run:cmd("create server replica with rpl_master=default, script='replication/replica.lua'")
>> +test_run:cmd("start server replica")
>> +test_run:cmd("switch replica")
>> +
>> +test_run:grep_log('replica', 'replication')
>> +replication = box.cfg.replication
>> +box.cfg{replication = {replication}}
>> +test_run:grep_log('replica', 'has not change') ~= nil
>> +test_run:grep_log('replica', "set 'replication' configuration option to") == nil
>> +box.cfg{replication = replication}
>> +test_run:grep_log('replica', "set 'replication' configuration option to") == nil
> There's a better way to check if this works: revoke replication grants
> on the master and try to reconfigure replication on the replica.
Will try, thanks.

>
>> +
>> +test_run:cmd("switch default")
>> +test_run:cmd('stop server replica')
>> +test_run:cmd('cleanup server test')
>> +test_run:cmd("start server replica")
>> +test_run:cmd("switch replica")
>> +
>> +listen = box.cfg.listen
>> +replication = box.cfg.replication
>> +
>> +box.cfg{replication = {replication, listen}}
>> +test_run:grep_log('replica', "set 'replication' configuration option to")
>> +test_run:grep_log('replica', "has not change") == nil
>> +box.cfg{replication = {listen, replication}}
>> +test_run:grep_log('replica', "has not change") ~= nil
>> +
>> +test_run:cmd("switch default")
>> +test_run:cmd("stop server replica")
>> +test_run:cmd("cleanup server replica")
>> +test_run:cmd("delete server replica")
>> +box.schema.user.revoke('guest', 'replication')


[-- Attachment #2: Type: text/html, Size: 9586 bytes --]