A hard to trigger race with another unrelated systemd service and
xenstored.service unveiled a bug in the way how xenstored is launched
with systemd.

launch-xenstore may start either a daemon or a domain. In case a domain
is used, systemd-notify was called. If another service triggered a
restart of systemd while xenstored.service was executed, systemd may
temporary lose track of services with Type=notify. As a result,
xenstored.service would be marked as failed and units that depend on it
will not be started anymore. This breaks the enire Xen toolstack.

Currently the decision which variant of xenstore should be used is
controlled via /etc/sysconfig/xencommons:XENSTORETYPE=[domain|daemon].
This change preserves this functionality for the sysv and systemd.

One way to fix it is to handle the domain case as Type=oneshot because
there is nothing to monitor for systemd. The daemon case has to be
handled as Type=simple, which is the default if no Type= is specified.
A single unit can have just one type, so a new unit for the daemon case
has to be created to preserve existing setups during upgrading of Xen.
This new xenstored-daemon.service is started on demand by the existing
xenstored.service. Since it is a separate unit, systemd will supervise
the xenstored daemon in dom0.

launch-xenstore expects now two arguments, the type of the init
system, and the optional xenstore type. The systemd-notify calls are
removed because systemd does not expect any notification with the
updated .service files. In case the init system is systemd the daemon
or init-xenstore-domain is started via exec to make sure systemd
monitors the process it has just launched. Various things specific to
either sysv or systemd are now handled separately.

The start of xenstored-daemon.service in launch-xenstore will
essentially call this helper twice. A separate internal state is
introduced to deal with this. This is required to handle the xencommons
case explained above.

A followup change will remove the code which calls to sd_notify, they
are not needed after the separation of Type=oneshot for domain and
Type=simple for daemon.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---

Tested with systemd on SLE15.
sysv case untested because support for SLE11 was dropped a while ago.

 tools/configure                                    |  3 +-
 tools/configure.ac                                 |  1 +
 tools/hotplug/Linux/init.d/xencommons.in           |  2 +-
 tools/hotplug/Linux/launch-xenstore.in             | 66 +++++++++++++++++-----
 .../Linux/systemd/xenstored-daemon.service.in      | 17 ++++++
 tools/hotplug/Linux/systemd/xenstored.service.in   |  5 +-
 6 files changed, 74 insertions(+), 20 deletions(-)
 create mode 100644 tools/hotplug/Linux/systemd/xenstored-daemon.service.in

diff --git a/tools/configure b/tools/configure
index 0be0be75de..2612c11490 100755
--- a/tools/configure
+++ b/tools/configure
@@ -9761,7 +9761,7 @@ fi
 
 if test "x$systemd" = "xy"; then :
 
-    ac_config_files="$ac_config_files hotplug/Linux/systemd/proc-xen.mount hotplug/Linux/systemd/var-lib-xenstored.mount hotplug/Linux/systemd/xen-init-dom0.service hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service hotplug/Linux/systemd/xen-watchdog.service hotplug/Linux/systemd/xenconsoled.service hotplug/Linux/systemd/xendomains.service hotplug/Linux/systemd/xendriverdomain.service hotplug/Linux/systemd/xenstored.service"
+    ac_config_files="$ac_config_files hotplug/Linux/systemd/proc-xen.mount hotplug/Linux/systemd/var-lib-xenstored.mount hotplug/Linux/systemd/xen-init-dom0.service hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service hotplug/Linux/systemd/xen-watchdog.service hotplug/Linux/systemd/xenconsoled.service hotplug/Linux/systemd/xendomains.service hotplug/Linux/systemd/xendriverdomain.service hotplug/Linux/systemd/xenstored.service hotplug/Linux/systemd/xenstored-daemon.service"
 
 
 fi
@@ -10516,6 +10516,7 @@ do
     "hotplug/Linux/systemd/xendomains.service") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/systemd/xendomains.service" ;;
     "hotplug/Linux/systemd/xendriverdomain.service") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/systemd/xendriverdomain.service" ;;
     "hotplug/Linux/systemd/xenstored.service") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/systemd/xenstored.service" ;;
+    "hotplug/Linux/systemd/xenstored-daemon.service") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/systemd/xenstored-daemon.service" ;;
 
   *) as_fn_error $? "invalid argument: \`$ac_config_target'" "$LINENO" 5;;
   esac
diff --git a/tools/configure.ac b/tools/configure.ac
index fcf282e74e..c61a327d73 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -481,6 +481,7 @@ AS_IF([test "x$systemd" = "xy"], [
     hotplug/Linux/systemd/xendomains.service
     hotplug/Linux/systemd/xendriverdomain.service
     hotplug/Linux/systemd/xenstored.service
+    hotplug/Linux/systemd/xenstored-daemon.service
     ])
 ])
 
diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in
index 7fd6903b98..e5a91350a2 100644
--- a/tools/hotplug/Linux/init.d/xencommons.in
+++ b/tools/hotplug/Linux/init.d/xencommons.in
@@ -60,7 +60,7 @@ do_start () {
 	mkdir -m700 -p ${XEN_LOCK_DIR}
 	mkdir -p ${XEN_LOG_DIR}
 
-	@XEN_SCRIPT_DIR@/launch-xenstore || exit 1
+	@XEN_SCRIPT_DIR@/launch-xenstore 'sysv' "${XENSTORETYPE}" || exit 1
 
 	echo Setting domain 0 name, domid and JSON config...
 	${LIBEXEC_BIN}/xen-init-dom0 ${XEN_DOM0_UUID}
diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in
index 991dec8d25..e97e339481 100644
--- a/tools/hotplug/Linux/launch-xenstore.in
+++ b/tools/hotplug/Linux/launch-xenstore.in
@@ -15,6 +15,26 @@
 # License along with this library; If not, see <http://www.gnu.org/licenses/>.
 #
 
+initd=$1
+xenstore_type=$2
+maybe_exec=
+
+case "$initd" in
+	sysv) ;;
+	systemd) maybe_exec='exec' ;;
+	*)
+	echo "first argument must be 'sysv' or 'systemd'"
+	exit 1
+	;;
+esac
+case "$xenstore_type" in
+	""|daemon|domain|supervised-by-systemd) ;;
+	*)
+	echo "second argument must be one of 'daemon',  'domain', 'supervised-by-systemd', or empty"
+	exit 1
+	;;
+esac
+
 XENSTORED=@XENSTORED@
 
 . @XEN_SCRIPT_DIR@/hotplugpath.sh
@@ -44,15 +64,7 @@ timeout_xenstore () {
 	return 0
 }
 
-test_xenstore && exit 0
-
-test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
-
-[ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon
-
-/bin/mkdir -p @XEN_RUN_DIR@
-
-[ "$XENSTORETYPE" = "daemon" ] && {
+run_xenstored () {
 	[ -z "$XENSTORED_ROOTDIR" ] && XENSTORED_ROOTDIR="@XEN_LIB_STORED@"
 	[ -z "$XENSTORED_TRACE" ] || XENSTORED_ARGS="$XENSTORED_ARGS -T @XEN_LOG_DIR@/xenstored-trace.log"
 	[ -z "$XENSTORED" ] && XENSTORED=@XENSTORED@
@@ -61,15 +73,40 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 		exit 1
 	}
 
-	echo -n Starting $XENSTORED...
-	$XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
+	${maybe_exec} $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
+}
+
+if test "$initd" = 'sysv' ; then
+	test_xenstore && exit 0
+fi
 
-	systemd-notify --booted 2>/dev/null || timeout_xenstore $XENSTORED || exit 1
+test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
+
+[ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon
+[ "$xenstore_type" = "" ] && xenstore_type="$XENSTORETYPE"
+
+/bin/mkdir -p @XEN_RUN_DIR@
+
+[ "$xenstore_type" = "supervised-by-systemd" ] && {
+	XENSTORED_ARGS="$XENSTORED_ARGS -N"
+	run_xenstored
+	exit 1
+}
+
+[ "$xenstore_type" = "daemon" ] && {
+
+	if test "$initd" = 'sysv' ; then
+		echo -n Starting $XENSTORED...
+		run_xenstored
+		timeout_xenstore $XENSTORED || exit 1
+	else
+		systemctl start xenstored-daemon.service
+	fi
 
 	exit 0
 }
 
-[ "$XENSTORETYPE" = "domain" ] && {
+[ "$xenstore_type" = "domain" ] && {
 	[ -z "$XENSTORE_DOMAIN_KERNEL" ] && XENSTORE_DOMAIN_KERNEL=@LIBEXEC@/boot/xenstore-stubdom.gz
 	XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --kernel $XENSTORE_DOMAIN_KERNEL"
 	[ -z "$XENSTORE_DOMAIN_SIZE" ] && XENSTORE_DOMAIN_SIZE=8
@@ -77,8 +114,7 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
 	[ -z "$XENSTORE_MAX_DOMAIN_SIZE" ] || XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --maxmem $XENSTORE_MAX_DOMAIN_SIZE"
 
 	echo -n Starting $XENSTORE_DOMAIN_KERNEL...
-	${LIBEXEC_BIN}/init-xenstore-domain $XENSTORE_DOMAIN_ARGS || exit 1
-	systemd-notify --ready 2>/dev/null
+	${maybe_exec} ${LIBEXEC_BIN}/init-xenstore-domain $XENSTORE_DOMAIN_ARGS || exit 1
 
 	exit 0
 }
diff --git a/tools/hotplug/Linux/systemd/xenstored-daemon.service.in b/tools/hotplug/Linux/systemd/xenstored-daemon.service.in
new file mode 100644
index 0000000000..5158df304b
--- /dev/null
+++ b/tools/hotplug/Linux/systemd/xenstored-daemon.service.in
@@ -0,0 +1,17 @@
+[Unit]
+Description=The xenstore daemon in dom0
+Requires=proc-xen.mount var-lib-xenstored.mount
+After=proc-xen.mount var-lib-xenstored.mount
+Before=libvirtd.service libvirt-guests.service
+RefuseManualStop=true
+ConditionPathExists=/proc/xen/capabilities
+
+[Service]
+PIDFile=@XEN_RUN_DIR@/xenstored.pid
+ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
+ExecStart=@XEN_SCRIPT_DIR@/launch-xenstore 'systemd' 'supervised-by-systemd'
+
+[Install]
+WantedBy=multi-user.target
+Also=proc-xen.mount
+Also=var-lib-xenstored.mount
diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
index 80c1d408a5..268e33399b 100644
--- a/tools/hotplug/Linux/systemd/xenstored.service.in
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -7,11 +7,10 @@ RefuseManualStop=true
 ConditionPathExists=/proc/xen/capabilities
 
 [Service]
-Type=notify
-NotifyAccess=all
+Type=oneshot
 RemainAfterExit=true
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
-ExecStart=@XEN_SCRIPT_DIR@/launch-xenstore
+ExecStart=@XEN_SCRIPT_DIR@/launch-xenstore 'systemd' ''
 
 [Install]
 WantedBy=multi-user.target

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Am Wed, 15 May 2019 17:06:32 +0200
schrieb Olaf Hering <olaf@aepfle.de>:

> A followup change will remove the code which calls to sd_notify, they
> are not needed after the separation of Type=oneshot for domain and
> Type=simple for daemon.

This is not accurate. Type=notify has to be used because units that do
depend on xenstored must be started once it is ready to serve them.
The sd_notify exists exactly for this purpose, systemd can not assume
a daemon is ready right after exec().

I will rework this patch.

Olaf
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel