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@xxxxxxxxxxxxx>
> >>>>> ---
> >>>>> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
