Revert "cgroup/LXC: Do not condition availability of v2 by controllers"

[libvirt PATCH v2] Revert "cgroup/LXC: Do not condition availability of v2 by controllers"

Posted by Pavel Hrdina 1 year, 6 months ago

This reverts commit e49313b54ed2a149c71f9073659222742ff3ffb0.
This reverts commit a0f37232b9c4296ca16955cc625f75eb848ace39.

Revert them together to not break build.

This fix of the issue is incorrect and breaks usage of other controllers
in hybrid mode that systemd creates, specifically usage of devices and
cpuacct controllers as they are now assumed to be part of the cgroup v2
topology which is not true.

We need to find different solution to the issue.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/util/vircgroup.c   |  6 ++----
 src/util/vircgroupv2.c | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 49ebd37ded..a6a409af3d 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -2921,12 +2921,10 @@ int
 virCgroupBindMount(virCgroup *group, const char *oldroot,
                    const char *mountopts)
 {
-    ssize_t i;
+    size_t i;
     virCgroup *parent = virCgroupGetNested(group);
 
-    /* In hybrid environments, V2 may be mounted over V1.
-     * Mount the backends in reverse order. */
-    for (i = VIR_CGROUP_BACKEND_TYPE_LAST - 1; i >= 0; i--) {
+    for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
         if (parent->backends[i] &&
             parent->backends[i]->bindMount(parent, oldroot, mountopts) < 0) {
             return -1;
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index bf6bd11fef..4c110940cf 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -69,13 +69,28 @@ virCgroupV2Available(void)
         return false;
 
     while (getmntent_r(mounts, &entry, buf, sizeof(buf)) != NULL) {
+        g_autofree char *contFile = NULL;
+        g_autofree char *contStr = NULL;
+
         if (STRNEQ(entry.mnt_type, "cgroup2"))
             continue;
 
+        /* Systemd uses cgroup v2 for process tracking but no controller is
+         * available. We should consider this configuration as cgroup v2 is
+         * not available. */
+        contFile = g_strdup_printf("%s/cgroup.controllers", entry.mnt_dir);
+
+        if (virFileReadAll(contFile, 1024 * 1024, &contStr) < 0)
+            goto cleanup;
+
+        if (STREQ(contStr, ""))
+            continue;
+
         ret = true;
         break;
     }
 
+ cleanup:
     VIR_FORCE_FCLOSE(mounts);
     return ret;
 }
-- 
2.37.3

Re: [libvirt PATCH v2] Revert "cgroup/LXC: Do not condition availability of v2 by controllers"

Posted by Michal Prívozník 1 year, 6 months ago

On 10/25/22 13:43, Pavel Hrdina wrote:
> This reverts commit e49313b54ed2a149c71f9073659222742ff3ffb0.
> This reverts commit a0f37232b9c4296ca16955cc625f75eb848ace39.
> 
> Revert them together to not break build.
> 
> This fix of the issue is incorrect and breaks usage of other controllers
> in hybrid mode that systemd creates, specifically usage of devices and
> cpuacct controllers as they are now assumed to be part of the cgroup v2
> topology which is not true.
> 
> We need to find different solution to the issue.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/util/vircgroup.c   |  6 ++----
>  src/util/vircgroupv2.c | 15 +++++++++++++++
>  2 files changed, 17 insertions(+), 4 deletions(-)


Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

This brings me back to my original proposal - let's forget that hybrid
mode exist and aim on v1 or v2, solely.

Michal

Re: [libvirt PATCH v2] Revert "cgroup/LXC: Do not condition availability of v2 by controllers"

Posted by Daniel P. Berrangé 1 year, 6 months ago

On Tue, Oct 25, 2022 at 01:46:22PM +0200, Michal Prívozník wrote:
> On 10/25/22 13:43, Pavel Hrdina wrote:
> > This reverts commit e49313b54ed2a149c71f9073659222742ff3ffb0.
> > This reverts commit a0f37232b9c4296ca16955cc625f75eb848ace39.
> > 
> > Revert them together to not break build.
> > 
> > This fix of the issue is incorrect and breaks usage of other controllers
> > in hybrid mode that systemd creates, specifically usage of devices and
> > cpuacct controllers as they are now assumed to be part of the cgroup v2
> > topology which is not true.
> > 
> > We need to find different solution to the issue.
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >  src/util/vircgroup.c   |  6 ++----
> >  src/util/vircgroupv2.c | 15 +++++++++++++++
> >  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> This brings me back to my original proposal - let's forget that hybrid
> mode exist and aim on v1 or v2, solely.

FWIW, I thought that was already our intention. It has been recommended
to me by systemd maintainers that hybrid mode is best ignored.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [libvirt PATCH v2] Revert "cgroup/LXC: Do not condition availability of v2 by controllers"

Posted by Pavel Hrdina 1 year, 6 months ago

On Tue, Oct 25, 2022 at 01:13:44PM +0100, Daniel P. Berrangé wrote:
> On Tue, Oct 25, 2022 at 01:46:22PM +0200, Michal Prívozník wrote:
> > On 10/25/22 13:43, Pavel Hrdina wrote:
> > > This reverts commit e49313b54ed2a149c71f9073659222742ff3ffb0.
> > > This reverts commit a0f37232b9c4296ca16955cc625f75eb848ace39.
> > > 
> > > Revert them together to not break build.
> > > 
> > > This fix of the issue is incorrect and breaks usage of other controllers
> > > in hybrid mode that systemd creates, specifically usage of devices and
> > > cpuacct controllers as they are now assumed to be part of the cgroup v2
> > > topology which is not true.
> > > 
> > > We need to find different solution to the issue.
> > > 
> > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > > ---
> > >  src/util/vircgroup.c   |  6 ++----
> > >  src/util/vircgroupv2.c | 15 +++++++++++++++
> > >  2 files changed, 17 insertions(+), 4 deletions(-)
> > 
> > 
> > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> > 
> > This brings me back to my original proposal - let's forget that hybrid
> > mode exist and aim on v1 or v2, solely.
> 
> FWIW, I thought that was already our intention. It has been recommended
> to me by systemd maintainers that hybrid mode is best ignored.

The issue is that on systemd based distributions where you have v1 only
it is technically hybrid setup where systemd uses v2 without any
controller enabled for process tracking and that's the root cause of the
issue that the original patch wanted to address.

We need to add the lxc-container process to the correct cgroup in order
to be able to find the cgroup by PID of the lxc-container using systemd
API.

True v1 mode is enabled only on few distributions without systemd.

Pavel

Re: [libvirt PATCH v2] Revert "cgroup/LXC: Do not condition availability of v2 by controllers"

Posted by Daniel P. Berrangé 1 year, 6 months ago

On Tue, Oct 25, 2022 at 02:35:00PM +0200, Pavel Hrdina wrote:
> On Tue, Oct 25, 2022 at 01:13:44PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Oct 25, 2022 at 01:46:22PM +0200, Michal Prívozník wrote:
> > > On 10/25/22 13:43, Pavel Hrdina wrote:
> > > > This reverts commit e49313b54ed2a149c71f9073659222742ff3ffb0.
> > > > This reverts commit a0f37232b9c4296ca16955cc625f75eb848ace39.
> > > > 
> > > > Revert them together to not break build.
> > > > 
> > > > This fix of the issue is incorrect and breaks usage of other controllers
> > > > in hybrid mode that systemd creates, specifically usage of devices and
> > > > cpuacct controllers as they are now assumed to be part of the cgroup v2
> > > > topology which is not true.
> > > > 
> > > > We need to find different solution to the issue.
> > > > 
> > > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > > > ---
> > > >  src/util/vircgroup.c   |  6 ++----
> > > >  src/util/vircgroupv2.c | 15 +++++++++++++++
> > > >  2 files changed, 17 insertions(+), 4 deletions(-)
> > > 
> > > 
> > > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> > > 
> > > This brings me back to my original proposal - let's forget that hybrid
> > > mode exist and aim on v1 or v2, solely.
> > 
> > FWIW, I thought that was already our intention. It has been recommended
> > to me by systemd maintainers that hybrid mode is best ignored.
> 
> The issue is that on systemd based distributions where you have v1 only
> it is technically hybrid setup where systemd uses v2 without any
> controller enabled for process tracking and that's the root cause of the
> issue that the original patch wanted to address.

Oh, that's the setup present in RHEL-8  IIUC, but I didn't consider
that to be hybrid, and yes we need to support that.

I considered hybrid to be when the resource controllers used a mix of
v1 and v2 attachment. Something people tried todo when some of the
controllers were not ported to v2 yet.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|