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:

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;

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

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

On 07/06/2017 12:11 PM, Daniel P. Berrange wrote: > 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 :-) > I created the following BZ: Don't use cgroup mount points from /proc/mounts that are hidden https://bugzilla.redhat.com/1468214 I will assign it to myself, and I will try to create a fix. Should I fail, I will ask for help.

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