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

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

[Olga Arkhangelskaia]

box.cfg() updates only those options that have actually changed.
However, for replication it is not always true: box.cfg{replication = x}
and box.cfg{replication = {x}} are treated differently, and as
the result - replication is restarted. The patch fixes such behaviour.

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

v1:
https://www.freelists.org/post/tarantool-patches/PATCH-box-fixed-comparison-of-old-and-new-config-options

v2:
https://www.freelists.org/post/tarantool-patches/PATCH-v2-box-fixed-comparison-of-old-and-new-config-options

Changes in v2:
- changed test
- conversion from num to string now in modify_cfg
- no sort table in comparison
- got rid of table.getn

Changes in v3:
- changed test
- fixed comments, names, identation
- added normalize_uri_list

 src/box/lua/load_cfg.lua       |  39 +++++++++++++--
 test/replication/misc.result   | 106 +++++++++++++++++++++++++++++++++++++++++
 test/replication/misc.test.lua |  41 ++++++++++++++++
 3 files changed, 183 insertions(+), 3 deletions(-)

diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index df0c3c6ae..0dcb62f26 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -151,10 +151,25 @@ local function normalize_uri(port)
     return tostring(port);
 end
 
+local function normalize_uri_list(port)
+    if port == nil then
+        return port
+    end
+    if type(port) == 'table' then
+        if #port == 1 then
+            return tostring(port[1])
+        end
+        for i in pairs(port) do
+            port[i] = tostring(port[i])
+        end
+        return port
+    end
+    return tostring(port);
+end
 -- options that require special handling
 local modify_cfg = {
     listen             = normalize_uri,
-    replication        = normalize_uri,
+    replication        = normalize_uri_list,
 }
 
 local function purge_password_from_uri(uri)
@@ -365,6 +380,24 @@ local function apply_default_cfg(cfg, default_cfg)
     end
 end
 
+-- Return true if two configurations are equivalent.
+local function compare_cfg(t1, t2)
+    if type(t1) ~= 'table' and type(t2) ~= 'table' then
+        return t1 == t2
+    end
+    if type(t1) == 'table' and type(t2) == 'table' then
+        if #t1 ~= #t2 then
+            return false
+        end
+        for i, j in pairs(t2) do
+            if t1[i] ~= j 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)
@@ -377,7 +410,7 @@ local function reload_cfg(oldcfg, cfg)
     for key in pairs(cfg) do
         local val = newcfg[key]
         local oldval = oldcfg[key]
-        if oldval ~= val then
+        if not compare_cfg(val, oldval) then
             rawset(oldcfg, key, val)
             if not pcall(dynamic_cfg[key]) then
                 rawset(oldcfg, key, oldval) -- revert the old value
@@ -452,7 +485,7 @@ local function load_cfg(cfg)
         local val = cfg[key]
         if val ~= nil and not dynamic_cfg_skip_at_load[key] then
             fun()
-            if val ~= default_cfg[key] then
+            if not compare_cfg(val, default_cfg[key]) then
                 log.info("set '%s' configuration option to %s", key, json.encode(val))
             end
         end
diff --git a/test/replication/misc.result b/test/replication/misc.result
index f8aa8dab6..c9de813da 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
+...
+replication = box.cfg.replication
+---
+...
+test_run:cmd("switch default")
+---
+- true
+...
+-- Access rights are checked only during reconnect. If new config is equivalent
+-- to old one, replication will continue working.
+box.schema.user.revoke('guest', 'replication')
+---
+...
+test_run:cmd("switch replica")
+---
+- true
+...
+box.cfg{replication = {replication}}
+---
+...
+box.info.status == 'running'
+---
+- true
+...
+box.cfg{replication = replication}
+---
+...
+box.info.status == 'running'
+---
+- true
+...
+-- check table
+test_run:cmd("switch default")
+---
+- true
+...
+box.schema.user.grant('guest', 'replication')
+---
+...
+test_run:cmd("switch replica")
+---
+- true
+...
+listen = box.cfg.replication
+---
+...
+a = string.split(listen, ':')
+---
+...
+self = string.format('localhost:%s', a[2] + 1)
+---
+...
+box.cfg{listen = self, replication = {self, replication}}
+---
+...
+test_run:cmd("switch default")
+---
+- true
+...
+box.schema.user.revoke('guest', 'replication')
+---
+...
+test_run:cmd("switch replica")
+---
+- true
+...
+box.cfg{replication = {self, replication}}
+---
+...
+box.info.status == 'running'
+---
+- 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
+...
diff --git a/test/replication/misc.test.lua b/test/replication/misc.test.lua
index 46726b7f4..bf7408e96 100644
--- a/test/replication/misc.test.lua
+++ b/test/replication/misc.test.lua
@@ -191,3 +191,44 @@ 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")
+replication = box.cfg.replication
+test_run:cmd("switch default")
+-- Access rights are checked only during reconnect. If new config is equivalent
+-- to old one, replication will continue working.
+box.schema.user.revoke('guest', 'replication')
+test_run:cmd("switch replica")
+box.cfg{replication = {replication}}
+box.info.status == 'running'
+box.cfg{replication = replication}
+box.info.status == 'running'
+
+-- check table
+test_run:cmd("switch default")
+box.schema.user.grant('guest', 'replication')
+test_run:cmd("switch replica")
+listen = box.cfg.replication
+a = string.split(listen, ':')
+self = string.format('localhost:%s', a[2] + 1)
+box.cfg{listen = self, replication = {self, replication}}
+
+test_run:cmd("switch default")
+box.schema.user.revoke('guest', 'replication')
+
+test_run:cmd("switch replica")
+box.cfg{replication = {self, replication}}
+box.info.status == 'running'
+
+test_run:cmd("switch default")
+test_run:cmd("stop server replica")
+test_run:cmd("cleanup server replica")
+test_run:cmd("delete server replica")
-- 
2.14.3 (Apple Git-98)

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

[Vladimir Davydov]

On Wed, Oct 31, 2018 at 02:32:14PM +0300, Olga Arkhangelskaia wrote:
> box.cfg() updates only those options that have actually changed.
> However, for replication it is not always true: box.cfg{replication = x}
> and box.cfg{replication = {x}} are treated differently, and as
> the result - replication is restarted. The patch fixes such behaviour.
> 
> 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

Please rebase on top of 1.10.

> 
> v1:
> https://www.freelists.org/post/tarantool-patches/PATCH-box-fixed-comparison-of-old-and-new-config-options
> 
> v2:
> https://www.freelists.org/post/tarantool-patches/PATCH-v2-box-fixed-comparison-of-old-and-new-config-options
> 
> Changes in v2:
> - changed test
> - conversion from num to string now in modify_cfg
> - no sort table in comparison
> - got rid of table.getn
> 
> Changes in v3:
> - changed test
> - fixed comments, names, identation
> - added normalize_uri_list
> 
>  src/box/lua/load_cfg.lua       |  39 +++++++++++++--
>  test/replication/misc.result   | 106 +++++++++++++++++++++++++++++++++++++++++
>  test/replication/misc.test.lua |  41 ++++++++++++++++
>  3 files changed, 183 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index df0c3c6ae..0dcb62f26 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -151,10 +151,25 @@ local function normalize_uri(port)
>      return tostring(port);
>  end
>  
> +local function normalize_uri_list(port)

The argument name is confusing. It's not a port, it's a list of URIs.

> +    if port == nil then
> +        return port
> +    end
> +    if type(port) == 'table' then
> +        if #port == 1 then
> +            return tostring(port[1])
> +        end

I'd expect normalize_uri_list() to always return a Lua table, i.e.
it should turn 12345 into {'12345'} not vice versa.

> +        for i in pairs(port) do
> +            port[i] = tostring(port[i])

Modifying a table in-place isn't a good idea, because this might affect
a user defined variable, e.g.

  replication = {1234, 5678}
  box.cfg{replication = replication}
  print(type(replication[1])) -- prints 'string' instead of 'number'

Also, I think that instead of tostring() we should use normalize_uri()
here, to emphasize that normalize_uri_list() normalizes each URI in a
table.

> +        end
> +        return port
> +    end
> +    return tostring(port);
> +end

New line missing.

>  -- options that require special handling
>  local modify_cfg = {
>      listen             = normalize_uri,
> -    replication        = normalize_uri,
> +    replication        = normalize_uri_list,
>  }
>  
>  local function purge_password_from_uri(uri)
> @@ -365,6 +380,24 @@ local function apply_default_cfg(cfg, default_cfg)
>      end
>  end
>  
> +-- Return true if two configurations are equivalent.
> +local function compare_cfg(t1, t2)
> +    if type(t1) ~= 'table' and type(t2) ~= 'table' then
> +        return t1 == t2
> +    end
> +    if type(t1) == 'table' and type(t2) == 'table' then
> +        if #t1 ~= #t2 then
> +            return false
> +        end
> +        for i, j in pairs(t2) do
> +            if t1[i] ~= j then
> +                return false
> +            end
> +        end
> +        return true
> +    end

No return value in case t1 is a table and t2 is not or vice versa.

> +end
> +
>  local function reload_cfg(oldcfg, cfg)
>      cfg = upgrade_cfg(cfg, translate_cfg)
>      local newcfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg)
> diff --git a/test/replication/misc.test.lua b/test/replication/misc.test.lua
> index 46726b7f4..bf7408e96 100644
> --- a/test/replication/misc.test.lua
> +++ b/test/replication/misc.test.lua
> @@ -191,3 +191,44 @@ 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")
> +replication = box.cfg.replication
> +test_run:cmd("switch default")
> +-- Access rights are checked only during reconnect. If new config is equivalent
> +-- to old one, replication will continue working.
> +box.schema.user.revoke('guest', 'replication')
> +test_run:cmd("switch replica")
> +box.cfg{replication = {replication}}
> +box.info.status == 'running'
> +box.cfg{replication = replication}
> +box.info.status == 'running'
> +
> +-- check table
> +test_run:cmd("switch default")
> +box.schema.user.grant('guest', 'replication')
> +test_run:cmd("switch replica")
> +listen = box.cfg.replication
> +a = string.split(listen, ':')
> +self = string.format('localhost:%s', a[2] + 1)
> +box.cfg{listen = self, replication = {self, replication}}

This doesn't work on Travis CI, check out

https://travis-ci.org/tarantool/tarantool/builds/448792930?utm_source=github_status&utm_medium=notification

You can simply use box.cfg.listen instead:

replication = box.cfg.replication
table.insert(replication, box.cfg.listen)

> +
> +test_run:cmd("switch default")
> +box.schema.user.revoke('guest', 'replication')
> +
> +test_run:cmd("switch replica")
> +box.cfg{replication = {self, replication}}
> +box.info.status == 'running'
> +
> +test_run:cmd("switch default")
> +test_run:cmd("stop server replica")
> +test_run:cmd("cleanup server replica")
> +test_run:cmd("delete server replica")