[libvirt] [PATCH v2] daemon: Update libvirtd SysV/upstart scripts to avoid confusion

[Daniel P. Berrange]

On Mon, Sep 26, 2011 at 04:42:33PM +0200, Peter Krempa wrote:
> On some systems init scripts are installed along with upstart . This may
> cause trouble if user tries to restart/stop a instance of libvirtd
> managed with upstart with init script.
> 
> Upstart config file uses "respawn" stanza to ensure that libvirtd is
> restarted after a crash/kill. This combined with a
> 
> The SysV init script is now able to detect libvirtd managed with upstart
> explicitly and notify the user about this. This patch also modifies the
> way the PID file is handled. Libvirtd alone removes the pid file on a
> successful exit, so now, the init script does not delete it. It's only
> deleted while starting libvirtd, when the script detects, that no
> libvirtd with pid specified in the pid file is running.
> 
> This patch also modifies the upstart configuration file for libvirt.
> Same logic as in the SysV script for handling pid files is used. The
> upstart script does not explicitly check for a SysV managed instance.
> Upstart alone prohibits to stop a not-started instance, so this issue is
> handled automaticaly.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=728153
> ---
> initctl_check() in the SysV init file is not strongly required. The purpose
> of this is to notify the user of the source of problem the user is experiencing.
> 
> It can be left out, but then no (reasonable) notification will be provided
> and the user might kill libvirtd managed by upstart (which will thereafter
> respawn)
> 
>  daemon/libvirtd.init.in |   43 +++++++++++++++++++++++++++++++++++++++++--
>  daemon/libvirtd.upstart |   31 ++++++++++++++++++++++++-------
>  2 files changed, 65 insertions(+), 9 deletions(-)
> 
> diff --git a/daemon/libvirtd.init.in b/daemon/libvirtd.init.in
> index 0697a2b..28801d1 100644
> --- a/daemon/libvirtd.init.in
> +++ b/daemon/libvirtd.init.in
> @@ -43,6 +43,8 @@ LIBVIRTD_CONFIG=
>  LIBVIRTD_ARGS=
>  KRB5_KTNAME=/etc/libvirt/krb5.tab
> 
> +INITCTL_PATH=/sbin/initctl
> +
>  test -f @sysconfdir@/sysconfig/libvirtd && . @sysconfdir@/sysconfig/libvirtd
> 
>  export QEMU_AUDIO_DRV
> @@ -56,8 +58,45 @@ fi
> 
>  RETVAL=0
> 
> +# Check if libvirt is managed by upstart and fail if it's the case
> +initctl_check() {
> +    if [ -x $INITCTL_PATH ]; then
> +        #extract status from upstart
> +        LIBVIRTD_UPSTART_STATUS=`$INITCTL_PATH status libvirtd | tr "/" " " | cut -d " " -f 2`
> +        if [ $LIBVIRTD_UPSTART_STATUS = "start" ]; then
> +            logger -t "libvirtd" -s  "libvirtd is managed by upstart and started, use initctl instead"
> +            exit 1
> +        fi
> +    fi
> +}

IMHO this has no business being here.

> +
> +# test if a pidfile exists and if there's a libvirtd process associated with it
> +pidfile_check() {
> +    #check if libvirtd is running
> +    if [ -f "$PIDFILE" ]; then
> +        PID=`cat $PIDFILE`
> +        if [ -n "$PID" ]; then
> +            PROCESSES=`pidof $PROCESS | grep $PID`
> +            if [ -n "$PROCESSES" ]; then
> +                logger -t "libvirtd" -s "$SERVICE with pid $PID is running";
> +                exit 1
> +            else
> +                # pidfile exists but no running libvirtd found
> +                # remove stuck pidfile
> +                rm -f $PIDFILE
> +            fi
> +        else
> +            # pidfile is empty
> +            rm -f $PIDFILE
> +        fi
> +    fi
> +}

This code is all bogus as of

  commit c8a3a265135a0527b46aeb0ebd39de8a03189fb0
  Author: Daniel P. Berrange <berrange at redhat.com>
  Date:   Fri Aug 5 15:11:11 2011 +0100

    Convert libvirtd to use crash-safe pidfile APIs

With this commit, there is no such thing as a stale pidfile anymore,
since we use a fcntl() lock for exclusivity, instead of merely the
existance of the pidfile on disk. In fact doing an 'rm -f' on the
pidfile here is actively harmful because it can delete a pidfile
that another process is in the middle of creating.


> diff --git a/daemon/libvirtd.upstart b/daemon/libvirtd.upstart
> index fd1d951..c506d45 100644
> --- a/daemon/libvirtd.upstart
> +++ b/daemon/libvirtd.upstart
> @@ -7,6 +7,29 @@ stop on runlevel [!345]
> 
>  respawn
> 
> +pre-start script
> +    PIDFILE=/var/run/libvirtd.pid
> +    #check if libvirtd is running
> +    if [ -f "$PIDFILE" ]; then
> +        PID=`cat $PIDFILE`
> +        if [ -n "$PID" ]; then
> +            PROCESSES=`pidof libvirtd | grep $PID`
> +
> +            if [ -n "$PROCESSES" ]; then
> +                logger -t "libvirtd" -s "error: libvirtd is already running with pid $PID"
> +                stop
> +                exit 0
> +            else
> +                # remove stuck pidfile
> +                rm -f $PIDFILE
> +            fi
> +        else
> +            # empty pidfile
> +            rm -f $PIDFILE
> +        fi
> +    fi
> +end script


This is all bogus for the same reason as above.

> @@ -31,16 +54,10 @@ script
>          ulimit -c "$DAEMON_COREFILE_LIMIT"
>      fi
> 
> -    # Clean up a pidfile that might be left around
> -    rm -f /var/run/libvirtd.pid

This is correct to remove though.

> -
> +    # No pid file and/or libvirtd not running.
>      mkdir -p /var/cache/libvirt
>      rm -rf /var/cache/libvirt/*
> 
>      exec /usr/sbin/libvirtd $LIBVIRTD_CONFIG_ARGS $LIBVIRTD_ARGS
>  end script
> 
> -post-stop script
> -    rm -f $PIDFILE

As is this


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|