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

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 +} + +# 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 +} + start() { echo -n $"Starting $SERVICE daemon: " + initctl_check + pidfile_check + mkdir -p @localstatedir@/cache/libvirt rm -rf @localstatedir@/cache/libvirt/* KRB5_KTNAME=$KRB5_KTNAME daemon --pidfile $PIDFILE --check $SERVICE $PROCESS --daemon $LIBVIRTD_CONFIG_ARGS $LIBVIRTD_ARGS @@ -68,14 +107,13 @@ start() { stop() { echo -n $"Stopping $SERVICE daemon: " + initctl_check killproc -p $PIDFILE $PROCESS RETVAL=$? echo if [ $RETVAL -eq 0 ]; then rm -f @localstatedir@/lock/subsys/$SERVICE - rm -f $PIDFILE - rm -rf @localstatedir@/cache/libvirt/* else exit $RETVAL fi @@ -88,6 +126,7 @@ restart() { reload() { echo -n $"Reloading $SERVICE configuration: " + initctl_check killproc -p $PIDFILE $PROCESS -HUP RETVAL=$? 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 + script LIBVIRTD_CONFIG= LIBVIRTD_ARGS= @@ -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 - + # 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 - rm -rf /var/cache/libvirt/* -end script -- 1.7.3.4

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@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 :|

Init scripts removed pid file of the daemon. Removing pid files may be harmful as new api for crash-safe pidfiles is used (introduced by c8a3a26). --- daemon/libvirtd.init.in | 1 - daemon/libvirtd.upstart | 4 ---- 2 files changed, 0 insertions(+), 5 deletions(-) diff --git a/daemon/libvirtd.init.in b/daemon/libvirtd.init.in index 0697a2b..4e610cb 100644 --- a/daemon/libvirtd.init.in +++ b/daemon/libvirtd.init.in @@ -74,7 +74,6 @@ stop() { echo if [ $RETVAL -eq 0 ]; then rm -f @localstatedir@/lock/subsys/$SERVICE - rm -f $PIDFILE rm -rf @localstatedir@/cache/libvirt/* else exit $RETVAL diff --git a/daemon/libvirtd.upstart b/daemon/libvirtd.upstart index fd1d951..f51701a 100644 --- a/daemon/libvirtd.upstart +++ b/daemon/libvirtd.upstart @@ -31,9 +31,6 @@ script ulimit -c "$DAEMON_COREFILE_LIMIT" fi - # Clean up a pidfile that might be left around - rm -f /var/run/libvirtd.pid - mkdir -p /var/cache/libvirt rm -rf /var/cache/libvirt/* @@ -41,6 +38,5 @@ script end script post-stop script - rm -f $PIDFILE rm -rf /var/cache/libvirt/* end script -- 1.7.3.4

On 09/26/2011 12:17 PM, Peter Krempa wrote:
Init scripts removed pid file of the daemon. Removing pid files may be harmful as new api for crash-safe pidfiles is used (introduced by c8a3a26). --- daemon/libvirtd.init.in | 1 - daemon/libvirtd.upstart | 4 ---- 2 files changed, 0 insertions(+), 5 deletions(-)
ACK - now that the patch has been pruned down in effect, it is easier to say that this particular patch is correct (although harder to say whether it is comprehensive, or whether we will yet end up writing more patches for init script behaviors). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Mon, Sep 26, 2011 at 08:17:23PM +0200, Peter Krempa wrote:
Init scripts removed pid file of the daemon. Removing pid files may be harmful as new api for crash-safe pidfiles is used (introduced by c8a3a26). --- daemon/libvirtd.init.in | 1 - daemon/libvirtd.upstart | 4 ---- 2 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/daemon/libvirtd.init.in b/daemon/libvirtd.init.in index 0697a2b..4e610cb 100644 --- a/daemon/libvirtd.init.in +++ b/daemon/libvirtd.init.in @@ -74,7 +74,6 @@ stop() { echo if [ $RETVAL -eq 0 ]; then rm -f @localstatedir@/lock/subsys/$SERVICE - rm -f $PIDFILE rm -rf @localstatedir@/cache/libvirt/* else exit $RETVAL diff --git a/daemon/libvirtd.upstart b/daemon/libvirtd.upstart index fd1d951..f51701a 100644 --- a/daemon/libvirtd.upstart +++ b/daemon/libvirtd.upstart @@ -31,9 +31,6 @@ script ulimit -c "$DAEMON_COREFILE_LIMIT" fi
- # Clean up a pidfile that might be left around - rm -f /var/run/libvirtd.pid - mkdir -p /var/cache/libvirt rm -rf /var/cache/libvirt/*
@@ -41,6 +38,5 @@ script end script
post-stop script - rm -f $PIDFILE rm -rf /var/cache/libvirt/* end script
ACK 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 :|
participants (3)
-
Daniel P. Berrange
-
Eric Blake
-
Peter Krempa