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

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(a)virtuozzo.com&gt; >>>>> --- >>>>> 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.