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

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

[Max Melentiev]

`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.


Patch can be observed at https://github.com/tarantool/tarantool/pull/4381
It solves issue descrcibed here https://github.com/tarantool/tarantool/issues/4411
---
 extra/dist/tarantoolctl.in | 141 +++++++++++++++++++++++++++++--------
 1 file changed, 113 insertions(+), 28 deletions(-)

diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index 8adb57533..cdb601321 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -428,7 +428,9 @@ local cat_formats = setmetatable({
 -- -------------------------------------------------------------------------- --
 --                               Commands                                     --
 -- -------------------------------------------------------------------------- --
-local orig_cfg = box.cfg
+local orig_box_cfg = box.cfg
+-- Last configured log device to fetch logs from
+local configured_log_device

 local function wrapper_cfg(cfg)
     fiber.name(instance_name, {truncate=true})
@@ -464,23 +466,19 @@ local function wrapper_cfg(cfg)
     else
         cfg.username = nil
     end
-    if os.getenv("NOTIFY_SOCKET") then
-        cfg.background = false
-    elseif cfg.background == nil then
-        cfg.background = true
-    end
+    cfg.background = false

     mk_default_dirs(cfg)
-    local success, data = pcall(orig_cfg, cfg)
-    if not success then
-        log.error("Configuration failed: %s", data)
-        if type(cfg) ~= 'function' then
-            local log_type, log_args = logger_parse(cfg.log)
-            if log_type == 'file' and fio.stat(log_args) then
-                os.execute('tail -n 10 ' .. log_args)
-            end
+    configured_log_device = cfg.log
+    local data = orig_box_cfg(cfg)
+
+    local box_cfg_mt = getmetatable(box.cfg)
+    local orig_box_cfg_call = box_cfg_mt.__call
+    box_cfg_mt.__call = function(old_cfg, new_cfg)
+        if old_cfg.pid_file ~= nil and new_cfg ~= nil and new_cfg.pid_file ~= nil then
+            new_cfg.pid_file = old_cfg.pid_file
         end
-        os.exit(1)
+        return orig_box_cfg_call(old_cfg, new_cfg)
     end

     return data
@@ -506,6 +504,90 @@ local function start_check()
     return pid
 end

+ffi.cdef([[
+    pid_t fork(void);
+    int execve(const char *pathname, char *const argv[], char *const envp[]);
+    int dup2(int oldfd, int newfd);
+    int fileno(struct FILE *stream);
+]])
+
+local function ffi_table_to_const_char(input)
+    local result = ffi.new('char const*[?]', #input + 1, input)
+    result[#input] = nil
+    return ffi.cast('char *const*', result)
+end
+
+-- Starts process and returns immediately, not waiting until process is finished.
+-- @param path Executable path.
+-- @param[opt] args
+-- @param[opt] env
+local function fork_execve(path, args, env)
+    args = args or {}
+    env = env or {}
+    table.insert(args, 1, path)
+    local argv = ffi_table_to_const_char(args)
+    local env_list = fun.iter(env):map(function(k, v) return k .. '=' .. v end):totable()
+    local envp = ffi_table_to_const_char(env_list)
+    local pid = tonumber(ffi.C.fork())
+    if pid == -1 then
+        error('fork failed: ' .. errno.strerror())
+    elseif pid > 0 then
+        return pid
+    end
+    ffi.C.execve(path, argv, envp)
+    io.stderr:write('execve failed: ' .. errno.strerror() .. '\n')
+    os.exit(1)
+end
+
+-- Create socket and run forked tarantool with NOTIFY_SOCKET.
+--
+-- `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.
+local function fork_and_start_over()
+    local sock = assert(socket('AF_UNIX', 'SOCK_DGRAM', 0), 'Can not create socket')
+    local sock_name = '/tmp/tntctl-' .. require('uuid').str()
+    local ok = sock:bind('unix/', sock_name)
+    assert(ok, sock:error())
+    fio.chown(sock_name, default_cfg.username, group_name)
+    fio.chmod(sock_name, tonumber('0770', 8))
+
+    local env = table.copy(os.environ())
+    env.NOTIFY_SOCKET = sock_name
+    env.TARANTOOLCTL_DAEMONIZED = 'true'
+    -- slice 1..#arg indices into array-table
+    local args = fun.iter(arg):totable()
+    table.insert(args, 1, arg[0])
+    local pid = fork_execve(arg[-1], args, env)
+
+    local function exit(code)
+        fio.unlink(sock_name)
+        os.exit(code)
+    end
+
+    local function check_process_alive()
+        if os.execute('ps -p ' .. pid .. ' > /dev/null') ~= 0 then
+            exit(1)
+        end
+    end
+
+    local POLL_SOCKET_TIMEOUT = 1 -- sec.
+    while true do
+        check_process_alive()
+        if sock:readable(POLL_SOCKET_TIMEOUT) then
+            local str = sock:recv()
+            if str:match('READY=1') then
+                exit(0)
+            end
+            log.info(str)
+        end
+        check_process_alive()
+    end
+end
+
 local function start()
     log.info("Starting instance %s...", instance_name)
     if forward_to_systemd() then
@@ -524,6 +606,10 @@ local function start()
         log.error("The daemon is already running: PID %s", pid)
         os.exit(1)
     end
+    if not os.getenv('NOTIFY_SOCKET') then
+        return fork_and_start_over()
+    end
+
     box.cfg = wrapper_cfg
     require('title').update{
         script_name = instance_path,
@@ -539,26 +625,25 @@ local function start()
     -- if load fails - show last 10 lines of the log file and exit
     if not success then
         log.error("Start failed: %s", data)
-        if type(box.cfg) ~= 'function' then
-            local log_type, log_args = logger_parse(box.cfg.log)
+        local log_device = type(box.cfg) ~= 'function' and box.cfg.log or configured_log_device
+        if log_device then
+            local log_type, log_args = logger_parse(log_device)
             if log_type == 'file' and fio.stat(log_args) then
                 os.execute('tail -n 10 ' .. log_args)
             end
         end
         os.exit(1)
     end
-    local box_cfg_mt = getmetatable(box.cfg)
-    if box_cfg_mt == nil then
-        log.error('box.cfg() is not called in an instance file')
-        os.exit(1)
-    end
-    local old_call = box_cfg_mt.__call
-    box_cfg_mt.__call = function(old_cfg, cfg)
-        if old_cfg.pid_file ~= nil and cfg ~= nil and cfg.pid_file ~= nil then
-            cfg.pid_file = old_cfg.pid_file
-        end
-        old_call(old_cfg, cfg)
+
+    -- close descriptors in forked process after init script is finished
+    -- to show possible errors and logs.
+    if os.getenv('TARANTOOLCTL_DAEMONIZED') then
+        local fd = fio.open('/dev/null', fio.c.flag.O_RDONLY)
+        ffi.C.dup2(fd.fh, ffi.C.fileno(io.stdout))
+        ffi.C.dup2(fd.fh, ffi.C.fileno(io.stderr))
+        fd:close()
     end
+
     return 0
 end

--
2.21.0

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

[Konstantin Osipov]

* Max Melentiev <dmarc-noreply@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

[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@tarantool.org> wrote:
> 
> * Max Melentiev <dmarc-noreply@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

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

[Konstantin Osipov]

* Maxim Melentiev <m.melentiev@corp.mail.ru> [19/08/09 21:39]:
> 
> 1) wrapper_cfg does not override config but provides defaults,
> overwriting only `username` and `background` options.

You can do the same with setenv.

> 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

Yes.

> 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.

No, the parent of io.daemon() can wait for the child to start.

> 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?

No, we need to get the same semantics as your patch and we'll find
a way to do it.

> 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)”.

Well, your patch is not good, even though it solves your issues
now. So let's take a longer route.


-- 
Konstantin Osipov, Moscow, Russia

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

[Georgy Kirichenko]

[-- Attachment #1: Type: text/plain, Size: 3209 bytes --]

On Monday, August 12, 2019 1:31:51 PM MSK Konstantin Osipov wrote:
> * Maxim Melentiev <m.melentiev@corp.mail.ru> [19/08/09 21:39]:
> > 1) wrapper_cfg does not override config but provides defaults,
> > overwriting only `username` and `background` options.
> 
> You can do the same with setenv.
> 
> > 2) If I get the suggestion right, suggested final solution should work
> > this way:
> > 
> > tarantoolctl start
> > 
> >   io.daemon() -- parent exits, child goes to bg
which way you suggest to use here to control execution state
> >   exec(init_script, env_with_box_cfg_overrides)
here to
> >   
> >     init_script
> >     
> >       box.cfg() -- uses overrides from env
> 
> Yes.
> 
> > 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.
> 
> No, the parent of io.daemon() can wait for the child to start.
What you mean for the word "start" - a process is created, a special event 
happened or something else?
> 
> > 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?
> 
> No, we need to get the same semantics as your patch and we'll find
> a way to do it.

> 
> > 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)”.
> Well, your patch is not good, even though it solves your issues
> now. So let's take a longer route.
The biggest issue I see we don't have any clue how to control application on 
each stage of it's life. How could application to report an error or unwanted 
state right after daemonization (or even after READY=1 written) but before 
application is really initialized. We do not have even logging here (logger is 
initialized while box.cfg fired).
I think we need a framework which allows to customize and to control 
application which includes logging and application state.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Konstantin Osipov]

* Georgy Kirichenko <georgy@tarantool.org> [19/08/12 16:05]:
> > No, the parent of io.daemon() can wait for the child to start.
> What you mean for the word "start" - a process is created, a special event 
> happened or something else?

an event is sent to systemd notify socket


-- 
Konstantin Osipov, Moscow, Russia

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

[Konstantin Osipov]

* Georgy Kirichenko <georgy@tarantool.org> [19/08/12 16:05]:
> The biggest issue I see we don't have any clue how to control application on 
> each stage of it's life. How could application to report an error or unwanted 
> state right after daemonization (or even after READY=1 written) but before 
> application is really initialized. We do not have even logging here (logger is 
> initialized while box.cfg fired).
> I think we need a framework which allows to customize and to control 
> application which includes logging and application state.

There is already such framework and we use it, it's systemd
socket.

-- 
Konstantin Osipov, Moscow, Russia

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

[Георгий Кириченко]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On Monday, August 12, 2019 4:24:52 PM MSK Konstantin Osipov wrote:
> * Georgy Kirichenko <georgy@tarantool.org> [19/08/12 16:05]:
> > The biggest issue I see we don't have any clue how to control application
> > on each stage of it's life. How could application to report an error or
> > unwanted state right after daemonization (or even after READY=1 written)
> > but before application is really initialized. We do not have even logging
> > here (logger is initialized while box.cfg fired).
> > I think we need a framework which allows to customize and to control
> > application which includes logging and application state.
> 
> There is already such framework and we use it, it's systemd
> socket.

There is completely no problem with systemd because there is nothing to do as 
system already able to daemonize a service and track it's state (including 
starting->started switch).

Out issue stems from a no-systemd environment when user starts service without 
root-privileges and/or on-systemd installations.


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Konstantin Osipov]

* Георгий Кириченко <georgy@tarantool.org> [19/08/12 19:12]:
> There is completely no problem with systemd because there is nothing to do as 
> system already able to daemonize a service and track it's state (including 
> starting->started switch).
> 
> Out issue stems from a no-systemd environment when user starts service without 
> root-privileges and/or on-systemd installations.

In no systemd environment we still can use systemd socket. This is
what the original patch does already. The child process can
communicate its state using this socket.

-- 
Konstantin Osipov, Moscow, Russia