Remove more uses of Version#minimumCompatibilityVersion #99988

williamrandolph · 2023-09-27T21:42:59Z

#99834 removed string uses of minimumCompatibilityVersion, but left some of the nearby logic and comparisons untouched. This PR adds some static methods to Build to help with comparisons and error messages for version Strings, and uses them where possible.

elasticsearchmachine · 2023-09-28T02:57:46Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

server/src/main/java/org/elasticsearch/Build.java

ldematte · 2023-09-29T09:34:14Z

server/src/main/java/org/elasticsearch/env/NodeEnvironment.java

@@ -525,19 +525,22 @@ static void checkForIndexCompatibility(Logger logger, DataPath... dataPaths) thr
        logger.info("oldest index version recorded in NodeMetadata {}", metadata.oldestIndexVersion());

        if (metadata.oldestIndexVersion().isLegacyIndexVersion()) {
+            String bestDowngradeVersion = Build.isIndexCompatibleWithCurrentAllowingLegacy(metadata.previousNodeVersion().toString())


I wonder if a call to isIndexCompatibleWithCurrentAllowingLegacy is really needed.
Qe are passing down a Version (metadata.previousNodeVersion().toString()).
Wouldn't it be better to just have the Version logic here? (metadata.previousNodeVersion()..onOrAfter(Version.CURRENT.minimumCompatibilityVersion()))

Also, I think we are mixing 2 concepts here: the message is telling to the user: "there are indices with <too old IndexVersion> which cannot be handled by this Node of < "Build" version >. Downgrade the Node to < X >

Where X could be either a Version or a IndexVersion. Both can be confusing as neither would really match "Build" version; probably we can consider Version a good enough replacement for "Build" version, but in case of the second one I think we are comparing different things (we could tell the user to downgrade to 8.50.0 (an IndexVersion) or similar).

This could be a non-issue as we are in the "legacy index version" path, so in that era IndexVersion and Version probably match, but it lets me puzzled.

Ideally I think here we should have a IndexVersion based logic, plus a IndexVersion -> Build (or Version -- or era?) mapping. But I wonder if I am missing something - or maybe it has already been decided this is a direction we do not want to go.

That's a good point. I think we should return what's most useful to an on-prem user, because on-disk index compatibility will be less of an issue in environments that we're managing. This message should give the user what they need to figure out which Elasticsearch artifact needs to be installed, and I don't think an IndexVersion would be helpful there.

I agree. I think the best would be to have the check based purely on IndexVersion, and the messaging purely on Version (via Build, possibly)

I did some thinking, and I believe I understand better what the original version maximum check was doing. In the case where we have an 8.x node starting up, an incompatible index would be from 6.x or earlier. We want to tell the user to downgrade to 7.17.x. If the last installation was from 7.16.x or earlier, we can tell them to go to minimum wire compat version (7.17.0). Otherwise, we tell them to go to the version they were previously using, for example, 7.17.5.

The minimum index version is at the start of the last major, meaning 7.0.0 in this case. We definitely don't want to tell the user to downgrade to that!

I think your reasoning is sound, but I have concerns about using wire compatibility strings to express index compatibility. This may work now, as Version and TransportVersion are aligned, but may stop working soon - what if we make a Transport breaking change, update the wire version, but still have the prev-major index compatibility? E.g. minimum wire compat version becomes for unrelated reasons 8.1.0 - the message now would be wrong.

On the other hand, I don't know what we could do better.
One option could be to hardcode the fallback version - add a Build.indexCompatibilityDowngradeVersion() that in on-prem would return Version.V_7_17_14. That would need to be updated though.
One option maybe to express it as a general message (e.g. "the most recent revision of the previous major ES version" - possibly better written then this!)
I would also consider keeping the message minimal/generic and adding links to relevant docs (e.g. ReferenceDocs.ARCHIVE_INDICES). More precise instructions can then be added to the docs.

I don't know why I'm having such a hard time thinking through what will happen with this code when breaking changes happen. It's in a context that's very specific to the 7.x -> 8.x upgrade, but who's to say it won't get re-used elsewhere.

I really don't like the idea of making the user go cross-reference things in documentation when we have a way of providing that information directly. If I were trying to figure out why my upgrade failed, I'd much prefer "go back to 7.17.5" (for which I might already have a distribution file ready) to "go find and install the distribution for the latest bugfix version of the previous major version's latest minor version."

I think I'm just struggling with how hard it's becoming to express a reference to a user-facing release in terms that would be clear to someone who just needs to figure out what version to download.

I think I'm just struggling with how hard it's becoming to express a reference to a user-facing release in terms that would be clear to someone who just needs to figure out what version to download.

Yes! I think this is one major pain point of removing Version: it becomes really difficult to create useful messages for the user with the correct version information. Without a concept of "previous", or any link between IndexVersion/TransportVersion and downloadable artifacts, these lines becomes really difficult to create. I don't really have a good solution.
What about hardcoding? Build.indexCompatibilityDowngradeVersion(), always returing "7.17.x" on on-prem (and the usual hash in serverless - which BTW will generate a useless message?)
We can add an assert inside it (assert Version.CURRENT.major == 8) with a message that this has to be upgraded when we change major?

How about: if version matches regex ^7\.17\.[0-9]+$, output version, otherwise return "7.17.0" (or "7.17.x")?

I think using the regex is a good compromise! Consider adding an assertion too (to fail if it matches 9.?), so when the time comes and if we need to, we will be able to update it quickly.

ldematte

I have some doubts posted in the comments. It may well be I lack knowledge to understand the implications however.

ldematte · 2023-09-29T09:35:57Z

server/src/main/java/org/elasticsearch/env/NodeMetadata.java

@@ -113,7 +113,7 @@ public void verifyUpgradeToCurrentVersion() {
        assert (nodeVersion.equals(Version.V_EMPTY) == false) || (Version.CURRENT.major <= Version.V_7_0_0.major + 1)
            : "version is required in the node metadata from v9 onwards";

-        if (nodeVersion.before(Version.CURRENT.minimumCompatibilityVersion())) {
+        if (Build.isWireCompatibleWithCurrentAllowingLegacy(nodeVersion.toString()) == false) {
            throw new IllegalStateException(


Similar here: we are passing down a Version, so the first part of isWireCompatibleWithCurrentAllowingLegacy will always fail. Should we apply the logic here directly?

Moving forward, wire compatibility should be linked to TransportVersion, right? Should we do introduce a TransportVersion comparison here too?

There's a separate ticket for removing Version from NodeMetadata.

I think it makes sense to move the call to Version into Build so we can re-work how we do that sort of comparison in one place rather than in a bunch of places in the codebase.

I would also be okay leaving this as-is and handling it when we work on NodeMetadata.

ldematte · 2023-09-29T09:57:37Z

server/src/main/java/org/elasticsearch/env/NodeEnvironment.java

@@ -525,19 +525,22 @@ static void checkForIndexCompatibility(Logger logger, DataPath... dataPaths) thr
        logger.info("oldest index version recorded in NodeMetadata {}", metadata.oldestIndexVersion());

        if (metadata.oldestIndexVersion().isLegacyIndexVersion()) {
+            String bestDowngradeVersion = Build.isIndexCompatibleWithCurrentAllowingLegacy(metadata.previousNodeVersion().toString())


Also, I think we are mixing 2 concepts here: the message is telling to the user: "there are indices with <too old IndexVersion> which cannot be handled by this Node of < "Build" version >. Downgrade the Node to < X >

Where X could be either a Version or a IndexVersion. Both can be confusing as neither would really match "Build" version; probably we can consider Version a good enough replacement for "Build" version, but in case of the second one I think we are comparing different things (we could tell the user to downgrade to 8.50.0 (an IndexVersion) or similar).

This could be a non-issue as we are in the "legacy index version" path, so in that era IndexVersion and Version probably match, but it lets me puzzled.

Ideally I think here we should have a IndexVersion based logic, plus a IndexVersion -> Build (or Version -- or era?) mapping. But I wonder if I am missing something - or maybe it has already been decided this is a direction we do not want to go.

ldematte

I have left a couple of comments. Sorry for being maybe too cautious, but these are tricky: different types (index version, node version, transport version) are at play at the same time, and we are contrasting 2 needs: do not do "arithmetics" on Version (it's just a string) VS. giving the most useful user messages by "finding" the best version (which has math and comparison).
If I'm too pedantic, feel free to ignore/override me :)

server/src/main/java/org/elasticsearch/indices/SystemIndexDescriptor.java

ldematte · 2023-10-03T14:57:53Z

server/src/main/java/org/elasticsearch/env/NodeEnvironment.java

@@ -525,19 +525,22 @@ static void checkForIndexCompatibility(Logger logger, DataPath... dataPaths) thr
        logger.info("oldest index version recorded in NodeMetadata {}", metadata.oldestIndexVersion());

        if (metadata.oldestIndexVersion().isLegacyIndexVersion()) {
+            String bestDowngradeVersion = Build.isIndexCompatibleWithCurrentAllowingLegacy(metadata.previousNodeVersion().toString())


I think your reasoning is sound, but I have concerns about using wire compatibility strings to express index compatibility. This may work now, as Version and TransportVersion are aligned, but may stop working soon - what if we make a Transport breaking change, update the wire version, but still have the prev-major index compatibility? E.g. minimum wire compat version becomes for unrelated reasons 8.1.0 - the message now would be wrong.

On the other hand, I don't know what we could do better.
One option could be to hardcode the fallback version - add a Build.indexCompatibilityDowngradeVersion() that in on-prem would return Version.V_7_17_14. That would need to be updated though.
One option maybe to express it as a general message (e.g. "the most recent revision of the previous major ES version" - possibly better written then this!)
I would also consider keeping the message minimal/generic and adding links to relevant docs (e.g. ReferenceDocs.ARCHIVE_INDICES). More precise instructions can then be added to the docs.

ldematte

LGTM; one minor comment but I'll let you decide what's best :)

server/src/main/java/org/elasticsearch/Build.java

In #99988, one of the code changes just obfuscated a check against minimumCompatibilityVersion, rather than changing NodeMetadata in such a way that the comparison could work in serverless. This PR switches it back so that we can make a more comprehensive fix in NodeMetadata.

Remove more uses of Version#minimumCompatibilityVersion

5f2275c

williamrandolph requested review from ldematte and thecoop September 27, 2023 21:42

elasticsearchmachine added v8.11.0 needs:triage Requires assignment of a team area label labels Sep 27, 2023

williamrandolph added :Core/Infra/Core Core issues without another label >refactoring and removed needs:triage Requires assignment of a team area label labels Sep 28, 2023

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Sep 28, 2023

thecoop reviewed Sep 28, 2023

View reviewed changes

server/src/main/java/org/elasticsearch/Build.java Outdated Show resolved Hide resolved

williamrandolph added 2 commits September 28, 2023 16:52

Be more explicit about when to allow legacy versions

877fc75

Merge branch 'main' into version/node-metadata-checks

dae202e

thecoop reviewed Sep 29, 2023

View reviewed changes

server/src/main/java/org/elasticsearch/Build.java Outdated Show resolved Hide resolved

thecoop reviewed Sep 29, 2023

View reviewed changes

server/src/main/java/org/elasticsearch/Build.java Outdated Show resolved Hide resolved

ldematte reviewed Sep 29, 2023

View reviewed changes

williamrandolph added 2 commits September 29, 2023 11:05

Merge branch 'main' into version/node-metadata-checks

2658ed5

Respond to PR feedback

f5ae981

williamrandolph requested review from ldematte and thecoop September 29, 2023 19:21

williamrandolph added 2 commits September 29, 2023 15:24

Fix tests

284779d

Merge branch 'main' into version/node-metadata-checks

c158c44

ldematte reviewed Oct 3, 2023

View reviewed changes

Back out change to SystemIndexDescriptor

e303384

mattc58 added v8.12.0 and removed v8.11.0 labels Oct 4, 2023

williamrandolph added 2 commits October 4, 2023 10:38

Avoid mixing use of wire compat and index compat versions

110a1c0

Merge branch 'main' into version/node-metadata-checks

eff10bd

ldematte approved these changes Oct 4, 2023

View reviewed changes

server/src/main/java/org/elasticsearch/Build.java Outdated Show resolved Hide resolved

williamrandolph added 4 commits October 4, 2023 12:00

fix failing test

61a842a

Move static utility method from Build to NodeMetadata

881766e

Merge branch 'main' into version/node-metadata-checks

ffa626e

Merge branch 'main' into version/node-metadata-checks

6b1528a

williamrandolph merged commit 60daef3 into elastic:main Oct 4, 2023

williamrandolph deleted the version/node-metadata-checks branch October 4, 2023 19:27

williamrandolph mentioned this pull request Oct 31, 2023

Revert NodeMetadata check to simple comparison #101644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove more uses of Version#minimumCompatibilityVersion #99988

Remove more uses of Version#minimumCompatibilityVersion #99988

williamrandolph commented Sep 27, 2023

elasticsearchmachine commented Sep 28, 2023

ldematte Sep 29, 2023 •

edited

Loading

ldematte Sep 29, 2023

williamrandolph Sep 29, 2023

ldematte Sep 29, 2023

williamrandolph Sep 29, 2023

ldematte Oct 3, 2023

williamrandolph Oct 4, 2023

ldematte Oct 4, 2023 •

edited

Loading

williamrandolph Oct 4, 2023 •

edited

Loading

ldematte Oct 4, 2023

ldematte left a comment

ldematte Sep 29, 2023

ldematte Sep 29, 2023

williamrandolph Sep 29, 2023

ldematte Sep 29, 2023

ldematte left a comment

ldematte Oct 3, 2023

ldematte left a comment

Remove more uses of Version#minimumCompatibilityVersion #99988

Remove more uses of Version#minimumCompatibilityVersion #99988

Conversation

williamrandolph commented Sep 27, 2023

elasticsearchmachine commented Sep 28, 2023

ldematte Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldematte Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

williamrandolph Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldematte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldematte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldematte left a comment

Choose a reason for hiding this comment

ldematte Sep 29, 2023 •

edited

Loading

ldematte Oct 4, 2023 •

edited

Loading

williamrandolph Oct 4, 2023 •

edited

Loading