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

[Daniel P. Berrange]

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 :|