On Thu, Jun 23, 2011 at 08:36:28AM -0600, Eric Blake wrote:
> 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?
I don't think there's anything useful it can do.
> > +LOCKSPACE="__LIBVIRT__DISKS__"
> > +
> > +LOCKDIR=`augtool print /files@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@SYSCONFDIR@/libvirt/qemu-sanlock.conf/disk_lease_dir'`
OK
> > +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?
This directory doesn't contain disk images. It only contains
leases whose names are always an MD5 sum. So there are no
dot-files to worry about.
> > +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:
Yes, I've already fixed that.
>
> 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
> }
OK
>
> > + 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.
Cut+past error. It should always succeed. We actually expect to get
errors for any of the leases which are associated with running VMs,
so don't want to propagate that.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list