[libvirt] [PATCH V2 0/2] Support libvirt's lock manager in the libxl driver
V2 of https://www.redhat.com/archives/libvir-list/2015-April/msg00845.html Patch 2 has been ACKed and pushed since it is independent of the lock manager integration. Patch 1 has been changed to address Daniel's comments. Patch 2 (was patch 3 in V1) is unchanged for the time being. Jim Fehlig (2): locking: relax PID requirement libxl: provide integration with lock manager src/Makefile.am | 12 +++++++++ src/libxl/libvirtd_libxl.aug | 2 ++ src/libxl/libxl.conf | 10 ++++++++ src/libxl/libxl_conf.c | 14 +++++++++++ src/libxl/libxl_conf.h | 6 +++++ src/libxl/libxl_domain.c | 47 ++++++++++++++++++++++++++++++++++-- src/libxl/libxl_domain.h | 1 + src/libxl/libxl_driver.c | 25 +++++++++++++++++++ src/libxl/libxl_migration.c | 6 +++++ src/libxl/test_libvirtd_libxl.aug.in | 1 + src/locking/lock_daemon.c | 2 +- src/locking/lock_daemon_dispatch.c | 16 ++++++------ src/locking/lock_driver_lockd.c | 7 ++---- 13 files changed, 133 insertions(+), 16 deletions(-) -- 1.8.4.5
Some hypervisors like Xen do not have PIDs associated with domains. Relax the requirement for PID != 0 in the locking code so it can be used by hypervisors that do not represent domains as a process running on the host. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- V2: Change check for ownerPid to ownerId in lock daemon dispatcher Change instance of VIR_WARN to VIR_DEBUG to prevent polluting the logs with warnings when creating Xen domains. src/locking/lock_daemon.c | 2 +- src/locking/lock_daemon_dispatch.c | 16 ++++++++-------- src/locking/lock_driver_lockd.c | 7 ++----- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index bb165c0..042ff94 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -678,7 +678,7 @@ virLockDaemonClientFree(void *opaque) signum = SIGKILL; else signum = 0; - if (virProcessKill(priv->clientPid, signum) < 0) { + if (priv->clientPid != 0 && virProcessKill(priv->clientPid, signum) < 0) { if (errno == ESRCH) break; diff --git a/src/locking/lock_daemon_dispatch.c b/src/locking/lock_daemon_dispatch.c index a7cee9d..bad646c 100644 --- a/src/locking/lock_daemon_dispatch.c +++ b/src/locking/lock_daemon_dispatch.c @@ -62,7 +62,7 @@ virLockSpaceProtocolDispatchAcquireResource(virNetServerPtr server ATTRIBUTE_UNU goto cleanup; } - if (!priv->ownerPid) { + if (!priv->ownerId) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; @@ -120,7 +120,7 @@ virLockSpaceProtocolDispatchCreateResource(virNetServerPtr server ATTRIBUTE_UNUS goto cleanup; } - if (!priv->ownerPid) { + if (!priv->ownerId) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; @@ -169,7 +169,7 @@ virLockSpaceProtocolDispatchDeleteResource(virNetServerPtr server ATTRIBUTE_UNUS goto cleanup; } - if (!priv->ownerPid) { + if (!priv->ownerId) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; @@ -218,7 +218,7 @@ virLockSpaceProtocolDispatchNew(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - if (!priv->ownerPid) { + if (!priv->ownerId) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; @@ -273,9 +273,9 @@ virLockSpaceProtocolDispatchRegister(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - if (priv->ownerPid) { + if (!args->owner.id) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("lock owner details have already been registered")); + _("lock owner details have not been registered")); goto cleanup; } @@ -320,7 +320,7 @@ virLockSpaceProtocolDispatchReleaseResource(virNetServerPtr server ATTRIBUTE_UNU goto cleanup; } - if (!priv->ownerPid) { + if (!priv->ownerId) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; @@ -370,7 +370,7 @@ virLockSpaceProtocolDispatchRestrict(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - if (!priv->ownerPid) { + if (!priv->ownerId) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("lock owner details have not been registered")); goto cleanup; diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 3a48a6a..c974d60 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -466,11 +466,8 @@ static int virLockManagerLockDaemonNew(virLockManagerPtr lock, _("Missing ID parameter for domain object")); return -1; } - if (priv->pid == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Missing PID parameter for domain object")); - return -1; - } + if (priv->pid == 0) + VIR_DEBUG("Missing PID parameter for domain object"); if (!priv->name) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing name parameter for domain object")); -- 1.8.4.5
On Tue, Apr 21, 2015 at 04:24:29PM -0600, Jim Fehlig wrote:
Some hypervisors like Xen do not have PIDs associated with domains. Relax the requirement for PID != 0 in the locking code so it can be used by hypervisors that do not represent domains as a process running on the host.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
V2: Change check for ownerPid to ownerId in lock daemon dispatcher
Change instance of VIR_WARN to VIR_DEBUG to prevent polluting the logs with warnings when creating Xen domains.
src/locking/lock_daemon.c | 2 +- src/locking/lock_daemon_dispatch.c | 16 ++++++++-------- src/locking/lock_driver_lockd.c | 7 ++----- 3 files changed, 11 insertions(+), 14 deletions(-)
ACK Regards, 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 :|
Daniel P. Berrange wrote:
On Tue, Apr 21, 2015 at 04:24:29PM -0600, Jim Fehlig wrote:
Some hypervisors like Xen do not have PIDs associated with domains. Relax the requirement for PID != 0 in the locking code so it can be used by hypervisors that do not represent domains as a process running on the host.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
V2: Change check for ownerPid to ownerId in lock daemon dispatcher
Change instance of VIR_WARN to VIR_DEBUG to prevent polluting the logs with warnings when creating Xen domains.
src/locking/lock_daemon.c | 2 +- src/locking/lock_daemon_dispatch.c | 16 ++++++++-------- src/locking/lock_driver_lockd.c | 7 ++----- 3 files changed, 11 insertions(+), 14 deletions(-)
ACK
Thanks. I've pushed this one. Regards, Jim
Provide integration with libvirt's lock manager in the libxl driver. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- ATM, no change from V1. src/Makefile.am | 12 +++++++++ src/libxl/libvirtd_libxl.aug | 2 ++ src/libxl/libxl.conf | 10 ++++++++ src/libxl/libxl_conf.c | 14 +++++++++++ src/libxl/libxl_conf.h | 6 +++++ src/libxl/libxl_domain.c | 47 ++++++++++++++++++++++++++++++++++-- src/libxl/libxl_domain.h | 1 + src/libxl/libxl_driver.c | 25 +++++++++++++++++++ src/libxl/libxl_migration.c | 6 +++++ src/libxl/test_libvirtd_libxl.aug.in | 1 + 10 files changed, 122 insertions(+), 2 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 9a5f16c..1438174 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -2246,6 +2246,12 @@ BUILT_SOURCES += locking/qemu-lockd.conf DISTCLEANFILES += locking/qemu-lockd.conf endif WITH_QEMU +if WITH_LIBXL +nodist_conf_DATA += locking/libxl-lockd.conf +BUILT_SOURCES += locking/libxl-lockd.conf +DISTCLEANFILES += locking/libxl-lockd.conf +endif WITH_LIBXL + locking/%-lockd.conf: $(srcdir)/locking/lockd.conf $(AM_V_GEN)$(MKDIR_P) locking ; \ cp $< $@ @@ -2431,6 +2437,12 @@ nodist_conf_DATA += locking/qemu-sanlock.conf BUILT_SOURCES += locking/qemu-sanlock.conf DISTCLEANFILES += locking/qemu-sanlock.conf endif WITH_QEMU + +if WITH_LIBXL +nodist_conf_DATA += locking/libxl-sanlock.conf +BUILT_SOURCES += locking/libxl-sanlock.conf +DISTCLEANFILES += locking/libxl-sanlock.conf +endif WITH_LIBXL else ! WITH_SANLOCK EXTRA_DIST += $(LOCK_DRIVER_SANLOCK_SOURCES) endif ! WITH_SANLOCK diff --git a/src/libxl/libvirtd_libxl.aug b/src/libxl/libvirtd_libxl.aug index f225954..d5aa150 100644 --- a/src/libxl/libvirtd_libxl.aug +++ b/src/libxl/libvirtd_libxl.aug @@ -25,9 +25,11 @@ module Libvirtd_libxl = (* Config entry grouped by function - same order as example config *) let autoballoon_entry = bool_entry "autoballoon" + let lock_entry = str_entry "lock_manager" (* Each entry in the config is one of the following ... *) let entry = autoballoon_entry + | lock_entry let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] diff --git a/src/libxl/libxl.conf b/src/libxl/libxl.conf index c104d40..ba3de7a 100644 --- a/src/libxl/libxl.conf +++ b/src/libxl/libxl.conf @@ -10,3 +10,13 @@ # autoballoon setting. # #autoballoon = 1 + + +# In order to prevent accidentally starting two domains that +# share one writable disk, libvirt offers two approaches for +# locking files: sanlock and virtlockd. sanlock is an external +# project which libvirt integrates with via the libvirt-lock-sanlock +# package. virtlockd is a libvirt implementation that is enabled with +# "lockd". Accepted values are "sanlock" and "lockd". +# +#lock_manager = "lockd" diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 1b504fa..29498d5 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -102,6 +102,7 @@ libxlDriverConfigDispose(void *obj) VIR_FREE(cfg->libDir); VIR_FREE(cfg->saveDir); VIR_FREE(cfg->autoDumpDir); + VIR_FREE(cfg->lockManagerName); } @@ -1495,6 +1496,7 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg, const char *filename) { virConfPtr conf = NULL; + virConfValuePtr p; int ret = -1; /* Check the file is readable before opening it, otherwise @@ -1512,6 +1514,18 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg, if (libxlGetAutoballoonConf(cfg, conf) < 0) goto cleanup; + if ((p = virConfGetValue(conf, "lock_manager"))) { + if (p->type != VIR_CONF_STRING) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", + _("Unexpected type for 'lock_manager' setting")); + goto cleanup; + } + + if (VIR_STRDUP(cfg->lockManagerName, p->str) < 0) + goto cleanup; + } + ret = 0; cleanup: diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 5ba1a71..0a1c0db 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -38,6 +38,7 @@ # include "virobject.h" # include "virchrdev.h" # include "virhostdev.h" +# include "locking/lock_manager.h" # define LIBXL_DRIVER_NAME "xenlight" # define LIBXL_VNC_PORT_MIN 5900 @@ -98,6 +99,8 @@ struct _libxlDriverConfig { * memory for new domains from domain0. */ bool autoballoon; + char *lockManagerName; + /* Once created, caps are immutable */ virCapsPtr caps; @@ -144,6 +147,9 @@ struct _libxlDriverPrivate { /* Immutable pointer, lockless APIs*/ virSysinfoDefPtr hostsysinfo; + + /* Immutable pointer. lockless access */ + virLockManagerPluginPtr lockManager; }; # define LIBXL_SAVE_MAGIC "libvirt-xml\n \0 \r" diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 3039427..656c0e0 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -34,6 +34,7 @@ #include "virlog.h" #include "virstring.h" #include "virtime.h" +#include "locking/domain_lock.h" #define VIR_FROM_THIS VIR_FROM_LIBXL @@ -217,12 +218,36 @@ libxlDomainObjPrivateFree(void *data) { libxlDomainObjPrivatePtr priv = data; + VIR_FREE(priv->lockState); virObjectUnref(priv); } +static int +libxlDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data) +{ + libxlDomainObjPrivatePtr priv = data; + + priv->lockState = virXPathString("string(./lockstate)", ctxt); + + return 0; +} + +static int +libxlDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) +{ + libxlDomainObjPrivatePtr priv = data; + + if (priv->lockState) + virBufferAsprintf(buf, "<lockstate>%s</lockstate>\n", priv->lockState); + + return 0; +} + virDomainXMLPrivateDataCallbacks libxlDomainXMLPrivateDataCallbacks = { .alloc = libxlDomainObjPrivateAlloc, .free = libxlDomainObjPrivateFree, + .parse = libxlDomainObjPrivateXMLParse, + .format = libxlDomainObjPrivateXMLFormat, }; @@ -667,6 +692,11 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, virHostdevReAttachDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, vm->def, VIR_HOSTDEV_SP_PCI, NULL); + VIR_FREE(priv->lockState); + if (virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState) < 0) + VIR_WARN("Unable to release lease on %s", vm->def->name); + VIR_DEBUG("Preserving lock state '%s'", NULLSTR(priv->lockState)); + vm->def->id = -1; if (priv->deathW) { @@ -960,6 +990,14 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, vm->def, VIR_HOSTDEV_SP_PCI) < 0) goto cleanup; + VIR_FREE(priv->lockState); + if (virDomainLockProcessStart(driver->lockManager, + "xen:///system", + vm, + false, + NULL) < 0) + goto cleanup; + /* Unlock virDomainObj while creating the domain */ virObjectUnlock(vm); @@ -990,7 +1028,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight failed to restore domain '%s'"), d_config.c_info.name); - goto cleanup; + goto release_dom; } /* @@ -1003,6 +1041,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW)) goto cleanup_dom; + if ((dom_xml = virDomainDefFormat(vm->def, 0)) == NULL) goto cleanup_dom; @@ -1024,7 +1063,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, } if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto cleanup_dom; + goto release_dom; if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); @@ -1040,6 +1079,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, goto cleanup; cleanup_dom: + ret = -1; if (priv->deathW) { libxl_evdisable_domain_death(cfg->ctx, priv->deathW); priv->deathW = NULL; @@ -1048,6 +1088,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, vm->def->id = -1; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); + release_dom: + virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState); + cleanup: libxl_domain_config_dispose(&d_config); VIR_FREE(dom_xml); diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index aa647b8..8c73cc4 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -64,6 +64,7 @@ struct _libxlDomainObjPrivate { virChrdevsPtr devs; libxl_evgen_domain_death *deathW; unsigned short migrationPort; + char *lockState; struct libxlDomainJobObj job; }; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 6a54c73..1ecfd71 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -57,6 +57,7 @@ #include "viratomic.h" #include "virhostdev.h" #include "network/bridge_driver.h" +#include "locking/domain_lock.h" #define VIR_FROM_THIS VIR_FROM_LIBXL @@ -411,6 +412,7 @@ libxlStateCleanup(void) virObjectUnref(libxl_driver->domains); virObjectUnref(libxl_driver->reservedVNCPorts); virObjectUnref(libxl_driver->migrationPorts); + virLockManagerPluginUnref(libxl_driver->lockManager); virObjectEventStateFree(libxl_driver->domainEventState); virSysinfoDefFree(libxl_driver->hostsysinfo); @@ -590,6 +592,14 @@ libxlStateInitialize(bool privileged, goto error; } + if (!(libxl_driver->lockManager = + virLockManagerPluginNew(cfg->lockManagerName ? + cfg->lockManagerName : "nop", + "libxl", + cfg->configBaseDir, + 0))) + goto error; + /* read the host sysinfo */ libxl_driver->hostsysinfo = virSysinfoRead(); @@ -2860,11 +2870,21 @@ libxlDomainAttachDeviceDiskLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev) if (libxlMakeDisk(l_disk, &x_disk) < 0) goto cleanup; + if (virDomainLockDiskAttach(libxl_driver->lockManager, + "xen:///system", + vm, l_disk) < 0) + goto cleanup; + if ((ret = libxl_device_disk_add(cfg->ctx, vm->def->id, &x_disk, NULL)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight failed to attach disk '%s'"), l_disk->dst); + if (virDomainLockDiskDetach(libxl_driver->lockManager, + vm, l_disk) < 0) { + VIR_WARN("Unable to release lock on %s", + virDomainDiskGetSource(l_disk)); + } goto cleanup; } @@ -3005,6 +3025,11 @@ libxlDomainDetachDeviceDiskLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev) goto cleanup; } + if (virDomainLockDiskDetach(libxl_driver->lockManager, + vm, l_disk) < 0) + VIR_WARN("Unable to release lock on %s", + virDomainDiskGetSource(l_disk)); + virDomainDiskRemove(vm->def, idx); virDomainDiskDefFree(l_disk); diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 51993c6..1efd98f 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -41,6 +41,7 @@ #include "libxl_driver.h" #include "libxl_conf.h" #include "libxl_migration.h" +#include "locking/domain_lock.h" #define VIR_FROM_THIS VIR_FROM_LIBXL @@ -469,6 +470,7 @@ libxlDomainMigrationPerform(libxlDriverPrivatePtr driver, const char *dname ATTRIBUTE_UNUSED, unsigned int flags) { + libxlDomainObjPrivatePtr priv = vm->privateData; char *hostname = NULL; unsigned short port = 0; char portstr[100]; @@ -503,6 +505,10 @@ libxlDomainMigrationPerform(libxlDriverPrivatePtr driver, sockfd = virNetSocketDupFD(sock, true); virObjectUnref(sock); + if (virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState) < 0) + VIR_WARN("Unable to release lease on %s", vm->def->name); + VIR_DEBUG("Preserving lock state '%s'", NULLSTR(priv->lockState)); + /* suspend vm and send saved data to dst through socket fd */ virObjectUnlock(vm); ret = libxlDoMigrateSend(driver, vm, flags, sockfd); diff --git a/src/libxl/test_libvirtd_libxl.aug.in b/src/libxl/test_libvirtd_libxl.aug.in index 23e667c..baa8c79 100644 --- a/src/libxl/test_libvirtd_libxl.aug.in +++ b/src/libxl/test_libvirtd_libxl.aug.in @@ -3,3 +3,4 @@ module Test_libvirtd_libxl = test Libvirtd_libxl.lns get conf = { "autoballoon" = "1" } +{ "lock_manager" = "lockd" } -- 1.8.4.5
participants (2)
-
Daniel P. Berrange -
Jim Fehlig