[tarantool-patches] Re: [PATCH] tarantoolctl doesn't fail when box.cfg is delayed in init script

[Alexander Turenko <alexander.turenko@tarantool.org>]

On Tue, Aug 20, 2019 at 02:24:23PM +0300, Maxim Melentiev wrote:
> > BTW, is this is enough for you in the scope of your task or we need to
> > boost other tarantoolctl discussions very soon?
> This should be enough to complete our task (with some limitations).
> 
> However we decided not to proceed with patching tarantoolctl right now
> and provide start/stop features in rock’s binary replacing ctl for cluster apps.
> Thank you for review. Please, ignore this patch request.

The patch looks good for me as well. Thanks!

I think that it is valuable even if your team does not needed it right
now.

CCed Kirill to decide about branches to which it should land.

Max, I guess a mainainer may ask you to provide a branch in
tarantool/tarantool repository with your commit at top of current
master.

Kirill, the patch has LGTMs from Kostya O. and me.

WBR, Alexander Turenko.

> Best regards,
> Max
> 
> From 1f53e9582ecf311323f88f9ac9b23a329072f901 Mon Sep 17 00:00:00 2001
> From: Max Melentiev <m.melentiev@corp.mail.ru>
> Date: Mon, 19 Aug 2019 10:35:55 +0300
> Subject: [PATCH] tarantoolctl: allow to start instances with delayed box.cfg{}
> 
> `tarantoolctl start` patches box.cfg two times:
> 1) before the init script to set default values and enforce some others,
> 2) after the init script to prevent changing a pid_file in runtime.
> 
> The second patching fails if an init file does not call
> box.cfg{} before it's finished. This can take a place in apps with
> managed instances which receive configuration from external server.
> 
> This patch moves the second patching into the box.cfg
> wrapper created during the first patching. So the second patching
> is performed only after box.cfg{} was invoked, and it does not fail anymore.
> 
> However there is relatively minor flaw for applications that
> invoke box.cfg{} after init script is finished:
> `tarantoolctl start` goes to background only when box.cfg{} is called.
> Though this is not the case for daemon management systems like systemd,
> as they handle backgrounding on their side.
> 
> Fixes #4435
> 
> @TarantoolBot document
> Title: tarantoolctl allows to start instances without a box.cfg{}
> 
> tarantoolctl now works for instances without box.cfg{} or
> with delayed box.cfg{} call. This can be managed instances which receive
> configuration from external server.
> 
> For such instances `tarantoolctl start` goes to background when
> box.cfg{} is called, so it will wait until options for box.cfg are received.
> However this is not the case for daemon management systems like systemd,
> as they handle backgrounding on their side.
> ---
>  extra/dist/tarantoolctl.in | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
> index 8adb57533..6daf866ac 100755
> --- a/extra/dist/tarantoolctl.in
> +++ b/extra/dist/tarantoolctl.in
> @@ -483,6 +483,16 @@ local function wrapper_cfg(cfg)
>          os.exit(1)
>      end
> 
> +    -- Prevent overwriting pid_file in subsequent box.cfg calls.
> +    local box_cfg_mt = getmetatable(box.cfg)
> +    local orig_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
> +        orig_cfg_call(old_cfg, new_cfg)
> +    end
> +
>      return data
>  end
> 
> @@ -547,18 +557,6 @@ local function start()
>          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)
> -    end
>      return 0
>  end
> 
> --
> 2.21.0