[tarantool-patches] Re: [PATCH 3/3] Introduce storage reload evolution

[Vladislav Shpilevoy]

Thanks for the patch! See 17 comments below.

On 18/07/2018 20:47, AKhatskevich wrote:
> Changes:
> 1. Introduce storage reload evolution.
> 2. Setup cross-version reload testing.
> 
> 1:
> This mechanism updates Lua objects on reload in case they are
> changed in a new vshard.storage version.
> 
> Since this commit, any change in vshard.storage.M has to be
> reflected in vshard.storage.reload_evolution to guarantee
> correct reload.
> 
> 2:
> The testing uses git infrastructure and is performed in the following
> way:
> 1. Copy old version of vshard to a temp folder.
> 2. Run vshard on this code.
> 3. Checkout the latest version of the vshard sources.
> 4. Reload vshard storage.
> 5. Make sure it works (Perform simple tests).
> 
> Notes:
> * this patch contains some legacy-driven decisions:
>    1. SOURCEDIR path retrieved differentpy in case of

1. Typo: differentpy.

>       packpack build.
>    2. git directory in the `reload_evolution/storage` test
>       is copied with respect to Centos 7 and `ro` mode of
>       SOURCEDIR.
> 
> Closes <shut git>112 125

2. Stray 'shut'.

> ---
>   .travis.yml                            |   2 +-
>   rpm/prebuild.sh                        |   2 +
>   test/lua_libs/git_util.lua             |  39 +++++++
>   test/lua_libs/util.lua                 |  20 ++++
>   test/reload_evolution/storage.result   | 184 +++++++++++++++++++++++++++++++++
>   test/reload_evolution/storage.test.lua |  64 ++++++++++++
>   test/reload_evolution/storage_1_a.lua  | 144 ++++++++++++++++++++++++++
>   test/reload_evolution/storage_1_b.lua  |   1 +
>   test/reload_evolution/storage_2_a.lua  |   1 +
>   test/reload_evolution/storage_2_b.lua  |   1 +
>   test/reload_evolution/suite.ini        |   6 ++
>   test/reload_evolution/test.lua         |   9 ++
>   vshard/storage/init.lua                |  11 ++
>   vshard/storage/reload_evolution.lua    |  58 +++++++++++
>   14 files changed, 541 insertions(+), 1 deletion(-)
>   create mode 100644 test/lua_libs/git_util.lua
>   create mode 100644 test/reload_evolution/storage.result
>   create mode 100644 test/reload_evolution/storage.test.lua
>   create mode 100755 test/reload_evolution/storage_1_a.lua
>   create mode 120000 test/reload_evolution/storage_1_b.lua
>   create mode 120000 test/reload_evolution/storage_2_a.lua
>   create mode 120000 test/reload_evolution/storage_2_b.lua
>   create mode 100644 test/reload_evolution/suite.ini
>   create mode 100644 test/reload_evolution/test.lua
>   create mode 100644 vshard/storage/reload_evolution.lua
> 
> diff --git a/rpm/prebuild.sh b/rpm/prebuild.sh
> index 768b22b..554032b 100755
> --- a/rpm/prebuild.sh
> +++ b/rpm/prebuild.sh
> @@ -1 +1,3 @@
>   curl -s https://packagecloud.io/install/repositories/tarantool/1_9/script.rpm.sh | sudo bash
> +sudo yum -y install python-devel python-pip
> +sudo pip install tarantool msgpack

3. Why do you need it? As I understand, 'curl' above installs all
the needed things. Besides, pip install downloads not necessary
Tarantool 1.9.

> diff --git a/test/lua_libs/git_util.lua b/test/lua_libs/git_util.lua
> new file mode 100644
> index 0000000..e2c17d0
> --- /dev/null
> +++ b/test/lua_libs/git_util.lua
> @@ -0,0 +1,39 @@
> +--
> +-- Lua bridge for some of the git commands.
> +--
> +local os = require('os')
> +
> +local temp_file = 'some_strange_rare_unique_file_name_for_git_util'
> +local function exec_cmd(options, cmd, args, files, dir, fout)

4. You can remove 'options' arg (it is always '' as I see).

> +    files = files or ''
> +    options = options or ''
> +    args = args or ''

5. files, args, options are always non-nil.

> +    local shell_cmd
> +    shell_cmd = string.format('git %s %s %s %s', options, cmd, args, files)

6. Why do you need to announce shell_cmd? Just do

     local shell_cmd = ...

> +    if fout then

7. 'fout' is always nil.

> +        shell_cmd = shell_cmd .. ' >' .. fout
> +    end
> +    if dir then

8. 'dir' is always non-nil.

> +        shell_cmd = string.format('cd %s && %s', dir, shell_cmd)
> +    end
> +    local res = os.execute(shell_cmd)
> +    assert(res == 0, 'Git cmd error: ' .. res)
> +end
> +
> +local function log_hashes(options, args, files, dir)
> +    args = args .. " --format='%h'"
> +    local local_temp_file = string.format('%s/%s', os.getenv('PWD'), temp_file)

9. Instead of writing output into a temporary file use
http://pgl.yoyo.org/luai/i/io.popen.

> +    exec_cmd(options, 'log', args, files, dir, local_temp_file)
> +    local lines = {}
> +    for line in io.lines(local_temp_file) do
> +        table.insert(lines, line)
> +    end
> +    os.remove(local_temp_file)
> +    return lines
> +end
> +
> +
> +return {
> +    exec_cmd = exec_cmd,
> +    log_hashes = log_hashes
> +}
> diff --git a/test/reload_evolution/storage.result b/test/reload_evolution/storage.result
> new file mode 100644
> index 0000000..2cf21fd
> --- /dev/null
> +++ b/test/reload_evolution/storage.result
> @@ -0,0 +1,184 @@
> +test_run = require('test_run').new()
> +---
> +...
> +git_util = require('git_util')
> +---
> +...
> +util = require('util')
> +---
> +...
> +vshard_copy_path = util.BUILDDIR .. '/test/var/vshard_git_tree_copy'
> +---
> +...
> +evolution_log = git_util.log_hashes('', '', 'vshard/storage/reload_evolution.lua', util.SOURCEDIR)
> +---
> +...
> +-- Cleanup the directory after a previous build.
> +_ = os.execute('rm -rf ' .. vshard_copy_path)
> +---
> +...
> +-- 1. `git worktree` cannot be used because PACKPACK mounts
> +-- `/source/` in `ro` mode.
> +-- 2. Just `cp -rf` cannot be used due to a little different
> +-- behavior in Centos 7.
> +_ = os.execute('mkdir ' .. vshard_copy_path)
> +---
> +...
> +_ = os.execute("cd " .. util.SOURCEDIR .. ' && cp -rf `ls -A --ignore=build` ' .. vshard_copy_path)
> +---
> +...
> +-- Checkout the first commit with a reload_evolution mechanism.
> +git_util.exec_cmd('', 'checkout', '-f', '', vshard_copy_path)
> +---
> +...
> +git_util.exec_cmd('', 'checkout', evolution_log[#evolution_log] .. '~1', '', vshard_copy_path)
> +---
> +...
> +REPLICASET_1 = { 'storage_1_a', 'storage_1_b' }
> +---
> +...
> +REPLICASET_2 = { 'storage_2_a', 'storage_2_b' }
> +---
> +...
> +test_run:create_cluster(REPLICASET_1, 'reload_evolution')
> +---
> +...
> +test_run:create_cluster(REPLICASET_2, 'reload_evolution')
> +---
> +...
> +util = require('util')
> +---
> +...
> +util.wait_master(test_run, REPLICASET_1, 'storage_1_a')
> +---
> +...
> +util.wait_master(test_run, REPLICASET_2, 'storage_2_a')
> +---
> +...
> +test_run:switch('storage_1_a')
> +---
> +- true
> +...
> +vshard.storage.internal.reload_evolution_version
> +---
> +- null
> +...
> +vshard.storage.bucket_force_create(1, vshard.consts.DEFAULT_BUCKET_COUNT / 2)> +---
> +- true
> +...
> +box.space.customer:insert({1, 1, 'customer_name'})
> +---
> +- [1, 1, 'customer_name']
> +...
> +test_run:switch('storage_2_a')
> +---
> +- true
> +...
> +fiber = require('fiber')
> +---
> +...
> +vshard.storage.bucket_force_create(vshard.consts.DEFAULT_BUCKET_COUNT / 2 + 1, vshard.consts.DEFAULT_BUCKET_COUNT / 2)
> +---
> +- true
> +...
> +while test_run:grep_log('storage_2_a', 'The cluster is balanced ok') == nil do vshard.storage.rebalancer_wakeup() fiber.sleep(0.1) end
> +---
> +...
> +test_run:switch('default')
> +---
> +- true
> +...
> +git_util.exec_cmd('', 'checkout', evolution_log[1], '', vshard_copy_path)
> +---
> +...
> +test_run:switch('storage_1_a')
> +---
> +- true
> +...
> +package.loaded["vshard.storage"] = nil
> +---
> +...
> +vshard.storage = require("vshard.storage")
> +---
> +...
> +test_run:grep_log('storage_1_a', 'vshard.storage.reload_evolution: upgraded to') ~= nil
> +---
> +- true
> +...
> +vshard.storage.internal.reload_evolution_version
> +---
> +- 1
> +...
> +-- Make sure storage operates well.
> +vshard.storage.bucket_force_drop(2)
> +---
> +- true
> +...
> +vshard.storage.bucket_force_create(2)

10. This is too simple test. Force_drop/create merely do DML on _bucket. Lets
test the rebalancer creating a dis-balance. For example, put on storage_1 +10
buckets, and on storage_2 -10 buckets and wait for the balance.

> diff --git a/test/reload_evolution/storage_1_a.lua b/test/reload_evolution/storage_1_a.lua
> new file mode 100755
> index 0000000..3e03f8f
> --- /dev/null
> +++ b/test/reload_evolution/storage_1_a.lua
> @@ -0,0 +1,144 @@
> +#!/usr/bin/env tarantool

11. Just use a symbolic link to the existing storage_1_a. Same
for other storages.

> diff --git a/test/reload_evolution/suite.ini b/test/reload_evolution/suite.ini
> new file mode 100644
> index 0000000..bb5435b
> --- /dev/null
> +++ b/test/reload_evolution/suite.ini

12. You do not need a new test suite for one storage test. Please,
put it into test/storage/

> diff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
> index bf560e6..1740c98 100644
> --- a/vshard/storage/init.lua
> +++ b/vshard/storage/init.lua
> @@ -105,6 +110,11 @@ if not M then
>           -- a destination replicaset must drop already received
>           -- data.
>           rebalancer_sending_bucket = 0,
> +
> +        ------------------------- Reload -------------------------
> +        -- Version of the loaded module. This number is used on
> +        -- reload to determine which upgrade scripts to run.
> +        reload_evolution_version = reload_evolution.version,

13. Use 'version' name.

>       }
>   end
>   
> diff --git a/vshard/storage/reload_evolution.lua b/vshard/storage/reload_evolution.lua
> new file mode 100644
> index 0000000..cfac888
> --- /dev/null
> +++ b/vshard/storage/reload_evolution.lua
> @@ -0,0 +1,58 @@
> +--
> +-- This module is used to upgrade the vshard.storage on the fly.
> +-- It updates internal Lua structures in case they are changed
> +-- in a commit.
> +--
> +local log = require('log')
> +
> +--
> +-- Array of upgrade functions.
> +-- magrations[version] = function which upgrades module version

14. Typo: magrations.

> +-- from `version` to `version + 1`.

15. Not +1. I think, we should use real version numbers:
0.1.1, 0.1.2, etc similar to tarantool.

> +--
> +local migrations = {}
> +
> +-- Initialize reload_upgrade mechanism
> +migrations[#migrations + 1] = function (M)
> +    -- Code to update Lua objects.
> +end
> +
> +--
> +-- Perform an update based on a version stored in `M` (internals).
> +-- @param M Old module internals which should be updated.
> +--
> +local function upgrade(M)
> +    local start_version = M.reload_evolution_version or 1
> +    if start_version > #migrations then
> +        local err_msg = string.format(
> +            'vshard.storage.reload_evolution: ' ..
> +            'auto-downgrade is not implemented; ' ..
> +            'loaded version is %d, upgrade script version is %d',
> +            start_version, #migrations

16. Did you test it? I do not see a test.

> +        )
> +        log.error(err_msg)
> +        error(err_msg)
> +    end
> +    for i = start_version, #migrations  do
> +        local ok, err = pcall(migrations[i], M)
> +        if ok then
> +            log.info('vshard.storage.reload_evolution: upgraded to %d version',
> +                     i)
> +        else
> +            local err_msg = string.format(
> +                'vshard.storage.reload_evolution: ' ..
> +                'error during upgrade to %d version: %s', i, err
> +            )
> +            log.error(err_msg)
> +            error(err_msg)
> +        end
> +        -- Update the version just after upgrade to have an
> +        -- actual version in case of an error.
> +        M.reload_evolution_version = i
> +    end
> +end
> +
> +return {
> +    version = #migrations,

17. Where do you use it?

> +    upgrade = upgrade,
> +}
>