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

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

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.
>>>