From: Leonid Vasiliev via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Roman Khabibov <roman.habibov@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH] box: set cfg via environment variables Date: Mon, 12 Apr 2021 11:37:31 +0300 [thread overview] Message-ID: <1e6cb78e-661f-e762-a952-a057b78bb4eb@tarantool.org> (raw) In-Reply-To: <FF1656A1-072D-405D-BC36-F4FE948F8631@tarantool.org> Hi! Thank you for the patch. See 9 comments below: > > commit ee6525f843a347c1df8a92a015414f4cbcceafb9 > Author: Roman Khabibov <roman.habibov@tarantool.org> > Date: Tue Mar 9 16:47:11 2021 +0300 > > box: set cfg via environment variables > > Add ability to set box.cfg options via env variables. These > variables should have name "TT_<OPTION>". When Tarantool instance > is started under tarantoolctl utility env variables options have > more priority than instance file's cfg. > > Closes #5602 > > @TarantoolBot document > Title: Set box.cfg options via env variables > > Now, it is possible to set box.cfg options via environment > variables. The name of variable should correspond the following > pattern: "TT_<NAME>", where <NAME> is an option name. For example: > "TT_LISTEN", "TT_READAHEAD". > If it is complex value, for example, "replication" can be set by a > table, the corresponding environment variable value has the syntax > of lua array (without keys): > > `export TT_REPLICATION="{'uri1', 'uri2'}"` > > diff --git a/changelogs/unreleased/environment-cfg.md b/changelogs/unreleased/environment-cfg.md > new file mode 100755 > index 000000000..c5d5005ac > --- /dev/null > +++ b/changelogs/unreleased/environment-cfg.md > @@ -0,0 +1,5 @@ > +## feature/core > + > +* Now, it is possibly to set box.cfg options with environment variables. > +The priority of sources of configuration options is the following (from low > +to high): default, tarantoolctl, environment, box.cfg{}. (gh-5602). > \ No newline at end of file > diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua > index 574c8bef4..a1439ae46 100644 > --- a/src/box/lua/load_cfg.lua > +++ b/src/box/lua/load_cfg.lua > @@ -416,6 +416,20 @@ local translate_cfg = { > end}, > } > > +-- Configuration options from environment is taken into account > +-- only at the first box.cfg{} call. When box.cfg is reload, they > +-- are ignored. > +-- > +-- The priority of sources of configuration options is the > +-- following (from low to high): default, tarantoolctl, > +-- environment, box.cfg{}. > +local env_cfg = {} > +-- This is auxiliary table and it needed to keep values which > +-- passed or typed to box.cfg{} at the first call. Then we > +-- distinguish values (it was from environment or not) and decide > +-- ignore it or not during reload. > +local user_cfg = {} 1) AFAIU we decided what it's not needed. In my mind, on reload, we have no difference between the options that were set using environment and any other way. > + > -- Upgrade old config > local function upgrade_cfg(cfg, translate_cfg) > if cfg == nil then > @@ -515,6 +529,22 @@ local function prepare_cfg(cfg, default_cfg, template_cfg, > return new_cfg > end > > +--- > +-- Transfer options from env_cfg to cfg. > +-- > +local function apply_env_cfg(cfg, env_cfg) > + if env_cfg == nil then > + return > + end > + for k, v in pairs(env_cfg) do > + if cfg[k] == nil or os.getenv('TARANTOOLCTL') == 'true' then 2) Add a comment describing the condition `os.getenv('TARANTOOLCTL') == 'true'`. > + cfg[k] = v > + elseif type(v) == 'table' then > + apply_env_cfg(cfg[k], v) > + end > + end > +end > + > local function apply_default_cfg(cfg, default_cfg, module_cfg) > for k,v in pairs(default_cfg) do > if cfg[k] == nil then > @@ -549,7 +579,19 @@ local function compare_cfg(cfg1, cfg2) > return true > end > > +--- > +-- Nullify option values from environment in old configuration. > +-- > +local function reset_env_options(oldcfg) 3) Why should we nullify option values from environment in old configuration? > + for k, v in pairs(env_cfg) do > + if user_cfg[k] == nil then > + oldcfg[k] = nil > + end > + end > +end > + > local function reload_cfg(oldcfg, cfg) > + reset_env_options(oldcfg) > cfg = upgrade_cfg(cfg, translate_cfg) > local newcfg = prepare_cfg(cfg, default_cfg, template_cfg, > module_cfg, modify_cfg) > @@ -616,6 +658,104 @@ setmetatable(box, { > end > }) > > +--- > +-- Erase quotes at the begin and the end of string. > +-- > +local function dequote(str) > + if string.sub(str, 1, 1) == '\'' and string.sub(str, -1) == '\'' or > + string.sub(str, 1, 1) == '"' and string.sub(str, -1) == '"' then > + return string.sub(str, 2, -2) > + end > + return str > +end > + > +--- > +-- Convert raw option value becoming from the corresponding > +-- environment variable represented as string to string, number or > +-- boolean (depending on option). > +-- > +local function process_option_value(option, raw_value) > + local param_type = template_cfg[option] > + > + -- May be peculiar to "feedback*" parameters. > + if param_type == nil then > + return nil > + end > + > + local value = nil > + local err_msg_fmt = 'Environment variable TT_%s has '.. 4) Add a space before `..`. > + 'incorrect value for option \'%s\': should be %s' > + > + -- "log_level" option should be a number. > + if param_type == 'number' or option == 'log_level' then 5) As for me it's strange, that in a94a9b3fd629b5abbc99a23ba527cd91e5c6ebf5 Cyrill Gorcunov moves the type check to a module, but you (in fact) return it back (determine the type by the parameter name). I think you need to discuss this question with Cyrill and check types either inside the module or in the `load_cfg.lua`. > + value = tonumber(raw_value) > + if value == nil then > + error(err_msg_fmt:format(string.upper(option), option, > + "convertible to number")) > + end > + -- "log_nonblock" option should be boolean. > + elseif param_type == 'boolean' or option == 'log_nonblock' then > + if raw_value == 'true' then > + value = true > + elseif raw_value == 'false' then > + value = false > + else > + error(err_msg_fmt:format(string.upper(option), option, > + "'true' or 'false'")) > + end > + elseif param_type == 'string, number' then 6) Since some options can have several types, I think it's more comfortable for such options to use a table listing all possible types. But such refactoring doesn't seem to be mandatory for the patch. > + -- Firstly, try to convert to number. > + value = tonumber(raw_value) > + if value == nil then > + value = raw_value > + end > + elseif param_type == 'string, number, table' then > + if string.sub(raw_value, 1, 1) == '{' and > + string.sub(raw_value, -1) == '}' then > + value = string.sub(raw_value, 2, -2) > + value = require('csv').load(value)[1] > + for i, item in pairs(value) do > + value[i] = dequote(item) 7) In this case, the number inside the table will be parsed as a "string". We don't want that. > + end > + else > + value = tonumber(raw_value) > + if value == nil then > + value = raw_value > + end > + end > + else > + value = raw_value > + end > + > + return value > +end > + > +-- local function collect_env_cfg() > +-- local env_cfg = {} > +-- for option, _ in pairs(template_cfg) do > +-- if cfg[option] == nil or os.getenv('TARANTOOLCTL') == 'true' then > +-- local env_var_name = 'TT_'..string.upper(option) > +-- local raw_value = os.getenv(env_var_name) > +-- if raw_value ~= nil then > +-- cfg[option] = process_option_value(option, raw_value) > +-- end > +-- end > +-- end > +-- end 8) Don't leave commented code after work. > + > +--- > +-- Pull and process configuration values from environment. > +-- > +local function collect_env_cfg() > + for option, _ in pairs(template_cfg) do > + local env_var_name = 'TT_'..string.upper(option) 9) Add spaces around the `..` > + local raw_value = os.getenv(env_var_name) > + if raw_value ~= nil then > + env_cfg[option] = process_option_value(option, raw_value) > + end > + end > +end > + > -- Whether box is loaded. > -- > -- `false` when box is not configured or when the initialization > @@ -627,6 +767,7 @@ setmetatable(box, { > local box_is_configured = false > > local function load_cfg(cfg) > + user_cfg = cfg > -- A user may save box.cfg (this function) before box loading > -- and call it afterwards. We should reconfigure box in the > -- case. > @@ -638,6 +779,8 @@ local function load_cfg(cfg) > cfg = upgrade_cfg(cfg, translate_cfg) > cfg = prepare_cfg(cfg, default_cfg, template_cfg, > module_cfg, modify_cfg) > + collect_env_cfg() > + apply_env_cfg(cfg, env_cfg) > apply_default_cfg(cfg, default_cfg, module_cfg); > -- Save new box.cfg > box.cfg = cfg > diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua > index 3c3023d34..060316714 100755 > --- a/test/app-tap/tarantoolctl.test.lua > +++ b/test/app-tap/tarantoolctl.test.lua > @@ -160,7 +160,7 @@ local function merge(...) > end > > local test = tap.test('tarantoolctl') > -test:plan(8) > +test:plan(9) > > -- basic start/stop test > -- must be stopped afterwards > @@ -632,4 +632,35 @@ test:test('filter_xlog', function(test) > end > end) > > +-- gh-5602: Check that environment cfg variables are more > +-- prioritized than tarantoolctl cfg. > +do > + local dir = fio.tempdir() > + local code = [[ box.cfg ]] > + create_script(dir, 'show_cfg.lua', code) > + local code = [[ box.cfg{checkpoint_interval=3302, readahead=10001} ]] > + create_script(dir, 'cfg_script.lua', code) > + > + local status, err = pcall(function() > + test:test("check answers in case of call", function(test_i) > + test_i:plan(4) > + os.setenv('TT_CHECKPOINT_INTERVAL', '3301') > + os.setenv('TT_READAHEAD', '10000') > + check_ok(test_i, dir, 'start', 'cfg_script', 0) > + tctl_wait_start(dir, 'cfg_script') > + check_ok(test_i, dir, 'eval', 'cfg_script show_cfg.lua', 0, > + '---\n- 3301\n- 10000\n...', nil) > + check_ok(test_i, dir, 'stop', 'cfg_script', 0) > + end) > + end) > + > + cleanup_instance(dir, 'cfg_script') > + recursive_rmdir(dir) > + > + if status == false then > + print(("Error: %s"):format(err)) > + os.exit() > + end > +end > + > os.exit(test:check() == true and 0 or -1) > diff --git a/test/box-tap/gh-5602-environment-vars-cfg.result b/test/box-tap/gh-5602-environment-vars-cfg.result > new file mode 100644 > index 000000000..c90dd301b > --- /dev/null > +++ b/test/box-tap/gh-5602-environment-vars-cfg.result > @@ -0,0 +1,41 @@ > +TAP version 13 > +1..5 > +ok - box.cfg is successful > +ok - listen > +ok - readahead > +ok - strip_core > +ok - log_format is not set > +TAP version 13 > +1..7 > +ok - box.cfg is successful > +ok - listen > +ok - replication is table > +ok - replication URI 1 > +ok - replication URI 2 > +ok - replication_connect_timeout > +ok - replication_synchro_quorum > +TAP version 13 > +1..3 > +ok - box.cfg is successful > +ok - box.cfg{} background value is prioritized > +ok - box.cfg{} vinyl_timeout value is prioritized > +TAP version 13 > +1..2 > +ok - box.cfg is not successful > +ok - bad sql_cache_size value > +TAP version 13 > +1..2 > +ok - box.cfg is not successful > +ok - bad strip_core value > +TAP version 13 > +1..2 > +ok - box.cfg is not successful > +ok - bad replication value > +TAP version 13 > +1..6 > +ok - set 1 > +ok - set 2 > +ok - set 3 > +ok - set 4 > +ok - set 5 > +ok - set 6 > diff --git a/test/box-tap/gh-5602-environment-vars-cfg.test.lua b/test/box-tap/gh-5602-environment-vars-cfg.test.lua > new file mode 100755 > index 000000000..3d19fdc90 > --- /dev/null > +++ b/test/box-tap/gh-5602-environment-vars-cfg.test.lua > @@ -0,0 +1,57 @@ > +#!/usr/bin/env tarantool > + > +local os = require('os') > +local fio = require('fio') > +local tap = require('tap') > + > +local test = tap.test('gh-5602') > + > +-- gh-5602: Check that environment cfg variables working. > +local TARANTOOL_PATH = arg[-1] > +local script_name = 'gh-5602-test-cases-script.lua' > +local path_to_script = fio.pathjoin( > + os.getenv('PWD'), > + 'box-tap', > + script_name) > + > +--- > +-- Generate shell command where set is sequence of environment > +-- variables, then path to tarantool, the first arg is path to > +-- test script and the second one is number of set. > +-- > +local function shell_command(set, i) > + return ('%s %s %s %d'):format( > + set, > + TARANTOOL_PATH, > + path_to_script, > + i) > +end > + > +local set_1 = ('%s %s %s %s'):format('TT_LISTEN=3301', 'TT_READAHEAD=10000', > + 'TT_STRIP_CORE=false', 'TT_LOG_FORMAT=json') > +local set_2 = ('%s %s %s %s'):format('TT_LISTEN=3301', > + 'TT_REPLICATION=\"{\'0.0.0.0:12345\', \'1.1.1.1:12345\'}\"', > + 'TT_REPLICATION_CONNECT_TIMEOUT=0.01', > + 'TT_REPLICATION_SYNCHRO_QUORUM=\'4 + 1\'') > +local set_3 = 'TT_BACKGROUND=true TT_VINYL_TIMEOUT=60.1' > +local set_4 = 'TT_SQL_CACHE_SIZE=a' > +local set_5 = 'TT_STRIP_CORE=a' > +local set_6 = 'TT_REPLICATION=\"{0.0.0.0:12345\', \'1.1.1.1:12345\'}\"' > + > +local sets = {} > +sets[1] = set_1 > +sets[2] = set_2 > +sets[3] = set_3 > +sets[4] = set_4 > +sets[5] = set_5 > +sets[6] = set_6 > + > +test:plan(#sets) > +for i, set in ipairs(sets) do > + local tmpdir = fio.tempdir() > + local new_path = fio.pathjoin(tmpdir, script_name) > + fio.copyfile(path_to_script, new_path) > + test:is(os.execute(shell_command(set, i)), 0, 'set '..i) > +end > + > +os.exit(test:check() and 0 or 1) > diff --git a/test/box-tap/gh-5602-test-cases-script.lua b/test/box-tap/gh-5602-test-cases-script.lua > new file mode 100755 > index 000000000..a8b2fc987 > --- /dev/null > +++ b/test/box-tap/gh-5602-test-cases-script.lua > @@ -0,0 +1,69 @@ > +local tap = require('tap') > + > +local test = tap.test('gh-5602') > + > +local status, err = pcall(box.cfg, {background = false, vinyl_timeout = 70.1}) > + > +-- Check that environment cfg values are set correctly. > +if arg[1] == '1' then > + test:plan(5) > + test:ok(status ~= false, 'box.cfg is successful') > + test:is(box.cfg['listen'], 3301, 'listen') > + test:is(box.cfg['readahead'], 10000, 'readahead') > + test:is(box.cfg['strip_core'], false, 'strip_core') > + test:is(box.cfg['log_format'], 'json', 'log_format is not set') > +end > +if arg[1] == '2' then > + test:plan(7) > + test:ok(status ~= false, 'box.cfg is successful') > + test:is(box.cfg['listen'], 3301, 'listen') > + local replication = box.cfg['replication'] > + test:is(type(replication), 'table', 'replication is table') > + test:ok(replication[1] == '0.0.0.0:12345' or > + replication[1] == '1.1.1.1:12345', 'replication URI 1') > + test:ok(replication[2] == '0.0.0.0:12345' or > + replication[2] == '1.1.1.1:12345', 'replication URI 2') > + test:is(box.cfg['replication_connect_timeout'], 0.01, > + 'replication_connect_timeout') > + test:is(box.cfg['replication_synchro_quorum'], '4 + 1', > + 'replication_synchro_quorum') > +end > + > +-- Check that box.cfg{} values are more prioritized than > +-- environment cfg values. > +if arg[1] == '3' then > + test:plan(3) > + test:ok(status ~= false, 'box.cfg is successful') > + test:is(box.cfg['background'], false, > + 'box.cfg{} background value is prioritized') > + test:is(box.cfg['vinyl_timeout'], 70.1, > + 'box.cfg{} vinyl_timeout value is prioritized') > +end > + > +local err_msg_fmt = 'Environment variable TT_%s has '.. > + 'incorrect value for option \'%s\': should be %s' > + > +-- Check bad environment cfg values. > +if arg[1] == '4' then > + test:plan(2) > + test:ok(status == false, 'box.cfg is not successful') > + local index = string.find(err, err_msg_fmt:format('SQL_CACHE_SIZE', > + 'sql_cache_size', 'convertible to number')) > + test:ok(index ~= nil, 'bad sql_cache_size value') > +end > +if arg[1] == '5' then > + test:plan(2) > + test:ok(status == false, 'box.cfg is not successful') > + local index = string.find(err, err_msg_fmt:format('STRIP_CORE', > + 'strip_core', '\'true\' or \'false\'')) > + test:ok(index ~= nil, 'bad strip_core value') > +end > +if arg[1] == '6' then > + test:plan(2) > + test:ok(status == false, 'box.cfg is not successful') > + local index = string.find(tostring(err), 'Incorrect value for option '.. > + '\'replication\': expected host:service or /unix.socket') > + test:ok(index ~= nil, 'bad replication value') > +end > + > +os.exit(test:check() and 0 or 1) >
next prev parent reply other threads:[~2021-04-12 8:39 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-14 15:34 Roman Khabibov via Tarantool-patches 2021-03-19 12:38 ` Leonid Vasiliev via Tarantool-patches 2021-04-09 8:56 ` Roman Khabibov via Tarantool-patches 2021-04-12 8:37 ` Leonid Vasiliev via Tarantool-patches [this message] 2021-04-13 12:47 ` Roman Khabibov via Tarantool-patches 2021-04-12 9:52 ` Leonid Vasiliev via Tarantool-patches 2021-03-19 13:23 ` Leonid Vasiliev via Tarantool-patches
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=1e6cb78e-661f-e762-a952-a057b78bb4eb@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=lvasiliev@tarantool.org \ --cc=roman.habibov@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH] box: set cfg via environment variables' \ /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