-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container #18201
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 sgehwolf! A progress list of the required criteria for merging this PR into |
@jerboaa The following labels will be automatically applied to this pull request:
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. |
@jerboaa 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:
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 8 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
Anyone willing to review this? |
Gentle ping. |
IMHO Container detection is IIUC/AFAIK being used to maximize resource usage by OpenJDK. But if OpenJDK runs in a container with the same limits as the hardware box OpenJDK should still use reduced resources as it is sharing them with other processes on the hardware box. |
The idea here is to use this property to tune OpenJDK for in-container, specifically k8s, use. In such a setup it's custom to run a single process within set resource constraints. In order to do this, we need a reliable way to distinguish that vs. non-containerized setup. If somebody really wants to run OpenJDK in a container expecting it to run like a physical OpenJDK deployment, that's when |
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.
I am not enough of a container expert to judge if the basic approach is right - I trust you on this. This is just a technical code review.
The in-container tuning means to use all the available resources. Containers in the real world have some memory limits set which is where my modified patch still correctly identifies it as a container to use all the available resources of the node which is the whole goal of the container detection code.
I expect it should have been written "We need a reliable way to distinguish real world in-container vs. non-containerized setup. We do not mind behavior for artificial containers on OpenJDK development machines.". Which is what my patch does in an easier and less error-prone way.
That behaves still the same with my patch. Could you give a countercase where my patch behaves wrongly? |
@jankratochvil I believe this boils down to what we actually want. Should The truth table of the patch in this PR looks like this:
As you can see, the case of "OpenJDK runs in a privileged container without a cpu/memory limit" gives the wrong result. However, I consider this a fairly uncommon setup and there isn't really anything we can do to detect this kind of setup. Even if we did manage to detect it, why would we care? It's a question of probability.
Again, it's not a case of right or wrong IMO. Since we are in the land of heuristics, they will be wrong in some cases. We should make them so that we cover the common cases and, perhaps, are able to use those in serviceability tools to help guide diagnosis and/or further tuning. So far the existing capabilities were OK, but prevent further out-of-the-box tuning and/or accurate data collection. Your proposed patch (it's one I had in a previous iteration too, fwiw) would also return Let's have that discussion more broadly and see if we can reach consensus! |
Could not we rename I did not test it but I expect the values will be:
|
I'm not sure. There is value to have |
Mailing list message from Laurence Cable on serviceability-dev: On 4/18/24 9:38 AM, Severin Gehwolf wrote:
agree 100% |
Mailing list message from Laurence Cable on serviceability-dev: On 4/18/24 2:54 AM, Severin Gehwolf wrote:
I think (I am agreeing with you Severin) that the goal of the heuristic Rgds - Larry |
Thanks for your input Larry! |
"resource constrained" (my patch) vs. "managed" (this patch) is the difference of the two patches being discussed. Anyway in this patch one could unify naming across variables/parameters, the same value is called |
tools/javac/annotations/typeAnnotations/api/ArrayCreationTree test failure in GHA on 32 bit Linux seems unrelated. |
@tstuefe Do you mind to take another look? Thank you! |
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.
Seems okay. I don't understand the depths of V1 vs V2 controller files as well as you do, @jerboaa , but I trust you there. The mechanics seem fine.
Currently this patch conflicts a lot with #19085 (jerboaa:jdk-8331560-cgroup-controller-delegation). |
Yes, I'll resolve it one way or another depending on which one goes in first. |
Could I get a second review on this please? @larry-cable maybe? Thanks! |
Mailing list message from Laurence Cable on serviceability-dev: will do! On 6/26/24 3:04 AM, Severin Gehwolf wrote: |
Thanks for the review! |
@adinn @iklam Could one of you please help with a second review, please? Not sure if @larry-cable review gets recorded with him having no OpenJDK project role :-/ Thanks in advance! |
On 6/28/24 8:47 AM, Severin Gehwolf wrote:
@adinn
<https://urldefense.com/v3/__https://github.com/adinn__;!!ACWV5N9M2RV99hQ!JFioMWt3M387QFQK8UEgSmPMiy736aUVfLbZdVscJ-BDxoO9GjUqLFHtvCkNB5oG3MBadzdxxytGTNNNu9cWG6X3BQ$>
@iklam
<https://urldefense.com/v3/__https://github.com/iklam__;!!ACWV5N9M2RV99hQ!JFioMWt3M387QFQK8UEgSmPMiy736aUVfLbZdVscJ-BDxoO9GjUqLFHtvCkNB5oG3MBadzdxxytGTNNNu9dyhXpCrw$>
Could one of you please help with a second review, please? Not sure if
@larry-cable
<https://urldefense.com/v3/__https://github.com/larry-cable__;!!ACWV5N9M2RV99hQ!JFioMWt3M387QFQK8UEgSmPMiy736aUVfLbZdVscJ-BDxoO9GjUqLFHtvCkNB5oG3MBadzdxxytGTNNNu9cijrbcsw$>
review gets recorded with him having no OpenJDK project role :-/
Thanks in advance!
yeah sorry - I'm a "newbie" ... only since 1.1 ... :)
…
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/18201*issuecomment-2197212014__;Iw!!ACWV5N9M2RV99hQ!JFioMWt3M387QFQK8UEgSmPMiy736aUVfLbZdVscJ-BDxoO9GjUqLFHtvCkNB5oG3MBadzdxxytGTNNNu9eqRE9O0w$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67RGGSXHJWFIRNCSGRLZJWARNAVCNFSM6AAAAABEQV4TGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJXGIYTEMBRGQ__;!!ACWV5N9M2RV99hQ!JFioMWt3M387QFQK8UEgSmPMiy736aUVfLbZdVscJ-BDxoO9GjUqLFHtvCkNB5oG3MBadzdxxytGTNNNu9fmaD5wrA$>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--------------UaLc7Fb3y3GBgvf0paImu5tU
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit
<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<br>
<br>
<div class="moz-cite-prefix">On 6/28/24 8:47 AM, Severin Gehwolf
wrote:<br>
</div>
<blockquote type="cite" ***@***.***">
<p dir="auto"><a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/adinn/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urldefense.com/v3/__https://github.com/adinn__;!!ACWV5N9M2RV99hQ!JFioMWt3M387QFQK8UEgSmPMiy736aUVfLbZdVscJ-BDxoO9GjUqLFHtvCkNB5oG3MBadzdxxytGTNNNu9cWG6X3BQ$" ***@***.***</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/iklam/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urldefense.com/v3/__https://github.com/iklam__;!!ACWV5N9M2RV99hQ!JFioMWt3M387QFQK8UEgSmPMiy736aUVfLbZdVscJ-BDxoO9GjUqLFHtvCkNB5oG3MBadzdxxytGTNNNu9dyhXpCrw$" ***@***.***</a> Could one of you please help
with a second review, please? Not sure if <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/larry-cable/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urldefense.com/v3/__https://github.com/larry-cable__;!!ACWV5N9M2RV99hQ!JFioMWt3M387QFQK8UEgSmPMiy736aUVfLbZdVscJ-BDxoO9GjUqLFHtvCkNB5oG3MBadzdxxytGTNNNu9cijrbcsw$" ***@***.***</a> review gets recorded
with him having no OpenJDK project role :-/ Thanks in advance!</p>
</blockquote>
<br>
yeah sorry - I'm a "newbie" ... only since 1.1 ... :)<br>
<blockquote type="cite" ***@***.***">
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br>
Reply to this email directly, <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/18201*issuecomment-2197212014__;Iw!!ACWV5N9M2RV99hQ!JFioMWt3M387QFQK8UEgSmPMiy736aUVfLbZdVscJ-BDxoO9GjUqLFHtvCkNB5oG3MBadzdxxytGTNNNu9eqRE9O0w$" moz-do-not-send="true">view it on GitHub</a>, or <a href="https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67RGGSXHJWFIRNCSGRLZJWARNAVCNFSM6AAAAABEQV4TGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJXGIYTEMBRGQ__;!!ACWV5N9M2RV99hQ!JFioMWt3M387QFQK8UEgSmPMiy736aUVfLbZdVscJ-BDxoO9GjUqLFHtvCkNB5oG3MBadzdxxytGTNNNu9fmaD5wrA$" moz-do-not-send="true">unsubscribe</a>.<br>
You are receiving this because you were mentioned.<img src="https://github.com/notifications/beacon/ANTA67R6DREAIZH7GP3I5PTZJWARNA5CNFSM6AAAAABEQV4TGSWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTUC63FW4.gif" alt="" moz-do-not-send="true" width="1" height="1"><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message
ID: <span><openjdk/jdk/pull/18201/c2197212014</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
***@***.***": "http://schema.org",
***@***.***": "EmailMessage",
"potentialAction": {
***@***.***": "ViewAction",
"target": "#18201 (comment)",
"url": "#18201 (comment)",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
***@***.***": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>
</blockquote>
<br>
</body>
</html>
--------------UaLc7Fb3y3GBgvf0paImu5tU--
|
Mailing list message from Laurence Cable on serviceability-dev: On 6/28/24 8:45 AM, Severin Gehwolf wrote:
Uugh ... makes sense (I guess) :) |
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.
Looks reasonable to me
Thank you for the review! |
/integrate |
Going to push as commit 0a6ffa5.
Your commit was automatically rebased without conflicts. |
Please review this enhancement to the container detection code which allows it to figure out whether the JVM is actually running inside a container (
podman
,docker
,crio
), or with some other means that enforces memory/cpu limits by means of the cgroup filesystem. If neither of those conditions hold, the JVM runs in not containerized mode, addressing the issue described in the JBS tracker. For example, on my Linux system `is_containerized() == false" is being indicated with the following trace log line:This state is being exposed by the Java
Metrics
API class using the new (still JDK internal)isContainerized()
method. Example:The basic property this is being built on is the observation that the cgroup controllers typically get mounted read only into containers. Note that the current container tests assert that
OSContainer::is_containerized() == true
in various tests. Therefore, using the heuristic of "is any memory or cpu limit present" isn't sufficient. I had considered that in an earlier iteration, but many container tests failed.Overall, I think, with this patch we improve the current situation of claiming a containerized system being present when it's actually just a regular Linux system.
Testing:
Thoughts?
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18201/head:pull/18201
$ git checkout pull/18201
Update a local copy of the PR:
$ git checkout pull/18201
$ git pull https://git.openjdk.org/jdk.git pull/18201/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18201
View PR using the GUI difftool:
$ git pr show -t 18201
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18201.diff
Webrev
Link to Webrev Comment