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

Clarify cgroupsV2 Java statements #40189

Merged
merged 1 commit into from
May 9, 2023

Conversation

kgibm
Copy link
Contributor

@kgibm kgibm commented Mar 21, 2023

  • The previous statement was missing HotSpot 8 which had cgroupsV2 support backported.
  • The previous statement was ambiguous as different JVM implementations added and backported cgroupsV2 at different times. The linked bug report was specifically for the HotSpot JVM.
  • This PR adds additional JVM vendors (specifically IBM/Eclipse OpenJ9, and others can be added by other vendors).
  • Note that there is a translation at content/zh-cn/blog/_posts/2022-08-31-cgroupv2-ga.md which would be nice to update as well.

Although this is an older blog post, people still use and reference it.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 21, 2023
@k8s-ci-robot k8s-ci-robot requested a review from onlydole March 21, 2023 15:20
@k8s-ci-robot k8s-ci-robot added the area/blog Issues or PRs related to the Kubernetes Blog subproject label Mar 21, 2023
@k8s-ci-robot k8s-ci-robot requested a review from sftim March 21, 2023 15:20
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 21, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

@bobbypage & @mrunalp, thoughts on this?

@netlify
Copy link

netlify bot commented Mar 21, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 24d2120
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/641df6ffca7cf5000876777b
😎 Deploy Preview https://deploy-preview-40189--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@bobbypage
Copy link
Member

Thanks! This looks good and great to see that cgroupv2 was backported to JDK8.

It does look like OpenJDK 8u372 is not released yet though, it seems like it will be released April 18th 2023.

image

@bobbypage
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 931d335fe10181f414f55006db1df025f0246ed1

* If you deploy Java applications with the JDK, prefer to use JDK 11.0.16 and
later or JDK 15 and later, which [fully support cgroup v2](https://bugs.openjdk.org/browse/JDK-8230305).
* If you deploy Java applications, prefer to use versions which fully support cgroup v2:
* [HotSpot](https://bugs.openjdk.org/browse/JDK-8230305): jdk8u372, 11.0.16, 15 and later
Copy link
Member

Choose a reason for hiding this comment

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

Should we say Hotspot / Open JDK ?

Copy link
Contributor Author

@kgibm kgibm Mar 23, 2023

Choose a reason for hiding this comment

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

I've always found the term OpenJDK ambiguous in JVM diagnostic situations because, for example, IBM Semeru Runtimes also uses OpenJDK for its JDK implementation, but the issue here isn't about the JDK classes but rather about the JVM which does the cgroups detection (IBM's JVM is called J9). However, OpenJDK is also commonly used to refer to the whole package of JDK and HotSpot JVM, so I can see an argument for both sides. Do you happen to know anyone from Oracle that could comment?

Copy link
Member

Choose a reason for hiding this comment

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

/cc @jmtd, I see you added the backport, do you have any thoughts here?

Copy link

@jmtd jmtd Mar 24, 2023

Choose a reason for hiding this comment

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

It's a tricky one. I'd definitely choose to include "OpenJDK" in the wording, because we know that the support has landed there, and can't speak more broadly about downstream vendors' versions (Vendors are capable of removing or disabling features as well as adding them: although it's very unlikely for that to happen for a feature like this). As for mentioning HotSpot, I'd argue it's less important: the majority of OpenJDK users are using HotSpot, but many of them may be unfamiliar with the terminology; on the other hand, for users who don't use HotSpot, such as OpenJ9 users, I'd wager they are more likely to be aware that they can't take a statement about OpenJDK on face value for their situation.

Choose a reason for hiding this comment

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

As an OpenJ9 project lead, I'm not sure it matters in this case which term is used given the next two lines refer to Semeru and IBM Java as both are OpenJ9-based distributions.

Generally, we prefer Hotspot is used for Hotspot-specific behaviours as OpenJDK is frequently an umbrella term for all distributions which ship OpenJDK class libraries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so correct me if I misunderstood, but it sounds like the preference is to just write "OpenJDK / HotSpot". I'll update the commit and squash soon (-ish; have a few higher priority things right now).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @jmtd and @DanHeidinga for the feedback!

@bobbypage
Copy link
Member

bobbypage commented Mar 24, 2023

@kgibm can you please also update the docs here with the same info? Thank you again for updating!

@kgibm
Copy link
Contributor Author

kgibm commented Mar 24, 2023

Ah, yes, I wasn't aware of that page. I'll update that as well.

Signed-off-by: Kevin Grigorenko <[email protected]>
@kgibm kgibm force-pushed the clarifycgroupsv2 branch from 06c48ac to 24d2120 Compare March 24, 2023 19:16
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 24, 2023
@k8s-ci-robot k8s-ci-robot requested a review from bobbypage March 24, 2023 19:16
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 24, 2023
@bobbypage
Copy link
Member

Looks great, thanks for the update!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 24, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: a0489ba992ed921c45fc4b93a29cd043fc038a01

@bobbypage
Copy link
Member

/assign @sftim

@bobbypage
Copy link
Member

cc @shannonxtreme

@kbhawkey
Copy link
Contributor

kbhawkey commented May 9, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kbhawkey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 9, 2023
@k8s-ci-robot k8s-ci-robot merged commit 3103f9c into kubernetes:main May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/blog Issues or PRs related to the Kubernetes Blog subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants