From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org, AKhatskevich <avkhatskevich@tarantool.org> Subject: [tarantool-patches] Re: [PATCH 3/3] Introduce storage reload evolution Date: Thu, 19 Jul 2018 18:14:50 +0300 [thread overview] Message-ID: <ca84b3e6-a742-7e95-ac5f-df5eabbcb173@tarantool.org> (raw) In-Reply-To: <da7e29aad5c823570f3b552c2b36e2b5dcec02f1.1531935745.git.avkhatskevich@tarantool.org> 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, > +} >
next prev parent reply other threads:[~2018-07-19 15:14 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-18 17:47 [tarantool-patches] [PATCH 0/3] vshard reload mechanism AKhatskevich 2018-07-18 17:47 ` [tarantool-patches] [PATCH 1/3] Add test on error during reconfigure AKhatskevich 2018-07-19 15:14 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-19 20:33 ` Alex Khatskevich 2018-07-20 11:34 ` Alex Khatskevich 2018-07-20 14:15 ` Vladislav Shpilevoy 2018-07-18 17:47 ` [tarantool-patches] [PATCH 2/3] Complete module reload AKhatskevich 2018-07-19 15:14 ` [tarantool-patches] " Vladislav Shpilevoy 2018-07-19 20:32 ` Alex Khatskevich 2018-07-20 11:34 ` Alex Khatskevich 2018-07-20 14:15 ` Vladislav Shpilevoy 2018-07-18 17:47 ` [tarantool-patches] [PATCH 3/3] Introduce storage reload evolution AKhatskevich 2018-07-19 15:14 ` Vladislav Shpilevoy [this message] 2018-07-20 11:32 ` [tarantool-patches] " Alex Khatskevich 2018-07-20 14:15 ` Vladislav Shpilevoy 2018-07-20 23:58 ` Alex Khatskevich
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=ca84b3e6-a742-7e95-ac5f-df5eabbcb173@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=avkhatskevich@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH 3/3] Introduce storage reload evolution' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox