[PATCH] 8217766: Container Support doesn't work for some Join Controllers combinations
Bob Vandette
bob.vandette at oracle.com
Fri Mar 1 14:41:23 UTC 2019
Hi Benjamin,
Thanks for working on this issue. Your approach is exactly what I was planning on doing for JDK 13.
I’d be happy to sponsor your work. There are a couple of other container issues that have been reported and
maybe I can fix them all at once.
As for your proposed patch, there is a Java implementation that provides Container Metrics that also needs to be
updated.
jdk/open/src/java.base/linux/classes/jdk/internal/platform/cgroup1/Metrics.java
I’ve been considering modifying the Java level to use JNI in order to reduce the
duplication but lets get these changes done first.
Bob.
> On Feb 27, 2019, at 1:58 AM, Benjamin Pineau <ben.pineau at gmail.com> wrote:
>
> Hi,
>
> Here is a patch for the cgroup identification issue reported
> in https://bugs.openjdk.java.net/browse/JDK-8217766
> I do not have a sponsor, is someone here willing to help?
>
> Thanks,
>
> # HG changeset patch
> # User Benjamin Pineau <ben.pineau at gmail.com>
> # Date 1551124966 -3600
> # Mon Feb 25 21:02:46 2019 +0100
> # Node ID 1217b6d4cc5fbe07596ad131cf628b50d091f5f2
> # Parent ee4488381c78135bbb93ad93d6883ff5b8d0d721
> [PATCH] 8217766: Container Support doesn't work for some Join Controllers combinations
>
> The cgroup identification -implemented by parsing /proc/self/mountinfo
> and /proc/self/cgroup- assumed each cgroup controller was mounted
> disjoint from the others (except for "cpu" and "cpuacct" controllers).
> Which means, we expected one single controller per mountinfo line.
>
> This matches the way most Linux distributions currently configure
> cgroupsv1 by default. Yet controllers can be grouped arbitrarily.
> For instance, using the JoinControllers systemd directive.
> One use case for that is to let Kubernetes' kubelet discover his own
> dedicated and reserved cgroup hierarchy. In that situation, the JVM
> fails to discover the expected cgroup controllers set, and, when runing
> containerized, default to a suboptimal understanding of available resources.
>
> Supporting arbitrarily controllers groups per mountpoint actually
> allows for simpler and easier to follow code, as we don't need nested
> if/else for every controller.
>
> diff --git a/src/hotspot/os/linux/osContainer_linux.cpp b/src/hotspot/os/linux/osContainer_linux.cpp
> --- a/src/hotspot/os/linux/osContainer_linux.cpp
> +++ b/src/hotspot/os/linux/osContainer_linux.cpp
> @@ -217,16 +217,11 @@
> * we are running under cgroup control.
> */
> void OSContainer::init() {
> - int mountid;
> - int parentid;
> - int major;
> - int minor;
> FILE *mntinfo = NULL;
> FILE *cgroup = NULL;
> char buf[MAXPATHLEN+1];
> char tmproot[MAXPATHLEN+1];
> char tmpmount[MAXPATHLEN+1];
> - char tmpbase[MAXPATHLEN+1];
> char *p;
> jlong mem_limit;
>
> @@ -260,87 +255,27 @@
> return;
> }
>
> - while ( (p = fgets(buf, MAXPATHLEN, mntinfo)) != NULL) {
> - // Look for the filesystem type and see if it's cgroup
> - char fstype[MAXPATHLEN+1];
> - fstype[0] = '\0';
> - char *s = strstr(p, " - ");
> - if (s != NULL &&
> - sscanf(s, " - %s", fstype) == 1 &&
> - strcmp(fstype, "cgroup") == 0) {
> + while ((p = fgets(buf, MAXPATHLEN, mntinfo)) != NULL) {
> + char tmpcgroups[MAXPATHLEN+1];
> + char *cptr = tmpcgroups;
> + char *token;
> +
> + // mountinfo format is documented at https://www.kernel.org/doc/Documentation/filesystems/proc.txt
> + if (sscanf(p, "%*d %*d %*d:%*d %s %s %*[^-]- cgroup %*s %s", tmproot, tmpmount, tmpcgroups) != 3) {
> + continue;
> + }
>
> - if (strstr(p, "memory") != NULL) {
> - int matched = sscanf(p, "%d %d %d:%d %s %s",
> - &mountid,
> - &parentid,
> - &major,
> - &minor,
> - tmproot,
> - tmpmount);
> - if (matched == 6) {
> - memory = new CgroupSubsystem(tmproot, tmpmount);
> - }
> - else
> - log_debug(os, container)("Incompatible str containing cgroup and memory: %s", p);
> - } else if (strstr(p, "cpuset") != NULL) {
> - int matched = sscanf(p, "%d %d %d:%d %s %s",
> - &mountid,
> - &parentid,
> - &major,
> - &minor,
> - tmproot,
> - tmpmount);
> - if (matched == 6) {
> - cpuset = new CgroupSubsystem(tmproot, tmpmount);
> - }
> - else {
> - log_debug(os, container)("Incompatible str containing cgroup and cpuset: %s", p);
> - }
> - } else if (strstr(p, "cpu,cpuacct") != NULL || strstr(p, "cpuacct,cpu") != NULL) {
> - int matched = sscanf(p, "%d %d %d:%d %s %s",
> - &mountid,
> - &parentid,
> - &major,
> - &minor,
> - tmproot,
> - tmpmount);
> - if (matched == 6) {
> - cpu = new CgroupSubsystem(tmproot, tmpmount);
> - cpuacct = new CgroupSubsystem(tmproot, tmpmount);
> - }
> - else {
> - log_debug(os, container)("Incompatible str containing cgroup and cpu,cpuacct: %s", p);
> - }
> - } else if (strstr(p, "cpuacct") != NULL) {
> - int matched = sscanf(p, "%d %d %d:%d %s %s",
> - &mountid,
> - &parentid,
> - &major,
> - &minor,
> - tmproot,
> - tmpmount);
> - if (matched == 6) {
> - cpuacct = new CgroupSubsystem(tmproot, tmpmount);
> - }
> - else {
> - log_debug(os, container)("Incompatible str containing cgroup and cpuacct: %s", p);
> - }
> - } else if (strstr(p, "cpu") != NULL) {
> - int matched = sscanf(p, "%d %d %d:%d %s %s",
> - &mountid,
> - &parentid,
> - &major,
> - &minor,
> - tmproot,
> - tmpmount);
> - if (matched == 6) {
> - cpu = new CgroupSubsystem(tmproot, tmpmount);
> - }
> - else {
> - log_debug(os, container)("Incompatible str containing cgroup and cpu: %s", p);
> - }
> - }
> - }
> + while ((token = strsep(&cptr, ",")) != NULL) {
> + if (strcmp(token, "memory") == 0) {
> + memory = new CgroupSubsystem(tmproot, tmpmount);
> + } else if (strcmp(token, "cpuset") == 0) {
> + cpuset = new CgroupSubsystem(tmproot, tmpmount);
> + } else if (strcmp(token, "cpu") == 0) {
> + cpu = new CgroupSubsystem(tmproot, tmpmount);
> + } else if (strcmp(token, "cpuacct") == 0) {
> + cpuacct= new CgroupSubsystem(tmproot, tmpmount);
> + }
> + }
> }
>
> fclose(mntinfo);
> @@ -392,30 +327,30 @@
> return;
> }
>
> - while ( (p = fgets(buf, MAXPATHLEN, cgroup)) != NULL) {
> - int cgno;
> - int matched;
> - char *controller;
> + while ((p = fgets(buf, MAXPATHLEN, cgroup)) != NULL) {
> + char *controllers;
> + char *token;
> char *base;
>
> /* Skip cgroup number */
> strsep(&p, ":");
> - /* Get controller and base */
> - controller = strsep(&p, ":");
> + /* Get controllers and base */
> + controllers = strsep(&p, ":");
> base = strsep(&p, "\n");
>
> - if (controller != NULL) {
> - if (strstr(controller, "memory") != NULL) {
> + if (controllers == NULL) {
> + continue;
> + }
> +
> + while ((token = strsep(&controllers, ",")) != NULL) {
> + if (strcmp(token, "memory") == 0) {
> memory->set_subsystem_path(base);
> - } else if (strstr(controller, "cpuset") != NULL) {
> + } else if (strcmp(token, "cpuset") == 0) {
> cpuset->set_subsystem_path(base);
> - } else if (strstr(controller, "cpu,cpuacct") != NULL || strstr(controller, "cpuacct,cpu") != NULL) {
> + } else if (strcmp(token, "cpu") == 0) {
> cpu->set_subsystem_path(base);
> + } else if (strcmp(token, "cpuacct") == 0) {
> cpuacct->set_subsystem_path(base);
> - } else if (strstr(controller, "cpuacct") != NULL) {
> - cpuacct->set_subsystem_path(base);
> - } else if (strstr(controller, "cpu") != NULL) {
> - cpu->set_subsystem_path(base);
> }
> }
> }
More information about the hotspot-runtime-dev
mailing list