Skip to content

8309191: Reduce JDK dependencies of cgroup support #14216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

8309191: Reduce JDK dependencies of cgroup support #14216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
76cc3e8
Factor out logging from CgroupSubsystemFactory.create
pejovica May 22, 2023
edbacaf
Use simple patterns to parse cgroup files
pejovica May 23, 2023
dd9d8e5
Reimplement mountinfo parsing without using regex
pejovica May 24, 2023
51bd7d9
Use java.io for reading cgroup files
pejovica May 25, 2023
51904e0
Use simple loops to process cgroup files
pejovica May 26, 2023
4fc3af2
Merge branch 'master' into ap/cgroup-tweaks
pejovica May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/java.base/linux/classes/jdk/internal/platform/CgroupInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public void setCgroupPath(String cgroupPath) {
*
*/
static CgroupInfo fromCgroupsLine(String line) {
String[] tokens = line.split("\\s+");
String[] tokens = line.split("\t");
Copy link
Contributor

@jerboaa jerboaa Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, we now hard-code the expected delimiter and, thus, depend on what the kernel does. Do we have sufficient evidence this hasn't changed/won't change in the future?

Copy link
Author

@pejovica pejovica Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, the delimiter hasn't changed since the file was introduced, and judging by the kernel mailing list (e.g., see the following), I don't think it will change any time soon.

Copy link
Contributor

@jerboaa jerboaa Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced this is a good change. Going by that mailing list thread, it suggests that people considered changing it. If one of those attempts were successful, it would have broken this code. It makes it rather fragile. The issue, with container detection code going wrong is that you most likely never notice. Translating this to GraalVM means that the native image, would a) detect the wrong version in use or b) fail detection and use host values. In both cases the application will likely misbehave in a container setup with resource limits applied and you won't (easily) know why. So even though it's unlikely to be a problem, there is a chance it could be and it's asking for trouble for no good reason.

Therefore, being conservative about delimiters makes sense here. My choice in this case would be more robust code over relying on external factors. YMMV.

Copy link
Author

@pejovica pejovica Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so how about something like the following, would that be more acceptable?

Suggested change
String[] tokens = line.split("\t");
String[] tokens = Collections.list(new StringTokenizer(line, " \t")).toArray(new String[0]);

Copy link
Contributor

@jerboaa jerboaa Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringTokenizer() is a legacy class and is discouraged in new code. So not ideal either.

if (tokens.length != 4) {
return null;
}
Expand Down
23 changes: 11 additions & 12 deletions src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,12 @@
package jdk.internal.platform;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.math.BigInteger;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.function.Function;
import java.util.stream.Stream;

/**
* Cgroup version agnostic controller logic
Expand Down Expand Up @@ -161,16 +158,18 @@ public static double getDoubleValue(CgroupSubsystemController controller, String
public static long getLongEntry(CgroupSubsystemController controller, String param, String entryname, long defaultRetval) {
if (controller == null) return defaultRetval;

try (Stream<String> lines = CgroupUtil.readFilePrivileged(Paths.get(controller.path(), param))) {

Optional<String> result = lines.map(line -> line.split(" "))
.filter(line -> (line.length == 2 &&
line[0].equals(entryname)))
.map(line -> line[1])
.findFirst();
try {
long result = defaultRetval;
for (String line : CgroupUtil.readAllLinesPrivileged(Paths.get(controller.path(), param))) {
String[] tokens = line.split(" ");
if (tokens.length == 2 && tokens[0].equals(entryname)) {
result = Long.parseLong(tokens[1]);
break;
}
}

return result.isPresent() ? Long.parseLong(result.get()) : defaultRetval;
} catch (UncheckedIOException | IOException e) {
return result;
} catch (IOException e) {
return defaultRetval;
}
}
Expand Down
119 changes: 73 additions & 46 deletions src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
package jdk.internal.platform;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.lang.System.Logger;
import java.lang.System.Logger.Level;
import java.nio.file.Path;
Expand All @@ -38,9 +37,6 @@
import java.util.Optional;
import java.util.Objects;
import java.util.function.Consumer;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Stream;

import jdk.internal.platform.cgroupv1.CgroupV1Subsystem;
import jdk.internal.platform.cgroupv2.CgroupV2Subsystem;
Expand All @@ -54,36 +50,11 @@ public class CgroupSubsystemFactory {
private static final String MEMORY_CTRL = "memory";
private static final String PIDS_CTRL = "pids";

/*
* From https://www.kernel.org/doc/Documentation/filesystems/proc.txt
*
* 36 35 98:0 /mnt1 /mnt2 rw,noatime master:1 - ext3 /dev/root rw,errors=continue
* (1)(2)(3) (4) (5) (6) (7) (8) (9) (10) (11)
*
* (1) mount ID: unique identifier of the mount (may be reused after umount)
* (2) parent ID: ID of parent (or of self for the top of the mount tree)
* (3) major:minor: value of st_dev for files on filesystem
* (4) root: root of the mount within the filesystem
* (5) mount point: mount point relative to the process's root
* (6) mount options: per mount options
* (7) optional fields: zero or more fields of the form "tag[:value]"
* (8) separator: marks the end of the optional fields
* (9) filesystem type: name of filesystem of the form "type[.subtype]"
* (10) mount source: filesystem specific information or "none"
* (11) super options: per super block options
*/
private static final Pattern MOUNTINFO_PATTERN = Pattern.compile(
"^[^\\s]+\\s+[^\\s]+\\s+[^\\s]+\\s+" + // (1), (2), (3)
"([^\\s]+)\\s+([^\\s]+)\\s+" + // (4), (5) - group 1, 2: root, mount point
"[^-]+-\\s+" + // (6), (7), (8)
"([^\\s]+)\\s+" + // (9) - group 3: filesystem type
".*$"); // (10), (11)

static CgroupMetrics create() {
Optional<CgroupTypeResult> optResult = null;
try {
optResult = determineType("/proc/self/mountinfo", "/proc/cgroups", "/proc/self/cgroup");
} catch (IOException | UncheckedIOException e) {
} catch (IOException e) {
return null;
}
return create(optResult);
Expand All @@ -104,8 +75,7 @@ public static CgroupMetrics create(Optional<CgroupTypeResult> optResult) {
// not ready to deal with that on a per-controller basis. Return no metrics
// in that case
if (result.isAnyCgroupV1Controllers() && result.isAnyCgroupV2Controllers()) {
Logger logger = System.getLogger("jdk.internal.platform");
logger.log(Level.DEBUG, "Mixed cgroupv1 and cgroupv2 not supported. Metrics disabled.");
warn("Mixed cgroupv1 and cgroupv2 not supported. Metrics disabled.");
return null;
}

Expand All @@ -121,6 +91,11 @@ public static CgroupMetrics create(Optional<CgroupTypeResult> optResult) {
}
}

private static void warn(String msg) {
Logger logger = System.getLogger("jdk.internal.platform");
logger.log(Level.DEBUG, msg);
}
Comment on lines +94 to +97
Copy link
Contributor

@jerboaa jerboaa Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine and uncontroversial. Suggested name change log over warn. Perhaps apply this as a separate change?


/*
* Determine the type of the cgroup system (v1 - legacy or hybrid - or, v2 - unified)
* based on three files:
Expand Down Expand Up @@ -196,16 +171,15 @@ public static Optional<CgroupTypeResult> determineType(String mountInfo,
// See:
// setCgroupV1Path() for the action run for cgroups v1 systems
// setCgroupV2Path() for the action run for cgroups v2 systems
try (Stream<String> selfCgroupLines =
CgroupUtil.readFilePrivileged(Paths.get(selfCgroup))) {
Consumer<String[]> action = (tokens -> setCgroupV1Path(infos, tokens));
if (isCgroupsV2) {
action = (tokens -> setCgroupV2Path(infos, tokens));
}
Consumer<String[]> action = (tokens -> setCgroupV1Path(infos, tokens));
if (isCgroupsV2) {
action = (tokens -> setCgroupV2Path(infos, tokens));
}
for (String line : CgroupUtil.readAllLinesPrivileged(Paths.get(selfCgroup))) {
// The limit value of 3 is because /proc/self/cgroup contains three
// colon-separated tokens per line. The last token, cgroup path, might
// contain a ':'.
selfCgroupLines.map(line -> line.split(":", 3)).forEach(action);
action.accept(line.split(":", 3));
}

CgroupTypeResult result = new CgroupTypeResult(isCgroupsV2,
Expand Down Expand Up @@ -281,8 +255,8 @@ private static void setCgroupV1Path(Map<String, CgroupInfo> infos,
/**
* Amends cgroup infos with mount path and mount root. The passed in
* 'mntInfoLine' represents a single line in, for example,
* /proc/self/mountinfo. Each line is matched with MOUNTINFO_PATTERN
* (see above), so as to extract the relevant tokens from the line.
* /proc/self/mountinfo. Each line is parsed with {@link MountInfo#parse},
* so as to extract the relevant tokens from the line.
*
* Host example cgroups v1:
*
Expand All @@ -303,13 +277,13 @@ private static void setCgroupV1Path(Map<String, CgroupInfo> infos,
private static boolean amendCgroupInfos(String mntInfoLine,
Map<String, CgroupInfo> infos,
boolean isCgroupsV2) {
Matcher lineMatcher = MOUNTINFO_PATTERN.matcher(mntInfoLine.trim());
MountInfo mountInfo = MountInfo.parse(mntInfoLine);
boolean cgroupv1ControllerFound = false;
boolean cgroupv2ControllerFound = false;
if (lineMatcher.matches()) {
String mountRoot = lineMatcher.group(1);
String mountPath = lineMatcher.group(2);
String fsType = lineMatcher.group(3);
if (mountInfo != null) {
String mountRoot = mountInfo.mountRoot;
String mountPath = mountInfo.mountPath;
String fsType = mountInfo.fsType;
if (fsType.equals("cgroup")) {
Path p = Paths.get(mountPath);
String[] controllerNames = p.getFileName().toString().split(",");
Expand Down Expand Up @@ -345,6 +319,59 @@ private static boolean amendCgroupInfos(String mntInfoLine,
return cgroupv1ControllerFound || cgroupv2ControllerFound;
}

private record MountInfo(String mountRoot, String mountPath, String fsType) {
/*
* From https://www.kernel.org/doc/Documentation/filesystems/proc.txt
*
* 36 35 98:0 /mnt1 /mnt2 rw,noatime master:1 - ext3 /dev/root rw,errors=continue
* (1)(2)(3) (4) (5) (6) (7) (8) (9) (10) (11)
*
* (1) mount ID: unique identifier of the mount (may be reused after umount)
* (2) parent ID: ID of parent (or of self for the top of the mount tree)
* (3) major:minor: value of st_dev for files on filesystem
* (4) root: root of the mount within the filesystem
* (5) mount point: mount point relative to the process's root
* (6) mount options: per mount options
* (7) optional fields: zero or more fields of the form "tag[:value]"
* (8) separator: marks the end of the optional fields
* (9) filesystem type: name of filesystem of the form "type[.subtype]"
* (10) mount source: filesystem specific information or "none"
* (11) super options: per super block options
*/
static MountInfo parse(String line) {
String mountRoot = null;
String mountPath = null;

int separatorOrdinal = -1;
// loop over space-separated tokens
for (int tOrdinal = 1, tStart = 0, tEnd = line.indexOf(' '); tEnd != -1; tOrdinal++, tStart = tEnd + 1, tEnd = line.indexOf(' ', tStart)) {
Comment on lines +346 to +347
Copy link
Contributor

@jerboaa jerboaa Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, this now also hard-codes the delimiter: A single space. If we really want this custom parser, please add a unit test for it and extract it to a separate class.

A hypothetical line like the following would confuse the parser, setting ordinal to wrong values:

36   35 98:0 /mnt1 /mnt2 rw,noatime master:1 - cgroup /dev/root rw,errors=continue

We'd have: mountRoot = 98:0, mountPath = /mnt1, fsType = cgroup since it expects single space separated values. Seems fragile.

if (tStart == tEnd) {
break; // unexpected empty token
}
switch (tOrdinal) {
case 1, 2, 3, 6 -> {} // skip token
case 4 -> mountRoot = line.substring(tStart, tEnd); // root token
case 5 -> mountPath = line.substring(tStart, tEnd); // mount point token
default -> {
assert tOrdinal >= 7;
if (separatorOrdinal == -1) {
// check if we found a separator token
if (tEnd - tStart == 1 && line.charAt(tStart) == '-') {
separatorOrdinal = tOrdinal;
}
continue; // skip token
}
if (tOrdinal == separatorOrdinal + 1) { // filesystem type token
String fsType = line.substring(tStart, tEnd);
return new MountInfo(mountRoot, mountPath, fsType);
}
}
}
}
return null; // parsing failed
}
}

private static void setMountPoints(CgroupInfo info, String mountPath, String mountRoot) {
if (info.getMountPoint() != null) {
// On some systems duplicate controllers get mounted in addition to
Expand Down
36 changes: 14 additions & 22 deletions src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,32 +26,19 @@
package jdk.internal.platform;

import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.file.Files;
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.security.AccessController;
import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Stream;

public final class CgroupUtil {

@SuppressWarnings("removal")
public static Stream<String> readFilePrivileged(Path path) throws IOException {
try {
PrivilegedExceptionAction<Stream<String>> pea = () -> Files.lines(path);
return AccessController.doPrivileged(pea);
} catch (PrivilegedActionException e) {
unwrapIOExceptionAndRethrow(e);
throw new InternalError(e.getCause());
} catch (UncheckedIOException e) {
throw e.getCause();
}
}

static void unwrapIOExceptionAndRethrow(PrivilegedActionException pae) throws IOException {
Throwable x = pae.getCause();
if (x instanceof IOException)
Expand All @@ -64,29 +51,34 @@ static void unwrapIOExceptionAndRethrow(PrivilegedActionException pae) throws IO

static String readStringValue(CgroupSubsystemController controller, String param) throws IOException {
PrivilegedExceptionAction<BufferedReader> pea = () ->
Files.newBufferedReader(Paths.get(controller.path(), param));
new BufferedReader(new FileReader(Paths.get(controller.path(), param).toString(), StandardCharsets.UTF_8));
try (@SuppressWarnings("removal") BufferedReader bufferedReader =
AccessController.doPrivileged(pea)) {
String line = bufferedReader.readLine();
return line;
} catch (PrivilegedActionException e) {
unwrapIOExceptionAndRethrow(e);
throw new InternalError(e.getCause());
} catch (UncheckedIOException e) {
throw e.getCause();
}
}

@SuppressWarnings("removal")
public static List<String> readAllLinesPrivileged(Path path) throws IOException {
try {
PrivilegedExceptionAction<List<String>> pea = () -> Files.readAllLines(path);
PrivilegedExceptionAction<List<String>> pea = () -> {
try (BufferedReader bufferedReader = new BufferedReader(new FileReader(path.toString(), StandardCharsets.UTF_8))) {
String line;
List<String> lines = new ArrayList<>();
while ((line = bufferedReader.readLine()) != null) {
lines.add(line);
}
return lines;
}
};
return AccessController.doPrivileged(pea);
} catch (PrivilegedActionException e) {
unwrapIOExceptionAndRethrow(e);
throw new InternalError(e.getCause());
} catch (UncheckedIOException e) {
throw e.getCause();
}
}
}
2 changes: 1 addition & 1 deletion src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1SubsystemController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public static long getLongValueMatchingLine(CgroupSubsystemController controller
}

public static long convertHierachicalLimitLine(String line) {
Copy link
Contributor

@jerboaa jerboaa Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing: typo convertHierarchicalLimitLine

String[] tokens = line.split("\\s");
String[] tokens = line.split(" ");
Copy link
Contributor

@jerboaa jerboaa Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, assumes single space ( ) delimited entries in memory.stat. I'm not sure we should hard-code this.

Copy link
Author

@pejovica pejovica Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This delimiter also hasn't changed since the memory.stat file was introduced, and since cgroup v1 is in maintenance mode I'd expect it to stay that way.

if (tokens.length == 2) {
String strVal = tokens[1];
return CgroupV1SubsystemController.convertStringToLong(strVal);
Expand Down
16 changes: 8 additions & 8 deletions src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,9 @@
package jdk.internal.platform.cgroupv2;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.file.Paths;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;
import java.util.stream.Collectors;

import jdk.internal.platform.CgroupInfo;
import jdk.internal.platform.CgroupSubsystem;
Expand Down Expand Up @@ -143,7 +141,7 @@ private long getFromCpuMax(int tokenIdx) {
return CgroupSubsystem.LONG_RETVAL_UNLIMITED;
}
// $MAX $PERIOD
String[] tokens = cpuMaxRaw.split("\\s+");
String[] tokens = cpuMaxRaw.split(" ");
Copy link
Contributor

@jerboaa jerboaa Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems OK. According to https://docs.kernel.org/admin-guide/cgroup-v2.html#format

if (tokens.length != 2) {
return CgroupSubsystem.LONG_RETVAL_UNLIMITED;
}
Expand Down Expand Up @@ -329,10 +327,12 @@ public long getBlkIOServiced() {

private long sumTokensIOStat(Function<String, Long> mapFunc) {
try {
return CgroupUtil.readFilePrivileged(Paths.get(unified.path(), "io.stat"))
.map(mapFunc)
.collect(Collectors.summingLong(e -> e));
} catch (UncheckedIOException | IOException e) {
long sum = 0L;
for (String line : CgroupUtil.readAllLinesPrivileged(Paths.get(unified.path(), "io.stat"))) {
sum += mapFunc.apply(line);
}
return sum;
} catch (IOException e) {
return CgroupSubsystem.LONG_RETVAL_UNLIMITED;
}
}
Expand All @@ -359,7 +359,7 @@ private static Long ioStatLineToLong(String line, String[] matchNames) {
if (line == null || EMPTY_STR.equals(line)) {
return Long.valueOf(0);
}
String[] tokens = line.split("\\s+");
String[] tokens = line.split(" ");
Copy link
Contributor

@jerboaa jerboaa Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems OK. According to https://docs.kernel.org/admin-guide/cgroup-v2.html#format

long retval = 0;
for (String t: tokens) {
String[] valKeys = t.split("=");
Expand Down