Re: [libvirt] [PATCH 3/3] libxl: provide integration with lock manager

Daniel P. Berrange wrote: > On Fri, Apr 17, 2015 at 03:36:22PM -0600, Jim Fehlig wrote: > >> Provide integration with libvirt's lock manager in the libxl driver. >> >> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; >> --- >> 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 6ea2889..503e8a4 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 77d46d0..70247f5 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, >> }; >> >> >> @@ -668,6 +693,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)); >> > > I see a few calls to ProcessPause, but I'm not seeing any to ProcessResume, > so I think you might have missed a few places needing hooks. > Currently I'm not using ProcessResume and instead calling ProcessStart with paused param set to false. ProcessStart is called in libxlDomainStart, which is called by all paths that start a domain, e.g. start, create, restore, migrate, etc. I was under the impression that ProcessResume is not needed if ProcessStart is called with paused=false.