[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]