<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jan 20, 2014 at 11:27 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On Fri, Dec 20, 2013 at 11:03:53PM -0500, Adam Walters wrote:<br>

> This patchset adds a driver named 'config' that allows access to configuration<br>
> data, such as secret and storage definitions. This is a pre-requisite for my<br>
> next patchset which resolves the race condition on libvirtd startup and the<br>
> circular dependencies between QEMU and the storage driver.<br>
<br>
</div>I vaguely recall something being mentioned in the past, but not the<br>
details. Can you explain the details of the circular dependancy<br>
problem that you're currently facing ?<br></blockquote><div><br></div><div><div>Peter's description is nearly complete. I have actually reproduced the issue even with file-backed volumes.</div><div>Only when using the disk type of volume, though. There had been a thread on the list late last year where I</div>
<div>brought up the problem, but the discussion sort of died off. I then worked on the patches here to resolve the</div><div>issue. This certainly isn't the only method of resolution, but merely the best one that I could think of and implement</div>
<div>on my own.</div><div><br></div><div>In more detail, there are actually two problems (and thus the two patchsets I submitted) that I discovered in libvirt.</div><div><br></div><div>The first is the unpredictable driver load order.</div>
<div><br></div><div>In this issue, all of the VMs I ran (all were QEMU-based, using either RBD- or file-backed storage pool volumes) were</div><div>randomly restarted due to a race condition in libvirt. QEMU and the storage driver are both starting simultaneously.</div>
<div>If QEMU manages to start before storage, the domains get restarted due to the storage pool being offline (they are</div><div>all offline initially). The patchset that changes the libvirt driver loading code resolves this particular issue. Once I fixed</div>
<div>this locally, I ran into the second problem below.</div><div><br></div><div>The second issue is the circular dependency problem. This issue is only really visible once the driver load race condition is resolved.</div>
<div><br></div><div>In this issue, the storage driver has a hard-coded connection to QEMU, as Peter mentioned in his reply. This is the only problem</div></div><div>that the config driver actually resolves. Without this (or a similar patch to fix the inter-dependency between QEMU and storage),</div>
<div>an errata would be needed to note that using secrets with storage pools is not to be used in production due to known issues. If I</div><div>remember correctly, RBD-backed pools used to require authentication. If that has not been resolved (and I will be honest that I</div>
<div>have not checked in a while since I use cephx authentication), then no RBD pools should be used for the time being, unless the</div><div>user is okay with their VMs being restarted anytime libvirt restarts. It should be a rare event, but painful when not expected.</div>
<div><br></div><div>My third patchset was merely a resubmit of my code to add support for RBD storage pools to the domain XML. Currently, you are</div><div>able to define a RBD pool, but not actually use it. I didn't like having to duplicate my cephx username, key, and the ceph monitor</div>
<div>hosts in all of my domain's XML, so I wrote the patch to fix it. I resubmitted it due to the length of time that had passed. The only</div><div>change was that it had been rebased to the latest libvirt version.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im"><br>
> The basic rationale behind this idea is that there exist circumstances under<br>
> which a driver may need to access things such as secrets during a time at<br>
> which there is no active connection to a hypervisor. Without a connection,<br>
> the data can't be accessed currently. I felt that this was a much simpler<br>
> solution to the problem that building new APIs that do not require a connection<br>
> to operate.<br>
<br>
</div>We have a handful of places in our code where we call out to public<br>
APIs to implement some piece of functionality. I've never been all<br>
that happy about these scenarios. If we call the public API directly,<br>
they cause havoc with error reporting because we end up dispatching<br>
and resetting errors in the middle of an nested API call. Calling the<br>
driver methods directly by doing conn->driver->invokeMethod() is<br>
somewhat tedious & error prone because we're then skipping various<br>
sanity tests that the public APIs do.  With ACL checking now implemented<br>
we also have the slightly odd situation that a public API check which<br>
is documented as requiring permission 'xxxx' may also require permissions<br>
'yyyy' and 'zzzz' to deal with other public APIs we invoke secretly.<br>
<br>
I think there is a fairly strong argument that our internal implementations<br>
could be decoupled from the public APIs, so we can call methods internally<br>
without having to go via the public APIs at all.<br>
<br>
On the flip side though, there's also a long term desire to separate<br>
the different drivers into separate daemons, eg so the secrets driver<br>
might move into a  libvirt-secretsd daemon, which might explicitly<br>
require use to go via the public APIs.<br>
<div class="im"><br></div></blockquote><div><br></div><div><div>If there were an internal API that did not require a connection, that would probably be a better solution, at least as long</div><div>as libvirt stays one monolithic process, anyway. In the specific problem I have run in to, the conn variable is NULL, due to</div>
<div>no connection existing (or being able to exist). In this case, trying conn->driver->invokeMethod() would fail unless there is</div><div>some magic in the code I'm not aware of.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> This driver is technically what one may call a hypervisor driver, but it<br>
> does not implement any domain operations. It simply exists to handle requests<br>
> by drivers for access to informatino that would otherwise require a connection.<br>
> The URI used for this driver is 'config:///' and has been tested working on 4<br>
> different machines of mine, running three different distributions of Linux<br>
> (Archlinux, Gentoo, and CentOS). Being a very simple driver, I would expect<br>
> it to work pretty much anywhere.<br>
><br>
> I would love to hear any comments and suggestions you may have about this<br>
> driver. At the very least this plus my next patchset resolves the startup<br>
> race condition on my machine. If a more robust setup (likely a new internal<br>
> API) is in the works, this driver could act as a band-aid to allow access<br>
> to this type of data in the interim if a better resolution is a ways off.<br>
<br>
</div>One big concern I have about this approach is the fact that once we add<br>
this 'config:///' driver, it is very hard for us to ever remove it again,<br>
since this concept leaks out into the public API. So we're going to want<br>
to be very sure that this is something we wish to support long term, and<br>
I don't really have such confidence myself.<br>
<br>
I'd like to understand a bit more about the dependancy issue you're facing<br>
to see if there's something else we can do to address it.<br></blockquote><div><br></div><div><div>I agree that adding a new connection driver would make it difficult to remove later. In fact, as I (think I) mentioned in my</div>
<div>patch descriptions, adding the config driver is not required to resolve the race condition. It is (or some similar fix to the</div><div>circular dependency between storage and qemu), however, needed in order to allow storage volumes (of any kind) to use</div>
<div>secrets on running VMs during a libvirt restart. Without a fix for that issue, any VM using a volume with a secret will be killed</div><div>and restarted upon a libvirt restart. In fact, if the driver load order is resolved, but the circular dependency is not, the restart</div>
</div><div>no longer randomly reboots VMs on my system, it always does. Though, to be fair, in that case, it never reboots a VM with</div><div>a file-backed volume once the race condition is removed. I've also never seen a VM get rebooted on a libvirt restart when</div>
<div>not using storage pools, either. Unless I'm mistaken, though, it looks to me like storage pools are sort of the future in libvirt.</div><div>It makes the configuration of a VM a lot simpler, since the custom stuff is held in the storage pool. Storage pools also enable</div>
<div>management frontends to support new storage backends with little to no work, which is a big plus for any administrator. Currently,</div><div>you're pretty much just stuck with a file-backed VM unless you want to hand-craft your XML.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><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>