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

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