<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi Alexander!</p>
<p>Thank you very much for detailed feedback and pointing out the
things I even did not think about.</p>
<p>As I understand (correct me if I am wrong) in 2 cases:</p>
<p>1) Installing a package when previous that was removed but not
purged</p>
<p>2) Upgrading the package. <br>
</p>
<p>So if package already exists - all postrm, prerm are useless. <br>
</p>
<p>The only places we actually can take advantage is preinst and
postinst scripts. <br>
</p>
<p>Moreover, when <span style="color: rgb(62, 67, 73);
font-family: Arial, sans-serif; font-size: 14.4px; font-style:
normal; font-variant-ligatures: normal; font-variant-caps:
normal; font-weight: 400; letter-spacing: normal; orphans: 2;
text-align: center; text-indent: 0px; text-transform: none;
white-space: normal; widows: 2; word-spacing: 0px;
-webkit-text-stroke-width: 0px; background-color: rgb(255, 255,
255); text-decoration-style: initial; text-decoration-color:
initial; display: inline !important; float: none;">Installing a
package that was previously removed, but not purged</span> [2]
preinst scripts will be called from <br>
</p>
<p>previous version. So only postinst is left to stop existing
example instance and remove the link.</p>
<p>I am confused with the idea of pushing patch in later versions,
because it is rather surprising behaviour.<br>
</p>
<p>So I do not mind the first patch, however we still need to stop
running instances (in case they are still running).</p>
<p>The second thought I have that tracking one-time-actions is
actually good idea and may be useful so far(not only in this
case). And confuse user with disabling example instances (in case
when they are enabled again)every time it is bad idea. So the
second patch wins.</p>
<p>I guess I need to test it with full CI?<br>
</p>
<p>Moreover, I would write a comment why it is done in such a way
(with links to the diagram). Because I failed to find them when
writing the first version of patch.</p>
<br>
<div class="moz-cite-prefix">27.05.2020 19:02, Alexander Turenko
пишет:<br>
</div>
<blockquote type="cite"
cite="mid:20200527160242.akqs35kka34fj2cb@tkn_work_nb">
<pre class="moz-quote-pre" wrap="">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]: <a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/4507#issuecomment-634567994">https://github.com/tarantool/tarantool/issues/4507#issuecomment-634567994</a>
WBR, Alexander Turenko.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">core: don't start example instance in postinstall
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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]: <a class="moz-txt-link-freetext" href="https://www.tarantool.io/en/doc/2.4/getting_started/getting_started_db/#using-a-binary-package">https://www.tarantool.io/en/doc/2.4/getting_started/getting_started_db/#using-a-binary-package</a>
[2]: <a class="moz-txt-link-freetext" href="https://www.tarantool.io/en/doc/2.4/getting_started/using_binary/">https://www.tarantool.io/en/doc/2.4/getting_started/using_binary/</a>
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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">---
Branch:
<a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/tree/gh-4507-disable-start-example-instance">https://github.com/tarantool/tarantool/tree/gh-4507-disable-start-example-instance</a>
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> 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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
/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]: <a class="moz-txt-link-freetext" href="https://www.debian.org/doc/debian-policy/ch-maintainerscripts.html">https://www.debian.org/doc/debian-policy/ch-maintainerscripts.html</a>
[3]: <a class="moz-txt-link-freetext" href="https://www.debian.org/doc/debian-policy/ap-flowcharts.html">https://www.debian.org/doc/debian-policy/ap-flowcharts.html</a>
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" <a class="moz-txt-link-rfc2396E" href="mailto:$wantdir/tarantool@$instance.service">"$wantdir/tarantool@$instance.service"</a>
done
</pre>
</blockquote>
</body>
</html>