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

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 >