[jdk8u-dev] RFR: 8239785: Cgroups: Incorrect detection logic on old systems in hotspot [v5]
Severin Gehwolf
sgehwolf at openjdk.org
Mon Nov 7 14:37:44 UTC 2022
On Fri, 4 Nov 2022 10:25:11 GMT, Jonathan Dowland <jdowland at openjdk.org> wrote:
>> This is a backport of add18914fb1294999877b563c734a25b4c17b922 for cgroups v2 support injdk8u-dev, via the 11u backport.
>>
>> It does not apply clean:
>>
>> * context issues for changes made for the different approach for logging in 8u
>> * copyright lines
>>
>> Small amount of re-working of new code that used `log_trace`/`log_debug` to use the 8u approach.
>
> Jonathan Dowland has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 21 additional commits since the last revision:
>
> - Move test to a more 8u-appropriate location
>
> Container (cgroups, docker) tests in 8u reside in
> hotspot/test/runtime/containers
> - remove duplicate include of osContainer_linux
>
> With the backport of 8189762, the include of osContainer_linux was
> moved to a later #ifdef stanza, relative to the original. This caused
> a context problem with this backport.
> - 8239785: Cgroups: Incorrect detection logic on old systems in hotspot
>
> Return NULL subsystem if no cgroup controllers are mounted.
>
> Reviewed-by: sgehwolf
> Backport-of: add18914fb1294999877b563c734a25b4c17b922
> - 8237479: 8230305 causes slowdebug build failure
>
> Declare methods as pure virtual.
>
> Backport-of: 4ca06995855b5c974321d7b3622d661b8d27ba76
> - 8253714: [cgroups v2] Soft memory limit incorrectly using memory.high
>
> The early implementation of cgroups v2 support was done with
> crun 0.8 and it contained a bug which set memory.high over
> memory.low when --memory-reservation was being used as a CLI
> option.
>
> This bug has been fixed in later crun versions, starting with
> crun 0.11. Use memory.low in OpenJDK as well.
>
> Backport-of: ff6843ca4842498791061f924c545fa9469cc1dc
> - Address style nit
> - TestCgroupSubsystemController: rework use of Files.writeString
> - CgroupSubsystemController: fix library paths
>
> We need the testlibrary copy of FileUtils but the test.lib.util copy of
> Utils (method createTempDirectory is missing from the testlib copy)
> - TestCgroupSubsystemController: fix jtreg @library path
> - Replace Arrays.compare with Arrays.equals
>
> jdk8u does not have Arrays.compare()
> - ... and 11 more: https://git.openjdk.org/jdk8u-dev/compare/26db7880...e4a44241
Did you run the newly added test `hotspot/test/runtime/containers/cgroup/CgroupSubsystemFactory.java`? It didn't work for me and required a patch like this to actually work:
commit 74dbb1af01d367194a3638b52a286909e78c38b6
Author: Severin Gehwolf <sgehwolf at redhat.com>
Date: Mon Nov 7 15:25:27 2022 +0100
Test updates for JDK-8239785
diff --git a/hotspot/test/runtime/containers/cgroup/CgroupSubsystemFactory.java b/hotspot/test/runtime/containers/cgroup/CgroupSubsystemFactory.java
index c08b64dcda..036eae9156 100644
--- a/hotspot/test/runtime/containers/cgroup/CgroupSubsystemFactory.java
+++ b/hotspot/test/runtime/containers/cgroup/CgroupSubsystemFactory.java
@@ -24,9 +24,9 @@
/*
* @test CgroupSubsystemFactory
* @requires os.family == "linux"
- * @library /testlibrary /test/lib
- * @build sun.hotspot.WhiteBox
- * @run driver ClassFileInstaller sun.hotspot.WhiteBox
+ * @library /testlibrary /testlibrary/whitebox
+ * @build CgroupSubsystemFactory
+ * @run main ClassFileInstaller sun.hotspot.WhiteBox
* sun.hotspot.WhiteBox$WhiteBoxPermission
* @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI CgroupSubsystemFactory
*/
@@ -36,10 +36,12 @@ import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
+import java.nio.file.FileVisitResult;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
-import jdk.test.lib.Asserts;
-import jdk.test.lib.Utils;
-import jdk.test.lib.util.FileUtils;
+import com.oracle.java.testlibrary.Asserts;
+import com.oracle.java.testlibrary.Utils;
import sun.hotspot.WhiteBox;
/*
@@ -129,35 +131,35 @@ public class CgroupSubsystemFactory {
try {
existingDirectory = Utils.createTempDirectory(CgroupSubsystemFactory.class.getSimpleName());
Path cgroupsZero = Paths.get(existingDirectory.toString(), "cgroups_zero");
- Files.writeString(cgroupsZero, cgroupsZeroHierarchy, StandardCharsets.UTF_8);
+ Files.write(cgroupsZero, cgroupsZeroHierarchy.getBytes(StandardCharsets.UTF_8));
cgroupv1CgInfoZeroHierarchy = cgroupsZero;
cgroupv2CgInfoZeroHierarchy = cgroupsZero;
cgroupv1MntInfoZeroHierarchy = Paths.get(existingDirectory.toString(), "mountinfo_empty");
- Files.writeString(cgroupv1MntInfoZeroHierarchy, mntInfoEmpty);
+ Files.write(cgroupv1MntInfoZeroHierarchy, mntInfoEmpty.getBytes());
cgroupv2MntInfoZeroHierarchy = Paths.get(existingDirectory.toString(), "mountinfo_cgroupv2");
- Files.writeString(cgroupv2MntInfoZeroHierarchy, mntInfoCgroupsV2Only);
+ Files.write(cgroupv2MntInfoZeroHierarchy, mntInfoCgroupsV2Only.getBytes());
cgroupv1CgInfoNonZeroHierarchy = Paths.get(existingDirectory.toString(), "cgroups_non_zero");
- Files.writeString(cgroupv1CgInfoNonZeroHierarchy, cgroupsNonZeroHierarchy);
+ Files.write(cgroupv1CgInfoNonZeroHierarchy, cgroupsNonZeroHierarchy.getBytes());
cgroupv1MntInfoNonZeroHierarchy = Paths.get(existingDirectory.toString(), "mountinfo_non_zero");
- Files.writeString(cgroupv1MntInfoNonZeroHierarchy, mntInfoHybrid);
+ Files.write(cgroupv1MntInfoNonZeroHierarchy, mntInfoHybrid.getBytes());
cgroupv1MntInfoNonZeroHierarchyOtherOrder = Paths.get(existingDirectory.toString(), "mountinfo_non_zero_cgroupv2_last");
- Files.writeString(cgroupv1MntInfoNonZeroHierarchyOtherOrder, mntInfoHybridFlippedOrder);
+ Files.write(cgroupv1MntInfoNonZeroHierarchyOtherOrder, mntInfoHybridFlippedOrder.getBytes());
cgroupV1SelfCgroup = Paths.get(existingDirectory.toString(), "cgroup_self_hybrid");
- Files.writeString(cgroupV1SelfCgroup, procSelfCgroupHybridContent);
+ Files.write(cgroupV1SelfCgroup, procSelfCgroupHybridContent.getBytes());
cgroupV2SelfCgroup = Paths.get(existingDirectory.toString(), "cgroup_self_v2");
- Files.writeString(cgroupV2SelfCgroup, procSelfCgroupV2UnifiedContent);
+ Files.write(cgroupV2SelfCgroup, procSelfCgroupV2UnifiedContent.getBytes());
cgroupv1MntInfoMissingMemoryController = Paths.get(existingDirectory.toString(), "mnt_info_missing_memory");
- Files.writeString(cgroupv1MntInfoMissingMemoryController, mntInfoHybridMissingMemory);
+ Files.write(cgroupv1MntInfoMissingMemoryController, mntInfoHybridMissingMemory.getBytes());
cgroupV2MntInfoMissingCgroupv2 = Paths.get(existingDirectory.toString(), "mnt_info_missing_cgroup2");
- Files.writeString(cgroupV2MntInfoMissingCgroupv2, mntInfoHybridStub);
+ Files.write(cgroupV2MntInfoMissingCgroupv2, mntInfoHybridStub.getBytes());
} catch (IOException e) {
throw new RuntimeException(e);
}
@@ -165,12 +167,31 @@ public class CgroupSubsystemFactory {
private void teardown() {
try {
- FileUtils.deleteFileTreeWithRetry(existingDirectory);
+ deleteFileTree(existingDirectory);
} catch (IOException e) {
System.err.println("Teardown failed. " + e.getMessage());
}
}
+ private static void deleteFileTree(Path dir) throws IOException {
+ java.nio.file.Files.walkFileTree(dir, new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
+ Files.delete(file);
+ return FileVisitResult.CONTINUE;
+ }
+ @Override
+ public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException {
+ Files.delete(dir);
+ return FileVisitResult.CONTINUE;
+ }
+ @Override
+ public FileVisitResult visitFileFailed(Path file, IOException exc) {
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ }
+
private boolean isValidCgroup(int value) {
return value == CGROUPS_V1 || value == CGROUPS_V2;
}
diff --git a/hotspot/test/testlibrary/com/oracle/java/testlibrary/Utils.java b/hotspot/test/testlibrary/com/oracle/java/testlibrary/Utils.java
index 902e3b1383..0e79599d20 100644
--- a/hotspot/test/testlibrary/com/oracle/java/testlibrary/Utils.java
+++ b/hotspot/test/testlibrary/com/oracle/java/testlibrary/Utils.java
@@ -32,6 +32,10 @@ import java.io.IOException;
import java.net.InetAddress;
import java.net.ServerSocket;
import java.net.UnknownHostException;
+import java.nio.file.Path;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.nio.file.attribute.FileAttribute;
import java.util.ArrayList;
import java.util.List;
import java.util.Arrays;
@@ -370,4 +374,25 @@ public final class Utils {
}
return new String(hexView);
}
+
+ /**
+ * Creates an empty directory in "user.dir" or "."
+ * <p>
+ * This method is meant as a replacement for {@code Files#createTempDirectory(String, String, FileAttribute...)}
+ * that doesn't leave files behind in /tmp directory of the test machine
+ * <p>
+ * If the property "user.dir" is not set, "." will be used.
+ *
+ * @param prefix
+ * @param attrs
+ * @return the path to the newly created directory
+ * @throws IOException
+ *
+ * @see {@link Files#createTempDirectory(String, String, FileAttribute...)}
+ */
+ public static Path createTempDirectory(String prefix, FileAttribute<?>... attrs) throws IOException {
+ Path dir = Paths.get(System.getProperty("user.dir", "."));
+ return Files.createTempDirectory(dir, prefix);
+ }
+
}
diff --git a/hotspot/test/testlibrary/whitebox/sun/hotspot/WhiteBox.java b/hotspot/test/testlibrary/whitebox/sun/hotspot/WhiteBox.java
index 24632c037e..393f3b1ca8 100644
--- a/hotspot/test/testlibrary/whitebox/sun/hotspot/WhiteBox.java
+++ b/hotspot/test/testlibrary/whitebox/sun/hotspot/WhiteBox.java
@@ -256,5 +256,8 @@ public class WhiteBox {
// Container testing
public native boolean isContainerized();
public native void printOsInfo();
+ public native int validateCgroup(String procCgroups,
+ String procSelfCgroup,
+ String procSelfMountinfo);
}
diff --git a/jdk/test/lib/sun/hotspot/WhiteBox.java b/jdk/test/lib/sun/hotspot/WhiteBox.java
index da3a617661..9497c95305 100644
--- a/jdk/test/lib/sun/hotspot/WhiteBox.java
+++ b/jdk/test/lib/sun/hotspot/WhiteBox.java
@@ -443,9 +443,6 @@ public class WhiteBox {
// Container testing
public native boolean isContainerized();
- public native int validateCgroup(String procCgroups,
- String procSelfCgroup,
- String procSelfMountinfo);
public native void printOsInfo();
// Decoder
hotspot/test/runtime/containers/cgroup/CgroupSubsystemFactory.java line 50:
> 48: public class CgroupSubsystemFactory {
> 49:
> 50: // Mirrored from src/hotspot/os/linux/cgroupSubsystem_linux.hpp
The JDK 8u path seems to be `hotspot/src/os/linux/vm/cgroupSubsystem_linux.hpp`. We should update that comment in the backport.
-------------
Changes requested by sgehwolf (Reviewer).
PR: https://git.openjdk.org/jdk8u-dev/pull/135
More information about the jdk8u-dev
mailing list