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