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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]