From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0B06D469710 for ; Fri, 5 Jun 2020 01:00:11 +0300 (MSK) Date: Fri, 5 Jun 2020 00:51:03 +0300 From: Igor Munkin Message-ID: <20200604215103.GK5745@tarantool.org> References: <20200429122038.53296-1-arkholga@tarantool.org> <20200527160242.akqs35kka34fj2cb@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200527160242.akqs35kka34fj2cb@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH] core: don't start example instance in postinstall List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@tarantool.org Sasha, Many thanks for the complex research and clarification! I dumped parts of our offline discussion below. Feel free to correct me if I'm wrong and add anything I missed. Long story short: comparing both approaches I personally prefer to apply the one from the origin patch and take some changes you provided below (e.g. empty directory handling). I also Cced Yaroslav as the person who faced the issue "for several times across these years". On 27.05.20, Alexander Turenko wrote: > I was wondered why the instance is started automatically and found past > discussions on this topic, see [1]. > > I also added some thoughts whether it is right to change this: I think > so, we should disable the default example instance. We need to define "disable" here. As we discussed there are two ways: * Just stop enabling (i.e. creating symlink for available instance to directory) the example.lua instance after installing or upgrading Tarantool and let users to control it on their own in future. * In addition to everything above, if example.lua instance is already running, stop it and disable (i.e. remove symlink to directory). Please correct me if I'm wrong here. > > See comments for the patch itself below. > > [1]: https://github.com/tarantool/tarantool/issues/4507#issuecomment-634567994 > > WBR, Alexander Turenko. > > > > > @TarantoolBot document > > Title: manage example instance after installation > > After tarantool installation from repos on all distros example instance > > should be managed manually. > > I verified, where the old behaviour is mentioned in the documentation: > > | Depending on the release, during installation Tarantool may start a > | demonstrative global example.lua instance that listens to the 3301 > | port by default. <...> > | > | <...> > | > | However, we encourage you to perform the instance startup manually, > | so you can learn. > > Cited from [1] and [2]. > > [1]: https://www.tarantool.io/en/doc/2.4/getting_started/getting_started_db/#using-a-binary-package > [2]: https://www.tarantool.io/en/doc/2.4/getting_started/using_binary/ > > In fact this wording does not contradicts with neither of the > behaviours. I guess it is better to left the documentation as is for a > while. Agree. > > > > debian/tarantool-common.postinst | 8 -------- > > 1 file changed, 8 deletions(-) > > > > diff --git a/debian/tarantool-common.postinst b/debian/tarantool-common.postinst > > index e2eda3415..03e4b2215 100644 > > --- a/debian/tarantool-common.postinst > > +++ b/debian/tarantool-common.postinst > > @@ -22,14 +22,6 @@ case "$1" in > > install -d -o$SYSUSER -gadm -m2750 /var/log/tarantool > > install -d -o$SYSUSER -g$SYSUSER -m750 /var/run/tarantool > > install -d -o$SYSUSER -g$SYSUSER -m750 /var/lib/tarantool > > - > > - # Enable example.lua by default > > - if [ -z $2 ] && [ ! -e /etc/tarantool/instances.enabled/example.lua ] && > > - [ -d /etc/tarantool/instances.enabled ] && > > - [ -d /etc/tarantool/instances.available/ ]; then > > - ln -s /etc/tarantool/instances.available/example.lua \ > > - /etc/tarantool/instances.enabled/example.lua > > - fi > > /etc/tarantool/instances.enabled/example.lua will not be removed > automatically even after tarantool-common upgrade, because the symlink > will be removed in debian/tarantool-common.postrm only at `apt-get purge > tarantool-common`. I guess it should not be removed (or even added) via package removal (installation). As we discussed Tarantool instances are not similar to NGINX or MySQL ones: user can run multiple instances and ought to maintain them all by himself. I see Tarantool as a *platform* that can be used in various ways: stateless application server, database or cache, or even Lua interpreter. Side note: It can substitute any of the letters or a combination of them in LAMP except the L one (well, honestly A is better substituted with NGINX, but no one forbids to bind Tarantool HTTP server on 80 port). > > One possible way is to left everything as is and mention necessary steps > to get rid from example.lua symlink (say, `apt-get purge > tarantool-common`, `apt-get install tarantool-common`) in release notes. AFAIR, it can be done without package re-installation but using service managing tools (i.e. systemctl, since systemd is the sole init system on deb-based distros today). > > However it looks as a lie: as user I would expect that the new behaviour > will become in effect just after upgrade. But we should not remove the > symlink every time when tarantool-common is updated, because a user may > want to use this example instance. Yes, but listen, if the example instance is not bothering you for a long period of time, you likely might have no idea it is running. Otherwise you've already disabled it once and hope it will not start again. But since the instance is *re-enabled* as a result of every update on deb-based distros, it makes you either change the distro or just crazy. > > I looked over the Debian maintainer scripts documentation ([2], [3]) and > found several ways to stop and disable an instance on upgrade. > > [2]: https://www.debian.org/doc/debian-policy/ch-maintainerscripts.html > [3]: https://www.debian.org/doc/debian-policy/ap-flowcharts.html > Oh, I snipped everything related to the first approach you described for the one reason: it's too fragile, artificial, ugly and what's more important doesn't differ from the one provided by Olya, since it just drops a symlink with no other benefits (at least I see not a one). > > So I implemented it in the migration-style way, see the patch below the > email. > > Please, note that enabled instances directory now can be empty and it > would be good to handle this case in the systemd unit generator file (it > is in the patch below too). Good catch! I guess this change should be applied anyway. > > To be honest, I still don't very like the result (those upgrade markers > right in /etc), but at least it looks like a general mechanism that may > be used again (but I hope it will not). If dpkg would provide some > database for performing migrations during updates, I would use it. But > it seems, no: it is overkill for dpkg (I don't found anything like > this). > > Olya, Igor, Oleg, can you look into this change and say what do you > think about? Personally, I think this solution is overcomplicated. User can (and must) manage his services by himself and I see the next problem is the root cause of the issue: even when user found the example instance running on his machine, once stopped and disabled it, the instance is *re-enabled* and ergo started after upgrading/reinstalling the package. As a result user has to fix the issue manually again and again. > > ---- > > diff --git a/debian/tarantool-common.postinst b/debian/tarantool-common.postinst > index 03e4b2215..e368cdd2e 100644 > --- a/debian/tarantool-common.postinst > +++ b/debian/tarantool-common.postinst > @@ -22,6 +22,46 @@ case "$1" in > install -d -o$SYSUSER -gadm -m2750 /var/log/tarantool > install -d -o$SYSUSER -g$SYSUSER -m750 /var/run/tarantool > install -d -o$SYSUSER -g$SYSUSER -m750 /var/lib/tarantool > + > + # Track configuration updates we want to perform only > + # once. > + UPGRADE_DIR="/etc/tarantool/debian/upgrade" > + mkdir -p "${UPGRADE_DIR}" > + > + # Add README file to /etc/tarantool/debian/upgrade > + # to don't confuse a user with this directory. > + UPGRADE_FILE_0000="${UPGRADE_DIR}/0000-add-readme.done" > + if [ ! -e "${UPGRADE_FILE_0000}" ]; then > + cat << EOF > "${UPGRADE_DIR}/README" > +There are rare cases, when existing configuration files > +content or structure should be upgraded when a new package > +version is installed. > + > +This directory tracks performed updates. > +EOF > + touch "${UPGRADE_FILE_0000}" > + fi > + > + # Stop and disable example.lua if it was started/enabled, > + # but do it only once in case a user actually want the > + # instance be enabled. > + UPGRADE_FILE_0001="${UPGRADE_DIR}/0001-disable-example-instance.done" > + if [ ! -e "${UPGRADE_FILE_0001}" ]; then > + # Stop the instance if it was started. > + if command -v invoke-rc.d > /dev/null; then > + invoke-rc.d tarantool@example stop || true > + else > + /etc/init.d/tarantool@example stop || true > + fi > + > + # Disable the instance, but keep it in > + # instances.available. > + if [ -e /etc/tarantool/instances.enabled/example.lua ]; then > + rm /etc/tarantool/instances.enabled/example.lua > + fi > + > + touch "${UPGRADE_FILE_0001}" > + fi > ;; > esac AFAICS, there are some files left on the system after this "migration" that are not tracked by the package. So they are not removed anymore even after the package is deinstalled. > > diff --git a/extra/dist/tarantool-generator.in b/extra/dist/tarantool-generator.in > index f6a6a2540..2048b7517 100755 > --- a/extra/dist/tarantool-generator.in > +++ b/extra/dist/tarantool-generator.in > @@ -13,6 +13,7 @@ mkdir -p "$wantdir" > > for file in @TARANTOOL_ENABLEDDIR@/*.lua; do > instance=`basename $file .lua` > + [ "${instance}" = "*" ] && break # skip empty directory > ln -s "$service" "$wantdir/tarantool@$instance.service" > done -- Best regards, IM