Re: [libvirt] [PATCH RESEND] qemu: Remove inactive vm when failed tostart it
On 07/12/2017 07:34 AM, wang.yi59(a)zte.com.cn wrote:
Hi John,
Thanks for your review!
Somehow your response is out of synch with the rest of the series -
things like this get lost very quickly.
> This seems to be a strange sequence of operations, but the claim
is that
> by adding this logic to CreateWithFlags, then the problem you're facing
> is resolved. However, is adding this to the Create logic the right thing
> to do?
> IIUC:
This condition is rare, so I designed some operations which can help us
to understand what happened:
# virsh define win7 -> successful
# virsh start win7 &; sleep 0.2; virsh undefine win7 -> start failed and
undefine successful
and we may see libvirtd output such logs like this:
qemuDomainDefineXMLFlags:7386 : Creating domain win7
qemuProcessStart:6086 : conn=....
qemuDomainUndefineFlags:7501 : Undefining domain win7
error : qemuMonitorOpenUnix:379 : failed to connect to monitor socket
Libvirtd unlocked vm during qemuProcessStart, so qemuDomainUndefineFlags
can get the lock and undefine vm successfully.
Can you be a bit more specific. Do you mean during qemuConnectMonitor
processing during ProcessLaunch? Or perhaps Agent processing?
In any case, perhaps then the better fix is to prohibit undefine whilst
unlocked for Monitor or Agent start. The perhaps interesting question is
where to set flag. Setting/clearing just around Monitor/Agent startup
would be a seemingly short window and perhaps where you're seeing the
virObjectUnlock; however, I wonder if perhaps all of qemuProcessLaunch
should be "protected" as there's some really critical things happening
in there. Thus at the top one sets a boolean "obj->starting = true" and
at cleanup "obj->starting = false".
In qemuDomainUndefineFlags, there'd need to be a similar check to the if
(!persistent) along the lines of "if (starting)" then an error message
indicating that the domain is starting up and cannot be undefined.
Perhaps similarly for Destroy just to be safe.
I didn't think all that long about it, but hopefully it's enough to
perhaps have to generate more patches... I don't believe with that it'd
be possible to have the need to qemuDomainRemoveInactive in the
CreateWithFlags, but I could be wrong.
John
After finishing the above steps, we can get something wrong:
# virsh list --all
Id Name State
----------------------------------------------------
- win7 shut off
# virsh undefine win7
error: Failed to undefine domain win7
error: Requested operation is not valid: cannot undefine transient domain
> There's two reasons to get to the endjob: label...
> #1 virDomainObjIsActive (domain already active)
> #2 qemuDomainObjStart fails
> So this certainly wouldn't be right for the IsActive condition, under
> "normal" circumstances. And again, I don't think requiring a
subsequent
> call to Start would be "correct" either. I am curious though if when in
> this condition, is the vm->def->id set? IOW: What does 'virsh list'
> indicate or even virsh dominfo $domain?
What you said sounds reasonable, I can call qemuDomainObjStart at once
after qemuDomainObjStart fails like libvirt does in qemuDomainCreateXML,
but end job should be called before qemuDomainRemoveInactive :)