From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B8B232874F for ; Fri, 9 Aug 2019 14:38:12 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Ozwz8LsC5E-5 for ; Fri, 9 Aug 2019 14:38:12 -0400 (EDT) Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1691628746 for ; Fri, 9 Aug 2019 14:38:11 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH] Make `tarantoolctl start` work withiout box.cfg in init script From: "Maxim Melentiev" (Redacted sender "m.melentiev" for DMARC) In-Reply-To: <20190809134251.GA6081@atlas> Date: Fri, 9 Aug 2019 21:38:09 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <7EF663D6-87CA-47D6-B217-0C44746A6846@corp.mail.ru> References: <20190809081022.1237-1-m.melentiev@corp.mail.ru> <20190809134251.GA6081@atlas> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Konstantin Osipov Cc: tarantool-patches@freelists.org, k.nazarov@corp.mail.ru Thanks for review and comments. I see multiple backward incompatibilities in suggested approach: 1) wrapper_cfg does not override config but provides defaults, overwriting only `username` and `background` options. 2) If I get the suggestion right, suggested final solution should work = this way: tarantoolctl start io.daemon() -- parent exits, child goes to bg exec(init_script, env_with_box_cfg_overrides) init_script box.cfg() -- uses overrides from env tarantoolctl will always return with 0 code, no mater if there is error = in main script or not. Maybe it=E2=80=99s just wrong naming and it should be `io.fork` instead, = allowing parent to continue and wait for READY=3D1 on NOTIFY_SOCKET. If it is expected to work this way=20 tarantoolctl start exec(init_script, env_with_box_cfg_overrides) if (ENV.TNT_DAEMONIZE) io.daemon() -- parent exits, child goes to bg init_script box.cfg() -- uses overrides from env it also won=E2=80=99t show errors from child process. 3) io.daemon is expected to close STD* FDs in child process, isn=E2=80=99t= it? If so - child won=E2=80=99t be able to show errors during startup after = daemonization if any. Current implementation of `tarantoolctl start` shows this errors and = exits with non-zero code. 4) `tarantoolctl start` would exit before init_script is finished = (actually even before it=E2=80=99s started). Are this incompatibilities acceptable? Tarantool=E2=80=99s built-in toolset (tarantoolctl) does not work for = app-server applications but only for DB apps. The changes you=E2=80=99re suggesting are more robust and complex, it = may take significant amount of time to resolve all issues and consider all details. This patch solves the = problem in backward-compatible way right now. We would like to open-source modules which makes it much easier to use = tarantool as app server soon. The issue brings a limitation: =E2=80=9CDon=E2=80=99t use this module = with tarantoolctl (because it=E2=80=99s buggy)=E2=80=9D. > On 9 Aug 2019, at 16:42, Konstantin Osipov = wrote: >=20 > * Max Melentiev [19/08/09 11:12]: >> `box.cfg` may not be called inside init script and tarantool >> will not be daemonized (forcing `background =3D true` in hijacked = `box.cfg`), >> so we need to fork tarantool before running this script. >>=20 >> To find out the moment when tarantool finishes startup it waits for = `READY=3D1` >> message on NOTIFY_SOCKET which is emitted after init script is = finished. >=20 > Thanks for the patch. >=20 > Most of this patch has to be done on the server level. >=20 > Please prepare a patch that allows a user to override box.cfg with > environment variables. Since this is a user-visible change, it > needs an issue at github. >=20 >=20 > Basically I should be able to say:=20 >=20 > setenv TARANTOOL_WAL_DIR=3D"path" and this would override box.cfg > setting, if any. >=20 > The same is true for all variables which are overrides in > /etc/default: >=20 > = https://github.com/tarantool/tarantool/blob/master/extra/dist/default/tara= ntool.in >=20 > Alternatively, you could have a single environment variable to > force box.cfg to look at /etc/default/tarantool file, or some > other file, for these defaults: >=20 > TARANTOOL_DEFAULT_FILE=3D >=20 > Please check with the solutions team which approach is better - I > prefer the second one, since it requires less maintenance when new > defaults are added to /etc/default. >=20 > Once this patch is in place, tarantoolctl should be changed to > exec() instance file, not dofile() it. >=20 > This will solve most of the problems reflected this patch tries to > address (and a few more). >=20 > The next patch should split daemon features away from box.cfg, so > that it would be possible to become daemon before calling box.cfg. > We already can configure logger before box.cfg, and on the road of > making listen independent of box.cfg. Daemon is an easy step in > the same direction. This patch is also against the core server,not > against tarantoolctl. >=20 > It is easy to do, take a look here: >=20 > https://github.com/tarantool/tarantool/blob/master/src/main.cc#L387 >=20 > This function needs to become available via > io.daemon(pid_file_path). >=20 > io.daemon() should fail and do nothing if the logging subsystem is > not initialized. >=20 > There should be a test case in app-tap. >=20 > Please feel free to solicit help from the core team for this > patch. >=20 > If io.daemon() has been called, box.cfg.background=3Dtrue/false > should be ignored when box.cfg is called.=20 >=20 >=20 > io.daemon() should respect sysctl events if sysctl socket is > provided. >=20 >=20 > --=20 > Konstantin Osipov, Moscow, Russia