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

[Nikolay Shirokovskiy]

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 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.
> 

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