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

Do not use runtime Java when starting BWC nodes #43071

Merged
merged 1 commit into from
Jun 11, 2019

Conversation

jasontedor
Copy link
Member

When starting BWC nodes, it could be that runtime Java home is set. Yet, runtime Java home can advance beyond what a BWC node might be compatible with. For example, if runtime Java home is set to JDK 13 and we are starting a 7.1.2 node, we do not have any guarantees that 7.1.2 is compatible with JDK 13 (since we never did any work to make it so). This will continue to be the case as JDK releases advance, but we still need to test against BWC nodes. This commit stops applying runtime Java home when starting a BWC node. Instead, we would use the bundled JDK.

Closes #43024

When starting BWC nodes, it could be that runtime Java home is set. Yet,
runtime Java home can advance beyond what a BWC node might be compatible
with. For example, if runtime Java home is set to JDK 13 and we are
starting a 7.1.2 node, we do not have any guarantees that 7.1.2 is
compatible with JDK 13 (since we never did any work to make it so). This
will continue to be the case as JDK releases advance, but we still need
to test against BWC nodes. This commit stops applying runtime Java home
when starting a BWC node. Instead, we would use the bundled JDK.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM, but does this need a similar change in testclusters? In ElasticsearchNode.getESEnvironment() we similarly pass JAVA_HOME through.

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

Ditto on @rjernst's comment about updating ElasticsearchNode as well for projects using TestClustersPlugin.

@@ -107,6 +108,9 @@ class NodeInfo {
/** the version of elasticsearch that this node runs */
Version nodeVersion

/** true if the node is not the current version */
boolean isBwcNode
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably make this final.

@alpar-t
Copy link
Contributor

alpar-t commented Jun 11, 2019

We don't use testclusters for BWC yet, so we are fine on that front.

I just want to point out that we are somewhat reducing coverage by this change. In our java matrix testing in CI we cycle trough all meaningful runtime java versions. Before this change we would test with the BWC version running on each, which is problematic as you point out that it could be that we add one for the newer version that is not supported by the old version, but we would circle trough all those that we do support. With this change we would only ever use the embedded JDK. Runtime java is always set in CI ( we have #40531 to address that ) so without this change we didn't cover running bwc with the embedded JDK.

To be able to reason about the version of the JDK used here we would need to embed information in the distribution about what it supports, since I don't think we have access to that information in cluster formation tasks right now. With this metadata we could still use runtime java if it's set to something that the bwc version supports.

I'm not arguing we should do it, I'm not convinced either, just wanted to point it out.
On one hand, using the same runtime java is more realistic for a BWC scenario ( users will probably not upgrade java and elasticsearch at the exact same time ) but on the other hand both the old and the new versions are independently tested against all the java versions they support, so running into something that is java dependent and only manifests in a bwc test seems unlikely. On the other hand there's not much additional CI cost to cover it ( but there will be once we do #40531 to cover the embedded java explicitly ).

@jasontedor
Copy link
Member Author

I just want to point out that we are somewhat reducing coverage by this change.

I am okay with this, as you mention below, we test each version against all the versions that it supports. The BWC tests are to give us confidence that our supported upgrade paths are successful, the likelihood of an issue arising in a BWC scenario that doesn't arise in the runtime matrix tests for the version being upgraded from is extremely unlikely. We will never have 100% coverage, our goal is reasonably high confidence, and I think we have that even without sweeping over every runtime Java for the BWC versions.

On one hand, using the same runtime java is more realistic for a BWC scenario ( users will probably not upgrade java and elasticsearch at the exact same time ) but on the other hand both the old and the new versions are independently tested against all the java versions they support, so running into something that is java dependent and only manifests in a bwc test seems unlikely.

We are in agreement, see above, that's why I am not worried about the loss of coverage here. To add, it's our expectation that over time users would be using the bundled JDK anyway (they certainly are in the Docker images).

@jasontedor jasontedor merged commit 76b02dd into elastic:master Jun 11, 2019
@jasontedor jasontedor deleted the no-runtime-java-for-bwc-nodes branch June 11, 2019 14:22
jasontedor added a commit that referenced this pull request Jun 11, 2019
When starting BWC nodes, it could be that runtime Java home is set. Yet,
runtime Java home can advance beyond what a BWC node might be compatible
with. For example, if runtime Java home is set to JDK 13 and we are
starting a 7.1.2 node, we do not have any guarantees that 7.1.2 is
compatible with JDK 13 (since we never did any work to make it so). This
will continue to be the case as JDK releases advance, but we still need
to test against BWC nodes. This commit stops applying runtime Java home
when starting a BWC node. Instead, we would use the bundled JDK.
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team v7.3.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] openjdk13 full-cluster-restart failures
6 participants