Add validation for supported index version on node join, restore, upgrade & open index #21830

s1monw · 2016-11-28T16:25:39Z

Today we can easily join a cluster that holds an index we don't support since
we currently allow rolling upgrades from 5.x to 6.x. Along the same lines we don't check if we can support an index based on the nodes in the cluster when we open, restore or metadata-upgrade and index. This commit adds
additional safety that fails cluster state validation, open, restore and /or upgrade if there is an open index with an incompatible index version created in the cluster.

Realtes to #21670

Today we can easily join a cluster that holds an index we don't support since we currently allow rolling upgrades from 5.x to 6.x. This commit adds additional safety that fails cluster state validation if there is an open index with an incompatible index version created in the cluster. Realtes to elastic#21670

s1monw · 2016-11-28T16:25:47Z

@bleskes can you look

bleskes

The code LGTM and does what was, I think, intended.

However I have some concerns about the approach:

Why allow closed indices? Someone can just open them and then we're in trouble no? I think it's better to prevent joining and give people the option to open the indices and snapshot them/reindex them/delete them.
I think we also need to deal with dangling indices
Because of dangling indices, we need to do this validation on the cluster state update thread. we can maybe "preflight" it before the node join but we can still end up in trouble (and the preflight can happen on the master, there is no need to go back the the node for this).

An alternative to all of this (but more complex) is to have allocation deciders prevent these indices from being allocated to 6.x nodes and auto close them as soon as a 6.x master is elected. Although the question than becomes - what can people do once they are closed? we don't allow downgrading a data folder anyway (as we may have re-written the state).

s1monw · 2016-11-29T12:55:14Z

Why allow closed indices? Someone can just open them and then we're in trouble no? I think it's better to prevent joining and give people the option to open the indices and snapshot them/reindex them/delete them.

I completely agree, I will remove that.

I think this is not a complete solution but rather a building block towards a more complete solution. The dangling indices case should be done as well and I think we should just not import dangling indices if we have a node in the cluster that doesn't support it but that is a different change / PR - ok with that?

An alternative to all of this (but more complex) is to have allocation deciders prevent these indices from being allocated to 6.x nodes and auto close them as soon as a 6.x master is elected. Although the question than becomes - what can people do once they are closed? we don't allow downgrading a data folder anyway (as we may have re-written the state).

I think we can't do that since we then need to be able to understand mappings etc for these indices as well even though we don't support the index.

bleskes · 2016-11-29T12:57:52Z

I think we should just not import dangling indices if we have a node in the cluster that doesn't support it but that is a different change / PR - ok with that?

Sure thing. Do note that we have to do this join validation on the cluster state thread for it to be effective.

I think we can't do that since we then need to be able to understand mappings etc for these indices as well even though we don't support the index.

I agree it's a rabbit hole. I tend to say "no mixed version clusters unless all metadata/data is supported by both versions".

s1monw · 2016-11-29T13:02:11Z

@bleskes pushed a new commit

s1monw · 2016-11-29T15:27:18Z

@bleskes are you ok with pushing this?

bleskes · 2016-11-29T15:32:01Z

@s1monw sorry - did some other stuff. Did you see my comment about doing this on the cluster state thread? To be clear - on the master

…sage

s1monw · 2016-11-29T16:01:55Z

@s1monw sorry - did some other stuff. Did you see my comment about doing this on the cluster state thread? To be clear - on the master

yeah but I thought we do this in another PR?

…tate thread to ensure we don't have a race

s1monw · 2016-11-29T16:28:44Z

@bleskes I pushed new commits

bleskes

Thx @s1monw . LGTM

s1monw · 2016-11-30T12:29:14Z

@elasticmachine test this please

s1monw · 2016-11-30T12:29:45Z

@bleskes I had to add some more stuff to ensure we are sane in a bunch of other places can you take another look

s1monw · 2016-11-30T12:31:06Z

@jpountz I think this is relevant for #21670 - once this is in we can merge #21670 as well

s1monw · 2016-11-30T16:02:56Z

@elasticmachine test this please

bleskes

Left some minor suggestion. The one main one is how we treat Coordinating Nodes. Last, I think the PR title is out date now? it's more than just join?

bleskes · 2016-11-30T16:15:21Z

core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexStateService.java

@@ -160,12 +161,14 @@ public ClusterState execute(ClusterState currentState) {
                MetaData.Builder mdBuilder = MetaData.builder(currentState.metaData());
                ClusterBlocks.Builder blocksBuilder = ClusterBlocks.builder()
                        .blocks(currentState.blocks());
+                Version minIndexCompatibilityVersion = currentState.getNodes().getSmallestNonClientNodeVersion()


hmm... I'm wondering if we should exclude the non-data non-master nodes here. They do all the coordination and might rely on some metadata they can't digest? We're still have to keep the transport client around, but I don't think we should exclude ClientNode (now called coordinating nodes) ?

This holds for other places in the code as well...

I don't think coordinating nodes do rely on any metadata they can't digest. They don't parse mappings afaik (I am convinced they should not) which is the main thing here so I thin we are good

the actual question is why do we use the smallest, we have to use the largest node in the cluster since that is the one relevant for the index version to be opened.

yikes. Of course.

bleskes · 2016-11-30T16:21:03Z

core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java

@@ -92,21 +92,22 @@ boolean isUpgraded(IndexMetaData indexMetaData) {
    }

    /**
-     * Elasticsearch 5.0 no longer supports indices with pre Lucene v5.0 (Elasticsearch v2.0.0.beta1) segments. All indices
-     * that were created before Elasticsearch v2.0.0.beta1 should be reindexed in Elasticsearch 2.x
+     * Elasticsearch 6.0 no longer supports indices with pre Lucene v5.0 (Elasticsearch v5.0.0.beta1) segments. All indices


nit: we lead with ES and make lucene second? it's a bit weird now as ES v5 uses lucene 6.0. How about "ES 6.0 no longer supports indices created by an ES version earlier than 5.0" and leave it at that?

bleskes · 2016-11-30T16:27:18Z

core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeService.java

+    private void checkSupportedVersion(IndexMetaData indexMetaData, Version minimumIndexCompatibilityVersion) {
+        if (indexMetaData.getState() == IndexMetaData.State.OPEN && isSupportedVersion(indexMetaData,
+            minimumIndexCompatibilityVersion) == false) {
+            throw new IllegalStateException("The index [" + indexMetaData.getIndex() + "] was created before v5.0.0.beta1."


Any way we could bake the minimumIndexCompatibilityVersion into the message ? I'm afraid we'll forget in the future and the error message will be wrong.

bleskes · 2016-11-30T16:40:03Z

core/src/test/java/org/elasticsearch/bwcompat/RestoreBackwardsCompatIT.java

@@ -98,7 +98,7 @@ public void testRestoreOldSnapshots() throws Exception {
        for (Version v : VersionUtils.allReleasedVersions()) {
            if (VersionUtils.isSnapshot(v)) continue;  // snapshots are unreleased, so there is no backcompat yet
            if (v.isRelease() == false) continue; // no guarantees for prereleases
-            if (v.onOrBefore(Version.V_2_0_0_beta1)) continue; // we can only test back one major lucene version
+            if (v.onOrBefore(Version.V_5_0_0_beta1)) continue; // we can only test back one major lucene version


Wondering - we didn't you go with Version.CURRENT.minimumIndexCompatibilityVersion() here?

bleskes · 2016-11-30T16:43:41Z

core/src/test/java/org/elasticsearch/cluster/metadata/MetaDataIndexUpgradeServiceTests.java

@@ -96,7 +97,7 @@ public void testFailUpgrade() {
            .put(IndexMetaData.SETTING_VERSION_CREATED, Version.fromString("2.1.0"))


nit: Should we bump these to realistic current versions? i.e., 5.0.0 for index created and Version.V_6_0_0. minimumIndexCompatibilityVersion

s1monw · 2016-12-01T07:08:30Z

@bleskes I pushed new commits

bleskes

Looks great to me. Thanks @s1monw

…rade & open index (elastic#21830) Today we can easily join a cluster that holds an index we don't support since we currently allow rolling upgrades from 5.x to 6.x. Along the same lines we don't check if we can support an index based on the nodes in the cluster when we open, restore or metadata-upgrade and index. This commit adds additional safety that fails cluster state validation, open, restore and /or upgrade if there is an open index with an incompatible index version created in the cluster. Realtes to elastic#21670

…rade & open index (#21830) (#21914) Today we can easily join a cluster that holds an index we don't support since we currently allow rolling upgrades from 5.x to 6.x. Along the same lines we don't check if we can support an index based on the nodes in the cluster when we open, restore or metadata-upgrade and index. This commit adds additional safety that fails cluster state validation, open, restore and /or upgrade if there is an open index with an incompatible index version created in the cluster. Realtes to #21670

…23311) When a node wants to join a cluster, it sends a join request to the master. The master then sends a join validation request to the node. This checks that the node can deserialize the current cluster state that exists on the master and that it can thus handle all the indices that are currently in the cluster (see #21830). The current code can trip an assertion as it does not take the cluster state as is but sets itself as the local node on the cluster state. This can result in an inconsistent DiscoveryNodes object as the local node is not yet part of the cluster state and a node with same id but different address can still exist in the cluster state. Also another node with the same address but different id can exist in the cluster state if multiple nodes are run on the same machine and ports have been swapped after node crashes/restarts.

Today we fail the node at startup if it contains an index that is too old to be compatible with the current version, unless that index is closed. If the index is closed then the node will start up and this puts us into a bad state: the index cannot be opened and must be reindexed using an earlier version, but we offer no way to get that index into a node running an earlier version so that it can be reindexed. Downgrading the node in-place is decidedly unsupported and cannot be expected to work since the node already started up and upgraded the rest of its metadata. Since elastic#41731 we actively reject downgrades to versions ≥ v7.2.0 too. This commit prevents the node from starting in the presence of any too-old indices (closed or not). In particular, it does not write any upgraded metadata in this situation, increasing the chances an in-place downgrade might be successful. We still actively reject the downgrade using elastic#41731, because we wrote the node metadata file before checking the index metadata, but at least there is a way to override this check. Relates elastic#21830, elastic#44230

Today we fail the node at startup if it contains an index that is too old to be compatible with the current version, unless that index is closed. If the index is closed then the node will start up and this puts us into a bad state: the index cannot be opened and must be reindexed using an earlier version, but we offer no way to get that index into a node running an earlier version so that it can be reindexed. Downgrading the node in-place is decidedly unsupported and cannot be expected to work since the node already started up and upgraded the rest of its metadata. Since #41731 we actively reject downgrades to versions ≥ v7.2.0 too. This commit prevents the node from starting in the presence of any too-old indices (closed or not). In particular, it does not write any upgraded metadata in this situation, increasing the chances an in-place downgrade might be successful. We still actively reject the downgrade using #41731, because we wrote the node metadata file before checking the index metadata, but at least there is a way to override this check. Relates #21830, #44230

s1monw added :Cluster resiliency >upgrade v6.0.0-alpha1 labels Nov 28, 2016

clintongormley added >enhancement and removed >upgrade labels Nov 28, 2016

bleskes reviewed Nov 29, 2016

View reviewed changes

don't allow closed indices either

d9aed75

add more comments, inline method and add a more informative error mes…

5c9a61f

…sage

Add more validation into several places that execute on the cluster s…

f63bb98

…tate thread to ensure we don't have a race

s1monw added the v5.2.0 label Nov 29, 2016

bleskes approved these changes Nov 29, 2016

View reviewed changes

s1monw added 2 commits November 30, 2016 09:45

move old 2.x version to unsupported

7231d45

Ensure we can't restore or open any indices that are not supported

c709e34

s1monw added 2 commits November 30, 2016 14:56

Merge branch 'master' into validate_indices_versions_on_join

544d70e

Fix test

df43c5e

bleskes suggested changes Nov 30, 2016

View reviewed changes

more feedback

0d1be73

s1monw changed the title ~~Add validation for supported index version on node join~~ Add validation for supported index version on node join, restore, upgrade & open index Nov 30, 2016

s1monw added 3 commits November 30, 2016 21:09

Use the max node version to determin the minIndexCompatibileVersion

fbf4dbc

Merge branch 'master' into validate_indices_versions_on_join

d835ac4

fix version constant

31aee20

bleskes approved these changes Dec 1, 2016

View reviewed changes

s1monw merged commit 6522538 into elastic:master Dec 1, 2016

s1monw deleted the validate_indices_versions_on_join branch December 1, 2016 14:40

s1monw mentioned this pull request Dec 1, 2016

Backport #21830 to 5.x #21914

Merged

ywelsch mentioned this pull request Feb 22, 2017

Don't set local node on cluster state used for node join validation #23311

Merged

imotov mentioned this pull request Jun 5, 2017

Add validation for supported version of cluster state metadata on node join #25061

Closed

clintongormley added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. and removed :Cluster labels Feb 13, 2018

This was referenced Jul 12, 2019

Fail node containing ancient closed index #44264

Merged

Better handling of ancient indices #44230

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation for supported index version on node join, restore, upgrade & open index #21830

Add validation for supported index version on node join, restore, upgrade & open index #21830

s1monw commented Nov 28, 2016 •

edited

Loading

s1monw commented Nov 28, 2016

bleskes left a comment

s1monw commented Nov 29, 2016

bleskes commented Nov 29, 2016

s1monw commented Nov 29, 2016

s1monw commented Nov 29, 2016

bleskes commented Nov 29, 2016 •

edited

Loading

s1monw commented Nov 29, 2016

s1monw commented Nov 29, 2016

bleskes left a comment

s1monw commented Nov 30, 2016

s1monw commented Nov 30, 2016

s1monw commented Nov 30, 2016

s1monw commented Nov 30, 2016

bleskes left a comment

bleskes Nov 30, 2016

bleskes Nov 30, 2016

s1monw Nov 30, 2016

s1monw Nov 30, 2016

bleskes Dec 1, 2016

bleskes Nov 30, 2016

s1monw Nov 30, 2016

bleskes Nov 30, 2016

bleskes Nov 30, 2016

bleskes Nov 30, 2016

s1monw commented Dec 1, 2016

bleskes left a comment

		@@ -96,7 +97,7 @@ public void testFailUpgrade() {
		.put(IndexMetaData.SETTING_VERSION_CREATED, Version.fromString("2.1.0"))

Add validation for supported index version on node join, restore, upgrade & open index #21830

Add validation for supported index version on node join, restore, upgrade & open index #21830

Conversation

s1monw commented Nov 28, 2016 • edited Loading

s1monw commented Nov 28, 2016

bleskes left a comment

Choose a reason for hiding this comment

s1monw commented Nov 29, 2016

bleskes commented Nov 29, 2016

s1monw commented Nov 29, 2016

s1monw commented Nov 29, 2016

bleskes commented Nov 29, 2016 • edited Loading

s1monw commented Nov 29, 2016

s1monw commented Nov 29, 2016

bleskes left a comment

Choose a reason for hiding this comment

s1monw commented Nov 30, 2016

s1monw commented Nov 30, 2016

s1monw commented Nov 30, 2016

s1monw commented Nov 30, 2016

bleskes 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

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

s1monw commented Dec 1, 2016

bleskes left a comment

Choose a reason for hiding this comment

s1monw commented Nov 28, 2016 •

edited

Loading

bleskes commented Nov 29, 2016 •

edited

Loading