Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: handle box.cfg's TT_* env vars #9499

Merged

Conversation

Totktonada
Copy link
Member

@Totktonada Totktonada commented Dec 18, 2023

There may be some confusion, so let's start with a background information.

There are TT_* environment variables introduced in commit 1b33012 ("box: set box.cfg options via environment variables"). They're interpreted by the box.cfg() call.

There are TT_* environment variables introduced in commit 82b0cff ("config: introduce env source"). They're interpreted by the declarative configuration logic, when tarantool starts with the --name <...> CLI option.

box.cfg's env variables have names deduced from box.cfg option names, while config's env variable names are deduced from the config schema.

Some options have the same names here and there, for example TT_REPLICATION_ANON (from box.cfg.replication_anon and replication.anon). However, there are ones that have different names, for example TT_LISTEN and TT_IPROTO_LISTEN.

Moreover, the declarative configuration has its own restrictions on the configuration data. For example, TT_IPROTO_LISTEN is always a list of URIs (like [{"uri": <...>, "params": {<...>}}]), not a single URI, not a string, not a number. The declarative configuration has a certain shape and doesn't allow polymorphic values.

Next, handling of box.cfg's variables by the old code in load_cfg.lua doesn't work well with the declarative configuration flow.

The main reason is that the new configuration flow calls box.cfg() with all the box.cfg values set, including default ones. If a user removes an option from its config, it applies its default. On the same time it instructs box.cfg() to don't read the corresponding environment variables.

This commit offers a partial solution: it adds support of the most of the box.cfg environment variables. The values are added into the configuration data with the lowest priority: if the same value is set in, for example, a file configuration, the file's value is preferred.

The following box.cfg's environment variables are not handled in this commit.

  • TT_LOG
  • TT_METRICS
  • TT_INSTANCE_NAME
  • TT_REPLICASET_NAME
  • TT_CLUSTER_NAME
  • TT_FORCE_RECOVERY,
  • TT_READ_ONLY
  • TT_BOOTSTRAP_LEADER
  • TT_REPLICATION
  • TT_REPLICATION_CONNECT_QUORUM

Fixes #9485

@Totktonada Totktonada self-assigned this Dec 18, 2023
@coveralls
Copy link

coveralls commented Dec 18, 2023

Coverage Status

coverage: 86.842% (+0.02%) from 86.818%
when pulling cd0a95b on Totktonada:gh-9485-handle-old-boxcfg-env-vars
into dde7342
on tarantool:master
.

@Totktonada Totktonada force-pushed the gh-9485-handle-old-boxcfg-env-vars branch from 49bdfcf to 4f130d1 Compare December 21, 2023 17:30
@Totktonada Totktonada changed the title WIP: config: handle box.cfg's TT_* env vars config: handle box.cfg's TT_* env vars Dec 21, 2023
@Totktonada Totktonada removed their assignment Dec 21, 2023
@Totktonada Totktonada marked this pull request as ready for review December 21, 2023 17:32
@Totktonada Totktonada requested review from a team as code owners December 21, 2023 17:32
Copy link
Collaborator

@ImeevMA ImeevMA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch!

@Totktonada Totktonada force-pushed the gh-9485-handle-old-boxcfg-env-vars branch from 4f130d1 to 36d93a5 Compare December 24, 2023 00:04
@Totktonada
Copy link
Member Author

@ImeevMA I've significantly changed the code and the test (see my comments in the pull request above). So, I would appreciate if you'll do a second look. Sorry for the false start.

@Totktonada Totktonada marked this pull request as draft December 24, 2023 00:34
@Totktonada Totktonada force-pushed the gh-9485-handle-old-boxcfg-env-vars branch from 36d93a5 to 54f5b0b Compare December 24, 2023 01:35
@Totktonada Totktonada marked this pull request as ready for review December 24, 2023 01:56
Copy link
Collaborator

@ImeevMA ImeevMA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes! Now it looks more complete. LGTM.

This function makes it easier to run a code that can't be run directly
for some reason: for example, it needs the initialized database.

It is a wrapper around treegen and justrun.

Part of tarantool#9485

NO_DOC=testing helper change
NO_CHANGELOG=see NO_DOC
NO_TEST=see NO_DOC
@Totktonada
Copy link
Member Author

I'm going to rebase it on top of the recently pushed PR #9544 and add two test cases with the key=value,key=value mapping syntax in TT_LISTEN.

The added test cases.
diff --git a/test/config-luatest/box_cfg_env_test.lua b/test/config-luatest/box_cfg_env_test.lua
index 16ee08ca1c..16282aaf33 100644
--- a/test/config-luatest/box_cfg_env_test.lua
+++ b/test/config-luatest/box_cfg_env_test.lua
@@ -152,6 +152,19 @@ g.test_more = helpers.run_as_script(function()
                 {uri = 'localhost:3302', params = {transport = 'plain'}},
             }}},
         },
+        -- TT_LISTEN: mapping.
+        {
+            env = {['TT_LISTEN'] = 'uri=3301'},
+            config = {iproto = {listen = {
+                {uri = '3301'},
+            }}},
+        },
+        {
+            env = {['TT_LISTEN'] = 'uri=localhost:3301'},
+            config = {iproto = {listen = {
+                {uri = 'localhost:3301'},
+            }}},
+        },
         -- TT_LISTEN: JSON object.
         {
             env = {['TT_LISTEN'] = '{"uri": "3301"}'},

There may be some confusion, so let's start with a background
information.

There are `TT_*` environment variables introduced in commit 1b33012
("box: set box.cfg options via environment variables"). They're
interpreted by the `box.cfg()` call.

There are `TT_*` environment variables introduced in commit 82b0cff
("config: introduce env source"). They're interpreted by the declarative
configuration logic, when tarantool starts with the `--name <...>` CLI
option.

box.cfg's env variables have names deduced from box.cfg option names,
while config's env variable names are deduced from the config schema.

Some options have the same names here and there, for example
`TT_REPLICATION_ANON` (from `box.cfg.replication_anon` and
`replication.anon`). However, there are ones that have different names,
for example `TT_LISTEN` and `TT_IPROTO_LISTEN`.

Moreover, the declarative configuration has its own restrictions on the
configuration data. For example, `TT_IPROTO_LISTEN` is always a list of
URIs (like `[{"uri": <...>, "params": {<...>}}]`), not a single URI, not
a string, not a number. The declarative configuration has a certain
shape and doesn't allow polymorphic values.

Next, handling of box.cfg's variables by the old code in `load_cfg.lua`
doesn't work well with the declarative configuration flow.

The main reason is that the new configuration flow calls `box.cfg()`
with all the `box.cfg` values set, including default ones. If a user
removes an option from its config, it applies its default. On the same
time it instructs `box.cfg()` to don't read the corresponding
environment variables.

This commit offers a partial solution: it adds support of the most of
the box.cfg environment variables. The values are added into the
configuration data with the lowest priority: if the same value is set
in, for example, a file configuration, the file's value is preferred.

The following box.cfg's environment variables are not handled in this
commit.

* `TT_LOG`
* `TT_METRICS`
* `TT_INSTANCE_NAME`
* `TT_REPLICASET_NAME`
* `TT_CLUSTER_NAME`
* `TT_FORCE_RECOVERY`,
* `TT_READ_ONLY`
* `TT_BOOTSTRAP_LEADER`
* `TT_REPLICATION`
* `TT_REPLICATION_CONNECT_QUORUM`

Fixes tarantool#9485

NO_DOC=looks more like a bug fix or a kind of compatibility layer
@Totktonada Totktonada force-pushed the gh-9485-handle-old-boxcfg-env-vars branch from 54f5b0b to cd0a95b Compare December 25, 2023 12:51
@Totktonada Totktonada added the full-ci Enables all tests for a pull request label Dec 25, 2023
@Totktonada Totktonada merged commit ce20268 into tarantool:master Dec 25, 2023
106 checks passed
@Totktonada Totktonada deleted the gh-9485-handle-old-boxcfg-env-vars branch December 25, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-ci Enables all tests for a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Listen socket is not created after restart of the instance, if set using environmental variable.
5 participants