[libvirt] [PATCH v2] qemu: Honour <on_reboot/>

Michal Privoznik posted 1 patch 6 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/ac121b81273f07179ec76fa6e375b40123104db3.1502894187.git.mprivozn@redhat.com
src/qemu/qemu_process.c | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
[libvirt] [PATCH v2] qemu: Honour <on_reboot/>
Posted by Michal Privoznik 6 years, 7 months ago
https://bugzilla.redhat.com/show_bug.cgi?id=1476866

For some reason, we completely ignore <on_reboot/> setting for
domains. The implementation is simply not there. It never was.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

diff to v1:
- dropped the spoofed logic
- Switch from qemuProcessShutdownOrReboot() to qemuPrcoessStop() because that's
  what <on_crash/> impl does too.

 src/qemu/qemu_process.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fed2bc588..3df6c320e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -484,6 +484,7 @@ qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
     virObjectEventPtr event;
     qemuDomainObjPrivatePtr priv;
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+    int ret = -1;
 
     virObjectLock(vm);
 
@@ -495,12 +496,32 @@ qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
     if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0)
         VIR_WARN("Failed to save status on vm %s", vm->def->name);
 
+    if (vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY ||
+        vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_PRESERVE) {
+
+        if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+            goto cleanup;
+
+        if (!virDomainObjIsActive(vm)) {
+            VIR_DEBUG("Ignoring RESET event from inactive domain %s",
+                      vm->def->name);
+            goto endjob;
+        }
+
+        qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED,
+                        QEMU_ASYNC_JOB_NONE, 0);
+        virDomainAuditStop(vm, "destroyed");
+        qemuDomainRemoveInactive(driver, vm);
+     endjob:
+        qemuDomainObjEndJob(driver, vm);
+    }
+
+    ret = 0;
+ cleanup:
     virObjectUnlock(vm);
-
     qemuDomainEventQueue(driver, event);
-
     virObjectUnref(cfg);
-    return 0;
+    return ret;
 }
 
 
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Honour <on_reboot/>
Posted by Michal Privoznik 6 years, 7 months ago
On 08/16/2017 04:38 PM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1476866
> 
> For some reason, we completely ignore <on_reboot/> setting for
> domains. The implementation is simply not there. It never was.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> diff to v1:
> - dropped the spoofed logic
> - Switch from qemuProcessShutdownOrReboot() to qemuPrcoessStop() because that's
>   what <on_crash/> impl does too.
> 
>  src/qemu/qemu_process.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)

Ping.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Honour <on_reboot/>
Posted by Martin Kletzander 6 years, 7 months ago
On Wed, Aug 16, 2017 at 04:38:09PM +0200, Michal Privoznik wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1476866
>
>For some reason, we completely ignore <on_reboot/> setting for
>domains. The implementation is simply not there. It never was.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
>
>diff to v1:
>- dropped the spoofed logic
>- Switch from qemuProcessShutdownOrReboot() to qemuPrcoessStop() because that's
>  what <on_crash/> impl does too.
>
> src/qemu/qemu_process.c | 27 ++++++++++++++++++++++++---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
>diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>index fed2bc588..3df6c320e 100644
>--- a/src/qemu/qemu_process.c
>+++ b/src/qemu/qemu_process.c
>@@ -484,6 +484,7 @@ qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>     virObjectEventPtr event;
>     qemuDomainObjPrivatePtr priv;
>     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>+    int ret = -1;
>
>     virObjectLock(vm);
>
>@@ -495,12 +496,32 @@ qemuProcessHandleReset(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>     if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0)
>         VIR_WARN("Failed to save status on vm %s", vm->def->name);
>
>+    if (vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY ||
>+        vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_PRESERVE) {
>+
>+        if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>+            goto cleanup;
>+
>+        if (!virDomainObjIsActive(vm)) {
>+            VIR_DEBUG("Ignoring RESET event from inactive domain %s",
>+                      vm->def->name);
>+            goto endjob;
>+        }
>+
>+        qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED,
>+                        QEMU_ASYNC_JOB_NONE, 0);
>+        virDomainAuditStop(vm, "destroyed");

Queuing another event here that the domain is being destroyed seems both
appropriate and weird to me.  So I'll leave it up to you.  It's not like
anyone ever used this functionality... ever.  ACK either way.

>+        qemuDomainRemoveInactive(driver, vm);
>+     endjob:
>+        qemuDomainObjEndJob(driver, vm);
>+    }
>+
>+    ret = 0;
>+ cleanup:
>     virObjectUnlock(vm);
>-
>     qemuDomainEventQueue(driver, event);
>-
>     virObjectUnref(cfg);
>-    return 0;
>+    return ret;
> }
>
>
>--
>2.13.0
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Honour <on_reboot/>
Posted by Michal Privoznik 6 years, 7 months ago
On 08/29/2017 11:08 AM, Martin Kletzander wrote:
> On Wed, Aug 16, 2017 at 04:38:09PM +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1476866
>>
>> For some reason, we completely ignore <on_reboot/> setting for
>> domains. The implementation is simply not there. It never was.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>
>> diff to v1:
>> - dropped the spoofed logic
>> - Switch from qemuProcessShutdownOrReboot() to qemuPrcoessStop()
>> because that's
>>  what <on_crash/> impl does too.
>>
>> src/qemu/qemu_process.c | 27 ++++++++++++++++++++++++---
>> 1 file changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index fed2bc588..3df6c320e 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -484,6 +484,7 @@ qemuProcessHandleReset(qemuMonitorPtr mon
>> ATTRIBUTE_UNUSED,
>>     virObjectEventPtr event;
>>     qemuDomainObjPrivatePtr priv;
>>     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> +    int ret = -1;
>>
>>     virObjectLock(vm);
>>
>> @@ -495,12 +496,32 @@ qemuProcessHandleReset(qemuMonitorPtr mon
>> ATTRIBUTE_UNUSED,
>>     if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
>> driver->caps) < 0)
>>         VIR_WARN("Failed to save status on vm %s", vm->def->name);
>>
>> +    if (vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY ||
>> +        vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_PRESERVE) {
>> +
>> +        if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>> +            goto cleanup;
>> +
>> +        if (!virDomainObjIsActive(vm)) {
>> +            VIR_DEBUG("Ignoring RESET event from inactive domain %s",
>> +                      vm->def->name);
>> +            goto endjob;
>> +        }
>> +
>> +        qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED,
>> +                        QEMU_ASYNC_JOB_NONE, 0);
>> +        virDomainAuditStop(vm, "destroyed");
> 
> Queuing another event here that the domain is being destroyed seems both
> appropriate and weird to me.  So I'll leave it up to you.  It's not like
> anyone ever used this functionality... ever.  ACK either way.

I think we want to queue the event here. Imagine that there's a mgmt app
that acts like HA daemon. Whenever a domain is destroyed it has to start
it up again. Well, with the current code it has to listen to RESET event
and depending on libvirt version it has to either ignore the event or
start it up again. However, if we queue the event all that the app has
to do is to listen to DESTROY event. Therefore I'm inclined to leave it
here.

Pushed, thanks.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Honour <on_reboot/>
Posted by Martin Kletzander 6 years, 7 months ago
On Tue, Aug 29, 2017 at 12:46:43PM +0200, Michal Privoznik wrote:
>On 08/29/2017 11:08 AM, Martin Kletzander wrote:
>> On Wed, Aug 16, 2017 at 04:38:09PM +0200, Michal Privoznik wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1476866
>>>
>>> For some reason, we completely ignore <on_reboot/> setting for
>>> domains. The implementation is simply not there. It never was.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>
>>> diff to v1:
>>> - dropped the spoofed logic
>>> - Switch from qemuProcessShutdownOrReboot() to qemuPrcoessStop()
>>> because that's
>>>  what <on_crash/> impl does too.
>>>
>>> src/qemu/qemu_process.c | 27 ++++++++++++++++++++++++---
>>> 1 file changed, 24 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index fed2bc588..3df6c320e 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -484,6 +484,7 @@ qemuProcessHandleReset(qemuMonitorPtr mon
>>> ATTRIBUTE_UNUSED,
>>>     virObjectEventPtr event;
>>>     qemuDomainObjPrivatePtr priv;
>>>     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>> +    int ret = -1;
>>>
>>>     virObjectLock(vm);
>>>
>>> @@ -495,12 +496,32 @@ qemuProcessHandleReset(qemuMonitorPtr mon
>>> ATTRIBUTE_UNUSED,
>>>     if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
>>> driver->caps) < 0)
>>>         VIR_WARN("Failed to save status on vm %s", vm->def->name);
>>>
>>> +    if (vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY ||
>>> +        vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_PRESERVE) {
>>> +
>>> +        if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>>> +            goto cleanup;
>>> +
>>> +        if (!virDomainObjIsActive(vm)) {
>>> +            VIR_DEBUG("Ignoring RESET event from inactive domain %s",
>>> +                      vm->def->name);
>>> +            goto endjob;
>>> +        }
>>> +
>>> +        qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED,
>>> +                        QEMU_ASYNC_JOB_NONE, 0);
>>> +        virDomainAuditStop(vm, "destroyed");
>>
>> Queuing another event here that the domain is being destroyed seems both
>> appropriate and weird to me.  So I'll leave it up to you.  It's not like
>> anyone ever used this functionality... ever.  ACK either way.
>
>I think we want to queue the event here. Imagine that there's a mgmt app
>that acts like HA daemon. Whenever a domain is destroyed it has to start
>it up again. Well, with the current code it has to listen to RESET event
>and depending on libvirt version it has to either ignore the event or
>start it up again. However, if we queue the event all that the app has
>to do is to listen to DESTROY event. Therefore I'm inclined to leave it
>here.
>

Leave?  I was talking about adding it.  I don't see it here.

>Pushed, thanks.
>
>Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Honour <on_reboot/>
Posted by Michal Privoznik 6 years, 7 months ago
On 08/29/2017 12:57 PM, Martin Kletzander wrote:
> On Tue, Aug 29, 2017 at 12:46:43PM +0200, Michal Privoznik wrote:
>> On 08/29/2017 11:08 AM, Martin Kletzander wrote:
>>> On Wed, Aug 16, 2017 at 04:38:09PM +0200, Michal Privoznik wrote:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1476866
>>>>
>>>> For some reason, we completely ignore <on_reboot/> setting for
>>>> domains. The implementation is simply not there. It never was.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>
>>>> diff to v1:
>>>> - dropped the spoofed logic
>>>> - Switch from qemuProcessShutdownOrReboot() to qemuPrcoessStop()
>>>> because that's
>>>>  what <on_crash/> impl does too.
>>>>
>>>> src/qemu/qemu_process.c | 27 ++++++++++++++++++++++++---
>>>> 1 file changed, 24 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>>> index fed2bc588..3df6c320e 100644
>>>> --- a/src/qemu/qemu_process.c
>>>> +++ b/src/qemu/qemu_process.c
>>>> @@ -484,6 +484,7 @@ qemuProcessHandleReset(qemuMonitorPtr mon
>>>> ATTRIBUTE_UNUSED,
>>>>     virObjectEventPtr event;
>>>>     qemuDomainObjPrivatePtr priv;
>>>>     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>>>> +    int ret = -1;
>>>>
>>>>     virObjectLock(vm);
>>>>
>>>> @@ -495,12 +496,32 @@ qemuProcessHandleReset(qemuMonitorPtr mon
>>>> ATTRIBUTE_UNUSED,
>>>>     if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm,
>>>> driver->caps) < 0)
>>>>         VIR_WARN("Failed to save status on vm %s", vm->def->name);
>>>>
>>>> +    if (vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_DESTROY ||
>>>> +        vm->def->onReboot == VIR_DOMAIN_LIFECYCLE_PRESERVE) {
>>>> +
>>>> +        if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>>>> +            goto cleanup;
>>>> +
>>>> +        if (!virDomainObjIsActive(vm)) {
>>>> +            VIR_DEBUG("Ignoring RESET event from inactive domain %s",
>>>> +                      vm->def->name);
>>>> +            goto endjob;
>>>> +        }
>>>> +
>>>> +        qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED,
>>>> +                        QEMU_ASYNC_JOB_NONE, 0);
>>>> +        virDomainAuditStop(vm, "destroyed");
>>>
>>> Queuing another event here that the domain is being destroyed seems both
>>> appropriate and weird to me.  So I'll leave it up to you.  It's not like
>>> anyone ever used this functionality... ever.  ACK either way.
>>
>> I think we want to queue the event here. Imagine that there's a mgmt app
>> that acts like HA daemon. Whenever a domain is destroyed it has to start
>> it up again. Well, with the current code it has to listen to RESET event
>> and depending on libvirt version it has to either ignore the event or
>> start it up again. However, if we queue the event all that the app has
>> to do is to listen to DESTROY event. Therefore I'm inclined to leave it
>> here.
>>
> 
> Leave?  I was talking about adding it.  I don't see it here.

Oh, I though that qemuProcessStop does queue an event. But it doesn't. I
shouldn't reply to e-mails right after having lunch. I'm going to post a
patch that does enqueue the event. Stay tuned.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list