[libvirt] [PATCH] qemu: add two hook script events "prepare" and "release"

[Daniel Veillard]

On Tue, Mar 22, 2011 at 11:47:03AM +0100, Thibault VINCENT wrote:
> On 22/03/2011 10:59, Daniel Veillard wrote:
> > On Mon, Mar 21, 2011 at 10:36:55AM +0100, Thibault VINCENT wrote:
> >> >From 827261dbe2ab8f1cfd8c84df14bb42fd04df2307 Mon Sep 17 00:00:00 2001
> >> From: Thibault VINCENT <thibault.vincent at smartjog.com>
> >> Date: Mon, 21 Mar 2011 10:18:06 +0100
> >> Subject: [PATCH] qemu: add two hook script events "prepare" and "release"
> >>
> >>  * Fix for bug #618970
> >>
> >>  * The "prepare" hook is called very early in the VM statup process
> >> before device labeling, so that it can allocate ressources not
> >> managed by libvirt, such as DRBD, or for instance create missing
> >> bridges and vlan interfaces.
> >>
> >>  * To shutdown these devices, the "release" hook is also required at
> >> the very end of the VM destruction.
> >>
> >> Signed-off-by: Thibault VINCENT <thibault.vincent at smartjog.com>
> >> ---
> >>  src/qemu/qemu_process.c |   26 ++++++++++++++++++++++++++
> >>  src/util/hooks.c        |    4 +++-
> >>  src/util/hooks.h        |    2 ++
> >>  3 files changed, 31 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> >> index 793a43c..7831c3b 100644
> >> --- a/src/qemu/qemu_process.c
> >> +++ b/src/qemu/qemu_process.c
> >> @@ -1927,6 +1927,22 @@ int qemuProcessStart(virConnectPtr conn,
> >>
> >>      vm->def->id = driver->nextvmid++;
> >>
> >> +    /* Run a early hook to set-up missing devices */
> >> +    if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) {
> >> +        char *xml = virDomainDefFormat(vm->def, 0);
> >> +        int hookret;
> >> +
> >> +        hookret = virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name,
> >> +                    VIR_HOOK_QEMU_OP_PREPARE, VIR_HOOK_SUBOP_BEGIN,
> >> NULL, xml);
> >> +        VIR_FREE(xml);
> >> +
> >> +        /*
> >> +         * If the script raised an error abort the launch
> >> +         */
> >> +        if (hookret < 0)
> >> +            goto cleanup;
> >> +    }
> > 
> >   Hum, the main problem I see there is that we may fail startup there
> > and go to cleanup. In that case the domain is not started but no hook is
> > called to reverse the operation. I wonder if VIR_HOOK_QEMU_OP_RELEASE
> > shouldn't be called in the cleanup ... if present, to avoid resource
> > leak when startup fails.
> 
> The cleanup section of qemuProcessStart() does a call to
> qemuProcessStop() so I assumed it was reversed correctly.

 Dohh, I missed that, okay !

> >>      /* Must be run before security labelling */
> >>      VIR_DEBUG0("Preparing host devices");
> >>      if (qemuPrepareHostDevices(driver, vm->def) < 0)
> >> @@ -2419,6 +2435,16 @@ retry:
> >>      VIR_FREE(priv->vcpupids);
> >>      priv->nvcpupids = 0;
> >>
> >> +    /* The "release" hook cleans up additional ressources */
> >> +    if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) {
> >> +        char *xml = virDomainDefFormat(vm->def, 0);
> >> +
> >> +        /* we can't stop the operation even if the script raised an
> >> error */
> >> +        virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name,
> >> +                    VIR_HOOK_QEMU_OP_RELEASE, VIR_HOOK_SUBOP_END, NULL,
> >> xml);
> >> +        VIR_FREE(xml);
> >> +    }
> >> +
> > 
> >   That's okay, but possibly not sufficient.
> > 
> >>      if (vm->newDef) {
> >>          virDomainDefFree(vm->def);
> >>          vm->def = vm->newDef;
> >> diff --git a/src/util/hooks.c b/src/util/hooks.c
> >> index 5ba2036..c258318 100644
> >> --- a/src/util/hooks.c
> >> +++ b/src/util/hooks.c
> >> @@ -70,8 +70,10 @@ VIR_ENUM_IMPL(virHookSubop, VIR_HOOK_SUBOP_LAST,
> >>                "end")
> >>
> >>  VIR_ENUM_IMPL(virHookQemuOp, VIR_HOOK_QEMU_OP_LAST,
> >> +              "prepare",
> >>                "start",
> >> -              "stopped")
> >> +              "stopped",
> >> +              "release")
> > 
> >   prepare should be added after stopped to avoid changing the order.
> > 
> >>  VIR_ENUM_IMPL(virHookLxcOp, VIR_HOOK_LXC_OP_LAST,
> >>                "start",
> >> diff --git a/src/util/hooks.h b/src/util/hooks.h
> >> index f311ed1..66b378a 100644
> >> --- a/src/util/hooks.h
> >> +++ b/src/util/hooks.h
> >> @@ -52,8 +52,10 @@ enum virHookSubopType {
> >>  };
> >>
> >>  enum virHookQemuOpType {
> >> +    VIR_HOOK_QEMU_OP_PREPARE,          /* domain startup initiated */
> >>      VIR_HOOK_QEMU_OP_START,            /* domain is about to start */
> >>      VIR_HOOK_QEMU_OP_STOPPED,          /* domain has stopped */
> >> +    VIR_HOOK_QEMU_OP_RELEASE,          /* domain destruction is over */
> > 
> >   same thing, only add at the end.
> > 
> >>      VIR_HOOK_QEMU_OP_LAST,
> >>  };
> > 
> >   Fixing the 2 last problems is trivial and I did it in my tree, but I
> > don't feel like commiting this without handling the failure to start
> > the domain (running VIR_HOOK_QEMU_OP_RELEASE with a new virHookSubopType
> > VIR_HOOK_SUBOP_FAILED for example).
> > 
> 
> That would imply the ressources allocated by the PREPARE hook should be
> deconfigured differently whether they where used once or not. And the
> user should take great care of the SUBOP provided to it's script,
> because in a failure situation it would receive two RELEASE calls, one
> with VIR_HOOK_SUBOP_FAILED then a second with VIR_HOOK_SUBOP_END.
> 
> Given that failure seems to be handled already, maybe it's sufficient
> not to use an other SUBOP. It's depends if you feel like adding one more
> feature.

  Okay, agreed, then a simple reorg of the enums looks sufficient to me !
I just did that amd pushed.
  Now there is a small TODO left consisting of extending the
documentation about those two, and maybe look if the LXC driver could
support those hooks too,

   thanks !

Daniel


-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/