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

["Alexander V. Tikhonov" <avtikhon@tarantool.org>]

Hi Alexander, thanks a lot for the review, I'll try to find the other
way to fix it. Unfortunately the patch you suggested at the end was the
same that I've started from, please check:

https://github.com/tarantool/tarantool/commit/ee080600df32f177dcdd5432217d081d3efc22aa

but in this way the tests still fail like I showed at the issue:

https://github.com/tarantool/tarantool/issues/4459#issuecomment-628430495

that is why I had to find some other way in addition to the fix.


On Thu, May 21, 2020 at 06:15:58AM +0300, Alexander Turenko wrote:
> 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