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

[Olaf Hering <olaf@xxxxxxxxx>]

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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel

Attachment: signature.asc
Description: PGP signature