[libvirt] [PATCH RESEND] qemu: Remove inactive vm when failed tostart it

[John Ferlan]

On 07/12/2017 07:34 AM, wang.yi59 at 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 :)
> 
>