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

[Nikolay Shirokovskiy]

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.

Nikolay