[libvirt] [PATCH 3/3] Support automatic creation of leases for disks in sanlock

[Eric Blake]

On 06/17/2011 06:38 AM, Daniel P. Berrange wrote:
> To make use of this capability the admin will need todo
> several tasks:
> 
>  - Mount an NFS volume (or other shared filesystem)
>    on /var/lib/libvirt/sanlock
>  - Configure 'host_id' in /etc/libvirt/qemu-sanlock.conf
>    with a unique value for each host with the same NFS
>    mount
>  - Toggle the 'auto_disk_leases' parameter in qemu-sanlock.conf

And where is this documented, besides the commit message?  While the
code has been ACK'd, we're still lacking the most important piece for
making this actually useful to sysadmins, since it is not an
out-of-the-box setup.

>      p = virConfGetValue(conf, "require_lease_for_disks");
>      CHECK_TYPE("require_lease_for_disks", VIR_CONF_LONG);
>      if (p)
>          driver->requireLeaseForDisks = p->l;
> +    else
> +        driver->requireLeaseForDisks = driver->autoDiskLease ? false : true;

s/driver->autoDiskLease ? false : true/!driver->autoDiskLease/

> +#
> +# The default location in which lockspaces are created when
> +# automatic lease creation is enabled. For each unique disk
> +# path, a file $LEASE_DIR/NNNNNNNNNNNNNN will be created
> +# where                  'NNNNNNNNNNNN' is the MD5 checkout of the disk path.

Intentional length mismatch?  We could probably get by with just the
shorter NNN, and users should realize that MD5 sums are longer than 3
hex digits.

>  #
>  # Flag to determine whether we allow starting of guests
>  # which do not have any <lease> elements defined in their
>  # configuration.
>  #
> +# If 'auto_disk_leases' is disabled, this setting defaults
> +# to enabled, otherwise it defaults to disabled.
> +#
>  #require_lease_for_disks = 1

Well, that addresses my comment on 2/3.

> +++ b/tools/virt-sanlock-cleanup.cron.in
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +
> + at SBINDIR@/virt-sanlock-cleanup -q
> +exit 0

Do we want to always ignore failure like this?  Or can cron reasonably
act on non-zero $? from virt-sanlock-cleanup?

> diff --git a/tools/virt-sanlock-cleanup.in b/tools/virt-sanlock-cleanup.in
> new file mode 100644
> index 0000000..483b595
> --- /dev/null
> +++ b/tools/virt-sanlock-cleanup.in
> @@ -0,0 +1,91 @@
> +#!/bin/sh
> +
> +# A script to cleanup resource leases auto-created by
> +# the libvirt lock plugin for sanlock
> +
> +verbose=1
> +if test "$1" = "-q" ; then
> +  verbose=0
> +fi
> +
> +LOCKSPACE="__LIBVIRT__DISKS__"
> +
> +LOCKDIR=`augtool print /files at SYSCONFDIR@/libvirt/qemu-sanlock.conf/disk_lease_dir`

Not that @SYSCONFDIR@ will ever have spaces, but this would be more
robust as:

LOCKDIR=`augtool print
'/files at SYSCONFDIR@/libvirt/qemu-sanlock.conf/disk_lease_dir'`

> +if test $? != 0 -o "x$DIR" = "x" ; then

s/DIR/LOCKDIR/

> +  LOCKDIR="@LOCALSTATEDIR@/lib/libvirt/sanlock"
> +fi
> +
> +function notify() {

"function" is a bash-ism, not guaranteed to be in /bin/sh.  Just use:

notify() {

(that is, s/function //)

> +  if test $verbose = 1 ; then
> +     echo "$@"

echo is unsafe on arbitrary text, if that text starts with - or includes
\.  Instead, you want:

printf %s\\n "$*"

> +  fi
> +}
> +
> +cd $LOCKDIR

Check for failure, and use quoting:

cd "$LOCKDIR" || ...

> +
> +for MD5 in *

This ignores dot-files.  Do we need to worry about people naming disk
images with a leading dot and thus wasting resources?

> +do
> +  if test $MD5 != '*' -a $MD5 != $LOCKSPACE ; then

'test ... -a ...' is not portable (didn't 'make syntax-check' call you
on this?  If not, it should have, since we have a syntax check for
that).  Missing quoting:

if test "$MD5" != '*' && test "$MD5" != $LOCKSPACE; then

> +    RESOURCE="$LOCKSPACE:$MD5:$LOCKDIR/$MD5:0"
> +    notify -n "Cleanup: $RESOURCE "

Oh, you want to conditionally suppress trailing newline.  Then earlier,
you need:

notify() {
  test $verbose = 1 || return
  if test "x$1" = "x-n"; then
    shift
    printf %s "$*"
  else
    printf %s\\n "$*"
  fi
}

> +    sanlock client command -r $RESOURCE -c /bin/rm -f "$LOCKDIR/$MD5" 2>/dev/null
> +    if test $? = 0 ; then
> +      notify "PASS"
> +    else
> +      notify "FAIL"
> +    fi
> +  fi
> +done
> +
> +exit 0

...

> +=head1 EXIT STATUS
> +
> +Upon successful cleanup, an exit status of 0 will be set. Upon
> +failure a non-zero status will be set.

Really?  Looks to me like the script blindly succeeds, rather than
passing on exit status.

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

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