* [tarantool-patches] [PATCH] box: fixed comparison of old and new config options
@ 2018-10-24 10:14 Olga Arkhangelskaia
2018-10-29 10:21 ` Vladimir Davydov
0 siblings, 1 reply; 2+ messages in thread
From: Olga Arkhangelskaia @ 2018-10-24 10:14 UTC (permalink / raw)
To: tarantool-patches; +Cc: Olga Arkhangelskaia
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.
+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
+ 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
+ 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
+ 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]
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
end
if type(box.on_reload_configuration) == 'function' then
box.on_reload_configuration()
diff --git a/test/replication/misc.result b/test/replication/misc.result
index f8aa8dab6..c8e50d8f9 100644
--- a/test/replication/misc.result
+++ b/test/replication/misc.result
@@ -467,3 +467,109 @@ 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'")
+---
+- true
+...
+test_run:cmd("start server replica")
+---
+- true
+...
+test_run:cmd("switch replica")
+---
+- true
+...
+test_run:grep_log('replica', 'replication')
+---
+- replication
+...
+replication = box.cfg.replication
+---
+...
+box.cfg{replication = {replication}}
+---
+...
+test_run:grep_log('replica', 'has not change') ~= nil
+---
+- true
+...
+test_run:grep_log('replica', "set 'replication' configuration option to") == nil
+---
+- true
+...
+box.cfg{replication = replication}
+---
+...
+test_run:grep_log('replica', "set 'replication' configuration option to") == nil
+---
+- true
+...
+test_run:cmd("switch default")
+---
+- true
+...
+test_run:cmd('stop server replica')
+---
+- true
+...
+test_run:cmd('cleanup server test')
+---
+- true
+...
+test_run:cmd("start server replica")
+---
+- true
+...
+test_run:cmd("switch replica")
+---
+- true
+...
+listen = box.cfg.listen
+---
+...
+replication = box.cfg.replication
+---
+...
+box.cfg{replication = {replication, listen}}
+---
+...
+test_run:grep_log('replica', "set 'replication' configuration option to")
+---
+- set 'replication' configuration option to
+...
+test_run:grep_log('replica', "has not change") == nil
+---
+- true
+...
+box.cfg{replication = {listen, replication}}
+---
+...
+test_run:grep_log('replica', "has not change") ~= nil
+---
+- true
+...
+test_run:cmd("switch default")
+---
+- true
+...
+test_run:cmd("stop server replica")
+---
+- true
+...
+test_run:cmd("cleanup server replica")
+---
+- true
+...
+test_run:cmd("delete server replica")
+---
+- true
+...
+box.schema.user.revoke('guest', 'replication')
+---
+...
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
+
+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')
--
2.14.3 (Apple Git-98)
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [tarantool-patches] [PATCH] box: fixed comparison of old and new config options
2018-10-24 10:14 [tarantool-patches] [PATCH] box: fixed comparison of old and new config options Olga Arkhangelskaia
@ 2018-10-29 10:21 ` Vladimir Davydov
0 siblings, 0 replies; 2+ messages in thread
From: Vladimir Davydov @ 2018-10-29 10:21 UTC (permalink / raw)
To: Olga Arkhangelskaia; +Cc: tarantool-patches
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.
> +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
> + 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.
> + 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.
> + 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.
> 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.
> 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.
> +
> +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')
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-10-29 10:21 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-24 10:14 [tarantool-patches] [PATCH] box: fixed comparison of old and new config options Olga Arkhangelskaia
2018-10-29 10:21 ` Vladimir Davydov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox