1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemu: don't fail on destroy if domain is inactive

Nikolay Shirokovskiy posted 1 patch 5 years, 8 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1553758141-450597-1-git-send-email-nshirokovskiy@virtuozzo.com
src/qemu/qemu_driver.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
[libvirt] [PATCH] qemu: don't fail on destroy if domain is inactive
Posted by Nikolay Shirokovskiy 5 years, 8 months ago 
Mgmt can not track if domain is already inactive before
calling destroy because domain can become inactive because
of crash/shutdown from guest. Thus it is make sense to
report success in this case. Another option is to return
special error code but this is a bit more complicated.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---
 src/qemu/qemu_driver.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 62d8d97..0789efc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2172,8 +2172,10 @@ qemuDomainDestroyFlags(virDomainPtr dom,
     if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0)
         goto cleanup;
 
-    if (virDomainObjCheckActive(vm) < 0)
+    if (!virDomainObjIsActive(vm)) {
+        ret = 0;
         goto cleanup;
+    }
 
     state = virDomainObjGetState(vm, &reason);
     starting = (state == VIR_DOMAIN_PAUSED &&
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't fail on destroy if domain is inactive
Posted by Peter Krempa 5 years, 8 months ago 
On Thu, Mar 28, 2019 at 10:29:01 +0300, Nikolay Shirokovskiy wrote:
> Mgmt can not track if domain is already inactive before
> calling destroy because domain can become inactive because
> of crash/shutdown from guest. Thus it is make sense to

Well mgmt apps can use events emitted by libvirt precisely for this
case.

> report success in this case. Another option is to return
> special error code but this is a bit more complicated.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
>  src/qemu/qemu_driver.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 62d8d97..0789efc 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2172,8 +2172,10 @@ qemuDomainDestroyFlags(virDomainPtr dom,
>      if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0)
>          goto cleanup;
>  
> -    if (virDomainObjCheckActive(vm) < 0)
> +    if (!virDomainObjIsActive(vm)) {
> +        ret = 0;
>          goto cleanup;
> +    }

I'm not persuaded we want this. The commit message does not provide
enough means to justify it. Every other API we have returns error in
case when the domain is in the state the API will change it to so I'm
not in favor of making this api behave differently.

NACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't fail on destroy if domain is inactive
Posted by Nikolay Shirokovskiy 5 years, 8 months ago 

On 28.03.2019 11:27, Peter Krempa wrote:
> On Thu, Mar 28, 2019 at 10:29:01 +0300, Nikolay Shirokovskiy wrote:
>> Mgmt can not track if domain is already inactive before
>> calling destroy because domain can become inactive because
>> of crash/shutdown from guest. Thus it is make sense to
> 
> Well mgmt apps can use events emitted by libvirt precisely for this
> case.

This is still racy.

> 
>> report success in this case. Another option is to return
>> special error code but this is a bit more complicated.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>> ---
>>  src/qemu/qemu_driver.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 62d8d97..0789efc 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -2172,8 +2172,10 @@ qemuDomainDestroyFlags(virDomainPtr dom,
>>      if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0)
>>          goto cleanup;
>>  
>> -    if (virDomainObjCheckActive(vm) < 0)
>> +    if (!virDomainObjIsActive(vm)) {
>> +        ret = 0;
>>          goto cleanup;
>> +    }
> 
> I'm not persuaded we want this. The commit message does not provide
> enough means to justify it. Every other API we have returns error in
> case when the domain is in the state the API will change it to so I'm
> not in favor of making this api behave differently.
> 

Ok then here is the usecase. We want to shutdown domain and unfortunately
this operation failed to bring domain to shutoff state in time. Thus mgmt try
to call destroy as it wants domain to be shutoff. Destroy returns quite
general VIR_ERR_OPERATION_INVALID error code so mgmt need to face
the problem but in reality everything is ok.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't fail on destroy if domain is inactive
Posted by Peter Krempa 5 years, 8 months ago 
On Thu, Mar 28, 2019 at 08:43:46 +0000, Nikolay Shirokovskiy wrote:
> 
> 
> On 28.03.2019 11:27, Peter Krempa wrote:
> > On Thu, Mar 28, 2019 at 10:29:01 +0300, Nikolay Shirokovskiy wrote:
> >> Mgmt can not track if domain is already inactive before
> >> calling destroy because domain can become inactive because
> >> of crash/shutdown from guest. Thus it is make sense to
> > 
> > Well mgmt apps can use events emitted by libvirt precisely for this
> > case.
> 
> This is still racy.
> 
> > 
> >> report success in this case. Another option is to return
> >> special error code but this is a bit more complicated.
> >>
> >> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> >> ---
> >>  src/qemu/qemu_driver.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >> index 62d8d97..0789efc 100644
> >> --- a/src/qemu/qemu_driver.c
> >> +++ b/src/qemu/qemu_driver.c
> >> @@ -2172,8 +2172,10 @@ qemuDomainDestroyFlags(virDomainPtr dom,
> >>      if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0)
> >>          goto cleanup;
> >>  
> >> -    if (virDomainObjCheckActive(vm) < 0)
> >> +    if (!virDomainObjIsActive(vm)) {
> >> +        ret = 0;
> >>          goto cleanup;
> >> +    }
> > 
> > I'm not persuaded we want this. The commit message does not provide
> > enough means to justify it. Every other API we have returns error in
> > case when the domain is in the state the API will change it to so I'm
> > not in favor of making this api behave differently.
> > 
> 
> Ok then here is the usecase. We want to shutdown domain and unfortunately
> this operation failed to bring domain to shutoff state in time. Thus mgmt try
> to call destroy as it wants domain to be shutoff. Destroy returns quite
> general VIR_ERR_OPERATION_INVALID error code so mgmt need to face
> the problem but in reality everything is ok.

I understand the problem here, but I disagree that the API should return
success if it didn't do anything when it previously was returning
errors.

You can choose to implement a new error code to be used instead of
VIR_ERR_OPERATION_INVALID in virDomainObjCheckActive. E.g.
VIR_ERR_OBJECT_INACTIVE (to be generic enough to work with
networks/storage pools/etc.)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't fail on destroy if domain is inactive
Posted by Daniel P. Berrangé 5 years, 8 months ago 
On Thu, Mar 28, 2019 at 09:53:08AM +0100, Peter Krempa wrote:
> On Thu, Mar 28, 2019 at 08:43:46 +0000, Nikolay Shirokovskiy wrote:
> > 
> > 
> > On 28.03.2019 11:27, Peter Krempa wrote:
> > > On Thu, Mar 28, 2019 at 10:29:01 +0300, Nikolay Shirokovskiy wrote:
> > >> Mgmt can not track if domain is already inactive before
> > >> calling destroy because domain can become inactive because
> > >> of crash/shutdown from guest. Thus it is make sense to
> > > 
> > > Well mgmt apps can use events emitted by libvirt precisely for this
> > > case.
> > 
> > This is still racy.
> > 
> > > 
> > >> report success in this case. Another option is to return
> > >> special error code but this is a bit more complicated.
> > >>
> > >> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> > >> ---
> > >>  src/qemu/qemu_driver.c | 4 +++-
> > >>  1 file changed, 3 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > >> index 62d8d97..0789efc 100644
> > >> --- a/src/qemu/qemu_driver.c
> > >> +++ b/src/qemu/qemu_driver.c
> > >> @@ -2172,8 +2172,10 @@ qemuDomainDestroyFlags(virDomainPtr dom,
> > >>      if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0)
> > >>          goto cleanup;
> > >>  
> > >> -    if (virDomainObjCheckActive(vm) < 0)
> > >> +    if (!virDomainObjIsActive(vm)) {
> > >> +        ret = 0;
> > >>          goto cleanup;
> > >> +    }
> > > 
> > > I'm not persuaded we want this. The commit message does not provide
> > > enough means to justify it. Every other API we have returns error in
> > > case when the domain is in the state the API will change it to so I'm
> > > not in favor of making this api behave differently.
> > > 
> > 
> > Ok then here is the usecase. We want to shutdown domain and unfortunately
> > this operation failed to bring domain to shutoff state in time. Thus mgmt try
> > to call destroy as it wants domain to be shutoff. Destroy returns quite
> > general VIR_ERR_OPERATION_INVALID error code so mgmt need to face
> > the problem but in reality everything is ok.
> 
> I understand the problem here, but I disagree that the API should return
> success if it didn't do anything when it previously was returning
> errors.
> 
> You can choose to implement a new error code to be used instead of
> VIR_ERR_OPERATION_INVALID in virDomainObjCheckActive. E.g.
> VIR_ERR_OBJECT_INACTIVE (to be generic enough to work with
> networks/storage pools/etc.)

Why can't the mgmt app simply ignore the existing OPERATION_INVALID
error they get from destroy.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't fail on destroy if domain is inactive
Posted by Nikolay Shirokovskiy 5 years, 8 months ago 

On 28.03.2019 12:44, Daniel P. Berrangé wrote:
> On Thu, Mar 28, 2019 at 09:53:08AM +0100, Peter Krempa wrote:
>> On Thu, Mar 28, 2019 at 08:43:46 +0000, Nikolay Shirokovskiy wrote:
>>>
>>>
>>> On 28.03.2019 11:27, Peter Krempa wrote:
>>>> On Thu, Mar 28, 2019 at 10:29:01 +0300, Nikolay Shirokovskiy wrote:
>>>>> Mgmt can not track if domain is already inactive before
>>>>> calling destroy because domain can become inactive because
>>>>> of crash/shutdown from guest. Thus it is make sense to
>>>>
>>>> Well mgmt apps can use events emitted by libvirt precisely for this
>>>> case.
>>>
>>> This is still racy.
>>>
>>>>
>>>>> report success in this case. Another option is to return
>>>>> special error code but this is a bit more complicated.
>>>>>
>>>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>>>>> ---
>>>>>  src/qemu/qemu_driver.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>>> index 62d8d97..0789efc 100644
>>>>> --- a/src/qemu/qemu_driver.c
>>>>> +++ b/src/qemu/qemu_driver.c
>>>>> @@ -2172,8 +2172,10 @@ qemuDomainDestroyFlags(virDomainPtr dom,
>>>>>      if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0)
>>>>>          goto cleanup;
>>>>>  
>>>>> -    if (virDomainObjCheckActive(vm) < 0)
>>>>> +    if (!virDomainObjIsActive(vm)) {
>>>>> +        ret = 0;
>>>>>          goto cleanup;
>>>>> +    }
>>>>
>>>> I'm not persuaded we want this. The commit message does not provide
>>>> enough means to justify it. Every other API we have returns error in
>>>> case when the domain is in the state the API will change it to so I'm
>>>> not in favor of making this api behave differently.
>>>>
>>>
>>> Ok then here is the usecase. We want to shutdown domain and unfortunately
>>> this operation failed to bring domain to shutoff state in time. Thus mgmt try
>>> to call destroy as it wants domain to be shutoff. Destroy returns quite
>>> general VIR_ERR_OPERATION_INVALID error code so mgmt need to face
>>> the problem but in reality everything is ok.
>>
>> I understand the problem here, but I disagree that the API should return
>> success if it didn't do anything when it previously was returning
>> errors.
>>
>> You can choose to implement a new error code to be used instead of
>> VIR_ERR_OPERATION_INVALID in virDomainObjCheckActive. E.g.
>> VIR_ERR_OBJECT_INACTIVE (to be generic enough to work with
>> networks/storage pools/etc.)
> 
> Why can't the mgmt app simply ignore the existing OPERATION_INVALID
> error they get from destroy.
> 

Looks inactive domain is the only reason for OPERATION_INVALID right now.
Still it sounds very general for mgmt to ignore.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't fail on destroy if domain is inactive
Posted by Daniel P. Berrangé 5 years, 8 months ago 
On Thu, Mar 28, 2019 at 09:53:03AM +0000, Nikolay Shirokovskiy wrote:
> 
> 
> On 28.03.2019 12:44, Daniel P. Berrangé wrote:
> > On Thu, Mar 28, 2019 at 09:53:08AM +0100, Peter Krempa wrote:
> >> On Thu, Mar 28, 2019 at 08:43:46 +0000, Nikolay Shirokovskiy wrote:
> >>>
> >>>
> >>> On 28.03.2019 11:27, Peter Krempa wrote:
> >>>> On Thu, Mar 28, 2019 at 10:29:01 +0300, Nikolay Shirokovskiy wrote:
> >>>>> Mgmt can not track if domain is already inactive before
> >>>>> calling destroy because domain can become inactive because
> >>>>> of crash/shutdown from guest. Thus it is make sense to
> >>>>
> >>>> Well mgmt apps can use events emitted by libvirt precisely for this
> >>>> case.
> >>>
> >>> This is still racy.
> >>>
> >>>>
> >>>>> report success in this case. Another option is to return
> >>>>> special error code but this is a bit more complicated.
> >>>>>
> >>>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> >>>>> ---
> >>>>>  src/qemu/qemu_driver.c | 4 +++-
> >>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >>>>> index 62d8d97..0789efc 100644
> >>>>> --- a/src/qemu/qemu_driver.c
> >>>>> +++ b/src/qemu/qemu_driver.c
> >>>>> @@ -2172,8 +2172,10 @@ qemuDomainDestroyFlags(virDomainPtr dom,
> >>>>>      if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0)
> >>>>>          goto cleanup;
> >>>>>  
> >>>>> -    if (virDomainObjCheckActive(vm) < 0)
> >>>>> +    if (!virDomainObjIsActive(vm)) {
> >>>>> +        ret = 0;
> >>>>>          goto cleanup;
> >>>>> +    }
> >>>>
> >>>> I'm not persuaded we want this. The commit message does not provide
> >>>> enough means to justify it. Every other API we have returns error in
> >>>> case when the domain is in the state the API will change it to so I'm
> >>>> not in favor of making this api behave differently.
> >>>>
> >>>
> >>> Ok then here is the usecase. We want to shutdown domain and unfortunately
> >>> this operation failed to bring domain to shutoff state in time. Thus mgmt try
> >>> to call destroy as it wants domain to be shutoff. Destroy returns quite
> >>> general VIR_ERR_OPERATION_INVALID error code so mgmt need to face
> >>> the problem but in reality everything is ok.
> >>
> >> I understand the problem here, but I disagree that the API should return
> >> success if it didn't do anything when it previously was returning
> >> errors.
> >>
> >> You can choose to implement a new error code to be used instead of
> >> VIR_ERR_OPERATION_INVALID in virDomainObjCheckActive. E.g.
> >> VIR_ERR_OBJECT_INACTIVE (to be generic enough to work with
> >> networks/storage pools/etc.)
> > 
> > Why can't the mgmt app simply ignore the existing OPERATION_INVALID
> > error they get from destroy.
> > 
> 
> Looks inactive domain is the only reason for OPERATION_INVALID right now.
> Still it sounds very general for mgmt to ignore.

OPERATION_INVALID is a code that is intended to be reported when a guest
is in the wrong running state. So if the operation requires a running
guest & it is shutoff, or if the operation requires an inactive guest
and it is running.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: don't fail on destroy if domain is inactive
Posted by Nikolay Shirokovskiy 5 years, 8 months ago 

On 28.03.2019 12:56, Daniel P. Berrangé wrote:
> On Thu, Mar 28, 2019 at 09:53:03AM +0000, Nikolay Shirokovskiy wrote:
>>
>>
>> On 28.03.2019 12:44, Daniel P. Berrangé wrote:
>>> On Thu, Mar 28, 2019 at 09:53:08AM +0100, Peter Krempa wrote:
>>>> On Thu, Mar 28, 2019 at 08:43:46 +0000, Nikolay Shirokovskiy wrote:
>>>>>
>>>>>
>>>>> On 28.03.2019 11:27, Peter Krempa wrote:
>>>>>> On Thu, Mar 28, 2019 at 10:29:01 +0300, Nikolay Shirokovskiy wrote:
>>>>>>> Mgmt can not track if domain is already inactive before
>>>>>>> calling destroy because domain can become inactive because
>>>>>>> of crash/shutdown from guest. Thus it is make sense to
>>>>>>
>>>>>> Well mgmt apps can use events emitted by libvirt precisely for this
>>>>>> case.
>>>>>
>>>>> This is still racy.
>>>>>
>>>>>>
>>>>>>> report success in this case. Another option is to return
>>>>>>> special error code but this is a bit more complicated.
>>>>>>>
>>>>>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>>>>>>> ---
>>>>>>>  src/qemu/qemu_driver.c | 4 +++-
>>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>>>>> index 62d8d97..0789efc 100644
>>>>>>> --- a/src/qemu/qemu_driver.c
>>>>>>> +++ b/src/qemu/qemu_driver.c
>>>>>>> @@ -2172,8 +2172,10 @@ qemuDomainDestroyFlags(virDomainPtr dom,
>>>>>>>      if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0)
>>>>>>>          goto cleanup;
>>>>>>>  
>>>>>>> -    if (virDomainObjCheckActive(vm) < 0)
>>>>>>> +    if (!virDomainObjIsActive(vm)) {
>>>>>>> +        ret = 0;
>>>>>>>          goto cleanup;
>>>>>>> +    }
>>>>>>
>>>>>> I'm not persuaded we want this. The commit message does not provide
>>>>>> enough means to justify it. Every other API we have returns error in
>>>>>> case when the domain is in the state the API will change it to so I'm
>>>>>> not in favor of making this api behave differently.
>>>>>>
>>>>>
>>>>> Ok then here is the usecase. We want to shutdown domain and unfortunately
>>>>> this operation failed to bring domain to shutoff state in time. Thus mgmt try
>>>>> to call destroy as it wants domain to be shutoff. Destroy returns quite
>>>>> general VIR_ERR_OPERATION_INVALID error code so mgmt need to face
>>>>> the problem but in reality everything is ok.
>>>>
>>>> I understand the problem here, but I disagree that the API should return
>>>> success if it didn't do anything when it previously was returning
>>>> errors.
>>>>
>>>> You can choose to implement a new error code to be used instead of
>>>> VIR_ERR_OPERATION_INVALID in virDomainObjCheckActive. E.g.
>>>> VIR_ERR_OBJECT_INACTIVE (to be generic enough to work with
>>>> networks/storage pools/etc.)
>>>
>>> Why can't the mgmt app simply ignore the existing OPERATION_INVALID
>>> error they get from destroy.
>>>
>>
>> Looks inactive domain is the only reason for OPERATION_INVALID right now.
>> Still it sounds very general for mgmt to ignore.
> 
> OPERATION_INVALID is a code that is intended to be reported when a guest
> is in the wrong running state. So if the operation requires a running
> guest & it is shutoff, or if the operation requires an inactive guest
> and it is running.
> 

There are a a lot of cases in qemu_driver.c when this code is reported when
operation is not applicable for current general domain state/configuration, not
only running state. We can specify invalid state restriction for destroy
operation I guess...

Nikolay

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.