[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Init script for suspending/resuming guests on shutdown/boot



On 05/14/2010 07:50 AM, Jiri Denemark wrote:
> Example output during shutdown:
> 
> Running guests on default URI: console, rhel6-1, rhel5-64
> Running guests on lxc:/// URI: lxc-shell
> Running guests on xen:/// URI: error: no hypervisor driver available for xen:///
> error: failed to connect to the hypervisor
> Running guests on vbox+tcp://orkuz/system URI: no running guests.
> Suspending guests on default URI...
> Suspending console: done
> Suspending rhel6-1: done
> Suspending rhel5-64: done
> Suspending guests on lxc:/// URI...
> Suspending lxc-shell: error: Failed to save domain 9cba8bfb-56f4-6589-2d12-8a58c886dd3b state
> error: this function is not supported by the hypervisor: virDomainManagedSave

Nice recipes for running multiple guests - I'll have to branch out on my
machine and run some non-qemu guests.

> +++ b/daemon/libvirt-guests.init.in
> @@ -0,0 +1,194 @@
> +#!/bin/sh

This script has bash-isms, but this situation is no different than
daemon/libvirtd.init.in - since we know that init scripts only run on
Fedora/RHEL systems where we are guaranteed that /bin/sh==bash, there's
no issue.

> +
> +# the following is the LSB init header see
> +# http://www.linux-foundation.org/spec//booksets/LSB-Core-generic/LSB-Core-generic.html#INITSCRCOMCONV

Is the second // necessary, or can it just be spec/booksets?  For that
matter, that URL didn't work for me; I found:

http://refspecs.freestandards.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html

> +
> +sysconfdir= sysconfdir@
> +localstatedir= localstatedir@
> +
> +# Source function library.
> +. $sysconfdir/rc.d/init.d/functions

Technically, it would be safer to quote $sysconfdir, even if in
practice, it never contains whitespace.

> +
> +URIS=default
> +
> +test -f $sysconfdir/sysconfig/libvirt-guests && . $sysconfdir/sysconfig/libvirt-guests

Likewise.  Also, should we fail if sourcing the config file failed?

> +
> +run_virsh() {
> +    uri=$1
> +    shift
> +
> +    if [ "x$uri" = xdefault ]; then
> +        conn=
> +    else
> +        conn="-c $uri"
> +    fi
> +
> +    virsh $conn "$@"

Fails if $uri contains spaces, but that's not a valid URI in the first
place, so no big deal.

> +}
> +
> +run_virsh_c() {
> +    ( export LC_ALL=C; run_virsh "$@" )
> +}
> +
> +list_guests() {
> +    uri=$1
> +
> +    list=`run_virsh_c $uri list`

While bashisms in init scripts is questionable, use of POSIX is not; you
can safely use $() instead of `` for readability.

> +    if [ $? -ne 0 ]; then
> +        RETVAL=1
> +        return 1
> +    fi
> +
> +    for id in `echo "$list" | awk 'NR > 2 {print $1}'`; do
> +        run_virsh_c $uri dominfo $id | awk '/^UUID:/{print $2}'
> +    done

Failure to run virsh doesn't affect our exit status?  Maybe that's okay,
but even so, we should probably log if a virsh invocation unexpectedly
fails.

> +}
> +
> +guest_name() {
> +    uri=$1
> +    uuid=$2
> +
> +    name=`run_virsh_c $uri dominfo $uuid 2>/dev/null | \
> +          awk '/^Name:/{print $2}'`
> +    [ -n "$name" ] || name=$uuid
> +
> +    echo "$name"
> +}
> +
> +guest_is_on() {
> +    uri=$1
> +    uuid=$2
> +
> +    id=`run_virsh_c $uri dominfo $uuid 2>/dev/null | \
> +        awk '/^Id:/{print $2}'`
> +
> +    [ -n "$id" ] && ! [ "x$id" = x- ]

If we were worried about portability to non-POSIX, I would have written
this as '[ "x$id" != x- ]' rather than '! [ "x$id" = x- ]', but either
way works here since we assume POSIX.

> +start() {
> +    while read uri list; do
> +        configured=false
> +        for confuri in $URIS; do
> +            if [ $confuri = $uri ]; then
> +                configured=true
> +                break
> +            fi
> +        done
> +        if ! $configured; then
> +            echo $"Ignoring guests on $uri URI"
> +            continue
> +        fi
> +
> +        echo $"Resuming guests on $uri URI..."
> +        for guest in $list; do
> +            name=`guest_name $uri $guest`
> +            echo -n $"Resuming guest $name: "

Bash-isms.  Neither 'echo -n' nor $"" are portable, but as this style is
used in LOTS of other places for i18n of init scripts, you are no worse
than existing style (that is, fixing all init scripts to use /bin/bash
instead of /bin/sh, or to use alternative constructs that are portable
to POSIX, is outside the scope of this patch).

> +            if retval guest_is_on $uri $guest; then
> +                echo $"already active"
> +            else
> +                retval run_virsh $uri start "$name" >/dev/null && echo $"done"
> +            fi
> +        done
> +    done <$LISTFILE

This fails if $LISTFILE does not exist, which will be the case if you
run start() twice in a row.  But LSB requires that a start() on an
already started service be a successful no-op.  Maybe all you need is a
line at the start of the function:
 test -f $LISTFILE || return 0

> +
> +    rm -f $LISTFILE
> +}
> +
> +stop() {
> +    >$LISTFILE

Bash-ism; trivial to use ': >$LISTFILE' instead to be portable to POSIX.

> +    for uri in $URIS; do
> +        echo -n $"Running guests on $uri URI: "
> +        list=`list_guests $uri`
> +        if [ $? -eq 0 ]; then
> +            empty=true
> +            for uuid in $list; do
> +                $empty || echo -n ", "

Another echo -n.  Unlike the $"" case (where the style is pervasive),
here, I'd like to see the use of 'printf ", "'.

> +                echo -n `guest_name $uri $uuid`
> +                empty=false
> +            done
> +            if $empty; then
> +                echo $"no running guests."
> +            else
> +                echo
> +                echo $uri $list >>$LISTFILE
> +            fi
> +        fi
> +    done
> +
> +    while read uri list; do
> +        echo $"Suspending guests on $uri URI..."
> +        for guest in $list; do
> +            name=`guest_name $uri $guest`
> +            label=$"Suspending $name: "
> +            echo -n "$label"
> +            run_virsh $uri managedsave $guest >/dev/null &
> +            virsh_pid=$!
> +            while true; do
> +                sleep .5

sleep .5 is a GNU-ism, but both coreutils and busybox support it, so I
think you're okay.

> +                kill -0 $virsh_pid >&/dev/null || break
> +                progress=`run_virsh_c $uri domjobinfo $guest 2>/dev/null | \
> +                          awk '/^Data processed:/{print $3, $4}'`
> +                if [ -n "$progress" ]; then
> +                    printf '\r%s%12s ' "$label" "$progress"
> +                else
> +                    printf '\r%s%-12s ' "$label" "..."
> +                fi

Don't you also need to print the ANSI sequence for clear-to-eol, so that
a shorter line overwriting a previous longer line doesn't leave garbage
from the longer line?

> +            done
> +            retval wait $virsh_pid && printf '\r%s%-12s\n' "$label" $"done"
> +        done
> +    done <$LISTFILE
> +}
> +
> +gueststatus() {
> +    for uri in $URIS; do
> +        echo "* $uri URI:"
> +        retval run_virsh $uri list || echo
> +    done
> +}
> +
> +# See how we were called.
> +case "$1" in
> +    start|stop|gueststatus)
> +        $1
> +        ;;
> +    *)
> +        echo $"Usage: $0 {start|stop|gueststatus}"
> +        exit 3
> +        ;;
> +esac

Missing restart, force-reload, and status actions, per the LSB.  Also,
status should be logged prior to exit, with log_*_msg.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]