RFR: JDK-8293472: Potentially incorrect container resource limit detection if manual cgroup fs mounts present [v6]

[Severin Gehwolf]

On Fri, 9 Sep 2022 05:39:55 GMT, 王超 <duke at openjdk.org> wrote:

>> Similar to [JDK-8253435](https://bugs.openjdk.org/browse/JDK-8253435), when setting the mount path of cgroup controller "memory/cpu/cpuacct/pids", it should also discard duplicate items.
>
> 王超 has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix for cgroup v2

Thanks for the updates! A few more thoughts.

src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 126:

> 124: }
> 125: 
> 126: void CgroupSubsystemFactory::set_controller_pathes(CgroupInfo* cg_infos,

Typo: `set_controller_paths`

src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 346:

> 344:                                    cg_infos[0]._mount_path, tmp_mount_point);
> 345:         }
> 346:       }

Couldn't we simplify this? For example by changing:


      if (!cgroupv2_mount_point_found && strcmp("cgroup2", tmp_fs_type) == 0) {
        cgroupv2_mount_point_found = true;
        any_cgroup_mounts_found = true;
        for (int i = 0; i < CG_INFO_LENGTH; i++) {
          assert(cg_infos[i]._mount_path == NULL, "_mount_path memory stomping");
          cg_infos[i]._mount_path = os::strdup(tmp_mount_point);
        }


to something like this:


      if (strcmp("cgroup2", tmp_fs_type) == 0) {
        cgroupv2_mount_point_found = true;
        any_cgroup_mounts_found = true;
        for (int i = 0; i < CG_INFO_LENGTH; i++) {
          set_controller_paths(cg_infos, i, "(cg2, unified)", tmpmount, tmproot);
        }


It would entail modifying the `sscanf` to something like `sscanf(p, "%*d %*d %*d:%*d %s %s %*[^-]- %s %*s %*s", tmproot, tmpmount, tmp_fs_type) == 3`. We could also get rid of duplicate local var `tmp_mount_point`.

test/hotspot/jtreg/containers/docker/TestCPUAwareness.java line 80:

> 78:             testCpuQuotaAndPeriod(100*1000, 100*1000, true);
> 79:             testCpuQuotaAndPeriod(150*1000, 100*1000, true);
> 80:             testCpuQuotaAndPeriod(400*1000, 100*1000, true);

It should be sufficient to test the `true` case once. I.e. one test `testCpuQuotaAndPeriod(50*1000, 100*1000, true);` should be enough (as this would extend container test runs needlessly).

test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java line 73:

> 71:             testMemoryLimit("500m", "524288000", true);
> 72:             testMemoryLimit("1g", "1073741824", true);
> 73:             testMemoryLimit("4g", "4294967296", true);

One test with the additional cgroup mounts should be sufficient. I suggest:


testMemoryLimit("500m", "524288000", true /* additional cgroup mount */);

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

PR: https://git.openjdk.org/jdk/pull/10193