Re: [Xen-devel] [PATCH v2] hotplug/Linux: fix starting of xenstored with restarting systemd

[Olaf Hering <olaf@aepfle.de>]

[-- Attachment #1: Type: text/plain, Size: 6378 bytes --]

Ping?

On Fri, May 17, Olaf Hering wrote:

> 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.
> 
> The chain of events is basically: xenstored.service sends the
> notification to systemd, this is a one-way event. Then systemd may be
> restarted by the other unit. During this time xenstored.service is done
> and exits. Once systemd is done with its restart it collects the pending
> notifications and childs. If it does not find the unit which sent the
> notification it will declare it as failed.
> 
> A workaround for this scenario is to wait for a short time after sending
> to notification. If systemd happens to restart it will still find the
> unit it launched.
> 
> Adjust the callers of launch-xenstore to specifiy the init system.
> Do not fork xenstored with systemd, preserve pid.
> Be verbose about xenstored startup only with sysv to avoid interleaved
> output in systemd journal. Do the same also for domain case, even if is
> not strictly needed because init-xenstore-domain has no output.
> 
> The fix for upstream systemd which is supposed to fix it:
> 575b300b795b6 ("pid1: rework how we dispatch SIGCHLD and other signals")
> 
> v02:
> - preserve Type=notify
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  tools/hotplug/Linux/init.d/xencommons.in         |  2 +-
>  tools/hotplug/Linux/launch-xenstore.in           | 56 ++++++++++++++++++------
>  tools/hotplug/Linux/systemd/xenstored.service.in |  2 +-
>  3 files changed, 44 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in
> index 7fd6903b98..dcb0ce4b73 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' || 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..8ff243670d 100644
> --- a/tools/hotplug/Linux/launch-xenstore.in
> +++ b/tools/hotplug/Linux/launch-xenstore.in
> @@ -15,6 +15,16 @@
>  # License along with this library; If not, see <http://www.gnu.org/licenses/>.
>  #
>  
> +initd=$1
> +
> +case "$initd" in
> +	sysv|systemd) ;;
> +	*)
> +	echo "first argument must be 'sysv' or 'systemd'"
> +	exit 1
> +	;;
> +esac
> +
>  XENSTORED=@XENSTORED@
>  
>  . @XEN_SCRIPT_DIR@/hotplugpath.sh
> @@ -44,15 +54,9 @@ 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
> +run_xenstored () {
> +	local maybe_exec=$1
>  
> -/bin/mkdir -p @XEN_RUN_DIR@
> -
> -[ "$XENSTORETYPE" = "daemon" ] && {
>  	[ -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@
> @@ -60,13 +64,30 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
>  		echo "No xenstored found"
>  		exit 1
>  	}
> +	$maybe_exec $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
> +}
>  
> -	echo -n Starting $XENSTORED...
> -	$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
>  
> -	exit 0
> +[ "$XENSTORETYPE" = "" ] && XENSTORETYPE=daemon
> +
> +/bin/mkdir -p @XEN_RUN_DIR@
> +
> +[ "$XENSTORETYPE" = "daemon" ] && {
> +	if test "$initd" = 'sysv' ; then
> +		echo -n Starting $XENSTORED...
> +		run_xenstored ''
> +		timeout_xenstore $XENSTORED || exit 1
> +		exit 0
> +	else
> +		XENSTORED_ARGS="$XENSTORED_ARGS -N"
> +		run_xenstored 'exec'
> +		exit 1
> +	fi
>  }
>  
>  [ "$XENSTORETYPE" = "domain" ] && {
> @@ -76,9 +97,16 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF
>  	XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --memory $XENSTORE_DOMAIN_SIZE"
>  	[ -z "$XENSTORE_MAX_DOMAIN_SIZE" ] || XENSTORE_DOMAIN_ARGS="$XENSTORE_DOMAIN_ARGS --maxmem $XENSTORE_MAX_DOMAIN_SIZE"
>  
> -	echo -n Starting $XENSTORE_DOMAIN_KERNEL...
> +	if test "$initd" = 'sysv' ; then
> +		echo -n Starting $XENSTORE_DOMAIN_KERNEL...
> +	else
> +		echo Starting $XENSTORE_DOMAIN_KERNEL...
> +	fi
>  	${LIBEXEC_BIN}/init-xenstore-domain $XENSTORE_DOMAIN_ARGS || exit 1
> -	systemd-notify --ready 2>/dev/null
> +	if test "$initd" = 'systemd' ; then
> +		systemd-notify --ready
> +		sleep 9
> +	fi
>  
>  	exit 0
>  }
> diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
> index 80c1d408a5..c226eb3635 100644
> --- a/tools/hotplug/Linux/systemd/xenstored.service.in
> +++ b/tools/hotplug/Linux/systemd/xenstored.service.in
> @@ -11,7 +11,7 @@ Type=notify
>  NotifyAccess=all
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]