Introduce index.version.compatibility setting #83264

ywelsch · 2022-01-28T13:26:04Z

Introduces an index.version.compatibility setting (that defaults to index.version.created) that's used to determine whether the index can be handled by the current cluster.

Allows older (archive) indices (which have older index.version.created) to be imported with a higher index.version.compatibility version, and thereby be accepted by the nodes in the cluster. In the future, I can envision the capability of indices to even have the ability to "upgrade" to a newer index.version.compatibility version (after they undergo a series of checks which makes them eligible for "upgrade").

Relates #81210

elasticmachine · 2022-01-31T13:06:19Z

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

dnhatn

LGTM. Thanks Yannick!

henningandersen

I did an initial read of this and have a few questions/clarifications that I hope you can help with.

I also have a mild concern (that I need to investigate more) about how we have settings or logic that is based on created version but already removed support for older versions in master under the assumption that created-version >= 7. For instance index.soft_deletes.retention.operations (which is unlikely to be an issue). Maybe you have done some digging here already?

henningandersen · 2022-02-02T08:33:30Z

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java

                );
-                if (snapshotIndexMetadata.getCreationVersion()
-                    .before(currentState.getNodes().getMaxNodeVersion().minimumIndexCompatibilityVersion())) {
+                if (snapshotIndexMetadata.getCompatibilityVersion().before(minIndexCompatibilityVersion)) {


I wonder if we should add an opt-in option to this behavior. I see two ways this could help:

Avoid someone restoring an index in legacy mode inadvertently.

Ensure that you can also restore an index in legacy mode explicitly. Suppose I am on 7.latest and I want to restore a 6.x index in legacy mode to enable me to upgrade to 8. Could also be useful for testing/validation of long-term archival performance before users can even have a version that would trigger it for their data.

I see that we still have this under a feature flag so no changes needed here, but would be happy to hear your thoughts.

For the "restore/mount from snapshot" flow, I'm not sure how someone would "inadvertently" restore/mount an old index. They have chosen the index to be restored, so it should be restored (today such a restore would fail, in the future it will go through). It might help if we would show index version information via snapshot status APIs so that users know in advance which indices might be older and which ones not so that they can do a more informed choice on restore. This feels like a problem to be solved on the snapshot discovery APIs (see backlinks to #54940). WDYT?

For the major version upgrade of a cluster, there are a number of paths to explore. I'm less focusing right now on the second scenario you described, as that only becomes relevant once we are at the end of 8.x (as there is no 7.x releases of this feature), i.e. the only approach we can recommend in the immediate future is to allow users to take a snapshot in their old cluster, and restore that to a parallel 8.x cluster to try it out. What you're describing will certainly be useful once we go towards the next major. What I'm also thinking more deeply about right now is whether we can find a way to allow users to do a rolling / full cluster restart upgrade from 7.last to 8.x with 6.x indices still present in the cluster that satisfy certain conditions (e.g. warm/cold/frozen tiers) and that will be auto-upgraded to legacy mode, not having to trigger the process via snapshots. Here, we could also require users to take certain steps using existing APIs on 7.x (e.g. flush/mark index read-only) for this auto-upgrade to be successful.

I'm not sure how someone would "inadvertently" restore/mount an old index

One case is to do a restore of all indices. If you restore a 7.x snapshot in an 8.x one, I think we would silently degrade functionality and performance of 6.x indices. My intuition would be to error instead and then ask to add an explicit "archive everything before 7.0" mode in the request.

upgrade from 7.last to 8.x with 6.x indices still present in the cluster that satisfy certain conditions

I think our upgrade assistant would advice to reindex such data. I wonder if we would not need to extend that with this functionality - and thus let it explicitly set a flag to archive on upgrade (with a warning to upgrade to at least 8.some-version)?

One case is to do a restore of all indices. If you restore a 7.x snapshot in an 8.x one, I think we would silently degrade functionality and performance of 6.x indices. My intuition would be to error instead and then ask to add an explicit "archive everything before 7.0" mode in the request.

I'm not sure about the situation where you would encounter this (restore of all indices into next major):

Disaster recovery into a newer major version cluster than what you had when the snapshot was taken?

Some kind of upgrade process where the upgrade happens via snapshot?

I'm generally wary of having to require an extra flag, as it means that any existing place / UI that allows operating with / restoring snapshots will have to become aware of this flag. Not requiring an extra flag would make this new capability of restoring older indices directly available everywhere.

For the in-place upgrade (upgrading cluster to next major version, with some indices in the cluster now becoming legacy indices), I agree that this should not just happen silently.

OK, thanks for your thoughts on this, I do agree there is a trade-off here.

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

henningandersen · 2022-02-02T08:47:17Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

    @Deprecated
    public static final String SETTING_VERSION_UPGRADED_STRING = "index.version.upgraded_string";

+    public static final String SETTING_VERSION_COMPATIBILITY = "index.version.compatibility";


index.version.created concerns both metadata and data. I think this new settings concerns only metadata/mappings and perhaps we should make that explicit in the name?

I've been going back and forth on that myself a few times. It also concerns the data, but just a tiny bit (i.e. we "upgraded" the segments during recovery, creating a new commit point). Initially I thought about using a name like index.version.upgraded to denote that some "upgrade" has taken place, but that name was already taken by some legacy functionality.

henningandersen · 2022-02-02T10:29:03Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java


+    /**
+     * Return the {@link Version} that this index provides compatibility for.
+     * This is typically compared to the {@link Version#minimumIndexCompatibilityVersion()} to figure out whether the index can be handled


This is likely mostly my uncertainty on the project and intentions. It nags me a little that we instantiate the new mappings/metadata on a specific version and then guarantee to be able to use that on the next major, but no longer than that. Both aspects of that seems wrong and should really be tied to the created-version (if version 10 can read 5 indices, there is no need to do anything about an index restored in 7.x during upgrade to 8, 9 and 10).

I suppose we could upgrade metadata/mappings for every major. And also on minors in case we change how we derive the metadata/mapping. Is that part of the intention? It makes me wonder if we could do this completely dynamically (even at the shard level to support mixed clusters), but that also sounds complex.

I suppose we could upgrade metadata/mappings for every major. And also on minors in case we change how we derive the metadata/mapping. Is that part of the intention?

Correct, that's the intention with the compatibility version. Similarly to the created-version, it defines how "old" the index metadata is and what legacy stuff from the past it might contain. At the same time, I envision this to be at least auto-upgraded every major (preserving / remapping as much of the original metadata as possible). This is why again we have the natural N-1 compatibility with index.version.compatibility.

Note that as we preserve the original mapping as well (under _meta/legacy-mappings), it might make sense to remap across minors even to make use of some new features (e.g. doc-value-only fields for new field types).

henningandersen · 2022-02-02T10:46:29Z

...in/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/IndexDeprecationChecks.java

-        Version createdWith = indexMetadata.getCreationVersion();
-        if (createdWith.before(Version.V_7_0_0)) {
+        // TODO: this check needs to be revised. It's trivially true right now.
+        Version currentCompatibilityVersion = indexMetadata.getCompatibilityVersion();


I think this is right. But would we end up with a data-check as well, for instance if an upgrade from 8 to 9 means we no longer support ES 5 data, would we then have two checks go here?

Yeah, the policy for how long we want to provide support for older versions needs to be decoupled from the N-1 index compatibility policy. While the checks in this PR are now purely based on getCompatibilityVersion, in the future they would be based on both getCompatibilityVersion and getCreationVersion (e.g. we could have 9.x reject indices created in 5.x, even if getCompatibilityVersion() == 8.x).

henningandersen

I had another look, also at the 7.x use of the created setting. I found nothing concerning, so I am good with moving forward with introducing the setting. Left a couple more comments.

This might be the wrong PR, but would this check need to be added back in if we support querying from doc-values (not really sure)?

elasticsearch/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java

Line 265 in 6358afb

    
           DateFormatter defaultFormat = resolution == Resolution.NANOSECONDS && indexCreatedVersion.onOrAfter(Version.V_7_0_0)

henningandersen · 2022-02-07T12:00:02Z

server/src/main/java/org/elasticsearch/snapshots/RestoreService.java

+                    .put(snapshotIndexMetadata.getSettings())
+                    .put(
+                        IndexMetadata.SETTING_INDEX_VERSION_COMPATIBILITY.getKey(),
+                        clusterState.getNodes().getSmallestNonClientNodeVersion()


Can we use getMinNodeVersion or fail if getSmallestNonClientNodeVersion != getMinNodeVersion? The version barrier only has guarantees on getMinNodeVersion and it would be nice to rely on it here.

I've used the same logic as we use for index creation, as this felt similar in nature. I agree that the interplay with the version barrier is confusing, but it's something to be addressed everywhere then (index creation is probably more frequent than what's being done here).

I prefer to leave this to a follow-up, as it's a more general change to make (to index creation as well).

henningandersen · 2022-02-07T12:04:38Z

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java

+     * Return the {@link Version} that this index provides compatibility for.
+     * This is typically compared to the {@link Version#minimumIndexCompatibilityVersion()} to figure out whether the index can be handled
+     * by the cluster.
+     * By default, this is equal to the {@link #getCreationVersion()}, but can also be a newer version if the index has been imported as


I think we should adapt the javadoc on getCreationVersion to reflect that this new method exists, i.e., to say something like this should normally not be used for N-1 compatibility checks, see #getCompatibilityVersion.

fixed in f0d45c6

ywelsch · 2022-02-08T09:59:43Z

This might be the wrong PR, but would this check need to be added back in if we support querying from doc-values (not really sure)?

elasticsearch/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java

Line 265 in 6358afb

DateFormatter defaultFormat = resolution == Resolution.NANOSECONDS && indexCreatedVersion.onOrAfter(Version.V_7_0_0)

That version check does not make sense to me. date_nanos has been introduced in 7.0.0, and while the default here has been changed in a later version (but fully bwc change), I don't understand why we have that version check at all there. It means it's ok to change the behavior retroactively for 7.x indices, but not ok for 6.x indices? Weird. In any case, there's more testing to come for legacy indices once I'm adding support for auto-converting mappings.

ywelsch · 2022-02-08T10:53:14Z

I also have a mild concern (that I need to investigate more) about how we have settings or logic that is based on created version but already removed support for older versions in master under the assumption that created-version >= 7. For instance index.soft_deletes.retention.operations (which is unlikely to be an issue). Maybe you have done some digging here already?

Before working on this PR, I investigated how index settings changed over these major releases (e.g. deprecated / removed / changed behavior), and was positively surprised to see that very little has changed there, and almost nothing at all when it comes to the pure read path of ES (which is what archive is interested in). Mappings might have some version-specific logic, when it comes to the subset of the mappings that we're going to be interested in (doc-value-only fields / runtime fields), again, not much has changed. My goal is to follow-up with more thorough BWC testing for the parts of the mappings (doc-value-only fields) that I think we are able to carry forward across major versions.

henningandersen

LGTM, thanks for the additional information Yannick.

ywelsch · 2022-02-08T13:26:48Z

Thank you @dnhatn and @henningandersen!

* upstream/master: (166 commits) Bind host all instead of just _site_ when needed (elastic#83145) [DOCS] Fix min/max agg snippets for histograms (elastic#83695) [DOCS] Add deprecation notice for system indices (elastic#83688) Cache ILM policy name on IndexMetadata (elastic#83603) [DOCS] Fix 8.0 breaking changes sort order (elastic#83685) [ML] fix random sampling background query consistency (elastic#83676) Move internal APIs into their own namespace '_internal' Runtime fields core-with-mapped tests support tsdb (elastic#83577) Optimize calculating the presence of a quorum (elastic#83638) Use switch expressions in EnableAllocationDecider and NodeShutdownAllocationDecider (elastic#83641) Note libffi error message in tmpdir docs (elastic#83662) Fix TransportDesiredNodesActionsIT batch tests (elastic#83659) [DOCS] Remove unused upgrade doc files (elastic#83617) [ML] Wait for model process to stop in stop deployment (elastic#83644) [ML] Fix submit after shutdown in process worker service (elastic#83645) Remove req/resp classes associated with HLRC (elastic#83599) Introduce index.version.compatibility setting (elastic#83264) Rename InternalTestCluster#getMasterNodeInstance (elastic#83407) Mute TimeSeriesIndexSearcherTests testCollectInOrderAcrossSegments (elastic#83648) Add rollover add max_primary_shard_docs condition (elastic#80981) ... # Conflicts: # x-pack/plugin/rollup/build.gradle # x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/v2/RollupActionSingleNodeTests.java

Introduce index.version.current

934929a

elasticsearchmachine added the v8.1.0 label Jan 28, 2022

ywelsch added 3 commits January 28, 2022 14:38

fix version

23ac60a

register setting

e20a16f

Merge remote-tracking branch 'elastic/master' into index-current-version

df45f90

ywelsch mentioned this pull request Jan 28, 2022

Snapshots as simple archives #81210

Closed

32 tasks

ywelsch added 5 commits January 28, 2022 15:40

test fixø

bd3edfc

test

a5546ab

test fixø

293b2e9

fix

4026f06

fix

051787a

ywelsch requested a review from DaveCTurner January 31, 2022 08:07

ywelsch added :Core/Infra/Core Core issues without another label >non-issue labels Jan 31, 2022

Merge remote-tracking branch 'elastic/master' into index-current-version

5ca36cc

ywelsch removed the request for review from DaveCTurner January 31, 2022 12:04

rename

01021cd

ywelsch changed the title ~~Introduce index.version.current~~ Introduce index.version.compatible Jan 31, 2022

ywelsch changed the title ~~Introduce index.version.compatible~~ Introduce index.version.compatibility Jan 31, 2022

ywelsch requested a review from dnhatn January 31, 2022 13:06

ywelsch marked this pull request as ready for review January 31, 2022 13:06

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jan 31, 2022

ywelsch changed the title ~~Introduce index.version.compatibility~~ Introduce index.version.compatibility setting Jan 31, 2022

henningandersen self-requested a review January 31, 2022 16:19

dnhatn approved these changes Feb 1, 2022

View reviewed changes

henningandersen reviewed Feb 2, 2022

View reviewed changes

mark-vieira added v8.2.0 and removed v8.1.0 labels Feb 2, 2022

henningandersen reviewed Feb 7, 2022

View reviewed changes

ywelsch added 4 commits February 7, 2022 14:04

Merge remote-tracking branch 'elastic/master' into index-current-version

e26e644

review feedback

f0d45c6

Merge remote-tracking branch 'elastic/master' into index-current-version

b8f8aec

spotless

776100b

ywelsch requested a review from henningandersen February 8, 2022 10:53

henningandersen approved these changes Feb 8, 2022

View reviewed changes

ywelsch merged commit 09b8773 into elastic:master Feb 8, 2022

thecoop mentioned this pull request May 5, 2023

Index version compatibility setting does not use its validator #95873

Closed

Introduce index.version.compatibility setting #83264

Introduce index.version.compatibility setting #83264

Uh oh!

Conversation

ywelsch commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jan 31, 2022

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywelsch commented Feb 8, 2022

Uh oh!

ywelsch commented Feb 8, 2022

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

ywelsch commented Feb 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ywelsch commented Jan 28, 2022 •

edited

Loading