[tarantool-patches] Re: [PATCH] Make `tarantoolctl start` work withiout box.cfg in init script

[Maxim Melentiev]

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’s just wrong naming and it should be `io.fork` instead, allowing parent to continue
and wait for READY=1 on NOTIFY_SOCKET.

If it is expected to work this way 

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’t show errors from child process.

3) io.daemon is expected to close STD* FDs in child process, isn’t it?
If so - child won’t 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’s started).

Are this incompatibilities acceptable?

Tarantool’s built-in toolset (tarantoolctl) does not work for app-server applications but only for DB apps.
The changes you’re 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: “Don’t use this module with tarantoolctl (because it’s buggy)”.

> On 9 Aug 2019, at 16:42, Konstantin Osipov <kostja at tarantool.org> wrote:
> 
> * Max Melentiev <dmarc-noreply at freelists.org> [19/08/09 11:12]:
>> `box.cfg` may not be called inside init script and tarantool
>> will not be daemonized (forcing `background = true` in hijacked `box.cfg`),
>> so we need to fork tarantool before running this script.
>> 
>> To find out the moment when tarantool finishes startup it waits for `READY=1`
>> message on NOTIFY_SOCKET which is emitted after init script is finished.
> 
> Thanks for the patch.
> 
> Most of this patch has to be done on the server level.
> 
> 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.
> 
> 
> Basically I should be able to say: 
> 
> setenv TARANTOOL_WAL_DIR="path" and this would override box.cfg
> setting, if any.
> 
> The same is true for all variables which are overrides in
> /etc/default:
> 
> https://github.com/tarantool/tarantool/blob/master/extra/dist/default/tarantool.in
> 
> 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:
> 
> TARANTOOL_DEFAULT_FILE=
> 
> 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.
> 
> Once this patch is in place, tarantoolctl should be changed to
> exec() instance file, not dofile() it.
> 
> This will solve most of the problems reflected this patch tries to
> address (and a few more).
> 
> 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.
> 
> It is easy to do, take a look here:
> 
> https://github.com/tarantool/tarantool/blob/master/src/main.cc#L387
> 
> This function needs to become available via
> io.daemon(pid_file_path).
> 
> io.daemon() should fail and do nothing if the logging subsystem is
> not initialized.
> 
> There should be a test case in app-tap.
> 
> Please feel free to solicit help from the core team for this
> patch.
> 
> If io.daemon() has been called, box.cfg.background=true/false
> should be ignored when box.cfg is called. 
> 
> 
> io.daemon() should respect sysctl events if sysctl socket is
> provided.
> 
> 
> -- 
> Konstantin Osipov, Moscow, Russia