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

[Juan Hernández]

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
>