From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9BE21469710 for ; Thu, 21 May 2020 06:16:22 +0300 (MSK) Date: Thu, 21 May 2020 06:15:58 +0300 From: Alexander Turenko Message-ID: <20200521031558.3ty5x7cgcp6ffaky@tkn_work_nb> References: <5e8736537209a6b2d45d47a15edb22d8e6a09f59.1589443360.git.avtikhon@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5e8736537209a6b2d45d47a15edb22d8e6a09f59.1589443360.git.avtikhon@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v1] test: set unix sockets for iproto at core = app List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Alexander V. Tikhonov" Cc: Oleg Piskunov , tarantool-patches@dev.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 = ''} 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 ':' 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:' 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