JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups #4518

MBaesken · 2021-06-17T12:27:25Z

Hello, please review this PR; it extend the OSContainer API in order to also support the pids controller of cgroups.

I noticed that unlike the other controllers "cpu", "cpuset", "cpuacct", "memory" on some older Linux distros (SLES 12.1, RHEL 7.1) the pids controller might not be there (or not fully supported) so it was added as optional , see the coding

  if (!cg_infos[PIDS_IDX]._data_complete) {
    log_debug(os, container)("Optional cgroup v1 pids subsystem not found");
    // keep the other controller info, pids is optional
  }

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups

Reviewers

Severin Gehwolf (@jerboaa - Reviewer) ⚠️ Review applies to 0dd58e0
Lutz Schmidt (@RealLucy - Reviewer) ⚠️ Review applies to 0dd58e0

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4518/head:pull/4518
$ git checkout pull/4518

Update a local copy of the PR:
$ git checkout pull/4518
$ git pull https://git.openjdk.java.net/jdk pull/4518/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4518

View PR using the GUI difftool:
$ git pr show -t 4518

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4518.diff

bridgekeeper · 2021-06-17T12:28:29Z

👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-06-17T12:30:40Z

@MBaesken The following labels will be automatically applied to this pull request:

core-libs
hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-06-17T12:34:34Z

Webrevs

jerboaa

Thanks for this work. How did you test this? Did you run container tests on a cgroups v1 and cgroups v2 system?

jerboaa · 2021-06-17T13:21:35Z

src/hotspot/os/linux/cgroupSubsystem_linux.cpp

@@ -122,9 +133,10 @@ bool CgroupSubsystemFactory::determine_type(CgroupInfo* cg_infos,
  char buf[MAXPATHLEN+1];
  char *p;
  bool is_cgroupsV2;
-  // true iff all controllers, memory, cpu, cpuset, cpuacct are enabled
+  // true iff all required controllers, memory, cpu, cpuset, cpuacct enabled


are enabled, please.

jerboaa · 2021-06-17T13:25:19Z

src/hotspot/os/linux/cgroupSubsystem_linux.cpp

+      is_cgroupsV2 = is_cgroupsV2 && cg_infos[i]._hierarchy_id == 0;
+      all_required_controllers_enabled = all_required_controllers_enabled && cg_infos[i]._enabled;
+    }
+    if (! cg_infos[i]._enabled) {


This if is only present for debug logging and should be guarded to that effect.

jerboaa · 2021-06-17T13:28:12Z

src/hotspot/os/linux/cgroupSubsystem_linux.cpp

  for (int i = 0; i < CG_INFO_LENGTH; i++) {
-    is_cgroupsV2 = is_cgroupsV2 && cg_infos[i]._hierarchy_id == 0;
-    all_controllers_enabled = all_controllers_enabled && cg_infos[i]._enabled;
+    // the pids controller is not there on older Linux distros


Suggestion: Change the code comment to // pids controller is optional. All other controllers are required

jerboaa · 2021-06-17T13:30:38Z

src/hotspot/os/linux/cgroupSubsystem_linux.cpp

@@ -387,10 +421,13 @@ bool CgroupSubsystemFactory::determine_type(CgroupInfo* cg_infos,
    *flags = INVALID_CGROUPS_V1;
    return false;
  }
+  if (!cg_infos[PIDS_IDX]._data_complete) {


Same here, this if should be guarded with debug logging being enabled.

jerboaa · 2021-06-17T13:49:30Z

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp

+    int err2;
+    err2 = subsystem_file_line_contents(_pids, "/pids.max", NULL, "%1023s", myline);
+    if (err2 != 0) {
+      if (strncmp(myline, "max", 3) == 0) return -3;


We use -1 for "unlimited" elsewhere and should probably do the same here.

jerboaa · 2021-06-17T14:09:04Z

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp

+    char myline[1024];
+    int err2;
+    err2 = subsystem_file_line_contents(_pids, "/pids.max", NULL, "%1023s", myline);
+    if (err2 != 0) {
+      if (strncmp(myline, "max", 3) == 0) return -3;


This looks like it should use GET_CONTAINER_INFO_CPTR macro and then limit_from_str from cgroups v2 code. Perhaps move limit_from_str method to the base class.

jerboaa · 2021-06-17T14:11:00Z

src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp

+jlong CgroupV2Subsystem::pids_max() {
+  // we have to handle the special "max" value
+  GET_CONTAINER_INFO(jlong, _unified, "/pids.max",
+                     "Maximum number of tasks is: " JLONG_FORMAT, JLONG_FORMAT, pidsmax);
+  // not a number -> could be "max"
+  if (pidsmax < 0) {
+    char myline[1024];
+    int err2;
+    err2 = subsystem_file_line_contents(_unified, "/pids.max", NULL, "%1023s", myline);
+    if (err2 != 0) {
+      if (strncmp(myline, "max", 3) == 0) return -3;
+    }
+  }
+  return pidsmax;


We have this pattern of needing to handle max elsewhere in cgroups v2 code. See for example: CgroupV2Subsystem::cpu_quota(). We should handle it similarly here.

jerboaa · 2021-06-17T14:14:53Z

src/hotspot/os/linux/os_linux.cpp

+    if (j == -3) {
+      st->print_cr("max");
+    } else {
+      st->print_cr("%s", j == OSCONTAINER_ERROR ? "not supported" : "unlimited");


We should treat the unlimited case similar to how we handle them elsewhere. I'm not sure this magic constant of -3 gives us any more info that we'd get with -1 that we use elsewhere.

jerboaa · 2021-06-17T14:21:50Z

src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1Subsystem.java

+     *  pids subsystem
+     ****************************************************************/
+    public long getPidsMax() {
+        return CgroupV1SubsystemController.longValOrUnlimited(getLongValue(pids, "pids.max"));


Since this value may be max we should use the same logic than for v2. I.e.:

String pidsMaxStr = CgroupSubsystemController.getStringValue(pids, "pids.max"); return CgroupSubsystemController.limitFromString(pidsMaxStr);

jerboaa · 2021-06-17T14:28:41Z

test/hotspot/jtreg/containers/cgroup/CgroupSubsystemFactory.java

@@ -168,7 +169,7 @@
            "perf_event  4   1   1\n" +
            "net_prio    5   1   1\n" +
            "hugetlb 6   1   1\n" +
-            "pids    3   80  1";
+            "pids    9   80  1";  //  the 3 did not match 9


This comment leaves the reader none the wiser. I think you are alluding to controller id matching between /proc/cgroups and /proc/self/cgroup. If so, please use that info.

MBaesken · 2021-06-23T13:38:58Z

Hi Severin , thanks for all the comments. I prepared a second version with those changes
added a couple of log_is_enabled checks like you suggested
moved limit_from_str to CgroupSubsystem
added helpers pids_max_val() and swicthed to GET_CONTAINER_INFO_CPTR
pids_max() now returns -1 for unlimited/max , and the -3 is gone
moved limitFromString java coding to src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystem.java
added a better comment to test/hotspot/jtreg/containers/cgroup/CgroupSubsystemFactory.java about pids hiearchy values

Regarding your questions about tests, I run the exisiting docker/cgroup related tests; and also checked
the hs_err output (on SLES/Ubuntu) for new added "maximum number of tasks" (this is present because systemd cgroup usage).
But I think that the testing needs to be enhanced (e.g. with some added docker tests?). Do you have some good suggestions
where I could look at existing (docker?) tests and adjust those for the new pids.max ?

jerboaa · 2021-06-23T14:48:22Z

But I think that the testing needs to be enhanced (e.g. with some added docker tests?). Do you have some good suggestions
where I could look at existing (docker?) tests and adjust those for the new pids.max ?

Have a look at test/hotspot/jtreg/containers/docker/TestMisc.java which already does some assertions on print_container_info() output. Either extend that test with some actual pid limits (--pids-limit= option) in place or write a similar one. That would cover the hotspot side.

Then consider adding the pids limit to the -Xshowsettings:system output (see LauncherHelper.printSystemMetrics()) using the Java API and add a docker test using that in test/jdk/jdk/internal/platform/docker/.

MBaesken · 2021-06-24T08:57:21Z

But I think that the testing needs to be enhanced (e.g. with some added docker tests?). Do you have some good suggestions
where I could look at existing (docker?) tests and adjust those for the new pids.max ?

Have a look at test/hotspot/jtreg/containers/docker/TestMisc.java which already does some assertions on print_container_info() output. Either extend that test with some actual pid limits (--pids-limit= option) in place or write a similar one. That would cover the hotspot side.

Then consider adding the pids limit to the -Xshowsettings:system output (see LauncherHelper.printSystemMetrics()) using the Java API and add a docker test using that in test/jdk/jdk/internal/platform/docker/.

Hi Severin, thanks for the suggestions .
I'll have a look.

Best regards, Matthias

jerboaa

This looks pretty good now. Looking forward to seeing container tests for this new code.

jerboaa · 2021-06-29T08:05:46Z

src/hotspot/os/linux/cgroupSubsystem_linux.cpp

+  if (limit_str == NULL) {
+    return OSCONTAINER_ERROR;
+  }
+  // Unlimited memory in Cgroups V2 is the literal string 'max'


Please don't add version specific comments to version agnostic code. Suggestion: "Unlimited memory in cgroups is the literal string 'max' for some controllers, for example the pids controller."

jerboaa · 2021-06-29T08:13:41Z

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp

+  jlong pidsmax = limit_from_str(pidsmax_str);
+  return pidsmax;


Do we need this local variable? Consider using return limit_from_str(pidsmax_str); instead.

jerboaa · 2021-06-29T08:15:02Z

src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp

+  jlong pidsmax = limit_from_str(pidsmax_str);
+  return pidsmax;


Same here. Use return limit_from_str(pidsmax_str);

MBaesken · 2021-07-09T11:35:34Z

This looks pretty good now. Looking forward to seeing container tests for this new code.

Hi Severin , I did some adjustments following your suggestions.
I added docker based test coding for testing pids-limit (with limits and also with unlimited value).
I noticed that on our ppc64le based Linux , the message "WARNING: Your kernel does not support pids limit capabilities or the cgroup is not mounted. PIDs limit discarded." shows up , and the docker "--pids-limit" limitation does not work because of this.
So I had to take this into account.

jerboaa · 2021-07-09T13:42:15Z

This looks pretty good now. Looking forward to seeing container tests for this new code.

Hi Severin , I did some adjustments following your suggestions.
I added docker based test coding for testing pids-limit (with limits and also with unlimited value).
I noticed that on our ppc64le based Linux , the message "WARNING: Your kernel does not support pids limit capabilities or the cgroup is not mounted. PIDs limit discarded." shows up , and the docker "--pids-limit" limitation does not work because of this.
So I had to take this into account.

OK. Please also add a test on the hotspot side. You may want to add relevant parts to TestMisc.java.

MBaesken · 2021-07-09T13:53:27Z

OK. Please also add a test on the hotspot side. You may want to add relevant parts to TestMisc.java.

Thanks for the suggestion, I will look into TestMisc.java .

MBaesken · 2021-07-15T13:21:36Z

OK. Please also add a test on the hotspot side. You may want to add relevant parts to TestMisc.java.

Thanks for the suggestion, I will look into TestMisc.java .

I added some HS testing code in the latest commit.

Best regards, Matthias

jerboaa · 2021-07-15T13:39:21Z

Thanks, I'll test it and review once again. Meanwhile, please merge the master branch into your JDK-8266490 branch and push so that we get a better ruling of pre-integration tests.

jerboaa · 2021-07-19T14:37:09Z

@MBaesken TestPidsLimit.java fails for me for the Unlimited pids case with:

----------System.err:(43/1901)----------
 stdout: [Operating System Metrics:
    Provider: cgroupv1
    Effective CPU Count: 8
    CPU Period: 100000us
    CPU Quota: -1
    CPU Shares: -1
    List of Processors, 8 total:
    0 1 2 3 4 5 6 7
    List of Effective Processors, 8 total:
    0 1 2 3 4 5 6 7
    List of Memory Nodes, 1 total:
    0
    List of Available Memory Nodes, 1 total:
    0
    Memory Limit: Unlimited
    Memory Soft Limit: Unlimited
    Memory & Swap Limit: Unlimited
    Maximum number of tasks available to the process: 38019

openjdk version "18-internal" 2022-03-15
OpenJDK Runtime Environment (build 18-internal+0-adhoc.sgehwolf.jdk-jdk)
OpenJDK 64-Bit Server VM (build 18-internal+0-adhoc.sgehwolf.jdk-jdk, mixed mode, sharing)
];
 stderr: []
 exitValue = 0

java.lang.RuntimeException: 'Maximum number of tasks available to the process: Unlimited' missing from stdout/stderr

        at jdk.test.lib.process.OutputAnalyzer.shouldContain(OutputAnalyzer.java:221)
        at TestPidsLimit.testPidsLimit(TestPidsLimit.java:74)
        at TestPidsLimit.main(TestPidsLimit.java:52)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:568)
        at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
        at java.base/java.lang.Thread.run(Thread.java:833)

jerboaa · 2021-07-19T15:22:06Z

I think we should do something like this, which seems to solve the podman case (that's in-line with the docker-run man page):

diff --git a/test/jdk/jdk/internal/platform/docker/TestPidsLimit.java b/test/jdk/jdk/internal/platform/docker/TestPidsLimit.java
index d12e0b2d46d..9d4fd5d32dd 100644
--- a/test/jdk/jdk/internal/platform/docker/TestPidsLimit.java
+++ b/test/jdk/jdk/internal/platform/docker/TestPidsLimit.java
@@ -60,9 +60,11 @@ public class TestPidsLimit {
     private static void testPidsLimit(String pidsLimit) throws Exception {
         Common.logNewTestCase("TestPidsLimit");
         DockerRunOptions opts = Common.newOptsShowSettings(imageName);
-        if (! pidsLimit.equals("Unlimited")) {
+        if (pidsLimit.equals("Unlimited")) {
+            opts.addDockerOpts("--pids-limit=-1");
+        } else {
             opts.addDockerOpts("--pids-limit="+pidsLimit);
-        }
+       }

Note that for docker the default value for not setting it via --pids-limit seems to be 38019. Same for a pids-limit=-1. Contrast this to the podman setting on my cgroups v2 system (default limit 2048, vs max for -1):

$ sudo podman run -v $(pwd)/jdk18-build:/opt/jdk:z --rm -ti fedora:34 /bin/bash
[root@bf6f52fac5ea /]# cat /sys/fs/cgroup/pids.max 
2048
$ sudo podman run --pids-limit=-1 -v $(pwd)/jdk18-build:/opt/jdk:z --rm -ti fedora:34 /bin/bash
[root@41b5a2a5dcc7 /]# cat /sys/fs/cgroup/pids.max 
max

jerboaa

For the hotspot container test there is no "unlimited" test. Is that intentional? Perhaps align with the JDK side?

jerboaa · 2021-07-19T15:36:52Z

src/hotspot/os/linux/cgroupSubsystem_linux.cpp

@@ -167,19 +178,30 @@ bool CgroupSubsystemFactory::determine_type(CgroupInfo* cg_infos,
      cg_infos[CPUACCT_IDX]._name = os::strdup(name);
      cg_infos[CPUACCT_IDX]._hierarchy_id = hierarchy_id;
      cg_infos[CPUACCT_IDX]._enabled = (enabled == 1);
+    } else if (strcmp(name, "pids") == 0) {
+      log_info(os, container)("Detected optional pids controller entry in %s", proc_cgroups);


Is there a particular reason why this is level INFO? We have log_debug() or log_trace() for similar debugging in the same file. I don't think this will need a higher level than debug.

jerboaa · 2021-07-19T15:42:25Z

src/java.base/share/classes/sun/launcher/LauncherHelper.java

@@ -405,12 +405,23 @@ public static void printSystemMetrics() {
        limit = c.getMemoryAndSwapLimit();
        ostream.println(formatLimitString(limit, INDENT + "Memory & Swap Limit: ", longRetvalNotSupported));

+        limit = c.getPidsMax();
+        ostream.println(formatLimitString(limit, INDENT + "Maximum number of tasks available to the process: ",


This uses some different capitalization that for other metrics. Could we perhaps shorten this to: Maximum Processes Limit:

jerboaa · 2021-07-19T15:43:31Z

test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java

@@ -37,6 +37,7 @@ public static void main(String[] args) {
            System.out.println("Metrics.getMemoryLimit() == " + metrics.getMemoryLimit());
            System.out.println("Metrics.getMemoryAndSwapUsage() == " + metrics.getMemoryAndSwapUsage());
            System.out.println("Metrics.getMemoryUsage() == " + metrics.getMemoryUsage());
+            System.out.println("Metrics.getPidsMax() == " + metrics.getPidsMax());


Is there a reason why this got added? It seems unrelated to the change at hand.

jerboaa · 2021-07-19T15:46:42Z

test/jdk/jdk/internal/platform/docker/TestPidsLimit.java

+    }
+
+    private static void testPidsLimit(String pidsLimit) throws Exception {
+        Common.logNewTestCase("TestPidsLimit");


It would be nice if the test case log included the expected limit. Something like:

Common.logNewTestCase("TestPidsLimit (limit: " + pidsLimit + ")");

MBaesken · 2021-07-22T12:15:03Z

Hi Severin, thanks for the comments. I added a commit with a number of adjustments

src/hotspot/os/linux/cgroupSubsystem_linux.cpp
adjusted log_info to log_debug

src/java.base/share/classes/sun/launcher/LauncherHelper.java
adjusted the output to "Maximum Processes Limit:"

test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java
removed the getPidsMax related line (I think I inserted it while running some tests and forgot previously to remove it)

test/hotspot/jtreg/containers/docker/TestPids.java
added testing of "Unlimited"; added --pids-limit=-1 for Unlimited procs like you suggested

test/jdk/jdk/internal/platform/docker/TestPidsLimit.java
adjusted output; added --pids-limit=-1 for Unlimited procs like you suggested

jerboaa · 2021-07-22T15:38:59Z

@MBaesken Thanks. We need a solution for #4518 (comment) though. --pids-limit=-1 doesn't seem to make it unlimited on all container runtimes. For example it fails for me here with:

$ docker --version
Docker version 20.10.6, build 370c289

MBaesken · 2021-07-23T06:49:15Z

@MBaesken Thanks. We need a solution for #4518 (comment) though. --pids-limit=-1 doesn't seem to make it unlimited on all container runtimes. For example it fails for me here with:
$ docker --version
Docker version 20.10.6, build 370c289

Hi Severin, that's a pity and looks like a bug, because the docker documentation says
https://docs.docker.com/engine/reference/commandline/run/

--pids-limit		Tune container pids limit (set -1 for unlimited)

Do you have an idea what to set with docker 20 on your setup? I did not find much about this in the docker 20 release notes https://docs.docker.com/engine/release-notes/ .

jerboaa · 2021-07-23T08:39:37Z

@MBaesken Thanks. We need a solution for #4518 (comment) though. --pids-limit=-1 doesn't seem to make it unlimited on all container runtimes. For example it fails for me here with:
$ docker --version
Docker version 20.10.6, build 370c289
Hi Severin, that's a pity and looks like a bug, because the docker documentation says
https://docs.docker.com/engine/reference/commandline/run/
--pids-limit Tune container pids limit (set -1 for unlimited)

Do you have an idea what to set with docker 20 on your setup? I did not find much about this in the docker 20 release notes https://docs.docker.com/engine/release-notes/ .

No, I don't know what to do about it. All I can see it comes back with a pids limit of 38019 when set to -1. It does seem like a bug or an intentional setting so as to avoid fork bombs.

MBaesken · 2021-07-23T11:02:13Z

No, I don't know what to do about it. All I can see it comes back with a pids limit of 38019 when set to -1. It does seem like a bug or an intentional setting so as to avoid fork bombs.

Very strange indeed, I have docker 20.10.2 on my Ubuntu test server and there the tests work and no "38019" is coming back for -1/unlimited . What distro are you using? I did a quick search but did not find much about the mysterious 38019 . Could this be some setting configured on your box that is picked up as an additional limit ?

jerboaa · 2021-07-23T12:28:47Z

No, I don't know what to do about it. All I can see it comes back with a pids limit of 38019 when set to -1. It does seem like a bug or an intentional setting so as to avoid fork bombs.

Very strange indeed, I have docker 20.10.2 on my Ubuntu test server and there the tests work and no "38019" is coming back for -1/unlimited . What distro are you using? I did a quick search but did not find much about the mysterious 38019 . Could this be some setting configured on your box that is picked up as an additional limit ?

I'm on Fedora 34 and have the moby distro build of docker:
https://koji.fedoraproject.org/koji/buildinfo?buildID=1781164

jerboaa · 2021-07-23T12:32:04Z

Could this be some setting configured on your box that is picked up as an additional limit ?

Possibly. Not sure where to look, though.

MBaesken · 2021-07-27T10:25:22Z

Could this be some setting configured on your box that is picked up as an additional limit ?

Possibly. Not sure where to look, though.

I ask some local experts about that issue.
What do you think about accepting, when setting -1/unlimited, a high limit number like 20.000+ as well (and and a comment that on some setups unlimited means just "high number" but not unlimited?
Another Idea I had was to start a little test java program that creates e.g. 50.000 (or another high number) of threads. If this fails with "unilimited" pids-limit set, we might have a setup like yours and then skip the test (or accept a high number like I suggested).

jerboaa · 2021-08-02T11:42:33Z

What do you think about accepting, when setting -1/unlimited, a high limit number like 20.000+ as well (and and a comment that on some setups unlimited means just "high number" but not unlimited?

This seems most reasonable. I'd suggest to accept a limit of > 20000 or Unlimited in the test output. In case of it NOT being Unlimited for the --pids-limit=-1 case, I'd also include the actual output in logs with a message that it got accepted as unlimited.

Another Idea I had was to start a little test java program that creates e.g. 50.000 (or another high number) of threads. If this fails with "unilimited" pids-limit set, we might have a setup like yours and then skip the test (or accept a high number like I suggested).

This seems overkill and prone to failures, IMHO.

…mber

MBaesken · 2021-08-05T11:19:17Z

What do you think about accepting, when setting -1/unlimited, a high limit number like 20.000+ as well (and and a comment that on some setups unlimited means just "high number" but not unlimited?

This seems most reasonable. I'd suggest to accept a limit of > 20000 or Unlimited in the test output. In case of it NOT being Unlimited for the --pids-limit=-1 case, I'd also include the actual output in logs with a message that it got accepted as unlimited.

Hi Severin, I adjusted the tests so that in case of Unlimited, an integer value > 20000 is accepted as well.
Hopefully this addresses the issues observed on your setup.
Best regards, Matthias

jerboaa · 2021-08-05T14:53:29Z

@MBaesken Thanks. I'll test it and will report back.

jerboaa

This looks good to me now and passes tests on my cgroup v1 and cgroup v2 setups.

jerboaa · 2021-08-06T08:27:32Z

test/hotspot/jtreg/containers/docker/TestPids.java

+        if (value.equals("Unlimited")) {
+            opts.addDockerOpts("--pids-limit=-1");
+        } else {
+            opts.addDockerOpts("--pids-limit="+value);


Style nit: Space before and after +.

jerboaa · 2021-08-06T08:28:23Z

test/jdk/jdk/internal/platform/docker/TestPidsLimit.java

+        if (pidsLimit.equals("Unlimited")) {
+            opts.addDockerOpts("--pids-limit=-1");
+        } else {
+            opts.addDockerOpts("--pids-limit="+pidsLimit);


Style nit: Space before and after +.

openjdk · 2021-08-06T08:31:28Z

@MBaesken This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8266490: Extend the OSContainer API to support the pids controller of cgroups

Reviewed-by: sgehwolf, lucy

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 224 new commits pushed to the master branch:

2384e12: 8270098: ZGC: ZBarrierSetC2::clone_at_expansion fails with "Guard against surprises" assert
d53d94b: 8271925: ZGC: Arraycopy stub passes invalid oop to load barrier
3b899ef: 8272168: some hotspot runtime/logging tests don't check exit code
abdc107: 8270454: G1: Simplify region index comparison
eb6f3fe: 8272169: runtime/logging/LoaderConstraintsTest.java doesn't build test.Empty
9654fd7: 8271892: mark hotspot runtime/PrintStringTableStats/PrintStringTableStatsTest.java test as ignoring external VM flags
843943c: 8263567: gtests don't terminate the VM safely
7fc99cf: 8225488: Examine ExecutableType.getReceiverType behavior when source receiver parameter is absent
4548677: 8268824: Remove unused jdk.accessibility APIs deprecated for removal in JDK 9
b53828b: 8272047: java/nio/channels/FileChannel/Transfer2GPlus.java failed with Unexpected transfer size: 2147418112
... and 214 more: https://git.openjdk.java.net/jdk/compare/8e27d4e8ceb3c4ea15e3a3a4328368dbe801870b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

MBaesken · 2021-08-06T10:26:32Z

This looks good to me now and passes tests on my cgroup v1 and cgroup v2 setups.

Thanks for the review and approval .

Best regards, Matthias

RealLucy

The changes look good to me.

I have one question/suggestion re. copyright comments. The files
test/hotspot/jtreg/containers/docker/TestPids.java
and
test/jdk/jdk/internal/platform/docker/TestPidsLimit.java
are new with this PR. Shouldn't the copyright then just specify the current year and not a range?

And how about adding the author's company as additional copyright holder?

MBaesken · 2021-08-10T07:38:59Z

/integrate

openjdk · 2021-08-10T07:40:33Z

Going to push as commit 089e83b.
Since your change was applied there have been 224 commits pushed to the master branch:

2384e12: 8270098: ZGC: ZBarrierSetC2::clone_at_expansion fails with "Guard against surprises" assert
d53d94b: 8271925: ZGC: Arraycopy stub passes invalid oop to load barrier
3b899ef: 8272168: some hotspot runtime/logging tests don't check exit code
abdc107: 8270454: G1: Simplify region index comparison
eb6f3fe: 8272169: runtime/logging/LoaderConstraintsTest.java doesn't build test.Empty
9654fd7: 8271892: mark hotspot runtime/PrintStringTableStats/PrintStringTableStatsTest.java test as ignoring external VM flags
843943c: 8263567: gtests don't terminate the VM safely
7fc99cf: 8225488: Examine ExecutableType.getReceiverType behavior when source receiver parameter is absent
4548677: 8268824: Remove unused jdk.accessibility APIs deprecated for removal in JDK 9
b53828b: 8272047: java/nio/channels/FileChannel/Transfer2GPlus.java failed with Unexpected transfer size: 2147418112
... and 214 more: https://git.openjdk.java.net/jdk/compare/8e27d4e8ceb3c4ea15e3a3a4328368dbe801870b...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-08-10T07:40:49Z

@MBaesken Pushed as commit 089e83b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8266490

0e6ecb8

openjdk bot added the rfr Pull request is ready for review label Jun 17, 2021

openjdk bot added hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Jun 17, 2021

jerboaa suggested changes Jun 17, 2021

View reviewed changes

Adjustments following Severins comments

afd7bf6

jerboaa reviewed Jun 29, 2021

View reviewed changes

test and small adjustments suggested by Severin

f552714

Add hotspot tests

3fe73c3

Merge remote-tracking branch 'origin/master' into JDK-8266490

5fc52fb

jerboaa suggested changes Jul 19, 2021

View reviewed changes

Minor adjustments, handling of Unlimited

857ab1d

MBaesken added 2 commits August 5, 2021 09:45

Merge remote-tracking branch 'origin/master' into JDK-8266490

39b5008

Adjust tests, unlimited pids value can lead on some setups to high nu…

0dd58e0

…mber

jerboaa approved these changes Aug 6, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 6, 2021

RealLucy approved these changes Aug 9, 2021

View reviewed changes

Update test Copyright headers

217ffb2

openjdk bot closed this Aug 10, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 10, 2021

JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups #4518

JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups #4518

Conversation

MBaesken commented Jun 17, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 17, 2021

openjdk bot commented Jun 17, 2021

mlbridge bot commented Jun 17, 2021 • edited Loading

Webrevs

jerboaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBaesken commented Jun 23, 2021

jerboaa commented Jun 23, 2021

MBaesken commented Jun 24, 2021

jerboaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBaesken commented Jul 9, 2021 • edited Loading

jerboaa commented Jul 9, 2021

MBaesken commented Jul 9, 2021

MBaesken commented Jul 15, 2021

jerboaa commented Jul 15, 2021

jerboaa commented Jul 19, 2021

jerboaa commented Jul 19, 2021

jerboaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBaesken commented Jul 22, 2021

jerboaa commented Jul 22, 2021

MBaesken commented Jul 23, 2021

jerboaa commented Jul 23, 2021

MBaesken commented Jul 23, 2021

jerboaa commented Jul 23, 2021

jerboaa commented Jul 23, 2021

MBaesken commented Jul 27, 2021

jerboaa commented Aug 2, 2021

MBaesken commented Aug 5, 2021

jerboaa commented Aug 5, 2021

jerboaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Aug 6, 2021 • edited Loading

MBaesken commented Aug 6, 2021

RealLucy left a comment

Choose a reason for hiding this comment

MBaesken commented Aug 10, 2021

openjdk bot commented Aug 10, 2021

openjdk bot commented Aug 10, 2021

MBaesken commented Jun 17, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 17, 2021 •

edited

Loading

MBaesken commented Jul 9, 2021 •

edited

Loading

openjdk bot commented Aug 6, 2021 •

edited

Loading