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@xxxxxxxxxxxx>
> >> 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@xxxxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list