[libvirt] [PATCH v2 0/6] Add support for lock failure action

When sanlock is used as a locking driver and sanlock deamon loses access to its lockspace, it automatically kills off the domains that had their locks stored there. Apparently some management apps would like to change this behavior by configuring what should happen when locks are lost. Patch 3/7 from v1 was dropped in v2. Other patches were updated according to review comments. More details may be found in each patch. Jiri Denemark (6): conf: Rename life cycle actions to event actions conf: Add on_lockfailure event configuration locking: Add const char * parameter to avoid ugly typecasts locking: Pass hypervisor driver name when acquiring locks locking: Add support for lock failure action locking: Implement lock failure action in sanlock driver docs/formatdomain.html.in | 37 ++++++++-- docs/internals/locking.html.in | 8 +++ docs/locking.html.in | 24 +++++++ docs/schemas/domaincommon.rng | 30 +++++++-- libvirt.spec.in | 1 + po/POTFILES.in | 1 + src/Makefile.am | 13 +++- src/conf/domain_conf.c | 85 ++++++++++++++--------- src/conf/domain_conf.h | 18 ++++- src/libvirt_private.syms | 2 + src/locking/domain_lock.c | 36 ++++++---- src/locking/domain_lock.h | 4 ++ src/locking/lock_driver.h | 8 ++- src/locking/lock_driver_nop.c | 1 + src/locking/lock_driver_sanlock.c | 114 +++++++++++++++++++++++++++---- src/locking/lock_manager.c | 10 ++- src/locking/lock_manager.h | 1 + src/locking/sanlock_helper.c | 138 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 9 +-- src/qemu/qemu_hotplug.c | 15 +++-- src/qemu/qemu_process.c | 4 +- 22 files changed, 479 insertions(+), 81 deletions(-) create mode 100644 src/locking/sanlock_helper.c -- 1.7.12

While current on_{poweroff,reboot,crash} action configuration is about configuring life cycle actions, they can all be considered events and actions that need to be done on a particular event. Let's generalize the code this way so that it can be reused later for non-lifecycle events. --- Notes: Version 2: - $SUBJECT =~ s/Generalize/Rename/ docs/formatdomain.html.in | 15 ++++++---- docs/schemas/domaincommon.rng | 8 +++--- src/conf/domain_conf.c | 66 +++++++++++++++++++++++-------------------- src/conf/domain_conf.h | 4 +-- 4 files changed, 51 insertions(+), 42 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3cc6c51..8821470 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -907,15 +907,11 @@ This guest NUMA specification is currently available only for QEMU/KVM. </p> - <h3><a name="elementsLifecycle">Lifecycle control</a></h3> + <h3><a name="elementsEvents">Events configuration</a></h3> <p> It is sometimes necessary to override the default actions taken - when a guest OS triggers a lifecycle operation. The following - collections of elements allow the actions to be specified. A - common use case is to force a reboot to be treated as a poweroff - when doing the initial OS installation. This allows the VM to be - re-configured for the first post-install bootup. + on various events. </p> <pre> @@ -925,6 +921,13 @@ <on_crash>restart</on_crash> ...</pre> + <p> + The following collections of elements allow the actions to be + specified when a guest OS triggers a lifecycle operation. A + common use case is to force a reboot to be treated as a poweroff + when doing the initial OS installation. This allows the VM to be + re-configured for the first post-install bootup. + </p> <dl> <dt><code>on_poweroff</code></dt> <dd>The content of this element specifies the action to take when diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f47fdad..ab8d4a1 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -51,7 +51,7 @@ <ref name="clock"/> <ref name="resources"/> <ref name="features"/> - <ref name="termination"/> + <ref name="events"/> <optional> <ref name="pm"/> </optional> @@ -2226,10 +2226,10 @@ </element> </define> <!-- - When a domain terminates multiple policies can be applied depending - on how it ended: + When a certain event happens, multiple policies can be applied + depends on what happened: --> - <define name="termination"> + <define name="events"> <interleave> <optional> <element name="on_reboot"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 33e1e7f..40abf39 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7518,11 +7518,12 @@ error: return NULL; } -static int virDomainLifecycleParseXML(xmlXPathContextPtr ctxt, - const char *xpath, - int *val, - int defaultVal, - virLifecycleFromStringFunc convFunc) +static int virDomainEventActionParseXML(xmlXPathContextPtr ctxt, + const char *name, + const char *xpath, + int *val, + int defaultVal, + virEventActionFromStringFunc convFunc) { char *tmp = virXPathString(xpath, ctxt); if (tmp == NULL) { @@ -7531,7 +7532,7 @@ static int virDomainLifecycleParseXML(xmlXPathContextPtr ctxt, *val = convFunc(tmp); if (*val < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown lifecycle action %s"), tmp); + _("unknown %s action: %s"), name, tmp); VIR_FREE(tmp); return -1; } @@ -8941,20 +8942,25 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, VIR_FREE(nodes); } - if (virDomainLifecycleParseXML(ctxt, "string(./on_reboot[1])", - &def->onReboot, VIR_DOMAIN_LIFECYCLE_RESTART, - virDomainLifecycleTypeFromString) < 0) + if (virDomainEventActionParseXML(ctxt, "on_reboot", + "string(./on_reboot[1])", + &def->onReboot, + VIR_DOMAIN_LIFECYCLE_RESTART, + virDomainLifecycleTypeFromString) < 0) goto error; - if (virDomainLifecycleParseXML(ctxt, "string(./on_poweroff[1])", - &def->onPoweroff, VIR_DOMAIN_LIFECYCLE_DESTROY, - virDomainLifecycleTypeFromString) < 0) + if (virDomainEventActionParseXML(ctxt, "on_poweroff", + "string(./on_poweroff[1])", + &def->onPoweroff, + VIR_DOMAIN_LIFECYCLE_DESTROY, + virDomainLifecycleTypeFromString) < 0) goto error; - if (virDomainLifecycleParseXML(ctxt, "string(./on_crash[1])", - &def->onCrash, - VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY, - virDomainLifecycleCrashTypeFromString) < 0) + if (virDomainEventActionParseXML(ctxt, "on_crash", + "string(./on_crash[1])", + &def->onCrash, + VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY, + virDomainLifecycleCrashTypeFromString) < 0) goto error; if (virDomainPMStateParseXML(ctxt, @@ -11452,15 +11458,15 @@ virDomainEmulatorPinDel(virDomainDefPtr def) } static int -virDomainLifecycleDefFormat(virBufferPtr buf, - int type, - const char *name, - virLifecycleToStringFunc convFunc) +virDomainEventActionDefFormat(virBufferPtr buf, + int type, + const char *name, + virEventActionToStringFunc convFunc) { const char *typeStr = convFunc(type); if (!typeStr) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected lifecycle type %d"), type); + _("unexpected %s action: %d"), name, type); return -1; } @@ -13674,17 +13680,17 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, " </clock>\n"); } - if (virDomainLifecycleDefFormat(buf, def->onPoweroff, - "on_poweroff", - virDomainLifecycleTypeToString) < 0) + if (virDomainEventActionDefFormat(buf, def->onPoweroff, + "on_poweroff", + virDomainLifecycleTypeToString) < 0) goto cleanup; - if (virDomainLifecycleDefFormat(buf, def->onReboot, - "on_reboot", - virDomainLifecycleTypeToString) < 0) + if (virDomainEventActionDefFormat(buf, def->onReboot, + "on_reboot", + virDomainLifecycleTypeToString) < 0) goto cleanup; - if (virDomainLifecycleDefFormat(buf, def->onCrash, - "on_crash", - virDomainLifecycleCrashTypeToString) < 0) + if (virDomainEventActionDefFormat(buf, def->onCrash, + "on_crash", + virDomainLifecycleCrashTypeToString) < 0) goto cleanup; if (def->pm.s3 || def->pm.s4) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 14dead3..0ae6e48 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2144,8 +2144,8 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model); virSecurityLabelDefPtr virDomainDefAddSecurityLabelDef(virDomainDefPtr def, const char *model); -typedef const char* (*virLifecycleToStringFunc)(int type); -typedef int (*virLifecycleFromStringFunc)(const char *type); +typedef const char* (*virEventActionToStringFunc)(int type); +typedef int (*virEventActionFromStringFunc)(const char *type); VIR_ENUM_DECL(virDomainTaint) -- 1.7.12

On Wed, Oct 10, 2012 at 01:35:28PM +0200, Jiri Denemark wrote:
While current on_{poweroff,reboot,crash} action configuration is about configuring life cycle actions, they can all be considered events and actions that need to be done on a particular event. Let's generalize the code this way so that it can be reused later for non-lifecycle events. --- Notes: Version 2: - $SUBJECT =~ s/Generalize/Rename/
You can do likewise in the commit message. ACK 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 :|

Using this new element, one can configure an action that should be performed when resource locks are lost. --- Notes: Version 2: - no changes docs/formatdomain.html.in | 22 ++++++++++++++++++++++ docs/schemas/domaincommon.rng | 22 ++++++++++++++++++++++ src/conf/domain_conf.c | 19 +++++++++++++++++++ src/conf/domain_conf.h | 14 ++++++++++++++ src/libvirt_private.syms | 2 ++ 5 files changed, 79 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8821470..263a2b9 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -919,6 +919,7 @@ <on_poweroff>destroy</on_poweroff> <on_reboot>restart</on_reboot> <on_crash>restart</on_crash> + <on_lockfailure>poweroff</on_lockfailure> ...</pre> <p> @@ -974,6 +975,27 @@ domain will be restarted with the same configuration</dd> </dl> + <p> + The <code>on_lockfailure</code> element (<span class="since">since + 0.10.3</span>) may be used to configure what action should be + taken when a lock manager loses resource locks. The following + actions are recognized by libvirt, although not all of them need + to be supported by individual lock managers. When no action is + specified, each lock manager will take its default action. + </p> + <dl> + <dt><code>poweroff</code></dt> + <dd>The domain will be forcefully powered off.</dd> + <dt><code>restart</code></dt> + <dd>The domain will be powered off and started up again to + reacquire its locks.</dd> + <dt><code>pause</code></dt> + <dd>The domain will be paused so that it can be manually resumed + when lock issues are solved.</dd> + <dt><code>ignore</code></dt> + <dd>Keep the domain running as if nothing happened.</dd> + </dl> + <h3><a name="elementsPowerManagement">Power Management</a></h3> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index ab8d4a1..2df2efa 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2246,6 +2246,11 @@ <ref name="crashOptions"/> </element> </optional> + <optional> + <element name="on_lockfailure"> + <ref name="lockfailureOptions"/> + </element> + </optional> </interleave> </define> <!-- @@ -2288,6 +2293,23 @@ </choice> </define> <!-- + Options when resource locks are lost: + poweroff: power off the domain + restart: power off the domain and start it up again to reacquire the + locks + pause: pause the execution of the domain so that it can be manually + resumed when lock issues are solved + ignore: keep the domain running + --> + <define name="lockfailureOptions"> + <choice> + <value>poweroff</value> + <value>restart</value> + <value>pause</value> + <value>ignore</value> + </choice> + </define> + <!-- Control ACPI sleep states (dis)allowed for the domain For each of the states the following rules apply: on: the state will be forcefully enabled diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 40abf39..120d82f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -134,6 +134,13 @@ VIR_ENUM_IMPL(virDomainLifecycleCrash, VIR_DOMAIN_LIFECYCLE_CRASH_LAST, "coredump-destroy", "coredump-restart") +VIR_ENUM_IMPL(virDomainLockFailure, VIR_DOMAIN_LOCK_FAILURE_LAST, + "default", + "poweroff", + "restart", + "pause", + "ignore") + VIR_ENUM_IMPL(virDomainPMState, VIR_DOMAIN_PM_STATE_LAST, "default", "yes", @@ -8963,6 +8970,13 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, virDomainLifecycleCrashTypeFromString) < 0) goto error; + if (virDomainEventActionParseXML(ctxt, "on_lockfailure", + "string(./on_lockfailure[1])", + &def->onLockFailure, + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, + virDomainLockFailureTypeFromString) < 0) + goto error; + if (virDomainPMStateParseXML(ctxt, "string(./pm/suspend-to-mem/@enabled)", &def->pm.s3) < 0) @@ -13692,6 +13706,11 @@ virDomainDefFormatInternal(virDomainDefPtr def, "on_crash", virDomainLifecycleCrashTypeToString) < 0) goto cleanup; + if (def->onLockFailure != VIR_DOMAIN_LOCK_FAILURE_DEFAULT && + virDomainEventActionDefFormat(buf, def->onLockFailure, + "on_lockfailure", + virDomainLockFailureTypeToString) < 0) + goto cleanup; if (def->pm.s3 || def->pm.s4) { virBufferAddLit(buf, " <pm>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0ae6e48..c41e0cf 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1413,6 +1413,18 @@ enum virDomainLifecycleCrashAction { VIR_DOMAIN_LIFECYCLE_CRASH_LAST }; +typedef enum { + VIR_DOMAIN_LOCK_FAILURE_DEFAULT, + VIR_DOMAIN_LOCK_FAILURE_POWEROFF, + VIR_DOMAIN_LOCK_FAILURE_RESTART, + VIR_DOMAIN_LOCK_FAILURE_PAUSE, + VIR_DOMAIN_LOCK_FAILURE_IGNORE, + + VIR_DOMAIN_LOCK_FAILURE_LAST +} virDomainLockFailureAction; + +VIR_ENUM_DECL(virDomainLockFailure) + enum virDomainPMState { VIR_DOMAIN_PM_STATE_DEFAULT = 0, VIR_DOMAIN_PM_STATE_ENABLED, @@ -1681,6 +1693,8 @@ struct _virDomainDef { int onPoweroff; int onCrash; + int onLockFailure; /* enum virDomainLockFailureAction */ + struct { /* These options are actually type of enum virDomainPMState */ int s3; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a8c81e7..fe31bbe 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -421,6 +421,8 @@ virDomainLifecycleTypeFromString; virDomainLifecycleTypeToString; virDomainLiveConfigHelperMethod; virDomainLoadAllConfigs; +virDomainLockFailureTypeFromString; +virDomainLockFailureTypeToString; virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; virDomainMemDumpTypeFromString; -- 1.7.12

On Wed, Oct 10, 2012 at 01:35:29PM +0200, Jiri Denemark wrote:
Using this new element, one can configure an action that should be performed when resource locks are lost. --- Notes: Version 2: - no changes
docs/formatdomain.html.in | 22 ++++++++++++++++++++++ docs/schemas/domaincommon.rng | 22 ++++++++++++++++++++++ src/conf/domain_conf.c | 19 +++++++++++++++++++ src/conf/domain_conf.h | 14 ++++++++++++++ src/libvirt_private.syms | 2 ++ 5 files changed, 79 insertions(+)
ACK 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 :|

--- Notes: Version 2: - no changes src/locking/lock_driver.h | 2 ++ src/locking/lock_manager.c | 3 +++ 2 files changed, 5 insertions(+) diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 83ef323..c33aea7 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -66,6 +66,7 @@ typedef enum { enum { VIR_LOCK_MANAGER_PARAM_TYPE_STRING, + VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING, VIR_LOCK_MANAGER_PARAM_TYPE_INT, VIR_LOCK_MANAGER_PARAM_TYPE_LONG, VIR_LOCK_MANAGER_PARAM_TYPE_UINT, @@ -84,6 +85,7 @@ struct _virLockManagerParam { unsigned long long ul; double d; char *str; + const char *cstr; unsigned char uuid[16]; } value; }; diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index f5e967f..23126c9 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -99,6 +99,9 @@ static void virLockManagerLogParams(size_t nparams, case VIR_LOCK_MANAGER_PARAM_TYPE_STRING: VIR_DEBUG(" key=%s type=string value=%s", params[i].key, params[i].value.str); break; + case VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING: + VIR_DEBUG(" key=%s type=cstring value=%s", params[i].key, params[i].value.cstr); + break; case VIR_LOCK_MANAGER_PARAM_TYPE_UUID: virUUIDFormat(params[i].value.uuid, uuidstr); VIR_DEBUG(" key=%s type=uuid value=%s", params[i].key, uuidstr); -- 1.7.12

On Wed, Oct 10, 2012 at 01:35:30PM +0200, Jiri Denemark wrote:
--- Notes: Version 2: - no changes
src/locking/lock_driver.h | 2 ++ src/locking/lock_manager.c | 3 +++ 2 files changed, 5 insertions(+)
ACK 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 :|

This is required in case a lock manager needs to contact libvirtd in case of an unexpected event. --- Notes: Version 2: - update after dropping ``qemu: Use macro instead of "qemu" in the context of URI scheme'' - pass driver URI rather than its name - rename driver parameter to uri docs/internals/locking.html.in | 8 ++++++++ src/locking/domain_lock.c | 25 +++++++++++++++++-------- src/locking/domain_lock.h | 4 ++++ src/locking/lock_driver.h | 1 + src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 9 +++++---- src/qemu/qemu_hotplug.c | 15 ++++++++++----- src/qemu/qemu_process.c | 4 +++- 8 files changed, 49 insertions(+), 18 deletions(-) diff --git a/docs/internals/locking.html.in b/docs/internals/locking.html.in index e84321b..34e50c7 100644 --- a/docs/internals/locking.html.in +++ b/docs/internals/locking.html.in @@ -183,6 +183,10 @@ .key = "pid", .value = { .i = dom->pid }, }, + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING, + .key = "uri", + .value = { .cstr = driver->uri }, + }, }; mgr = virLockManagerNew(lockPlugin, VIR_LOCK_MANAGER_TYPE_DOMAIN, @@ -225,6 +229,10 @@ .key = "pid", .value = { .i = dom->pid }, }, + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING, + .key = "uri", + .value = { .cstr = driver->uri }, + }, }; mgr = virLockManagerNew(lockPlugin, VIR_LOCK_MANAGER_TYPE_DOMAIN, diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index 3417361..9d6fd6a 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -97,6 +97,7 @@ static int virDomainLockManagerAddDisk(virLockManagerPtr lock, } static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, + const char *uri, virDomainObjPtr dom, bool withResources) { @@ -118,6 +119,10 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, .key = "pid", .value = { .i = dom->pid }, }, + { .type = VIR_LOCK_MANAGER_PARAM_TYPE_CSTRING, + .key = "uri", + .value = { .cstr = uri }, + }, }; VIR_DEBUG("plugin=%p dom=%p withResources=%d", plugin, dom, withResources); @@ -152,6 +157,7 @@ error: int virDomainLockProcessStart(virLockManagerPluginPtr plugin, + const char *uri, virDomainObjPtr dom, bool paused, int *fd) @@ -163,7 +169,7 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p paused=%d fd=%p", plugin, dom, paused, fd); - if (!(lock = virDomainLockManagerNew(plugin, dom, true))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true))) return -1; if (paused) @@ -186,7 +192,7 @@ int virDomainLockProcessPause(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%p", plugin, dom, state); - if (!(lock = virDomainLockManagerNew(plugin, dom, true))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true))) return -1; ret = virLockManagerRelease(lock, state, 0); @@ -196,6 +202,7 @@ int virDomainLockProcessPause(virLockManagerPluginPtr plugin, } int virDomainLockProcessResume(virLockManagerPluginPtr plugin, + const char *uri, virDomainObjPtr dom, const char *state) { @@ -205,7 +212,7 @@ int virDomainLockProcessResume(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%s", plugin, dom, NULLSTR(state)); - if (!(lock = virDomainLockManagerNew(plugin, dom, true))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true))) return -1; ret = virLockManagerAcquire(lock, state, 0, NULL); @@ -224,7 +231,7 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p state=%p", plugin, dom, state); - if (!(lock = virDomainLockManagerNew(plugin, dom, true))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, true))) return -1; ret = virLockManagerInquire(lock, state, 0); @@ -235,6 +242,7 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, + const char *uri, virDomainObjPtr dom, virDomainDiskDefPtr disk) { @@ -244,7 +252,7 @@ int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p disk=%p", plugin, dom, disk); - if (!(lock = virDomainLockManagerNew(plugin, dom, false))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false))) return -1; if (virDomainLockManagerAddDisk(lock, disk) < 0) @@ -271,7 +279,7 @@ int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p disk=%p", plugin, dom, disk); - if (!(lock = virDomainLockManagerNew(plugin, dom, false))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false))) return -1; if (virDomainLockManagerAddDisk(lock, disk) < 0) @@ -290,6 +298,7 @@ cleanup: int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, + const char *uri, virDomainObjPtr dom, virDomainLeaseDefPtr lease) { @@ -299,7 +308,7 @@ int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p lease=%p", plugin, dom, lease); - if (!(lock = virDomainLockManagerNew(plugin, dom, false))) + if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false))) return -1; if (virDomainLockManagerAddLease(lock, lease) < 0) @@ -326,7 +335,7 @@ int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, VIR_DEBUG("plugin=%p dom=%p lease=%p", plugin, dom, lease); - if (!(lock = virDomainLockManagerNew(plugin, dom, false))) + if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false))) return -1; if (virDomainLockManagerAddLease(lock, lease) < 0) diff --git a/src/locking/domain_lock.h b/src/locking/domain_lock.h index 3bedeb1..eefe6cd 100644 --- a/src/locking/domain_lock.h +++ b/src/locking/domain_lock.h @@ -27,6 +27,7 @@ # include "lock_manager.h" int virDomainLockProcessStart(virLockManagerPluginPtr plugin, + const char *uri, virDomainObjPtr dom, bool paused, int *fd); @@ -34,6 +35,7 @@ int virDomainLockProcessPause(virLockManagerPluginPtr plugin, virDomainObjPtr dom, char **state); int virDomainLockProcessResume(virLockManagerPluginPtr plugin, + const char *uri, virDomainObjPtr dom, const char *state); int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, @@ -41,6 +43,7 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, char **state); int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, + const char *uri, virDomainObjPtr dom, virDomainDiskDefPtr disk); int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, @@ -48,6 +51,7 @@ int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, virDomainDiskDefPtr disk); int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, + const char *uri, virDomainObjPtr dom, virDomainLeaseDefPtr lease); int virDomainLockLeaseDetach(virLockManagerPluginPtr plugin, diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index c33aea7..8fe7ceb 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -155,6 +155,7 @@ typedef int (*virLockDriverDeinit)(void); * - uuid: the domain uuid (uuid) * - name: the domain name (string) * - pid: process ID to own/owning the lock (unsigned int) + * - uri: URI for connecting to the driver the domain belongs to (string) * * Returns 0 if successful initialized a new context, -1 on error */ diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index ca2f694..2c7f70c 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -57,6 +57,7 @@ struct qemud_driver { virThreadPoolPtr workerPool; int privileged; + const char *uri; uid_t user; gid_t group; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 97ad23e..9aa0296 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -611,7 +611,9 @@ qemudStartup(int privileged) { return -1; } qemuDriverLock(qemu_driver); + qemu_driver->privileged = privileged; + qemu_driver->uri = privileged ? "qemu:///system" : "qemu:///session"; /* Don't have a dom0 so start from 1 */ qemu_driver->nextvmid = 1; @@ -860,9 +862,7 @@ qemudStartup(int privileged) { virHashForEach(qemu_driver->domains.objs, qemuDomainNetsRestart, NULL); - conn = virConnectOpen(qemu_driver->privileged ? - "qemu:///system" : - "qemu:///session"); + conn = virConnectOpen(qemu_driver->uri); qemuProcessReconnectAll(conn, qemu_driver); @@ -10695,7 +10695,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, origdriver = disk->driverType; disk->driverType = (char *) "raw"; /* Don't want to probe backing files */ - if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + if (virDomainLockDiskAttach(driver->lockManager, driver->uri, + vm, disk) < 0) goto cleanup; if (cgroup && qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) { if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a738b19..d6d161f 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -88,7 +88,8 @@ int qemuDomainChangeEjectableMedia(struct qemud_driver *driver, return -1; } - if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + if (virDomainLockDiskAttach(driver->lockManager, driver->uri, + vm, disk) < 0) return -1; if (virSecurityManagerSetImageLabel(driver->securityManager, @@ -217,7 +218,8 @@ int qemuDomainAttachPciDiskDevice(virConnectPtr conn, } } - if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + if (virDomainLockDiskAttach(driver->lockManager, driver->uri, + vm, disk) < 0) return -1; if (virSecurityManagerSetImageLabel(driver->securityManager, @@ -449,7 +451,8 @@ int qemuDomainAttachSCSIDisk(virConnectPtr conn, } } - if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + if (virDomainLockDiskAttach(driver->lockManager, driver->uri, + vm, disk) < 0) return -1; if (virSecurityManagerSetImageLabel(driver->securityManager, @@ -572,7 +575,8 @@ int qemuDomainAttachUsbMassstorageDevice(virConnectPtr conn, } } - if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + if (virDomainLockDiskAttach(driver->lockManager, driver->uri, + vm, disk) < 0) return -1; if (virSecurityManagerSetImageLabel(driver->securityManager, @@ -2389,7 +2393,8 @@ int qemuDomainAttachLease(struct qemud_driver *driver, if (virDomainLeaseInsertPreAlloc(vm->def) < 0) return -1; - if (virDomainLockLeaseAttach(driver->lockManager, vm, lease) < 0) { + if (virDomainLockLeaseAttach(driver->lockManager, driver->uri, + vm, lease) < 0) { virDomainLeaseInsertPreAlloced(vm->def, NULL); return -1; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 31909b7..8c4bd9e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2597,6 +2597,7 @@ static int qemuProcessHook(void *data) if (virSecurityManagerSetSocketLabel(h->driver->securityManager, h->vm->def) < 0) goto cleanup; if (virDomainLockProcessStart(h->driver->lockManager, + h->driver->uri, h->vm, /* QEMU is always pased initially */ true, @@ -2666,7 +2667,8 @@ qemuProcessStartCPUs(struct qemud_driver *driver, virDomainObjPtr vm, qemuDomainObjPrivatePtr priv = vm->privateData; VIR_DEBUG("Using lock state '%s'", NULLSTR(priv->lockState)); - if (virDomainLockProcessResume(driver->lockManager, vm, priv->lockState) < 0) { + if (virDomainLockProcessResume(driver->lockManager, driver->uri, + vm, priv->lockState) < 0) { /* Don't free priv->lockState on error, because we need * to make sure we have state still present if the user * tries to resume again -- 1.7.12

On Wed, Oct 10, 2012 at 01:35:31PM +0200, Jiri Denemark wrote:
This is required in case a lock manager needs to contact libvirtd in case of an unexpected event. --- Notes: Version 2: - update after dropping ``qemu: Use macro instead of "qemu" in the context of URI scheme'' - pass driver URI rather than its name - rename driver parameter to uri
docs/internals/locking.html.in | 8 ++++++++ src/locking/domain_lock.c | 25 +++++++++++++++++-------- src/locking/domain_lock.h | 4 ++++ src/locking/lock_driver.h | 1 + src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 9 +++++---- src/qemu/qemu_hotplug.c | 15 ++++++++++----- src/qemu/qemu_process.c | 4 +++- 8 files changed, 49 insertions(+), 18 deletions(-)
ACK 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 :|

--- Notes: Version 2: - no changes src/Makefile.am | 2 +- src/locking/domain_lock.c | 11 +++++++---- src/locking/lock_driver.h | 5 ++++- src/locking/lock_driver_nop.c | 1 + src/locking/lock_driver_sanlock.c | 1 + src/locking/lock_manager.c | 7 ++++--- src/locking/lock_manager.h | 1 + 7 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 34bc75c..0aefc02 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1502,7 +1502,7 @@ lockdriverdir = $(libdir)/libvirt/lock-driver lockdriver_LTLIBRARIES = sanlock.la sanlock_la_SOURCES = $(LOCK_DRIVER_SANLOCK_SOURCES) -sanlock_la_CFLAGS = $(AM_CFLAGS) +sanlock_la_CFLAGS = -I$(top_srcdir)/src/conf $(AM_CFLAGS) sanlock_la_LDFLAGS = -module -avoid-version sanlock_la_LIBADD = -lsanlock_client \ ../gnulib/lib/libgnu.la diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index 9d6fd6a..1e3da5f 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -175,7 +175,8 @@ int virDomainLockProcessStart(virLockManagerPluginPtr plugin, if (paused) flags |= VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY; - ret = virLockManagerAcquire(lock, NULL, flags, fd); + ret = virLockManagerAcquire(lock, NULL, flags, + dom->def->onLockFailure, fd); virLockManagerFree(lock); @@ -215,7 +216,7 @@ int virDomainLockProcessResume(virLockManagerPluginPtr plugin, if (!(lock = virDomainLockManagerNew(plugin, uri, dom, true))) return -1; - ret = virLockManagerAcquire(lock, state, 0, NULL); + ret = virLockManagerAcquire(lock, state, 0, dom->def->onLockFailure, NULL); virLockManagerFree(lock); return ret; @@ -258,7 +259,8 @@ int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, if (virDomainLockManagerAddDisk(lock, disk) < 0) goto cleanup; - if (virLockManagerAcquire(lock, NULL, 0, NULL) < 0) + if (virLockManagerAcquire(lock, NULL, 0, + dom->def->onLockFailure, NULL) < 0) goto cleanup; ret = 0; @@ -314,7 +316,8 @@ int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, if (virDomainLockManagerAddLease(lock, lease) < 0) goto cleanup; - if (virLockManagerAcquire(lock, NULL, 0, NULL) < 0) + if (virLockManagerAcquire(lock, NULL, 0, + dom->def->onLockFailure, NULL) < 0) goto cleanup; ret = 0; diff --git a/src/locking/lock_driver.h b/src/locking/lock_driver.h index 8fe7ceb..e8ee226 100644 --- a/src/locking/lock_driver.h +++ b/src/locking/lock_driver.h @@ -23,6 +23,7 @@ # define __VIR_PLUGINS_LOCK_DRIVER_H__ # include "internal.h" +# include "domain_conf.h" typedef struct _virLockManager virLockManager; typedef virLockManager *virLockManagerPtr; @@ -218,12 +219,13 @@ typedef int (*virLockDriverAddResource)(virLockManagerPtr man, * @manager: the lock manager context * @state: the current lock state * @flags: optional flags, currently unused + * @action: action to take when lock is lost * @fd: optional return the leaked FD * * Start managing resources for the object. This * must be called from the PID that represents the * object to be managed. If the lock is lost at any - * time, the PID will be killed off by the lock manager. + * time, the specified action will be taken. * The optional state contains information about the * locks previously held for the object. * @@ -237,6 +239,7 @@ typedef int (*virLockDriverAddResource)(virLockManagerPtr man, typedef int (*virLockDriverAcquire)(virLockManagerPtr man, const char *state, unsigned int flags, + virDomainLockFailureAction action, int *fd); /** diff --git a/src/locking/lock_driver_nop.c b/src/locking/lock_driver_nop.c index 7f6cdca..c9ab806 100644 --- a/src/locking/lock_driver_nop.c +++ b/src/locking/lock_driver_nop.c @@ -69,6 +69,7 @@ static int virLockManagerNopAddResource(virLockManagerPtr lock ATTRIBUTE_UNUSED, static int virLockManagerNopAcquire(virLockManagerPtr lock ATTRIBUTE_UNUSED, const char *state ATTRIBUTE_UNUSED, unsigned int flags_unused ATTRIBUTE_UNUSED, + virDomainLockFailureAction action ATTRIBUTE_UNUSED, int *fd ATTRIBUTE_UNUSED) { return 0; diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 113fd47..8c0ac8c 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -686,6 +686,7 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, static int virLockManagerSanlockAcquire(virLockManagerPtr lock, const char *state, unsigned int flags, + virDomainLockFailureAction action ATTRIBUTE_UNUSED, int *fd) { virLockManagerSanlockPrivatePtr priv = lock->privateData; diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index 23126c9..423997b 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -347,17 +347,18 @@ int virLockManagerAddResource(virLockManagerPtr lock, int virLockManagerAcquire(virLockManagerPtr lock, const char *state, unsigned int flags, + virDomainLockFailureAction action, int *fd) { - VIR_DEBUG("lock=%p state='%s' flags=%x fd=%p", - lock, NULLSTR(state), flags, fd); + VIR_DEBUG("lock=%p state='%s' flags=%x action=%d fd=%p", + lock, NULLSTR(state), flags, action, fd); CHECK_MANAGER(drvAcquire, -1); if (fd) *fd = -1; - return lock->driver->drvAcquire(lock, state, flags, fd); + return lock->driver->drvAcquire(lock, state, flags, action, fd); } diff --git a/src/locking/lock_manager.h b/src/locking/lock_manager.h index d955ee0..4fee12d 100644 --- a/src/locking/lock_manager.h +++ b/src/locking/lock_manager.h @@ -56,6 +56,7 @@ int virLockManagerAddResource(virLockManagerPtr manager, int virLockManagerAcquire(virLockManagerPtr manager, const char *state, unsigned int flags, + virDomainLockFailureAction action, int *fd); int virLockManagerRelease(virLockManagerPtr manager, char **state, -- 1.7.12

On Wed, Oct 10, 2012 at 01:35:32PM +0200, Jiri Denemark wrote:
--- Notes: Version 2: - no changes
src/Makefile.am | 2 +- src/locking/domain_lock.c | 11 +++++++---- src/locking/lock_driver.h | 5 ++++- src/locking/lock_driver_nop.c | 1 + src/locking/lock_driver_sanlock.c | 1 + src/locking/lock_manager.c | 7 ++++--- src/locking/lock_manager.h | 1 + 7 files changed, 19 insertions(+), 9 deletions(-)
ACK 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 :|

While the changes to sanlock driver should be stable, the actual implementation of sanlock_helper is supposed to be replaced in the future. However, before we can implement a better sanlock_helper, we need an administrative interface to libvirtd so that the helper can just pass a "leases lost" event to the particular libvirt driver and everything else will be taken care of internally. This approach will also allow libvirt to pass such event to applications and use appropriate reasons when changing domain states. The temporary implementation handles all actions directly by calling appropriate libvirt APIs (which among other things means that it needs to know the credentials required to connect to libvirtd). --- Notes: Version 2: - take URI rather than driver name as the first argument - make use of built-in infrastructure for loading credentials - add docs for sanlock_helper configuration - mark for sanlock_helper translation docs/locking.html.in | 24 +++++++ libvirt.spec.in | 1 + po/POTFILES.in | 1 + src/Makefile.am | 11 +++ src/locking/lock_driver_sanlock.c | 115 +++++++++++++++++++++++++++---- src/locking/sanlock_helper.c | 138 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 277 insertions(+), 13 deletions(-) create mode 100644 src/locking/sanlock_helper.c diff --git a/docs/locking.html.in b/docs/locking.html.in index 0d039da..6d7b517 100644 --- a/docs/locking.html.in +++ b/docs/locking.html.in @@ -208,5 +208,29 @@ </pool> </pre> + <h2><a name="domainconfig">Domain configuration</a></h2> + + <p> + In case sanlock loses access to disk locks for some reason, it will + kill all domains that lost their locks. This default behavior may + be changed using + <a href="formatdomain.html#elementsEvents">on_lockfailure + element</a> in domain XML. When this element is present, sanlock + will call <code>sanlock_helper</code> (provided by libvirt) with + the specified action. This helper binary will connect to libvirtd + and thus it may need to authenticate if libvirtd was configured to + require that on the read-write UNIX socket. To provide the + appropriate credentials to sanlock_helper, a + <a href="auth.html#Auth_client_config">client authentication + file</a> needs to contain something like the following: + </p> + <pre> +[auth-libvirt-localhost] +credentials=sanlock + +[credentials-sanlock] +authname=login +password=password + </pre> </body> </html> diff --git a/libvirt.spec.in b/libvirt.spec.in index e3d0a2d..318fe92 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1788,6 +1788,7 @@ rm -f $RPM_BUILD_ROOT%{_sysconfdir}/sysctl.d/libvirtd %dir %attr(0700, root, root) %{_localstatedir}/lib/libvirt/sanlock %{_sbindir}/virt-sanlock-cleanup %{_mandir}/man8/virt-sanlock-cleanup.8* +%attr(0755, root, root) %{_libexecdir}/libvirt_sanlock_helper %endif %files client -f %{name}.lang diff --git a/po/POTFILES.in b/po/POTFILES.in index 2538225..815e992 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -47,6 +47,7 @@ src/libvirt.c src/libvirt-qemu.c src/locking/lock_driver_sanlock.c src/locking/lock_manager.c +src/locking/sanlock_helper.c src/lxc/lxc_cgroup.c src/lxc/lxc_container.c src/lxc/lxc_conf.c diff --git a/src/Makefile.am b/src/Makefile.am index 0aefc02..4f19bcf 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -142,6 +142,8 @@ DRIVER_SOURCES = \ LOCK_DRIVER_SANLOCK_SOURCES = \ locking/lock_driver_sanlock.c +LOCK_DRIVER_SANLOCK_HELPER_SOURCES = \ + locking/sanlock_helper.c NETDEV_CONF_SOURCES = \ conf/netdev_bandwidth_conf.h conf/netdev_bandwidth_conf.c \ @@ -1649,6 +1651,15 @@ endif EXTRA_DIST += $(STORAGE_HELPER_DISK_SOURCES) +if HAVE_SANLOCK +libexec_PROGRAMS += libvirt_sanlock_helper + +libvirt_sanlock_helper_SOURCES = $(LOCK_DRIVER_SANLOCK_HELPER_SOURCES) +libvirt_sanlock_helper_CFLAGS = -I$(top_srcdir)/src/conf $(AM_CFLAGS) +libvirt_sanlock_helper_LDFLAGS = $(WARN_LDFLAGS) $(AM_LDFLAGS) +libvirt_sanlock_helper_LDADD = libvirt.la +endif + if WITH_LXC if WITH_LIBVIRTD libexec_PROGRAMS += libvirt_lxc diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 8c0ac8c..a218432 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -50,6 +50,7 @@ #define VIR_FROM_THIS VIR_FROM_LOCKING #define VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE "__LIBVIRT__DISKS__" +#define VIR_LOCK_MANAGER_SANLOCK_KILLPATH LIBEXECDIR "/libvirt_sanlock_helper" /* * temporary fix for the case where the sanlock devel package is @@ -75,8 +76,9 @@ struct _virLockManagerSanlockDriver { static virLockManagerSanlockDriver *driver = NULL; struct _virLockManagerSanlockPrivate { + const char *vm_uri; char vm_name[SANLK_NAME_LEN]; - char vm_uuid[VIR_UUID_BUFLEN]; + unsigned char vm_uuid[VIR_UUID_BUFLEN]; unsigned int vm_id; unsigned int vm_pid; unsigned int flags; @@ -383,6 +385,8 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock, priv->vm_pid = param->value.ui; } else if (STREQ(param->key, "id")) { priv->vm_id = param->value.ui; + } else if (STREQ(param->key, "uri")) { + priv->vm_uri = param->value.cstr; } } @@ -683,10 +687,86 @@ static int virLockManagerSanlockAddResource(virLockManagerPtr lock, return 0; } +static int +virLockManagerSanlockRegisterKillscript(int sock, + const char *vmuri, + const char *uuidstr, + virDomainLockFailureAction action) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *path; + char *args = NULL; + int ret = -1; + int rv; + + if (action > VIR_DOMAIN_LOCK_FAILURE_IGNORE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Failure action %s is not supported by sanlock"), + virDomainLockFailureTypeToString(action)); + goto cleanup; + } + + virBufferEscape(&buf, '\\', "\\ ", "%s", vmuri); + virBufferAddLit(&buf, " "); + virBufferEscape(&buf, '\\', "\\ ", "%s", uuidstr); + virBufferAddLit(&buf, " "); + virBufferEscape(&buf, '\\', "\\ ", "%s", + virDomainLockFailureTypeToString(action)); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + goto cleanup; + } + + /* Unfortunately, sanlock_killpath() does not use const for either + * path or args even though it will just copy them into its own + * buffers. + */ + path = (char *) VIR_LOCK_MANAGER_SANLOCK_KILLPATH; + args = virBufferContentAndReset(&buf); + + VIR_DEBUG("Register sanlock killpath: %s %s", path, args); + + /* sanlock_killpath() would just crop the strings */ + if (strlen(path) >= SANLK_HELPER_PATH_LEN) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Sanlock helper path is longer than %d: '%s'"), + SANLK_HELPER_PATH_LEN - 1, path); + goto cleanup; + } + if (strlen(args) >= SANLK_HELPER_ARGS_LEN) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Sanlock helper arguments are longer than %d:" + " '%s'"), + SANLK_HELPER_ARGS_LEN - 1, args); + goto cleanup; + } + + if ((rv = sanlock_killpath(sock, 0, path, args)) < 0) { + if (rv <= -200) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to register lock failure action:" + " error %d"), rv); + } else { + virReportSystemError(-rv, "%s", + _("Failed to register lock failure" + " action")); + } + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(args); + return ret; +} + static int virLockManagerSanlockAcquire(virLockManagerPtr lock, const char *state, unsigned int flags, - virDomainLockFailureAction action ATTRIBUTE_UNUSED, + virDomainLockFailureAction action, int *fd) { virLockManagerSanlockPrivatePtr priv = lock->privateData; @@ -741,23 +821,32 @@ static int virLockManagerSanlockAcquire(virLockManagerPtr lock, res_count = priv->res_count; } - VIR_DEBUG("Register sanlock %d", flags); /* We only initialize 'sock' if we are in the real * child process and we need it to be inherited * * If sock==-1, then sanlock auto-open/closes a * temporary sock */ - if (priv->vm_pid == getpid() && - (sock = sanlock_register()) < 0) { - if (sock <= -200) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to open socket to sanlock daemon: error %d"), - sock); - else - virReportSystemError(-sock, "%s", - _("Failed to open socket to sanlock daemon")); - goto error; + if (priv->vm_pid == getpid()) { + VIR_DEBUG("Register sanlock %d", flags); + if ((sock = sanlock_register()) < 0) { + if (sock <= -200) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to open socket to sanlock daemon: error %d"), + sock); + else + virReportSystemError(-sock, "%s", + _("Failed to open socket to sanlock daemon")); + goto error; + } + + if (action != VIR_DOMAIN_LOCK_FAILURE_DEFAULT) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(priv->vm_uuid, uuidstr); + if (virLockManagerSanlockRegisterKillscript(sock, priv->vm_uri, + uuidstr, action) < 0) + goto error; + } } if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) { diff --git a/src/locking/sanlock_helper.c b/src/locking/sanlock_helper.c new file mode 100644 index 0000000..a73b49c --- /dev/null +++ b/src/locking/sanlock_helper.c @@ -0,0 +1,138 @@ +#include <config.h> +#include <stdio.h> +#include <stdlib.h> +#include <locale.h> + +#include "configmake.h" +#include "internal.h" +#include "conf.h" +#include "memory.h" +#include "domain_conf.h" + + +static int +getArgs(int argc, + char **argv, + const char **uri, + const char **uuid, + virDomainLockFailureAction *action) +{ + int act; + + if (argc != 4) { + fprintf(stderr, _("%s uri uuid action\n"), argv[0]); + return -1; + } + + *uri = argv[1]; + *uuid = argv[2]; + + act = virDomainLockFailureTypeFromString(argv[3]); + if (act < 0) { + fprintf(stderr, _("invalid failure action: '%s'\n"), argv[3]); + return -1; + } + *action = act; + + return 0; +} + + +static int +authCallback(virConnectCredentialPtr cred ATTRIBUTE_UNUSED, + unsigned int ncred ATTRIBUTE_UNUSED, + void *cbdata ATTRIBUTE_UNUSED) +{ + return -1; +} + + +int +main(int argc, char **argv) +{ + const char *uri; + const char *uuid; + virDomainLockFailureAction action; + char *xml = NULL; + virConnectPtr conn = NULL; + virDomainPtr dom = NULL; + int ret = EXIT_FAILURE; + + int authTypes[] = { + VIR_CRED_AUTHNAME, + VIR_CRED_ECHOPROMPT, + VIR_CRED_PASSPHRASE, + VIR_CRED_NOECHOPROMPT, + }; + virConnectAuth auth = { + .credtype = authTypes, + .ncredtype = ARRAY_CARDINALITY(authTypes), + .cb = authCallback, + }; + + if (setlocale(LC_ALL, "") == NULL || + bindtextdomain(PACKAGE, LOCALEDIR) == NULL || + textdomain(PACKAGE) == NULL) { + fprintf(stderr, _("%s: initialization failed\n"), argv[0]); + exit(EXIT_FAILURE); + } + + if (getArgs(argc, argv, &uri, &uuid, &action) < 0) + goto cleanup; + + if (!(conn = virConnectOpenAuth(uri, &auth, 0)) || + !(dom = virDomainLookupByUUIDString(conn, uuid))) + goto cleanup; + + switch (action) { + case VIR_DOMAIN_LOCK_FAILURE_POWEROFF: + if (virDomainDestroy(dom) == 0 || + virDomainIsActive(dom) == 0) + ret = EXIT_SUCCESS; + break; + + case VIR_DOMAIN_LOCK_FAILURE_RESTART: + if (virDomainIsPersistent(dom)) { + if ((virDomainDestroy(dom) == 0 || + virDomainIsActive(dom) == 0) && + virDomainCreate(dom) == 0) + ret = EXIT_SUCCESS; + } else { + xml = virDomainGetXMLDesc(dom, + VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_INACTIVE); + + if (!xml || + (virDomainDestroy(dom) < 0 && + virDomainIsActive(dom) != 0)) + goto cleanup; + virDomainFree(dom); + if ((dom = virDomainCreateXML(conn, xml, 0))) + ret = EXIT_SUCCESS; + } + break; + + case VIR_DOMAIN_LOCK_FAILURE_PAUSE: + if (virDomainSuspend(dom) == 0) + ret = EXIT_SUCCESS; + break; + + case VIR_DOMAIN_LOCK_FAILURE_IGNORE: + ret = EXIT_SUCCESS; + break; + + default: + fprintf(stderr, _("unsupported failure action: '%s'\n"), + virDomainLockFailureTypeToString(action)); + break; + } + +cleanup: + if (dom) + virDomainFree(dom); + if (conn) + virConnectClose(conn); + VIR_FREE(xml); + + return ret; +} -- 1.7.12

On Wed, Oct 10, 2012 at 01:35:33PM +0200, Jiri Denemark wrote:
While the changes to sanlock driver should be stable, the actual implementation of sanlock_helper is supposed to be replaced in the future. However, before we can implement a better sanlock_helper, we need an administrative interface to libvirtd so that the helper can just pass a "leases lost" event to the particular libvirt driver and everything else will be taken care of internally. This approach will also allow libvirt to pass such event to applications and use appropriate reasons when changing domain states.
The temporary implementation handles all actions directly by calling appropriate libvirt APIs (which among other things means that it needs to know the credentials required to connect to libvirtd). --- Notes: Version 2: - take URI rather than driver name as the first argument - make use of built-in infrastructure for loading credentials - add docs for sanlock_helper configuration - mark for sanlock_helper translation
diff --git a/docs/locking.html.in b/docs/locking.html.in index 0d039da..6d7b517 100644 --- a/docs/locking.html.in +++ b/docs/locking.html.in @@ -208,5 +208,29 @@ </pool> </pre>
+ <h2><a name="domainconfig">Domain configuration</a></h2> + + <p> + In case sanlock loses access to disk locks for some reason, it will + kill all domains that lost their locks. This default behavior may + be changed using + <a href="formatdomain.html#elementsEvents">on_lockfailure + element</a> in domain XML. When this element is present, sanlock + will call <code>sanlock_helper</code> (provided by libvirt) with + the specified action. This helper binary will connect to libvirtd + and thus it may need to authenticate if libvirtd was configured to + require that on the read-write UNIX socket. To provide the + appropriate credentials to sanlock_helper, a + <a href="auth.html#Auth_client_config">client authentication + file</a> needs to contain something like the following: + </p> + <pre> +[auth-libvirt-localhost] +credentials=sanlock + +[credentials-sanlock] +authname=login +password=password + </pre>
Hmm, I think it might be a little more complicated. IIRC, the sanlock daemon runs under a dedicated user ID, so it will hit the policykit auth rules by default. So should we be dropping in a .pkla file with the libvirt sanlock RPM to allow this script to run. We might need to mention where the config file should be located too. ACK in any case since this is docs stuff only 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 :|

On Wed, Oct 10, 2012 at 15:11:18 +0100, Daniel P. Berrange wrote:
On Wed, Oct 10, 2012 at 01:35:33PM +0200, Jiri Denemark wrote:
+ <h2><a name="domainconfig">Domain configuration</a></h2> + + <p> + In case sanlock loses access to disk locks for some reason, it will + kill all domains that lost their locks. This default behavior may + be changed using + <a href="formatdomain.html#elementsEvents">on_lockfailure + element</a> in domain XML. When this element is present, sanlock + will call <code>sanlock_helper</code> (provided by libvirt) with + the specified action. This helper binary will connect to libvirtd + and thus it may need to authenticate if libvirtd was configured to + require that on the read-write UNIX socket. To provide the + appropriate credentials to sanlock_helper, a + <a href="auth.html#Auth_client_config">client authentication + file</a> needs to contain something like the following: + </p> + <pre> +[auth-libvirt-localhost] +credentials=sanlock + +[credentials-sanlock] +authname=login +password=password + </pre>
Hmm, I think it might be a little more complicated. IIRC, the sanlock daemon runs under a dedicated user ID, so it will hit the policykit auth rules by default. So should we be dropping in a .pkla file with the libvirt sanlock RPM to allow this script to run.
Ah, that's possible. I'll prepare an additional patch for that if it appears to be necessary.
We might need to mention where the config file should be located too.
That's done by linking to auth.html#Auth_client_config, which mentions all the possibilities where to store that file.
ACK in any case since this is docs stuff only
Thanks, I pushed this series. Jirka
participants (2)
-
Daniel P. Berrange
-
Jiri Denemark