<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jan 28, 2014 at 8:05 AM, Daniel P. Berrange <span dir="ltr"><<a href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Thu, Jan 23, 2014 at 03:06:16PM -0500, Adam Walters wrote:<br>
> This patchset implements a tiered driver loading system. I split the hypervisor<br>
> drivers out into their own tier, which is loaded after the other drivers. This<br>
> has the net effect of ensuring that things like secrets, networks, etc., are<br>
> initialized and auto-started before any hypervisors, such as QEMU, LXC, etc.<br>
> This resolves the race condition currently present when starting libvirtd<br>
> while domains are running, which happens when restarting libvirtd after having<br>
> started at least one domain.<br>
<br>
</div>[snip]<br>
<div class="im"><br>
> For anyone who is not familiar with the race condition I mention above, the<br>
> basic description is that upon restarting libvirtd, any running QEMU domains<br>
> using storage volumes are killed randomly due to their associated storage pool<br>
> not yet being online. This is due to storage pool auto-start not having<br>
> completed prior to QEMU initialization. In my prior testing, I found that this<br>
> race condition affected at least one domain approximately 40% of the time. I<br>
> sent this information to the mailing list back on 06DEC2013, if anyone is<br>
> interested in going back and re-reading my description.<br>
<br>
</div>Ok, so the current sequence is /supposed/ to be<br>
<br>
1 StateInitialize<br>
1.1 StorageInitialize<br>
1.1.1 Load all configs<br>
1.1.2 Detect currently active pools<br>
1.2 QEMUInitialize<br>
1.2.1 Load all configs<br>
1.2.2 Detect currently active guests<br>
2 StateAutostart<br>
2.1 StorageAutostart<br>
2.1.1 Start any inactive pools marked autostart<br>
2.2 QEMUAutostart<br>
2.2.1 Start any inactive guests marked autostart<br>
<br>
<br>
Looking at the storage driver code though, I see a flaw in that<br>
1.1.2 does not actually exist. The checking for existing active<br>
storage pools is only done in step 2.1.1 as part of autostart,<br>
which as you say is clearly too late.<br>
<br>
Also the checking for existing active storage pools looks like<br>
it only works for storage pools which actually have some persistent<br>
kernel / userspace state outside of libvirt. ie iSCSI pools will<br>
remain active even when libvirt is not running, since the iSCSI<br>
initiator is outside scope of libvirt. If we have a RBD pool,<br>
however, which is using librbd instead of the FUSE driver there's<br>
no persistent state to detect. The issue here is that the storage<br>
driver is not correctly keeping track of what storage pools were<br>
active prior to restart. ie any active storage pools before restart,<br>
should still be active after restart, *regardless* of the autostart<br>
flag.<br>
<br>
So I understand why your proposed split of driver loading works<br>
to solve this, but even with this it only solves the startup<br>
ordering problem. There's still the issue that we're reliant on<br>
the autostart flag being set. If there's a guest with an RBD<br>
volume against a pool that isn't set to autostart, we're still<br>
doomed.<br></blockquote><div><br></div><div>I hadn't really thought about that piece, as I always set my storage pools</div><div>to auto-start. I'm still not sure that a race condition would not still exist if</div>
<div>the storage driver continues to utilize a hard-coded connection to 'qemu:///',</div><div>though. I'm not particularly familiar enough with the libvirt code to know</div><div>exactly at which point the QEMU driver can accept a connection to its URI</div>
<div>and facilitate access to information about networks, secrets, etc.</div><div><br></div><div>Also note that I was also able to reproduce the race condition with a file-backed</div><div>storage pool, too. So even taking out the semi-exotic RBD piece, I think there</div>
<div>would still be problems if 1.1.2 were added into the code. I could certainly look</div><div>into implementing something to store state information for storage pools. I'll likely</div><div>look toward existing domain code for an idea of how to implement it.</div>
<div><br></div><div>That issue does sort of bleed into my other patchset that implements the</div><div>'config:///' URI, which we had some discussion over prior to the fixup and</div><div>resubmit.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The storage drivers are just plain broken wrt libvirt restarts<br>
because they're not correctly recording their state and restoring<br>
back to the exact same state after restart.<br>
<span class="HOEnZb"><font color="#888888"><br>
Daniel<br>
--<br>
|: <a href="http://berrange.com" target="_blank">http://berrange.com</a> -o- <a href="http://www.flickr.com/photos/dberrange/" target="_blank">http://www.flickr.com/photos/dberrange/</a> :|<br>
|: <a href="http://libvirt.org" target="_blank">http://libvirt.org</a> -o- <a href="http://virt-manager.org" target="_blank">http://virt-manager.org</a> :|<br>
|: <a href="http://autobuild.org" target="_blank">http://autobuild.org</a> -o- <a href="http://search.cpan.org/~danberr/" target="_blank">http://search.cpan.org/~danberr/</a> :|<br>
|: <a href="http://entangle-photo.org" target="_blank">http://entangle-photo.org</a> -o- <a href="http://live.gnome.org/gtk-vnc" target="_blank">http://live.gnome.org/gtk-vnc</a> :|<br>
</font></span></blockquote></div><br></div></div>