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.

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
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -245,6 +245,7 @@ class CgroupSubsystem: public CHeapObj<mtInternal> {
virtual int cpu_period() = 0;
virtual int cpu_shares() = 0;
virtual jlong pids_max() = 0;
virtual jlong pids_current() = 0;
virtual jlong memory_usage_in_bytes() = 0;
virtual jlong memory_and_swap_limit_in_bytes() = 0;
virtual jlong memory_soft_limit_in_bytes() = 0;
@@ -266,3 +266,18 @@ jlong CgroupV1Subsystem::pids_max() {
char * pidsmax_str = pids_max_val();
return limit_from_str(pidsmax_str);
}

/* pids_current
*
* The number of tasks currently in the cgroup (and its descendants) of the process
*
* return:
* current number of tasks
* OSCONTAINER_ERROR for not supported
*/
jlong CgroupV1Subsystem::pids_current() {
Copy link
Contributor

@jerboaa jerboaa Sep 9, 2021

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;
GET_CONTAINER_INFO(jlong, _pids, "/pids.current",
"Current number of tasks is: " JLONG_FORMAT, JLONG_FORMAT, pids_current);
return pids_current;
}
@@ -88,6 +88,7 @@ class CgroupV1Subsystem: public CgroupSubsystem {
int cpu_shares();

jlong pids_max();
jlong pids_current();

const char * container_type() {
return "cgroupv1";
@@ -249,3 +249,16 @@ jlong CgroupV2Subsystem::pids_max() {
return limit_from_str(pidsmax_str);
}

/* pids_current
*
* The number of tasks currently in the cgroup (and its descendants) of the process
*
* return:
* current number of tasks
* OSCONTAINER_ERROR for not supported
*/
jlong CgroupV2Subsystem::pids_current() {
Copy link
Contributor

@jerboaa jerboaa Sep 10, 2021

Choose a reason for hiding this comment

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

This should use something akin to memory_usage_in_bytes in this class. pids_current_val() isn't needed.

GET_CONTAINER_INFO(jlong, _unified, "/pids.current",
"Current number of tasks is: " JLONG_FORMAT, JLONG_FORMAT, pids_current);
return pids_current;
}
@@ -80,6 +80,7 @@ class CgroupV2Subsystem: public CgroupSubsystem {
char * cpu_cpuset_cpus();
char * cpu_cpuset_memory_nodes();
jlong pids_max();
jlong pids_current();

const char * container_type() {
return "cgroupv2";
@@ -134,3 +134,8 @@ jlong OSContainer::pids_max() {
assert(cgroup_subsystem != NULL, "cgroup subsystem not available");
return cgroup_subsystem->pids_max();
}

jlong OSContainer::pids_current() {
assert(cgroup_subsystem != NULL, "cgroup subsystem not available");
return cgroup_subsystem->pids_current();
}
@@ -63,6 +63,7 @@ class OSContainer: AllStatic {
static int cpu_shares();

static jlong pids_max();
static jlong pids_current();
};

inline bool OSContainer::is_containerized() {
@@ -2327,6 +2327,16 @@ bool os::Linux::print_container_info(outputStream* st) {
st->print_cr("%s", j == OSCONTAINER_ERROR ? "not supported" : "unlimited");
}

j = OSContainer::OSContainer::pids_current();
st->print("current number of tasks: ");
if (j > 0) {
st->print_cr(JLONG_FORMAT, j);
} else {
if (j == OSCONTAINER_ERROR) {
st->print_cr("not supported");
}
}

return true;
}

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, Red Hat Inc.
* Copyright (c) 2020, 2021, 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
@@ -154,6 +154,11 @@ public long getPidsMax() {
return subsystem.getPidsMax();
}

@Override
public long getPidsCurrent() {
return subsystem.getPidsCurrent();
}

@Override
public long getBlkIOServiceCount() {
return subsystem.getBlkIOServiceCount();
@@ -416,6 +416,10 @@ public long getPidsMax() {
return CgroupSubsystem.limitFromString(pidsMaxStr);
}

public long getPidsCurrent() {
return getLongValue(pids, "pids.current");
}

/*****************************************************************
* BlKIO Subsystem
****************************************************************/
@@ -311,6 +311,11 @@ public long getPidsMax() {
return CgroupSubsystem.limitFromString(pidsMaxStr);
}

@Override
public long getPidsCurrent() {
return getLongVal("pids.current");
}

@Override
public long getBlkIOServiceCount() {
return sumTokensIOStat(CgroupV2Subsystem::lineToRandWIOs);
@@ -365,6 +365,14 @@ public static Metrics systemMetrics() {
*/
public long getPidsMax();

/**
* Returns the current number of tasks in the Isolation Group.
*
* @return The current number of tasks or -2 if not supported
*
*/
public long getPidsCurrent();

/*****************************************************************
* BlKIO Subsystem
****************************************************************/
@@ -106,7 +106,8 @@ private static void checkContainerInfo(OutputAnalyzer out) throws Exception {
"Memory Usage",
"Maximum Memory Usage",
"memory_max_usage_in_bytes",
"maximum number of tasks"
"maximum number of tasks",
"current number of tasks"
};

for (String s : expectedToContain) {
@@ -46,6 +46,8 @@
public class TestPids {
private static final String imageName = Common.imageName("pids");

static final String warning_kernel_no_pids_support = "WARNING: Your kernel does not support pids limit capabilities";

public static void main(String[] args) throws Exception {
if (!DockerTestUtils.canTestDocker()) {
return;
@@ -83,7 +85,7 @@ private static void checkResult(List<String> lines, String lineMarker, String ex
boolean lineMarkerFound = false;

for (String line : lines) {
if (line.contains("WARNING: Your kernel does not support pids limit capabilities")) {
if (line.contains(warning_kernel_no_pids_support)) {
System.out.println("Docker pids limitation seems not to work, avoiding check");
return;
}
@@ -93,6 +95,17 @@ private static void checkResult(List<String> lines, String lineMarker, String ex
String[] parts = line.split(":");
System.out.println("DEBUG: line = " + line);
System.out.println("DEBUG: parts.length = " + parts.length);
if (expectedValue.equals("any_integer")) {
Asserts.assertEquals(parts.length, 2);
Copy link
Member

@iklam iklam Sep 14, 2021

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

@iklam iklam Sep 14, 2021

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

@jerboaa jerboaa Sep 14, 2021

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

@MBaesken MBaesken Sep 15, 2021

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

@MBaesken MBaesken Sep 15, 2021

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","");
try {
int ai = Integer.parseInt(ivalue);
System.out.println("Found " + lineMarker + " with value: " + ai + ". PASS.");
} catch (NumberFormatException ex) {
throw new RuntimeException("Could not convert " + ivalue + " to an integer, log line was " + line);
}
break;
}

Asserts.assertEquals(parts.length, 2);
String actual = parts[1].replaceAll("\\s","");
@@ -137,6 +150,8 @@ private static void testPids(String value) throws Exception {
} else {
checkResult(lines, "Maximum number of tasks is: ", value);
}
// current number of tasks value is hard to predict, so better expect no value
checkResult(lines, "Current number of tasks is: ", "any_integer");
}

}