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

Bundle java in distributions #38013

Merged
merged 45 commits into from Mar 5, 2019

Conversation

Projects
None yet
5 participants
@rjernst
Copy link
Member

commented Jan 30, 2019

Setting up a jdk is currently a required external step when installing
elasticsearch. This is particularly problematic for the rpm/deb packages
as installing a jdk in the same package installation command does not
guarantee any order, so must be done in separate steps. Additionally,
JAVA_HOME must be set and often causes problems in selecting a correct
jdk when, for example, the system java is an older unsupported version.

This commit bundles platform specific openjdks into each distribution.
In addition to eliminating the issues above, it also presents future
possible improvements like using jlink to build jdk images only
containing modules that elasticsearch uses.

closes #31845

rjernst added some commits Jan 29, 2019

Deprecate fallback to java on PATH
Finding java on the path is sometimes confusing for users and
unexpected, as well as leading to a different java being used than a
user expects.  This commit adds warning messages when starting
elasticsearch (or any tools like the plugin cli) and using java found
on the PATH instead of via JAVA_HOME.
Bundle java in distributions
Setting up a jdk is currently a required external step when installing
elasticsearch. This is particularly problematic for the rpm/deb packages
as installing a jdk in the same package installation command does not
guarantee any order, so must be done in separate steps. Additionally,
JAVA_HOME must be set and often causes problems in selecting a correct
jdk when, for example, the system java is an older unsupported version.

This commit bundles platform specific openjdks into each distribution.
In addition to eliminating the issues above, it also presents future
possible improvements like using jlink to build jdk images only
containing modules that elasticsearch uses.

closes #31845
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2019

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2019

I am still working on getting packaging tests right, but I opened this so reviewers can start looking at the general approach.

@rjernst rjernst requested review from jasontedor and atorok Jan 30, 2019

@atorok
Copy link
Contributor

left a comment

Look good overall, left some comments.

@@ -668,7 +668,6 @@ class ClusterFormationTasks {
static Task configureExecTask(String name, Project project, Task setup, NodeInfo node, Object[] execArgs) {
return project.tasks.create(name: name, type: LoggedExec, dependsOn: setup) { Exec exec ->
exec.workingDir node.cwd
exec.environment 'JAVA_HOME', node.getJavaHome()

This comment has been minimized.

Copy link
@atorok

atorok Jan 30, 2019

Contributor

is there reason to keep node.javaHome around ?
Maybe at least print a warning for now to make it obvious it's not being picked up.

On second taught, I'm not sure we want to use the bundled java here. We are rotating runtime java version in CI for this AFAIK and I think we should keep that, so all the bundled vs non bundled Java
testing would be done in packaging tests only and rest tests will continue to use a java version as provided.

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 30, 2019

Author Member

I don't think there is a need to keep java home here. IMO the integration tests should be as true to the expected default setup as they can be. We will have packaging tests to ensure the override behavior works.

This comment has been minimized.

Copy link
@atorok

atorok Jan 31, 2019

Contributor

My understanding is that with this change setting ES_RUNTIME_JAVA will no longer work, as getJavaHome() returns project.runtimeJavaHome when nothing specific is configured. So our CI tests that are supposed to test different runtime java versions like 8 and 12 will just use the bundled one.

This comment has been minimized.

Copy link
@rjernst

rjernst Feb 11, 2019

Author Member

Note on this that I reversed my previous thinking and added support for RUNTIME_JAVA_HOME. This is because I have experienced a bug with jdk 11.0.2 on macos which prevents Elasticsearch from starting. I haven't been able to get anyone else to replicate this issue, but the support of RUNTIME_JAVA_HOME is the only thing that allows me to run integ tests on my laptop at all with this change.

String jdkBuild = bundledJdkVersion[3]
repositories {
ivy {
url "https://download.java.net"

This comment has been minimized.

Copy link
@atorok

atorok Jan 30, 2019

Contributor

I'm worried about this in CI.
I assume this being a repository it does benefit from caching, and the artifacts will be pulled down by running the resolveAllDependencies task.
As long as we make sure that this is true I'm fine with the change.

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 30, 2019

Author Member

Yes, resolveAllDependencies is triggering the download:

:distribution:resolveAllDependencies > Resolve dependencies of :distribution:jdk_darwin > openjdk-11.0.1_osx-x64_bin.xml

* Note the "patch" version is not yet handled here, as it has not yet
* been used by java.
*/
public static List<String> getBundledJdk() {

This comment has been minimized.

Copy link
@atorok

atorok Jan 30, 2019

Contributor

Too bad that Gradles JavaVersion doesn't have all this information.
Given how broadly this class could be used it might be better to wrap the list in a class, or move the parsing logic to the build script that uses it ?

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 30, 2019

Author Member

I don't think it should be used broadly? I'm fine moving it to distribution/build.gradle if you insist, but I think it makes sense here since VersionProperties is all about the versions.properties file where this setting lives.

This comment has been minimized.

Copy link
@atorok

atorok Jan 31, 2019

Contributor

I'm ok to return it as a string from here and do the parsing in distribution/build.gradle since it's only needed there.

rjernst added some commits Jan 31, 2019

@@ -665,6 +665,12 @@ class ClusterFormationTasks {
static Task configureExecTask(String name, Project project, Task setup, NodeInfo node, Object[] execArgs) {
return project.tasks.create(name: name, type: LoggedExec, dependsOn: setup) { Exec exec ->
exec.workingDir node.cwd
if (project.runtimeJavaHome.equals(project.compilerJavaHome)) {

This comment has been minimized.

Copy link
@atorok

atorok Feb 4, 2019

Contributor

We have the version of the bundled JDK in build-tools.
Rather than assuming that it will match compilerJavaHome I think it would make more sense to have this conditional based on that. If the runtimeJavaVersion matches the bundled JDK version that use that instead.

This comment has been minimized.

Copy link
@rjernst

rjernst Feb 4, 2019

Author Member

The purpose of this conditional is to allow testing with Java home set. This is just checking whether runtime Java home was set, since if it wasn't it would be the same as the compiler Java. If we compared to the bundled version, we would likely never test the bundled jdk as devs aren't always on the same version on their machines as we are bundling.

This comment has been minimized.

Copy link
@atorok

atorok Feb 5, 2019

Contributor

I wasn't thinking about matching the version exactly, just the major, since that's what our matrix CI jobs also care about and what's included in the reproduction lines.
I don't think the conditional would behave differently, but it would convey intent better without relying on an implementation detail in the build plugin.

This comment has been minimized.

Copy link
@rjernst

rjernst Feb 11, 2019

Author Member

The intent is to test the default/common behavior by default. That is, to test the bundled jdk. If runtime java home is set, this tests whatever is there, whether is is an external 11 or 8. That seems straightforward to me.

This comment has been minimized.

Copy link
@atorok

atorok Feb 12, 2019

Contributor

I wouldn't have a problem with that, but we are not testing directly if ES_RUNTIME_JAVA is passed in, but rely on knowledge about how this is passed along instead. I think that these are worth avoiding because it makes the build harder to maintain. In other words looking at just this line and ignoring everything else you know about the build would make you wonder why the java used for compilation would matter in weather we do or do not use the bounded JDK ?

This comment has been minimized.

Copy link
@rjernst

rjernst Feb 28, 2019

Author Member

@atorok Tests are passing now. Do you have a concrete suggestion here? I do not want to compare anything about runtimeJavaHome to the version being bundled. Just because they are the same major version of java does not mean they are the same.

This comment has been minimized.

Copy link
@rjernst

rjernst Mar 1, 2019

Author Member

I added an extension property to indicate whether runtime java home is set in
29ec298

rjernst added some commits Feb 5, 2019

@jasontedor jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019

rjernst added some commits Feb 21, 2019

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/bwc

@rjernst rjernst added v7.0.0 v7.2.0 and removed WIP labels Feb 28, 2019

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

@elasticmachine
run elasticsearch-ci/1
run elasticsearch-ci/2
run elasticsearch-ci/bwc

rjernst added some commits Feb 28, 2019

@atorok

atorok approved these changes Mar 1, 2019

Copy link
Contributor

left a comment

LGTM

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

@elasticmachine
run elasticsearch-ci/bwc
run elasticsearch-ci/default-distro

@rjernst rjernst merged commit fef2153 into elastic:master Mar 5, 2019

9 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@rjernst rjernst deleted the rjernst:bundle_jdk2 branch Mar 5, 2019

rjernst added a commit to rjernst/elasticsearch that referenced this pull request Mar 5, 2019

Bundle java in distributions (elastic#38013)
* Bundle java in distributions

Setting up a jdk is currently a required external step when installing
elasticsearch. This is particularly problematic for the rpm/deb packages
as installing a jdk in the same package installation command does not
guarantee any order, so must be done in separate steps. Additionally,
JAVA_HOME must be set and often causes problems in selecting a correct
jdk when, for example, the system java is an older unsupported version.

This commit bundles platform specific openjdks into each distribution.
In addition to eliminating the issues above, it also presents future
possible improvements like using jlink to build jdk images only
containing modules that elasticsearch uses.

closes elastic#31845

rjernst added a commit to rjernst/elasticsearch that referenced this pull request Mar 5, 2019

Bundle java in distributions (elastic#38013)
* Bundle java in distributions

Setting up a jdk is currently a required external step when installing
elasticsearch. This is particularly problematic for the rpm/deb packages
as installing a jdk in the same package installation command does not
guarantee any order, so must be done in separate steps. Additionally,
JAVA_HOME must be set and often causes problems in selecting a correct
jdk when, for example, the system java is an older unsupported version.

This commit bundles platform specific openjdks into each distribution.
In addition to eliminating the issues above, it also presents future
possible improvements like using jlink to build jdk images only
containing modules that elasticsearch uses.

closes elastic#31845

rjernst added a commit that referenced this pull request Mar 8, 2019

Bundle java in distributions (#38013)
* Bundle java in distributions

Setting up a jdk is currently a required external step when installing
elasticsearch. This is particularly problematic for the rpm/deb packages
as installing a jdk in the same package installation command does not
guarantee any order, so must be done in separate steps. Additionally,
JAVA_HOME must be set and often causes problems in selecting a correct
jdk when, for example, the system java is an older unsupported version.

This commit bundles platform specific openjdks into each distribution.
In addition to eliminating the issues above, it also presents future
possible improvements like using jlink to build jdk images only
containing modules that elasticsearch uses.

closes #31845

rjernst added a commit that referenced this pull request Mar 8, 2019

Bundle java in distributions (#38013) (#39710)
* Bundle java in distributions

Setting up a jdk is currently a required external step when installing
elasticsearch. This is particularly problematic for the rpm/deb packages
as installing a jdk in the same package installation command does not
guarantee any order, so must be done in separate steps. Additionally,
JAVA_HOME must be set and often causes problems in selecting a correct
jdk when, for example, the system java is an older unsupported version.

This commit bundles platform specific openjdks into each distribution.
In addition to eliminating the issues above, it also presents future
possible improvements like using jlink to build jdk images only
containing modules that elasticsearch uses.

closes #31845

@michaelbaamonde michaelbaamonde added v7.0.0-rc1 and removed v7.0.0 labels Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.