Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
8293540: [Metrics] Incorrectly detected resource limits with addition… 
…al cgroup fs mounts

Reviewed-by: iklam
  • Loading branch information
@jerboaa
jerboaa committed Sep 30, 2022
1 parent 6974978 commit 6d83482
Show file tree
Hide file tree
Showing 6 changed files with 158 additions and 38 deletions.
44 changes: 19 additions & 25 deletions src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
View file
Expand Up @@ -318,30 +318,11 @@ private static boolean amendCgroupInfos(String mntInfoLine,
case MEMORY_CTRL: // fall-through
case CPU_CTRL:
case CPUACCT_CTRL:
case CPUSET_CTRL:
case PIDS_CTRL:
case BLKIO_CTRL: {
CgroupInfo info = infos.get(controllerName);
assert info.getMountPoint() == null;
assert info.getMountRoot() == null;
info.setMountPoint(mountPath);
info.setMountRoot(mountRoot);
cgroupv1ControllerFound = true;
break;
}
case CPUSET_CTRL: {
CgroupInfo info = infos.get(controllerName);
if (info.getMountPoint() != null) {
// On some systems duplicate cpuset controllers get mounted in addition to
// the main cgroup controllers most likely under /sys/fs/cgroup. In that
// case pick the one under /sys/fs/cgroup and discard others.
if (!info.getMountPoint().startsWith("/sys/fs/cgroup")) {
info.setMountPoint(mountPath);
info.setMountRoot(mountRoot);
}
} else {
info.setMountPoint(mountPath);
info.setMountRoot(mountRoot);
}
setMountPoints(info, mountPath, mountRoot);
cgroupv1ControllerFound = true;
break;
}
Expand All @@ -355,10 +336,7 @@ private static boolean amendCgroupInfos(String mntInfoLine,
// All controllers have the same mount point and root mount
// for unified hierarchy.
for (CgroupInfo info: infos.values()) {
assert info.getMountPoint() == null;
assert info.getMountRoot() == null;
info.setMountPoint(mountPath);
info.setMountRoot(mountRoot);
setMountPoints(info, mountPath, mountRoot);
}
}
cgroupv2ControllerFound = true;
Expand All @@ -367,6 +345,22 @@ private static boolean amendCgroupInfos(String mntInfoLine,
return cgroupv1ControllerFound || cgroupv2ControllerFound;
}

private static void setMountPoints(CgroupInfo info, String mountPath, String mountRoot) {
if (info.getMountPoint() != null) {
// On some systems duplicate controllers get mounted in addition to
// the main cgroup controllers (which are under /sys/fs/cgroup). In that
// case pick the main one and discard others as the limits
// are associated with the ones in /sys/fs/cgroup.
if (!info.getMountPoint().startsWith("/sys/fs/cgroup")) {
info.setMountPoint(mountPath);
info.setMountRoot(mountRoot);
}
} else {
info.setMountPoint(mountPath);
info.setMountRoot(mountRoot);
}
}

public static final class CgroupTypeResult {
private final boolean isCgroupV2;
private final boolean anyControllersEnabled;
Expand Down
15 changes: 15 additions & 0 deletions test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java
View file
Expand Up @@ -87,6 +87,11 @@ public static void main(String[] args) throws Exception {
"1G", Integer.toString(((int) Math.pow(2, 20)) * 1024),
"1500M", Integer.toString(((int) Math.pow(2, 20)) * (1500 - 1024))
);
testOperatingSystemMXBeanAwareness(
"100M", Integer.toString(((int) Math.pow(2, 20)) * 100),
"200M", Integer.toString(((int) Math.pow(2, 20)) * (200 - 100)),
true /* additional cgroup fs mounts */
);
final String hostMaxMem = getHostMaxMemory();
testOperatingSystemMXBeanIgnoresMemLimitExceedingPhysicalMemory(hostMaxMem);
testMetricsIgnoresMemLimitExceedingPhysicalMemory(hostMaxMem);
Expand Down Expand Up @@ -170,6 +175,12 @@ private static void testOOM(String dockerMemLimit, int sizeToAllocInMb) throws E

private static void testOperatingSystemMXBeanAwareness(String memoryAllocation, String expectedMemory,
String swapAllocation, String expectedSwap) throws Exception {
testOperatingSystemMXBeanAwareness(memoryAllocation, expectedMemory, swapAllocation, expectedSwap, false);
}

private static void testOperatingSystemMXBeanAwareness(String memoryAllocation, String expectedMemory,
String swapAllocation, String expectedSwap, boolean addCgroupMounts) throws Exception {

Common.logNewTestCase("Check OperatingSystemMXBean");

DockerRunOptions opts = Common.newOpts(imageName, "CheckOperatingSystemMXBean")
Expand All @@ -181,6 +192,10 @@ private static void testOperatingSystemMXBeanAwareness(String memoryAllocation,
// diagnostics
.addJavaOpts("--add-exports")
.addJavaOpts("java.base/jdk.internal.platform=ALL-UNNAMED");
if (addCgroupMounts) {
// Extra cgroup mount should be ignored by product code
opts.addDockerOpts("--volume", "/sys/fs/cgroup:/cgroup-in:ro");
}

OutputAnalyzer out = DockerTestUtils.dockerRunJava(opts);
out.shouldHaveExitValue(0)
Expand Down
37 changes: 24 additions & 13 deletions test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java
View file
Expand Up @@ -51,7 +51,7 @@

/*
* @test
* @bug 8287107 8287073
* @bug 8287107 8287073 8293540
* @key cgroups
* @requires os.family == "linux"
* @modules java.base/jdk.internal.platform
Expand All @@ -72,8 +72,8 @@ public class TestCgroupSubsystemFactory {
private Path cgroupv1CgInfoNonZeroHierarchy;
private Path cgroupv1MntInfoNonZeroHierarchy;
private Path cgroupv1MntInfoSystemdOnly;
private Path cgroupv1MntInfoDoubleCpusets;
private Path cgroupv1MntInfoDoubleCpusets2;
private Path cgroupv1MntInfoDoubleControllers;
private Path cgroupv1MntInfoDoubleControllers2;
private Path cgroupv1MntInfoColonsHierarchy;
private Path cgroupv1SelfCgroup;
private Path cgroupv1SelfColons;
Expand Down Expand Up @@ -194,9 +194,13 @@ public class TestCgroupSubsystemFactory {
private String mntInfoCgroupsV1SystemdOnly =
"35 26 0:26 / /sys/fs/cgroup/systemd rw,nosuid,nodev,noexec,relatime - cgroup systemd rw,name=systemd\n" +
"26 18 0:19 / /sys/fs/cgroup rw,relatime - tmpfs none rw,size=4k,mode=755\n";
private String mntInfoCgroupv1MoreCpusetLine = "121 32 0:37 / /cpuset rw,relatime shared:69 - cgroup none rw,cpuset\n";
private String mntInfoCgroupsV1DoubleCpuset = mntInfoHybrid + mntInfoCgroupv1MoreCpusetLine;
private String mntInfoCgroupsV1DoubleCpuset2 = mntInfoCgroupv1MoreCpusetLine + mntInfoHybrid;
private String mntInfoCgroupv1MoreControllers = "121 32 0:37 / /cpuset rw,relatime shared:69 - cgroup none rw,cpuset\n" +
"35 30 0:31 / /cgroup-in/memory rw,nosuid,nodev,noexec,relatime shared:7 - cgroup none rw,seclabel,memory\n" +
"36 30 0:32 / /cgroup-in/pids rw,nosuid,nodev,noexec,relatime shared:8 - cgroup none rw,seclabel,pids\n" +
"40 30 0:36 / /cgroup-in/cpu,cpuacct rw,nosuid,nodev,noexec,relatime shared:12 - cgroup none rw,seclabel,cpu,cpuacct\n" +
"40 30 0:36 / /cgroup-in/blkio rw,nosuid,nodev,noexec,relatime shared:12 - cgroup none rw,seclabel,blkio\n";
private String mntInfoCgroupsV1DoubleControllers = mntInfoHybrid + mntInfoCgroupv1MoreControllers;
private String mntInfoCgroupsV1DoubleControllers2 = mntInfoCgroupv1MoreControllers + mntInfoHybrid;
private String cgroupv1SelfCgroupContent = "11:memory:/user.slice/user-1000.slice/user@1000.service\n" +
"10:hugetlb:/\n" +
"9:cpuset:/\n" +
Expand Down Expand Up @@ -275,11 +279,11 @@ public void setup() {
cgroupv1MntInfoSystemdOnly = Paths.get(existingDirectory.toString(), "mountinfo_cgroupv1_systemd_only");
Files.writeString(cgroupv1MntInfoSystemdOnly, mntInfoCgroupsV1SystemdOnly);

cgroupv1MntInfoDoubleCpusets = Paths.get(existingDirectory.toString(), "mountinfo_cgroupv1_double_cpuset");
Files.writeString(cgroupv1MntInfoDoubleCpusets, mntInfoCgroupsV1DoubleCpuset);
cgroupv1MntInfoDoubleControllers = Paths.get(existingDirectory.toString(), "mountinfo_cgroupv1_double_controllers");
Files.writeString(cgroupv1MntInfoDoubleControllers, mntInfoCgroupsV1DoubleControllers);

cgroupv1MntInfoDoubleCpusets2 = Paths.get(existingDirectory.toString(), "mountinfo_cgroupv1_double_cpuset2");
Files.writeString(cgroupv1MntInfoDoubleCpusets2, mntInfoCgroupsV1DoubleCpuset2);
cgroupv1MntInfoDoubleControllers2 = Paths.get(existingDirectory.toString(), "mountinfo_cgroupv1_double_controllers2");
Files.writeString(cgroupv1MntInfoDoubleControllers2, mntInfoCgroupsV1DoubleControllers2);

cgroupv1CgroupsJoinControllers = Paths.get(existingDirectory.toString(), "cgroups_cgv1_join_controllers");
Files.writeString(cgroupv1CgroupsJoinControllers, cgroupsNonZeroJoinControllers);
Expand Down Expand Up @@ -390,11 +394,11 @@ public void testCgroupv1SystemdOnly() throws IOException {

@Test
public void testCgroupv1MultipleCpusetMounts() throws IOException {
doMultipleCpusetMountsTest(cgroupv1MntInfoDoubleCpusets);
doMultipleCpusetMountsTest(cgroupv1MntInfoDoubleCpusets2);
doMultipleMountsTest(cgroupv1MntInfoDoubleControllers);
doMultipleMountsTest(cgroupv1MntInfoDoubleControllers2);
}

private void doMultipleCpusetMountsTest(Path info) throws IOException {
private void doMultipleMountsTest(Path info) throws IOException {
String cgroups = cgroupv1CgInfoNonZeroHierarchy.toString();
String mountInfo = info.toString();
String selfCgroup = cgroupv1SelfCgroup.toString();
Expand All @@ -406,6 +410,13 @@ private void doMultipleCpusetMountsTest(Path info) throws IOException {
CgroupInfo cpuSetInfo = res.getInfos().get("cpuset");
assertEquals("/sys/fs/cgroup/cpuset", cpuSetInfo.getMountPoint());
assertEquals("/", cpuSetInfo.getMountRoot());
// Ensure controllers at /sys/fs/cgroup will be used
String[] ctrlNames = new String[] { "memory", "cpu", "cpuacct", "blkio", "pids" };
for (int i = 0; i < ctrlNames.length; i++) {
CgroupInfo cinfo = res.getInfos().get(ctrlNames[i]);
assertTrue(cinfo.getMountPoint().startsWith("/sys/fs/cgroup/"));
assertEquals("/", cinfo.getMountRoot());
}
}

@Test
Expand Down
72 changes: 72 additions & 0 deletions test/jdk/jdk/internal/platform/docker/TestDockerBasic.java
View file
@@ -0,0 +1,72 @@
/*
* Copyright (c) 2022, Red Hat, Inc.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/*
* @test
* @bug 8293540
* @summary Verify that -XshowSettings:system works
* @requires docker.support
* @library /test/lib
* @run main/timeout=360 TestDockerBasic
*/

import jdk.test.lib.Utils;
import jdk.test.lib.containers.docker.Common;
import jdk.test.lib.containers.docker.DockerRunOptions;
import jdk.test.lib.containers.docker.DockerTestUtils;

public class TestDockerBasic {
private static final String imageName = Common.imageName("javaDockerBasic");

public static void main(String[] args) throws Exception {
if (!DockerTestUtils.canTestDocker()) {
return;
}

DockerTestUtils.buildJdkContainerImage(imageName);

try {
testXshowSettingsSystem(true);
testXshowSettingsSystem(false);
} finally {
DockerTestUtils.removeDockerImage(imageName);
}
}

private static void testXshowSettingsSystem(boolean addCgroupMounts) throws Exception {
String testMsg = (addCgroupMounts ? " with " : " without ") + " additional cgroup FS mounts in /cgroup-in";
Common.logNewTestCase("Test TestDockerBasic " + testMsg);
DockerRunOptions opts =
new DockerRunOptions(imageName, "/jdk/bin/java", "-version");
opts.addJavaOpts("-esa");
opts.addJavaOpts("-XshowSettings:system");
opts.addDockerOpts("--memory", "300m");
if (addCgroupMounts) {
// Extra cgroup mount should be ignored by product code
opts.addDockerOpts("--volume", "/sys/fs/cgroup:/cgroup-in:ro");
}
DockerTestUtils.dockerRunJava(opts).shouldHaveExitValue(0)
.shouldNotContain("AssertionError")
.shouldContain("Memory Limit: 300.00M");
}
}
18 changes: 18 additions & 0 deletions test/jdk/jdk/internal/platform/docker/TestDockerCpuMetrics.java
View file
Expand Up @@ -65,11 +65,13 @@ public static void main(String[] args) throws Exception {
testCpuSet((((numCpus - 1) / 2) + 1) + "-" + (numCpus - 1));
}
testCpuSet(IntStream.range(0, numCpus).mapToObj(a -> Integer.toString(a)).collect(Collectors.joining(",")));
testCpuSet("0", true /* additional cgroup fs mount */);

testCpuQuota(50 * 1000, 100 * 1000);
testCpuQuota(100 * 1000, 100 * 1000);
testCpuQuota(150 * 1000, 100 * 1000);
testCpuQuota(400 * 1000, 100 * 1000);
testCpuQuota(200 * 1000, 100 * 1000, true /* additional cgroup fs mount */);

testCpuShares(256);
testCpuShares(2048);
Expand Down Expand Up @@ -108,10 +110,18 @@ private static void testCpuSetMems(String value) throws Exception {
}

private static void testCpuSet(String value) throws Exception {
testCpuSet(value, false);
}

private static void testCpuSet(String value, boolean addCgroupMount) throws Exception {
Common.logNewTestCase("testCpuSet, value = " + value);
DockerRunOptions opts =
new DockerRunOptions(imageName, "/jdk/bin/java", "MetricsCpuTester");
opts.addDockerOpts("--volume", Utils.TEST_CLASSES + ":/test-classes/");
if (addCgroupMount) {
// Extra cgroup mount should be ignored by product code
opts.addDockerOpts("--volume", "/sys/fs/cgroup:/cgroup-in:ro");
}
opts.addJavaOpts("-cp", "/test-classes/");
opts.addJavaOpts("--add-exports", "java.base/jdk.internal.platform=ALL-UNNAMED");
opts.addClassOptions("cpusets", value);
Expand All @@ -120,10 +130,18 @@ private static void testCpuSet(String value) throws Exception {
}

private static void testCpuQuota(long quota, long period) throws Exception {
testCpuQuota(quota, period, false);
}

private static void testCpuQuota(long quota, long period, boolean addCgroupMount) throws Exception {
Common.logNewTestCase("testCpuQuota, quota = " + quota + ", period = " + period);
DockerRunOptions opts =
new DockerRunOptions(imageName, "/jdk/bin/java", "MetricsCpuTester");
opts.addDockerOpts("--volume", Utils.TEST_CLASSES + ":/test-classes/");
if (addCgroupMount) {
// Extra cgroup mount should be ignored by product code
opts.addDockerOpts("--volume", "/sys/fs/cgroup:/cgroup-in:ro");
}
opts.addDockerOpts("--cpu-period=" + period).addDockerOpts("--cpu-quota=" + quota);
opts.addJavaOpts("-cp", "/test-classes/").addJavaOpts("--add-exports", "java.base/jdk.internal.platform=ALL-UNNAMED");
opts.addClassOptions("cpuquota", quota + "", period + "");
Expand Down
10 changes: 10 additions & 0 deletions test/jdk/jdk/internal/platform/docker/TestDockerMemoryMetrics.java
View file
Expand Up @@ -56,6 +56,8 @@ public static void main(String[] args) throws Exception {
try {
testMemoryLimit("200m");
testMemoryLimit("1g");
// Memory limit test with additional cgroup fs mounted
testMemoryLimit("500m", true /* cgroup fs mount */);

testMemoryAndSwapLimit("200m", "1g");
testMemoryAndSwapLimit("100m", "200m");
Expand Down Expand Up @@ -85,6 +87,10 @@ public static void main(String[] args) throws Exception {
}

private static void testMemoryLimit(String value) throws Exception {
testMemoryLimit(value, false);
}

private static void testMemoryLimit(String value, boolean addCgroupMount) throws Exception {
Common.logNewTestCase("testMemoryLimit, value = " + value);
DockerRunOptions opts =
new DockerRunOptions(imageName, "/jdk/bin/java", "MetricsMemoryTester");
Expand All @@ -93,6 +99,10 @@ private static void testMemoryLimit(String value) throws Exception {
.addJavaOpts("-cp", "/test-classes/")
.addJavaOpts("--add-exports", "java.base/jdk.internal.platform=ALL-UNNAMED")
.addClassOptions("memory", value);
if (addCgroupMount) {
// Extra cgroup mount should be ignored by product code
opts.addDockerOpts("--volume", "/sys/fs/cgroup:/cgroup-in:ro");
}
DockerTestUtils.dockerRunJava(opts).shouldHaveExitValue(0).shouldContain("TEST PASSED!!!");
}

Expand Down

3 comments on commit 6d83482

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Review

Issues

@jerboaa
Copy link
Contributor Author

Choose a reason for hiding this comment

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

/backport jdk17u-dev

@openjdk
Copy link

@openjdk openjdk bot commented on 6d83482 Oct 12, 2022

Choose a reason for hiding this comment

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

@jerboaa the backport was successfully created on the branch jerboaa-backport-6d83482a in my personal fork of openjdk/jdk17u-dev. To create a pull request with this backport targeting openjdk/jdk17u-dev:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 6d83482a from the openjdk/jdk repository.

The commit being backported was authored by Severin Gehwolf on 30 Sep 2022 and was reviewed by Ioi Lam.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk17u-dev:

$ git fetch https://github.com/openjdk-bots/jdk17u-dev jerboaa-backport-6d83482a:jerboaa-backport-6d83482a
$ git checkout jerboaa-backport-6d83482a
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk17u-dev jerboaa-backport-6d83482a

Please sign in to comment.