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 D87B527432 for ; Tue, 20 Aug 2019 07:24:27 -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 CtgvhpoQbukK for ; Tue, 20 Aug 2019 07:24:27 -0400 (EDT) Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 26B2A273F2 for ; Tue, 20 Aug 2019 07:24:26 -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] tarantoolctl doesn't fail when box.cfg is delayed in init script From: "Maxim Melentiev" (Redacted sender "m.melentiev" for DMARC) In-Reply-To: <20190819150532.gh25tyc73mcfazeg@tkn_work_nb> Date: Tue, 20 Aug 2019 14:24:23 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20190819073936.37388-1-m.melentiev@corp.mail.ru> <20190819075457.GA18442@atlas> <59464C78-29E6-4F91-B8E9-CA01CB63FE65@corp.mail.ru> <20190819150532.gh25tyc73mcfazeg@tkn_work_nb> 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: tarantool-patches@freelists.org Cc: Maxim Melentiev , Konstantin Osipov , alexander.turenko@tarantool.org > 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=E2=80=99s binary replacing ctl = for cluster apps. Thank you for review. Please, ignore this patch request. Best regards, Max =46rom 1f53e9582ecf311323f88f9ac9b23a329072f901 Mon Sep 17 00:00:00 2001 From: Max Melentiev 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 =3D getmetatable(box.cfg) + local orig_cfg_call =3D box_cfg_mt.__call + box_cfg_mt.__call =3D function(old_cfg, new_cfg) + if old_cfg.pid_file ~=3D nil and new_cfg ~=3D nil and = new_cfg.pid_file ~=3D nil then + new_cfg.pid_file =3D 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 =3D getmetatable(box.cfg) - if box_cfg_mt =3D=3D nil then - log.error('box.cfg() is not called in an instance file') - os.exit(1) - end - local old_call =3D box_cfg_mt.__call - box_cfg_mt.__call =3D function(old_cfg, cfg) - if old_cfg.pid_file ~=3D nil and cfg ~=3D nil and cfg.pid_file = ~=3D nil then - cfg.pid_file =3D old_cfg.pid_file - end - old_call(old_cfg, cfg) - end return 0 end -- 2.21.0 > On 19 Aug 2019, at 18:05, Alexander Turenko = wrote: >=20 > So, in brief the patch can be described in the following way? >=20 > | tarantoolctl patches (wraps) box.cfg function two times: before an > | init script to set default values from > | /etc/{default,sysconfig}/tarantool and after the init script to > | discard changing of a pid file during an instance work time. > | > | The second patching procedure fails if an instance file did not call > | box.cfg{} before an init script ends. Other then that an instance, > | which calls box.cfg{} in, say, a background fiber, works almost fine > | (see below). > | > | The patch moves the second patching procedure into the box.cfg > | wrapper created during the first patching. So the second patching > | procedure is called only after box.cfg{} was invoked second or next > | time, so it does not fails anymore as it did before. > | > | However there are the following relatively minor flaws for > | applications that invoked box.cfg{} in background: > | > | >=20 > I don't insist on my wording, but want to verify my understanding. >=20 > If it is so, then I don't have objections against the patch. You need > however pass the formal process: file an issue, ask one of maintainers > to set it to a right milestone (if it is 1.10 your business need = should > be really important), fix the issue from the commit message, help > documentation team with documenting the new feature right when the = patch > will be merged (they maybe will ask you something, maybe don't -- but = I > suggest to monitor the tarantool/doc issue when it will be created). >=20 > BTW, is this is enough for you in the scope of your task or we need to > boost other tarantoolctl discussions very soon? >=20 > See other comments below. >=20 > WBR, Alexander Turenko. >=20 > On Mon, Aug 19, 2019 at 12:48:22PM +0300, Maxim Melentiev wrote: >> =46rom cd94e49e7b23a30ed05e574eb8441b477fc1c47a Mon Sep 17 00:00:00 = 2001 >> From: Max Melentiev >> Date: Mon, 19 Aug 2019 10:35:55 +0300 >> Subject: [PATCH] tarantoolctl: allow to start instances without a = box.cfg{} >=20 > It is a bit confusing: AFAIU it allows to call box.cfg{} in background > after an init script execution. But it does not allow to miss = box.cfg{} > at all. >=20 >>=20 >> Before this patch, tarantoolctl would fail if box.cfg{} was not >> called in an instance script. It used too patch box.cfg >> once again after init script to prevent chainging pid file. >=20 > Typo: chainging. >=20 >>=20 >> This patch allows to start an instance without immediate call to >> box.cfg{} in it. For example, managed instances which receive >> configuration from external server. >>=20 >> @TarantoolBot document >> Title: tarantoolctl allows to start instances without a box.cfg{} >>=20 >> tarantoolctl now works for instances without box.cfg{} or >> with dealyed box.cfg{} call. This can be managed instances which = receive >=20 > Typo: dealyed. >=20 >> configuration from external server. >>=20 >> 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 bockgrounding on their side. >=20 > Typo: bockgrounding. >=20 >> --- >> extra/dist/tarantoolctl.in | 21 +++++++++------------ >> 1 file changed, 9 insertions(+), 12 deletions(-) >>=20 >> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in >> index 8adb57533..fd1abf9dc 100755 >> --- a/extra/dist/tarantoolctl.in >> +++ b/extra/dist/tarantoolctl.in >> @@ -483,6 +483,15 @@ local function wrapper_cfg(cfg) >> os.exit(1) >> end >>=20 >> + local box_cfg_mt =3D getmetatable(box.cfg) >> + local orig_cfg_call =3D box_cfg_mt.__call >> + box_cfg_mt.__call =3D function(old_cfg, new_cfg) >> + if old_cfg.pid_file ~=3D nil and new_cfg ~=3D nil and = new_cfg.pid_file ~=3D nil then >> + new_cfg.pid_file =3D old_cfg.pid_file >> + end >> + orig_cfg_call(old_cfg, new_cfg) >> + end >> + >=20 > Once you are here it would be good to add a comment what this block > does. >=20 >=20 >> return data >> end >>=20 >> @@ -547,18 +556,6 @@ local function start() >> end >> os.exit(1) >> end >> - local box_cfg_mt =3D getmetatable(box.cfg) >> - if box_cfg_mt =3D=3D nil then >> - log.error('box.cfg() is not called in an instance file') >> - os.exit(1) >> - end >> - local old_call =3D box_cfg_mt.__call >> - box_cfg_mt.__call =3D function(old_cfg, cfg) >> - if old_cfg.pid_file ~=3D nil and cfg ~=3D nil and = cfg.pid_file ~=3D nil then >> - cfg.pid_file =3D old_cfg.pid_file >> - end >> - old_call(old_cfg, cfg) >> - end >> return 0 >> end >>=20 >> -- >> 2.21.0 >>=20 >>=20 >>> On 19 Aug 2019, at 10:54, Konstantin Osipov = wrote: >>>=20 >>> * Max Melentiev [19/08/19 10:40]: >>>> Before this patch tarantoolctl failed with error if box.cfg >>>> was not called in init script because it used too patch box.cfg >>>> once again after init script. I've changed it to patch box.cfg >>>> second time inside wrapper_cfg. >>>=20 >>> Please explain better what this patch does, not how you achieved >>> that. >>>=20 >>> And while you are at it, explain why we need a secondary patching >>> of box.cfg at all. What's the point of preserving the pid file? >>>=20 >>> tarantoolctl: allow to start instances without a box.cfg{} >>>=20 >>> Before this patch, tarantoolctl would fail if box.cfg{} was not >>> called in an instance script. >>>=20 >>> This patch allows to start an instance without box.cfg{} in it.=20 >>> Such an instance: >>> - may only be started under systemd?=20 >>> - what can it do?=20 >>> - ??? >>>=20 >>> Please add a docboc entry, since it 's a significant behaviour >>> change. How such an instance is used?=20 >>>=20 >>> Otherwise LTGM. >>>=20 >>>=20 >>>> --- >>>> extra/dist/tarantoolctl.in | 21 +++++++++------------ >>>> 1 file changed, 9 insertions(+), 12 deletions(-) >>>>=20 >>>> diff --git a/extra/dist/tarantoolctl.in = b/extra/dist/tarantoolctl.in >>>> index 8adb57533..fd1abf9dc 100755 >>>> --- a/extra/dist/tarantoolctl.in >>>> +++ b/extra/dist/tarantoolctl.in >>>> @@ -483,6 +483,15 @@ local function wrapper_cfg(cfg) >>>> os.exit(1) >>>> end >>>>=20 >>>> + local box_cfg_mt =3D getmetatable(box.cfg) >>>> + local orig_cfg_call =3D box_cfg_mt.__call >>>> + box_cfg_mt.__call =3D function(old_cfg, new_cfg) >>>> + if old_cfg.pid_file ~=3D nil and new_cfg ~=3D nil and = new_cfg.pid_file ~=3D nil then >>>> + new_cfg.pid_file =3D old_cfg.pid_file >>>> + end >>>> + orig_cfg_call(old_cfg, new_cfg) >>>> + end >>>> + >>>> return data >>>> end >>>>=20 >>>> @@ -547,18 +556,6 @@ local function start() >>>> end >>>> os.exit(1) >>>> end >>>> - local box_cfg_mt =3D getmetatable(box.cfg) >>>> - if box_cfg_mt =3D=3D nil then >>>> - log.error('box.cfg() is not called in an instance file') >>>> - os.exit(1) >>>> - end >>>> - local old_call =3D box_cfg_mt.__call >>>> - box_cfg_mt.__call =3D function(old_cfg, cfg) >>>> - if old_cfg.pid_file ~=3D nil and cfg ~=3D nil and = cfg.pid_file ~=3D nil then >>>> - cfg.pid_file =3D old_cfg.pid_file >>>> - end >>>> - old_call(old_cfg, cfg) >>>> - end >>>> return 0 >>>> end >>>=20 >>> --=20 >>> Konstantin Osipov, Moscow, Russia >>=20 >>=20 >=20