[libvirt] [PATCH] output status information during guest shutdown again

[Eric Blake]

On 08/03/2012 08:52 AM, Gerd v. Egidy wrote:
> With SysV-init libvirt-guests wrote status information to the console
> during shutdown of the guests. 
> 
> Since the move to systemd libvirt-guests doesn't output this progress 
> information anymore. This patch brings back this feature.
> 
> It is helpful to show the admin what the system is waiting for and what 
> is left of the timeout (e.g. for calibrating the shutdown timing of a ups).
> 
> Rewriting the current line with \r doesn't work anymore in the context
> of systemd. So always write new lines, but move to 5 second intervals
> to avoid flooding the console.
> ---
>  tools/libvirt-guests.init.sh    | 65 +++++++++++++++++++++++++++++++----------
>  tools/libvirt-guests.service.in |  1 +
>  2 files changed, 51 insertions(+), 15 deletions(-)
>  mode change 100644 => 100755 tools/libvirt-guests.init.sh

The mode change is spurious, and should not be part of this patch.

> +++ b/tools/libvirt-guests.init.sh
> @@ -225,22 +225,27 @@ suspend_guest()
>      name=$(guest_name "$uri" "$guest")
>      label=$(eval_gettext "Suspending \$name: ")
>      bypass=
> +    slept=0
>      test "x$BYPASS_CACHE" = x0 || bypass=--bypass-cache
> -    printf %s "$label"
> +    printf '%s%-12s\n' "$label" "..."

Why are you printing trailing whitespace?  You are left-justifying the
3-byte ..., which means you now always have 9 bytes of trailing space
Also, the "..." doesn't technically need quoting, although I guess it
doesn't hurt.  Would it be worth folding the ... into the format string
itself, as in:

printf '%s...\n' "$label"

If we were using \r, then I would understand the trailing space as a
means of blanking out longer text printed on a previous loop, but you
are abandoning \r.

>      run_virsh "$uri" managedsave $bypass "$guest" >/dev/null &
>      virsh_pid=$!
>      while true; do
>          sleep 1
>          kill -0 "$virsh_pid" >/dev/null 2>&1 || 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" "..."
> +
> +        slept=$(($slept + 1))
> +        if [ $(($slept%5)) -eq 0 ]; then

Consistency argues that we should have spaces on both sides of '%'

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

Because you now print only every five seconds, you have lost out on the
final printing when progress is non-empty if that happens on 4 of the 5
iterations.  I think you want the logic to look more like:

slept=$(($slept + 1))
progress=$(...)
if [ -n "$progress" ]; then
  printf  ... label progress
elif [ $(($slept % 5)) -eq 0 ]; then
  printf ... label...
fi

>      timeout=$SHUTDOWN_TIMEOUT
>      check_timeout=false
>      if [ $timeout -gt 0 ]; then
>          check_timeout=true
> +        format=$(eval_gettext "Waiting for guest %s to shut down, %4d seconds left\n")

Why the padding for the seconds left?  If timeout is 60, I think this
looks awkward:

Waiting for guest foo to shut down,    60 seconds left

> +    else
> +        slept=0
> +        format=$(eval_gettext "Waiting for guest %s to shut down\n")
>      fi
>      while ! $check_timeout || [ "$timeout" -gt 0 ]; do
>          sleep 1
>          guest_is_on "$uri" "$guest" || return
>          "$guest_running" || break
> +
>          if $check_timeout; then
> -            timeout=$((timeout - 1))
> -            printf '\r%s%-12d ' "$label" "$timeout"
> +            if [ $(($timeout%5)) -eq 0 ]; then

Spaces around %

> +                printf "$format" "$name" "$timeout"
> +            fi
> +            timeout=$(($timeout - 1))
> +        else
> +            slept=$(($slept + 1))
> +            if [ $(($slept%5)) -eq 0 ]; then

Again.

> +                printf "$format" "$name"

This is new output that was not present beforehand.  Is the intent that
when there is no timeout, you want to show that the process is still
alive waiting for the guest?

> +            fi
>          fi
>      done
>  
>      if guest_is_on "$uri" "$guest"; then
>          if "$guest_running"; then
> -            printf '\r%s%-12s\n' "$label" \
> -                "$(gettext "failed to shutdown in time")"
> +            eval_gettext "Shutdown of guest \$name failed to complete in time."
>          else
> -            printf '\r%s%-12s\n' "$label" "$(gettext "done")"
> +            eval_gettext "Shutdown of guest \$name complete."
>          fi
>      fi
>  }
> @@ -356,6 +372,10 @@ shutdown_guests_parallel()
>      timeout=$SHUTDOWN_TIMEOUT
>      if [ $timeout -gt 0 ]; then
>          check_timeout=true
> +        format=$(eval_gettext "Waiting for %d guests to shut down, %4d seconds left\n")

Another awkward %4d.

> +    else
> +        slept=0
> +        format=$(eval_gettext "Waiting for %d guests to shut down\n")
>      fi
>      while [ -n "$on_shutdown" ] || [ -n "$guests" ]; do
>          while [ -n "$guests" ] &&
> @@ -368,14 +388,29 @@ shutdown_guests_parallel()
>              on_shutdown="$on_shutdown $guest"
>          done
>          sleep 1
> +
> +        set -- $guests
> +        guestcount=$#
> +        set -- $on_shutdown
> +        shutdowncount=$#
> +
>          if $check_timeout; then
> +            if [ $(($timeout%5)) -eq 0 ]; then
> +                printf "$format" $(($guestcount+$shutdowncount)) "$timeout"

Spaces around % and +

> +            fi
>              timeout=$(($timeout - 1))
>              if [ $timeout -le 0 ]; then
>                  eval_gettext "Timeout expired while shutting down domains"; echo
>                  RETVAL=1
>                  return
>              fi
> +        else
> +            slept=$(($slept + 1))
> +            if [ $(($slept%5)) -eq 0 ]; then
> +                printf "$format" $(($guestcount+$shutdowncount))

And again.

> +            fi
>          fi
> +
>          on_shutdown_prev=$on_shutdown
>          on_shutdown=$(check_guests_shutdown "$uri" "$on_shutdown")
>          print_guests_shutdown "$uri" "$on_shutdown_prev" "$on_shutdown"
> diff --git a/tools/libvirt-guests.service.in b/tools/libvirt-guests.service.in
> index db28f3f..0f0c41c 100644
> --- a/tools/libvirt-guests.service.in
> +++ b/tools/libvirt-guests.service.in
> @@ -10,6 +10,7 @@ ExecStart=/etc/init.d/libvirt-guests start
>  ExecStop=/etc/init.d/libvirt-guests stop
>  Type=oneshot
>  RemainAfterExit=yes
> +StandardOutput=journal+console

Overall, it looks like this patch is headed in the right direction.  Did
you also check that on F16, where we still used sysvinit, that the
output there is still reasonable?

-- 
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/20120813/cbc353e1/attachment-0001.sig>