8298730: Refactor subsystem_file_line_contents and add docs and tests

[jmtd]

Backport as a pre-requisite for 8313083. Not clean:

• adjustments for lack of log_trace
• remove hunks relating to the pid controller, which isn't in 8u
• remove use of stringStream in subsystem_file_line_contents, and stick to the existing char[] code
  • attempts to backport use of stringStream here caused a segfault for reasons so far unknown (but possibly due to differences between stringStream implementation in 8u and 11u+)
  • drop changes to ostream.hpp to ostream.cpp since we aren't using stringStream here
• dropped test_os_linux_cgroups.cpp from the patch as there are no gtests in 8u

I noticed a context difference in memory_and_swap_limit_in_bytes for Cgroups V1, which turns out to be because of 8284950. We might want to consider that for 8u as well.

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• JDK-8298730 needs maintainer approval

Issue

• JDK-8298730: Refactor subsystem_file_line_contents and add docs and tests (Enhancement - P4 - Requested)

Reviewers

• Severin Gehwolf (@jerboaa - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/776/head:pull/776
$ git checkout pull/776

Update a local copy of the PR:
$ git checkout pull/776
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/776/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 776

View PR using the GUI difftool:
$ git pr show -t 776

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/776.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jdowland! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

This backport pull request has now been updated with issue from the original commit.

[jmtd]

@jerboaa FYI! I've not un-drafted this yet because it's not ready for review due to the test failures. I'm working on those (although running tier1 keeps getting OOM killed on my workstation).

[jerboaa]

I noticed a context difference in memory_and_swap_limit_in_bytes for Cgroups V1, which turns out to be because of 8284950. We might want to consider that for 8u as well.

This is a cgroup v1 feature and at this point on its way out. More and more systems these days are cgroups v2. I don't think that's worth doing in the stable maintenance only phase 8u is in.

[jmtd]

The jdk_security_infra tests are failing for me locally in master, too.

[jerboaa]

Please merge latest master. Some more jdk_security_infra tests have been problem listed: #775

[jerboaa]

The jdk_security_infra tests are failing for me locally in master, too.

https://github.com/openjdk/jdk8u-dev/pull/776/checks?check_run_id=67917822702 shouldn't all fail, though.

[jerboaa]

The JDK with this seems to segfault:

Segmentation fault (core dumped)
Cannot determine version of java to run jtreg

[jmtd]

The JDK with this seems to segfault:

Thanks for looking. Did you reproduce this locally, or find segfault in the GHA logs? I haven't found that. I did find "Error: cannot determine version for JDK", and jtreg returning error code 5, which seems to mean "other fault occurred". I've also not managed to reproduce the problem locally yet, after several attempts, it is building and running fine. I've been using jdk_security_infra as the test target for iterating on, and all but "teliarootcav2" pass (and that fails for unrelated reasons).

[jerboaa]

Thanks for looking. Did you reproduce this locally, or find segfault in the GHA logs?

https://github.com/jmtd/jdk8u-dev/actions/runs/23535698710/job/68511568754#step:9:26

[jmtd]

My current investigative theory is, this change introduces using stringStream into part of the container code, but 8u and 11u (and newer)'s stringStream are quite different, 8u lacking at least JDK-8224193 , JDK-8225225 and JDK-8260030 which may be relevant.

[jmtd]

I'm exploring two avenues to fix this. One is to resolve the segfault, which revolves around stringStream, possibly using more backports. The other is to avoid stringStream for 8298730 in 8u. I've pushed two commits that implement the latter approach.

The original change for 8298730 introduces the first use of stringStream into the container code (8287007 introduces more later). If we stick to the older char[] code, we side-step the problem (waiting for GHA logs to confirm, but it looks good in another test branch). I also backed out the changes made to stringStream in ostream.{cpp,hpp} since we aren't exercising those in any of the other changed parts.

[jmtd]

New test failure (timeout) to investigate: gc/concurrentMarkSweep/CheckAllocateAndSystemGC.java

[jerboaa]

New test failure (timeout) to investigate: gc/concurrentMarkSweep/CheckAllocateAndSystemGC.java

That's a known intermittent failure on 32 bit x86.

[mlbridge]

Webrevs

• 00: Full (bed4a28e)

[jerboaa]

@jmtd Without the gtests please specify what kind of testing you've done to verify the modified patch does what it's supposed to? Thanks!

[jmtd]

The tests in test_cgroupSubsystem_linux.cpp are solely unit tests for the parsing in subsystem_file_line_contents. By bind-mounting a plain file ov er /proc/cgroups in a separate mount namespace, and invoking

  ./bin/java -XX:+UnlockDiagnosticVMOptions -XX:+PrintContainerInfo -version

I can observe java's behaviour which indirectly indicates whether or not the function parsed successfully.

I started with a copy of my system's real /proc/cgroups and verified the above command properly reported on the cgroup settings.

For the tests which specify content that should fail (e.g. file contents foo ), I altered the line for the cpu controller line according to the test (e.g., for foo , which checks for missing value in key/value format lines, I altered
the cpu entry to "cpu "). The result is java reporting on a missing controller at kernel level, which seems like the right behaviour.

For the tests that should pass:

• key/value delimited by '\t': existing format of /proc/cgroups on my host. controller properly detected
• k/v delimited by ' ': controller properly detected
• correct line preceded by a line with a key having a common prefix: this scenario is true in regular /proc/cgroups (cpuset/cpuacct/cpu); I tested all permutations of the cpu line preceding or following cpuset, either
  immediately or with other lines interposed: fine
• correct line followed by line with key having common prefix: as above
• multiple lines with matching keys, first match wins: fine

I haven't yet attempted to replicate the testing of "max 10000" and "max 10001" (which are for parsing other cgroups files) nor the individual tests in SubSystemFileLineContentsSingleLine.

I attempted to run TestMisc.java (I recall you pointing out that the logs from it in 8u indicated a problem that this backport would fix), however my workstation doesn't enumerate the memory controller in /proc/cgroups (8349988, not yet backported to 8u) so I would need retry this one in a VM with a suitable kernel (the bind-mount trick doesn't work for this test since it needs to be applied within the containers that the test create)

The success cases above, and also using this "fake /proc/cgroups" technique to expose the memory controller on my workstation, demonstrate java correctly enumerating the memory and cpu limits.

[jerboaa]

OK, I've tested this using TestMisc and #726 and this fixes the issues seen in the review of the PR. LGTM.

[openjdk]

⚠️ @jmtd This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[jmtd]

/approval request This is a pre-requisite for 8313083 which we want in 8u. It wasn't clean. For 8u, I've dropped some of the original patch: the new gtests, since 8u lacks gtest harness; changes to stringStream; and I reworked the patch to not introduce stringStream into the cgroups code: this avoids a SEGV that we hit and did not resolve.

[openjdk]

@jmtd
8298730: The approval request has been created successfully.