From d819b0e26af4d4e0ba4a065d447dc7843d7bb74e Mon Sep 17 00:00:00 2001 From: Przemko Robakowski Date: Tue, 18 Feb 2020 22:44:21 +0100 Subject: [PATCH 1/4] Make DeleteStep retryable This change marks `DeleteStep` as retryable and adds test to make sure we really can invoke it again. --- .../xpack/core/ilm/DeleteStep.java | 10 ++- .../xpack/ilm/DeleteStepTests.java | 72 +++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/DeleteStepTests.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java index c19f8f3ef7fc5..d1fc407a0ab16 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java @@ -24,12 +24,18 @@ public DeleteStep(StepKey key, StepKey nextStepKey, Client client) { @Override public void performDuringNoSnapshot(IndexMetaData indexMetaData, ClusterState currentState, Listener listener) { getClient().admin().indices() - .delete(new DeleteIndexRequest(indexMetaData.getIndex().getName()).masterNodeTimeout(getMasterTimeout(currentState)), - ActionListener.wrap(response -> listener.onResponse(true), listener::onFailure)); + .prepareDelete(indexMetaData.getIndex().getName()) + .setMasterNodeTimeout(getMasterTimeout(currentState)) + .execute(ActionListener.wrap(response -> listener.onResponse(true), listener::onFailure)); } @Override public boolean indexSurvives() { return false; } + + @Override + public boolean isRetryable() { + return true; + } } diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/DeleteStepTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/DeleteStepTests.java new file mode 100644 index 0000000000000..005ef553345cd --- /dev/null +++ b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/DeleteStepTests.java @@ -0,0 +1,72 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.ilm; + +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.ClusterStateObserver; +import org.elasticsearch.cluster.metadata.IndexMetaData; +import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.test.ESSingleNodeTestCase; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.xpack.core.ilm.AsyncActionStep; +import org.elasticsearch.xpack.core.ilm.DeleteStep; +import org.elasticsearch.xpack.core.ilm.Step.StepKey; + +import java.util.concurrent.atomic.AtomicBoolean; + +import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_READ_ONLY; +import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_READ_ONLY_ALLOW_DELETE; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; +import static org.hamcrest.Matchers.is; + +public class DeleteStepTests extends ESSingleNodeTestCase { + + public void testDeleteStepRetriesOnError() throws Exception { + assertAcked(client().admin().indices().prepareCreate("test") + .setSettings(Settings.builder().put(SETTING_READ_ONLY_ALLOW_DELETE, false).put(SETTING_READ_ONLY, true)) + .get()); + + ClusterService clusterService = getInstanceFromNode(ClusterService.class); + ClusterState state = clusterService.state(); + IndexMetaData indexMetaData = state.metaData().index("test"); + ThreadPool threadPool = getInstanceFromNode(ThreadPool.class); + ClusterStateObserver observer = new ClusterStateObserver(clusterService, null, logger, threadPool.getThreadContext()); + + DeleteStep step = new DeleteStep(new StepKey("delete", "action", "delete"), null, client()); + AtomicBoolean done = new AtomicBoolean(); + + step.performAction(indexMetaData, state, observer, new AsyncActionStep.Listener() { + @Override + public void onResponse(boolean complete) { + done.set(true); + fail("expected the test to fail when updating the setting to an invalid value"); + } + + @Override + public void onFailure(Exception e) { + assertAcked(client().admin().indices().prepareUpdateSettings("test") + .setSettings(Settings.builder().put(SETTING_READ_ONLY, false)) + .get()); + step.performAction(indexMetaData, state, observer, new AsyncActionStep.Listener() { + @Override + public void onResponse(boolean complete) { + done.set(true); + assertThat(complete, is(true)); + } + + @Override + public void onFailure(Exception e) { + done.set(true); + fail("unexpected failure when trying to update setting to a valid value"); + } + }); + } + }); + + assertBusy(() -> assertTrue(done.get())); + } +} From 34dfafdae1f7ef572d7738abd00b426d342e5079 Mon Sep 17 00:00:00 2001 From: Przemko Robakowski Date: Tue, 18 Feb 2020 22:53:38 +0100 Subject: [PATCH 2/4] Fix unused import --- .../main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java index d1fc407a0ab16..bdd9ac52e922b 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.core.ilm; import org.elasticsearch.action.ActionListener; -import org.elasticsearch.action.admin.indices.delete.DeleteIndexRequest; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.metadata.IndexMetaData; From c3b765f8aebe1427aa6d12c8ad0ad9cec4fb5de8 Mon Sep 17 00:00:00 2001 From: Przemko Robakowski Date: Tue, 18 Feb 2020 23:35:06 +0100 Subject: [PATCH 3/4] revert unneeded changes --- .../java/org/elasticsearch/xpack/core/ilm/DeleteStep.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java index bdd9ac52e922b..7c086a52a4ea1 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/DeleteStep.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.core.ilm; import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.admin.indices.delete.DeleteIndexRequest; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.metadata.IndexMetaData; @@ -23,9 +24,8 @@ public DeleteStep(StepKey key, StepKey nextStepKey, Client client) { @Override public void performDuringNoSnapshot(IndexMetaData indexMetaData, ClusterState currentState, Listener listener) { getClient().admin().indices() - .prepareDelete(indexMetaData.getIndex().getName()) - .setMasterNodeTimeout(getMasterTimeout(currentState)) - .execute(ActionListener.wrap(response -> listener.onResponse(true), listener::onFailure)); + .delete(new DeleteIndexRequest(indexMetaData.getIndex().getName()).masterNodeTimeout(getMasterTimeout(currentState)), + ActionListener.wrap(response -> listener.onResponse(true), listener::onFailure)); } @Override From 7fda67b3528484da7e24548db5b02043d807cd09 Mon Sep 17 00:00:00 2001 From: Przemko Robakowski Date: Wed, 19 Feb 2020 14:16:00 +0100 Subject: [PATCH 4/4] test reworked --- .../ilm/TimeSeriesLifecycleActionsIT.java | 22 ++++++ .../xpack/ilm/DeleteStepTests.java | 72 ------------------- 2 files changed, 22 insertions(+), 72 deletions(-) delete mode 100644 x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/DeleteStepTests.java 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 9b1abed6971f5..f967c3cc4325c 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 @@ -28,6 +28,7 @@ import org.elasticsearch.test.rest.ESRestTestCase; import org.elasticsearch.xpack.core.ilm.AllocateAction; import org.elasticsearch.xpack.core.ilm.DeleteAction; +import org.elasticsearch.xpack.core.ilm.DeleteStep; import org.elasticsearch.xpack.core.ilm.ErrorStep; import org.elasticsearch.xpack.core.ilm.ForceMergeAction; import org.elasticsearch.xpack.core.ilm.FreezeAction; @@ -206,6 +207,27 @@ public void testMoveToRolloverStep() throws Exception { assertBusy(() -> assertFalse(indexExists(shrunkenOriginalIndex))); } + public void testRetryFailedDeleteAction() throws Exception { + createIndexWithSettings(index, Settings.builder() + .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0) + .put(IndexMetaData.SETTING_READ_ONLY_ALLOW_DELETE, false)); + + createNewSingletonPolicy("delete", new DeleteAction()); + + Request request = new Request("PUT", index + "/_settings"); + request.setJsonEntity("{\"index.blocks.read_only\": true, \"index.lifecycle.name\": \"" + policy + "\"}"); + assertOK(client().performRequest(request)); + + assertBusy(() -> assertThat(getFailedStepForIndex(index), equalTo(DeleteStep.NAME))); + assertTrue(indexExists(index)); + + request.setJsonEntity("{\"index.blocks.read_only\":false}"); + assertOK(client().performRequest(request)); + + assertBusy(() -> assertFalse(indexExists(index))); + } + public void testRetryFailedShrinkAction() throws Exception { int numShards = 4; int divisor = randomFrom(2, 4); diff --git a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/DeleteStepTests.java b/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/DeleteStepTests.java deleted file mode 100644 index 005ef553345cd..0000000000000 --- a/x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/DeleteStepTests.java +++ /dev/null @@ -1,72 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.ilm; - -import org.elasticsearch.cluster.ClusterState; -import org.elasticsearch.cluster.ClusterStateObserver; -import org.elasticsearch.cluster.metadata.IndexMetaData; -import org.elasticsearch.cluster.service.ClusterService; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.test.ESSingleNodeTestCase; -import org.elasticsearch.threadpool.ThreadPool; -import org.elasticsearch.xpack.core.ilm.AsyncActionStep; -import org.elasticsearch.xpack.core.ilm.DeleteStep; -import org.elasticsearch.xpack.core.ilm.Step.StepKey; - -import java.util.concurrent.atomic.AtomicBoolean; - -import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_READ_ONLY; -import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_READ_ONLY_ALLOW_DELETE; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; -import static org.hamcrest.Matchers.is; - -public class DeleteStepTests extends ESSingleNodeTestCase { - - public void testDeleteStepRetriesOnError() throws Exception { - assertAcked(client().admin().indices().prepareCreate("test") - .setSettings(Settings.builder().put(SETTING_READ_ONLY_ALLOW_DELETE, false).put(SETTING_READ_ONLY, true)) - .get()); - - ClusterService clusterService = getInstanceFromNode(ClusterService.class); - ClusterState state = clusterService.state(); - IndexMetaData indexMetaData = state.metaData().index("test"); - ThreadPool threadPool = getInstanceFromNode(ThreadPool.class); - ClusterStateObserver observer = new ClusterStateObserver(clusterService, null, logger, threadPool.getThreadContext()); - - DeleteStep step = new DeleteStep(new StepKey("delete", "action", "delete"), null, client()); - AtomicBoolean done = new AtomicBoolean(); - - step.performAction(indexMetaData, state, observer, new AsyncActionStep.Listener() { - @Override - public void onResponse(boolean complete) { - done.set(true); - fail("expected the test to fail when updating the setting to an invalid value"); - } - - @Override - public void onFailure(Exception e) { - assertAcked(client().admin().indices().prepareUpdateSettings("test") - .setSettings(Settings.builder().put(SETTING_READ_ONLY, false)) - .get()); - step.performAction(indexMetaData, state, observer, new AsyncActionStep.Listener() { - @Override - public void onResponse(boolean complete) { - done.set(true); - assertThat(complete, is(true)); - } - - @Override - public void onFailure(Exception e) { - done.set(true); - fail("unexpected failure when trying to update setting to a valid value"); - } - }); - } - }); - - assertBusy(() -> assertTrue(done.get())); - } -}