Re: [Tarantool-patches] [PATCH vshard 1/2] storage: introduce upgrade strategy

[Oleg Babin <olegrok@tarantool.org>]

Hi! Thanks for your patch!

I have several quite minor comments.

On 21/03/2020 21:59, Vladislav Shpilevoy wrote:

> +
> +local function schema_upgrade_to_0_1_16_0(username)
> +    -- Since 0.1.16.0 the old versioning by
> +    -- 'oncevshard:storage:<number>' is dropped because it is not
> +    -- really extendible nor understandable.
> +    log.info('Insert vshard_version into _schema')
> +    box.space._schema:replace({'vshard_version', 0, 1, 16, 0})
> +    box.space._schema:delete({'oncevshard:storage:1'})
> +end

I think it's better to wrap all upgrades into transaction or may be the 
whole upgrade procedure below.

> +local function schema_current_version()
> +    local version = box.space._schema:get({'vshard_version'})
> +    if version == nil then
> +        version = {0, 1, 15, 0}
> +    else
> +        version = version:totable()
> +        version = {version[2], version[3], version[4], version[5]}

You could use {version:unpack(2)} here. However feel free to ignore this 
comment.

> +    end
> +    return version
> +end
> +
> +local schema_latest_version = {0, 1, 16, 0}
> +
> +local function schema_upgrade_replica()
> +    local version = schema_current_version()
> +    -- Replica can't do upgrade - it is read-only. And it
> +    -- shouldn't anyway - that would conflict with master doing
> +    -- the same. So the upgrade is either non-critical, and the
> +    -- replica can work with the new code but old schema. Or it
> +    -- it is critical, and need to wait the schema upgrade from
> +    -- the master.
> +    -- Or it may happen, that the upgrade just is not possible.
> +    -- For example, when an auto-upgrade tries to change a too old
> +    -- schema to the newest, skipping some intermediate versions.
> +    -- For example, from 1.2.3.4 to 1.7.8.9, when it is assumed
> +    -- that a safe upgrade should go 1.2.3.4 -> 1.2.4.1 ->
> +    -- 1.3.1.1 and so on step by step.
> +    if schema_compare_versions(version, schema_latest_version) == 0 then
> +        log.info('Replica\'s vshard schema version is up to date - %s',
> +                 table.concat(version, '.'))

You don't have such messages on a master (that current schema is the 
latest). I think the log level here should be "verbose".

> +    else
> +        log.info('Replica\' vshard schema version is outdated - current '..
> +                 '%s vs latest %s, but the replica still can work',
> +                 table.concat(version, '.'),
> +                 table.concat(schema_latest_version, '.'))
> +    end
> +    -- In future for hard changes the replica may be suspended
> +    -- until its schema is synced with master. Or it may
> +    -- reject to upgrade in case of incompatible changes. Now
> +    -- there are too few versions so as such problems could
> +    -- appear.
> +end
> +
> +local function schema_upgrade_master(username, password)
> +    local _schema = box.space._schema
> +    local is_old_versioning = _schema:get({'oncevshard:storage:1'}) ~= nil
> +    local version = schema_current_version()
> +    local is_bootstrap = not box.space._bucket
> +
> +    if is_bootstrap then
> +        schema_init_0_1_15_0(username, password)
> +    elseif is_old_versioning then
> +        log.info('The instance does not have vshard_version record. '..
> +                 'It is 0.1.15.0.')
> +    end
> +    local handlers = {
> +        {version = {0, 1, 16, 0}, func = schema_upgrade_to_0_1_16_0},
> +    }
> +    assert(schema_compare_versions(handlers[#handlers].version,
> +                                   schema_latest_version) == 0)
> +    for _, handler in ipairs(handlers) do
> +        local next_version = handler.version
> +        if schema_compare_versions(next_version, version) > 0 then
> +            local next_version_str = table.concat(next_version, '.')
> +            log.info("Upgrade vshard to {%s}", next_version_str)
> +            handler.func(username, password)

I think it will be good if you wrap it into pcall and add possible error 
handling. I see that all changes quite trivial but there is no guarantee 
that it will be in future. Ideally, if you rollback all already applied 
changes. This is an opportunity for user to downgrade module to previous 
version and report about a bug without any inconsistencies after upgrade.