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

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 :-)

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