test: cover storage-first upgrade#656
Conversation
kamenkremen
force-pushed
the
gh-654
branch
3 times, most recently
from
136a1ad to
9504900
Compare
Serpentian
left a comment
Serpentian
left a comment
There was a problem hiding this comment.
Solid starting point! Thank you a lot
| local g = t.group('reload_storage_router', group_config) | ||
| local cfg_template = { | ||
| sharding = { | ||
| { replicas = { |
There was a problem hiding this comment.
Nit: the formating is broken. Let's be consistent with other tests in code-style
| end) | ||
|
|
||
| -- | ||
| -- `vtest.router_cfg` cannot be used in this test, since |
There was a problem hiding this comment.
Let's make the call for that funciton optional then in vtest. We should reuse the same functions for all of the tests and not rewrite them for every individual one. I see, that it's copy-pasted from reload_test, then let's use the same vtest function there.
| update_mode .. '.lua') | ||
| local file = assert(io.open(path, 'w')) | ||
| file:write([[ | ||
| local ok, compat = pcall(require, 'compat') |
There was a problem hiding this comment.
This is not an option, we'll soon release the new Tarantool version, where this compat and the associated code will be dropped, so that it won't be possible to use old box.info.cluster.uuid. So this fix is only temporary. I'd propose to unconditionally set it in the test instead
box.info.cluster.uuid = box.info.replicaset.uuidThere was a problem hiding this comment.
I'd propose to create the new directory and call it cluster_luatest and put the test there. It's not really related to the router, it tests upgrade of the whole cluster
| -- test/reload_evolution/storage.test.lua for reload to latest. | ||
| local legacy_storage_reload_hash = '139223269cddefe2ba4b8e9f6e44712f099f4b35' | ||
|
|
||
| local function vshard_copy_new(vardir, dirname) |
There was a problem hiding this comment.
Let's move the function to the git_util.lua and reuse it in the reload_test too. Let's try to minimize the copy-pasted code between these two tests.
|
|
||
| -- Third phase: master is moved to the updated replica, previous master | ||
| -- is still on old vshard. | ||
| local master_id, master_lsn = old_master:exec(function() |
There was a problem hiding this comment.
You're using the manual master mode, so such master switch won't work, it requires master = 'auto' mode. Let's use auto, it's more widely used and is more sophisticated implementation-wise
There was a problem hiding this comment.
It seems that the vshard version used here as legacy does not support the master mode option in replicaset_template
Why doesn’t this work now? I tried to follow the documentation for switching the master and used the legacy option: https://www.tarantool.io/en/doc/latest/admin/upgrades/upgrade_cluster/#switching-the-master
There was a problem hiding this comment.
It seems that the vshard version used here as legacy does not support the master mode option in replicaset_template
I propose to bump the commit hash then and use in the test the one, which has auto master available. We need to guarantee this upgrade procedure for the newest version, commit with auto master is new enough for that
It'll cover a lot more code, if we use auto master
| t.assert_equals(err, nil, name .. ' storage reload') | ||
| else | ||
| storage:restart() | ||
| storage_cfg_by_name({ name }, global_cfg) |
There was a problem hiding this comment.
Let's use vtest.storage_stop/start, it reconfigures the storage properly after restart
| else | ||
| storage:restart() | ||
| storage_cfg_by_name({ name }, global_cfg) | ||
| local _, err = storage:exec(storage_test_api_create) |
There was a problem hiding this comment.
Let's better use persistent lua functions in order not to recreate them after every restart: https://www.tarantool.io/en/doc/latest/reference/reference_lua/box_schema/func_create/
| end | ||
| end | ||
|
|
||
| local function check_cluster(router, master_uuid, bucket_ids) |
There was a problem hiding this comment.
This is a good starting point. Let's try adding the master_search, failover tests. After every test let's grep, that logs doesn't have assertion failed (I remember, we had that problem in failover). Let's also check moving a bucket (manual bucket_send is enough) + its discovery on router.
| hash = g.previous_version, | ||
| storage_version = g.previous_version, | ||
| }) | ||
| end |
There was a problem hiding this comment.
I'd propose to split the commits as follows:
- Refactoring of the
reload_test. Moves some functions out of it togit_utils, so that they can be reused in the next commit. - Introduce the test + all the changes in the
vtestneeded for that test to work.
See, the first commit you have right now is not valuable without the second one, we should not split them like that. You add the feature to vtest, which is not used in the first commit
Router reload test copy vshard sources to a temporary directory and checkout old revisions there. The cluster reload test needs the same setup for router and storage copies. Move the copy and LUA_PATH construction to git_util so reload tests reuse the same helper. Needed for tarantool#654 NO_DOC=test NO_TEST=refactoring
The intended upgrade order for clusters is to upgrade storage first and routers afterwards. Cover that upgrade order from legacy and previous vshard versions in both hot reload and restart modes. The test starts a cluster on an old checkout, upgrades storages first, switches the master to an already upgraded replica, upgrades the previous master, and updates the router last. After each phase it checks basic vshard functionality. Add vtest fixes to let luatest helpers work with old vshard versions. Fix compatibility issues exposed by the test. After hot reload, a router could keep an old api_call_cache while the router api call signature had changed. Storage hot reload could also keep stale master_sync_f state and a dead master_sync_fiber, so master sync was not restarted with the reloaded implementation. For mixed-version bucket movement, fall back to vshard.storage.sync when an old storage doesn't have storage_bucket_checkpoint(). Closes tarantool#654 @TarantoolBot document Title: vshard: storage-first cluster upgrade order Root document: https://www.tarantool.io/en/doc/latest/admin/upgrades/upgrade_cluster/ Update the cluster upgrade procedure to describe the storage-first order. The current documentation describes the router-first order, but the intended order is the opposite: upgrade storage first, then upgrade routers. The motivation is that storages do not depend on routers during upgrade. Routers initiate communication with storages.
2 participants
Closes #654