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

[Daniel P. Berrangé]

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 at 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 :|