Comments on the debdiff:
diff -Nru docker.io-17.03.2/debian/docker.io.config
docker.io-17.03.2/debian/docker.io.config
--- docker.io-17.03.2/debian/docker.io.config 1970-01-01 00:00:00.000000000
+0000
+++ docker.io-17.03.2/debian/docker.io.config 2018-03-23 02:26:26.000000000
+0000
@@ -0,0 +1,11 @@
+#!/bin/sh
+set -e
+
+. /usr/share/debconf/confmodule
+
+db_version 2.0
+
+db_input high docker.io/restart || :
+db_go
+
+db_stop
This config script has the properties that:
- the first time the question is asked, it will (probably) be shown to the
user.
- on subsequent invocations of the package's maintainer scripts, the question
will not be shown.
This means that, once the user has answered the question, this behavior
will be sticky; every subsequent update of the package will run the risk
of either leaving the tools unable to talk to the daemon, or
unexpectedly stopping (and failing to restart) containers, resulting in
a service interruption.
The reason this is being done as a debconf question is because neither
option is good. And *since* we are forced to ask the user to pick their
poison, I consider it insufficient to ask this question only once. The
question should by default be re-asked, at each package upgrade, so that
the user has an opportunity to make the decision that is correct for
them at that moment.
The pattern for this is:
- no .config script
- call 'db_fset docker.io/restart seen false' in postinst
- call 'db_input high docker.io/restart' in postinst only if a restart may be
required
It would be ok to give the user an additional option to "always restart"
(so, a tristate question instead of a boolean); but at minimum users
must be able to select the behavior locally at the point of any given
upgrade.
In addition, the current default for the debconf question is "true"; so
by default, which includes on non-interactive upgrades (unattended
upgrades or the like), the behavior the user gets here is that the
service is restarted, stopping any un-supervised containers. If the
docker.io package is not able to detect whether all running containers
will be automatically restarted, then if this is to be a supported use
case at all, the default should be to not restart: because it is less
disruptive to have the docker service become unavailable, than to have
the docker containers become unavailable.
I am therefore rejecting the upload of 17.03.2-0ubuntu3~16.04.1.
Additional observations about the package, which are not blockers for an
SRU:
+shouldStartRestart=
+if [ "$1" != 'reconfigure' ] && [ -z "${DEBCONF_RECONFIGURE:-}" ]; then
+ # (if we're just doing "dpkg-reconfigure", there's no reason to
actually [re]start Docker)
+ shouldStartRestart=1
+fi
"reconfigure" is not a valid argument to a postinst script. (And dpkg-
reconfigure does not invoke it this way - it invokes it as 'postinst
configure'.) So the first part of this test is always true and can be
omitted.
+if \
+ [ "$RET" = 'true' ] \
+ && [ -n "$2" ] && [ "$1" = 'configure' ] \
+ && [ -n "$shouldStartRestart" ] \
+ && [ -x /etc/init.d/docker ] \
+ && invoke-rc.d --disclose-deny docker status > /dev/null 2>&1 \
The last two checks are not idiomatic. These may appear in debhelper
autogenerated snippets, but those need to account for a lot of corner
cases which shouldn't apply here. There should be no need to check for
/etc/init.d/docker's presence/executability as it should always exist
(anyone trying to disable a service by marking the init script not
executable is off the reservation). And the check for status is not
consistent with expected behavior of packages: upgrading the package
should normally start the service even if it was currently not running,
unless the administrator has explicitly disabled it through policy-rc.d.
+# because we had to use "dh_installinit --no-start", we get to make sure the
daemon is started on first install
+# (this is adapted from debhelper's "autoscripts/postinst-init")
+if [ "$1" = "configure" ] || [ "$1" = "abort-upgrade" ] || [ "$1" =
"abort-deconfigure" ] || [ "$1" = "abort-remove" ]; then
+ if [ -x /etc/init.d/docker ] && [ -n "$shouldStartRestart" ] && !
invoke-rc.d docker status > /dev/null 2>&1 ; then
+ invoke-rc.d docker start || :
+ fi
+fi
This code includes a check for "$shouldStartRestart"; but this is the
code to start the daemon on package initial installation, and the
debconf question doesn't ask about starting, only about restarting. So
I think this check shouldn't be there.
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1732368
Title:
upgrade to 17.03.2-ce in 16.04+
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/docker.io/+bug/1732368/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
