Re: [libvirt] Potential problem with /proc/mounts items that don't exist inside Kubernetes

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?

> > + > > 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. >