[libvirt] [PATCH] libvirt-guests: Enable parallel operations and improve error handling

[Eric Blake]

On 02/20/2012 06:32 AM, Peter Krempa wrote:
> This patch modifies the libvirt-guests init script to enable parallel
> shiutdown on guest machines and gets rid of error messages if the client

s/shiutdown/shutdown/

> is unable connect to the URI specified in the config file.
> ---
> Simultaneous shutdown of machines may speed up the shutdown process as the shutdown sequence
> of guests often consists of timeouts and storage un-intensive tasks. Simultaneous resume
> of machines was already supported, although not documented well enough.
> 
> This patch also checks if connection to the URI can be done, and prints a error
> message if it's not the case. This get's rid of unrelevant and repeated error messages

s/unrelevant/irrelevant/

(Stupid English, for being inconsistent on whether un- or ir- negates a
word :)

> if the URI is unreachable.
> 
> The last improvement is while using managed-save. Transient domains are excluded
> from the save sequence to get rid of error messages and a list of domains that are left
> behind is printed.
> 
> Please tell me your suggestions how to improve this, as I'm not a bash "native speaker".

Actually, we want this to be stricter than bash, since people are using
libvirt-guests on debian-based systems where /bin/sh is dash.  But no
fears - I'm fluent in shell.

> 
> +test_connect()

I find it helpful to document ALL shell functions; it makes maintenance
easier down the road (it doesn't help that we haven't been doing this in
the rest of the file).  Here, it looks like you are doing:

# test_connect URI
# check if URI is reachable
test_connect() {

> +{
> +    uri=$1
> +
> +    run_virsh "$uri" connect  2>/dev/null

Any reason for the two spaces?

> +    if [ $? -ne 0 ]; then
> +        eval_gettext "Can't connect to \$uri. Skipping."
> +        echo
> +        return 1
> +    fi
> +}
> +
> +# "persistent" argument options:
> +# yes: list only persistent guests
> +# no: list only transient guests
> +# all: list both persistent and transient guests
>  list_guests() {

Here, I would use:
# list_guests URI PERSISTENT

prior to the documentation of valid PERSISTENT values.

>      uri=$1
> +    persistent=$2
> 
>      list=$(run_virsh_c "$uri" list)
>      if [ $? -ne 0 ]; then
> @@ -89,12 +106,19 @@ list_guests() {
> 
>      uuids=
>      for id in $(echo "$list" | awk 'NR > 2 {print $1}'); do
> -        uuid=$(run_virsh_c "$uri" dominfo "$id" | awk '/^UUID:/{print $2}')

<aside>

Remind me again why we are doing an inefficient shell loop?  In fact,
why were we using 'dominfo | awk' to convert id to uuid?  It's faster to
use 'run_virsh_c "$uri" virsh domuuid $id" to get the same information.

Taking it one step further, I'd rather see us enhance 'virsh list' to
give us what we want up front.  That is, I think it would be nice to
have 'virsh list { [--table] | --uuid | --name | --id } ...', where
--table is default (for back-compat), but listing --uuid, --name, or
--id drops the header line, and lists just one identifier per line
rather than a full table.  Also, I'd also like to see 'virsh list {
[--running] | --all | --inactive | --persistent | --transient } ...';
that is, we ought to have virsh list fine-tune which domains it lists,
by adding new options such as --persistent and --transient.

By improving virsh, we could skip out on the awk loop altogether, and
have the initial virsh list give us what we want in the first place.

</aside>

> -        if [ -z "$uuid" ]; then
> +        dominfo=$(run_virsh_c "$uri" dominfo "$id")
> +        uuid=$(echo "$dominfo" |  awk '/^UUID:/{print $2}')
> +        dompersist=$(echo "$dominfo" |  awk '/^Persistent:/{print $2}')

This sets $dompersist to yes or no,...

> +
> +        if [ -z "$uuid" ] || [ -z "$dompersist" ]; then
>              RETVAL=1
>              return 1
>          fi
> -        uuids="$uuids $uuid"
> +
> +        if [ "$persistent" == "$dompersist" ] ||

For portability to dash, you must use '=', not '==', inside [].

> +           [ "$persistent" == "all" ]; then
> +            uuids="$uuids $uuid"

...if persistent is 'all', then you wasted an awk call above for a
result you don't care about, since you plan on using the uuid anyway.
Again, more reason to have virsh list give us what we want in the first
place.

> 
> +shutdown_guest_async()
> +{
> +    uri=$1
> +    guest=$2
> +
> +    name=$(guest_name "$uri" "$guest")
> +    label=$(eval_gettext "Starting shutdown on guest: \$name")
> +    echo $label

Missing quotes around $label.  Probably simpler to just:

name=$(guest_name "$uri" "$guest")
eval_gettext "Starting shutdown on guest: \$name"
echo

> +    retval run_virsh "$uri" shutdown "$guest" >/dev/null || return

No need for the ||return - since this is the last statement in the
function, you will return anyway, with the same status.

> +}
> +
> +set_add()
> +{
> +    item=$1
> +    items=$2
> +
> +    echo "$items $item"

Should you be checking for duplicates before adding into a set?  I'm not
sure without reviewing further to see how this is used.  Also, this
particular usage requires that the user capture the output of the shell
function using $(), which wastes a subshell; there are tricks for
passing the name of a variable which holds a set, where you can then
modify the variable in place with fewer forks, but I don't think we're
at the point where optimizing a few forks will help matters.

> +}
> +
> +set_remove()
> +{
> +   item=$1
> +   items=$2
> +
> +   newitems=
> +   for nit in $items; do
> +       if [ "$nit" != "$domain" ]; then

Where did "$domain" come from?  Don't you instead mean to be comparing
against "$item"?

> +           newitems="$newitems $nit"

The resulting $newitems will always have a leading space, even if $items
on entry did not.  Is that intentional?

> +       fi
> +   done
> +
> +   echo "$newitems"
> +}
> +
> +set_count()
> +{
> +    items=$1
> +
> +    count="0"
> +    for item in $items; do
> +        count=$((count+1))

It's slightly more portable to do $(($count+1)).  But even that is slow;
why not let the shell do it for you:

set_count() {
    set -- $1
    echo $#
}

> +    done
> +
> +    echo $count
> +}
> +
> +set_head()
> +{
> +    items=$1
> +
> +    for item in $items; do
> +        echo $item
> +        return 0
> +    done
> +}

I sure hope that the sets you are using have no whitespace or glob
characters (that is, a set of domain ids or uuids is safe, a set of
domain names or of URIs is not).

> +
> +remove_shutdown_domains()
> +{
> +    uri=$1
> +    domains=$2
> +
> +    newlist=
> +    for dom in $domains; do
> +        guest_is_on "$uri" "$dom" 2>&1 > /dev/null || return

Do you really want to break out of the entire for loop on the first
guest where you encounter failure?  Also, this redirection looks fishy -
it says 'make the error output of guest_is_on go to my stdout, and
discard the normal output of guest_is_on'.  Did you mean '>/dev/null
2>&1' which says to discard both output and error messages from guest_is_on?

> +        if "$guest_running"; then
> +            newlist="$newlist $dom"
> +        fi
> +    done
> +
> +    echo "$newlist"
> +}

I ran out of time to finish my review today, but hope this gives you
some things to think about.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120220/85b5402c/attachment-0001.sig>