RFR: 8217338: [Containers] Improve systemd slice memory limit support

[Severin Gehwolf]

On Fri, 2019-03-22 at 14:25 -0400, Bob Vandette wrote:
> Is there ever a situation where the memory.limit_in_bytes could be unlimited but the
> hierarchical_memory_limit is not?

Yes. This is the very bug. Under a systemd slice with newer kernels the
memory.limit_in_bytes file OpenJDK looks at has unlimited, but
memory.stat on the same hierarchy has the correct setting.

Patched JDK with -Xlog:os+container=trace

[0.051s][trace][os,container] Path to /memory.limit_in_bytes is /sys/fs/cgroup/memory/user.slice/user-cg.slice/run-raf53fa46d5cc47ce857fbfaae04459c1.scope/memory.limit_in_bytes
[0.051s][trace][os,container] Memory Limit is: 9223372036854771712
[0.051s][trace][os,container] Non-Hierarchical Memory Limit is: Unlimited
[0.051s][trace][os,container] Path to /memory.stat is /sys/fs/cgroup/memory/user.slice/user-cg.slice/run-raf53fa46d5cc47ce857fbfaae04459c1.scope/memory.stat
[0.051s][trace][os,container] Hierarchical Memory Limit is: 10485760

$ cat /sys/fs/cgroup/memory/user.slice/user-cg.slice/run-raf53fa46d5cc47ce857fbfaae04459c1.scope/memory.limit_in_bytes
9223372036854771712
$ cat /sys/fs/cgroup/memory/user.slice/user-cg.slice/memory.limit_in_bytes 
10485760
$ grep hierarchical /sys/fs/cgroup/memory/user.slice/user-cg.slice/run-raf53fa46d5cc47ce857fbfaae04459c1.scope/memory.stat
hierarchical_memory_limit 10485760
hierarchical_memsw_limit 9223372036854771712

This was mentioned in the bug comments as well.

> Could you maybe combine subsystem_file_contents with subsystem_file_line_contents
> by adding an additional argument?

Sure, I could look into that. I'll post an updated webrev soon.

Thanks for the review!

Cheers,
Severin


> BTW:  I found another problem with the mountinfo/cgroup parsing that impacts the
> container tests.
> 
> I don’t know why it only caused a failure on one of my systems.  I’m going to
> file another bug.  You might want to test with these changes to make sure
> your looking at the correct subsystem files.

OK. Thanks for the heads-up. It looks unrelated, though.

> 
> diff --git a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
> --- a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
> +++ b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
> @@ -60,7 +60,7 @@
>                      path = mountPoint;
>                  }
>                  else {
> -                    if (root.indexOf(cgroupPath) == 0) {
> +                    if (cgroupPath.indexOf(root) == 0) {
>                          if (cgroupPath.length() > root.length()) {
>                              String cgroupSubstr = cgroupPath.substring(root.length());
>                              path = mountPoint + cgroupSubstr;
> diff --git a/test/lib/jdk/test/lib/containers/cgroup/MetricsTester.java b/test/lib/jdk/test/lib/containers/cgroup/MetricsTester.java
> --- a/test/lib/jdk/test/lib/containers/cgroup/MetricsTester.java
> +++ b/test/lib/jdk/test/lib/containers/cgroup/MetricsTester.java
> @@ -85,7 +85,7 @@
>                  String mountPoint = paths[1];
>                  if (root != null && cgroupPath != null) {
>                      if (root.equals("/")) {
> -                        if (cgroupPath.equals("/")) {
> +                        if (!cgroupPath.equals("/")) {
>                              finalPath = mountPoint + cgroupPath;
>                          } else {
>                              finalPath = mountPoint;
> @@ -94,7 +94,7 @@
>                          if (root.equals(cgroupPath)) {
>                              finalPath = mountPoint;
>                          } else {
> -                            if (root.indexOf(cgroupPath) == 0) {
> +                            if (cgroupPath.indexOf(root) == 0) {
>                                  if (cgroupPath.length() > root.length()) {
>                                      String cgroupSubstr = cgroupPath.substring(root.length());
>                                      finalPath = mountPoint + cgroupSubstr;
> @@ -103,7 +103,7 @@
>                          }
>                      }
>                  }
> -                subSystemPaths.put(subSystem, new String[]{finalPath});
> +                subSystemPaths.put(subSystem, new String[]{finalPath, mountPoint});
>              }
>          }
>      }
> 
> 
> Bob.
> 
> 
> 
> > On Mar 22, 2019, at 6:43 AM, Severin Gehwolf <sgehwolf at redhat.com> wrote:
> > 
> > Hi,
> > 
> > Please review this change which improves container detection support
> > tin the JVM. While docker container detection works quite well the
> > results for systemd slices with memory limits are mixed and depend on
> > the Linux kernel version in use. With newer kernel versions becoming
> > more widely used we should improve JVMs memory limit detection support
> > as well. This should be entirely backwards compatible as the
> > hierarchical limit will only be used if everything else is unlimited.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8217338
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8217338/04/webrev/
> > 
> > Testing: Manual testing of -XshowSettings and -Xlog:os+container=trace
> > with systemd slices on affected Linux distributions: Fedora 29,
> > recent Ubuntu (18-10). Existing docker tests pass. I'm currently also
> > running this through jdk/submit.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> >