From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 29 Oct 2018 13:21:52 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH] box: fixed comparison of old and new config options Message-ID: <20181029102152.n7nseilxf7tormsb@esperanza> References: <20181024101431.32625-1-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181024101431.32625-1-arkholga@tarantool.org> To: Olga Arkhangelskaia Cc: tarantool-patches@freelists.org List-ID: 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')