[libvirt] [PATCH 0/4] Fix issues with guest lifecycle event action settings
https://bugzilla.redhat.com/show_bug.cgi?id=916052 Domain lifecycle events "on_reboot" and "on_poweroff" translate into QEMU command options "no-reboot" and "no-shutdown" at startup time. The existing code only checked the "on_reboot" setting to determine whether to add the "no-reboot" option even though "on_poweroff" could have been set to allow reboot. Changed the logic to only use "no-reboot" if each of the lifecycle event actions was "destroy". I considered adding "preserve" as well; however, a future preserve action might be able to cull something from a qemu process that doesn't just exit immediately. NB, previously it was possible to have both QEMU command options on the command line, which while not tagged as an error could be considered "odd". During the shutdown or reboot domain operations, the calls to qemuAgentShutdown and qemuDomainSetFakeReboot did not consider the possibility that the event action for shutdown was reboot or the event action for reboot was destroy. This patch fixes that. NB, I wasn't sure how I could "convey" that to the user though. The virsh reboot will still issue the message "Domain is being rebooted" even though it could be destroyed. Like was virsh shutdown will still issue the message "Domain is being shutdown" even though it is being rebooted. The usage of VIR_INFO was an attempt to do that even though one would have to have the correct debug level set. Adjusted the documentation in order to describe the possible actions based on the event lifecycle action settings of the domain. John Ferlan (4): Adjust usage of qemu -no-reboot and -no-shutdown options Adjust comments to describe on_poweroff and on_reboot action Handle the domain event 'on_reboot' and 'on_poweroff' settings docs: Update formatdomain for lifecycle events docs/formatdomain.html.in | 30 ++++++++++++++++++++++-------- src/libvirt.c | 15 +++++++++++---- src/qemu/qemu_command.c | 17 +++++++++++++---- src/qemu/qemu_driver.c | 26 ++++++++++++++++++++++---- 4 files changed, 68 insertions(+), 20 deletions(-) -- 1.8.1.4
During building of the qemu command line determine whether to add/use the '-no-reboot' option only if each of the 'on' events want to to destroy the domain; otherwise, use the '-no-shutdown' option. Prior to this change both could be on the command line, which while allowed could be construed as a conflict. --- src/qemu/qemu_command.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 741fa82..9f63781 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5880,6 +5880,7 @@ qemuBuildCommandLine(virConnectPtr conn, int last_good_net = -1; bool hasHwVirt = false; virCommandPtr cmd = NULL; + bool allowReboot = true; bool emitBootindex = false; int sdl = 0; int vnc = 0; @@ -6262,16 +6263,24 @@ qemuBuildCommandLine(virConnectPtr conn, } } - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_REBOOT) && - def->onReboot != VIR_DOMAIN_LIFECYCLE_RESTART) - virCommandAddArg(cmd, "-no-reboot"); + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_REBOOT)) { + /* Only add -no-reboot option if each event destroys domain */ + if (def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY && + def->onPoweroff == VIR_DOMAIN_LIFECYCLE_DESTROY && + def->onCrash == VIR_DOMAIN_LIFECYCLE_DESTROY) { + allowReboot = false; + virCommandAddArg(cmd, "-no-reboot"); + } + } /* If JSON monitor is enabled, we can receive an event * when QEMU stops. If we use no-shutdown, then we can * watch for this event and do a soft/warm reboot. */ - if (monitor_json && virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) + if (monitor_json && allowReboot && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) { virCommandAddArg(cmd, "-no-shutdown"); + } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NO_ACPI)) { if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI))) -- 1.8.1.4
--- src/libvirt.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index c236152..4bd6c9a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3184,9 +3184,11 @@ error: * virDomainShutdown: * @domain: a domain object * - * Shutdown a domain, the domain object is still usable thereafter but + * Shutdown a domain, the domain object is still usable thereafter, but * the domain OS is being stopped. Note that the guest OS may ignore the - * request. For guests that react to a shutdown request, the differences + * request. Additionally, the hypervisor may check and support the domain + * 'on_poweroff' XML setting resulting in a domain that reboots instead of + * shutting down. For guests that react to a shutdown request, the differences * from virDomainDestroy() are that the guests disk storage will be in a * stable state rather than having the (virtual) power cord pulled, and * this command returns as soon as the shutdown request is issued rather @@ -3241,7 +3243,9 @@ error: * * Shutdown a domain, the domain object is still usable thereafter but * the domain OS is being stopped. Note that the guest OS may ignore the - * request. For guests that react to a shutdown request, the differences + * request. Additionally, the hypervisor may check and support the domain + * 'on_poweroff' XML setting resulting in a domain that reboots instead of + * shutting down. For guests that react to a shutdown request, the differences * from virDomainDestroy() are that the guest's disk storage will be in a * stable state rather than having the (virtual) power cord pulled, and * this command returns as soon as the shutdown request is issued rather @@ -3300,9 +3304,12 @@ error: * @domain: a domain object * @flags: bitwise-OR of virDomainRebootFlagValues * - * Reboot a domain, the domain object is still usable there after but + * Reboot a domain, the domain object is still usable thereafter, but * the domain OS is being stopped for a restart. * Note that the guest OS may ignore the request. + * Additionally, the hypervisor may check and support the domain + * 'on_reboot' XML setting resulting in a domain that shuts down instead + * of rebooting. * * If @flags is set to zero, then the hypervisor will choose the * method of shutdown it considers best. To have greater control -- 1.8.1.4
On 24.04.2013 13:40, John Ferlan wrote:
--- src/libvirt.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index c236152..4bd6c9a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3184,9 +3184,11 @@ error: * virDomainShutdown: * @domain: a domain object * - * Shutdown a domain, the domain object is still usable thereafter but + * Shutdown a domain, the domain object is still usable thereafter, but * the domain OS is being stopped. Note that the guest OS may ignore the - * request. For guests that react to a shutdown request, the differences + * request. Additionally, the hypervisor may check and support the domain
Can we kill this double space when touching this? I've noticed vim sometimes insert two spaces when re-aligning text, so it's really just a suggestion ...
+ * 'on_poweroff' XML setting resulting in a domain that reboots instead of + * shutting down. For guests that react to a shutdown request, the differences
Here we have exactly one space.
* from virDomainDestroy() are that the guests disk storage will be in a * stable state rather than having the (virtual) power cord pulled, and * this command returns as soon as the shutdown request is issued rather @@ -3241,7 +3243,9 @@ error: * * Shutdown a domain, the domain object is still usable thereafter but * the domain OS is being stopped. Note that the guest OS may ignore the - * request. For guests that react to a shutdown request, the differences + * request. Additionally, the hypervisor may check and support the domain + * 'on_poweroff' XML setting resulting in a domain that reboots instead of + * shutting down. For guests that react to a shutdown request, the differences * from virDomainDestroy() are that the guest's disk storage will be in a * stable state rather than having the (virtual) power cord pulled, and * this command returns as soon as the shutdown request is issued rather @@ -3300,9 +3304,12 @@ error: * @domain: a domain object * @flags: bitwise-OR of virDomainRebootFlagValues * - * Reboot a domain, the domain object is still usable there after but + * Reboot a domain, the domain object is still usable thereafter, but * the domain OS is being stopped for a restart. * Note that the guest OS may ignore the request. + * Additionally, the hypervisor may check and support the domain + * 'on_reboot' XML setting resulting in a domain that shuts down instead + * of rebooting. * * If @flags is set to zero, then the hypervisor will choose the * method of shutdown it considers best. To have greater control
On 05/15/2013 12:02 PM, Michal Privoznik wrote:
On 24.04.2013 13:40, John Ferlan wrote:
--- src/libvirt.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index c236152..4bd6c9a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3184,9 +3184,11 @@ error: * virDomainShutdown: * @domain: a domain object * - * Shutdown a domain, the domain object is still usable thereafter but + * Shutdown a domain, the domain object is still usable thereafter, but * the domain OS is being stopped. Note that the guest OS may ignore the - * request. For guests that react to a shutdown request, the differences + * request. Additionally, the hypervisor may check and support the domain
Can we kill this double space when touching this? I've noticed vim sometimes insert two spaces when re-aligning text, so it's really just a suggestion ...
It's an old style of typing spaces between sentences when using Monospaced font (you haven't played old text games? ;-) ). I don't know about vim, but emacs uses two spaces after the dot when realigning text in case the dot ends the line (you start your next sentence on the next line). However, the double space is perfectly OK in there ;-) Martin
--- src/qemu/qemu_driver.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ba5600d..084412f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1697,6 +1697,8 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) { int ret = -1; qemuDomainObjPrivatePtr priv; bool useAgent = false, agentRequested, acpiRequested; + bool isReboot = false; + int agentFlag = QEMU_AGENT_SHUTDOWN_POWERDOWN; virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN | VIR_DOMAIN_SHUTDOWN_GUEST_AGENT, -1); @@ -1704,6 +1706,13 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) { if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; + if (vm->def->onPoweroff == VIR_DOMAIN_LIFECYCLE_RESTART || + vm->def->onPoweroff == VIR_DOMAIN_LIFECYCLE_RESTART_RENAME) { + isReboot = true; + agentFlag = QEMU_AGENT_SHUTDOWN_REBOOT; + VIR_INFO("Domain on_poweroff setting overridden, attempting reboot"); + } + priv = vm->privateData; agentRequested = flags & VIR_DOMAIN_SHUTDOWN_GUEST_AGENT; acpiRequested = flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN; @@ -1744,7 +1753,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) { if (useAgent) { qemuDomainObjEnterAgent(vm); - ret = qemuAgentShutdown(priv->agent, QEMU_AGENT_SHUTDOWN_POWERDOWN); + ret = qemuAgentShutdown(priv->agent, agentFlag); qemuDomainObjExitAgent(vm); } @@ -1753,7 +1762,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) { */ if (!useAgent || (ret < 0 && (acpiRequested || !flags))) { - qemuDomainSetFakeReboot(driver, vm, false); + qemuDomainSetFakeReboot(driver, vm, isReboot); qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorSystemPowerdown(priv->mon); @@ -1784,6 +1793,8 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) int ret = -1; qemuDomainObjPrivatePtr priv; bool useAgent = false; + bool isReboot = true; + int agentFlag = QEMU_AGENT_SHUTDOWN_REBOOT; virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN | VIR_DOMAIN_REBOOT_GUEST_AGENT , -1); @@ -1799,6 +1810,13 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; + if (vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY || + vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_PRESERVE) { + agentFlag = QEMU_AGENT_SHUTDOWN_POWERDOWN; + isReboot = false; + VIR_INFO("Domain on_reboot setting overridden, shutting down"); + } + priv = vm->privateData; if ((flags & VIR_DOMAIN_REBOOT_GUEST_AGENT) || @@ -1847,7 +1865,7 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) if (useAgent) { qemuDomainObjEnterAgent(vm); - ret = qemuAgentShutdown(priv->agent, QEMU_AGENT_SHUTDOWN_REBOOT); + ret = qemuAgentShutdown(priv->agent, agentFlag); qemuDomainObjExitAgent(vm); } else { qemuDomainObjEnterMonitor(driver, vm); @@ -1855,7 +1873,7 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) qemuDomainObjExitMonitor(driver, vm); if (ret == 0) - qemuDomainSetFakeReboot(driver, vm, true); + qemuDomainSetFakeReboot(driver, vm, isReboot); } endjob: -- 1.8.1.4
--- docs/formatdomain.html.in | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 888c005..2c876ed 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -957,7 +957,13 @@ <p> It is sometimes necessary to override the default actions taken - on various events. + on various events. Not all hypervisors support all events and actions. + The actions may be taken as a result of calls to libvirt APIs + <code class='docref'>virDomainReboot</code>, + <code class='docref'>virDomainShutdown</code>, or + <code class='docref'>virDomainShutdownFlags</code>. + Using <code>virsh reboot</code> or <code>virsh shutdown</code> would + also trigger the event. </p> <pre> @@ -994,20 +1000,28 @@ <dl> <dt><code>destroy</code></dt> <dd>The domain will be terminated completely and all resources - released</dd> + released.</dd> <dt><code>restart</code></dt> - <dd>The domain will be terminated, and then restarted with - the same configuration</dd> + <dd>The domain will be terminated and then restarted with + the same configuration.</dd> <dt><code>preserve</code></dt> - <dd>The domain will be terminated, and its resource preserved + <dd>The domain will be terminated and its resource preserved to allow analysis.</dd> <dt><code>rename-restart</code></dt> - <dd>The domain will be terminated, and then restarted with - a new name</dd> + <dd>The domain will be terminated and then restarted with + a new name.</dd> </dl> <p> - on_crash supports these additional + QEMU/KVM supports the <code>on_poweroff</code> and <code>on_reboot</code> + events handling the <code>destroy</code> and <code>restart</code> actions. + The <code>preserve</code> action for an <code>on_reboot</code> event + is treated as a <code>destroy</code> and the <code>rename-restart</code> + action for an <code>on_poweroff</code> event is treated as a + <code>restart</code> event. + + <p> + The <code>on_crash</code> event supports these additional actions <span class="since">since 0.8.4</span>. </p> -- 1.8.1.4
On 24.04.2013 13:40, John Ferlan wrote:
--- docs/formatdomain.html.in | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 888c005..2c876ed 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -957,7 +957,13 @@
<p> It is sometimes necessary to override the default actions taken - on various events. + on various events. Not all hypervisors support all events and actions. + The actions may be taken as a result of calls to libvirt APIs + <code class='docref'>virDomainReboot</code>, + <code class='docref'>virDomainShutdown</code>, or + <code class='docref'>virDomainShutdownFlags</code>. + Using <code>virsh reboot</code> or <code>virsh shutdown</code> would + also trigger the event. </p>
<pre> @@ -994,20 +1000,28 @@ <dl> <dt><code>destroy</code></dt> <dd>The domain will be terminated completely and all resources - released</dd> + released.</dd> <dt><code>restart</code></dt> - <dd>The domain will be terminated, and then restarted with - the same configuration</dd> + <dd>The domain will be terminated and then restarted with + the same configuration.</dd> <dt><code>preserve</code></dt> - <dd>The domain will be terminated, and its resource preserved + <dd>The domain will be terminated and its resource preserved to allow analysis.</dd> <dt><code>rename-restart</code></dt> - <dd>The domain will be terminated, and then restarted with - a new name</dd> + <dd>The domain will be terminated and then restarted with + a new name.</dd> </dl>
<p> - on_crash supports these additional + QEMU/KVM supports the <code>on_poweroff</code> and <code>on_reboot</code> + events handling the <code>destroy</code> and <code>restart</code> actions. + The <code>preserve</code> action for an <code>on_reboot</code> event + is treated as a <code>destroy</code> and the <code>rename-restart</code> + action for an <code>on_poweroff</code> event is treated as a + <code>restart</code> event. + + <p> + The <code>on_crash</code> event supports these additional actions <span class="since">since 0.8.4</span>. </p>
ACK with this squashed in: diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ef1db73..b118b49 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1029,6 +1029,7 @@ is treated as a <code>destroy</code> and the <code>rename-restart</code> action for an <code>on_poweroff</code> event is treated as a <code>restart</code> event. + </p> <p> The <code>on_crash</code> event supports these additional
On 04/24/2013 07:40 AM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=916052
Domain lifecycle events "on_reboot" and "on_poweroff" translate into QEMU command options "no-reboot" and "no-shutdown" at startup time. The existing code only checked the "on_reboot" setting to determine whether to add the "no-reboot" option even though "on_poweroff" could have been set to allow reboot. Changed the logic to only use "no-reboot" if each of the lifecycle event actions was "destroy". I considered adding "preserve" as well; however, a future preserve action might be able to cull something from a qemu process that doesn't just exit immediately. NB, previously it was possible to have both QEMU command options on the command line, which while not tagged as an error could be considered "odd".
During the shutdown or reboot domain operations, the calls to qemuAgentShutdown and qemuDomainSetFakeReboot did not consider the possibility that the event action for shutdown was reboot or the event action for reboot was destroy. This patch fixes that. NB, I wasn't sure how I could "convey" that to the user though. The virsh reboot will still issue the message "Domain is being rebooted" even though it could be destroyed. Like was virsh shutdown will still issue the message "Domain is being shutdown" even though it is being rebooted. The usage of VIR_INFO was an attempt to do that even though one would have to have the correct debug level set.
Adjusted the documentation in order to describe the possible actions based on the event lifecycle action settings of the domain.
John Ferlan (4): Adjust usage of qemu -no-reboot and -no-shutdown options Adjust comments to describe on_poweroff and on_reboot action Handle the domain event 'on_reboot' and 'on_poweroff' settings docs: Update formatdomain for lifecycle events
docs/formatdomain.html.in | 30 ++++++++++++++++++++++-------- src/libvirt.c | 15 +++++++++++---- src/qemu/qemu_command.c | 17 +++++++++++++---- src/qemu/qemu_driver.c | 26 ++++++++++++++++++++++---- 4 files changed, 68 insertions(+), 20 deletions(-)
ping
On 24.04.2013 13:40, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=916052
Domain lifecycle events "on_reboot" and "on_poweroff" translate into QEMU command options "no-reboot" and "no-shutdown" at startup time. The existing code only checked the "on_reboot" setting to determine whether to add the "no-reboot" option even though "on_poweroff" could have been set to allow reboot. Changed the logic to only use "no-reboot" if each of the lifecycle event actions was "destroy". I considered adding "preserve" as well; however, a future preserve action might be able to cull something from a qemu process that doesn't just exit immediately. NB, previously it was possible to have both QEMU command options on the command line, which while not tagged as an error could be considered "odd".
During the shutdown or reboot domain operations, the calls to qemuAgentShutdown and qemuDomainSetFakeReboot did not consider the possibility that the event action for shutdown was reboot or the event action for reboot was destroy. This patch fixes that. NB, I wasn't sure how I could "convey" that to the user though. The virsh reboot will still issue the message "Domain is being rebooted" even though it could be destroyed. Like was virsh shutdown will still issue the message "Domain is being shutdown" even though it is being rebooted. The usage of VIR_INFO was an attempt to do that even though one would have to have the correct debug level set.
Adjusted the documentation in order to describe the possible actions based on the event lifecycle action settings of the domain.
John Ferlan (4): Adjust usage of qemu -no-reboot and -no-shutdown options Adjust comments to describe on_poweroff and on_reboot action Handle the domain event 'on_reboot' and 'on_poweroff' settings docs: Update formatdomain for lifecycle events
docs/formatdomain.html.in | 30 ++++++++++++++++++++++-------- src/libvirt.c | 15 +++++++++++---- src/qemu/qemu_command.c | 17 +++++++++++++---- src/qemu/qemu_driver.c | 26 ++++++++++++++++++++++---- 4 files changed, 68 insertions(+), 20 deletions(-)
ACK series, but see my comments to 2/4 and 4/4. Michal
On 05/15/2013 06:02 AM, Michal Privoznik wrote:
On 24.04.2013 13:40, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=916052
Domain lifecycle events "on_reboot" and "on_poweroff" translate into QEMU command options "no-reboot" and "no-shutdown" at startup time. The existing code only checked the "on_reboot" setting to determine whether to add the "no-reboot" option even though "on_poweroff" could have been set to allow reboot. Changed the logic to only use "no-reboot" if each of the lifecycle event actions was "destroy". I considered adding "preserve" as well; however, a future preserve action might be able to cull something from a qemu process that doesn't just exit immediately. NB, previously it was possible to have both QEMU command options on the command line, which while not tagged as an error could be considered "odd".
During the shutdown or reboot domain operations, the calls to qemuAgentShutdown and qemuDomainSetFakeReboot did not consider the possibility that the event action for shutdown was reboot or the event action for reboot was destroy. This patch fixes that. NB, I wasn't sure how I could "convey" that to the user though. The virsh reboot will still issue the message "Domain is being rebooted" even though it could be destroyed. Like was virsh shutdown will still issue the message "Domain is being shutdown" even though it is being rebooted. The usage of VIR_INFO was an attempt to do that even though one would have to have the correct debug level set.
Adjusted the documentation in order to describe the possible actions based on the event lifecycle action settings of the domain.
John Ferlan (4): Adjust usage of qemu -no-reboot and -no-shutdown options Adjust comments to describe on_poweroff and on_reboot action Handle the domain event 'on_reboot' and 'on_poweroff' settings docs: Update formatdomain for lifecycle events
docs/formatdomain.html.in | 30 ++++++++++++++++++++++-------- src/libvirt.c | 15 +++++++++++---- src/qemu/qemu_command.c | 17 +++++++++++++---- src/qemu/qemu_driver.c | 26 ++++++++++++++++++++++---- 4 files changed, 68 insertions(+), 20 deletions(-)
ACK series, but see my comments to 2/4 and 4/4.
Michal
Adjusted the comments for 2/4 and 4/4 and pushed. Thanks, John
participants (3)
-
John Ferlan -
Martin Kletzander -
Michal Privoznik