From 91819a0552256c45bbf2f18079d22e8793e31b5d Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Mon, 28 Oct 2019 14:50:37 +0000 Subject: [PATCH 01/15] Add a setting to control dangling index allocation --- .../gateway/DanglingIndicesState.java | 16 ++++- .../gateway/DanglingIndicesStateTests.java | 70 +++++++++++++++++-- 2 files changed, 79 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java index d649c02af4e77..e028b82014dac 100644 --- a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java +++ b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java @@ -30,6 +30,7 @@ import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; +import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.util.concurrent.ConcurrentCollections; import org.elasticsearch.env.NodeEnvironment; import org.elasticsearch.index.Index; @@ -55,6 +56,12 @@ public class DanglingIndicesState implements ClusterStateListener { private static final Logger logger = LogManager.getLogger(DanglingIndicesState.class); + public static final Setting ALLOCATE_DANGLING_INDICES_SETTING = Setting.boolSetting( + "index.allocate_dangling", + false, + Setting.Property.Dynamic + ); + private final NodeEnvironment nodeEnv; private final MetaStateService metaStateService; private final LocalAllocateDangledIndices allocateDangledIndices; @@ -80,7 +87,7 @@ public void processDanglingIndices(final MetaData metaData) { } cleanupAllocatedDangledIndices(metaData); findNewAndAddDanglingIndices(metaData); - allocateDanglingIndices(); + allocateDanglingIndices(metaData); } /** @@ -171,10 +178,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(MetaData metaData) { + if (ALLOCATE_DANGLING_INDICES_SETTING.get(metaData.settings()) == false) { + return; + } + if (danglingIndices.isEmpty()) { return; } + try { allocateDangledIndices.allocateDangled(Collections.unmodifiableCollection(new ArrayList<>(danglingIndices.values())), new ActionListener<>() { diff --git a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java index e7dfbadeeda78..8ca06b00c9130 100644 --- a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java @@ -36,15 +36,20 @@ import java.util.Map; import static org.hamcrest.Matchers.equalTo; +import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; public class DanglingIndicesStateTests extends ESTestCase { private static Settings indexSettings = Settings.builder() - .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) - .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT) - .build(); + .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) + .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT) + .build(); + + private static final Settings allocateSettings = Settings.builder().put("index.allocate_dangling", true).build(); public void testCleanupWhenEmpty() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { @@ -57,6 +62,7 @@ public void testCleanupWhenEmpty() throws Exception { assertTrue(danglingState.getDanglingIndices().isEmpty()); } } + public void testDanglingIndicesDiscovery() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); @@ -181,7 +187,61 @@ public void testDanglingIndicesStripAliases() throws Exception { } } + public void testDanglingIndicesAreNotAllocatedWhenDisabled() throws Exception { + try (NodeEnvironment env = newNodeEnvironment()) { + MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); + LocalAllocateDangledIndices localAllocateDangledIndices = mock(LocalAllocateDangledIndices.class); + DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService, localAllocateDangledIndices); + + assertTrue(danglingState.getDanglingIndices().isEmpty()); + + // Given a metadata that does not enable allocation of dangling indices + MetaData metaData = MetaData.builder().build(); + + final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); + IndexMetaData dangledIndex = IndexMetaData.builder("test1").settings(settings).build(); + metaStateService.writeIndex("test_write", dangledIndex); + + danglingState.findNewAndAddDanglingIndices(metaData); + + // When calling the allocate method + danglingState.allocateDanglingIndices(metaData); + + // Ensure that allocation is not attempted + verify(localAllocateDangledIndices, never()).allocateDangled(any(), any()); + } + } + + public void testDanglingIndicesAreAllocatedWhenEnabled() throws Exception { + try (NodeEnvironment env = newNodeEnvironment()) { + MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); + LocalAllocateDangledIndices localAllocateDangledIndices = mock(LocalAllocateDangledIndices.class); + DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService, localAllocateDangledIndices); + + assertTrue(danglingState.getDanglingIndices().isEmpty()); + + // Given a metadata that enables allocation of dangling indices + MetaData metaData = MetaData.builder().persistentSettings(allocateSettings).build(); + + final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); + IndexMetaData dangledIndex = IndexMetaData.builder("test1").settings(settings).build(); + metaStateService.writeIndex("test_write", dangledIndex); + + danglingState.findNewAndAddDanglingIndices(metaData); + + // When calling the allocate method + danglingState.allocateDanglingIndices(metaData); + + // Ensure that allocation is attempted + verify(localAllocateDangledIndices).allocateDangled(any(), any()); + } + } + private DanglingIndicesState createDanglingIndicesState(NodeEnvironment env, MetaStateService metaStateService) { - return new DanglingIndicesState(env, metaStateService, null, mock(ClusterService.class)); + return createDanglingIndicesState(env, metaStateService, null); + } + + private DanglingIndicesState createDanglingIndicesState(NodeEnvironment env, MetaStateService metaStateService, LocalAllocateDangledIndices allocateDangledIndices) { + return new DanglingIndicesState(env, metaStateService, allocateDangledIndices, mock(ClusterService.class)); } } From 865831e2f001bd84e585ac71499c3844fa64ac63 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 14 Nov 2019 16:40:56 +0000 Subject: [PATCH 02/15] Fix allocate setting and implement ITs 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. --- .../common/settings/ClusterSettings.java | 2 + .../gateway/DanglingIndicesState.java | 21 ++- .../gateway/DanglingIndicesStateTests.java | 42 ++++- .../indices/recovery/DanglingIndicesIT.java | 156 ++++++++++++++++++ .../indices/recovery/IndexRecoveryIT.java | 1 - .../test/InternalTestCluster.java | 21 ++- 6 files changed, 225 insertions(+), 18 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java diff --git a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java index 81429e011f49c..b8919feee1353 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java @@ -70,6 +70,7 @@ import org.elasticsearch.discovery.SettingsBasedSeedHostsProvider; import org.elasticsearch.env.Environment; import org.elasticsearch.env.NodeEnvironment; +import org.elasticsearch.gateway.DanglingIndicesState; import org.elasticsearch.gateway.GatewayService; import org.elasticsearch.gateway.IncrementalClusterStateWriter; import org.elasticsearch.http.HttpTransportSettings; @@ -184,6 +185,7 @@ public void apply(Settings value, Settings current, Settings previous) { BalancedShardsAllocator.THRESHOLD_SETTING, ClusterRebalanceAllocationDecider.CLUSTER_ROUTING_ALLOCATION_ALLOW_REBALANCE_SETTING, ConcurrentRebalanceAllocationDecider.CLUSTER_ROUTING_ALLOCATION_CLUSTER_CONCURRENT_REBALANCE_SETTING, + DanglingIndicesState.ALLOCATE_DANGLING_INDICES_SETTING, EnableAllocationDecider.CLUSTER_ROUTING_ALLOCATION_ENABLE_SETTING, EnableAllocationDecider.CLUSTER_ROUTING_REBALANCE_ENABLE_SETTING, FilterAllocationDecider.CLUSTER_ROUTING_INCLUDE_GROUP_SETTING, diff --git a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java index e028b82014dac..57e895361af0e 100644 --- a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java +++ b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java @@ -30,6 +30,7 @@ import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; +import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.util.concurrent.ConcurrentCollections; import org.elasticsearch.env.NodeEnvironment; @@ -57,8 +58,9 @@ public class DanglingIndicesState implements ClusterStateListener { private static final Logger logger = LogManager.getLogger(DanglingIndicesState.class); public static final Setting ALLOCATE_DANGLING_INDICES_SETTING = Setting.boolSetting( - "index.allocate_dangling", + "cluster.allocate_dangling", false, + Setting.Property.NodeScope, Setting.Property.Dynamic ); @@ -68,6 +70,8 @@ public class DanglingIndicesState implements ClusterStateListener { private final Map danglingIndices = ConcurrentCollections.newConcurrentMap(); + private volatile boolean allocateDanglingIndices; + @Inject public DanglingIndicesState(NodeEnvironment nodeEnv, MetaStateService metaStateService, LocalAllocateDangledIndices allocateDangledIndices, ClusterService clusterService) { @@ -75,6 +79,15 @@ public DanglingIndicesState(NodeEnvironment nodeEnv, MetaStateService metaStateS this.metaStateService = metaStateService; this.allocateDangledIndices = allocateDangledIndices; clusterService.addListener(this); + + final ClusterSettings clusterSettings = clusterService.getClusterSettings(); + this.allocateDanglingIndices = ALLOCATE_DANGLING_INDICES_SETTING.get(clusterService.getSettings()); + clusterSettings + .addSettingsUpdateConsumer(ALLOCATE_DANGLING_INDICES_SETTING, this::setAllocateDanglingIndicesSetting); + } + + private void setAllocateDanglingIndicesSetting(boolean allocateDanglingIndices) { + this.allocateDanglingIndices = allocateDanglingIndices; } /** @@ -87,7 +100,7 @@ public void processDanglingIndices(final MetaData metaData) { } cleanupAllocatedDangledIndices(metaData); findNewAndAddDanglingIndices(metaData); - allocateDanglingIndices(metaData); + allocateDanglingIndices(); } /** @@ -178,8 +191,8 @@ private IndexMetaData stripAliases(IndexMetaData indexMetaData) { * Allocates the provided list of the dangled indices by sending them to the master node * for allocation. */ - void allocateDanglingIndices(MetaData metaData) { - if (ALLOCATE_DANGLING_INDICES_SETTING.get(metaData.settings()) == false) { + void allocateDanglingIndices() { + if (this.allocateDanglingIndices == false) { return; } diff --git a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java index 8ca06b00c9130..447017e36ac7b 100644 --- a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java @@ -24,6 +24,7 @@ import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.NodeEnvironment; import org.elasticsearch.index.Index; @@ -35,30 +36,38 @@ import java.nio.file.StandardCopyOption; import java.util.Map; +import static org.elasticsearch.gateway.DanglingIndicesState.ALLOCATE_DANGLING_INDICES_SETTING; import static org.hamcrest.Matchers.equalTo; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class DanglingIndicesStateTests extends ESTestCase { - private static Settings indexSettings = Settings.builder() + private static Settings indexSettings = Settings + .builder() .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT) .build(); - private static final Settings allocateSettings = Settings.builder().put("index.allocate_dangling", true).build(); + private static final Settings allocateSettings = Settings.builder().put(ALLOCATE_DANGLING_INDICES_SETTING.getKey(), true).build(); public void testCleanupWhenEmpty() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService); + // Given an empty state assertTrue(danglingState.getDanglingIndices().isEmpty()); + + // When passed an empty metadata MetaData metaData = MetaData.builder().build(); danglingState.cleanupAllocatedDangledIndices(metaData); + + // Then the state remains empty assertTrue(danglingState.getDanglingIndices().isEmpty()); } } @@ -68,15 +77,24 @@ public void testDanglingIndicesDiscovery() throws Exception { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService); + // Given an empty state assertTrue(danglingState.getDanglingIndices().isEmpty()); + + // When passed a metdata with an unknown index MetaData metaData = MetaData.builder().build(); final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); IndexMetaData dangledIndex = IndexMetaData.builder("test1").settings(settings).build(); metaStateService.writeIndex("test_write", dangledIndex); Map newDanglingIndices = danglingState.findNewDanglingIndices(metaData); + + // Then that index is considered dangling assertTrue(newDanglingIndices.containsKey(dangledIndex.getIndex())); + + // And when passed another metadata with that index metaData = MetaData.builder().put(dangledIndex, false).build(); newDanglingIndices = danglingState.findNewDanglingIndices(metaData); + + // Then then index is not considered to be a new dangling index for a second time assertFalse(newDanglingIndices.containsKey(dangledIndex.getIndex())); } } @@ -171,7 +189,8 @@ public void testDanglingIndicesStripAliases() throws Exception { DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService); final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); - IndexMetaData dangledIndex = IndexMetaData.builder("test1") + IndexMetaData dangledIndex = IndexMetaData + .builder("test1") .settings(settings) .putAlias(AliasMetaData.newAliasMetaDataBuilder("test_aliasd").build()) .build(); @@ -205,7 +224,7 @@ public void testDanglingIndicesAreNotAllocatedWhenDisabled() throws Exception { danglingState.findNewAndAddDanglingIndices(metaData); // When calling the allocate method - danglingState.allocateDanglingIndices(metaData); + danglingState.allocateDanglingIndices(); // Ensure that allocation is not attempted verify(localAllocateDangledIndices, never()).allocateDangled(any(), any()); @@ -230,7 +249,7 @@ public void testDanglingIndicesAreAllocatedWhenEnabled() throws Exception { danglingState.findNewAndAddDanglingIndices(metaData); // When calling the allocate method - danglingState.allocateDanglingIndices(metaData); + danglingState.allocateDanglingIndices(); // Ensure that allocation is attempted verify(localAllocateDangledIndices).allocateDangled(any(), any()); @@ -241,7 +260,16 @@ private DanglingIndicesState createDanglingIndicesState(NodeEnvironment env, Met return createDanglingIndicesState(env, metaStateService, null); } - private DanglingIndicesState createDanglingIndicesState(NodeEnvironment env, MetaStateService metaStateService, LocalAllocateDangledIndices allocateDangledIndices) { - return new DanglingIndicesState(env, metaStateService, allocateDangledIndices, mock(ClusterService.class)); + private DanglingIndicesState createDanglingIndicesState( + NodeEnvironment env, + MetaStateService metaStateService, + LocalAllocateDangledIndices indexAllocator + ) { + final ClusterService clusterServiceMock = mock(ClusterService.class); + when(clusterServiceMock.getClusterSettings()) + .thenReturn(new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)); + when(clusterServiceMock.getSettings()).thenReturn(allocateSettings); + + return new DanglingIndicesState(env, metaStateService, indexAllocator, clusterServiceMock); } } diff --git a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java new file mode 100644 index 0000000000000..a7a2ba0bd1883 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java @@ -0,0 +1,156 @@ +package org.elasticsearch.indices.recovery; + +import org.elasticsearch.action.index.IndexRequestBuilder; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.store.Store; +import org.elasticsearch.test.ESIntegTestCase; +import org.elasticsearch.test.ESIntegTestCase.ClusterScope; +import org.elasticsearch.test.InternalTestCluster; +import org.junit.After; + +import java.util.Collections; + +import static org.elasticsearch.cluster.metadata.IndexGraveyard.SETTING_MAX_TOMBSTONES; +import static org.elasticsearch.gateway.DanglingIndicesState.ALLOCATE_DANGLING_INDICES_SETTING; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.hamcrest.Matchers.equalTo; + +@ClusterScope(numDataNodes = 3) +public class DanglingIndicesIT extends ESIntegTestCase { + private static final String INDEX_NAME = "test-idx-1"; + + private static final int MIN_DOC_COUNT = 500; + private static final int MAX_DOC_COUNT = 1000; + private static final int SHARD_COUNT = 1; + private static final int REPLICA_COUNT = 2; + + @Override + protected Settings nodeSettings(int nodeOrdinal) { + return Settings + .builder() + .put(super.nodeSettings(nodeOrdinal)) + // Don't keep any indices in the graveyard, so that when we delete an index, + // it's definitely considered to be dangling. + .put(SETTING_MAX_TOMBSTONES.getKey(), 0) + .build(); + } + + @After + public void cleanup() { + // Set to null in order to clean up whatever actions the tests took + setRecoveryEnabled(null); + } + + /** + * Check that when dangling indices are discovered, then they are recovered into + * the cluster, so long as the recovery setting is enabled. + */ + public void testDanglingIndicesAreRecoveredWhenSettingIsEnabled() throws Exception { + logger.info("--> starting cluster"); + internalCluster().startNodes(); + + // Create an index and distribute it across the 3 nodes + createAndPopulateIndex(INDEX_NAME, SHARD_COUNT, REPLICA_COUNT); + ensureGreen(); + + // Recover dangling indices automatically. + setRecoveryEnabled(true); + + // This is so that when then node comes back up, we have a dangling index that can be recovered. + logger.info("--> restarted a random node and deleting the index while it's down"); + internalCluster().restartRandomDataNode(new InternalTestCluster.RestartCallback() { + + @Override + public Settings onNodeStopped(String nodeName) throws Exception { + deleteIndex(INDEX_NAME); + return super.onNodeStopped(nodeName); + } + }); + + ensureGreen(); + + assertTrue("Expected dangling index to be recovered", indexExists(INDEX_NAME)); + } + + /** + * Check that when dangling indices are discovered, then they are not recovered into + * the cluster when the recovery setting is disabled. + */ + public void testDanglingIndicesAreNotRecoveredWhenSettingIsDisabled() throws Exception { + logger.info("--> starting cluster"); + internalCluster().startNodes(); + + // Create an index and distribute it across the 3 nodes + createAndPopulateIndex(INDEX_NAME, SHARD_COUNT, REPLICA_COUNT); + + // Create another index so that once we drop the first index, we + // can still assert that the cluster is green. + createAndPopulateIndex(INDEX_NAME + "-other", SHARD_COUNT, REPLICA_COUNT); + + ensureGreen(); + + // 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"); + internalCluster().restartRandomDataNode(new InternalTestCluster.RestartCallback() { + + @Override + public Settings onNodeStopped(String nodeName) throws Exception { + deleteIndex(INDEX_NAME); + return super.onNodeStopped(nodeName); + } + }); + + ensureGreen(); + + assertFalse("Expected dangling index to be recovered", indexExists(INDEX_NAME)); + } + + private void createAndPopulateIndex(String name, int shardCount, int replicaCount) throws InterruptedException { + logger.info("--> creating test index: {}", name); + assertAcked( + prepareCreate( + name, + Settings + .builder() + .put("number_of_shards", shardCount) + .put("number_of_replicas", replicaCount) + .put(Store.INDEX_STORE_STATS_REFRESH_INTERVAL_SETTING.getKey(), 0) + ) + ); + ensureGreen(); + + logger.info("--> indexing sample data"); + final int numDocs = between(MIN_DOC_COUNT, MAX_DOC_COUNT); + final IndexRequestBuilder[] docs = new IndexRequestBuilder[numDocs]; + + for (int i = 0; i < numDocs; i++) { + docs[i] = client() + .prepareIndex(name) + .setSource("foo-int", randomInt(), "foo-string", randomAlphaOfLength(32), "foo-float", randomFloat()); + } + + indexRandom(true, docs); + flush(); + assertThat(client().prepareSearch(name).setSize(0).get().getHits().getTotalHits().value, equalTo((long) numDocs)); + + client().admin().indices().prepareStats(name).execute().actionGet(); + } + + private void deleteIndex(String indexName) { + logger.info("--> deleting test index: {}", indexName); + + assertAcked(client().admin().indices().prepareDelete(indexName)); + } + + private void setRecoveryEnabled(Boolean enabled) { + assertAcked( + client() + .admin() + .cluster() + .prepareUpdateSettings() + // Map.of doesn't like null ¯\_(ツ)_/¯ + .setTransientSettings(Collections.singletonMap(ALLOCATE_DANGLING_INDICES_SETTING.getKey(), enabled)) + ); + } +} diff --git a/server/src/test/java/org/elasticsearch/indices/recovery/IndexRecoveryIT.java b/server/src/test/java/org/elasticsearch/indices/recovery/IndexRecoveryIT.java index 6985e33e8afd3..def89755aa0f7 100644 --- a/server/src/test/java/org/elasticsearch/indices/recovery/IndexRecoveryIT.java +++ b/server/src/test/java/org/elasticsearch/indices/recovery/IndexRecoveryIT.java @@ -142,7 +142,6 @@ public class IndexRecoveryIT extends ESIntegTestCase { private static final String INDEX_NAME = "test-idx-1"; - private static final String INDEX_TYPE = "test-type-1"; private static final String REPO_NAME = "test-repo-1"; private static final String SNAP_NAME = "test-snap-1"; diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java index d0fd121f3cb84..488378d1621f8 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java @@ -121,6 +121,7 @@ import java.util.Map; import java.util.NavigableMap; import java.util.Objects; +import java.util.Optional; import java.util.Random; import java.util.Set; import java.util.TreeMap; @@ -150,10 +151,11 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.greaterThanOrEqualTo; -import static org.junit.Assert.assertEquals; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -1467,21 +1469,22 @@ public InetSocketAddress[] httpAddresses() { } /** - * 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. */ - public synchronized boolean stopRandomDataNode() throws IOException { + public synchronized Optional stopRandomDataNode() throws IOException { ensureOpen(); NodeAndClient nodeAndClient = getRandomNodeAndClient(DATA_NODE_PREDICATE); if (nodeAndClient != null) { logger.info("Closing random node [{}] ", nodeAndClient.name); stopNodesAndClient(nodeAndClient); - return true; + return Optional.of(nodeAndClient.name); } - return false; + return Optional.empty(); } /** - * Stops a random node in the cluster that applies to the given filter or non if the non of the nodes applies to the + * Stops a random node in the cluster that applies to the given filter. Does nothing if none of the nodes match the * filter. */ public synchronized void stopRandomNode(final Predicate filter) throws IOException { @@ -1902,6 +1905,12 @@ public String startNode(Settings settings) { return startNodes(settings).get(0); } + public void startNode(String nodeName) { + final NodeAndClient nodeAndClient = nodes.get(nodeName); + assertNotNull("No client found for node name: " + nodeName, nodeAndClient); + nodeAndClient.startNode(); + } + /** * Starts multiple nodes with default settings and returns their names */ From e3777722dbb856ad90117eab54f45bb08bc9791b Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 15 Nov 2019 09:47:17 +0000 Subject: [PATCH 03/15] Finish fixing unit tests --- .../gateway/DanglingIndicesState.java | 2 +- .../gateway/DanglingIndicesStateTests.java | 39 +++++++++++++------ .../indices/recovery/DanglingIndicesIT.java | 12 ++---- 3 files changed, 32 insertions(+), 21 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java index 57e895361af0e..6331886d14b72 100644 --- a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java +++ b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java @@ -86,7 +86,7 @@ public DanglingIndicesState(NodeEnvironment nodeEnv, MetaStateService metaStateS .addSettingsUpdateConsumer(ALLOCATE_DANGLING_INDICES_SETTING, this::setAllocateDanglingIndicesSetting); } - private void setAllocateDanglingIndicesSetting(boolean allocateDanglingIndices) { + public void setAllocateDanglingIndicesSetting(boolean allocateDanglingIndices) { this.allocateDanglingIndices = allocateDanglingIndices; } diff --git a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java index 447017e36ac7b..0d87ffa4750bc 100644 --- a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java @@ -46,15 +46,12 @@ public class DanglingIndicesStateTests extends ESTestCase { - private static Settings indexSettings = Settings - .builder() + private static Settings indexSettings = Settings.builder() .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT) .build(); - private static final Settings allocateSettings = Settings.builder().put(ALLOCATE_DANGLING_INDICES_SETTING.getKey(), true).build(); - public void testCleanupWhenEmpty() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); @@ -101,9 +98,11 @@ public void testDanglingIndicesDiscovery() throws Exception { public void testInvalidIndexFolder() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { + // Given an empty state MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService); + // When passed settings for an index whose folder does not exist MetaData metaData = MetaData.builder().build(); final String uuid = "test1UUID"; final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, uuid); @@ -114,6 +113,8 @@ public void testInvalidIndexFolder() throws Exception { Files.move(path, path.resolveSibling("invalidUUID"), StandardCopyOption.ATOMIC_MOVE); } } + + // Then an exception is thrown describing the problem try { danglingState.findNewDanglingIndices(metaData); fail("no exception thrown for invalid folder name"); @@ -169,28 +170,33 @@ public void testDanglingProcessing() throws Exception { public void testDanglingIndicesNotImportedWhenTombstonePresent() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { + // Given an empty state MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService); + // When passed a dangling index final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); IndexMetaData dangledIndex = IndexMetaData.builder("test1").settings(settings).build(); metaStateService.writeIndex("test_write", dangledIndex); + // And there is a tombstone for that index final IndexGraveyard graveyard = IndexGraveyard.builder().addTombstone(dangledIndex.getIndex()).build(); final MetaData metaData = MetaData.builder().indexGraveyard(graveyard).build(); - assertThat(danglingState.findNewDanglingIndices(metaData).size(), equalTo(0)); + // Then that index is not imported + assertThat(danglingState.findNewDanglingIndices(metaData).size(), equalTo(0)); } } public void testDanglingIndicesStripAliases() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { + // Given an empty state MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService); + // When passed an index that has an alias final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); - IndexMetaData dangledIndex = IndexMetaData - .builder("test1") + IndexMetaData dangledIndex = IndexMetaData.builder("test1") .settings(settings) .putAlias(AliasMetaData.newAliasMetaDataBuilder("test_aliasd").build()) .build(); @@ -199,9 +205,13 @@ public void testDanglingIndicesStripAliases() throws Exception { final MetaData metaData = MetaData.builder().build(); Map newDanglingIndices = danglingState.findNewDanglingIndices(metaData); + + // Then the index is identifying as dangling assertThat(newDanglingIndices.size(), equalTo(1)); Map.Entry entry = newDanglingIndices.entrySet().iterator().next(); assertThat(entry.getKey().getName(), equalTo("test1")); + + // And the alias is removed assertThat(entry.getValue().getAliases().size(), equalTo(0)); } } @@ -226,7 +236,7 @@ public void testDanglingIndicesAreNotAllocatedWhenDisabled() throws Exception { // When calling the allocate method danglingState.allocateDanglingIndices(); - // Ensure that allocation is not attempted + // Then allocation is not attempted verify(localAllocateDangledIndices, never()).allocateDangled(any(), any()); } } @@ -239,8 +249,10 @@ public void testDanglingIndicesAreAllocatedWhenEnabled() throws Exception { assertTrue(danglingState.getDanglingIndices().isEmpty()); - // Given a metadata that enables allocation of dangling indices - MetaData metaData = MetaData.builder().persistentSettings(allocateSettings).build(); + // Given a state where automatic allocation is enabled + danglingState.setAllocateDanglingIndicesSetting(true); + + MetaData metaData = MetaData.builder().build(); final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); IndexMetaData dangledIndex = IndexMetaData.builder("test1").settings(settings).build(); @@ -265,9 +277,12 @@ private DanglingIndicesState createDanglingIndicesState( MetaStateService metaStateService, LocalAllocateDangledIndices indexAllocator ) { + final Settings allocateSettings = Settings.builder().put(ALLOCATE_DANGLING_INDICES_SETTING.getKey(), false).build(); + final ClusterService clusterServiceMock = mock(ClusterService.class); - when(clusterServiceMock.getClusterSettings()) - .thenReturn(new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)); + when(clusterServiceMock.getClusterSettings()).thenReturn( + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS) + ); when(clusterServiceMock.getSettings()).thenReturn(allocateSettings); return new DanglingIndicesState(env, metaStateService, indexAllocator, clusterServiceMock); diff --git a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java index a7a2ba0bd1883..4239a95cdd79b 100644 --- a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java +++ b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java @@ -26,8 +26,7 @@ public class DanglingIndicesIT extends ESIntegTestCase { @Override protected Settings nodeSettings(int nodeOrdinal) { - return Settings - .builder() + return Settings.builder() .put(super.nodeSettings(nodeOrdinal)) // Don't keep any indices in the graveyard, so that when we delete an index, // it's definitely considered to be dangling. @@ -111,8 +110,7 @@ private void createAndPopulateIndex(String name, int shardCount, int replicaCoun assertAcked( prepareCreate( name, - Settings - .builder() + Settings.builder() .put("number_of_shards", shardCount) .put("number_of_replicas", replicaCount) .put(Store.INDEX_STORE_STATS_REFRESH_INTERVAL_SETTING.getKey(), 0) @@ -125,8 +123,7 @@ private void createAndPopulateIndex(String name, int shardCount, int replicaCoun final IndexRequestBuilder[] docs = new IndexRequestBuilder[numDocs]; for (int i = 0; i < numDocs; i++) { - docs[i] = client() - .prepareIndex(name) + docs[i] = client().prepareIndex(name) .setSource("foo-int", randomInt(), "foo-string", randomAlphaOfLength(32), "foo-float", randomFloat()); } @@ -145,8 +142,7 @@ private void deleteIndex(String indexName) { private void setRecoveryEnabled(Boolean enabled) { assertAcked( - client() - .admin() + client().admin() .cluster() .prepareUpdateSettings() // Map.of doesn't like null ¯\_(ツ)_/¯ From 63933573ab00a14aa4a1b172bbde7bba1cd4529f Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 15 Nov 2019 09:56:27 +0000 Subject: [PATCH 04/15] Rename the new setting --- .../elasticsearch/common/settings/ClusterSettings.java | 2 +- .../org/elasticsearch/gateway/DanglingIndicesState.java | 8 ++++---- .../elasticsearch/gateway/DanglingIndicesStateTests.java | 4 ++-- .../elasticsearch/indices/recovery/DanglingIndicesIT.java | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java index b8919feee1353..4266e1219e902 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java @@ -185,7 +185,7 @@ public void apply(Settings value, Settings current, Settings previous) { BalancedShardsAllocator.THRESHOLD_SETTING, ClusterRebalanceAllocationDecider.CLUSTER_ROUTING_ALLOCATION_ALLOW_REBALANCE_SETTING, ConcurrentRebalanceAllocationDecider.CLUSTER_ROUTING_ALLOCATION_CLUSTER_CONCURRENT_REBALANCE_SETTING, - DanglingIndicesState.ALLOCATE_DANGLING_INDICES_SETTING, + DanglingIndicesState.AUTO_IMPORT_DANGLING_INDICES_SETTING, EnableAllocationDecider.CLUSTER_ROUTING_ALLOCATION_ENABLE_SETTING, EnableAllocationDecider.CLUSTER_ROUTING_REBALANCE_ENABLE_SETTING, FilterAllocationDecider.CLUSTER_ROUTING_INCLUDE_GROUP_SETTING, diff --git a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java index 6331886d14b72..1d6f098475a56 100644 --- a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java +++ b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java @@ -57,8 +57,8 @@ public class DanglingIndicesState implements ClusterStateListener { private static final Logger logger = LogManager.getLogger(DanglingIndicesState.class); - public static final Setting ALLOCATE_DANGLING_INDICES_SETTING = Setting.boolSetting( - "cluster.allocate_dangling", + public static final Setting AUTO_IMPORT_DANGLING_INDICES_SETTING = Setting.boolSetting( + "gateway.auto_import_dangling_indices", false, Setting.Property.NodeScope, Setting.Property.Dynamic @@ -81,9 +81,9 @@ public DanglingIndicesState(NodeEnvironment nodeEnv, MetaStateService metaStateS clusterService.addListener(this); final ClusterSettings clusterSettings = clusterService.getClusterSettings(); - this.allocateDanglingIndices = ALLOCATE_DANGLING_INDICES_SETTING.get(clusterService.getSettings()); + this.allocateDanglingIndices = AUTO_IMPORT_DANGLING_INDICES_SETTING.get(clusterService.getSettings()); clusterSettings - .addSettingsUpdateConsumer(ALLOCATE_DANGLING_INDICES_SETTING, this::setAllocateDanglingIndicesSetting); + .addSettingsUpdateConsumer(AUTO_IMPORT_DANGLING_INDICES_SETTING, this::setAllocateDanglingIndicesSetting); } public void setAllocateDanglingIndicesSetting(boolean allocateDanglingIndices) { diff --git a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java index 0d87ffa4750bc..1a0e39f30e4b8 100644 --- a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java @@ -36,7 +36,7 @@ import java.nio.file.StandardCopyOption; import java.util.Map; -import static org.elasticsearch.gateway.DanglingIndicesState.ALLOCATE_DANGLING_INDICES_SETTING; +import static org.elasticsearch.gateway.DanglingIndicesState.AUTO_IMPORT_DANGLING_INDICES_SETTING; import static org.hamcrest.Matchers.equalTo; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; @@ -277,7 +277,7 @@ private DanglingIndicesState createDanglingIndicesState( MetaStateService metaStateService, LocalAllocateDangledIndices indexAllocator ) { - final Settings allocateSettings = Settings.builder().put(ALLOCATE_DANGLING_INDICES_SETTING.getKey(), false).build(); + final Settings allocateSettings = Settings.builder().put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), false).build(); final ClusterService clusterServiceMock = mock(ClusterService.class); when(clusterServiceMock.getClusterSettings()).thenReturn( diff --git a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java index 4239a95cdd79b..f77a99dc2905a 100644 --- a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java +++ b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java @@ -11,7 +11,7 @@ import java.util.Collections; import static org.elasticsearch.cluster.metadata.IndexGraveyard.SETTING_MAX_TOMBSTONES; -import static org.elasticsearch.gateway.DanglingIndicesState.ALLOCATE_DANGLING_INDICES_SETTING; +import static org.elasticsearch.gateway.DanglingIndicesState.AUTO_IMPORT_DANGLING_INDICES_SETTING; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.hamcrest.Matchers.equalTo; @@ -146,7 +146,7 @@ private void setRecoveryEnabled(Boolean enabled) { .cluster() .prepareUpdateSettings() // Map.of doesn't like null ¯\_(ツ)_/¯ - .setTransientSettings(Collections.singletonMap(ALLOCATE_DANGLING_INDICES_SETTING.getKey(), enabled)) + .setTransientSettings(Collections.singletonMap(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), enabled)) ); } } From 7950e8660c5ee512825b01ef8cf255b3c47eebe8 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 15 Nov 2019 15:03:38 +0000 Subject: [PATCH 05/15] WIP - trying to make new setting static New setting `gateway.auto_import_dangling_indices` doesn't need to be dynamic. Unfortunatlely, this has broken one of the ITs. --- .../gateway/DanglingIndicesState.java | 9 +--- .../indices/recovery/DanglingIndicesIT.java | 42 +++++-------------- 2 files changed, 12 insertions(+), 39 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java index 1d6f098475a56..5e5db2b37d735 100644 --- a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java +++ b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java @@ -30,7 +30,6 @@ import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.inject.Inject; -import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.util.concurrent.ConcurrentCollections; import org.elasticsearch.env.NodeEnvironment; @@ -60,8 +59,7 @@ public class DanglingIndicesState implements ClusterStateListener { public static final Setting AUTO_IMPORT_DANGLING_INDICES_SETTING = Setting.boolSetting( "gateway.auto_import_dangling_indices", false, - Setting.Property.NodeScope, - Setting.Property.Dynamic + Setting.Property.NodeScope ); private final NodeEnvironment nodeEnv; @@ -70,7 +68,7 @@ public class DanglingIndicesState implements ClusterStateListener { private final Map danglingIndices = ConcurrentCollections.newConcurrentMap(); - private volatile boolean allocateDanglingIndices; + private boolean allocateDanglingIndices; @Inject public DanglingIndicesState(NodeEnvironment nodeEnv, MetaStateService metaStateService, @@ -80,10 +78,7 @@ public DanglingIndicesState(NodeEnvironment nodeEnv, MetaStateService metaStateS this.allocateDangledIndices = allocateDangledIndices; clusterService.addListener(this); - final ClusterSettings clusterSettings = clusterService.getClusterSettings(); this.allocateDanglingIndices = AUTO_IMPORT_DANGLING_INDICES_SETTING.get(clusterService.getSettings()); - clusterSettings - .addSettingsUpdateConsumer(AUTO_IMPORT_DANGLING_INDICES_SETTING, this::setAllocateDanglingIndicesSetting); } public void setAllocateDanglingIndicesSetting(boolean allocateDanglingIndices) { diff --git a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java index f77a99dc2905a..1a50ba6aed22f 100644 --- a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java +++ b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java @@ -6,55 +6,43 @@ import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.ESIntegTestCase.ClusterScope; import org.elasticsearch.test.InternalTestCluster; -import org.junit.After; - -import java.util.Collections; import static org.elasticsearch.cluster.metadata.IndexGraveyard.SETTING_MAX_TOMBSTONES; import static org.elasticsearch.gateway.DanglingIndicesState.AUTO_IMPORT_DANGLING_INDICES_SETTING; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.hamcrest.Matchers.equalTo; -@ClusterScope(numDataNodes = 3) +@ClusterScope(numDataNodes = 3, scope = ESIntegTestCase.Scope.TEST) public class DanglingIndicesIT extends ESIntegTestCase { private static final String INDEX_NAME = "test-idx-1"; private static final int MIN_DOC_COUNT = 500; - private static final int MAX_DOC_COUNT = 1000; private static final int SHARD_COUNT = 1; private static final int REPLICA_COUNT = 2; - @Override - protected Settings nodeSettings(int nodeOrdinal) { + private Settings buildSettings(boolean importDanglingIndices) { return Settings.builder() - .put(super.nodeSettings(nodeOrdinal)) // Don't keep any indices in the graveyard, so that when we delete an index, // it's definitely considered to be dangling. .put(SETTING_MAX_TOMBSTONES.getKey(), 0) + .put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), importDanglingIndices) .build(); } - @After - public void cleanup() { - // Set to null in order to clean up whatever actions the tests took - setRecoveryEnabled(null); - } - /** * Check that when dangling indices are discovered, then they are recovered into * the cluster, so long as the recovery setting is enabled. */ public void testDanglingIndicesAreRecoveredWhenSettingIsEnabled() throws Exception { logger.info("--> starting cluster"); - internalCluster().startNodes(); + final Settings settings = buildSettings(true); + logger.warn("FOO: " + settings.keySet()); + internalCluster().startNodes(settings); // Create an index and distribute it across the 3 nodes createAndPopulateIndex(INDEX_NAME, SHARD_COUNT, REPLICA_COUNT); ensureGreen(); - // Recover dangling indices automatically. - setRecoveryEnabled(true); - // This is so that when then node comes back up, we have a dangling index that can be recovered. logger.info("--> restarted a random node and deleting the index while it's down"); internalCluster().restartRandomDataNode(new InternalTestCluster.RestartCallback() { @@ -68,7 +56,7 @@ public Settings onNodeStopped(String nodeName) throws Exception { ensureGreen(); - assertTrue("Expected dangling index to be recovered", indexExists(INDEX_NAME)); + assertTrue("Expected dangling index " + INDEX_NAME + " to be recovered", indexExists(INDEX_NAME)); } /** @@ -77,7 +65,7 @@ public Settings onNodeStopped(String nodeName) throws Exception { */ public void testDanglingIndicesAreNotRecoveredWhenSettingIsDisabled() throws Exception { logger.info("--> starting cluster"); - internalCluster().startNodes(); + internalCluster().startNodes(buildSettings(false)); // Create an index and distribute it across the 3 nodes createAndPopulateIndex(INDEX_NAME, SHARD_COUNT, REPLICA_COUNT); @@ -102,7 +90,7 @@ public Settings onNodeStopped(String nodeName) throws Exception { ensureGreen(); - assertFalse("Expected dangling index to be recovered", indexExists(INDEX_NAME)); + assertFalse("Did not expect dangling index " + INDEX_NAME + " to be recovered", indexExists(INDEX_NAME)); } private void createAndPopulateIndex(String name, int shardCount, int replicaCount) throws InterruptedException { @@ -119,7 +107,7 @@ private void createAndPopulateIndex(String name, int shardCount, int replicaCoun ensureGreen(); logger.info("--> indexing sample data"); - final int numDocs = between(MIN_DOC_COUNT, MAX_DOC_COUNT); + final int numDocs = between(MIN_DOC_COUNT, 1000); final IndexRequestBuilder[] docs = new IndexRequestBuilder[numDocs]; for (int i = 0; i < numDocs; i++) { @@ -139,14 +127,4 @@ private void deleteIndex(String indexName) { assertAcked(client().admin().indices().prepareDelete(indexName)); } - - private void setRecoveryEnabled(Boolean enabled) { - assertAcked( - client().admin() - .cluster() - .prepareUpdateSettings() - // Map.of doesn't like null ¯\_(ツ)_/¯ - .setTransientSettings(Collections.singletonMap(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), enabled)) - ); - } } From ce3b5f43b743c50d390063f89420bff56137248e Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 15 Nov 2019 15:17:25 +0000 Subject: [PATCH 06/15] Add docs for gateway.auto_import_dangling_indices --- docs/reference/modules/gateway.asciidoc | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/docs/reference/modules/gateway.asciidoc b/docs/reference/modules/gateway.asciidoc index 2b0783c9de0b0..4460c92f27e27 100644 --- a/docs/reference/modules/gateway.asciidoc +++ b/docs/reference/modules/gateway.asciidoc @@ -52,9 +52,22 @@ NOTE: These settings only take effect on a full cluster restart. [[modules-gateway-dangling-indices]] === Dangling indices -When a node joins the cluster, any shards stored in its local data -directory which do not already exist in the cluster 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 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 +stored in its local data directory and do not already exist in the +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 +of the other indices in the cluster, adding the old nodes will cause the +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): + +* A deleted index might suddenly reappear when a node joins the cluster. +* You might delete an index and see the immediate creation of another index + with the same name, containing stale mappings and old data. +* New documents could be written to the index before anyone realises that + it has been recovered +* {es} may not be able to find copies of all of the shards of the index, + resulting in a red cluster state. From 77b4f09d33408c2dd832dac9456ee1a602de157b Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 15 Nov 2019 15:41:00 +0000 Subject: [PATCH 07/15] Fix ITs --- .../elasticsearch/indices/recovery/DanglingIndicesIT.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java index 1a50ba6aed22f..891cba59e44bc 100644 --- a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java +++ b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java @@ -12,7 +12,7 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.hamcrest.Matchers.equalTo; -@ClusterScope(numDataNodes = 3, scope = ESIntegTestCase.Scope.TEST) +@ClusterScope(numDataNodes = 0, scope = ESIntegTestCase.Scope.TEST) public class DanglingIndicesIT extends ESIntegTestCase { private static final String INDEX_NAME = "test-idx-1"; @@ -36,8 +36,7 @@ private Settings buildSettings(boolean importDanglingIndices) { public void testDanglingIndicesAreRecoveredWhenSettingIsEnabled() throws Exception { logger.info("--> starting cluster"); final Settings settings = buildSettings(true); - logger.warn("FOO: " + settings.keySet()); - internalCluster().startNodes(settings); + internalCluster().startNodes(3, settings); // Create an index and distribute it across the 3 nodes createAndPopulateIndex(INDEX_NAME, SHARD_COUNT, REPLICA_COUNT); @@ -65,7 +64,7 @@ public Settings onNodeStopped(String nodeName) throws Exception { */ public void testDanglingIndicesAreNotRecoveredWhenSettingIsDisabled() throws Exception { logger.info("--> starting cluster"); - internalCluster().startNodes(buildSettings(false)); + internalCluster().startNodes(3, buildSettings(false)); // Create an index and distribute it across the 3 nodes createAndPopulateIndex(INDEX_NAME, SHARD_COUNT, REPLICA_COUNT); From 3a453e92c6b6536deeb7fed1807db6f1edb4504f Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 15 Nov 2019 16:03:20 +0000 Subject: [PATCH 08/15] Add missing license --- .../indices/recovery/DanglingIndicesIT.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java index 891cba59e44bc..26765c67caa4d 100644 --- a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java +++ b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java @@ -1,3 +1,22 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + package org.elasticsearch.indices.recovery; import org.elasticsearch.action.index.IndexRequestBuilder; From 54db504d3113a722226d41ec56463ca8ee82955a Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Tue, 19 Nov 2019 13:19:38 +0000 Subject: [PATCH 09/15] Fix test --- .../coordination/UnsafeBootstrapAndDetachCommandIT.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java b/server/src/test/java/org/elasticsearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java index 55fc91b943640..cbf311d0d34b2 100644 --- a/server/src/test/java/org/elasticsearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java +++ b/server/src/test/java/org/elasticsearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java @@ -44,6 +44,7 @@ import java.util.Locale; import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; +import static org.elasticsearch.gateway.DanglingIndicesState.AUTO_IMPORT_DANGLING_INDICES_SETTING; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; import static org.elasticsearch.indices.recovery.RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; @@ -396,8 +397,13 @@ public boolean clearData(String nodeName) { } }); + final Settings settingsWithAutoImport = Settings.builder() + .put(dataNodeDataPathSettings) + .put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), true) + .build(); + logger.info("--> start data-only only node and ensure 2 nodes stable cluster"); - internalCluster().startDataOnlyNode(dataNodeDataPathSettings); + internalCluster().startDataOnlyNode(settingsWithAutoImport); ensureStableCluster(2); logger.info("--> verify that the dangling index exists and has green status"); From 71bcbde1b6f1e646952e1d017649fe6d2c161a3a Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 21 Nov 2019 14:53:31 +0000 Subject: [PATCH 10/15] Address review feedback --- docs/reference/modules/gateway.asciidoc | 25 +++------- .../gateway/DanglingIndicesState.java | 8 +-- .../UnsafeBootstrapAndDetachCommandIT.java | 8 +-- .../gateway/DanglingIndicesStateTests.java | 49 +++---------------- 4 files changed, 18 insertions(+), 72 deletions(-) diff --git a/docs/reference/modules/gateway.asciidoc b/docs/reference/modules/gateway.asciidoc index 4460c92f27e27..2b0783c9de0b0 100644 --- a/docs/reference/modules/gateway.asciidoc +++ b/docs/reference/modules/gateway.asciidoc @@ -52,22 +52,9 @@ NOTE: These settings only take effect on a full cluster restart. [[modules-gateway-dangling-indices]] === Dangling indices -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 -`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 -of the other indices in the cluster, adding the old nodes will cause the -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): - -* A deleted index might suddenly reappear when a node joins the cluster. -* You might delete an index and see the immediate creation of another index - with the same name, containing stale mappings and old data. -* New documents could be written to the index before anyone realises that - it has been recovered -* {es} may not be able to find copies of all of the shards of the index, - resulting in a red cluster state. +When a node joins the cluster, any shards stored in its local data +directory which do not already exist in the cluster 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 of +the other indices in the cluster, adding the old nodes will cause the old +indices to be imported, instead of being deleted. diff --git a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java index 5e5db2b37d735..953019d622640 100644 --- a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java +++ b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java @@ -58,7 +58,7 @@ public class DanglingIndicesState implements ClusterStateListener { public static final Setting AUTO_IMPORT_DANGLING_INDICES_SETTING = Setting.boolSetting( "gateway.auto_import_dangling_indices", - false, + true, Setting.Property.NodeScope ); @@ -76,9 +76,11 @@ public DanglingIndicesState(NodeEnvironment nodeEnv, MetaStateService metaStateS this.nodeEnv = nodeEnv; this.metaStateService = metaStateService; this.allocateDangledIndices = allocateDangledIndices; - clusterService.addListener(this); - this.allocateDanglingIndices = AUTO_IMPORT_DANGLING_INDICES_SETTING.get(clusterService.getSettings()); + + if (this.allocateDanglingIndices) { + clusterService.addListener(this); + } } public void setAllocateDanglingIndicesSetting(boolean allocateDanglingIndices) { diff --git a/server/src/test/java/org/elasticsearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java b/server/src/test/java/org/elasticsearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java index cbf311d0d34b2..55fc91b943640 100644 --- a/server/src/test/java/org/elasticsearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java +++ b/server/src/test/java/org/elasticsearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java @@ -44,7 +44,6 @@ import java.util.Locale; import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; -import static org.elasticsearch.gateway.DanglingIndicesState.AUTO_IMPORT_DANGLING_INDICES_SETTING; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; import static org.elasticsearch.indices.recovery.RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; @@ -397,13 +396,8 @@ public boolean clearData(String nodeName) { } }); - final Settings settingsWithAutoImport = Settings.builder() - .put(dataNodeDataPathSettings) - .put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), true) - .build(); - logger.info("--> start data-only only node and ensure 2 nodes stable cluster"); - internalCluster().startDataOnlyNode(settingsWithAutoImport); + internalCluster().startDataOnlyNode(dataNodeDataPathSettings); ensureStableCluster(2); logger.info("--> verify that the dangling index exists and has green status"); diff --git a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java index 1a0e39f30e4b8..bbb063d05d6af 100644 --- a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java @@ -57,14 +57,9 @@ public void testCleanupWhenEmpty() throws Exception { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService); - // Given an empty state assertTrue(danglingState.getDanglingIndices().isEmpty()); - - // When passed an empty metadata MetaData metaData = MetaData.builder().build(); danglingState.cleanupAllocatedDangledIndices(metaData); - - // Then the state remains empty assertTrue(danglingState.getDanglingIndices().isEmpty()); } } @@ -73,36 +68,24 @@ public void testDanglingIndicesDiscovery() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService); - - // Given an empty state assertTrue(danglingState.getDanglingIndices().isEmpty()); - - // When passed a metdata with an unknown index MetaData metaData = MetaData.builder().build(); final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); IndexMetaData dangledIndex = IndexMetaData.builder("test1").settings(settings).build(); metaStateService.writeIndex("test_write", dangledIndex); Map newDanglingIndices = danglingState.findNewDanglingIndices(metaData); - - // Then that index is considered dangling assertTrue(newDanglingIndices.containsKey(dangledIndex.getIndex())); - - // And when passed another metadata with that index metaData = MetaData.builder().put(dangledIndex, false).build(); newDanglingIndices = danglingState.findNewDanglingIndices(metaData); - - // Then then index is not considered to be a new dangling index for a second time assertFalse(newDanglingIndices.containsKey(dangledIndex.getIndex())); } } public void testInvalidIndexFolder() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { - // Given an empty state MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService); - // When passed settings for an index whose folder does not exist MetaData metaData = MetaData.builder().build(); final String uuid = "test1UUID"; final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, uuid); @@ -113,8 +96,6 @@ public void testInvalidIndexFolder() throws Exception { Files.move(path, path.resolveSibling("invalidUUID"), StandardCopyOption.ATOMIC_MOVE); } } - - // Then an exception is thrown describing the problem try { danglingState.findNewDanglingIndices(metaData); fail("no exception thrown for invalid folder name"); @@ -170,31 +151,24 @@ public void testDanglingProcessing() throws Exception { public void testDanglingIndicesNotImportedWhenTombstonePresent() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { - // Given an empty state MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService); - // When passed a dangling index final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); IndexMetaData dangledIndex = IndexMetaData.builder("test1").settings(settings).build(); metaStateService.writeIndex("test_write", dangledIndex); - // And there is a tombstone for that index final IndexGraveyard graveyard = IndexGraveyard.builder().addTombstone(dangledIndex.getIndex()).build(); final MetaData metaData = MetaData.builder().indexGraveyard(graveyard).build(); - - // Then that index is not imported assertThat(danglingState.findNewDanglingIndices(metaData).size(), equalTo(0)); } } public void testDanglingIndicesStripAliases() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { - // Given an empty state MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService); - // When passed an index that has an alias final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); IndexMetaData dangledIndex = IndexMetaData.builder("test1") .settings(settings) @@ -205,13 +179,9 @@ public void testDanglingIndicesStripAliases() throws Exception { final MetaData metaData = MetaData.builder().build(); Map newDanglingIndices = danglingState.findNewDanglingIndices(metaData); - - // Then the index is identifying as dangling assertThat(newDanglingIndices.size(), equalTo(1)); Map.Entry entry = newDanglingIndices.entrySet().iterator().next(); assertThat(entry.getKey().getName(), equalTo("test1")); - - // And the alias is removed assertThat(entry.getValue().getAliases().size(), equalTo(0)); } } @@ -220,11 +190,10 @@ public void testDanglingIndicesAreNotAllocatedWhenDisabled() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); LocalAllocateDangledIndices localAllocateDangledIndices = mock(LocalAllocateDangledIndices.class); - DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService, localAllocateDangledIndices); + DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService, localAllocateDangledIndices, false); assertTrue(danglingState.getDanglingIndices().isEmpty()); - // Given a metadata that does not enable allocation of dangling indices MetaData metaData = MetaData.builder().build(); final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); @@ -233,10 +202,8 @@ public void testDanglingIndicesAreNotAllocatedWhenDisabled() throws Exception { danglingState.findNewAndAddDanglingIndices(metaData); - // When calling the allocate method danglingState.allocateDanglingIndices(); - // Then allocation is not attempted verify(localAllocateDangledIndices, never()).allocateDangled(any(), any()); } } @@ -245,13 +212,10 @@ public void testDanglingIndicesAreAllocatedWhenEnabled() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); LocalAllocateDangledIndices localAllocateDangledIndices = mock(LocalAllocateDangledIndices.class); - DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService, localAllocateDangledIndices); + DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService, localAllocateDangledIndices, true); assertTrue(danglingState.getDanglingIndices().isEmpty()); - // Given a state where automatic allocation is enabled - danglingState.setAllocateDanglingIndicesSetting(true); - MetaData metaData = MetaData.builder().build(); final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); @@ -260,24 +224,23 @@ public void testDanglingIndicesAreAllocatedWhenEnabled() throws Exception { danglingState.findNewAndAddDanglingIndices(metaData); - // When calling the allocate method danglingState.allocateDanglingIndices(); - // Ensure that allocation is attempted verify(localAllocateDangledIndices).allocateDangled(any(), any()); } } private DanglingIndicesState createDanglingIndicesState(NodeEnvironment env, MetaStateService metaStateService) { - return createDanglingIndicesState(env, metaStateService, null); + return createDanglingIndicesState(env, metaStateService, null, true); } private DanglingIndicesState createDanglingIndicesState( NodeEnvironment env, MetaStateService metaStateService, - LocalAllocateDangledIndices indexAllocator + LocalAllocateDangledIndices indexAllocator, + boolean shouldAutoImport ) { - final Settings allocateSettings = Settings.builder().put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), false).build(); + final Settings allocateSettings = Settings.builder().put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), shouldAutoImport).build(); final ClusterService clusterServiceMock = mock(ClusterService.class); when(clusterServiceMock.getClusterSettings()).thenReturn( From d1563856ec9ce6db733c68d2dc39ab7c92fa7b09 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 27 Nov 2019 13:03:46 +0000 Subject: [PATCH 11/15] Address review feedback --- .../gateway/DanglingIndicesState.java | 17 ++++------------- .../gateway/DanglingIndicesStateTests.java | 8 ++++---- .../indices/recovery/DanglingIndicesIT.java | 14 ++++---------- .../elasticsearch/test/InternalTestCluster.java | 12 +++--------- 4 files changed, 15 insertions(+), 36 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java index 953019d622640..ee6fb23c91646 100644 --- a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java +++ b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java @@ -68,30 +68,25 @@ public class DanglingIndicesState implements ClusterStateListener { private final Map 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; - this.allocateDanglingIndices = AUTO_IMPORT_DANGLING_INDICES_SETTING.get(clusterService.getSettings()); - if (this.allocateDanglingIndices) { + boolean allocateDanglingIndices = AUTO_IMPORT_DANGLING_INDICES_SETTING.get(clusterService.getSettings()); + + if (allocateDanglingIndices) { clusterService.addListener(this); } } - public void setAllocateDanglingIndicesSetting(boolean allocateDanglingIndices) { - this.allocateDanglingIndices = allocateDanglingIndices; - } - /** * Process dangling indices based on the provided meta data, handling cleanup, finding * new dangling indices, and allocating outstanding ones. */ - public void processDanglingIndices(final MetaData metaData) { + private void processDanglingIndices(final MetaData metaData) { if (nodeEnv.hasNodeFile() == false) { return; } @@ -189,10 +184,6 @@ private IndexMetaData stripAliases(IndexMetaData indexMetaData) { * for allocation. */ void allocateDanglingIndices() { - if (this.allocateDanglingIndices == false) { - return; - } - if (danglingIndices.isEmpty()) { return; } diff --git a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java index bbb063d05d6af..78f49a47e94bd 100644 --- a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java @@ -47,10 +47,10 @@ public class DanglingIndicesStateTests extends ESTestCase { private static Settings indexSettings = Settings.builder() - .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) - .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT) - .build(); + .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) + .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT) + .build(); public void testCleanupWhenEmpty() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { diff --git a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java index 26765c67caa4d..ec099ef13e018 100644 --- a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java +++ b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java @@ -21,7 +21,6 @@ import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.index.store.Store; import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.ESIntegTestCase.ClusterScope; import org.elasticsearch.test.InternalTestCluster; @@ -67,7 +66,8 @@ public void testDanglingIndicesAreRecoveredWhenSettingIsEnabled() throws Excepti @Override public Settings onNodeStopped(String nodeName) throws Exception { - deleteIndex(INDEX_NAME); + logger.info("--> deleting test index: {}", INDEX_NAME); + assertAcked(client().admin().indices().prepareDelete(INDEX_NAME)); return super.onNodeStopped(nodeName); } }); @@ -101,7 +101,8 @@ public void testDanglingIndicesAreNotRecoveredWhenSettingIsDisabled() throws Exc @Override public Settings onNodeStopped(String nodeName) throws Exception { - deleteIndex(INDEX_NAME); + logger.info("--> deleting test index: {}", INDEX_NAME); + assertAcked(client().admin().indices().prepareDelete(INDEX_NAME)); return super.onNodeStopped(nodeName); } }); @@ -119,7 +120,6 @@ private void createAndPopulateIndex(String name, int shardCount, int replicaCoun Settings.builder() .put("number_of_shards", shardCount) .put("number_of_replicas", replicaCount) - .put(Store.INDEX_STORE_STATS_REFRESH_INTERVAL_SETTING.getKey(), 0) ) ); ensureGreen(); @@ -139,10 +139,4 @@ private void createAndPopulateIndex(String name, int shardCount, int replicaCoun client().admin().indices().prepareStats(name).execute().actionGet(); } - - private void deleteIndex(String indexName) { - logger.info("--> deleting test index: {}", indexName); - - assertAcked(client().admin().indices().prepareDelete(indexName)); - } } diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java index 488378d1621f8..87c679b11e4c2 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java @@ -1472,15 +1472,15 @@ public InetSocketAddress[] httpAddresses() { * 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. */ - public synchronized Optional stopRandomDataNode() throws IOException { + public synchronized boolean stopRandomDataNode() throws IOException { ensureOpen(); NodeAndClient nodeAndClient = getRandomNodeAndClient(DATA_NODE_PREDICATE); if (nodeAndClient != null) { logger.info("Closing random node [{}] ", nodeAndClient.name); stopNodesAndClient(nodeAndClient); - return Optional.of(nodeAndClient.name); + return true; } - return Optional.empty(); + return false; } /** @@ -1905,12 +1905,6 @@ public String startNode(Settings settings) { return startNodes(settings).get(0); } - public void startNode(String nodeName) { - final NodeAndClient nodeAndClient = nodes.get(nodeName); - assertNotNull("No client found for node name: " + nodeName, nodeAndClient); - nodeAndClient.startNode(); - } - /** * Starts multiple nodes with default settings and returns their names */ From f4f3340fa1a82e5f5b2fdcb365d537b2642d540c Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 27 Nov 2019 14:32:53 +0000 Subject: [PATCH 12/15] Address review comments --- .../gateway/DanglingIndicesState.java | 8 ++-- .../gateway/DanglingIndicesStateTests.java | 43 ++++++++++--------- .../indices/recovery/DanglingIndicesIT.java | 38 +++------------- 3 files changed, 33 insertions(+), 56 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java index ee6fb23c91646..c88d91a17ad38 100644 --- a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java +++ b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java @@ -59,7 +59,8 @@ public class DanglingIndicesState implements ClusterStateListener { public static final Setting AUTO_IMPORT_DANGLING_INDICES_SETTING = Setting.boolSetting( "gateway.auto_import_dangling_indices", true, - Setting.Property.NodeScope + Setting.Property.NodeScope, + Setting.Property.Deprecated ); private final NodeEnvironment nodeEnv; @@ -79,6 +80,8 @@ public DanglingIndicesState(NodeEnvironment nodeEnv, MetaStateService metaStateS if (allocateDanglingIndices) { clusterService.addListener(this); + } else { + logger.warn(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey() + " is disabled, dangling indices will not be detected or imported"); } } @@ -86,7 +89,7 @@ public DanglingIndicesState(NodeEnvironment nodeEnv, MetaStateService metaStateS * Process dangling indices based on the provided meta data, handling cleanup, finding * new dangling indices, and allocating outstanding ones. */ - private void processDanglingIndices(final MetaData metaData) { + public void processDanglingIndices(final MetaData metaData) { if (nodeEnv.hasNodeFile() == false) { return; } @@ -187,7 +190,6 @@ void allocateDanglingIndices() { if (danglingIndices.isEmpty()) { return; } - try { allocateDangledIndices.allocateDangled(Collections.unmodifiableCollection(new ArrayList<>(danglingIndices.values())), new ActionListener<>() { diff --git a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java index 78f49a47e94bd..7ebea1148649c 100644 --- a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java @@ -24,7 +24,6 @@ import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.service.ClusterService; -import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.NodeEnvironment; import org.elasticsearch.index.Index; @@ -52,6 +51,12 @@ public class DanglingIndicesStateTests extends ESTestCase { .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() { + return false; + } + public void testCleanupWhenEmpty() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); @@ -190,21 +195,13 @@ public void testDanglingIndicesAreNotAllocatedWhenDisabled() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); LocalAllocateDangledIndices localAllocateDangledIndices = mock(LocalAllocateDangledIndices.class); - DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService, localAllocateDangledIndices, false); - - assertTrue(danglingState.getDanglingIndices().isEmpty()); - - MetaData metaData = MetaData.builder().build(); - - final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); - IndexMetaData dangledIndex = IndexMetaData.builder("test1").settings(settings).build(); - metaStateService.writeIndex("test_write", dangledIndex); - danglingState.findNewAndAddDanglingIndices(metaData); + final ClusterService mockClusterService = buildMockClusterService(false); - danglingState.allocateDanglingIndices(); + createDanglingIndicesState(env, metaStateService, localAllocateDangledIndices, mockClusterService); - verify(localAllocateDangledIndices, never()).allocateDangled(any(), any()); + // Check that no listener was registered, because auto-import is disabled + verify(mockClusterService, never()).addListener(any()); } } @@ -212,7 +209,12 @@ public void testDanglingIndicesAreAllocatedWhenEnabled() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); LocalAllocateDangledIndices localAllocateDangledIndices = mock(LocalAllocateDangledIndices.class); - DanglingIndicesState danglingState = createDanglingIndicesState(env, metaStateService, localAllocateDangledIndices, true); + DanglingIndicesState danglingState = createDanglingIndicesState( + env, + metaStateService, + localAllocateDangledIndices, + buildMockClusterService(true) + ); assertTrue(danglingState.getDanglingIndices().isEmpty()); @@ -231,23 +233,24 @@ public void testDanglingIndicesAreAllocatedWhenEnabled() throws Exception { } private DanglingIndicesState createDanglingIndicesState(NodeEnvironment env, MetaStateService metaStateService) { - return createDanglingIndicesState(env, metaStateService, null, true); + return createDanglingIndicesState(env, metaStateService, null, buildMockClusterService(true)); } private DanglingIndicesState createDanglingIndicesState( NodeEnvironment env, MetaStateService metaStateService, LocalAllocateDangledIndices indexAllocator, - boolean shouldAutoImport + ClusterService clusterService ) { + return new DanglingIndicesState(env, metaStateService, indexAllocator, clusterService); + } + + private ClusterService buildMockClusterService(boolean shouldAutoImport) { final Settings allocateSettings = Settings.builder().put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), shouldAutoImport).build(); final ClusterService clusterServiceMock = mock(ClusterService.class); - when(clusterServiceMock.getClusterSettings()).thenReturn( - new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS) - ); when(clusterServiceMock.getSettings()).thenReturn(allocateSettings); - return new DanglingIndicesState(env, metaStateService, indexAllocator, clusterServiceMock); + return clusterServiceMock; } } diff --git a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java index ec099ef13e018..85ebb938baed3 100644 --- a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java +++ b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java @@ -19,7 +19,6 @@ package org.elasticsearch.indices.recovery; -import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.test.ESIntegTestCase; import org.elasticsearch.test.ESIntegTestCase.ClusterScope; @@ -28,16 +27,11 @@ import static org.elasticsearch.cluster.metadata.IndexGraveyard.SETTING_MAX_TOMBSTONES; import static org.elasticsearch.gateway.DanglingIndicesState.AUTO_IMPORT_DANGLING_INDICES_SETTING; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; -import static org.hamcrest.Matchers.equalTo; @ClusterScope(numDataNodes = 0, scope = ESIntegTestCase.Scope.TEST) public class DanglingIndicesIT extends ESIntegTestCase { private static final String INDEX_NAME = "test-idx-1"; - private static final int MIN_DOC_COUNT = 500; - private static final int SHARD_COUNT = 1; - private static final int REPLICA_COUNT = 2; - private Settings buildSettings(boolean importDanglingIndices) { return Settings.builder() // Don't keep any indices in the graveyard, so that when we delete an index, @@ -57,7 +51,7 @@ public void testDanglingIndicesAreRecoveredWhenSettingIsEnabled() throws Excepti internalCluster().startNodes(3, settings); // Create an index and distribute it across the 3 nodes - createAndPopulateIndex(INDEX_NAME, SHARD_COUNT, REPLICA_COUNT); + createAndPopulateIndex(INDEX_NAME, 1, 2); ensureGreen(); // This is so that when then node comes back up, we have a dangling index that can be recovered. @@ -86,11 +80,11 @@ public void testDanglingIndicesAreNotRecoveredWhenSettingIsDisabled() throws Exc internalCluster().startNodes(3, buildSettings(false)); // Create an index and distribute it across the 3 nodes - createAndPopulateIndex(INDEX_NAME, SHARD_COUNT, REPLICA_COUNT); + createAndPopulateIndex(INDEX_NAME, 1, 2); // Create another index so that once we drop the first index, we // can still assert that the cluster is green. - createAndPopulateIndex(INDEX_NAME + "-other", SHARD_COUNT, REPLICA_COUNT); + createAndPopulateIndex(INDEX_NAME + "-other", 1, 2); ensureGreen(); @@ -112,31 +106,9 @@ public Settings onNodeStopped(String nodeName) throws Exception { assertFalse("Did not expect dangling index " + INDEX_NAME + " to be recovered", indexExists(INDEX_NAME)); } - private void createAndPopulateIndex(String name, int shardCount, int replicaCount) throws InterruptedException { + private void createAndPopulateIndex(String name, int shardCount, int replicaCount) { logger.info("--> creating test index: {}", name); - assertAcked( - prepareCreate( - name, - Settings.builder() - .put("number_of_shards", shardCount) - .put("number_of_replicas", replicaCount) - ) - ); + assertAcked(prepareCreate(name, Settings.builder().put("number_of_shards", shardCount).put("number_of_replicas", replicaCount))); ensureGreen(); - - logger.info("--> indexing sample data"); - final int numDocs = between(MIN_DOC_COUNT, 1000); - final IndexRequestBuilder[] docs = new IndexRequestBuilder[numDocs]; - - for (int i = 0; i < numDocs; i++) { - docs[i] = client().prepareIndex(name) - .setSource("foo-int", randomInt(), "foo-string", randomAlphaOfLength(32), "foo-float", randomFloat()); - } - - indexRandom(true, docs); - flush(); - assertThat(client().prepareSearch(name).setSize(0).get().getHits().getTotalHits().value, equalTo((long) numDocs)); - - client().admin().indices().prepareStats(name).execute().actionGet(); } } From a9dfc4810d04e176351d4b2e0d8bfc05b97ee4d8 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 27 Nov 2019 16:41:00 +0000 Subject: [PATCH 13/15] Checkstyle --- .../main/java/org/elasticsearch/test/InternalTestCluster.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java index 17117b32399e4..d1c2b18368d48 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java @@ -121,7 +121,6 @@ import java.util.Map; import java.util.NavigableMap; import java.util.Objects; -import java.util.Optional; import java.util.Random; import java.util.Set; import java.util.TreeMap; @@ -155,7 +154,6 @@ import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; From 85f7f574879c34d963c67dd186846ce75a4ecc13 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 28 Nov 2019 12:01:10 +0000 Subject: [PATCH 14/15] Address review comments --- .../gateway/DanglingIndicesState.java | 9 +++- .../gateway/DanglingIndicesStateTests.java | 52 +++++++++---------- .../indices/recovery/DanglingIndicesIT.java | 45 +++++----------- .../test/InternalTestCluster.java | 3 +- 4 files changed, 45 insertions(+), 64 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java index c88d91a17ad38..15a9570db807e 100644 --- a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java +++ b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java @@ -66,6 +66,7 @@ public class DanglingIndicesState implements ClusterStateListener { private final NodeEnvironment nodeEnv; private final MetaStateService metaStateService; private final LocalAllocateDangledIndices allocateDangledIndices; + private final boolean isAutoImportDanglingIndicesEnabled; private final Map danglingIndices = ConcurrentCollections.newConcurrentMap(); @@ -76,15 +77,19 @@ public DanglingIndicesState(NodeEnvironment nodeEnv, MetaStateService metaStateS this.metaStateService = metaStateService; this.allocateDangledIndices = allocateDangledIndices; - boolean allocateDanglingIndices = AUTO_IMPORT_DANGLING_INDICES_SETTING.get(clusterService.getSettings()); + this.isAutoImportDanglingIndicesEnabled = AUTO_IMPORT_DANGLING_INDICES_SETTING.get(clusterService.getSettings()); - if (allocateDanglingIndices) { + if (this.isAutoImportDanglingIndicesEnabled) { clusterService.addListener(this); } else { logger.warn(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey() + " is disabled, dangling indices will not be detected or imported"); } } + public boolean isAutoImportDanglingIndicesEnabled() { + return this.isAutoImportDanglingIndicesEnabled; + } + /** * Process dangling indices based on the provided meta data, handling cleanup, finding * new dangling indices, and allocating outstanding ones. diff --git a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java index 7ebea1148649c..42c546e192812 100644 --- a/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java +++ b/server/src/test/java/org/elasticsearch/gateway/DanglingIndicesStateTests.java @@ -39,7 +39,6 @@ import static org.hamcrest.Matchers.equalTo; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -53,6 +52,7 @@ public class DanglingIndicesStateTests extends ESTestCase { // The setting AUTO_IMPORT_DANGLING_INDICES_SETTING is deprecated, so we must disable // warning checks or all the tests will fail. + @Override protected boolean enableWarningsCheck() { return false; } @@ -196,12 +196,19 @@ public void testDanglingIndicesAreNotAllocatedWhenDisabled() throws Exception { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); LocalAllocateDangledIndices localAllocateDangledIndices = mock(LocalAllocateDangledIndices.class); - final ClusterService mockClusterService = buildMockClusterService(false); + final Settings allocateSettings = Settings.builder().put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), false).build(); - createDanglingIndicesState(env, metaStateService, localAllocateDangledIndices, mockClusterService); + final ClusterService clusterServiceMock = mock(ClusterService.class); + when(clusterServiceMock.getSettings()).thenReturn(allocateSettings); - // Check that no listener was registered, because auto-import is disabled - verify(mockClusterService, never()).addListener(any()); + final DanglingIndicesState danglingIndicesState = new DanglingIndicesState( + env, + metaStateService, + localAllocateDangledIndices, + clusterServiceMock + ); + + assertFalse("Expected dangling imports to be disabled", danglingIndicesState.isAutoImportDanglingIndicesEnabled()); } } @@ -209,48 +216,37 @@ public void testDanglingIndicesAreAllocatedWhenEnabled() throws Exception { try (NodeEnvironment env = newNodeEnvironment()) { MetaStateService metaStateService = new MetaStateService(env, xContentRegistry()); LocalAllocateDangledIndices localAllocateDangledIndices = mock(LocalAllocateDangledIndices.class); - DanglingIndicesState danglingState = createDanglingIndicesState( + final Settings allocateSettings = Settings.builder().put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), true).build(); + + final ClusterService clusterServiceMock = mock(ClusterService.class); + when(clusterServiceMock.getSettings()).thenReturn(allocateSettings); + + DanglingIndicesState danglingIndicesState = new DanglingIndicesState( env, metaStateService, - localAllocateDangledIndices, - buildMockClusterService(true) + localAllocateDangledIndices, clusterServiceMock ); - assertTrue(danglingState.getDanglingIndices().isEmpty()); - - MetaData metaData = MetaData.builder().build(); + assertTrue("Expected dangling imports to be enabled", danglingIndicesState.isAutoImportDanglingIndicesEnabled()); final Settings.Builder settings = Settings.builder().put(indexSettings).put(IndexMetaData.SETTING_INDEX_UUID, "test1UUID"); IndexMetaData dangledIndex = IndexMetaData.builder("test1").settings(settings).build(); metaStateService.writeIndex("test_write", dangledIndex); - danglingState.findNewAndAddDanglingIndices(metaData); + danglingIndicesState.findNewAndAddDanglingIndices(MetaData.builder().build()); - danglingState.allocateDanglingIndices(); + danglingIndicesState.allocateDanglingIndices(); verify(localAllocateDangledIndices).allocateDangled(any(), any()); } } private DanglingIndicesState createDanglingIndicesState(NodeEnvironment env, MetaStateService metaStateService) { - return createDanglingIndicesState(env, metaStateService, null, buildMockClusterService(true)); - } - - private DanglingIndicesState createDanglingIndicesState( - NodeEnvironment env, - MetaStateService metaStateService, - LocalAllocateDangledIndices indexAllocator, - ClusterService clusterService - ) { - return new DanglingIndicesState(env, metaStateService, indexAllocator, clusterService); - } - - private ClusterService buildMockClusterService(boolean shouldAutoImport) { - final Settings allocateSettings = Settings.builder().put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), shouldAutoImport).build(); + final Settings allocateSettings = Settings.builder().put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), true).build(); final ClusterService clusterServiceMock = mock(ClusterService.class); when(clusterServiceMock.getSettings()).thenReturn(allocateSettings); - return clusterServiceMock; + return new DanglingIndicesState(env, metaStateService, null, clusterServiceMock); } } diff --git a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java index 85ebb938baed3..eaceb690f4e78 100644 --- a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java +++ b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java @@ -24,6 +24,8 @@ import org.elasticsearch.test.ESIntegTestCase.ClusterScope; import org.elasticsearch.test.InternalTestCluster; +import java.util.concurrent.TimeUnit; + import static org.elasticsearch.cluster.metadata.IndexGraveyard.SETTING_MAX_TOMBSTONES; import static org.elasticsearch.gateway.DanglingIndicesState.AUTO_IMPORT_DANGLING_INDICES_SETTING; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; @@ -46,29 +48,22 @@ private Settings buildSettings(boolean importDanglingIndices) { * the cluster, so long as the recovery setting is enabled. */ public void testDanglingIndicesAreRecoveredWhenSettingIsEnabled() throws Exception { - logger.info("--> starting cluster"); final Settings settings = buildSettings(true); internalCluster().startNodes(3, settings); - // Create an index and distribute it across the 3 nodes - createAndPopulateIndex(INDEX_NAME, 1, 2); - ensureGreen(); + createIndex(INDEX_NAME, Settings.builder().put("number_of_shards", 1).put("number_of_replicas", 2).build()); - // This is so that when then node comes back up, we have a dangling index that can be recovered. - logger.info("--> restarted a random node and deleting the index while it's down"); + // Restart node, deleting the index in its absence, so that there is a dangling index to recover internalCluster().restartRandomDataNode(new InternalTestCluster.RestartCallback() { @Override public Settings onNodeStopped(String nodeName) throws Exception { - logger.info("--> deleting test index: {}", INDEX_NAME); assertAcked(client().admin().indices().prepareDelete(INDEX_NAME)); return super.onNodeStopped(nodeName); } }); - ensureGreen(); - - assertTrue("Expected dangling index " + INDEX_NAME + " to be recovered", indexExists(INDEX_NAME)); + assertBusy(() -> assertTrue("Expected dangling index " + INDEX_NAME + " to be recovered", indexExists(INDEX_NAME))); } /** @@ -76,39 +71,25 @@ public Settings onNodeStopped(String nodeName) throws Exception { * the cluster when the recovery setting is disabled. */ public void testDanglingIndicesAreNotRecoveredWhenSettingIsDisabled() throws Exception { - logger.info("--> starting cluster"); internalCluster().startNodes(3, buildSettings(false)); - // Create an index and distribute it across the 3 nodes - createAndPopulateIndex(INDEX_NAME, 1, 2); - - // Create another index so that once we drop the first index, we - // can still assert that the cluster is green. - createAndPopulateIndex(INDEX_NAME + "-other", 1, 2); - - ensureGreen(); + createIndex(INDEX_NAME, Settings.builder().put("number_of_shards", 1).put("number_of_replicas", 2).build()); - // 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"); + // Restart node, deleting the index in its absence, so that there is a dangling index to recover internalCluster().restartRandomDataNode(new InternalTestCluster.RestartCallback() { @Override public Settings onNodeStopped(String nodeName) throws Exception { - logger.info("--> deleting test index: {}", INDEX_NAME); assertAcked(client().admin().indices().prepareDelete(INDEX_NAME)); return super.onNodeStopped(nodeName); } }); - ensureGreen(); - - assertFalse("Did not expect dangling index " + INDEX_NAME + " to be recovered", indexExists(INDEX_NAME)); - } - - private void createAndPopulateIndex(String name, int shardCount, int replicaCount) { - logger.info("--> creating test index: {}", name); - assertAcked(prepareCreate(name, Settings.builder().put("number_of_shards", shardCount).put("number_of_replicas", replicaCount))); - ensureGreen(); + // Since index recovery is async, we can't prove index recovery will never occur, just that it doesn't occur within some reasonable + // amount of time + assertFalse( + "Did not expect dangling index " + INDEX_NAME + " to be recovered", + waitUntil(() -> indexExists(INDEX_NAME), 5, TimeUnit.SECONDS) + ); } } diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java index d1c2b18368d48..9b609040379eb 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java @@ -1473,8 +1473,7 @@ public InetSocketAddress[] httpAddresses() { } /** - * 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. + * Stops a random data node in the cluster. Returns true if a node was found to stop, false otherwise. */ public synchronized boolean stopRandomDataNode() throws IOException { ensureOpen(); From 2a40971c88b84bcc8d498901d513743a15b726b0 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 29 Nov 2019 09:39:58 +0000 Subject: [PATCH 15/15] Address review feedback --- .../org/elasticsearch/gateway/DanglingIndicesState.java | 2 +- .../elasticsearch/indices/recovery/DanglingIndicesIT.java | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java index 15a9570db807e..701a85fd90e50 100644 --- a/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java +++ b/server/src/main/java/org/elasticsearch/gateway/DanglingIndicesState.java @@ -86,7 +86,7 @@ public DanglingIndicesState(NodeEnvironment nodeEnv, MetaStateService metaStateS } } - public boolean isAutoImportDanglingIndicesEnabled() { + boolean isAutoImportDanglingIndicesEnabled() { return this.isAutoImportDanglingIndicesEnabled; } diff --git a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java index eaceb690f4e78..31afef73ad450 100644 --- a/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java +++ b/server/src/test/java/org/elasticsearch/indices/recovery/DanglingIndicesIT.java @@ -51,7 +51,7 @@ public void testDanglingIndicesAreRecoveredWhenSettingIsEnabled() throws Excepti 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()); + createIndex(INDEX_NAME, Settings.builder().put("number_of_replicas", 2).build()); // Restart node, deleting the index in its absence, so that there is a dangling index to recover internalCluster().restartRandomDataNode(new InternalTestCluster.RestartCallback() { @@ -73,7 +73,7 @@ public Settings onNodeStopped(String nodeName) throws Exception { public void testDanglingIndicesAreNotRecoveredWhenSettingIsDisabled() throws Exception { internalCluster().startNodes(3, buildSettings(false)); - createIndex(INDEX_NAME, Settings.builder().put("number_of_shards", 1).put("number_of_replicas", 2).build()); + createIndex(INDEX_NAME, Settings.builder().put("number_of_replicas", 2).build()); // Restart node, deleting the index in its absence, so that there is a dangling index to recover internalCluster().restartRandomDataNode(new InternalTestCluster.RestartCallback() { @@ -89,7 +89,7 @@ public Settings onNodeStopped(String nodeName) throws Exception { // amount of time assertFalse( "Did not expect dangling index " + INDEX_NAME + " to be recovered", - waitUntil(() -> indexExists(INDEX_NAME), 5, TimeUnit.SECONDS) + waitUntil(() -> indexExists(INDEX_NAME), 1, TimeUnit.SECONDS) ); } }