From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (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 8C59D469710 for ; Thu, 28 May 2020 12:29:06 +0300 (MSK) References: <20200429122038.53296-1-arkholga@tarantool.org> <20200527160242.akqs35kka34fj2cb@tkn_work_nb> From: Olga Arkhangelskaia Message-ID: <40b0d937-6f31-b11a-13e7-642f931672e6@tarantool.org> Date: Thu, 28 May 2020 12:28:57 +0300 MIME-Version: 1.0 In-Reply-To: <20200527160242.akqs35kka34fj2cb@tkn_work_nb> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB 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 I have got branch for full CI. See https://gitlab.com/tarantool/tarantool/pipelines/150018512 27.05.2020 19:02, Alexander Turenko пишет: > 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. > > See comments for the patch itself below. > > [1]: https://github.com/tarantool/tarantool/issues/4507#issuecomment-634567994 > > WBR, Alexander Turenko. > >> core: don't start example instance in postinstall > Nit: I would use prefix 'build' here. 'core' is usually about the code > that resides in src/lib/core. > > On Wed, Apr 29, 2020 at 03:20:38PM +0300, Olga Arkhangelskaia wrote: >> After taratool installation on Debuan/Ubuntu from repo, example instance >> was automatically started over 3301 port. On the other hand example instance >> on RHEL/CentOS is started manually. Patch does the same for Debian/Ubuntu. >> >> Closes #4507 >> >> @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. > >> --- >> Branch: >> https://github.com/tarantool/tarantool/tree/gh-4507-disable-start-example-instance > I would check it on a -full-ci branch just in case. Debian package build > is changed and we should at least ensure that packages are build. > > Hope we'll add such builds to verify each push to a developer branch, > but now it is only on -full-ci and release branches. > >> 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`. > > 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. > > 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. > > 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 > > First, we can compare 'previously configured' and 'newly configured' > versions on 'postinst configure' stage of tarantool-common. This, > however, both ugly and fragile. It is ugly, because we don't know exact > four-digits version at which the patch will land into all long-term > branches: so we'll likely remove the symlink during several upgrades, > not only one. It is fragile, because it will not be reliable if we'll > decide to push the patch to, say, 1.10 later (it should be included into > the condition of the master patch). > > It would look like so (not tested): > > diff --git a/debian/tarantool-common.postinst b/debian/tarantool-common.postinst > index 03e4b2215..17d6982c0 100644 > --- a/debian/tarantool-common.postinst > +++ b/debian/tarantool-common.postinst > @@ -22,6 +22,13 @@ 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 > + > + if [ -e /etc/tarantool/instances.enabled/example.lua ] && > + [ -n "$2" ] && (dpkg --compare-versions $2 lt 2.4.2 || > + (dpkg --compare-versions $2 gt 2.5.0 && > + dpkg --compare-versions $2 lt 2.5.1)); then > + rm /etc/tarantool/instances.enabled/example.lua > + fi > ;; > esac > > Then next way I considered is to add /etc/tarantool/debian/keep > directory, which will hold files that should be hold during upgrade. But > it looks weird: there is only one file we want to remove from the old > configuration during upgrade and unlikely there will be more. This > mechanism looks artificial here. > > 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). > > 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? > > ---- > > 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 > > 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