Re: [Tarantool-patches] [PATCH] box: check schema version after tarantool update

["Alexander V. Tikhonov" <avtikhon@tarantool.org>]

Hi, thanks for the patch, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline [1], patch LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/223982512

On Mon, Nov 16, 2020 at 02:54:16AM +0300, Roman Khabibov wrote:
> From: Sergey Voinov <sergeiv@tarantool.org>
> 
> Check schema version (stored in box.space._schema) on start and
> print a warning if it doesn't match last avaliable schema version.
> It is needed bcause some users forget to call box.schema.upgrade()
> after Tarantool update and get stuck with an old schema version
> until they encounter some hard to debug problems.
> 
> Closes #4574
> ---
> 
> @ChangeLog: Print log warning if schema version is older than last available schema version (gh-4574).
> 
> Branch: https://github.com/tarantool/tarantool/tree/servoin/gh-4574-schema-version
> Issue: https://github.com/tarantool/tarantool/issues/4574
> 
>  src/box/lua/load_cfg.lua   | 19 +++++++++++++++
>  src/box/lua/upgrade.lua    | 36 ++++++++++++++++-----------
>  test/box/cfg.result        | 50 ++++++++++++++++++++++++++++++++++++++
>  test/box/cfg.test.lua      | 18 ++++++++++++++
>  test/box/lua/cfg_test8.lua |  9 +++++++
>  test/box/lua/cfg_test9.lua |  9 +++++++
>  test/box/stat.result       | 16 ++++++------
>  7 files changed, 135 insertions(+), 22 deletions(-)
>  create mode 100644 test/box/lua/cfg_test8.lua
>  create mode 100644 test/box/lua/cfg_test9.lua
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 76e2e92c2..13088fe73 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -702,6 +702,25 @@ local function load_cfg(cfg)
>      box_configured = nil
>  
>      box_is_configured = true
> +
> +    -- Check if schema version matches Tarantool version
> +    -- and print warning if it's not (in case user forgot to call box.schema.upgrade())
> +    local version = box.space._schema:get{'version'}
> +    if version ~= nil then
> +        local major = version[2]
> +        local minor = version[3]
> +        local patch = version[4] or 0
> +        local schema_version = string.format("%s.%s.%s", major, minor, patch)
> +        local tarantool_version = box.info.version
> +        if private.schema_needs_upgrade() then
> +            -- Print the warning
> +            local msg = string.format(
> +                'Your schema version is %s while Tarantool %s requires a more'..
> +                ' recent schema version. Please, consider using '..
> +                'box.schema.upgrade().', schema_version, tarantool_version)
> +            log.warn(msg)
> +        end
> +    end
>  end
>  box.cfg = locked(load_cfg)
>  
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index add791cd7..5ea2fb23d 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -973,6 +973,21 @@ end
>  
>  --------------------------------------------------------------------------------
>  
> +local handlers = {
> +    {version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = true},
> +    {version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true},
> +    {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true},
> +    {version = mkversion(1, 10, 2), func = upgrade_to_1_10_2, auto = true},
> +    {version = mkversion(2, 1, 0), func = upgrade_to_2_1_0, auto = true},
> +    {version = mkversion(2, 1, 1), func = upgrade_to_2_1_1, auto = true},
> +    {version = mkversion(2, 1, 2), func = upgrade_to_2_1_2, auto = true},
> +    {version = mkversion(2, 1, 3), func = upgrade_to_2_1_3, auto = true},
> +    {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
> +    {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
> +    {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
> +}
> +
> +-- Schema version of the snapshot.
>  local function get_version()
>      local version = box.space._schema:get{'version'}
>      if version == nil then
> @@ -985,6 +1000,12 @@ local function get_version()
>      return mkversion(major, minor, patch)
>  end
>  
> +local function schema_needs_upgrade()
> +    -- Schema needs upgrade if current schema version is greater
> +    -- than schema version of the snapshot.
> +    return handlers[#handlers].version > get_version()
> +end
> +
>  local function upgrade(options)
>      options = options or {}
>      setmetatable(options, {__index = {auto = false}})
> @@ -995,20 +1016,6 @@ local function upgrade(options)
>          return
>      end
>  
> -    local handlers = {
> -        {version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = true},
> -        {version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true},
> -        {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true},
> -        {version = mkversion(1, 10, 2), func = upgrade_to_1_10_2, auto = true},
> -        {version = mkversion(2, 1, 0), func = upgrade_to_2_1_0, auto = true},
> -        {version = mkversion(2, 1, 1), func = upgrade_to_2_1_1, auto = true},
> -        {version = mkversion(2, 1, 2), func = upgrade_to_2_1_2, auto = true},
> -        {version = mkversion(2, 1, 3), func = upgrade_to_2_1_3, auto = true},
> -        {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true},
> -        {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true},
> -        {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true},
> -    }
> -
>      for _, handler in ipairs(handlers) do
>          if version >= handler.version then
>              goto continue
> @@ -1047,3 +1054,4 @@ end
>  
>  box.schema.upgrade = upgrade;
>  box.internal.bootstrap = bootstrap;
> +box.internal.schema_needs_upgrade = schema_needs_upgrade;
> diff --git a/test/box/cfg.result b/test/box/cfg.result
> index 4ad3c6493..676324662 100644
> --- a/test/box/cfg.result
> +++ b/test/box/cfg.result
> @@ -656,3 +656,53 @@ test_run:cmd("cleanup server cfg_tester7")
>   | ---
>   | - true
>   | ...
> +
> +--
> +-- gh-4574: Check schema version after Tarantool update.
> +--
> +test_run:cmd('create server cfg_tester8 with script = "box/lua/cfg_test8.lua", workdir="sql/upgrade/2.1.0/"')
> + | ---
> + | - true
> + | ...
> +test_run:cmd("start server cfg_tester8")
> + | ---
> + | - true
> + | ...
> +--- Check that the warning is printed.
> +version_warning = "Please, consider using box.schema.upgrade()."
> + | ---
> + | ...
> +test_run:grep_log('cfg_tester8', version_warning, 1000) ~= nil
> + | ---
> + | - true
> + | ...
> +test_run:cmd("stop server cfg_tester8")
> + | ---
> + | - true
> + | ...
> +test_run:cmd("cleanup server cfg_tester8")
> + | ---
> + | - true
> + | ...
> +
> +test_run:cmd('create server cfg_tester9 with script = "box/lua/cfg_test9.lua"')
> + | ---
> + | - true
> + | ...
> +test_run:cmd("start server cfg_tester9")
> + | ---
> + | - true
> + | ...
> +--- Check that the warning isn't printed.
> +test_run:grep_log('cfg_tester9', version_warning, 1000) == nil
> + | ---
> + | - true
> + | ...
> +test_run:cmd("stop server cfg_tester9")
> + | ---
> + | - true
> + | ...
> +test_run:cmd("cleanup server cfg_tester9")
> + | ---
> + | - true
> + | ...
> diff --git a/test/box/cfg.test.lua b/test/box/cfg.test.lua
> index 56018b1a0..015af1a91 100644
> --- a/test/box/cfg.test.lua
> +++ b/test/box/cfg.test.lua
> @@ -159,3 +159,21 @@ test_run:grep_log('cfg_tester7', 'set \'replication\' configuration option to',
>  test_run:grep_log('cfg_tester7', 'test%-cluster%-cookie', 1000)
>  test_run:cmd("stop server cfg_tester7")
>  test_run:cmd("cleanup server cfg_tester7")
> +
> +--
> +-- gh-4574: Check schema version after Tarantool update.
> +--
> +test_run:cmd('create server cfg_tester8 with script = "box/lua/cfg_test8.lua", workdir="sql/upgrade/2.1.0/"')
> +test_run:cmd("start server cfg_tester8")
> +--- Check that the warning is printed.
> +version_warning = "Please, consider using box.schema.upgrade()."
> +test_run:grep_log('cfg_tester8', version_warning, 1000) ~= nil
> +test_run:cmd("stop server cfg_tester8")
> +test_run:cmd("cleanup server cfg_tester8")
> +
> +test_run:cmd('create server cfg_tester9 with script = "box/lua/cfg_test9.lua"')
> +test_run:cmd("start server cfg_tester9")
> +--- Check that the warning isn't printed.
> +test_run:grep_log('cfg_tester9', version_warning, 1000) == nil
> +test_run:cmd("stop server cfg_tester9")
> +test_run:cmd("cleanup server cfg_tester9")
> diff --git a/test/box/lua/cfg_test8.lua b/test/box/lua/cfg_test8.lua
> new file mode 100644
> index 000000000..c61b86ae3
> --- /dev/null
> +++ b/test/box/lua/cfg_test8.lua
> @@ -0,0 +1,9 @@
> +#!/usr/bin/env tarantool
> +os = require('os')
> +
> +box.cfg{
> +    listen = os.getenv("LISTEN"),
> +    read_only = true
> +}
> +
> +require('console').listen(os.getenv('ADMIN'))
> diff --git a/test/box/lua/cfg_test9.lua b/test/box/lua/cfg_test9.lua
> new file mode 100644
> index 000000000..fa8b303f1
> --- /dev/null
> +++ b/test/box/lua/cfg_test9.lua
> @@ -0,0 +1,9 @@
> +#!/usr/bin/env tarantool
> +os = require('os')
> +
> +box.cfg{
> +    listen = os.getenv("LISTEN"),
> +    read_only = false
> +}
> +
> +require('console').listen(os.getenv('ADMIN'))
> diff --git a/test/box/stat.result b/test/box/stat.result
> index 55f29fe59..e808678eb 100644
> --- a/test/box/stat.result
> +++ b/test/box/stat.result
> @@ -24,7 +24,7 @@ box.stat.REPLACE.total
>  ...
>  box.stat.SELECT.total
>  ---
> -- 1
> +- 3
>  ...
>  box.stat.ERROR.total
>  ---
> @@ -59,7 +59,7 @@ box.stat.REPLACE.total
>  ...
>  box.stat.SELECT.total
>  ---
> -- 5
> +- 7
>  ...
>  -- check exceptions
>  space:get('Impossible value')
> @@ -77,14 +77,14 @@ space:get(1)
>  ...
>  box.stat.SELECT.total
>  ---
> -- 6
> +- 8
>  ...
>  space:get(11)
>  ---
>  ...
>  box.stat.SELECT.total
>  ---
> -- 7
> +- 9
>  ...
>  space:select(5)
>  ---
> @@ -92,7 +92,7 @@ space:select(5)
>  ...
>  box.stat.SELECT.total
>  ---
> -- 8
> +- 10
>  ...
>  space:select(15)
>  ---
> @@ -100,14 +100,14 @@ space:select(15)
>  ...
>  box.stat.SELECT.total
>  ---
> -- 9
> +- 11
>  ...
>  for _ in space:pairs() do end
>  ---
>  ...
>  box.stat.SELECT.total
>  ---
> -- 10
> +- 12
>  ...
>  -- reset
>  box.stat.reset()
> @@ -157,7 +157,7 @@ box.stat.REPLACE.total
>  ...
>  box.stat.SELECT.total
>  ---
> -- 1
> +- 3
>  ...
>  box.stat.ERROR.total
>  ---
> -- 
> 2.24.3 (Apple Git-128)
>