[Tarantool-patches] [PATCH] core: don't start example instance in postinstall

[Olga Arkhangelskaia]

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 at example stop || true
> +            else
> +                /etc/init.d/tarantool at 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