On Mon, 16 Aug 2021 17:25:57 GMT, Harold Seigel <hsei...@openjdk.org> wrote:

> Please review this small fix for JDK-8272124.  The fix puts a limit of 3 when 
> splitting self cgroup lines by ':' so that Cgroup paths won't get truncated 
> if they contain embedded ':'s.  For example, an entry of 
> "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a 
> Cgroup path of "/user.sli:ce" instead of "/user.sli".
> 
> The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 
> and Linux aarch64.
> 
> Thanks, Harold

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java 
line 199:

> 197:                 action = (tokens -> setCgroupV2Path(infos, tokens));
> 198:             }
> 199:             selfCgroupLines.map(line -> line.split(":", 3))

Could you please add a comment explaining that the limit value of 3 comes from 
the fact that `/proc/self/cgroup` contains three colon-separated tokens per 
line. The last value, cgroup path, might contain `:`.

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java 
line 200:

> 198:             }
> 199:             selfCgroupLines.map(line -> line.split(":", 3))
> 200:                      .filter(tokens -> (tokens.length >= 3))

This filter no longer makes sense as tokens.length == 3 in every case after 
this patch. Please remove. From the javadoc from `String.split(String, limit)`:

If the limit is positive then the pattern will be applied
at most limit-1 times, the array's length will be
no greater than limit, and the array's last entry will contain
all input beyond the last matched delimiter.

test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java line 175:

> 173:     private String mntInfoCgroupsV1DoubleCpuset = mntInfoHybrid + 
> mntInfoCgroupv1MoreCpusetLine;
> 174:     private String mntInfoCgroupsV1DoubleCpuset2 = 
> mntInfoCgroupv1MoreCpusetLine + mntInfoHybrid;
> 175:     private String cgroupv1SelfCgroupContent = 
> "11:memory:/user.slice/user-1000.slice/user@1000.s:ervice\n" +

Could you please add a comment explaining that `/proc/self/cgroup` is supposed 
to contain **three** colon-separated fields, 
`hierarchy-ID:controller-list:cgroup-path`. Thus, this cgroup-path 
intentionally contains a colon so as to ensure the correct path is being 
extracted by the logic in CgroupSubsystemFactory.

Better yet, add a new test with `mountinfo` and `/proc/self/cgroup` contents 
observed in the wild and assert that `memoryInfo.getCgroupPath()` equals 
`memoryInfo.getMountRoot()`

-------------

PR: https://git.openjdk.java.net/jdk/pull/5127

Reply via email to