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

[Sergey Ostanevich <sergos@tarantool.org>]

And two misprints.

> On 24 Nov 2020, at 15:11, Sergey Ostanevich <sergos@tarantool.org> wrote:
> 
> Hi!
> 
> Thanks for the patch!
> See my 2 comments inline.
> 
> Sergos
> 
> 
>> On 16 Nov 2020, at 02:54, Roman Khabibov <roman.habibov@tarantool.org> 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.
Misprint                                    ^^^^^^^^^
>> It is needed bcause some users forget to call box.schema.upgrade()
Another one     ^^^^^^
>> 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())
> 
> 1. The code starting from here 
>> +    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
> 
> to here doesn’t needed, as you do exactly the same inside the schema_needs_upgrade()
> 
>> +        if private.schema_needs_upgrade() then
>> +            -- Print the warning
>> +            local msg = string.format(
> 
> 1.1 The returning value of schema_needs_upgrade() may contain the expected upgrade version,
> so you can reuse it for the message itself.
>> +                '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()
> 
> 1.2 Update the return value here.
> 
>> +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”’)
> 
> 2. You can reuse the cfg_test1.lua for the writable node, no need in extra file.
> 
>> +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)