On Wed, 2016-07-27 at 17:40 +0100, Joao Martins wrote:
> 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.
Usually we have doc within the commit introducing the feature.
git log docs/hooks.html.in can confirm it.
> > <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?
Indeed, it's used in the qemu driver, but not in the lxc one... I'll add it.
I'll resubmit with your comments addressed. Thanks for the review.
--
Cedric
> > + 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);
> >
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list