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

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

 



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);
> 

--
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]