-
Notifications
You must be signed in to change notification settings - Fork 239
8230305: Cgroups v2: Container awareness #840
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
Conversation
👋 Welcome back zgu! A progress list of the required criteria for merging this PR into |
This backport pull request has now been updated with issue and summary from the original commit. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine. A few minor comments.
Aside: We need to devise a plan how we want to deal with integrating this set of patches. For example I'm seeing test failures of the hotspot container tests on a cgroups v2 system due to https://bugs.openjdk.java.net/browse/JDK-8253714.
@@ -68,8 +67,7 @@ class OSContainer: AllStatic { | |||
}; | |||
|
|||
inline bool OSContainer::is_containerized() { | |||
assert(_is_initialized, "OSContainer not initialized"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is essentially a backport of https://bugs.openjdk.java.net/browse/JDK-8229202 please mention that issue in the backport (/issue add JDK-8229202
). Not sure if this will work for a backport.
/* | ||
* PER_CPU_SHARES has been set to 1024 because CPU shares' quota | ||
* is commonly used in cloud frameworks like Kubernetes[1], | ||
* AWS[2] and Mesos[3] in a similar way. They spawn containers with | ||
* --cpu-shares option values scaled by PER_CPU_SHARES. Thus, we do | ||
* the inverse for determining the number of possible available | ||
* CPUs to the JVM inside a container. See JDK-8216366. | ||
* | ||
* [1] https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#meaning-of-cpu | ||
* In particular: | ||
* When using Docker: | ||
* The spec.containers[].resources.requests.cpu is converted to its core value, which is potentially | ||
* fractional, and multiplied by 1024. The greater of this number or 2 is used as the value of the | ||
* --cpu-shares flag in the docker run command. | ||
* [2] https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_ContainerDefinition.html | ||
* [3] https://github.com/apache/mesos/blob/3478e344fb77d931f6122980c6e94cd3913c441d/src/docker/docker.cpp#L648 | ||
* https://github.com/apache/mesos/blob/3478e344fb77d931f6122980c6e94cd3913c441d/src/slave/containerizer/mesos/isolators/cgroups/constants.hpp#L30 | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is essentially a backport of https://bugs.openjdk.java.net/browse/JDK-8216366. Perhaps add this issue as part of the backport (/isssue add JDK-8216366
).
@zhengyu123 This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
/issue add JDK-8229202 |
@zhengyu123 |
@zhengyu123 |
@jerboaa Thanks for the review. |
For me,
(I noticed this in a downstream backport and traced it back up here) I've put my JTreport here: https://jmtd.net/tmp/jdk11u_8230305_PlainRead_jtreport/JTreport/html/index.html Edit: @jerboaa points out that this is probably JDK-8278951, although backporting that does not fix the test for me locally. I'll keep exploring. Edit 2: fails for me on jdk17u-dev master, passes on jdk master. |
Mailing list message from Zhengyu Gu on jdk-updates-dev: On 3/11/22 10:52, Jonathan Dowland wrote:
Yes, I noticed. I would suggest you postpone your backport until the -Zhengyu |
Note that the
This shouldn't block this backport. |
/integrate |
Going to push as commit 8715f37. |
@zhengyu123 Pushed as commit 8715f37. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
I would like backport cgroups v2 support to openjdk11u.
The original patch does not apply cleanly, conflicts are resolved manually.
Test:
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/840/head:pull/840
$ git checkout pull/840
Update a local copy of the PR:
$ git checkout pull/840
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/840/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 840
View PR using the GUI difftool:
$ git pr show -t 840
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/840.diff