[libvirt] [PATCH] qemu: add two hook script events "prepare" and "release"
From 827261dbe2ab8f1cfd8c84df14bb42fd04df2307 Mon Sep 17 00:00:00 2001 From: Thibault VINCENT <thibault.vincent@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@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; + } + /* 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); + } + 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") 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 */ VIR_HOOK_QEMU_OP_LAST, }; -- 1.7.2.3
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@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@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.
/* 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). Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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@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@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. Could you give a scenario where VIR_HOOK_QEMU_OP_RELEASE would not be reached if a jump to cleanup occurs ? I think it's fine to do this as in any case (fail or normal stop) the domain was marked "shutdown" and freed only in qemuProcessStop() where I placed the "release" hook.
/* 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. Thanks Daniel! Cheers -- Thibault VINCENT - IT SmartJog S.A.S. - Groupe TDF - Pôle multimédia 27 Bd Hippolyte Marques, 94200 Ivry-sur-Seine, France Phone: +33.1.58.68.62.38 / Fax: +33.1.58.68.60.97
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@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@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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
* src/qemu/qemu_process.c (qemuProcessStart, qemuProcessStop): Fix typos. * docs/hooks.html.in: Document 'prepare' and 'release' hooks. ---
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,
I'm not a fan of releasing undocumented stuff. I didn't touch lxc (that can be a later day), but this should fix the qemu docs. docs/hooks.html.in | 23 +++++++++++++++++++---- src/qemu/qemu_process.c | 4 ++-- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/docs/hooks.html.in b/docs/hooks.html.in index 3503f8c..eec7a6a 100644 --- a/docs/hooks.html.in +++ b/docs/hooks.html.in @@ -100,11 +100,26 @@ <h5><a name="qemu">/etc/libvirt/hooks/qemu</a></h5> <ul> - <li>When a QEMU guest is started, the qemu hook script is called as:<br/> - <pre>/etc/libvirt/hooks/qemu guest_name start begin -</pre></li> + <li>Before a QEMU guest is started, the qemu hook script is + called in two locations; if either location fails, the guest + is not started. The first location, <span class="since">since + 0.9.0</span>, is before libvirt performs any resource + labeling, and the hook can allocate resources not managed by + libvirt such as DRBD or missing bridges. This is called as:</br> + <pre>/etc/libvirt/hooks/qemu guest_name prepare begin -</pre> + The second location, available <span class="since">Since + 0.8.0</span>, occurs after libvirt has finished labeling + all resources, but has not yet started the guest, called as:</br> + <pre>/etc/libvirt/hooks/qemu guest_name start begin -</pre></li> <li>When a QEMU guest is stopped, the qemu hook script is called - as:<br/> - <pre>/etc/libvirt/hooks/qemu guest_name stopped end -</pre></li> + in two locations, to match the startup. + First, <span class="since">since 0.8.0</span>, the hook is + called before libvirt restores any labels:</br> + <pre>/etc/libvirt/hooks/qemu guest_name stopped end -</pre> + Then, after libvirt has released all resources, the hook is + called again, <span class="since">since 0.9.0</span>, to allow + any additional resource cleanup:<br/> + <pre>/etc/libvirt/hooks/qemu guest_name release end -</pre></li> </ul> <h5><a name="lxc">/etc/libvirt/hooks/lxc</a></h5> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7831c3b..35281c3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1927,7 +1927,7 @@ int qemuProcessStart(virConnectPtr conn, vm->def->id = driver->nextvmid++; - /* Run a early hook to set-up missing devices */ + /* Run an early hook to set-up missing devices */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { char *xml = virDomainDefFormat(vm->def, 0); int hookret; @@ -2435,7 +2435,7 @@ retry: VIR_FREE(priv->vcpupids); priv->nvcpupids = 0; - /* The "release" hook cleans up additional ressources */ + /* The "release" hook cleans up additional resources */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { char *xml = virDomainDefFormat(vm->def, 0); -- 1.7.4
participants (3)
-
Daniel Veillard -
Eric Blake -
Thibault VINCENT