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

[Juan Hernández]

On 07/06/2017 01:07 PM, Juan Hernández wrote:
> 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.
>

The patch is here:

   https://www.redhat.com/archives/libvir-list/2017-July/msg00141.html

I'd appreciate any review.

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