On Mon, 24 Feb 2025 23:33:01 GMT, Sergey Chernyshev <schernys...@openjdk.org> wrote:
>> `CgroupV1Controller::set_subsystem_path` needs high level comment update to >> describe the logic happening. >> >> Testing: >>>>And after the patch this would become this, right? >>>>``` >>>>/sys/fs/cgroup/cpu,cpuacct/system.slice/garden.service/garden/bad/2f57368b-0eda-4e52-64d8-af5c >>>>/sys/fs/cgroup/cpu,cpuacct/ >>>>``` >>> >>>It depends on whether it was a subgroup in the initial path. If >>>bad/2f57368b-0eda-4e52-64d8-af5c is the subgroup, the reduction will be >>> >>>``` >>>/sys/fs/cgroup/cpu,cpuacct/system.slice/garden.service/garden/bad/2f57368b-0eda-4e52-64d8-af5c >>>/sys/fs/cgroup/cpu,cpuacct/bad/2f57368b-0eda-4e52-64d8-af5c >>>/sys/fs/cgroup/cpu,cpuacct/bad >>>/sys/fs/cgroup/cpu,cpuacct/ >>>``` >> >> The above case, doesn't seem to be reflected by any gtest test case (or >> others), please add those. > >> The above case, doesn't seem to be reflected by any gtest test case (or >> others), please add those. > > The subgroup path reduction is covered by > `TestMemoryWithSubgroups#testMemoryLimitSubgroupV1` (it wouldn't be possible > in gtests as it requires touching cgroup tree on a host system). It actually > checks that the subgroup directory exists and skips non-existent directories, > that is reflected in the test log below. The bottom line comes from > `CgroupV1Controller::set_subsystem_path`. > > ========== NEW TEST CASE: Cgroup V1 subgroup memory limit: 100m > [COMMAND] > docker run --tty=true --rm --privileged --cgroupns=host --memory 200m > jdk-internal:test-containers-docker-TestMemoryWithSubgroups-subgroup sh -c > mkdir -p /sys/fs/cgroup/memory/test ; echo 100m > > /sys/fs/cgroup/memory/test/memory.limit_in_bytes ; echo $$ > > /sys/fs/cgroup/memory/test/cgroup.procs ; /jdk/bin/java > -Xlog:os+container=trace -version > [2025-02-24T22:33:28.049656218Z] Gathering output for process 23369 > [ELAPSED: 5385 ms] > [STDERR] > > [STDOUT] > [0.002s][trace][os,container] OSContainer::init: Initializing Container > Support > [0.003s][debug][os,container] Detected optional pids controller entry in > /proc/cgroups > [0.004s][debug][os,container] Detected cgroups hybrid or legacy hierarchy, > using cgroups v1 controllers > [0.004s][trace][os,container] set_subsystem_path: cgroup v1 path reduced to: > /test. > > With additional logging added before line 77, this could be looking like > > [STDOUT] > [0.002s][trace][os,container] OSContainer::init: Initializing Container > Support > [0.002s][debug][os,container] Detected optional pids controller entry in > /proc/cgroups > [0.003s][debug][os,container] Detected cgroups hybrid or legacy hierarchy, > using cgroups v1 controllers > [0.004s][trace][os,container] set_subsystem_path: skipped non existent > directory: > /docker/cc32e455402a8c98d1df6a81c685a540e7e528e714c981b10845c31b64d8a370/test. > [0.004s][trace][os,container] set_subsystem_path: skipped non existent > directory: > /cc32e455402a8c98d1df6a81c685a540e7e528e714c981b10845c31b64d8a370/test. > [0.005s][trace][os,container] set_subsystem_path: cgroup v1 path reduced to: > /test. > > Before the fix, the current path adjustment scheme would produce the > following order: > > /sys/fs/cgroup/memory/docker/cc32e455402a8c98d1df6a81c685a540e7e528e714c981b10845c31b64d8a370/test > /sys/fs/cgroup/memory/docker/cc32e455402a8c98d1df6a81c685a540e7e528e714c981b10845c31b64d8a370 > /sys/fs/cgroup/memory/docker > /sys/fs/cgroup/memory > > Only the last path is valid in the conta... > @sercher As far as I can see this is a fairly simple case which would be > covered by a simpler patch. My comment was in regards to my comment > [here](https://github.com/openjdk/jdk/pull/21808#discussion_r1865630320). > Where you replied with [this > answer](https://github.com/openjdk/jdk/pull/21808#discussion_r1865663436). I > don't see where anything you've described in your answer is being tested, > covering this new code: > > https://github.com/openjdk/jdk/pull/21808/files#diff-8910f554ed4a7bc465e01679328b3e9bd64ceaa6c85f00f0c575670e748ebba9R63-R77 The code fragment you mentioned is executed under condition at line 62, that is, `_root` and `cgroup_path` are not equal. This happens exactly when a process PID is written to `cgroup.procs` file in the directory, belonging to a certain control group G, i.e. the process PID is moved to the control group G. Now that we have `_root` and `cgroup_path` non-equal, such as in my response [here](https://github.com/openjdk/jdk/pull/21808#discussion_r1865663436), i.e. _root: /system.slice/garden.service/garden/good/2f57368b-0eda-4e52-64d8-af5c cgroup_path: /system.slice/garden.service/garden/bad/2f57368b-0eda-4e52-64d8-af5c the loop at lines 67-77 is determining the "subgroup" part of the above `cgroup_path` , producing the debug message at line 73. The above case is CloudFoundry specific, while in the default docker setup it will be _root: /docker/cc32e455402a8c98d1df6a81c685a540e7e528e714c981b10845c31b64d8a370 cgroup_path: /docker/cc32e455402a8c98d1df6a81c685a540e7e528e714c981b10845c31b64d8a370/test In both cases it needs to be determined what (trailing) part of `cgroup_path` is an actual subgroup path, because this is how we find a directory that exists in the container. It's not known whether the subgroup path is /bad/2f57368b-0eda-4e52-64d8-af5c or /garden/bad/2f57368b-0eda-4e52-64d8-af5c and then the cgroup files path is either /sys/fs/cgroup/cpu,cpuacct/bad/2f57368b-0eda-4e52-64d8-af5c or /sys/fs/cgroup/cpu,cpuacct/garden/bad/2f57368b-0eda-4e52-64d8-af5c The docker case is not any different from the CF case. I therefore suggest [this](https://github.com/openjdk/jdk/pull/21808#discussion_r1865663436) case is covered by `TestMemoryWithSubgroups#testMemoryLimitSubgroupV1` as noted previously. Hope this helps. > > That is, some sub-group without a match, but some with one. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21808#issuecomment-2682721738
