On Thu, Jul 06, 2017 at 11:48:43AM +0200, Juan Hernández wrote: > On 07/06/2017 11:33 AM, Daniel P. Berrange wrote: > > On Thu, Jul 06, 2017 at 11:26:58AM +0200, Juan Hernández wrote: > > > On 07/06/2017 11:18 AM, Daniel P. Berrange wrote: > > > > On Thu, Jul 06, 2017 at 11:11:03AM +0200, Juan Hernández wrote: > > > > > Hello all, > > > > > > > > > > This is my first mail to this list, so let me introduce myself. My name is > > > > > Juan Hernandez, and I work in the oVirt team. Currently I am experimenting > > > > > with the integration between ManageIQ and KubeVirt. > > > > > > > > > > I recently detected a potential issue when running libvirt inside > > > > > Kubernetes, as part of KubeVirt. There are entries in /proc/mounts that > > > > > don't exist, and libvirt can't start virtual machines because of that. This > > > > > is specific to this enviroment, but I think it may be worth addressing it in > > > > > libvirt itself. See the following issue for details: > > > > > > > > > > Libvirt fails when there are hidden cgroup mount points in `/proc/mounts` > > > > > https://github.com/kubevirt/libvirt/issues/4 > > > > > > > > > > I suggested a possible fix there, which seems simple, but it makes all tests > > > > > fail. I'd be happy to fix the tests as well, but I would need some guidance > > > > > on how to do so. Any suggestion is welcome. > > > > > > > > The root cause problem will be the code that parse /proc/mounts. It needs > > > > to pick the last entry in the mounts file, since the earlier ones can be > > > > hidden. For some reason virCgroupDetectMountsFromFile instead picks the > > > > first entry, so that function needs updating todo the reverse. > > > > > > > > > > Is the order of /proc/mounts guaranteed? It may be, but I'd suggest to not > > > rely on that. Instead of that libvirt could check if the mount point does > > > actually exist, and skip if it it doesn't. That is the fix I proposed: > > > > The order of /proc/mounts reflects the order in which the mounts were > > performed. IOW, later entries will override earlier entries if there > > is path overlap. > > > > > > > > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c > > > index 5aa1db5..021a3f2 100644 > > > --- a/src/util/vircgroup.c > > > +++ b/src/util/vircgroup.c > > > @@ -393,6 +393,14 @@ virCgroupDetectMountsFromFile(virCgroupPtr group, > > > if (STRNEQ(entry.mnt_type, "cgroup")) > > > continue; > > > > > > + /* Some mount points in the /proc/mounts file may be > > > + * hidden by others, and may not actually exist from > > > + * the point of the view of the process, so we need > > > + * to skip them. > > > + */ > > > + if (!virFileExists(entry.mnt_dir)) > > > + continue; > > > > This is fragile because it is possible for the mount point to > > still exist, but for its contents to have been replaced or > > hidden. So we really do want to explicitly take only the last > > entry, instead of doing this check. > > > > That makes sense to me. Should I open a BZ to request that change? Yes, its worth tracking this in BZ. If you wanted to try to write a patch too, that'd be awesome :-) > > > > + > > > for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { > > > const char *typestr = virCgroupControllerTypeToString(i); > > > int typelen = strlen(typestr); > > > > > > That fix makes things work, for it doesn't pass the tests. > > Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
