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

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 > + > +@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?

> +LOCKSPACE="__LIBVIRT__DISKS__" > + > +LOCKDIR=`augtool print /files@SYSCONFDIR(a)/libvirt/qemu-sanlock.conf/disk_lease_dir` Not that @SYSCONFDIR@ will ever have spaces, but this would be more robust as: LOCKDIR=`augtool print &#39;/files@SYSCONFDIR(a)/libvirt/qemu-sanlock.conf/disk_lease_dir&#39;`

> +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.