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

[Juan Hernández]

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.
> 
> 
> Regards,
> Daniel
>