Fail start of non-data node if node has data #37347

henningandersen · 2019-01-11T09:02:16Z

Check that nodes started with node.data=false cannot start if they have
shard data to avoid (old) indexes being resurrected into the cluster in red status.

Issue #27073

Only fixes node.data=false part of issue, not coordinating only.

I were not sure if this should go into docs too?

Check that nodes started with node.data=false cannot start if they have shard data to avoid (old) indexes being resurrected into the cluster in red status. Issue elastic#27073

elasticmachine · 2019-01-11T12:13:10Z

Pinging @elastic/es-distributed

ywelsch

Thanks for picking this up @henningandersen. I've left some comments.

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

server/src/test/java/org/elasticsearch/env/NodeEnvironmentIT.java

ywelsch · 2019-01-11T13:52:46Z

server/src/test/java/org/elasticsearch/env/NodeEnvironmentIT.java

+            });
+            fail("Starting a node with node.data=false that has index/shard data must fail");
+        } catch (IllegalArgumentException e) {
+            // EXPECTED


also assert something on the exception message?

I ended up only asserting that the message contains the index UUID, would be nice to assert the entire path if possible, but I will need help if you think that is worth pursuing?

ywelsch · 2019-01-11T13:58:02Z

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

+            throw new IllegalArgumentException("Node is started with "
+                + Node.NODE_DATA_SETTING.getKey()
+                + "=false, but found shard data: "
+                + badDataPaths);


Is this the absolute path that we're outputting here?

This would be the relative path if path.node is relative. Do you prefer the absolute path?

Changed it to absolute path.

Improved message and exception type plus cleaner tests.

henningandersen · 2019-01-11T17:04:52Z

Thanks for your comments @ywelsch, I have addressed mostt of them in the latest commit and added comments to the rest. Will you please take another look?

henningandersen · 2019-01-12T16:03:41Z

@elasticmachine run gradle build tests 1

ywelsch

I've left a few more nits, but looks good o.w.

The reason I explicitly build settings here is to be able to reuse the path.node setting for the next newNodeEnvironment call. Is there a better way?

I missed that one. Thanks for clarifying. Can you perhaps add a comment where you create the second NodeEnvironment to make this more explicit? I don't see a better way btw.

ywelsch · 2019-01-15T13:01:54Z

server/src/test/java/org/elasticsearch/env/NodeEnvironmentIT.java


-        final String node_1 = internalCluster().startNodes(1).get(0);
+        logger.info("--> starting one node");
+        internalCluster().startNodes(1);


nit: just startNode()

ywelsch · 2019-01-15T13:05:59Z

server/src/test/java/org/elasticsearch/env/NodeEnvironmentIT.java

+        final String indexUUID = resolveIndex(indexName).getUUID();
+
+        logger.info("--> indexing a simple document");
+        client().prepareIndex(indexName, "type1", "1").setSource("field1", "value1").setRefreshPolicy(IMMEDIATE).get();


no need to set .setRefreshPolicy(IMMEDIATE). The indexed changes are guaranteed to be made durable before the index request returns here. Visibility for searches does not matter as we don't search here.

ywelsch · 2019-01-15T13:07:48Z

server/src/test/java/org/elasticsearch/env/NodeEnvironmentIT.java

+                        return Settings.builder().put(Node.NODE_DATA_SETTING.getKey(), false).build();
+                    }
+                }));
+        assertThat(ex.getMessage(), containsString(indexUUID));


assert that message starts with "Node is started with " + Node.NODE_DATA_SETTING.getKey() + "=false, but has shard data"

ywelsch · 2019-01-15T13:18:54Z

test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java

+            nodeAndClient.startNode();
+            success = true;
+        } finally {
+            if (!success)


success == false

ywelsch · 2019-01-15T13:20:07Z

server/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java

+            () -> newNodeEnvironment(noDataSettings).close());
+
+        assertThat(ex.getMessage(),
+            containsString(env.indexPaths(index)[0].resolve(shardDir).toAbsolutePath().toString()));


same as above, assert that message starts with "Node is started with " + Node.NODE_DATA_SETTING.getKey() + "=false, but has shard data"

ywelsch · 2019-01-15T13:22:29Z

server/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java

+            Files.createDirectories(path.resolve(MetaDataStateFormat.STATE_DIR_NAME));
+        }
+
+        env.close();


Note that you can also use try-with-resources here, e.g.:

try (NodeEnvironment env = newNodeEnvironment(settings)) { for (Path path : env.indexPaths(index)) { Files.createDirectories(path.resolve(MetaDataStateFormat.STATE_DIR_NAME)); } }

This helps in cleaning up resources even when tests fail, and avoids having secondary failures that might distract when analyzing a test failure.

Avoid impacting subsequent tests on failures and added assert for more of the message.

henningandersen · 2019-01-18T18:48:20Z

@elasticmachine run gradle build tests 2

henningandersen · 2019-01-19T00:26:19Z

@elasticmachine please run the Gradle build tests 1

henningandersen · 2019-01-21T13:26:33Z

@elasticmachine run elasticsearch-ci-1

henningandersen · 2019-01-21T13:29:21Z

@elasticmachine run gradle build tests 1

henningandersen · 2019-01-21T14:18:17Z

@elasticmachine run gradle build tests 1

henningandersen · 2019-01-21T15:54:05Z

@elasticmachine run gradle build tests 1

henningandersen · 2019-01-21T18:51:32Z

@elasticmachine run gradle build tests 1

henningandersen · 2019-01-21T20:50:30Z

@elasticmachine run gradle build tests 1

henningandersen · 2019-01-22T07:20:44Z

@elasticmachine run gradle build tests 2

…data_when_node_data_false

* elastic/master: (43 commits) Remove remaining occurances of "include_type_name=true" in docs (elastic#37646) SQL: Return Intervals in SQL format for CLI (elastic#37602) Publish to masters first (elastic#37673) Un-assign persistent tasks as nodes exit the cluster (elastic#37656) Fail start of non-data node if node has data (elastic#37347) Use cancel instead of timeout for aborting publications (elastic#37670) Follow stats api should return a 404 when requesting stats for a non existing index (elastic#37220) Remove deprecated FieldNamesFieldMapper.Builder#index (elastic#37305) Document that date math is locale independent Bootstrap a Zen2 cluster once quorum is discovered (elastic#37463) Upgrade to lucene-8.0.0-snapshot-83f9835. (elastic#37668) Mute failing test Fix java time formatters that round up (elastic#37604) Removes awaits fix as the fix is in. (elastic#37676) Mute failing test Ensure that max seq # is equal to the global checkpoint when creating ReadOnlyEngines (elastic#37426) Mute failing discovery disruption tests Add note about how the body is referenced (elastic#33935) Define constants for REST requests endpoints in tests (elastic#37610) Make prepare engine step of recovery source non-blocking (elastic#37573) ...

Now warn about both left-behind data and metadata for non-data or non-data and non-master nodes. Disable dangling indices check completely for coordinating only nodes (non-data and non-master). Issue #27073 6.x backport of #37347 and #37748 (without failing start up).

When a node is repurposed to master/no-data or no-master/no-data, v7.x will not start (see elastic#37748 and elastic#37347). The `elasticsearch repurpose` tool can fix this by cleaning up the problematic data.

When a node is repurposed to master/no-data or no-master/no-data, v7.x will not start (see #37748 and #37347). The `elasticsearch repurpose` tool can fix this by cleaning up the problematic data.

Fail start of non-data node if node has data

115c7f9

Check that nodes started with node.data=false cannot start if they have shard data to avoid (old) indexes being resurrected into the cluster in red status. Issue elastic#27073

henningandersen requested a review from ywelsch January 11, 2019 09:02

henningandersen added >enhancement v7.0.0 :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) labels Jan 11, 2019

henningandersen self-assigned this Jan 11, 2019

ywelsch suggested changes Jan 11, 2019

View reviewed changes

Fail start of non-data node if node has data

7d6d824

Improved message and exception type plus cleaner tests.

ywelsch self-requested a review January 14, 2019 11:38

ywelsch approved these changes Jan 15, 2019

View reviewed changes

Fail start of non-data node if node has data

288860b

Avoid impacting subsequent tests on failures and added assert for more of the message.

Merge branch 'master' into fail_on_data_when_node_data_false

2f54af4

Merge commit 'ca4b5861c8c84a8c1415a3dda89f83720c3b6da8' into fail_on_…

2b8ab2f

…data_when_node_data_false

henningandersen merged commit 2286118 into elastic:master Jan 22, 2019

ywelsch mentioned this pull request Jan 25, 2019

Prevent non-data nodes to start with dangling indices #35447

Closed

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

henningandersen mentioned this pull request Feb 26, 2019

Node repurpose tool #39403

Merged

ywelsch mentioned this pull request Mar 13, 2019

Make node.master a dynamic setting #10793

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fail start of non-data node if node has data #37347

Fail start of non-data node if node has data #37347

henningandersen commented Jan 11, 2019

elasticmachine commented Jan 11, 2019

ywelsch left a comment

ywelsch Jan 11, 2019

henningandersen Jan 11, 2019

ywelsch Jan 11, 2019

henningandersen Jan 11, 2019

henningandersen Jan 11, 2019

henningandersen commented Jan 11, 2019

henningandersen commented Jan 12, 2019

ywelsch left a comment

ywelsch Jan 15, 2019

ywelsch Jan 15, 2019

ywelsch Jan 15, 2019

ywelsch Jan 15, 2019

ywelsch Jan 15, 2019

ywelsch Jan 15, 2019

henningandersen commented Jan 18, 2019

henningandersen commented Jan 19, 2019

henningandersen commented Jan 21, 2019

henningandersen commented Jan 21, 2019

henningandersen commented Jan 21, 2019

henningandersen commented Jan 21, 2019

henningandersen commented Jan 21, 2019

henningandersen commented Jan 21, 2019

henningandersen commented Jan 22, 2019

Fail start of non-data node if node has data #37347

Fail start of non-data node if node has data #37347

Conversation

henningandersen commented Jan 11, 2019

elasticmachine commented Jan 11, 2019

ywelsch 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

henningandersen commented Jan 11, 2019

henningandersen commented Jan 12, 2019

ywelsch 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

henningandersen commented Jan 18, 2019

henningandersen commented Jan 19, 2019

henningandersen commented Jan 21, 2019

henningandersen commented Jan 21, 2019

henningandersen commented Jan 21, 2019

henningandersen commented Jan 21, 2019

henningandersen commented Jan 21, 2019

henningandersen commented Jan 21, 2019

henningandersen commented Jan 22, 2019