Re: [Tarantool-patches] [PATCH v1] test: set unix sockets for iproto at core = app

[Alexander Turenko <alexander.turenko@tarantool.org>]

The patch may be much simpler. I got 46 lines diff in result (see below
the email).

Please, consider comments below.

NB: Updated test-run with [1] on master, 2.4, 2.3, 1.10.

[1]: https://github.com/tarantool/test-run/pull/210

WBR, Alexander Turenko.

On Thu, May 14, 2020 at 11:06:06AM +0300, Alexander V. Tikhonov wrote:
> Set hard-coded unix sockets for iproto connections at core = app.
> Added its option in *-tap/suites.ini files and fixed tests for it.
> Fix helped to handle the problem with 'Address already in use' error.
> Check the previous commit that set the use of sockets:
> 
> 60f84cbfca24e3a91cea067c923e006b44ee589f ('test: use unix sockets for iproto connections')

Nit: over 72 symbols.

> 
> Closes #4459
> ---
> 
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4008-unix_sockets_iproto_sql_tap
> Issue: https://github.com/tarantool/tarantool/issues/4459
> 
>  test/app-tap/init_script.test.lua                    | 7 ++++++-
>  test/app-tap/suite.ini                               | 1 +
>  test/app-tap/tarantoolctl.test.lua                   | 4 ++--
>  test/box-tap/auth.test.lua                           | 6 +++++-
>  test/box-tap/session.test.lua                        | 7 +++++--
>  test/box-tap/suite.ini                               | 1 +
>  test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua | 6 +++++-
>  test/sql-tap/suite.ini                               | 1 +
>  8 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/test/app-tap/init_script.test.lua b/test/app-tap/init_script.test.lua
> index 155f149a7..d7cea7c40 100755
> --- a/test/app-tap/init_script.test.lua
> +++ b/test/app-tap/init_script.test.lua
> @@ -2,8 +2,12 @@
>  --
>  -- Testing init script
>  --
> +
> +local LISTEN_SOCKET = 'init_script.listen.sock'
> +
> +os.remove(LISTEN_SOCKET)
>  box.cfg{
> -    listen = os.getenv("LISTEN"),
> +    listen = 'unix/:./' .. LISTEN_SOCKET,
>      pid_file = "box.pid",
>      memtx_memory=107374182,
>      log="tarantool.log"
> @@ -86,4 +90,5 @@ fiber.sleep(0.0)
>  assert (require ~= nil)
>  
>  space:drop()
> +os.remove(LISTEN_SOCKET)
>  os.exit()

You implemented `use_unix_sockets_iproto` option for app tests in
test-run ([1]), which sets LISTEN environment variable to a Unix socket
path (instead of a TCP port).

The option is implemented in the way that always give full
(non-relative) path and commonly used `box.cfg({listen =
os.getenv("LISTEN")})` statement should not be broken by the option.
That's neat: we can reduce changes in tests to its minimum.

This patch enables the option for tarantool's app tests. And we should
fix rare cases, where box.cfg.listen is assumed to be a TCP port number.

But here you made very strange thing: you throw out the fresly
implemented feature to a trash can and hardcode a unix path. Well, this
way is also acceptable, but if we'll going this way, why we implemented
the feature in test-run? Let's choose one way, not both at once.

This change is suspectful. If you unable to use just implemented
feature, then maybe it is not usable and should be fixed (or should not
be implemented at all?)

Anyway, I tried to just enable the option and everything works fine
here. We just need to left it unchanged.

[1]: https://github.com/tarantool/test-run/pull/210

> diff --git a/test/app-tap/suite.ini b/test/app-tap/suite.ini
> index 9629dfad5..daababef5 100644
> --- a/test/app-tap/suite.ini
> +++ b/test/app-tap/suite.ini
> @@ -4,3 +4,4 @@ description = application server tests (TAP)
>  lua_libs = lua/require_mod.lua lua/serializer_test.lua
>  is_parallel = True
>  pretest_clean = True
> +use_unix_sockets_iproto = True

> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
> index 4d7059559..675e511dd 100755
> --- a/test/app-tap/tarantoolctl.test.lua
> +++ b/test/app-tap/tarantoolctl.test.lua
> @@ -465,12 +465,12 @@ else
>      local remote_path = create_script(dir, 'remote.lua', remote_code)
>      test_run:cmd(("create server remote with script='%s'"):format(remote_path))
>      test_run:cmd("start server remote")
> -    local port = tonumber(
> +    local admin_socket = tostring(
>          test_run:eval("remote",
>                        "return require('uri').parse(box.cfg.listen).service")[1]
>      )

Nit: A console socket is often called 'admin socket' (see code block
just above this one), not a binary protocol socket.

Nit: tostring() is not needed, it is already string.

> -    local command_base = ('tarantoolctl play localhost:%d filler/00000000000000000000.xlog'):format(port)
> +    local command_base = ('tarantoolctl play %s filler/00000000000000000000.xlog'):format(admin_socket)

I asked the question to myself: what is the idea of this (original)
code?

* Ask remote instance how we can connect to it (URI or kinda).
* Pass this value to `tarantoolctl play` argument.

But why the code is so complex?

When TCP socket is used, test-run set LISTEN value to just port, say,
'3301' (all environment variables are strings). So box.cfg() is called
with {listen = '<port number>'} option. `box.cfg.listen` gives just
passed value (convert it to string if it is number, but it does not
matter here).

However author of the code know that box.cfg.listen can be in form
'<host>:<port>' and decided to write general code, which will not lean
on test-run peculiars much. But (s)he just don't keep unix sockets in
mind.

Can we keep the code general and make it even more general: support unix
sockets -- without increasing complexity? Yes!

What is URI in `tarantoolctl play URI FILE` command? It just passed to
net_box.connect(), which in fact support all box.cfg.listen schemas:

* '3301'
* 'localhost:3301'
* '/full/path/to/listen.socket'
* 'unix/:/full/patch/to/listen.socket'
* 'unix/:./relative/path/to/listen.socket'

So we need just don't modify box.cfg.listen value and pass it to the
`tarantoolctl play` argument. Everything will work.

This is neat, because the code becomes simpler and more
self-descriptive: just get URI and connect to it.

 |      local remote_path = create_script(dir, 'remote.lua', remote_code)
 |      test_run:cmd(("create server remote with script='%s'"):format(remote_path))
 |      test_run:cmd("start server remote")
 | -    local port = tonumber(
 | -        test_run:eval("remote",
 | -                      "return require('uri').parse(box.cfg.listen).service")[1]
 | -    )
 | +    local listen_uri = test_run:eval("remote", "return box.cfg.listen")[1]
 |  
 | -    local command_base = ('tarantoolctl play localhost:%d filler/00000000000000000000.xlog'):format(port)
 | +    local command_base = ('tarantoolctl play %s filler/00000000000000000000.xlog'):format(listen_uri)

It works with TCP and Unix sockets both.

----

Side notes.

While I thinking how to support both TCP and Unix sockets and how to
don't make the code more complex, several variants came into my mind.
They are not so neat as one above, but I would note them (maybe it will
be useful somewhere else).

1. Before I found that just '3301' is okay to pass to tarantoolct play I
   was looking how to construct either 'unix/:<...>' or
   'localhost:<port>' URI. There is the following way.

   We can use 'uri' module to reconstruct URI (TCP or Unix) from
   `box.cfg.listen` value.

    | local urilib = require('uri')
    | local uri = urilib.parse(box.cfg.listen)
    | uri.host = uri.host or 'localhost'
    | local listen_uri = urilib.format(uri)

   It is not a one-liner, so I considered it too complex for use in the
   test.

2. 'create server' test-run command has 'return_listen_uri' option. When
   it is passed, the command returns URI that test-run will pass to
   instance's environment via LISTEN variable.

   The returned URI has form 'localhost:3301' or
   '/path/to/listen.socket' and so suitable for the tarantoolctl
   argument.

   This variant is as good as passing `box.cfg.listen` w/o changes and
   maybe even better, but it brings the new entity to the patch (the
   test-run option), so make the patch harder to understand by a reader.
   So I would use this way in a new test, but keep test_run:eval() in an
   old test is possible.

> diff --git a/test/box-tap/auth.test.lua b/test/box-tap/auth.test.lua
> index 4e9879408..5d6687a33 100755
> --- a/test/box-tap/auth.test.lua
> +++ b/test/box-tap/auth.test.lua

Not needed, same as app-tap/init_script.test.lua.

> diff --git a/test/box-tap/session.test.lua b/test/box-tap/session.test.lua
> index 5d4965533..333736d72 100755
> --- a/test/box-tap/session.test.lua
> +++ b/test/box-tap/session.test.lua

Not needed, same as app-tap/init_script.test.lua.

> diff --git a/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua b/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua
> index d4b597e35..412649df8 100755
> --- a/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua
> +++ b/test/sql-tap/gh-4077-iproto-execute-no-bind.test.lua

Not needed, same as app-tap/init_script.test.lua.

----

Resulting patch:

diff --git a/test/app-tap/suite.ini b/test/app-tap/suite.ini
index 9629dfad5..daababef5 100644
--- a/test/app-tap/suite.ini
+++ b/test/app-tap/suite.ini
@@ -4,3 +4,4 @@ description = application server tests (TAP)
 lua_libs = lua/require_mod.lua lua/serializer_test.lua
 is_parallel = True
 pretest_clean = True
+use_unix_sockets_iproto = True
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index 4d7059559..9f1464617 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -465,12 +465,9 @@ else
     local remote_path = create_script(dir, 'remote.lua', remote_code)
     test_run:cmd(("create server remote with script='%s'"):format(remote_path))
     test_run:cmd("start server remote")
-    local port = tonumber(
-        test_run:eval("remote",
-                      "return require('uri').parse(box.cfg.listen).service")[1]
-    )
+    local listen_uri = test_run:eval("remote", "return box.cfg.listen")[1]
 
-    local command_base = ('tarantoolctl play localhost:%d filler/00000000000000000000.xlog'):format(port)
+    local command_base = ('tarantoolctl play %s filler/00000000000000000000.xlog'):format(listen_uri)
 
     local status, err = pcall(function()
         test:test("fill and test play output", function(test_i)
diff --git a/test/box-tap/suite.ini b/test/box-tap/suite.ini
index 8d9e32d3f..a2818fe62 100644
--- a/test/box-tap/suite.ini
+++ b/test/box-tap/suite.ini
@@ -5,3 +5,4 @@ is_parallel = True
 pretest_clean = True
 fragile = cfg.test.lua     ; gh-4344
           key_def.test.lua ; gh-4252
+use_unix_sockets_iproto = True
diff --git a/test/sql-tap/suite.ini b/test/sql-tap/suite.ini
index 8f3c3eab1..9a80ab7f9 100644
--- a/test/sql-tap/suite.ini
+++ b/test/sql-tap/suite.ini
@@ -31,3 +31,4 @@ show_reproduce_content = False
 pretest_clean = True
 fragile = gh-4077-iproto-execute-no-bind.test.lua ; gh-4459
           selectG.test.lua                        ; gh-4458
+use_unix_sockets_iproto = True

Verified it locally on Linux:

$ cmake .                          \
      -DCMAKE_BUILD_TYPE=Debug     \
      -DENABLE_BACKTRACE=ON        \
      -DENABLE_DIST=ON             \
      -DENABLE_BUNDLED_LIBCURL=OFF \
      && make -j
$ (cd test && ./test-run.py --force --long)
<...>
Statistics:
* disabled: 53
* pass: 1091