Skip to content
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

JDK-8273526: Extend the OSContainer API pids controller with pids.current #5437

Closed
wants to merge 4 commits into from
+88 −3
Closed

JDK-8273526: Extend the OSContainer API pids controller with pids.current #5437

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa93a12
JDK-8273526
MBaesken Sep 9, 2021
422aef6
Some simplifications and test adjustment suggested by Severin
MBaesken Sep 10, 2021
afe0efd
Simplify coding following Severins advice
MBaesken Sep 14, 2021
86f3f53
Adjust TestPids
MBaesken Sep 15, 2021
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
5 changes: 3 additions & 2 deletions src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp
View file
Original file line number Diff line number Diff line change
@@ -286,6 +286,7 @@ jlong CgroupV1Subsystem::pids_max() {
*/
jlong CgroupV1Subsystem::pids_current() {
Copy link
Contributor

Choose a reason for hiding this comment

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

pids.current never contains a string max (for unlimited). Therefore, we shouldn't need to do the pids_current_val -> limit_from_str() trick. We should be able to use GET_CONTAINER_INFO(int, [...] akin to cpu_quota. int or long would both be suitable. Up to you to decide which data type to use. I don't think it'll ever be beyond the maximum integrer value.

if (_pids == NULL) return OSCONTAINER_ERROR;
char * pidscurrent_str = pids_current_val();
return limit_from_str(pidscurrent_str);
GET_CONTAINER_INFO(jlong, _pids, "/pids.current",
"Current number of tasks is: " JLONG_FORMAT, JLONG_FORMAT, pids_current);
return pids_current;
}
3 changes: 1 addition & 2 deletions src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1Subsystem.java
View file
Original file line number Diff line number Diff line change
@@ -417,8 +417,7 @@ public long getPidsMax() {
}

public long getPidsCurrent() {
String pidsCurrentStr = CgroupSubsystemController.getStringValue(pids, "pids.current");
return CgroupSubsystem.limitFromString(pidsCurrentStr);
return getLongValue(pids, "pids.current");
}

/*****************************************************************
9 changes: 8 additions & 1 deletion test/hotspot/jtreg/containers/docker/TestPids.java
View file
Original file line number Diff line number Diff line change
@@ -94,7 +94,14 @@ private static void checkResult(List<String> lines, String lineMarker, String ex
System.out.println("DEBUG: line = " + line);
System.out.println("DEBUG: parts.length = " + parts.length);
if (expectedValue.equals("no_value_expected")) {
System.out.println("No value expected for " + lineMarker);
Asserts.assertEquals(parts.length, 2);
Copy link
Member

Choose a reason for hiding this comment

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

Is "no_value_expected" generated by Docker? I searched the entire HotSpot source code and couldn't find it. I also couldn't find "WARNING: Your kernel does not support pids limit capabilities".

To make it easier to understand this test, I would suggest grouping all messages that were generated outside of HotSpot into something like:

// These messages are generated by Docker
static final String warning_kernel_no_pids_support = "WARNING: Your kernel does not support pids limit capabilities";
static final String no_value_expected = "no_value_expected";

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I misunderstood the test. "no_value_expected" was passed in from testPids() in this file, but that's confusing because you are expecting an integer value. Maybe it should be "any_integer"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it should be "any_integer"?

+1

Copy link
Member Author

Choose a reason for hiding this comment

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

Is "no_value_expected" generated by Docker? I searched the entire HotSpot source code and couldn't find it. I also couldn't find "WARNING: Your kernel does not support pids limit capabilities".

Hi, this warning is showing up on some of our Linux ppc64le machines where the pids limit capabilities is not supported.

Best regards, Matthias

Copy link
Member Author

Choose a reason for hiding this comment

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

Hello, I adjusted to any_integer, and introduced the "final String warning_kernel_no_pids_support" . I think the message is more related to the kernel features so I did not add the add the 'generated by Docker' comment.

Best regards, Matthias

String ivalue = parts[1].replaceAll("\\s","");
System.out.println("Found " + lineMarker + " with value: " + ivalue);
try {
int ai = Integer.parseInt(ivalue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move the debug print line to after line 101, please. It could say:

System.out.println("Found " + lineMarker + " with value: " + ai + ". PASS.");

Copy link
Member Author

Choose a reason for hiding this comment

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

the debug println has been moved down.

} catch (NumberFormatException ex) {
throw new RuntimeException("Could not convert " + ivalue + " to an integer, log line was " + line);
}
break;
}