Update elasticsearch plugin to require ES to have the same version as Kibana. #8229

cjcenizal · 2016-09-12T19:54:51Z

Problem

We were previously configuring the elasticsearch plugin with a hard-coded engineVersion value, and then using the semver package to ensure ES versions matched it.

Now that we're locking Kibana and ES versions, we have new rules that determine whether an ES version is compatible, incompatible, or merits a warning with a version of Kibana (This chart is outdated, scroll down for a newer one):

🚫 Incompatible

⚠️ Warning

💚 Compatible

Kibana version	ES -1.0.0	ES 1.-1.0	ES 1.1.-1	ES 1.1.1
-1.0.0	💚	🚫	🚫	🚫
1.-1.0	🚫	💚	⚠️	⚠️
1.1.-1	🚫	🚫	💚	⚠️
1.1.1	🚫	🚫	🚫	💚

Changes

Remove engineVersion from elasticsearch plugin config.
Use Kibana package.json version instead.
Use new rules, documented in README.
Log warning if ES is newer than Kibana.
Add isEsCompatibleWithKibana, isEsNewerThanKibana, parseVersionNumbers, and kibanaVersion.
Remove versionSatisfies.

spalger · 2016-09-13T01:55:15Z

esvm is updated and master snapshots should now match kibana version

cjcenizal · 2016-09-13T04:41:42Z

jenkins, test this

spalger · 2016-09-13T04:48:53Z

README.md

+
+🚫 **Fatal errors** are caused by a) any differences in major version and b) Elasticsearch having an older version number than Kibana.
+
+⚠️ **Server warnings** are displayed in the terminal if Elasticsearch has a newer minor or patch version number than Kibana. Kibana will still run, but only to make it easier for you to upgrade it to match the Elasticsearch version. You should try to run the same versions of Kibana and Elasticsearch at all times.


How about **Server warnings** are logged by the Kibana Server if Elasticsearch.... Many people won't be running Kibana via the command line, and will only see the logging output if they look at the log files.

Great point

spalger · 2016-09-13T05:06:26Z

I realize that the previous implementation did much of this, but I would prefer that we didn't reimplement the methods provided by the semver module for parsing and comparing versions.

I think we could delete 100+ lines of code+tests by switching to semver.gt(esVersion, kibanaVersion) and semver.major/minor/patch(version)

spalger · 2016-09-13T05:09:42Z

src/core_plugins/elasticsearch/lib/health_check.js

+// the same version defined in Kibana's package.json.
+// import {
+//   version as kibanaVersion,
+// } from '../../../../package.json';


Should probably delete this comment

Clearly I didn't review this PR myself before asking for a review!

cjcenizal · 2016-09-13T18:45:07Z

New comparison chart:

spalger · 2016-09-13T18:45:13Z

src/core_plugins/elasticsearch/lib/check_es_version.js

-      return 'Elasticsearch v' + node.version + ' @ ' + node.http_address + ' (' + node.ip + ')';
+    _.forEach(info.nodes, esNode => {
+      if (!isEsCompatibleWithKibana(esNode.version, kibanaVersion)) {
+        incompatibleNodes.push(esNode);


We should probably return here, right?

epixa · 2016-09-13T18:51:32Z

src/core_plugins/elasticsearch/lib/check_es_version.js


-    const badNodeNames = badNodes.map(function (node) {
-      return 'Elasticsearch v' + node.version + ' @ ' + node.http_address + ' (' + node.ip + ')';
+    _.forEach(info.nodes, esNode => {


Rather than imperatively building these arrays in the same loop, how about just filtering?

const incompatibleNodes = info.nodes.filter(esNode => { return !isEsCompatibleWithKibana(esNode.version, kibanaVersion); }) const warningNodes = info.nodes.filter(esNode => { return semver.gt(esNode.version, kibanaVersion); });

This would collect warningNodes for versions of ES that have a newer major number, but those should only go into the incompatibleNodes. So it's better to do them in one loop and exit early as @spalger suggested above.

pickypg · 2016-09-13T18:53:30Z

New comparison chart:

Should we actively block Kibana from working with a patch-release of ES that's older than it? If I happen to upgrade Kibana before completing a rolling upgrade of ES, then I think that I would be quite annoyed to find that it was broken until I finished the rolling upgrade. More importantly, the unified release of the full stack will mean that inevitably some ES patch releases (and Kibana patch releases) will be unchanged.

If there's something broken in ES 6.1.1 (the example above), then we can manually add the exception to Kibana 6.1.2 to prevent it from working with it.

I think the comms between Kibana and ES are stable enough at this point that we should be able to largely handle version discrepancies (5.0+), with features that draw the line in the sand causing hard breaks.

spalger · 2016-09-13T19:29:17Z

LGTM, I think that this PR is a great step in the right direction for es/kibana compatibility.

@pickypg While I think that your suggestion makes logical sense in terms of what versions should mean, I worry it might add undue complexity to this new compatibility/upgrade story.

While this discussion has certainly been had elsewhere, and I'm sure this suggestion was explicitly decided against for good reasons, my feelings about it totally revolve around keeping the compatibility story as memorable and predictable as possible.

Saying "you must always upgrade Elasticsearch before Kibana" seems easier to remember than "you must always upgrade Elasticsearch before Kibana, unless it's simply a patch upgrade, in which case you can upgrade Kibana first or even outright ignore Elasticseach, unless there is an exception in table X."

pickypg · 2016-09-13T19:34:05Z

Saying "you must always upgrade Elasticsearch before Kibana" seems easier to remember than "you must always upgrade Elasticsearch before Kibana, unless it's simply a patch upgrade, in which case you can upgrade Kibana first or even outright ignore Elasticseach, unless there is an exception in table X."

I agree that the former is definitely simpler and this PR is a simplification on the current story. But it's also a lot more common for users to go from x.y to x.[y + n] where n > 1 and I don't see that in the chart above (only where n = 1).

spalger · 2016-09-13T19:35:51Z

Good point, as long as Elasticsearch upgrades first that is supported, but we should add an example of that to the chart.

spalger · 2016-09-13T19:37:17Z

README.md

+| 6.1.2          | 6.1.2      | 💚      | Versions are the same. |
+| 6.1.2          | 6.1.3      | ⚠️      | ES patch number is newer. |
+| 6.1.2          | 6.2.0      | ⚠️      | ES major number is newer. |
+| 6.1.2          | 7.0.0      | 🚫      | ES major number is newer. |


@pickypg pointed out that we don't illustrate running from 6.1 against something like 6.5, which would be supported right?

See #8229 (comment)

epixa · 2016-09-13T19:38:06Z

We can always relax version checks, but it's hard to make them more restrictive. The change being done here is the implementation of a larger "how to upgrade the entire elastic stack" discussion that happened some months back.

Let's not get too carried away with this chart, by the way. There's an effort going on to build "upgrade" docs for the whole stack and such, which is where detailed descriptions about the upgrade process should be. People don't and shouldn't have to read our readme for this kind of stuff. The original reason why CJ wanted to add the chart and such to the readme was to better document to developers how our version checking works.

… Kibana. - Remove engineVersion from elasticsearch plugin config. - Use Kibana package.json version instead. - Use new rules, documented in README. - Log warning if ES is newer than Kibana. - Add isEsCompatibleWithKibana and kibanaVersion. - Remove versionSatisfies.

ppf2 · 2016-09-13T20:40:52Z

I like that we are working towards syncing up the version across the stack :)

Should we actively block Kibana from working with a patch-release of ES that's older than it?

+1. As much as we hope that users will read the docs before the upgrade, the reality is that some users will not (or miss the important points) :( so it will be nice to do a block with a descriptive message when Kibana detects that the ES version it is connecting to is running an incompatible version.

The chart is showing that if the ES minor version is higher, it will throw a warning. Do we intend to support higher ES minor version (may have breaking changes in ES), or just higher ES patch version (typically no breaking changes in ES for patch versions)?
All the version combinations with "Compatible" and "Warning" are the ones we will be formally testing correct?
Also, what does warning mean here (well I know that it means ES patch/minor version number is higher)? But what does it mean from an operational/supportability point of view, eg. do we mean that the ES version is supported only during the time of a full cross stack upgrade (i.e before Kibana upgrades to the same ES version)? Or does the warning here means that the version combination is not tested?, etc..

tsullivan · 2016-09-13T23:51:17Z

LGTM

--------- **Commit 1:** Update elasticsearch plugin to require ES to have the same version as Kibana. - Remove engineVersion from elasticsearch plugin config. - Use Kibana package.json version instead. - Use new rules, documented in README. - Log warning if ES is newer than Kibana. - Add isEsCompatibleWithKibana and kibanaVersion. - Remove versionSatisfies. * Original sha: 330e57d * Authored by CJ Cenizal <[email protected]> on 2016-09-09T22:36:09Z

spalger · 2016-09-14T03:12:51Z

@ppf2

The chart is showing that if the ES minor version is higher, it will throw a warning. Do we intend to support higher ES minor version (may have breaking changes in ES), or just higher ES patch version (typically no breaking changes in ES for patch versions)?

Yes, we will support but warn about users running a version of elasticsearch that has a higher minor or patch version than Kibana. This way, users can upgrade elasticsearch first and then Kibana once that is complete.

All the version combinations with "Compatible" and "Warning" are the ones we will be formally testing correct?

Good question :) @LeeDr?

But what does it mean from an operational/supportability point of view

I think the idea is that Elasticsearch will be stable enough through a major version to support Kibana, as long as a minimum minor version is met, but @epixa can probably add some color here.

ppf2 · 2016-09-14T05:40:22Z

@cjcenizal suggested earlier today to open a separate ticket for the ongoing discussion since the PR has been merged so I will move the outstanding discussion paragraphs to the new issue.

Btw, looking at the merged readme file again, I think we mean to say "ES minor number is newer." here ?

cjcenizal · 2016-09-14T15:06:14Z

Great catch @ppf2 ! Thanks!

cjcenizal force-pushed the feature/es-version-checking branch 2 times, most recently from 2776df4 to be4d4ab Compare September 12, 2016 20:01

cjcenizal mentioned this pull request Sep 12, 2016

Adds concept of kibanaVersion to plugin installer #8222

Merged

cjcenizal force-pushed the feature/es-version-checking branch from be4d4ab to 89a4585 Compare September 13, 2016 04:33

cjcenizal changed the title ~~WIP: Update elasticsearch plugin to require ES to have the same version as Kibana.~~ Update elasticsearch plugin to require ES to have the same version as Kibana. Sep 13, 2016

cjcenizal assigned epixa and spalger Sep 13, 2016

spalger reviewed Sep 13, 2016
View reviewed changes

cjcenizal force-pushed the feature/es-version-checking branch 2 times, most recently from 26f8b1c to 48f9c9f Compare September 13, 2016 18:44

spalger reviewed Sep 13, 2016
View reviewed changes

cjcenizal force-pushed the feature/es-version-checking branch from 48f9c9f to baf49e6 Compare September 13, 2016 18:45

epixa reviewed Sep 13, 2016
View reviewed changes

cjcenizal force-pushed the feature/es-version-checking branch from baf49e6 to b479698 Compare September 13, 2016 19:01

spalger reviewed Sep 13, 2016
View reviewed changes

cjcenizal force-pushed the feature/es-version-checking branch from b479698 to 330e57d Compare September 13, 2016 19:40

cjcenizal merged commit 9bd8e9a into elastic:master Sep 13, 2016

cjcenizal deleted the feature/es-version-checking branch September 13, 2016 23:54

elastic-jasper mentioned this pull request Sep 14, 2016

[backport] PR #8229 to 5.x #8264

Closed

elastic-jasper mentioned this pull request Sep 14, 2016

[backport] PR #8229 to 5.0 #8265

Closed

cjcenizal mentioned this pull request Sep 14, 2016

[backport] 5.x - Update elasticsearch plugin to require ES to have the same version as Kibana. #8266

Merged

ppf2 mentioned this pull request Sep 14, 2016

Discuss tweaking version compatibility with elasticsearch #8267

Closed

epixa added the v5.0.0 label Oct 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update elasticsearch plugin to require ES to have the same version as Kibana. #8229

Update elasticsearch plugin to require ES to have the same version as Kibana. #8229

cjcenizal commented Sep 12, 2016 •

edited

Loading

spalger commented Sep 13, 2016

cjcenizal commented Sep 13, 2016

spalger Sep 13, 2016

cjcenizal Sep 13, 2016

spalger commented Sep 13, 2016 •

edited

Loading

spalger Sep 13, 2016

cjcenizal Sep 13, 2016 •

edited

Loading

cjcenizal commented Sep 13, 2016

spalger Sep 13, 2016

epixa Sep 13, 2016

cjcenizal Sep 13, 2016

pickypg commented Sep 13, 2016

spalger commented Sep 13, 2016

pickypg commented Sep 13, 2016

spalger commented Sep 13, 2016

spalger Sep 13, 2016

spalger Sep 13, 2016

epixa commented Sep 13, 2016

ppf2 commented Sep 13, 2016

tsullivan commented Sep 13, 2016

spalger commented Sep 14, 2016

ppf2 commented Sep 14, 2016

cjcenizal commented Sep 14, 2016


		🚫 Fatal errors are caused by a) any differences in major version and b) Elasticsearch having an older version number than Kibana.

		⚠️ Server warnings are displayed in the terminal if Elasticsearch has a newer minor or patch version number than Kibana. Kibana will still run, but only to make it easier for you to upgrade it to match the Elasticsearch version. You should try to run the same versions of Kibana and Elasticsearch at all times.

Update elasticsearch plugin to require ES to have the same version as Kibana. #8229

Update elasticsearch plugin to require ES to have the same version as Kibana. #8229

Conversation

cjcenizal commented Sep 12, 2016 • edited Loading

Problem

Changes

spalger commented Sep 13, 2016

cjcenizal commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Sep 13, 2016 • edited Loading

Choose a reason for hiding this comment

cjcenizal Sep 13, 2016 • edited Loading

Choose a reason for hiding this comment

cjcenizal commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pickypg commented Sep 13, 2016

spalger commented Sep 13, 2016

pickypg commented Sep 13, 2016

spalger commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epixa commented Sep 13, 2016

ppf2 commented Sep 13, 2016

tsullivan commented Sep 13, 2016

spalger commented Sep 14, 2016

ppf2 commented Sep 14, 2016

cjcenizal commented Sep 14, 2016

cjcenizal commented Sep 12, 2016 •

edited

Loading

spalger commented Sep 13, 2016 •

edited

Loading

cjcenizal Sep 13, 2016 •

edited

Loading