[libvirt] [PATCHv2 0/6] Expose per-disk label overrides.

[Daniel Veillard]

On Thu, Dec 22, 2011 at 05:47:45PM -0700, Eric Blake wrote:
> In a system that uses dynamic_ownership=0 and NFS disks, the _only_
> path in the virDomainDestroy path where libvirt itself performs a
> syscall on the NFS file system was on attempting to relabel a disk
> back to its default label.  If an NFS server hosting a domain's
> disk image hangs, then this renders libvirtd stuck in an lstat()
> call, with the driver lock held, effectively blocking out _all_
> other attempts to connect to libvirtd for any remaining domains not
> tied to the hung NFS server.  (Verification that the SELinux relabel
> was the only point where libvirtd, rather than not qemu, could hang,
> was done by Red Hat VDSM folks in https://bugzilla.redhat.com/746666;
> with a hack to avoid the relabel, the destroy no longer hangs.)
> 
> But without bleeding edge NFS servers, you can't put a SELinux
> label on the file in the first place; libvirt was happily ignoring
> the failure to change the label on domain start (thanks to the
> selinux virt_use_nfs bool), only to hang on the stuck server on
> domain destroy that it would have again ignored had the server not
> been stuck.  If we allow the users to modify the XML to mark that
> a file does not need to be labeled in the first place, then we can
> avoid both attempts to label at startup and relabel at destroy, thus
> preventing libvirtd from getting stuck on a virDomainDestroy()
> called on a guest whose NFS server went down.
> 
> This patch series is based on an earlier attempt (the hack which
> got tested in the above-mentioned bz 746666), incorporating feedback
> from Dan Berrange suggesting that the ability for the user to
> override labeling on a per-disk basis is smarter than marking
> ignored failure to label in internal XML used only by libvirtd:
> v1: https://www.redhat.com/archives/libvir-list/2011-December/msg00246.html
> 
> I'm still working on patch 6/6 (actually hooking up the security
> manager to honor the relabel='no' request), and hope to post that
> tomorrow as part of this thread; that's really the only patch that
> resembles anything from v1 (the first 5 patches are basically brand
> new to this series), but I'm posting the first 5 patches now to get
> review started and make sure the proposed XML makes sense.
> 
> I envision that this can be further expanded (sticking <seclabel>
> elements on more XML items that get labeled in the host), but for
> now I focused just on disk devices, as those are the most likely
> to reside over NFS.
> 
> Ultimately, this patch series is only a bandaid; the underlying
> problem (making a syscall that can block indefinitely on a hung
> NFS server while holding the driver lock) is still present, and
> triggered if you have dynamic_ownership=1 (where we also attempt
> a chown of the disk image, regardless of the SELinux labels).
> Later down the road, I also plan to work on Dan's proposal to
> break the driver lock into smaller, more manageable sections, so
> we can get back to our documented goal of never blocking while
> holding lock, but instead using the job condition variables to
> detect when a single domain is stuck on a blocked call:
> https://www.redhat.com/archives/libvir-list/2011-November/msg00267.html

  Okay, I have reviewed the patch set, it looks sane to me.
The devil of course lies in the details, and for this kind of changes
I tend to think only serious testing can really validate the approach.
That's why I'm inclined to push this as part of the release candidate
to try to augment testing.

  So ACK, I'm pushing this, I just had to slightly rebase patch #4
because the context in one of the documentation example XML changed
slightly.

  Pushed to git head,

   thanks !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/