From 682f0d7b2221b17b33bf1956c24147f8cf2d50e7 Mon Sep 17 00:00:00 2001 From: Andrei Dan Date: Thu, 19 Dec 2019 17:24:20 +0000 Subject: [PATCH 1/5] ILM: Make the rollover step retryable. This commit makes the rollover more resilient, by having it execute only one cluster state update that creates the new (rollover index), rolls over the alias from the source to the target index and set the RolloverInfo on the source index. Before these 3 steps were represented as 3 chained cluster state updates, which would've seen the user manually intervene if, say, the alias rollover cluster state update (second in the chain) failed but the creation of the rollover index (first in the chain) update succeeded --- .../rollover/TransportRolloverAction.java | 109 ++++++++---------- .../metadata/MetaDataCreateIndexService.java | 2 +- .../metadata/MetaDataIndexAliasesService.java | 5 +- .../TransportRolloverActionTests.java | 13 +-- .../xpack/core/ilm/RolloverStep.java | 5 + .../ilm/TimeSeriesLifecycleActionsIT.java | 33 ++++++ 6 files changed, 92 insertions(+), 75 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java index bc67685a976f7..08baa8405f0be 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java @@ -20,7 +20,6 @@ package org.elasticsearch.action.admin.indices.rollover; import org.elasticsearch.action.ActionListener; -import org.elasticsearch.action.admin.indices.alias.IndicesAliasesClusterStateUpdateRequest; import org.elasticsearch.action.admin.indices.create.CreateIndexClusterStateUpdateRequest; import org.elasticsearch.action.admin.indices.create.CreateIndexRequest; import org.elasticsearch.action.admin.indices.stats.IndicesStatsAction; @@ -130,7 +129,7 @@ protected void masterOperation(Task task, final RolloverRequest rolloverRequest, .docs(true); statsRequest.setParentTask(clusterService.localNode().getId(), task.getId()); client.execute(IndicesStatsAction.INSTANCE, statsRequest, - new ActionListener() { + new ActionListener<>() { @Override public void onResponse(IndicesStatsResponse statsResponse) { final Map conditionResults = evaluateConditions(rolloverRequest.getConditions().values(), @@ -141,56 +140,41 @@ public void onResponse(IndicesStatsResponse statsResponse) { new RolloverResponse(sourceIndexName, rolloverIndexName, conditionResults, true, false, false, false)); return; } - List> metConditions = rolloverRequest.getConditions().values().stream() + List> metConditions = rolloverRequest.getConditions().values().stream() .filter(condition -> conditionResults.get(condition.toString())).collect(Collectors.toList()); if (conditionResults.size() == 0 || metConditions.size() > 0) { - CreateIndexClusterStateUpdateRequest updateRequest = prepareCreateIndexRequest(unresolvedName, rolloverIndexName, - rolloverRequest); - createIndexService.createIndex(updateRequest, ActionListener.wrap(createIndexClusterStateUpdateResponse -> { - final IndicesAliasesClusterStateUpdateRequest aliasesUpdateRequest; - if (explicitWriteIndex) { - aliasesUpdateRequest = prepareRolloverAliasesWriteIndexUpdateRequest(sourceIndexName, - rolloverIndexName, rolloverRequest); - } else { - aliasesUpdateRequest = prepareRolloverAliasesUpdateRequest(sourceIndexName, - rolloverIndexName, rolloverRequest); + CreateIndexClusterStateUpdateRequest createIndexRequest = prepareCreateIndexRequest(unresolvedName, + rolloverIndexName, rolloverRequest); + clusterService.submitStateUpdateTask("rollover_index", new ClusterStateUpdateTask() { + @Override + public ClusterState execute(ClusterState currentState) throws Exception { + ClusterState newState = createIndexService.applyCreateIndexRequest(currentState, createIndexRequest); + newState = indexAliasesService.innerExecute(newState, + rolloverAliasToNewIndex(sourceIndexName, rolloverIndexName, rolloverRequest, explicitWriteIndex)); + RolloverInfo rolloverInfo = new RolloverInfo(rolloverRequest.getAlias(), metConditions, + threadPool.absoluteTimeInMillis()); + return ClusterState.builder(newState) + .metaData(MetaData.builder(newState.metaData()) + .put(IndexMetaData.builder(newState.metaData().index(sourceIndexName)) + .putRolloverInfo(rolloverInfo))).build(); } - indexAliasesService.indicesAliases(aliasesUpdateRequest, - ActionListener.wrap(aliasClusterStateUpdateResponse -> { - if (aliasClusterStateUpdateResponse.isAcknowledged()) { - clusterService.submitStateUpdateTask("update_rollover_info", new ClusterStateUpdateTask() { - @Override - public ClusterState execute(ClusterState currentState) { - RolloverInfo rolloverInfo = new RolloverInfo(rolloverRequest.getAlias(), metConditions, - threadPool.absoluteTimeInMillis()); - return ClusterState.builder(currentState) - .metaData(MetaData.builder(currentState.metaData()) - .put(IndexMetaData.builder(currentState.metaData().index(sourceIndexName)) - .putRolloverInfo(rolloverInfo))).build(); - } - @Override - public void onFailure(String source, Exception e) { - listener.onFailure(e); - } + @Override + public void onFailure(String source, Exception e) { + listener.onFailure(e); + } - @Override - public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { - activeShardsObserver.waitForActiveShards(new String[]{rolloverIndexName}, - rolloverRequest.getCreateIndexRequest().waitForActiveShards(), - rolloverRequest.masterNodeTimeout(), - isShardsAcknowledged -> listener.onResponse(new RolloverResponse( - sourceIndexName, rolloverIndexName, conditionResults, false, true, true, - isShardsAcknowledged)), - listener::onFailure); - } - }); - } else { - listener.onResponse(new RolloverResponse(sourceIndexName, rolloverIndexName, conditionResults, - false, true, false, false)); - } - }, listener::onFailure)); - }, listener::onFailure)); + @Override + public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { + activeShardsObserver.waitForActiveShards(new String[]{rolloverIndexName}, + rolloverRequest.getCreateIndexRequest().waitForActiveShards(), + rolloverRequest.masterNodeTimeout(), + isShardsAcknowledged -> listener.onResponse(new RolloverResponse( + sourceIndexName, rolloverIndexName, conditionResults, false, true, true, + isShardsAcknowledged)), + listener::onFailure); + } + }); } else { // conditions not met listener.onResponse( @@ -207,27 +191,24 @@ public void onFailure(Exception e) { ); } - static IndicesAliasesClusterStateUpdateRequest prepareRolloverAliasesUpdateRequest(String oldIndex, String newIndex, - RolloverRequest request) { - final List actions = List.of( - new AliasAction.Add(newIndex, request.getAlias(), null, null, null, null), - new AliasAction.Remove(oldIndex, request.getAlias())); - return new IndicesAliasesClusterStateUpdateRequest(actions) - .ackTimeout(request.ackTimeout()) - .masterNodeTimeout(request.masterNodeTimeout()); - } - - static IndicesAliasesClusterStateUpdateRequest prepareRolloverAliasesWriteIndexUpdateRequest(String oldIndex, String newIndex, - RolloverRequest request) { - final List actions = List.of( + /** + * Creates the alias actions to reflect the alias rollover from the old (source) index to the new (target/rolled over) index. An + * alias pointing to multiple indices will have to be an explicit write index (ie. the old index alias has is_write_index set to true) + * in which case, after the rollover, the new index will need to be the explicit write index. + */ + static List rolloverAliasToNewIndex(String oldIndex, String newIndex, RolloverRequest request, + boolean explicitWriteIndex) { + if (explicitWriteIndex) { + return List.of( new AliasAction.Add(newIndex, request.getAlias(), null, null, null, true), new AliasAction.Add(oldIndex, request.getAlias(), null, null, null, false)); - return new IndicesAliasesClusterStateUpdateRequest(actions) - .ackTimeout(request.ackTimeout()) - .masterNodeTimeout(request.masterNodeTimeout()); + } else { + return List.of( + new AliasAction.Add(newIndex, request.getAlias(), null, null, null, null), + new AliasAction.Remove(oldIndex, request.getAlias())); + } } - static String generateRolloverIndexName(String sourceIndexName, IndexNameExpressionResolver indexNameExpressionResolver) { String resolvedName = indexNameExpressionResolver.resolveDateMathExpression(sourceIndexName); final boolean isDateMath = sourceIndexName.equals(resolvedName) == false; diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java index 6a977fdaee368..9e310280636a0 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java @@ -262,7 +262,7 @@ public void onFailure(String source, Exception e) { * Handles the cluster state transition to a version that reflects the {@link CreateIndexClusterStateUpdateRequest}. * All the requested changes are firstly validated before mutating the {@link ClusterState}. */ - ClusterState applyCreateIndexRequest(ClusterState currentState, CreateIndexClusterStateUpdateRequest request) throws Exception { + public ClusterState applyCreateIndexRequest(ClusterState currentState, CreateIndexClusterStateUpdateRequest request) throws Exception { logger.trace("executing IndexCreationTask for [{}] against cluster state version [{}]", request, currentState.version()); Index createdIndex = null; String removalExtraInfo = null; diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesService.java index 84e2f512e569f..a244bb001a62c 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesService.java @@ -90,7 +90,10 @@ public ClusterState execute(ClusterState currentState) { }); } - ClusterState innerExecute(ClusterState currentState, Iterable actions) { + /** + * Handles the cluster state transition to a version that reflects the provided {@link AliasAction}s. + */ + public ClusterState innerExecute(ClusterState currentState, Iterable actions) { List indicesToClose = new ArrayList<>(); Map indices = new HashMap<>(); try { diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverActionTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverActionTests.java index 9dac4a38a36b6..54454cd9f7ecb 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverActionTests.java @@ -22,7 +22,6 @@ import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.ActionRequest; -import org.elasticsearch.action.admin.indices.alias.IndicesAliasesClusterStateUpdateRequest; import org.elasticsearch.action.admin.indices.create.CreateIndexClusterStateUpdateRequest; import org.elasticsearch.action.admin.indices.stats.CommonStats; import org.elasticsearch.action.admin.indices.stats.IndexStats; @@ -219,15 +218,13 @@ public void testEvaluateWithoutMetaData() { results2.forEach((k, v) -> assertFalse(v)); } - public void testCreateUpdateAliasRequest() { + public void testRolloverAliasActions() { String sourceAlias = randomAlphaOfLength(10); String sourceIndex = randomAlphaOfLength(10); String targetIndex = randomAlphaOfLength(10); final RolloverRequest rolloverRequest = new RolloverRequest(sourceAlias, targetIndex); - final IndicesAliasesClusterStateUpdateRequest updateRequest = - TransportRolloverAction.prepareRolloverAliasesUpdateRequest(sourceIndex, targetIndex, rolloverRequest); - List actions = updateRequest.actions(); + List actions = TransportRolloverAction.rolloverAliasToNewIndex(sourceIndex, targetIndex, rolloverRequest, false); assertThat(actions, hasSize(2)); boolean foundAdd = false; boolean foundRemove = false; @@ -246,15 +243,13 @@ public void testCreateUpdateAliasRequest() { assertTrue(foundRemove); } - public void testCreateUpdateAliasRequestWithExplicitWriteIndex() { + public void testRolloverAliasActionsWithExplicitWriteIndex() { String sourceAlias = randomAlphaOfLength(10); String sourceIndex = randomAlphaOfLength(10); String targetIndex = randomAlphaOfLength(10); final RolloverRequest rolloverRequest = new RolloverRequest(sourceAlias, targetIndex); - final IndicesAliasesClusterStateUpdateRequest updateRequest = - TransportRolloverAction.prepareRolloverAliasesWriteIndexUpdateRequest(sourceIndex, targetIndex, rolloverRequest); + List actions = TransportRolloverAction.rolloverAliasToNewIndex(sourceIndex, targetIndex, rolloverRequest, true); - List actions = updateRequest.actions(); assertThat(actions, hasSize(2)); boolean foundAddWrite = false; boolean foundRemoveWrite = false; diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/RolloverStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/RolloverStep.java index 90b9d15f21b85..542ac156a9871 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/RolloverStep.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/RolloverStep.java @@ -30,6 +30,11 @@ public RolloverStep(StepKey key, StepKey nextStepKey, Client client) { super(key, nextStepKey, client); } + @Override + public boolean isRetryable() { + return true; + } + @Override public void performAction(IndexMetaData indexMetaData, ClusterState currentClusterState, ClusterStateObserver observer, Listener listener) { diff --git a/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java b/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java index afb5bb4b66dba..bbefef7e9bd7c 100644 --- a/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java +++ b/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java @@ -1004,6 +1004,39 @@ public void testILMRolloverOnManuallyRolledIndex() throws Exception { assertBusy(() -> assertTrue(indexExists(thirdIndex))); } + public void testILMRolloverRetriesIfRolloverIndexAlreadyExistsUntilIndexIsDeleted() throws Exception { + String firstIndex = index + "-000001"; + String secondIndex = index + "-000002"; + + createNewSingletonPolicy("hot", new RolloverAction(null, TimeValue.timeValueSeconds(1), null)); + + // create the second index so the rollover of the first index fails + createIndexWithSettings( + secondIndex, + Settings.builder().put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) + .put(RolloverAction.LIFECYCLE_ROLLOVER_ALIAS, "alias"), + false + ); + + createIndexWithSettings( + firstIndex, + Settings.builder().put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) + .put(LifecycleSettings.LIFECYCLE_NAME, policy) + .put(RolloverAction.LIFECYCLE_ROLLOVER_ALIAS, "alias"), + true + ); + + // wait for ILM to start retrying the step + assertBusy(() -> assertThat((Integer) explainIndex(firstIndex).get(FAILED_STEP_RETRY_COUNT_FIELD), greaterThanOrEqualTo(1))); + + deleteIndex(secondIndex); + + // the rollover step should now succeed + assertBusy(() -> assertThat(getStepKeyForIndex(firstIndex), equalTo(TerminalPolicyStep.KEY))); + } + public void testHistoryIsWrittenWithSuccess() throws Exception { String index = "index"; From 156b5560572d2b288097dcd6afb0169498e98dab Mon Sep 17 00:00:00 2001 From: Andrei Dan Date: Thu, 19 Dec 2019 17:25:45 +0000 Subject: [PATCH 2/5] Rename innerExecute to applyAliasActions --- .../rollover/TransportRolloverAction.java | 2 +- .../metadata/MetaDataIndexAliasesService.java | 4 +- .../MetaDataIndexAliasesServiceTests.java | 46 +++++++++---------- 3 files changed, 26 insertions(+), 26 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java index 08baa8405f0be..1c50626d8457a 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java @@ -149,7 +149,7 @@ public void onResponse(IndicesStatsResponse statsResponse) { @Override public ClusterState execute(ClusterState currentState) throws Exception { ClusterState newState = createIndexService.applyCreateIndexRequest(currentState, createIndexRequest); - newState = indexAliasesService.innerExecute(newState, + newState = indexAliasesService.applyAliasActions(newState, rolloverAliasToNewIndex(sourceIndexName, rolloverIndexName, rolloverRequest, explicitWriteIndex)); RolloverInfo rolloverInfo = new RolloverInfo(rolloverRequest.getAlias(), metConditions, threadPool.absoluteTimeInMillis()); diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesService.java index a244bb001a62c..5efd4b6eae8bc 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesService.java @@ -85,7 +85,7 @@ protected ClusterStateUpdateResponse newResponse(boolean acknowledged) { @Override public ClusterState execute(ClusterState currentState) { - return innerExecute(currentState, request.actions()); + return applyAliasActions(currentState, request.actions()); } }); } @@ -93,7 +93,7 @@ public ClusterState execute(ClusterState currentState) { /** * Handles the cluster state transition to a version that reflects the provided {@link AliasAction}s. */ - public ClusterState innerExecute(ClusterState currentState, Iterable actions) { + public ClusterState applyAliasActions(ClusterState currentState, Iterable actions) { List indicesToClose = new ArrayList<>(); Map indices = new HashMap<>(); try { diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesServiceTests.java index 7f7f26bcfbcbb..9b320ae9f27e7 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataIndexAliasesServiceTests.java @@ -72,7 +72,7 @@ public void testAddAndRemove() { ClusterState before = createIndex(ClusterState.builder(ClusterName.DEFAULT).build(), index); // Add an alias to it - ClusterState after = service.innerExecute(before, singletonList(new AliasAction.Add(index, "test", null, null, null, null))); + ClusterState after = service.applyAliasActions(before, singletonList(new AliasAction.Add(index, "test", null, null, null, null))); AliasOrIndex alias = after.metaData().getAliasAndIndexLookup().get("test"); assertNotNull(alias); assertTrue(alias.isAlias()); @@ -81,7 +81,7 @@ public void testAddAndRemove() { // Remove the alias from it while adding another one before = after; - after = service.innerExecute(before, Arrays.asList( + after = service.applyAliasActions(before, Arrays.asList( new AliasAction.Remove(index, "test"), new AliasAction.Add(index, "test_2", null, null, null, null))); assertNull(after.metaData().getAliasAndIndexLookup().get("test")); @@ -93,7 +93,7 @@ public void testAddAndRemove() { // Now just remove on its own before = after; - after = service.innerExecute(before, singletonList(new AliasAction.Remove(index, "test_2"))); + after = service.applyAliasActions(before, singletonList(new AliasAction.Remove(index, "test_2"))); assertNull(after.metaData().getAliasAndIndexLookup().get("test")); assertNull(after.metaData().getAliasAndIndexLookup().get("test_2")); assertAliasesVersionIncreased(index, before, after); @@ -109,7 +109,7 @@ public void testMultipleIndices() { before = createIndex(before, index); addActions.add(new AliasAction.Add(index, "alias-" + index, null, null, null, null)); } - final ClusterState afterAddingAliasesToAll = service.innerExecute(before, addActions); + final ClusterState afterAddingAliasesToAll = service.applyAliasActions(before, addActions); assertAliasesVersionIncreased(indices.toArray(new String[0]), before, afterAddingAliasesToAll); // now add some aliases randomly @@ -121,7 +121,7 @@ public void testMultipleIndices() { randomIndices.add(index); } } - final ClusterState afterAddingRandomAliases = service.innerExecute(afterAddingAliasesToAll, randomAddActions); + final ClusterState afterAddingRandomAliases = service.applyAliasActions(afterAddingAliasesToAll, randomAddActions); assertAliasesVersionIncreased(randomIndices.toArray(new String[0]), afterAddingAliasesToAll, afterAddingRandomAliases); assertAliasesVersionUnchanged( Sets.difference(indices, randomIndices).toArray(new String[0]), @@ -134,15 +134,15 @@ public void testChangingWriteAliasStateIncreasesAliasesVersion() { final ClusterState before = createIndex(ClusterState.builder(ClusterName.DEFAULT).build(), index); final ClusterState afterAddWriteAlias = - service.innerExecute(before, singletonList(new AliasAction.Add(index, "test", null, null, null, true))); + service.applyAliasActions(before, singletonList(new AliasAction.Add(index, "test", null, null, null, true))); assertAliasesVersionIncreased(index, before, afterAddWriteAlias); final ClusterState afterChangeWriteAliasToNonWriteAlias = - service.innerExecute(afterAddWriteAlias, singletonList(new AliasAction.Add(index, "test", null, null, null, false))); + service.applyAliasActions(afterAddWriteAlias, singletonList(new AliasAction.Add(index, "test", null, null, null, false))); assertAliasesVersionIncreased(index, afterAddWriteAlias, afterChangeWriteAliasToNonWriteAlias); final ClusterState afterChangeNonWriteAliasToWriteAlias = - service.innerExecute( + service.applyAliasActions( afterChangeWriteAliasToNonWriteAlias, singletonList(new AliasAction.Add(index, "test", null, null, null, true))); assertAliasesVersionIncreased(index, afterChangeWriteAliasToNonWriteAlias, afterChangeNonWriteAliasToWriteAlias); @@ -158,7 +158,7 @@ public void testAddingAliasMoreThanOnceShouldOnlyIncreaseAliasesVersionByOne() { for (int i = 0; i < length; i++) { addActions.add(new AliasAction.Add(index, "test", null, null, null, null)); } - final ClusterState afterAddingAliases = service.innerExecute(before, addActions); + final ClusterState afterAddingAliases = service.applyAliasActions(before, addActions); assertAliasesVersionIncreased(index, before, afterAddingAliases); } @@ -175,7 +175,7 @@ public void testAliasesVersionUnchangedWhenActionsAreIdempotent() { final String aliasName = randomValueOtherThanMany(v -> aliasNames.add(v) == false, () -> randomAlphaOfLength(8)); addActions.add(new AliasAction.Add(index, aliasName, null, null, null, null)); } - final ClusterState afterAddingAlias = service.innerExecute(before, addActions); + final ClusterState afterAddingAlias = service.applyAliasActions(before, addActions); // now perform a remove and add for each alias which is idempotent, the resulting aliases are unchanged final var removeAndAddActions = new ArrayList(2 * length); @@ -183,7 +183,7 @@ public void testAliasesVersionUnchangedWhenActionsAreIdempotent() { removeAndAddActions.add(new AliasAction.Remove(index, aliasName)); removeAndAddActions.add(new AliasAction.Add(index, aliasName, null, null, null, null)); } - final ClusterState afterRemoveAndAddAlias = service.innerExecute(afterAddingAlias, removeAndAddActions); + final ClusterState afterRemoveAndAddAlias = service.applyAliasActions(afterAddingAlias, removeAndAddActions); assertAliasesVersionUnchanged(index, afterAddingAlias, afterRemoveAndAddAlias); } @@ -193,7 +193,7 @@ public void testSwapIndexWithAlias() { before = createIndex(before, "test_2"); // Now remove "test" and add an alias to "test" to "test_2" in one go - ClusterState after = service.innerExecute(before, Arrays.asList( + ClusterState after = service.applyAliasActions(before, Arrays.asList( new AliasAction.Add("test_2", "test", null, null, null, null), new AliasAction.RemoveIndex("test"))); AliasOrIndex alias = after.metaData().getAliasAndIndexLookup().get("test"); @@ -208,7 +208,7 @@ public void testAddAliasToRemovedIndex() { ClusterState before = createIndex(ClusterState.builder(ClusterName.DEFAULT).build(), "test"); // Attempt to add an alias to "test" at the same time as we remove it - IndexNotFoundException e = expectThrows(IndexNotFoundException.class, () -> service.innerExecute(before, Arrays.asList( + IndexNotFoundException e = expectThrows(IndexNotFoundException.class, () -> service.applyAliasActions(before, Arrays.asList( new AliasAction.Add("test", "alias", null, null, null, null), new AliasAction.RemoveIndex("test")))); assertEquals("test", e.getIndex().getName()); @@ -219,7 +219,7 @@ public void testRemoveIndexTwice() { ClusterState before = createIndex(ClusterState.builder(ClusterName.DEFAULT).build(), "test"); // Try to remove an index twice. This should just remove the index once.... - ClusterState after = service.innerExecute(before, Arrays.asList( + ClusterState after = service.applyAliasActions(before, Arrays.asList( new AliasAction.RemoveIndex("test"), new AliasAction.RemoveIndex("test"))); assertNull(after.metaData().getAliasAndIndexLookup().get("test")); @@ -228,20 +228,20 @@ public void testRemoveIndexTwice() { public void testAddWriteOnlyWithNoExistingAliases() { ClusterState before = createIndex(ClusterState.builder(ClusterName.DEFAULT).build(), "test"); - ClusterState after = service.innerExecute(before, Arrays.asList( + ClusterState after = service.applyAliasActions(before, Arrays.asList( new AliasAction.Add("test", "alias", null, null, null, false))); assertFalse(after.metaData().index("test").getAliases().get("alias").writeIndex()); assertNull(((AliasOrIndex.Alias) after.metaData().getAliasAndIndexLookup().get("alias")).getWriteIndex()); assertAliasesVersionIncreased("test", before, after); - after = service.innerExecute(before, Arrays.asList( + after = service.applyAliasActions(before, Arrays.asList( new AliasAction.Add("test", "alias", null, null, null, null))); assertNull(after.metaData().index("test").getAliases().get("alias").writeIndex()); assertThat(((AliasOrIndex.Alias) after.metaData().getAliasAndIndexLookup().get("alias")).getWriteIndex(), equalTo(after.metaData().index("test"))); assertAliasesVersionIncreased("test", before, after); - after = service.innerExecute(before, Arrays.asList( + after = service.applyAliasActions(before, Arrays.asList( new AliasAction.Add("test", "alias", null, null, null, true))); assertTrue(after.metaData().index("test").getAliases().get("alias").writeIndex()); assertThat(((AliasOrIndex.Alias) after.metaData().getAliasAndIndexLookup().get("alias")).getWriteIndex(), @@ -258,7 +258,7 @@ public void testAddWriteOnlyWithExistingWriteIndex() { ClusterState before = ClusterState.builder(ClusterName.DEFAULT) .metaData(MetaData.builder().put(indexMetaData).put(indexMetaData2)).build(); - ClusterState after = service.innerExecute(before, Arrays.asList( + ClusterState after = service.applyAliasActions(before, Arrays.asList( new AliasAction.Add("test", "alias", null, null, null, null))); assertNull(after.metaData().index("test").getAliases().get("alias").writeIndex()); assertThat(((AliasOrIndex.Alias) after.metaData().getAliasAndIndexLookup().get("alias")).getWriteIndex(), @@ -266,7 +266,7 @@ public void testAddWriteOnlyWithExistingWriteIndex() { assertAliasesVersionIncreased("test", before, after); assertAliasesVersionUnchanged("test2", before, after); - Exception exception = expectThrows(IllegalStateException.class, () -> service.innerExecute(before, Arrays.asList( + Exception exception = expectThrows(IllegalStateException.class, () -> service.applyAliasActions(before, Arrays.asList( new AliasAction.Add("test", "alias", null, null, null, true)))); assertThat(exception.getMessage(), startsWith("alias [alias] has more than one write index [")); } @@ -286,7 +286,7 @@ public void testSwapWriteOnlyIndex() { new AliasAction.Add("test2", "alias", null, null, null, true) ); Collections.shuffle(swapActions, random()); - ClusterState after = service.innerExecute(before, swapActions); + ClusterState after = service.applyAliasActions(before, swapActions); assertThat(after.metaData().index("test").getAliases().get("alias").writeIndex(), equalTo(unsetValue)); assertTrue(after.metaData().index("test2").getAliases().get("alias").writeIndex()); assertThat(((AliasOrIndex.Alias) after.metaData().getAliasAndIndexLookup().get("alias")).getWriteIndex(), @@ -309,7 +309,7 @@ public void testAddWriteOnlyWithExistingNonWriteIndices() { assertNull(((AliasOrIndex.Alias) before.metaData().getAliasAndIndexLookup().get("alias")).getWriteIndex()); - ClusterState after = service.innerExecute(before, Arrays.asList( + ClusterState after = service.applyAliasActions(before, Arrays.asList( new AliasAction.Add("test3", "alias", null, null, null, true))); assertTrue(after.metaData().index("test3").getAliases().get("alias").writeIndex()); assertThat(((AliasOrIndex.Alias) after.metaData().getAliasAndIndexLookup().get("alias")).getWriteIndex(), @@ -333,7 +333,7 @@ public void testAddWriteOnlyWithIndexRemoved() { assertNull(before.metaData().index("test2").getAliases().get("alias").writeIndex()); assertNull(((AliasOrIndex.Alias) before.metaData().getAliasAndIndexLookup().get("alias")).getWriteIndex()); - ClusterState after = service.innerExecute(before, Collections.singletonList(new AliasAction.RemoveIndex("test"))); + ClusterState after = service.applyAliasActions(before, Collections.singletonList(new AliasAction.RemoveIndex("test"))); assertNull(after.metaData().index("test2").getAliases().get("alias").writeIndex()); assertThat(((AliasOrIndex.Alias) after.metaData().getAliasAndIndexLookup().get("alias")).getWriteIndex(), equalTo(after.metaData().index("test2"))); @@ -348,7 +348,7 @@ public void testAddWriteOnlyValidatesAgainstMetaDataBuilder() { ClusterState before = ClusterState.builder(ClusterName.DEFAULT) .metaData(MetaData.builder().put(indexMetaData).put(indexMetaData2)).build(); - Exception exception = expectThrows(IllegalStateException.class, () -> service.innerExecute(before, Arrays.asList( + Exception exception = expectThrows(IllegalStateException.class, () -> service.applyAliasActions(before, Arrays.asList( new AliasAction.Add("test", "alias", null, null, null, true), new AliasAction.Add("test2", "alias", null, null, null, true) ))); From aba5acdb34417126f9b92f8198386642a691a1bc Mon Sep 17 00:00:00 2001 From: Andrei Dan Date: Fri, 20 Dec 2019 14:03:52 +0000 Subject: [PATCH 3/5] Add the rollover index names in cluster update task name --- .../action/admin/indices/rollover/TransportRolloverAction.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java index 1c50626d8457a..60fce0a170cc0 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java @@ -145,7 +145,8 @@ public void onResponse(IndicesStatsResponse statsResponse) { if (conditionResults.size() == 0 || metConditions.size() > 0) { CreateIndexClusterStateUpdateRequest createIndexRequest = prepareCreateIndexRequest(unresolvedName, rolloverIndexName, rolloverRequest); - clusterService.submitStateUpdateTask("rollover_index", new ClusterStateUpdateTask() { + clusterService.submitStateUpdateTask("rollover_index source [" + sourceIndexName + "] to target [" + + rolloverIndexName + "]", new ClusterStateUpdateTask() { @Override public ClusterState execute(ClusterState currentState) throws Exception { ClusterState newState = createIndexService.applyCreateIndexRequest(currentState, createIndexRequest); From d899a103f5d56d6355f91d71c14f54dff5b6e32d Mon Sep 17 00:00:00 2001 From: Andrei Dan Date: Fri, 20 Dec 2019 14:07:51 +0000 Subject: [PATCH 4/5] Guard active shards observer by newState != oldState --- .../rollover/TransportRolloverAction.java | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java index 60fce0a170cc0..9c1c9d71d6708 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java @@ -167,13 +167,15 @@ public void onFailure(String source, Exception e) { @Override public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { - activeShardsObserver.waitForActiveShards(new String[]{rolloverIndexName}, - rolloverRequest.getCreateIndexRequest().waitForActiveShards(), - rolloverRequest.masterNodeTimeout(), - isShardsAcknowledged -> listener.onResponse(new RolloverResponse( - sourceIndexName, rolloverIndexName, conditionResults, false, true, true, - isShardsAcknowledged)), - listener::onFailure); + if (newState.equals(oldState) == false) { + activeShardsObserver.waitForActiveShards(new String[]{rolloverIndexName}, + rolloverRequest.getCreateIndexRequest().waitForActiveShards(), + rolloverRequest.masterNodeTimeout(), + isShardsAcknowledged -> listener.onResponse(new RolloverResponse( + sourceIndexName, rolloverIndexName, conditionResults, false, true, true, + isShardsAcknowledged)), + listener::onFailure); + } } }); } else { From 78aca58f829ee3b2170736411aa3d5662e33cf3c Mon Sep 17 00:00:00 2001 From: Andrei Dan Date: Fri, 20 Dec 2019 14:08:07 +0000 Subject: [PATCH 5/5] RolloverStep is not retryable --- .../xpack/core/ilm/RolloverStep.java | 5 --- .../ilm/TimeSeriesLifecycleActionsIT.java | 33 ------------------- 2 files changed, 38 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/RolloverStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/RolloverStep.java index 542ac156a9871..90b9d15f21b85 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/RolloverStep.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/RolloverStep.java @@ -30,11 +30,6 @@ public RolloverStep(StepKey key, StepKey nextStepKey, Client client) { super(key, nextStepKey, client); } - @Override - public boolean isRetryable() { - return true; - } - @Override public void performAction(IndexMetaData indexMetaData, ClusterState currentClusterState, ClusterStateObserver observer, Listener listener) { diff --git a/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java b/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java index bbefef7e9bd7c..afb5bb4b66dba 100644 --- a/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java +++ b/x-pack/plugin/ilm/qa/multi-node/src/test/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java @@ -1004,39 +1004,6 @@ public void testILMRolloverOnManuallyRolledIndex() throws Exception { assertBusy(() -> assertTrue(indexExists(thirdIndex))); } - public void testILMRolloverRetriesIfRolloverIndexAlreadyExistsUntilIndexIsDeleted() throws Exception { - String firstIndex = index + "-000001"; - String secondIndex = index + "-000002"; - - createNewSingletonPolicy("hot", new RolloverAction(null, TimeValue.timeValueSeconds(1), null)); - - // create the second index so the rollover of the first index fails - createIndexWithSettings( - secondIndex, - Settings.builder().put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) - .put(RolloverAction.LIFECYCLE_ROLLOVER_ALIAS, "alias"), - false - ); - - createIndexWithSettings( - firstIndex, - Settings.builder().put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) - .put(LifecycleSettings.LIFECYCLE_NAME, policy) - .put(RolloverAction.LIFECYCLE_ROLLOVER_ALIAS, "alias"), - true - ); - - // wait for ILM to start retrying the step - assertBusy(() -> assertThat((Integer) explainIndex(firstIndex).get(FAILED_STEP_RETRY_COUNT_FIELD), greaterThanOrEqualTo(1))); - - deleteIndex(secondIndex); - - // the rollover step should now succeed - assertBusy(() -> assertThat(getStepKeyForIndex(firstIndex), equalTo(TerminalPolicyStep.KEY))); - } - public void testHistoryIsWrittenWithSuccess() throws Exception { String index = "index";