Re: [libvirt] [PATCH 3/3] libxl: add hooks support

On 07/27/2016 02:42 PM, Cédric Bosdonnat wrote: > Introduce libxl hook and use it for start, prepare, started, > stop, stopped, migrate events. Looks good to me with one comment and few nits. Looking at lxc and qemu drivers virHook support seems to be similar so I am assuming this is correct. But this initial review has a grain of salt as I am not fully familiar (yet) with virHook support. > --- > docs/hooks.html.in | 53 ++++++++++++++++++++++++++++++-- > src/libxl/libxl_domain.c | 75 +++++++++++++++++++++++++++++++++++++++++++++ > src/libxl/libxl_driver.c | 24 +++++++++++++++ > src/libxl/libxl_migration.c | 58 +++++++++++++++++++++++++++++++++++ > src/util/virhook.c | 16 +++++++++- > src/util/virhook.h | 13 ++++++++ > 6 files changed, 236 insertions(+), 3 deletions(-) > > diff --git a/docs/hooks.html.in b/docs/hooks.html.in > index d4f4ac3..11073cb 100644 > --- a/docs/hooks.html.in > +++ b/docs/hooks.html.in > @@ -17,8 +17,10 @@ > (<span class="since">since 0.8.0</span>)<br/><br/></li> > <li>A QEMU guest is started or stopped > (<span class="since">since 0.8.0</span>)<br/><br/></li> > - <li>An LXC guest is started or stopped > + <li>An LXC guest is started or stopped > (<span class="since">since 0.8.0</span>)<br/><br/></li> > + <li>A libxl-handled Xen guest is started or stopped > + (<span class="since">since 2.1.0</span>)<br/><br/></li> > <li>A network is started or stopped or an interface is > plugged/unplugged to/from the network > (<span class="since">since 1.2.2</span>)<br/><br/></li> > @@ -41,7 +43,7 @@ > <br/> > > <h2><a name="names">Script names</a></h2> > - <p>At present, there are three hook scripts that can be called:</p> > + <p>At present, there are five hook scripts that can be called:</p> > <ul> > <li><code>/etc/libvirt/hooks/daemon</code><br/><br/> > Executed when the libvirt daemon is started, stopped, or reloads > @@ -50,6 +52,9 @@ > Executed when a QEMU guest is started, stopped, or migrated<br/><br/></li> > <li><code>/etc/libvirt/hooks/lxc</code><br /><br/> > Executed when an LXC guest is started or stopped</li> > + <li><code>/etc/libvirt/hooks/libxl</code><br/><br/> > + Executed when a libxl-handled Xen guest is started, stopped, or > + migrated<br/><br/></li> > <li><code>/etc/libvirt/hooks/network</code><br/><br/> > Executed when a network is started or stopped or an > interface is plugged/unplugged to/from the network</li> > @@ -235,6 +240,50 @@ > </li> > </ul> > > + <h5><a name="libxl">/etc/libvirt/hooks/libxl</a></h5> > + <ul> > + <li>Before a Xen guest is started using libxl driver, the libxl hook > + script is called in three locations; if any location fails, the guest > + is not started. The first location, <span class="since">since > + 2.1.0</span>, is before libvirt performs any resource > + labeling, and the hook can allocate resources not managed by > + libvirt. This is called as:<br/> > + <pre>/etc/libvirt/hooks/libxl guest_name prepare begin -</pre> > + The second location, available <span class="since">Since > + 2.1.0</span>, occurs after libvirt has finished labeling > + all resources, but has not yet started the guest, called as:<br/> > + <pre>/etc/libvirt/hooks/libxl guest_name start begin -</pre> > + The third location, <span class="since">2.1.0</span>, > + occurs after the domain has successfully started up:<br/> > + <pre>/etc/libvirt/hooks/libxl guest_name started begin -</pre> > + </li> > + <li>When a libxl-handled Xen guest is stopped, the libxl hook script > + is called in two locations, to match the startup. > + First, <span class="since">since 2.1.0</span>, the hook is > + called before libvirt restores any labels:<br/> > + <pre>/etc/libvirt/hooks/libxl guest_name stopped end -</pre> > + Then, after libvirt has released all resources, the hook is > + called again, <span class="since">since 2.1.0</span>, to allow > + any additional resource cleanup:<br/> > + <pre>/etc/libvirt/hooks/libxl guest_name release end -</pre></li> > + <li><span class="since">Since 2.1.0</span>, the libxl hook script > + is also called at the beginning of incoming migration. It is called > + as: <pre>/etc/libvirt/hooks/libxl guest_name migrate begin -</pre> > + with domain XML sent to standard input of the script. In this case, > + the script acts as a filter and is supposed to modify the domain > + XML and print it out on its standard output. Empty output is > + identical to copying the input XML without changing it. In case the > + script returns failure or the output XML is not valid, incoming > + migration will be canceled. This hook may be used, e.g., to change > + location of disk images for incoming domains.</li> > + <li><span class="since">Since 2.1.0</span>, the libxl hook script > + is also called when the libvirtd daemon restarts and reconnects > + to previously running Xen domains. If the script fails, the > + existing Xen domains will be killed off. It is called as: > + <pre>/etc/libvirt/hooks/libxl guest_name reconnect begin -</pre> > + </li> > + </ul> > + Not sure what's the norm for documentation patches on libvirt, but I still leave the suggestion of making docs part on a separate patch.

> <h5><a name="network">/etc/libvirt/hooks/network</a></h5> > <ul> > <li><span class="since">Since 1.2.2</span>, before a network is started, > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c > index 886b40f..d04dc5e 100644 > --- a/src/libxl/libxl_domain.c > +++ b/src/libxl/libxl_domain.c > @@ -32,6 +32,7 @@ > #include "viratomic.h" > #include "virfile.h" > #include "virerror.h" > +#include "virhook.h" > #include "virlog.h" > #include "virstring.h" > #include "virtime.h" > @@ -737,6 +738,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, > hostdev_flags |= VIR_HOSTDEV_SP_USB; > #endif > > + /* now that we know it's stopped call the hook if present */ > + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { > + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); > + > + /* we can't stop the operation even if the script raised an error */ > + virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, > + VIR_HOOK_LIBXL_OP_STOPPED, VIR_HOOK_SUBOP_END, > + NULL, xml, NULL); Shouldn't ignore_value(...) be around the virHookCall since we are ignoring the return value?

> + VIR_FREE(xml); > + } > + > virHostdevReAttachDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME, > vm->def, hostdev_flags, NULL); > > @@ -788,6 +800,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, > VIR_FREE(file); > } > > + /* The "release" hook cleans up additional resources */ > + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { > + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); > + > + /* we can't stop the operation even if the script raised an error */ > + virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, > + VIR_HOOK_LIBXL_OP_RELEASE, VIR_HOOK_SUBOP_END, > + NULL, xml, NULL); Same here? > + VIR_FREE(xml); > + } > + > if (vm->newDef) { > virDomainDefFree(vm->def); > vm->def = vm->newDef; > @@ -1107,6 +1130,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, > if (virDomainObjSetDefTransient(cfg->caps, driver->xmlopt, vm) < 0) > goto cleanup; > > + /* Run an early hook to set-up missing devices */ > + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { > + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); > + int hookret; > + > + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, > + VIR_HOOK_LIBXL_OP_PREPARE, VIR_HOOK_SUBOP_BEGIN, > + NULL, xml, NULL); > + VIR_FREE(xml); > + > + /* > + * If the script raised an error abort the launch > + */ > + if (hookret < 0) > + goto cleanup_dom; > + } > + > if (virDomainLockProcessStart(driver->lockManager, > "xen:///system", > vm, > @@ -1135,6 +1175,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, > vm->def, hostdev_flags) < 0) > goto cleanup_dom; > > + /* now that we know it is about to start call the hook if present */ > + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { > + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); > + int hookret; > + > + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, > + VIR_HOOK_LIBXL_OP_START, VIR_HOOK_SUBOP_BEGIN, > + NULL, xml, NULL); > + VIR_FREE(xml); > + > + /* > + * If the script raised an error abort the launch > + */ > + if (hookret < 0) > + goto cleanup_dom; > + } > + > if (priv->hookRun) { > char uuidstr[VIR_UUID_STRING_BUFLEN]; > virUUIDFormat(vm->def->uuid, uuidstr); > @@ -1143,6 +1200,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, > vm->def->id, > vm->def->name, > uuidstr); > + Spurious whitespace. > } > > /* Unlock virDomainObj while creating the domain */ > @@ -1215,6 +1273,23 @@ libxlDomainStart(libxlDriverPrivatePtr driver, > if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) > driver->inhibitCallback(true, driver->inhibitOpaque); > > + /* finally we can call the 'started' hook script if any */ > + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { > + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); > + int hookret; > + > + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, > + VIR_HOOK_LIBXL_OP_STARTED, VIR_HOOK_SUBOP_BEGIN, > + NULL, xml, NULL); > + VIR_FREE(xml); > + > + /* > + * If the script raised an error abort the launch > + */ > + if (hookret < 0) > + goto cleanup_dom; > + } > + > event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, > restore_fd < 0 ? > VIR_DOMAIN_EVENT_STARTED_BOOTED : > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index 5008c00..f500892 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > @@ -42,6 +42,7 @@ > #include "virfile.h" > #include "viralloc.h" > #include "viruuid.h" > +#include "virhook.h" > #include "vircommand.h" > #include "libxl_domain.h" > #include "libxl_driver.h" > @@ -415,6 +416,29 @@ libxlReconnectDomain(virDomainObjPtr vm, > /* Enable domain death events */ > libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW); > > + /* now that we know it's reconnected call the hook if present */ > + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL) && > + STRNEQ("Domain-0", vm->def->name)) { > + char *xml = virDomainDefFormat(vm->def, cfg->caps, 0); > + int hookret; > + > + /* we can't stop the operation even if the script raised an error */ > + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, vm->def->name, > + VIR_HOOK_LIBXL_OP_RECONNECT, VIR_HOOK_SUBOP_BEGIN, > + NULL, xml, NULL); > + VIR_FREE(xml); > + if (hookret < 0) { > + /* Stop the domain if the hook failed */ > + if (virDomainObjIsActive(vm)) { > + libxlDomainDestroyInternal(driver, vm); > + virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); > + } > + goto error; > + } > + } > + > + ret = 0; As mentioned in Patch 2, I think this needs to be a part of patch 2. > + > cleanup: > libxl_dominfo_dispose(&d_info); > virObjectUnlock(vm); > diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c > index bf285a4..1885d49 100644 > --- a/src/libxl/libxl_migration.c > +++ b/src/libxl/libxl_migration.c > @@ -36,6 +36,7 @@ > #include "virstring.h" > #include "virobject.h" > #include "virthread.h" > +#include "virhook.h" > #include "rpc/virnetsocket.h" > #include "libxl_domain.h" > #include "libxl_driver.h" > @@ -490,9 +491,11 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, > unsigned int flags) > { > libxlDriverPrivatePtr driver = dconn->privateData; > + libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver); > libxlMigrationCookiePtr mig = NULL; > virDomainObjPtr vm = NULL; > char *hostname = NULL; > + char *xmlout = NULL; > unsigned short port; > char portstr[100]; > virURIPtr uri = NULL; > @@ -500,6 +503,8 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, > size_t nsocks = 0; > int nsocks_listen = 0; > libxlMigrationDstArgs *args = NULL; > + bool taint_hook = false; > + libxlDomainObjPrivatePtr priv = NULL; > size_t i; > int ret = -1; > > @@ -513,6 +518,51 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, > goto error; > } > > + /* Let migration hook filter domain XML */ > + if (virHookPresent(VIR_HOOK_DRIVER_LIBXL)) { > + char *xml; > + int hookret; > + > + if (!(xml = virDomainDefFormat(*def, cfg->caps, > + VIR_DOMAIN_XML_SECURE | > + VIR_DOMAIN_XML_MIGRATABLE))) > + goto error; > + > + hookret = virHookCall(VIR_HOOK_DRIVER_LIBXL, (*def)->name, > + VIR_HOOK_LIBXL_OP_MIGRATE, VIR_HOOK_SUBOP_BEGIN, > + NULL, xml, &xmlout); > + VIR_FREE(xml); > + > + if (hookret < 0) { > + goto error; > + } else if (hookret == 0) { > + if (virStringIsEmpty(xmlout)) { > + VIR_DEBUG("Migrate hook filter returned nothing; using the" > + " original XML"); > + } else { > + virDomainDefPtr newdef; > + > + VIR_DEBUG("Using hook-filtered domain XML: %s", xmlout); > + newdef = virDomainDefParseString(xmlout, cfg->caps, driver->xmlopt, > + VIR_DOMAIN_DEF_PARSE_INACTIVE | > + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); > + if (!newdef) > + goto error; > + > + /* TODO At some stage we will want to have some check of what the user > + * did in the hook. */ > + > + virDomainDefFree(*def); > + *def = newdef; > + /* We should taint the domain here. However, @vm and therefore > + * privateData too are still NULL, so just notice the fact and > + * taint it later. */ > + taint_hook = true; > + } > + } > + } > + > + Probably one newline is enough here instead of two after the closing bracket. > if (!(vm = virDomainObjListAdd(driver->domains, *def, > driver->xmlopt, > VIR_DOMAIN_OBJ_LIST_ADD_LIVE | > @@ -520,6 +570,12 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, > NULL))) > goto error; > *def = NULL; > + priv = vm->privateData; > + > + if (taint_hook) { > + /* Domain XML has been altered by a hook script. */ > + priv->hookRun = true; > + } > > /* Create socket connection to receive migration data */ > if (!uri_in) { > @@ -640,6 +696,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, > } > > done: > + VIR_FREE(xmlout); > libxlMigrationCookieFree(mig); > if (!uri_in) > VIR_FREE(hostname); > @@ -647,6 +704,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, > virURIFree(uri); > if (vm) > virObjectUnlock(vm); > + virObjectUnref(cfg); > return ret; > } > > diff --git a/src/util/virhook.c b/src/util/virhook.c > index a8422a2..facd74a 100644 > --- a/src/util/virhook.c > +++ b/src/util/virhook.c > @@ -51,13 +51,15 @@ VIR_ENUM_DECL(virHookSubop) > VIR_ENUM_DECL(virHookQemuOp) > VIR_ENUM_DECL(virHookLxcOp) > VIR_ENUM_DECL(virHookNetworkOp) > +VIR_ENUM_DECL(virHookLibxlOp) > > VIR_ENUM_IMPL(virHookDriver, > VIR_HOOK_DRIVER_LAST, > "daemon", > "qemu", > "lxc", > - "network") > + "network", > + "libxl") > > VIR_ENUM_IMPL(virHookDaemonOp, VIR_HOOK_DAEMON_OP_LAST, > "start", > @@ -96,6 +98,15 @@ VIR_ENUM_IMPL(virHookNetworkOp, VIR_HOOK_NETWORK_OP_LAST, > "unplugged", > "updated") > > +VIR_ENUM_IMPL(virHookLibxlOp, VIR_HOOK_LIBXL_OP_LAST, > + "start", > + "stopped", > + "prepare", > + "release", > + "migrate", > + "started", > + "reconnect") > + > static int virHooksFound = -1; > > /** > @@ -261,6 +272,9 @@ virHookCall(int driver, > case VIR_HOOK_DRIVER_LXC: > opstr = virHookLxcOpTypeToString(op); > break; > + case VIR_HOOK_DRIVER_LIBXL: > + opstr = virHookLibxlOpTypeToString(op); > + break; > case VIR_HOOK_DRIVER_NETWORK: > opstr = virHookNetworkOpTypeToString(op); > } > diff --git a/src/util/virhook.h b/src/util/virhook.h > index 4015426..205249c 100644 > --- a/src/util/virhook.h > +++ b/src/util/virhook.h > @@ -31,6 +31,7 @@ typedef enum { > VIR_HOOK_DRIVER_QEMU, /* QEmu domains related events */ > VIR_HOOK_DRIVER_LXC, /* LXC domains related events */ > VIR_HOOK_DRIVER_NETWORK, /* network related events */ > + VIR_HOOK_DRIVER_LIBXL, /* Xen libxl domains related events */ > > VIR_HOOK_DRIVER_LAST, > } virHookDriverType; > @@ -87,6 +88,18 @@ typedef enum { > VIR_HOOK_NETWORK_OP_LAST, > } virHookNetworkOpType; > > +typedef enum { > + VIR_HOOK_LIBXL_OP_START, /* domain is about to start */ > + VIR_HOOK_LIBXL_OP_STOPPED, /* domain has stopped */ > + VIR_HOOK_LIBXL_OP_PREPARE, /* domain startup initiated */ > + VIR_HOOK_LIBXL_OP_RELEASE, /* domain destruction is over */ > + VIR_HOOK_LIBXL_OP_MIGRATE, /* domain is being migrated */ > + VIR_HOOK_LIBXL_OP_STARTED, /* domain has started */ > + VIR_HOOK_LIBXL_OP_RECONNECT, /* domain is being reconnected by libvirt */ > + > + VIR_HOOK_LIBXL_OP_LAST, > +} virHookLibxlOpType; > + > int virHookInitialize(void); > > int virHookPresent(int driver); >