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

Re: [libvirt] [PATCH 4/4] libvirt-guests: Add parallel startup and shutdown of guests



On 02/28/2012 11:00 AM, Peter Krempa wrote:
> With this patch, it's possible to shut down guests in parallel. Parallel
> startup was possible before, but this functionality was not documented
> properly.
> 
> To enable parallel startup set the START_DELAY to 0.
> 
> Parallel shutdown has a configurable parameter PARALLEL_SHUTDOWN that
> defines the number of machines being shut down in parallel. Enabling
> this feature changes the semantics of SHUTDOWN_TIMEOUT parameter that is
> applied as a cumulative timeout to shutdown all guests on a URI.
> ---
>  tools/libvirt-guests.init.sh |  140 +++++++++++++++++++++++++++++++++++++++--
>  tools/libvirt-guests.sysconf |   10 +++-
>  2 files changed, 141 insertions(+), 9 deletions(-)
> 

Missing an initialization of PARALLEL_SHUTDOWN=0 alongside
SHUTDOWN_TIMEOUT=0 before sourcing the user's values.

> +
> +# check_domains_shutdown URI GUESTS
> +# check if shutdown is complete on guests in "GUESTS" and returns only
> +# guests that are still shutting down
> +check_domains_shutdown()
> +{
> +    uri=$1
> +    guests=$2
> +
> +    guests_up=
> +    for guest in $guests; do
> +        guest_is_on "$uri" "$dom" 2>&1 > /dev/null || continue

You want '>/dev/null 2>&1' (order matters, when silencing a command and
using it just for its side effects).  But are we sure we want to
silently ignore a guest_is_on failure, or should we emit a message
stating that we are no longer able to track the shutdown status of that
guest?

> +print_domains_shutdown()
> +{
> +    uri=$1
> +    before=$2
> +    after=$3
> +
> +    for guest in $before; do
> +        found=false
> +        for running in $after; do
> +           if [ $guest = $running ]; then
> +               found=true
> +               break
> +           fi
> +        done

slightly faster to do:

for guest in $before; do
    found=false;
    case " $after " in
        *" $guest "*) found=true ;;
    esac

But what you have is functionally correct.

> +
> +        if ! "$found"; then
> +            name=$(guest_name "$uri" "$guest")
> +            eval_gettext "Shutdown of guest \$name complete."
> +            echo
> +        fi
> +    done
> +}
> +
> +# shutdown_guests_parallel URI GUESTS
> +# Shutdown guests GUESTS on machine URI in parallel
> +shutdown_guests_parallel()
> +{
> +    uri=$1
> +    guests=$2
> +
> +    on_shutdown=
> +    timeout=$SHUTDOWN_TIMEOUT

If SHUTDOWN_TIMEOUT is the default of 0, then this function times out
right away; shouldn't that really mean that we have no maximum timeout,
and wait until everything has shut down?  I think you need another
variable, based on whether $timeout is 0 on entry, and skip the
$((timeout - 1)) calculation if that variable is true.

> +    while [ -n "$on_shutdown" ] || [ -n "$guests" ]; do
> +        while [ -n "$guests" ] &&
> +              [ $(guest_count "$on_shutdown") -lt "$PARALLEL_SHUTDOWN" ]; do
> +            guest=$(guest_first $guests)
> +            guests=$(guest_remove "$guest" "$guests")

Faster to avoid these two sub-shells by doing:

set -- $guests
guest=$1
shift
guests=$*

and might even get rid of the need for some helper functions.  But what
you have is functionally correct.

> @@ -23,7 +24,12 @@
>  #             value suitable for your guests.
>  #ON_SHUTDOWN=suspend
> 
> -# number of seconds we're willing to wait for a guest to shut down
> +# If set to non-zero, shutdown will suspend domains concurently. Number of domains

s/concurently/concurrently/

I think it's probably worth a v2 (or is it v3?) for just this patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
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]