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

29 Feb 2012


      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

    