New setting to prevent automatically importing dangling indices #49174

pugnascotia · 2019-11-15T15:21:52Z

Part of #48366.

Introduce a new static setting, gateway.auto_import_dangling_indices, which prevents dangling indices from being automatically imported.

I've also updated the dangling index docs a little to cover the new setting, and add some scary caveats.

…-indices-flag

The new settings didn't work, but now it does, and I've written a couple of ITs that create dangling indices and check what affect the setting has. I'm still fixing the unit tests though.

…-indices-flag

New setting `gateway.auto_import_dangling_indices` doesn't need to be dynamic. Unfortunatlely, this has broken one of the ITs.

elasticmachine · 2019-11-15T15:44:10Z

Pinging @elastic/es-docs (:Docs)

elasticmachine · 2019-11-15T15:44:11Z

Pinging @elastic/es-distributed (:Distributed/Recovery)

pugnascotia · 2019-11-15T15:45:13Z

Note: there's more to do to satisfy the issue, e.g. an API to list the danglies, but I plan on doing that separately.

…-indices-flag

pugnascotia · 2019-11-19T13:20:45Z

@DaveCTurner @original-brownbear ready for a review, if you have time.

ywelsch

Talked to @DaveCTurner who will add some comments as well. No need to revise until then

ywelsch · 2019-11-18T11:49:10Z

docs/reference/modules/gateway.asciidoc

-indices to be imported, instead of being deleted.
+When a node joins the cluster, it will search for any shards that are
+stored in its local data directory and do not already exist in the
+cluster. If the static setting `gateway.auto_import_dangling_indices` is


perhaps call this node setting

ywelsch · 2019-11-18T12:00:33Z

docs/reference/modules/gateway.asciidoc

+cluster. If the static setting `gateway.auto_import_dangling_indices` is
+`true` (the default is `false`), then those shards will be imported into
+the cluster. This functionality is intended as a best effort to help users
+who lose all master nodes. If a new master node is started which is unaware


I wonder if we should explain this in terms of unsafe cluster bootstrapping. As far as I'm aware the only remaining use of dangling indices in 7.x is for the situation where a cluster has lost all master-eligible nodes and where the unsafe-bootstrap and/or detach-cluster commands have been used.

ywelsch · 2019-11-18T12:05:41Z

docs/reference/modules/gateway.asciidoc

+old indices to be imported, instead of being deleted.
+
+Enabling `gateway.auto_import_dangling_indices` should only be done if
+absolutely necessary, after understanding the possible consequences (this is not an exhaustive list):


I think we should strongly recommend against using auto import of dangling indices as it can cause inconsistencies in a cluster (e.g. automatically necroing deleted indices) and instead refer to our tooling (to be created in follow-ups).

Given that we don't provide the tooling at this time, I wonder if we should leave this setting undocumented for the time being and only write the docs once we can present the full story on this.

ywelsch · 2019-11-18T12:07:48Z

server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java

    private final NodeEnvironment nodeEnv;
    private final MetaStateService metaStateService;
    private final LocalAllocateDangledIndices allocateDangledIndices;

    private final Map<Index, IndexMetaData> danglingIndices = ConcurrentCollections.newConcurrentMap();

+    private boolean allocateDanglingIndices;
+
    @Inject
    public DanglingIndicesState(NodeEnvironment nodeEnv, MetaStateService metaStateService,
                                LocalAllocateDangledIndices allocateDangledIndices, ClusterService clusterService) {
        this.nodeEnv = nodeEnv;
        this.metaStateService = metaStateService;
        this.allocateDangledIndices = allocateDangledIndices;
        clusterService.addListener(this);


simpler might be to only conditionally add the listener based on AUTO_IMPORT_DANGLING_INDICES_SETTING

DaveCTurner

Thanks @pugnascotia, I left a few suggestions.

DaveCTurner · 2019-11-21T10:11:38Z

server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java

@@ -55,19 +56,33 @@

    private static final Logger logger = LogManager.getLogger(DanglingIndicesState.class);

+    public static final Setting<Boolean> AUTO_IMPORT_DANGLING_INDICES_SETTING = Setting.boolSetting(
+        "gateway.auto_import_dangling_indices",
+        false,


I think in this PR we should preserve the existing behaviour and default this to true, since we do not have any other alternatives for finding or cleaning up dangling indices. This means we can use this setting as a feature flag for testing the new behaviour while it's still in flight. Once we've added the APIs we have planned we can adjust the default to false.

DaveCTurner · 2019-11-21T10:16:33Z

docs/reference/modules/gateway.asciidoc

-lose all master nodes. If a new master node is started which is unaware of 
-the other indices in the cluster, adding the old nodes will cause the old 
-indices to be imported, instead of being deleted.
+When a node joins the cluster, it will search for any shards that are


Given my previous comment about preserving existing behaviour, I think we should leave the docs alone for now. I don't think we should put much effort into improving the docs for today's behaviour, and we'll need to make more changes here when we add these APIs.

DaveCTurner · 2019-11-21T10:27:03Z

server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java


    public void testCleanupWhenEmpty() throws Exception {
        try (NodeEnvironment env = newNodeEnvironment()) {
            MetaStateService metaStateService = new MetaStateService(env, xContentRegistry());
            DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService);

+            // Given an empty state


I am, personally, not a big fan of having these comments everywhere. The given/when/then phases of these tests seem pretty clear to me as written, and far too often do comments like these fall out of sync with the code and end up being misleading to readers.

There's a certain amount of judgement needed here to identify cases where there's something surprising that needs the reader's attention, because all readers are different. But in this specific test case for me they add, rather than remove, confusion.

DaveCTurner · 2019-11-21T10:50:48Z

server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java

            assertTrue(danglingState.getDanglingIndices().isEmpty());
+
+            // When passed a metdata with an unknown index


The metadata we pass in is empty, but we've created a dangling index on disk.

DaveCTurner · 2019-11-21T10:53:52Z

server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java

+
+            assertTrue(danglingState.getDanglingIndices().isEmpty());
+
+            // Given a metadata that does not enable allocation of dangling indices


Whether dangling indices importing is enabled is not a property of the metadata as suggested by this comment. Could we make the enabled/disabled property an explicit argument to createDanglingIndicesState() instead?

DaveCTurner · 2019-11-21T10:54:12Z

server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java

+            assertTrue(danglingState.getDanglingIndices().isEmpty());
+
+            // Given a state where automatic allocation is enabled
+            danglingState.setAllocateDanglingIndicesSetting(true);


Could we make the enabled/disabled property an explicit argument to createDanglingIndicesState() instead?

…-indices-flag

pugnascotia · 2019-11-21T14:58:20Z

Thanks for the reviews, I've pushed 71bcbde to address them, specifically:

Reverted the docs changes
Default the new setting to false
Only subscribe DanglingIndicesState to cluster state updates if the new setting is true
Revert changes to UnsafeBootstrapAndDetachCommandIT as they're no longer needed
Strip confusing comments from DanglingIndicesStateTests and be specify the new setting's value when calling createDanglingIndicesState().

pugnascotia · 2019-11-26T13:56:41Z

@ywelsch @DaveCTurner any chance of a re-review?

ywelsch

Thanks for the update. I've left some comments.

ywelsch · 2019-11-27T09:30:09Z

server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java

+        }
+    }
+
+    public void setAllocateDanglingIndicesSetting(boolean allocateDanglingIndices) {


this method is superfluous now?

Oops, yes, good catch.

ywelsch · 2019-11-27T09:30:25Z

server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java

@@ -171,10 +188,15 @@ private IndexMetaData stripAliases(IndexMetaData indexMetaData) {
     * Allocates the provided list of the dangled indices by sending them to the master node
     * for allocation.
     */
-    private void allocateDanglingIndices() {
+    void allocateDanglingIndices() {
+        if (this.allocateDanglingIndices == false) {


this condition is superfluous now?

ywelsch · 2019-11-27T09:30:40Z

server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java

    private final NodeEnvironment nodeEnv;
    private final MetaStateService metaStateService;
    private final LocalAllocateDangledIndices allocateDangledIndices;

    private final Map<Index, IndexMetaData> danglingIndices = ConcurrentCollections.newConcurrentMap();

+    private boolean allocateDanglingIndices;


this field is superfluous now?

ywelsch · 2019-11-27T09:31:14Z

server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java

-            .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0)
-            .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT)
-            .build();
+        .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1)


Let's avoid reformatting code unrelated to the PR.

ywelsch · 2019-11-27T09:40:13Z

server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java

+                Settings.builder()
+                    .put("number_of_shards", shardCount)
+                    .put("number_of_replicas", replicaCount)
+                    .put(Store.INDEX_STORE_STATS_REFRESH_INTERVAL_SETTING.getKey(), 0)


why is this necessary?

Ahem...that's copy-and-paste. Can you tell it's the first time I've written an ES IT? 😅

ywelsch · 2019-11-27T09:44:54Z

server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java

+        client().admin().indices().prepareStats(name).execute().actionGet();
+    }
+
+    private void deleteIndex(String indexName) {


why define an extra method here? just so that one logging line can be reused? Feels a bit over the top.

ywelsch · 2019-11-27T09:45:44Z

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

     */
-    public synchronized boolean stopRandomDataNode() throws IOException {
+    public synchronized Optional<String> stopRandomDataNode() throws IOException {


where is this change used?

ywelsch · 2019-11-27T09:46:08Z

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

@@ -1902,6 +1905,12 @@ public String startNode(Settings settings) {
        return startNodes(settings).get(0);
    }

+    public void startNode(String nodeName) {


where is this used? Revert

ywelsch · 2019-11-27T09:50:34Z

server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java

@@ -55,19 +56,35 @@

    private static final Logger logger = LogManager.getLogger(DanglingIndicesState.class);

+    public static final Setting<Boolean> AUTO_IMPORT_DANGLING_INDICES_SETTING = Setting.boolSetting(


Let's deprecate this setting right away, so that we can remove it in 8.0 if we want

ywelsch · 2019-11-27T09:53:52Z

server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java

+        this.allocateDanglingIndices = AUTO_IMPORT_DANGLING_INDICES_SETTING.get(clusterService.getSettings());
+
+        if (this.allocateDanglingIndices) {
+            clusterService.addListener(this);


with this change, we are now not scanning for new dangling indices anymore and therefore also not warning about the existence of dangling indices on disk. We should keep this in mind when working out a new story for managing dangling indices via an API. Perhaps we should think about still emitting a warning in the logs. Let's add a TODO for this on the meta issue.

…-indices-flag

pugnascotia · 2019-11-27T14:44:29Z

@ywelsch thank you for the review - seems there was quite a bit of cruft to clear up, sorry about that. Notes on the latest changes:

The new setting is now deprecated
The state class logs a warning if the setting is false, since dangling indices won't be detected or imported
Change the disabled case unit test to verify that no cluster state listener is registered, since the danglingState.allocateDanglingIndices() method no longer checks the setting value
Tidied up the IT

ywelsch

Thanks for the update @pugnascotia. I've left more comments.

ywelsch · 2019-11-28T10:01:46Z

server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java

@@ -46,6 +51,12 @@
            .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT)
            .build();

+    // The setting AUTO_IMPORT_DANGLING_INDICES_SETTING is deprecated, so we must disable
+    // warning checks or all the tests will fail.
+    protected boolean enableWarningsCheck() {


ywelsch · 2019-11-28T10:05:32Z

server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java

+            createDanglingIndicesState(env, metaStateService, localAllocateDangledIndices, mockClusterService);
+
+            // Check that no listener was registered, because auto-import is disabled
+            verify(mockClusterService, never()).addListener(any());


I'm not a big fan of these whitebox tests, given how fragile they are. There is also no test here that checks that addListener is ever called when auto-import is enabled? This means that if the dangling indices implementation changes to use another method on clusterservice to register a callback, this test will be pointless.

ywelsch · 2019-11-28T10:09:46Z

server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java

+        LocalAllocateDangledIndices indexAllocator,
+        ClusterService clusterService
+    ) {
+        return new DanglingIndicesState(env, metaStateService, indexAllocator, clusterService);


what's the point of this method? Let's avoid these unnecessary abstractions.

ywelsch · 2019-11-28T10:13:42Z

server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java

+            MetaStateService metaStateService = new MetaStateService(env, xContentRegistry());
+            LocalAllocateDangledIndices localAllocateDangledIndices = mock(LocalAllocateDangledIndices.class);
+
+            final ClusterService mockClusterService = buildMockClusterService(false);


I would find the test easier to understand if we were to explicitly pass the setting Settings.builder().put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), false).build() here instead of the magic boolean.

ywelsch · 2019-11-28T10:17:20Z

server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java

+
+        // Create an index and distribute it across the 3 nodes
+        createAndPopulateIndex(INDEX_NAME, 1, 2);
+        ensureGreen();


createAndPopulateIndex already has a call to ensureGreen.

ywelsch · 2019-11-28T10:21:35Z

server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java

+
+            @Override
+            public Settings onNodeStopped(String nodeName) throws Exception {
+                logger.info("--> deleting test index: {}", INDEX_NAME);


deletions are already logged by ES at info level, no need to log here.

ywelsch · 2019-11-28T10:23:01Z

server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java

+
+        ensureGreen();
+
+        assertTrue("Expected dangling index " + INDEX_NAME + " to be recovered", indexExists(INDEX_NAME));


There is no guarantee that the dangling index has been recovered at this point, as that has an asynchronous notification / import process. This line needs to be put into an assertBusy

ywelsch · 2019-11-28T10:24:19Z

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

@@ -1473,7 +1473,8 @@ public int size() {
    }

    /**
-     * Stops a random data node in the cluster. Returns true if a node was found to stop, false otherwise.
+     * Stops a random data node in the cluster and removes it.
+     * @return the name of the stopped node, if a node was found to stop.


revert this.

ywelsch · 2019-11-28T10:25:11Z

server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java

+     * the cluster when the recovery setting is disabled.
+     */
+    public void testDanglingIndicesAreNotRecoveredWhenSettingIsDisabled() throws Exception {
+        logger.info("--> starting cluster");


what does this logging message add? It's the first line in the test, which is already logged.

ywelsch · 2019-11-28T10:27:17Z

server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java

+
+        // This is so that when then node comes back up, we have a dangling index that could
+        // be recovered, but shouldn't be.
+        logger.info("--> restarted a random node and deleting the index while it's down");


The restart is already logged at info level. Maybe just change the comment to say // Restart node, deleting the index in its absence, so that there is a dangling index to recover

pugnascotia · 2019-11-28T12:09:11Z

Thanks again @ywelsch for bearing with me.

I've replaced the white box tests with a getter for the new setting. I had avoided that before because the only code calling the getter would be test code, but it'll be less fragile.
I've simplified the unit tests and ITs.

ywelsch

LGTM

ywelsch · 2019-11-28T16:04:48Z

server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java

+        final Settings settings = buildSettings(true);
+        internalCluster().startNodes(3, settings);
+
+        createIndex(INDEX_NAME, Settings.builder().put("number_of_shards", 1).put("number_of_replicas", 2).build());


does the number of (primary) shards matter? I understand that you want 2 replicas so that every node has a shard copy.

ywelsch · 2019-11-28T16:06:44Z

server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java

+        // amount of time
+        assertFalse(
+            "Did not expect dangling index " + INDEX_NAME + " to be recovered",
+            waitUntil(() -> indexExists(INDEX_NAME), 5, TimeUnit.SECONDS)


Let's reduce this to 1 second

ywelsch · 2019-11-28T16:08:10Z

server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java

+        }
+    }
+
+    public boolean isAutoImportDanglingIndicesEnabled() {


does not need to be public, only accessed by tests

…-indices-flag

Introduce a new static setting, `gateway.auto_import_dangling_indices`, which prevents dangling indices from being automatically imported. Part of #48366.

…tic#49174) Introduce a new static setting, `gateway.auto_import_dangling_indices`, which prevents dangling indices from being automatically imported. Part of elastic#48366.

pugnascotia added 10 commits October 28, 2019 14:50

Add a setting to control dangling index allocation

91819a0

Merge remote-tracking branch 'upstream/master' into allocate-dangling…

fedf771

…-indices-flag

Merge remote-tracking branch 'upstream/master' into allocate-dangling…

982be6f

…-indices-flag

Fix allocate setting and implement ITs

865831e

The new settings didn't work, but now it does, and I've written a couple of ITs that create dangling indices and check what affect the setting has. I'm still fixing the unit tests though.

Merge remote-tracking branch 'upstream/master' into allocate-dangling…

32d3646

…-indices-flag

Finish fixing unit tests

e377772

Rename the new setting

6393357

WIP - trying to make new setting static

7950e86

New setting `gateway.auto_import_dangling_indices` doesn't need to be dynamic. Unfortunatlely, this has broken one of the ITs.

Add docs for gateway.auto_import_dangling_indices

ce3b5f4

Fix ITs

77b4f09

pugnascotia requested review from DaveCTurner and ywelsch November 15, 2019 15:43

pugnascotia added :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. :Docs v8.0.0 labels Nov 15, 2019

pugnascotia marked this pull request as ready for review November 15, 2019 15:44

pugnascotia added 3 commits November 15, 2019 16:03

Add missing license

3a453e9

Fix test

54db504

Merge remote-tracking branch 'upstream/master' into allocate-dangling…

44834b8

…-indices-flag

ywelsch reviewed Nov 21, 2019

View reviewed changes

DaveCTurner requested changes Nov 21, 2019

View reviewed changes

pugnascotia added 2 commits November 21, 2019 12:55

Merge remote-tracking branch 'upstream/master' into allocate-dangling…

6bf3e9b

…-indices-flag

Address review feedback

71bcbde

pugnascotia force-pushed the allocate-dangling-indices-flag branch from 9b8c880 to 71bcbde Compare November 21, 2019 14:56

ywelsch suggested changes Nov 27, 2019

View reviewed changes

pugnascotia added 3 commits November 27, 2019 13:03

Address review feedback

d156385

Address review comments

f4f3340

Merge remote-tracking branch 'upstream/master' into allocate-dangling…

7cd372a

…-indices-flag

Checkstyle

a9dfc48

ywelsch suggested changes Nov 28, 2019

View reviewed changes

Address review comments

85f7f57

pugnascotia requested a review from ywelsch November 28, 2019 12:09

ywelsch approved these changes Nov 28, 2019

View reviewed changes

pugnascotia added 2 commits November 29, 2019 09:39

Address review feedback

2a40971

Merge remote-tracking branch 'upstream/master' into allocate-dangling…

77a7649

…-indices-flag

pugnascotia merged commit f8e39d2 into elastic:master Nov 29, 2019

pugnascotia deleted the allocate-dangling-indices-flag branch November 29, 2019 11:39

pugnascotia mentioned this pull request Dec 2, 2019

Handle dangling indices more safely #48366

Closed

7 tasks

ywelsch added >enhancement v7.6.0 >non-issue and removed :Docs >enhancement labels Jan 8, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

jakelandis mentioned this pull request Oct 28, 2021

Update Deprecation Info API checks for 8.0 #42404

Closed

80 tasks

		assertTrue(danglingState.getDanglingIndices().isEmpty());

		// When passed a metdata with an unknown index


		assertTrue(danglingState.getDanglingIndices().isEmpty());

		// Given a metadata that does not enable allocation of dangling indices

		@@ -55,19 +56,35 @@

		private static final Logger logger = LogManager.getLogger(DanglingIndicesState.class);

		public static final Setting<Boolean> AUTO_IMPORT_DANGLING_INDICES_SETTING = Setting.boolSetting(


		ensureGreen();

		assertTrue("Expected dangling index " + INDEX_NAME + " to be recovered", indexExists(INDEX_NAME));

New setting to prevent automatically importing dangling indices #49174

New setting to prevent automatically importing dangling indices #49174

Conversation

pugnascotia commented Nov 15, 2019 • edited Loading

elasticmachine commented Nov 15, 2019

elasticmachine commented Nov 15, 2019

pugnascotia commented Nov 15, 2019

pugnascotia commented Nov 19, 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

DaveCTurner 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

pugnascotia commented Nov 21, 2019

pugnascotia commented Nov 26, 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

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

pugnascotia commented Nov 27, 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

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

pugnascotia commented Nov 28, 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

pugnascotia commented Nov 15, 2019 •

edited

Loading