[jdk8u-dev] RFR: 8239785: Cgroups: Incorrect detection logic on old systems in hotspot [v5]
Severin Gehwolf
sgehwolf at openjdk.org
Fri Dec 9 11:07:03 UTC 2022
On Mon, 7 Nov 2022 15:38:47 GMT, Jonathan Dowland <jdowland at openjdk.org> wrote:
>> 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
>
>> Did you run the newly added test hotspot/test/runtime/containers/cgroup/CgroupSubsystemFactory.java?
>
> I guess not. I began the patch series running all the touched tests, but since review wasn't going to start for a while, and also some tests are known-broken until later patches are applied, I decided to focus on the non-test hunks and watch the status of test pass/fail via GHA on the "tip" of the backport effort.
>
> Now that review has started I should switch to paying attention to them!
>
>> It didn't work for me and required a patch like this to actually work:
>
> Thanks for fixing this one. I'll look at the rest downstream of this PR.
@jmtd Please add approval requests to the bug.
-------------
PR: https://git.openjdk.org/jdk8u-dev/pull/135
More information about the jdk8u-dev
mailing list