From 46943982c9ef92752a09e4bf0f5f9419e1c187f8 Mon Sep 17 00:00:00 2001 From: Juan Camilo Bages Date: Thu, 9 Jun 2022 17:52:12 -0500 Subject: [PATCH] Avoid making the index read-only in the Force merge action for ILM (Closes #43426) (#81162) Added a new NoopStep that we can use as a placeholder for a step that we had in a previous version but now we want to remove it. If the just go and erase the step then the indices that were in that step will get into a stuck state. Instead, we're using the same old step key but with this NoopStep which does nothing but the transition to the next step. Modify the ForceMergeLifecycleAction in ILM so it doesn't make the index read-only. For older indices already in the "convert index to read-only" step before the upgrade, we'll use the NoopStep mentioned before in order to ignore the step and just transition to the next one. Solves: #43426 Co-authored-by: Lee Hinman --- docs/changelog/81162.yaml | 6 ++++ .../xpack/core/ilm/ForceMergeAction.java | 24 +++++++------ .../xpack/core/ilm/NoopStep.java | 34 +++++++++++++++++++ .../xpack/core/ilm/ForceMergeActionTests.java | 29 +++++++++------- .../core/ilm/PhaseCacheManagementTests.java | 1 + .../xpack/ilm/TimeSeriesDataStreamsIT.java | 24 +++++++++---- .../ilm/TimeSeriesLifecycleActionsIT.java | 12 ++++--- 7 files changed, 94 insertions(+), 36 deletions(-) create mode 100644 docs/changelog/81162.yaml create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/NoopStep.java diff --git a/docs/changelog/81162.yaml b/docs/changelog/81162.yaml new file mode 100644 index 0000000000000..ca3b9379f8591 --- /dev/null +++ b/docs/changelog/81162.yaml @@ -0,0 +1,6 @@ +pr: 81162 +summary: Stop making index read-only when executing force merge index lifecycle management action +area: Infra/Core +type: enhancement +issues: + - 81162 diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/ForceMergeAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/ForceMergeAction.java index 4b1a9b54b7b89..a398738675522 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/ForceMergeAction.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/ForceMergeAction.java @@ -35,8 +35,6 @@ public class ForceMergeAction implements LifecycleAction { private static final Logger logger = LogManager.getLogger(ForceMergeAction.class); - private static final Settings READ_ONLY_SETTINGS = Settings.builder().put(IndexMetadata.SETTING_BLOCKS_WRITE, true).build(); - private static final Settings BEST_COMPRESSION_SETTINGS = Settings.builder() .put(EngineConfig.INDEX_CODEC_SETTING.getKey(), CodecService.BEST_COMPRESSION_CODEC) .build(); @@ -120,7 +118,7 @@ public List toSteps(Client client, String phase, Step.StepKey nextStepKey) final boolean codecChange = codec != null && codec.equals(CodecService.BEST_COMPRESSION_CODEC); StepKey preForceMergeBranchingKey = new StepKey(phase, NAME, CONDITIONAL_SKIP_FORCE_MERGE_STEP); - StepKey checkNotWriteIndex = new StepKey(phase, NAME, CheckNotDataStreamWriteIndexStep.NAME); + StepKey checkNotWriteIndexKey = new StepKey(phase, NAME, CheckNotDataStreamWriteIndexStep.NAME); StepKey readOnlyKey = new StepKey(phase, NAME, ReadOnlyAction.NAME); StepKey closeKey = new StepKey(phase, NAME, CloseIndexStep.NAME); @@ -133,7 +131,7 @@ public List toSteps(Client client, String phase, Step.StepKey nextStepKey) BranchingStep conditionalSkipShrinkStep = new BranchingStep( preForceMergeBranchingKey, - checkNotWriteIndex, + checkNotWriteIndexKey, nextStepKey, (index, clusterState) -> { IndexMetadata indexMetadata = clusterState.metadata().index(index); @@ -152,14 +150,18 @@ public List toSteps(Client client, String phase, Step.StepKey nextStepKey) return false; } ); - CheckNotDataStreamWriteIndexStep checkNotWriteIndexStep = new CheckNotDataStreamWriteIndexStep(checkNotWriteIndex, readOnlyKey); - UpdateSettingsStep readOnlyStep = new UpdateSettingsStep( - readOnlyKey, - codecChange ? closeKey : forceMergeKey, - client, - READ_ONLY_SETTINGS + + // Indices in this step key can skip the no-op step and jump directly to the step with closeKey/forcemergeKey key + CheckNotDataStreamWriteIndexStep checkNotWriteIndexStep = new CheckNotDataStreamWriteIndexStep( + checkNotWriteIndexKey, + codecChange ? closeKey : forceMergeKey ); + // Indices already in this step key when upgrading need to know how to move forward but stop making the index + // read-only. In order to achieve this we introduce a no-op step with the same key as the read-only step so that + // the index can safely move to the next step without performing any read-only action nor getting stuck in this step + NoopStep noopStep = new NoopStep(readOnlyKey, codecChange ? closeKey : forceMergeKey); + CloseIndexStep closeIndexStep = new CloseIndexStep(closeKey, updateCompressionKey, client); UpdateSettingsStep updateBestCompressionSettings = new UpdateSettingsStep( updateCompressionKey, @@ -180,7 +182,7 @@ public List toSteps(Client client, String phase, Step.StepKey nextStepKey) List mergeSteps = new ArrayList<>(); mergeSteps.add(conditionalSkipShrinkStep); mergeSteps.add(checkNotWriteIndexStep); - mergeSteps.add(readOnlyStep); + mergeSteps.add(noopStep); if (codecChange) { mergeSteps.add(closeIndexStep); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/NoopStep.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/NoopStep.java new file mode 100644 index 0000000000000..00abccca0790a --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/NoopStep.java @@ -0,0 +1,34 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ +package org.elasticsearch.xpack.core.ilm; + +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.index.Index; + +/** + * This is a Noop step that can be used for backwards compatibility when removing some step in newer versions. + * It literally does nothing so that we can safely proceed to the nextStepKey without getting stuck. + */ +public class NoopStep extends ClusterStateWaitStep { + public static final String NAME = "noop"; + + public NoopStep(StepKey key, StepKey nextStepKey) { + super(key, nextStepKey); + } + + @Override + public boolean isRetryable() { + // As this is a noop step and we don't want to get stuck in, we want it to be retryable + return true; + } + + @Override + public Result isConditionMet(Index index, ClusterState clusterState) { + // We always want to move forward with this step so this should always be true + return new Result(true, null); + } +} diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/ForceMergeActionTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/ForceMergeActionTests.java index ed50b12495253..c567913bfd700 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/ForceMergeActionTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/ForceMergeActionTests.java @@ -6,7 +6,6 @@ */ package org.elasticsearch.xpack.core.ilm; -import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.Writeable.Reader; import org.elasticsearch.common.xcontent.XContentHelper; @@ -70,7 +69,7 @@ private void assertNonBestCompression(ForceMergeAction instance) { assertEquals(5, steps.size()); BranchingStep firstStep = (BranchingStep) steps.get(0); CheckNotDataStreamWriteIndexStep secondStep = (CheckNotDataStreamWriteIndexStep) steps.get(1); - UpdateSettingsStep thirdStep = (UpdateSettingsStep) steps.get(2); + NoopStep thirdStep = (NoopStep) steps.get(2); ForceMergeStep fourthStep = (ForceMergeStep) steps.get(3); SegmentCountStep fifthStep = (SegmentCountStep) steps.get(4); @@ -78,13 +77,16 @@ private void assertNonBestCompression(ForceMergeAction instance) { firstStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ForceMergeAction.CONDITIONAL_SKIP_FORCE_MERGE_STEP)) ); + assertThat(secondStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, CheckNotDataStreamWriteIndexStep.NAME))); - assertThat(secondStep.getNextStepKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ReadOnlyAction.NAME))); + assertThat(secondStep.getNextStepKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ForceMergeStep.NAME))); + assertThat(thirdStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ReadOnlyAction.NAME))); assertThat(thirdStep.getNextStepKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ForceMergeStep.NAME))); - assertTrue(IndexMetadata.INDEX_BLOCKS_WRITE_SETTING.get(thirdStep.getSettings())); + assertThat(fourthStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, ForceMergeStep.NAME))); - assertThat(fourthStep.getNextStepKey(), equalTo(fifthStep.getKey())); + assertThat(fourthStep.getNextStepKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, SegmentCountStep.NAME))); + assertThat(fifthStep.getKey(), equalTo(new StepKey(phase, ForceMergeAction.NAME, SegmentCountStep.NAME))); assertThat(fifthStep.getNextStepKey(), equalTo(nextStepKey)); } @@ -101,8 +103,9 @@ private void assertBestCompression(ForceMergeAction instance) { .skip(1) .map(s -> new Tuple<>(s.getKey(), s.getNextStepKey())) .collect(Collectors.toList()); + StepKey checkNotWriteIndex = new StepKey(phase, ForceMergeAction.NAME, CheckNotDataStreamWriteIndexStep.NAME); - StepKey readOnly = new StepKey(phase, ForceMergeAction.NAME, ReadOnlyAction.NAME); + StepKey noop = new StepKey(phase, ForceMergeAction.NAME, ReadOnlyAction.NAME); StepKey closeIndex = new StepKey(phase, ForceMergeAction.NAME, CloseIndexStep.NAME); StepKey updateCodec = new StepKey(phase, ForceMergeAction.NAME, UpdateSettingsStep.NAME); StepKey openIndex = new StepKey(phase, ForceMergeAction.NAME, OpenIndexStep.NAME); @@ -116,8 +119,8 @@ private void assertBestCompression(ForceMergeAction instance) { assertThat( stepKeys, contains( - new Tuple<>(checkNotWriteIndex, readOnly), - new Tuple<>(readOnly, closeIndex), + new Tuple<>(checkNotWriteIndex, closeIndex), + new Tuple<>(noop, closeIndex), new Tuple<>(closeIndex, updateCodec), new Tuple<>(updateCodec, openIndex), new Tuple<>(openIndex, waitForGreen), @@ -127,11 +130,11 @@ private void assertBestCompression(ForceMergeAction instance) { ) ); - UpdateSettingsStep thirdStep = (UpdateSettingsStep) steps.get(2); - UpdateSettingsStep fifthStep = (UpdateSettingsStep) steps.get(4); - - assertTrue(IndexMetadata.INDEX_BLOCKS_WRITE_SETTING.get(thirdStep.getSettings())); - assertThat(fifthStep.getSettings().get(EngineConfig.INDEX_CODEC_SETTING.getKey()), equalTo(CodecService.BEST_COMPRESSION_CODEC)); + UpdateSettingsStep updateCodecStep = (UpdateSettingsStep) steps.get(4); + assertThat( + updateCodecStep.getSettings().get(EngineConfig.INDEX_CODEC_SETTING.getKey()), + equalTo(CodecService.BEST_COMPRESSION_CODEC) + ); } public void testMissingMaxNumSegments() throws IOException { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/PhaseCacheManagementTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/PhaseCacheManagementTests.java index bc6e248930e5a..42ec31e8b9470 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/PhaseCacheManagementTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/PhaseCacheManagementTests.java @@ -279,6 +279,7 @@ public void testReadStepKeys() { new Step.StepKey("phase", "allocate", AllocationRoutedStep.NAME), new Step.StepKey("phase", "forcemerge", ForceMergeAction.CONDITIONAL_SKIP_FORCE_MERGE_STEP), new Step.StepKey("phase", "forcemerge", CheckNotDataStreamWriteIndexStep.NAME), + // This read-only key is now a noop step but we preserved it for backwards compatibility new Step.StepKey("phase", "forcemerge", ReadOnlyAction.NAME), new Step.StepKey("phase", "forcemerge", ForceMergeAction.NAME), new Step.StepKey("phase", "forcemerge", SegmentCountStep.NAME) diff --git a/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesDataStreamsIT.java b/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesDataStreamsIT.java index f103d45fc5693..65f4538a5b705 100644 --- a/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesDataStreamsIT.java +++ b/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesDataStreamsIT.java @@ -15,6 +15,7 @@ import org.elasticsearch.cluster.metadata.Template; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.index.engine.EngineConfig; import org.elasticsearch.test.rest.ESRestTestCase; import org.elasticsearch.xcontent.XContentType; import org.elasticsearch.xpack.core.ilm.CheckNotDataStreamWriteIndexStep; @@ -241,8 +242,8 @@ public void testFreezeAction() throws Exception { assertNull(settings.get("index.frozen")); } - public void testForceMergeAction() throws Exception { - createNewSingletonPolicy(client(), policyName, "warm", new ForceMergeAction(1, null)); + public void checkForceMergeAction(String codec) throws Exception { + createNewSingletonPolicy(client(), policyName, "warm", new ForceMergeAction(1, codec)); createComposableTemplate(client(), template, dataStream + "*", getTemplate(policyName)); indexDocument(client(), dataStream, true); @@ -260,11 +261,20 @@ public void testForceMergeAction() throws Exception { // Manual rollover the original index such that it's not the write index in the data stream anymore rolloverMaxOneDocCondition(client(), dataStream); - assertBusy( - () -> assertThat(explainIndex(client(), backingIndexName).get("step"), is(PhaseCompleteStep.NAME)), - 30, - TimeUnit.SECONDS - ); + assertBusy(() -> { + assertThat(explainIndex(client(), backingIndexName).get("step"), is(PhaseCompleteStep.NAME)); + Map settings = getOnlyIndexSettings(client(), backingIndexName); + assertThat(settings.get(EngineConfig.INDEX_CODEC_SETTING.getKey()), equalTo(codec)); + assertThat(settings.containsKey(IndexMetadata.INDEX_BLOCKS_WRITE_SETTING.getKey()), equalTo(false)); + }, 30, TimeUnit.SECONDS); + } + + public void testForceMergeAction() throws Exception { + checkForceMergeAction(null); + } + + public void testForceMergeActionWithCompressionCodec() throws Exception { + checkForceMergeAction("best_compression"); } @SuppressWarnings("unchecked") diff --git a/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java b/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java index f445db2053bbf..25ea0b1bf7995 100644 --- a/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java +++ b/x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java @@ -473,7 +473,7 @@ public void testDeleteDuringSnapshot() throws Exception { assertOK(client().performRequest(new Request("DELETE", "/_snapshot/repo/" + snapName))); } - public void forceMergeActionWithCodec(String codec) throws Exception { + public void checkForceMergeAction(String codec) throws Exception { createIndexWithSettings( client(), index, @@ -495,17 +495,19 @@ public void forceMergeActionWithCodec(String codec) throws Exception { assertThat(getStepKeyForIndex(client(), index), equalTo(PhaseCompleteStep.finalStep("warm").getKey())); Map settings = getOnlyIndexSettings(client(), index); assertThat(settings.get(EngineConfig.INDEX_CODEC_SETTING.getKey()), equalTo(codec)); - assertThat(settings.get(IndexMetadata.INDEX_BLOCKS_WRITE_SETTING.getKey()), equalTo("true")); + assertThat(settings.containsKey(IndexMetadata.INDEX_BLOCKS_WRITE_SETTING.getKey()), equalTo(false)); }, 30, TimeUnit.SECONDS); - expectThrows(ResponseException.class, () -> indexDocument(client(), index)); + + // No exception should be thrown here as writes were not blocked + indexDocument(client(), index); } public void testForceMergeAction() throws Exception { - forceMergeActionWithCodec(null); + checkForceMergeAction(null); } public void testForceMergeActionWithCompressionCodec() throws Exception { - forceMergeActionWithCodec("best_compression"); + checkForceMergeAction("best_compression"); } public void testSetPriority() throws Exception {