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

Fix JVM vendor check to include Azul Systems #9399

Closed
wants to merge 3 commits into from
Closed

Fix JVM vendor check to include Azul Systems #9399

wants to merge 3 commits into from

Conversation

dekobon
Copy link

@dekobon dekobon commented Nov 23, 2017

Fix #9398

This change adds a list of approved JVM vendors the PrestoSystemRequirements class and adds Azul as an approved vendor in addition to Oracle.

@electrum
Copy link
Contributor

electrum commented Nov 23, 2017 via email

@dekobon
Copy link
Author

dekobon commented Nov 23, 2017

Here is an example of a TravisCI config working with the Azul Zuku JDK: https://github.com/dekobon/java-manta/blob/travis-ci-additional-jdks/.travis.yml

It isn't available as a first class citizen of Travis CI, but it can easily be installed via apt. I've added an issue to the jdk_switcher project in order to add support for Zulu. The jdk_switcher project is used by Travis CI to switch between JDKs.

@electrum
Copy link
Contributor

electrum commented Nov 23, 2017 via email


private PrestoSystemRequirements() {}

public static void verifyJvmRequirements()
{
String vendor = StandardSystemProperty.JAVA_VENDOR.value();
if (!"Oracle Corporation".equals(vendor)) {
if (!JVM_VENDORS.contains(vendor)) {
Copy link
Contributor

@kokosing kokosing Nov 23, 2017

Choose a reason for hiding this comment

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

maybe instead of making it supported or failing, we could raise a warning that you are using Azul Systems, Inc. which should be ok but it is not tested, so you are doing this on your own risk.

I do not expect that we will start to run tests on two different JVMs in order to check that we are ok with both of them. Either I do not expect that all the users would migrate to Azul ;)

Having such warning would be just good enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, we could have a config switch that skips this check.. But I guess there might be reasons that we don't have it yet, are there?

Copy link
Author

Choose a reason for hiding this comment

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

@electrum I've updated the previously referenced Travis CI config to use Java 9.

@kokosing A warning would solve the immediate problem, but it would be odd because the running JVM would be more compatible than most OpenJDK distributions. If that is the case, then wouldn't you also want to warn when running an OpenJDK?

@findepi A switch would also solve the problem.

More broadly, do you really need this check? I've never seen a Java project that does this check before. It is up to the user to choose a JVM and to vet it when running an open source Java application. Most projects publish in their documentation what JVMs they test with and that is sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. AFAIR we do not any tests for OpenJDK, so it sounds reasonable to have such warning for OpenJDK as well.

My understanding of this check is to prevent silly errors of cluster configuration which will cause misbehavior of Presto that would be hard to diagnose. That is why Presto output all the configuration when it starts and have such checks like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Presto is a heavy user of the JVM and has hit many bugs with the JIT, so we enforce a minimum Java version that is known to work well. This version check is specific to OpenJDK. Does Azul use the same versioning, i.e., would 8u92 be the same on OpenJDK and Azul?

@dekobon
Copy link
Author

dekobon commented Nov 27, 2017

I've updated the PR to do the following:

  • Check for the OpenJDK vs the Oracle JDK.
  • Warn instead of error if the user is running the OpenJDK.
  • Warn instead of error if the user is not running a JDK from Oracle.

I've tested on Linux with:

  • OpenJDK 7, 8, 9
  • Azul Zulu 8, 9
  • Oracle JDK 7, 8
  • IBM JDK 8

@electrum
Copy link
Contributor

electrum commented Nov 27, 2017

The purpose of the check is to reduce support costs and provide a good user experience. For example, we know that there major JIT bugs in Oracle / OpenJDK in versions older than 8u92, so we won't even attempt to run there.

Printing a warning is not good enough since users are unlikely to notice it, or they will choose to ignore it since that is easier than updating the JVM. Having a hard requirement forces the correct action and results in a good experience.

For the purpose of JIT bugs, Oracle vs OpenJDK is fine, since the code base is the same. Given that Azul has it's own JIT that is completely different than OpenJDK, it seems likely that the vendor+version check needs to be done separately based on testing performed at scale (the bugs sometimes occur once per day on one out of hundreds of machines).

For other JVMs, there are compatibility issues due to usage of various internal APIs such as Unsafe. Though in practice, the IBM JVM is the only other one, and it's likely everything we use is supported there as well.

@electrum
Copy link
Contributor

electrum commented Nov 27, 2017

So, I'm thinking we should do the checks like this:

  • If OpenJDK: validate known good version
  • If Azul: validate known good version
  • Otherwise fail

@dekobon
Copy link
Author

dekobon commented Nov 27, 2017

@electrum I just shot an email to someone at Azul to invite them to comment on this thread. As far as I know the Zulu JDK is 99.9% the same code as the OpenJDK and the version numbers follow the OpenJDK as well. I would expect that the subversion check could continue as expected. As for what is exactly different, I'm going to let them comment.

@electrum
Copy link
Contributor

@dekobon Thanks, I was actually under the impression you were with Azul. My comment about them using a different JIT is based on this: https://www.azul.com/called-new-jit-compiler-falcon/

However, I now realize I am mixing up Zulu and Zing. So it sounds like we will need to differentiate the two rather than just matching the vendor (which apparently provides two very different JVMs). This is my fault for not reading the original issue more closely.

@dekobon
Copy link
Author

dekobon commented Nov 27, 2017

@electrum Nope, I'm with Joyent and I'm working on packing up Presto installs with Terraform and Docker that are dynamically scalable and work with our object store.

In the process of doing this, I need a reliable OpenJDK that our customers can seek support for if they need it. Due to a variety of issues, this isn't possible with Oracle.

@electrum
Copy link
Contributor

Two questions here

  1. How can we differentiate between Zulu and Zing?
  2. Does Zulu use the same versioning as OpenJDK? i.e., can we check for 8u92 on Zulu and have it mean the same thing?

@giltene
Copy link

giltene commented Dec 8, 2017

Yes, Zulu versioning follows the versioning (including the update level) of the OpenJDK source code for the same version.

You can differentiate between Zulu and Zing by the version string:

Zulu:
% java -version
openjdk version "1.8.0_152"
OpenJDK Runtime Environment (Zulu 8.25.0.1-macosx) (build 1.8.0_152-b16)
OpenJDK 64-Bit Server VM (Zulu 8.25.0.1-macosx) (build 25.152-b16, mixed mode)

Zing:
% java -version
java version "1.8.0-zing_17.11.1.0"
Zing Runtime Environment for Java Applications (build 1.8.0-zing_17.11.1.0-b2)
Zing 64-Bit Tiered VM (build 1.8.0-zing_17.11.1.0-b4-product-azlinuxM-X86_64, mixed mode)

@giltene
Copy link

giltene commented Dec 8, 2017

It is worth noting that [quite unfortunately] in practice, just the "openjdk" string and version number are not a good indicator of the version of OpenJDK project you are actually running. The reason is that there are many mis-labeled versions of built-from-OpenJDK-sources binaries out there that mis-report things in the version string. In the past, some of those have escaped their "experimental" or "unstable" intended enclosure without a warning label, and contaminated half the world.

OpenJDK does not produce binaries. OpenJDK is a source code project. There are various binary distributions of OpenJDK out there, Zulu being one of them (and the one with the most platforms and targets AFAIK, including Linux/Windows/Mac, x86/x64/ARM64/ARM32). RedHat also makes one that is included with RHEL for example, and most other distros include some variation as well. The degree of testing and adherence to the actual version varies dramatically, and many packages out there are mis-labeled and have never been TCK-tested.

A specific, well-documented, and practical example of how badly mis-labeld openjdk builds have hit people in the past is that between September 2014 and March of 2015, the "Official Docker image" for java, which is basically what you got if you ran "docker run java:8 0 -version" in any docker environment, reported as "openjdk version "1.8.0_40". Unfortunately, the actual OpenJDK 8u40 was only released in March of 2015. So for a period of 6 months before the release, people around the world that were thinking they are running the latest, blessed, and stable version of OpenJDK were actually running something build from a top-of-tree snapshot of changing and unreleased sources, which was responded to "java -version" with a version string that was the same as the final eventual 8u40 version.

The way this happened is not the point, the fact that it id is. Had presto used the "java -version" string at the time, it would have concluded that it is running on a good version of OpenJDK. More importantly, for a long time after March 2015 (when a real 8u40 did exist), there still existed docker images that were forged between Sept. 2014 and March 2015 that purported to be 8u40, but still included many bug that were fixed in that actual release.

The way this happened in not "rare" or "strange". In this specific case,. it happened because properly-labeled "unstable" debian distros insisted on building their JDKs and JREs with the same OpenJDK version string that the final OpenJDK release would carry, and did not tag their interim and experimental JDK/JRE builds with anything that would identify them as different from the final production version. Their logic seems to have been that the debian Linux distro as a whole was clearly labeled as something other than "stable", so "people should know better". Unfortunately, the binary bits within those distros made it into the wild in various ways, either as tarballs or packages, and propagated through multiple steps to unsuspecting end-users. Since they did not carry labels to the contrary, those end-users (including applications that diligently checked the java -version string) assumed that the JDKs were what they claimed to be: a binary build of OpenJDK 8u40.

As to why these bits propagated so far, I can only speculate, but it seems likely that a main contributor was the fact that there was no other java 8 binary package (of any version) available in any stable debian distro or repo at the time. This meant that anyone wanting a java 8 binary would generally bring in a tarball or a package from an external source, and this thing that reported as 8u40 "seemed like a good one" to many. This mechanism is likely to repeat: E.g. stable versions of debian do not currently have a Java 9 version in their distro or repos, so people who want to run java 9 get it from elsewhere, and the unstable repos may be a common source.

@findepi
Copy link
Contributor

findepi commented Dec 12, 2018

Superseded by #11396

@findepi findepi closed this Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants