[libvirt] Re-2: [Patch] Libvirt - Fix locking for readonly devices

On Tue, May 08, 2012 at 09:41:02AM +0000, David Weber wrote:
Hi,
I'm currently working on getting sanlock into Debian/Ubuntu. While testing, I noticed that I wasn't able to add a readonly or shared device: "internal error unsupported configuration: Readonly leases are not supported".
After looking into the source, it seems to be the following situation: - Libvirt passes every device to the sanlock plugin, even if it is readonly - The sanlock plugin rejects to add a lease for the readonly device, returning an error so the machine starts to fail
The attached patch rejects passing readonly and shared devices to the lock-plugin so they shouldn't be a problem anymore.
This isn't good - the lock manager implementation must be the one to decide what todo with readonly & shared disks. The sanlock plugin, however, does not currently support readonly/shared leases hence why it rejects them. We could probably add a config param to allow readonly/shared leases to be skipped by the sanlock plugin.
Thanks for clarification. I've attached an updated patch which adds such a config param. It works but I can't test live-migration at the moment. But as far as I understood it shouldn't be a problem. David ---------------------------------------------------------------- commit 5a1546f328b166871fbf0e0556b5a2aafba93d63 Author: David Weber <wb@munzinger.de> Date: Mon May 14 09:43:27 2012 +0200 Add ignore param for readonly and shared disk in sanlock diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index d344d6a..57b688a 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c struct _virLockManagerSanlockDriver { bool requireLeaseForDisks; int hostID; bool autoDiskLease; + bool ignoreReadonlyShared; char *autoDiskLeasePath; }; static int virLockManagerSanlockLoadConfig(const char *configFile) CHECK_TYPE("auto_disk_leases", VIR_CONF_LONG); if (p) driver->autoDiskLease = p->l; + p = virConfGetValue(conf, "ignore_readonly_and_shared_disks"); + CHECK_TYPE("ignore_readonly_and_shared_disks", VIR_CONF_LONG); + if (p) driver->ignoreReadonlyShared = p->l; + p = virConfGetValue(conf, "disk_lease_dir"); CHECK_TYPE("disk_lease_dir", VIR_CONF_STRING); if (p && p->str) { static int virLockManagerSanlockAddResource(virLockManagerPtr lock, SANLK_MAX_RESOURCES); return -1; } + + if (((flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) || + (flags &VIR_LOCK_MANAGER_RESOURCE_SHARED)) && + (driver->ignoreReadonlyShared)) { + return 0; + } if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) { virLockError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/locking/sanlock.conf b/src/locking/sanlock.conf index efc35ee..5429522 100644 --- a/src/locking/sanlock.conf +++ b/src/locking/sanlock.conf # to enabled, otherwise it defaults to disabled. # #require_lease_for_disks = 1 + +# +# Ignore readonly and shared disks as they aren't supportet yet +# +#ignore_readonly_and_shared_disks = 1 To: berrange@redhat.com Cc: libvir-list@redhat.com sanlock-devel@lists.fedorahosted.org

On Mon, May 14, 2012 at 08:32:01AM +0000, David Weber wrote:
This isn't good - the lock manager implementation must be the one to decide what todo with readonly & shared disks. The sanlock plugin, however, does not currently support readonly/shared leases hence why it rejects them. We could probably add a config param to allow readonly/shared leases to be skipped by the sanlock plugin.
Thanks for clarification. I've attached an updated patch which adds such a config param. It works but I can't test live-migration at the moment. But as far as I understood it shouldn't be a problem.
Thanks, your patch looks good but needs two further small additions to the libvirt_sanlock.aug and test_libvirt_sanlock.aug files to take account of the new config parameter. 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 :|
participants (2)
-
Daniel P. Berrange
-
David Weber