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

[Roman Khabibov <roman.habibov@tarantool.org>]

Alexander, can you check it6 please?

> On Dec 2, 2020, at 18:25, roman via Tarantool-patches <tarantool-patches@dev.tarantool.org> wrote:
> 
> Hi! Thanks for LGTM.
> 
> Kirill, you can push it.
> 
> On 02.12.2020 12:17, Serge Petrenko wrote:
>> 
>> 02.12.2020 03:16, Roman Khabibov пишет:
>>> Hi! Thanks for the review.
>>>> On Dec 1, 2020, at 12:58, Serge Petrenko <sergepetrenko@tarantool.org> wrote:
>>>> 
>>>> 
>>>> 30.11.2020 16:43, Roman Khabibov пишет:
>>>>> Thanks.
>>>>> 
>>>>> Serge, could you, please, look through the patch? 
>>>> 
>>>> Hi! Thanks for the patch!
>>>> 
>>>> Since you're working on this instead of Sergey now, you may add yourself to
>>>> 
>>>> the Co-developed-by field in the commit message, like it is done here:
>>>> 
>>>> https://github.com/tarantool/tarantool/commit/cfccfd449c890c18615185ba4895d9081e50c318 
>>>> 
>>>> 
>>>> Please see 3 more comments below.
>>>> 
>>>>> commit 8b3265c1599772f5a85e47ed1a8232571ec23f8d
>>>>> Author: Sergey Voinov <sergeiv@tarantool.org>
>>>>> Date:   Wed Dec 11 17:28:39 2019 +0300
>>>>> 
>>>>>      box: check schema version after tarantool update
>>>>>           Check schema version (stored in box.space._schema) on start and
>>>>>      print a warning if it doesn't match last available schema version.
>>>>>      It is needed because 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
>>>>> 
>>>>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
>>>>> index 76e2e92c2..451247dcf 100644
>>>>> --- a/src/box/lua/load_cfg.lua
>>>>> +++ b/src/box/lua/load_cfg.lua
>>>>> @@ -702,6 +702,22 @@ 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'} 
>>>> 
>>>> 1. Version unused. You get schema version in `schema_needs_upgrade()` anyway.
>>>> 
>>>>     Also you may omit testing for nil here. You may just test schema version
>>>>     inside `schema_needs_upgrade()` and simply return false, if it is nil.
>>>> 
>>>> 
>>>>     You'll also need to update test/box/stat.result after this is done. 
>>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
>>> index 76e2e92c2..770442052 100644
>>> --- a/src/box/lua/load_cfg.lua
>>> +++ b/src/box/lua/load_cfg.lua
>>> @@ -702,6 +702,18 @@ 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 needs, schema_version_str = private.schema_needs_upgrade()
>>> +    if needs then
>>> +        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_str, box.info.version)
>>> +        log.warn(msg)
>>> +    end
>>>   end
>>>   box.cfg = locked(load_cfg)
>>>>> +    if version ~= nil then
>>>>> +        local needs, schema_version_str = private.schema_needs_upgrade()
>>>>> +        local tarantool_version_str = box.info.version
>>>>> +        if needs 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_str, tarantool_version_str)
>>>>> +            log.warn(msg)
>>>>> +        end
>>>>> +    end
>>>>>   end
>>>>>   box.cfg = locked(load_cfg)
>>>>>   diff --git a/test/box/cfg.test.lua b/test/box/cfg.test.lua
>>>>> index 56018b1a0..e806c9efe 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/"') 
>>>> 
>>>> 2. Can you reuse `cfg_test1.lua` here? 
>>> No, I need to have "read_only = true”. 
>> 
>> 
>> Oh, now I see. So that the instance doesn't auto-upgrade.
>> 
>> Thanks for the fixes! LGTM.
>> 
>>>>> +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 
>>>> 
>>>> 3. Better use `wait_log` instead of `grep_log`. It's not guaranteed that the
>>>>     server will print this message by the time you grep for it. 
>>> Done.
>>>>