From d4fcf5623fbf267cbe34320663b3a51bac1582ad Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Wed, 22 Jan 2025 17:45:25 +0100 Subject: [PATCH 1/4] Forbid the removal of the write block if the index is read-only --- .../AbstractIndexCompatibilityTestCase.java | 23 ++++++++++++++- ...sterRestartLuceneIndexCompatibilityIT.java | 12 +++++--- ...gradeLuceneIndexCompatibilityTestCase.java | 29 ++++++++++++++++--- .../MetadataUpdateSettingsService.java | 25 ++++++++++++++-- 4 files changed, 78 insertions(+), 11 deletions(-) diff --git a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/AbstractIndexCompatibilityTestCase.java b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/AbstractIndexCompatibilityTestCase.java index ac4e1d9175885..4b5762a4cacce 100644 --- a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/AbstractIndexCompatibilityTestCase.java +++ b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/AbstractIndexCompatibilityTestCase.java @@ -12,6 +12,8 @@ import org.apache.http.entity.ContentType; import org.apache.http.entity.InputStreamEntity; import org.elasticsearch.client.Request; +import org.elasticsearch.client.ResponseException; +import org.elasticsearch.cluster.block.ClusterBlockLevel; import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.common.Strings; import org.elasticsearch.common.settings.Settings; @@ -42,6 +44,7 @@ import static org.elasticsearch.test.cluster.util.Version.fromString; import static org.elasticsearch.test.rest.ObjectPath.createFromResponse; import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.is; @@ -272,9 +275,27 @@ protected void addIndexBlock(String indexName, IndexMetadata.APIBlock apiBlock) assertAcknowledged(client().performRequest(request)); } - protected void assertThatIndexBlock(String indexName, IndexMetadata.APIBlock apiBlock) throws Exception { + protected void assertIndexBlockExists(String indexName, IndexMetadata.APIBlock apiBlock) throws Exception { var indexSettings = getIndexSettingsAsMap(indexName); assertThat(indexSettings.get(VERIFIED_READ_ONLY_SETTING.getKey()), equalTo(Boolean.TRUE.toString())); assertThat(indexSettings.get(apiBlock.settingName()), equalTo(Boolean.TRUE.toString())); } + + protected void assertIndexBlockNotUpdateable(String indexName, IndexMetadata.APIBlock apiBlock) { + var settings = Settings.builder(); + if (apiBlock.getBlock().contains(ClusterBlockLevel.METADATA_WRITE) || randomBoolean()) { + settings.putNull(apiBlock.settingName()); + } else { + settings.put(apiBlock.settingName(), false); + } + var exception = expectThrows(ResponseException.class, () -> updateIndexSettings(indexName, settings)); + assertThat(exception.getResponse().getStatusLine().getStatusCode(), equalTo(400)); + assertThat( + exception.getMessage(), + allOf( + containsString("Can't update setting [" + apiBlock.settingName() + "] for read-only compatible"), + containsString(indexName) + ) + ); + } } diff --git a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/FullClusterRestartLuceneIndexCompatibilityIT.java b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/FullClusterRestartLuceneIndexCompatibilityIT.java index d7829d8225034..5d5860be049fc 100644 --- a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/FullClusterRestartLuceneIndexCompatibilityIT.java +++ b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/FullClusterRestartLuceneIndexCompatibilityIT.java @@ -67,7 +67,8 @@ public void testIndexUpgrade() throws Exception { assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); assertDocCount(client(), index, numDocs); - assertThatIndexBlock(index, IndexMetadata.APIBlock.WRITE); + assertIndexBlockExists(index, IndexMetadata.APIBlock.WRITE); + assertIndexBlockNotUpdateable(index, IndexMetadata.APIBlock.WRITE); var numberOfReplicas = getNumberOfReplicas(index); if (0 < numberOfReplicas) { @@ -86,6 +87,8 @@ public void testIndexUpgrade() throws Exception { closeIndex(index); ensureGreen(index); + assertIndexBlockNotUpdateable(index, IndexMetadata.APIBlock.WRITE); // test again on closed index + logger.debug("--> adding replica to test peer-recovery for closed shards"); updateIndexSettings(index, Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2)); ensureGreen(index); @@ -141,7 +144,8 @@ public void testIndexUpgradeReadOnlyBlock() throws Exception { assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); assertDocCount(client(), index, numDocs); - assertThatIndexBlock(index, IndexMetadata.APIBlock.READ_ONLY); + assertIndexBlockExists(index, IndexMetadata.APIBlock.READ_ONLY); + assertIndexBlockNotUpdateable(index, IndexMetadata.APIBlock.READ_ONLY); } } @@ -196,7 +200,7 @@ public void testRestoreIndex() throws Exception { restoreIndex(repository, snapshot, index, restoredIndex); ensureGreen(restoredIndex); - assertThatIndexBlock(restoredIndex, IndexMetadata.APIBlock.WRITE); + assertIndexBlockExists(restoredIndex, IndexMetadata.APIBlock.WRITE); assertThat(indexVersion(restoredIndex), equalTo(VERSION_MINUS_2)); assertDocCount(client(), restoredIndex, numDocs); @@ -277,7 +281,7 @@ public void testRestoreIndexOverClosedIndex() throws Exception { if (isFullyUpgradedTo(VERSION_CURRENT)) { assertThat(isIndexClosed(index), equalTo(true)); - assertThatIndexBlock(index, IndexMetadata.APIBlock.WRITE); + assertIndexBlockExists(index, IndexMetadata.APIBlock.WRITE); logger.debug("--> restoring index [{}] over existing closed index", index); restoreIndex(repository, snapshot, index, index); diff --git a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/RollingUpgradeLuceneIndexCompatibilityTestCase.java b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/RollingUpgradeLuceneIndexCompatibilityTestCase.java index b145b1e08c71d..6296c45a2d5d1 100644 --- a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/RollingUpgradeLuceneIndexCompatibilityTestCase.java +++ b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/RollingUpgradeLuceneIndexCompatibilityTestCase.java @@ -19,6 +19,7 @@ import java.util.List; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; public class RollingUpgradeLuceneIndexCompatibilityTestCase extends RollingUpgradeIndexCompatibilityTestCase { @@ -66,7 +67,8 @@ public void testIndexUpgrade() throws Exception { } if (nodesVersions().values().stream().anyMatch(v -> v.onOrAfter(VERSION_CURRENT))) { - assertThatIndexBlock(index, IndexMetadata.APIBlock.WRITE); + assertIndexBlockExists(index, IndexMetadata.APIBlock.WRITE); + assertIndexBlockNotUpdateable(index, IndexMetadata.APIBlock.WRITE); if (isIndexClosed(index)) { logger.debug("--> re-opening index [{}] after upgrade", index); @@ -123,7 +125,8 @@ public void testIndexUpgradeReadOnlyBlock() throws Exception { } if (nodesVersions().values().stream().anyMatch(v -> v.onOrAfter(VERSION_CURRENT))) { - assertThatIndexBlock(index, IndexMetadata.APIBlock.READ_ONLY); + assertIndexBlockExists(index, IndexMetadata.APIBlock.READ_ONLY); + assertIndexBlockNotUpdateable(index, IndexMetadata.APIBlock.READ_ONLY); assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); assertDocCount(client(), index, numDocs); @@ -174,16 +177,17 @@ public void testRestoreIndex() throws Exception { deleteIndex(index); return; } + if (nodesVersions().values().stream().anyMatch(v -> v.onOrAfter(VERSION_CURRENT))) { var restoredIndex = suffix("index-restored-rolling"); boolean success = false; try { - logger.debug("--> restoring index [{}] as [{}]", index, restoredIndex); restoreIndex(repository, snapshot, index, restoredIndex); ensureGreen(restoredIndex); - assertThatIndexBlock(restoredIndex, IndexMetadata.APIBlock.WRITE); + assertIndexBlockExists(restoredIndex, IndexMetadata.APIBlock.WRITE); + assertIndexBlockNotUpdateable(restoredIndex, IndexMetadata.APIBlock.WRITE); assertThat(indexVersion(restoredIndex), equalTo(VERSION_MINUS_2)); assertDocCount(client(), restoredIndex, numDocs); @@ -194,6 +198,8 @@ public void testRestoreIndex() throws Exception { closeIndex(restoredIndex); ensureGreen(restoredIndex); + assertIndexBlockNotUpdateable(restoredIndex, IndexMetadata.APIBlock.WRITE); // test again on closed index + logger.debug("--> re-opening restored index [{}]", restoredIndex); openIndex(restoredIndex); ensureGreen(restoredIndex); @@ -214,5 +220,20 @@ public void testRestoreIndex() throws Exception { } } } + + if (isFullyUpgradedTo(VERSION_CURRENT)) { + var exception = expectThrows( + ResponseException.class, + () -> restoreIndex( + repository, + snapshot, + index, + suffix("unrestorable"), + Settings.builder().put(IndexMetadata.APIBlock.WRITE.settingName(), false).build() + ) + ); + assertThat(exception.getResponse().getStatusLine().getStatusCode(), equalTo(500)); + assertThat(exception.getMessage(), containsString("must be marked as read-only using the setting")); + } } } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index e984768277d27..c4643b2727218 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -39,6 +39,7 @@ import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexSettings; +import org.elasticsearch.index.IndexVersion; import org.elasticsearch.indices.IndicesService; import org.elasticsearch.indices.ShardLimitValidator; import org.elasticsearch.threadpool.ThreadPool; @@ -330,7 +331,15 @@ ClusterState execute(ClusterState currentState) { final ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks()); boolean changedBlocks = false; for (IndexMetadata.APIBlock block : IndexMetadata.APIBlock.values()) { - changedBlocks |= maybeUpdateClusterBlock(actualIndices, blocks, block.block, block.setting, openSettings, metadataBuilder); + changedBlocks |= maybeUpdateClusterBlock( + actualIndices, + blocks, + block.block, + block.setting, + openSettings, + metadataBuilder, + currentState.nodes().getMinSupportedIndexVersion() + ); } changed |= changedBlocks; @@ -426,7 +435,8 @@ private static boolean maybeUpdateClusterBlock( ClusterBlock block, Setting setting, Settings openSettings, - Metadata.Builder metadataBuilder + Metadata.Builder metadataBuilder, + IndexVersion minSupportedIndexVersion ) { boolean changed = false; if (setting.exists(openSettings)) { @@ -443,6 +453,17 @@ private static boolean maybeUpdateClusterBlock( changed = true; if (block.contains(ClusterBlockLevel.WRITE)) { IndexMetadata indexMetadata = metadataBuilder.get(index); + if (indexMetadata.getCompatibilityVersion().before(minSupportedIndexVersion)) { + // Forbid the removal of the block if the index cannot be written by one or more nodes of the cluster + throw new IllegalArgumentException( + String.format( + Locale.ROOT, + "Can't update setting [%s] for read-only compatible index %s", + setting.getKey(), + indexMetadata.getIndex() + ) + ); + } Settings.Builder indexSettings = Settings.builder().put(indexMetadata.getSettings()); indexSettings.remove(MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING.getKey()); metadataBuilder.put(IndexMetadata.builder(indexMetadata).settings(indexSettings)); From 930c705821b5ce808367002a76ac8061f0a4a32f Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Fri, 24 Jan 2025 18:02:08 +0100 Subject: [PATCH 2/4] rework block & settings handling --- .../AbstractIndexCompatibilityTestCase.java | 66 +++++-- ...sterRestartLuceneIndexCompatibilityIT.java | 168 ++++++++++-------- ...gradeLuceneIndexCompatibilityTestCase.java | 84 +++++++-- .../indices/settings/UpdateSettingsIT.java | 33 ++++ .../cluster/block/ClusterBlocks.java | 4 + .../MetadataUpdateSettingsService.java | 72 ++++++-- .../test/rest/ESRestTestCase.java | 2 +- 7 files changed, 308 insertions(+), 121 deletions(-) diff --git a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/AbstractIndexCompatibilityTestCase.java b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/AbstractIndexCompatibilityTestCase.java index 4b5762a4cacce..9bb5b7e944389 100644 --- a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/AbstractIndexCompatibilityTestCase.java +++ b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/AbstractIndexCompatibilityTestCase.java @@ -13,9 +13,11 @@ import org.apache.http.entity.InputStreamEntity; import org.elasticsearch.client.Request; import org.elasticsearch.client.ResponseException; -import org.elasticsearch.cluster.block.ClusterBlockLevel; +import org.elasticsearch.cluster.block.ClusterBlock; import org.elasticsearch.cluster.metadata.IndexMetadata; +import org.elasticsearch.cluster.metadata.MetadataIndexStateService; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.mapper.MapperService; @@ -26,6 +28,7 @@ import org.elasticsearch.test.cluster.util.Version; import org.elasticsearch.test.rest.ESRestTestCase; import org.elasticsearch.xcontent.XContentType; +import org.hamcrest.Matcher; import org.junit.After; import org.junit.Before; import org.junit.ClassRule; @@ -34,12 +37,13 @@ import org.junit.rules.TestRule; import java.io.IOException; +import java.util.Comparator; import java.util.HashMap; +import java.util.List; import java.util.Locale; import java.util.Map; import java.util.stream.IntStream; -import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING; import static org.elasticsearch.test.cluster.util.Version.CURRENT; import static org.elasticsearch.test.cluster.util.Version.fromString; import static org.elasticsearch.test.rest.ObjectPath.createFromResponse; @@ -275,27 +279,51 @@ protected void addIndexBlock(String indexName, IndexMetadata.APIBlock apiBlock) assertAcknowledged(client().performRequest(request)); } - protected void assertIndexBlockExists(String indexName, IndexMetadata.APIBlock apiBlock) throws Exception { - var indexSettings = getIndexSettingsAsMap(indexName); - assertThat(indexSettings.get(VERIFIED_READ_ONLY_SETTING.getKey()), equalTo(Boolean.TRUE.toString())); - assertThat(indexSettings.get(apiBlock.settingName()), equalTo(Boolean.TRUE.toString())); + private static ClusterBlock toIndexBlock(String blockId) { + int block = Integer.parseInt(blockId); + for (var indexBlock : List.of( + IndexMetadata.INDEX_READ_ONLY_BLOCK, + IndexMetadata.INDEX_READ_BLOCK, + IndexMetadata.INDEX_WRITE_BLOCK, + IndexMetadata.INDEX_METADATA_BLOCK, + IndexMetadata.INDEX_READ_ONLY_ALLOW_DELETE_BLOCK, + IndexMetadata.INDEX_REFRESH_BLOCK, + MetadataIndexStateService.INDEX_CLOSED_BLOCK + )) { + if (block == indexBlock.id()) { + return indexBlock; + } + } + throw new AssertionError("No index block found with id [" + blockId + ']'); } - protected void assertIndexBlockNotUpdateable(String indexName, IndexMetadata.APIBlock apiBlock) { - var settings = Settings.builder(); - if (apiBlock.getBlock().contains(ClusterBlockLevel.METADATA_WRITE) || randomBoolean()) { - settings.putNull(apiBlock.settingName()); - } else { - settings.put(apiBlock.settingName(), false); + @SuppressWarnings("unchecked") + protected static List indexBlocks(String indexName) throws Exception { + var responseBody = createFromResponse(client().performRequest(new Request("GET", "_cluster/state/blocks/" + indexName))); + var blocks = (Map) responseBody.evaluate("blocks.indices." + indexName); + if (blocks == null || blocks.isEmpty()) { + return List.of(); } + return blocks.keySet() + .stream() + .map(AbstractIndexCompatibilityTestCase::toIndexBlock) + .sorted(Comparator.comparing(ClusterBlock::id)) + .toList(); + } + + @SuppressWarnings("unchecked") + protected static void assertIndexSetting(String indexName, Setting setting, Matcher matcher) throws Exception { + var indexSettings = getIndexSettingsAsMap(indexName); + assertThat(Boolean.parseBoolean((String) indexSettings.get(setting.getKey())), matcher); + } + + protected static ResponseException expectUpdateIndexSettingsThrows(String indexName, Settings.Builder settings) { var exception = expectThrows(ResponseException.class, () -> updateIndexSettings(indexName, settings)); assertThat(exception.getResponse().getStatusLine().getStatusCode(), equalTo(400)); - assertThat( - exception.getMessage(), - allOf( - containsString("Can't update setting [" + apiBlock.settingName() + "] for read-only compatible"), - containsString(indexName) - ) - ); + return exception; + } + + protected static Matcher containsStringCannotRemoveBlockOnReadOnlyIndex(String indexName) { + return allOf(containsString("Can't remove the write block on read-only compatible index"), containsString(indexName)); } } diff --git a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/FullClusterRestartLuceneIndexCompatibilityIT.java b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/FullClusterRestartLuceneIndexCompatibilityIT.java index 5d5860be049fc..a442bc2f53736 100644 --- a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/FullClusterRestartLuceneIndexCompatibilityIT.java +++ b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/FullClusterRestartLuceneIndexCompatibilityIT.java @@ -9,13 +9,23 @@ package org.elasticsearch.lucene; +import org.elasticsearch.client.ResponseException; import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.repositories.fs.FsRepository; import org.elasticsearch.test.cluster.util.Version; +import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_READ_ONLY_BLOCK; +import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_WRITE_BLOCK; +import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.INDEX_CLOSED_BLOCK; +import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.VERIFIED_BEFORE_CLOSE_SETTING; +import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.either; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; public class FullClusterRestartLuceneIndexCompatibilityIT extends FullClusterRestartIndexCompatibilityTestCase { @@ -28,14 +38,13 @@ public FullClusterRestartLuceneIndexCompatibilityIT(Version version) { } /** - * Creates an index on N-2, upgrades to N -1 and marks as read-only, then upgrades to N. + * Creates an index on N-2, upgrades to N-1 and marks as read-only, then upgrades to N. */ public void testIndexUpgrade() throws Exception { final String index = suffix("index"); final int numDocs = 2431; if (isFullyUpgradedTo(VERSION_MINUS_2)) { - logger.debug("--> creating index [{}]", index); createIndex( client(), index, @@ -45,30 +54,85 @@ public void testIndexUpgrade() throws Exception { .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true) .build() ); - - logger.debug("--> indexing [{}] docs in [{}]", numDocs, index); indexDocs(index, numDocs); return; } - if (isFullyUpgradedTo(VERSION_MINUS_1)) { - ensureGreen(index); + assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); + ensureGreen(index); - assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); + if (isIndexClosed(index) == false) { assertDocCount(client(), index, numDocs); + } - addIndexBlock(index, IndexMetadata.APIBlock.WRITE); + if (isFullyUpgradedTo(VERSION_MINUS_1)) { + final boolean maybeClose = randomBoolean(); + if (maybeClose) { + logger.debug("--> closing index [{}] before upgrade", index); + closeIndex(index); + } + + final var block = randomFrom(IndexMetadata.APIBlock.WRITE, IndexMetadata.APIBlock.READ_ONLY); + addIndexBlock(index, block); + + assertThat(indexBlocks(index), maybeClose ? contains(INDEX_CLOSED_BLOCK, block.getBlock()) : contains(block.getBlock())); + assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(maybeClose)); + assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true)); return; } if (isFullyUpgradedTo(VERSION_CURRENT)) { - ensureGreen(index); + final var isClosed = isIndexClosed(index); + logger.debug("--> upgraded index [{}] is in [{}] state", index, isClosed ? "closed" : "open"); + assertThat( + indexBlocks(index), + isClosed + ? either(contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK)).or(contains(INDEX_CLOSED_BLOCK, INDEX_READ_ONLY_BLOCK)) + : either(contains(INDEX_WRITE_BLOCK)).or(contains(INDEX_READ_ONLY_BLOCK)) + ); + assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(isClosed)); + assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true)); + + if (isClosed == false) { + logger.debug("--> write/read_only API blocks cannot be removed on an opened index"); + var ex = expectUpdateIndexSettingsThrows( + index, + Settings.builder() + .putNull(IndexMetadata.APIBlock.WRITE.settingName()) + .putNull(IndexMetadata.APIBlock.READ_ONLY.settingName()) + ); + assertThat(ex.getMessage(), containsStringCannotRemoveBlockOnReadOnlyIndex(index)); + + } else if (randomBoolean()) { + logger.debug("--> write/read_only API blocks can be removed on a closed index: INDEX_CLOSED_BLOCK already blocks writes"); + updateIndexSettings( + index, + Settings.builder() + .putNull(IndexMetadata.APIBlock.WRITE.settingName()) + .putNull(IndexMetadata.APIBlock.READ_ONLY.settingName()) + ); + logger.debug("--> but attempts to re-opening [{}] should fail due to the missing block", index); + var ex = expectThrows(ResponseException.class, () -> openIndex(index)); + assertThat(ex.getMessage(), containsString("must be marked as read-only")); + + // TODO this could be randomized once we support recovering verified-before-close closed indices with no write/ro block + addIndexBlock(index, IndexMetadata.APIBlock.WRITE); + } - assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); - assertDocCount(client(), index, numDocs); + var block = indexBlocks(index).stream().filter(c -> c.equals(INDEX_WRITE_BLOCK) || c.equals(INDEX_READ_ONLY_BLOCK)).findFirst(); + if (block.isPresent() && block.get().equals(INDEX_READ_ONLY_BLOCK)) { + logger.debug("--> read_only API block can be replaced by a write block (required for the remaining tests)"); + updateIndexSettings( + index, + Settings.builder() + .putNull(IndexMetadata.APIBlock.READ_ONLY.settingName()) + .put(IndexMetadata.APIBlock.WRITE.settingName(), true) + ); + } - assertIndexBlockExists(index, IndexMetadata.APIBlock.WRITE); - assertIndexBlockNotUpdateable(index, IndexMetadata.APIBlock.WRITE); + assertThat(indexBlocks(index), isClosed ? contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK) : contains(INDEX_WRITE_BLOCK)); + assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(isClosed)); + assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true)); var numberOfReplicas = getNumberOfReplicas(index); if (0 < numberOfReplicas) { @@ -83,69 +147,29 @@ public void testIndexUpgrade() throws Exception { updateIndexSettings(index, Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1)); ensureGreen(index); - logger.debug("--> closing restored index [{}]", index); - closeIndex(index); - ensureGreen(index); + if (isClosed) { + logger.debug("--> re-opening index [{}]", index); + openIndex(index); + ensureGreen(index); - assertIndexBlockNotUpdateable(index, IndexMetadata.APIBlock.WRITE); // test again on closed index + assertDocCount(client(), index, numDocs); + } else { + logger.debug("--> closing index [{}]", index); + closeIndex(index); + ensureGreen(index); + } - logger.debug("--> adding replica to test peer-recovery for closed shards"); + logger.debug("--> adding more replicas to test peer-recovery"); updateIndexSettings(index, Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 2)); ensureGreen(index); - logger.debug("--> re-opening restored index [{}]", index); - openIndex(index); - ensureGreen(index); - - assertDocCount(client(), index, numDocs); - - logger.debug("--> deleting index [{}]", index); - deleteIndex(index); - } - } - - /** - * Similar to {@link #testIndexUpgrade()} but with a read_only block. - */ - public void testIndexUpgradeReadOnlyBlock() throws Exception { - final String index = suffix("index"); - final int numDocs = 2531; - - if (isFullyUpgradedTo(VERSION_MINUS_2)) { - logger.debug("--> creating index [{}]", index); - createIndex( - client(), - index, - Settings.builder() - .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, randomInt(2)) - .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true) - .build() + assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true)); + assertThat( + indexBlocks(index), + isIndexClosed(index) ? contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK) : contains(INDEX_WRITE_BLOCK) ); - logger.debug("--> indexing [{}] docs in [{}]", numDocs, index); - indexDocs(index, numDocs); - return; - } - - if (isFullyUpgradedTo(VERSION_MINUS_1)) { - ensureGreen(index); - - assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); - assertDocCount(client(), index, numDocs); - - addIndexBlock(index, IndexMetadata.APIBlock.READ_ONLY); - return; - } - - if (isFullyUpgradedTo(VERSION_CURRENT)) { - ensureGreen(index); - - assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); - assertDocCount(client(), index, numDocs); - - assertIndexBlockExists(index, IndexMetadata.APIBlock.READ_ONLY); - assertIndexBlockNotUpdateable(index, IndexMetadata.APIBlock.READ_ONLY); + deleteIndex(index); } } @@ -200,7 +224,8 @@ public void testRestoreIndex() throws Exception { restoreIndex(repository, snapshot, index, restoredIndex); ensureGreen(restoredIndex); - assertIndexBlockExists(restoredIndex, IndexMetadata.APIBlock.WRITE); + assertIndexSetting(restoredIndex, VERIFIED_READ_ONLY_SETTING, is(true)); + assertThat(indexBlocks(restoredIndex), contains(INDEX_WRITE_BLOCK)); assertThat(indexVersion(restoredIndex), equalTo(VERSION_MINUS_2)); assertDocCount(client(), restoredIndex, numDocs); @@ -281,7 +306,8 @@ public void testRestoreIndexOverClosedIndex() throws Exception { if (isFullyUpgradedTo(VERSION_CURRENT)) { assertThat(isIndexClosed(index), equalTo(true)); - assertIndexBlockExists(index, IndexMetadata.APIBlock.WRITE); + assertThat(indexBlocks(index), contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK)); + assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true)); logger.debug("--> restoring index [{}] over existing closed index", index); restoreIndex(repository, snapshot, index, index); diff --git a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/RollingUpgradeLuceneIndexCompatibilityTestCase.java b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/RollingUpgradeLuceneIndexCompatibilityTestCase.java index 6296c45a2d5d1..3c8f41a284262 100644 --- a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/RollingUpgradeLuceneIndexCompatibilityTestCase.java +++ b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/RollingUpgradeLuceneIndexCompatibilityTestCase.java @@ -19,8 +19,16 @@ import java.util.List; +import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_READ_ONLY_BLOCK; +import static org.elasticsearch.cluster.metadata.IndexMetadata.INDEX_WRITE_BLOCK; +import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.INDEX_CLOSED_BLOCK; +import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.VERIFIED_BEFORE_CLOSE_SETTING; +import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.either; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; public class RollingUpgradeLuceneIndexCompatibilityTestCase extends RollingUpgradeIndexCompatibilityTestCase { @@ -40,7 +48,6 @@ public void testIndexUpgrade() throws Exception { final int numDocs = 2543; if (isFullyUpgradedTo(VERSION_MINUS_2)) { - logger.debug("--> creating index [{}]", index); createIndex( client(), index, @@ -50,25 +57,62 @@ public void testIndexUpgrade() throws Exception { .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true) .build() ); - - logger.debug("--> indexing [{}] docs in [{}]", numDocs, index); indexDocs(index, numDocs); return; } + assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); ensureGreen(index); - if (isFullyUpgradedTo(VERSION_MINUS_1)) { - assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); + if (isIndexClosed(index) == false) { assertDocCount(client(), index, numDocs); + } - addIndexBlock(index, IndexMetadata.APIBlock.WRITE); + if (isFullyUpgradedTo(VERSION_MINUS_1)) { + final var maybeClose = randomBoolean(); + if (maybeClose) { + logger.debug("--> closing index [{}] before upgrade", index); + closeIndex(index); + } + + final var block = randomFrom(IndexMetadata.APIBlock.WRITE, IndexMetadata.APIBlock.READ_ONLY); + addIndexBlock(index, block); + + assertThat(indexBlocks(index), maybeClose ? contains(INDEX_CLOSED_BLOCK, block.getBlock()) : contains(block.getBlock())); + assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(maybeClose)); + assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true)); return; } if (nodesVersions().values().stream().anyMatch(v -> v.onOrAfter(VERSION_CURRENT))) { - assertIndexBlockExists(index, IndexMetadata.APIBlock.WRITE); - assertIndexBlockNotUpdateable(index, IndexMetadata.APIBlock.WRITE); + final var isClosed = isIndexClosed(index); + logger.debug("--> upgraded index [{}] is now in [{}] state", index, isClosed ? "closed" : "open"); + assertThat( + indexBlocks(index), + isClosed + ? either(contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK)).or(contains(INDEX_CLOSED_BLOCK, INDEX_READ_ONLY_BLOCK)) + : either(contains(INDEX_WRITE_BLOCK)).or(contains(INDEX_READ_ONLY_BLOCK)) + ); + assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(isClosed)); + assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true)); + + var block = indexBlocks(index).stream() + .filter(c -> c.equals(INDEX_WRITE_BLOCK) || c.equals(INDEX_READ_ONLY_BLOCK)) + .findFirst() + .orElseThrow(() -> new AssertionError("Block not found")); + if (block.equals(INDEX_READ_ONLY_BLOCK)) { + logger.debug("--> read_only API block can be replaced by a write block (required for the remaining tests)"); + updateIndexSettings( + index, + Settings.builder() + .putNull(IndexMetadata.APIBlock.READ_ONLY.settingName()) + .put(IndexMetadata.APIBlock.WRITE.settingName(), true) + ); + } + + assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true)); + assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(isClosed)); + assertThat(indexBlocks(index), isClosed ? contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK) : contains(INDEX_WRITE_BLOCK)); if (isIndexClosed(index)) { logger.debug("--> re-opening index [{}] after upgrade", index); @@ -125,8 +169,8 @@ public void testIndexUpgradeReadOnlyBlock() throws Exception { } if (nodesVersions().values().stream().anyMatch(v -> v.onOrAfter(VERSION_CURRENT))) { - assertIndexBlockExists(index, IndexMetadata.APIBlock.READ_ONLY); - assertIndexBlockNotUpdateable(index, IndexMetadata.APIBlock.READ_ONLY); + assertThat(indexBlocks(index), contains(INDEX_READ_ONLY_BLOCK)); + assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true)); assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); assertDocCount(client(), index, numDocs); @@ -186,8 +230,15 @@ public void testRestoreIndex() throws Exception { restoreIndex(repository, snapshot, index, restoredIndex); ensureGreen(restoredIndex); - assertIndexBlockExists(restoredIndex, IndexMetadata.APIBlock.WRITE); - assertIndexBlockNotUpdateable(restoredIndex, IndexMetadata.APIBlock.WRITE); + assertThat(indexBlocks(restoredIndex), contains(INDEX_WRITE_BLOCK)); + assertIndexSetting(restoredIndex, VERIFIED_READ_ONLY_SETTING, is(true)); + + var ex = expectUpdateIndexSettingsThrows( + restoredIndex, + Settings.builder().putNull(IndexMetadata.APIBlock.WRITE.settingName()) + ); + assertThat(ex.getMessage(), containsStringCannotRemoveBlockOnReadOnlyIndex(restoredIndex)); + assertThat(indexVersion(restoredIndex), equalTo(VERSION_MINUS_2)); assertDocCount(client(), restoredIndex, numDocs); @@ -198,7 +249,14 @@ public void testRestoreIndex() throws Exception { closeIndex(restoredIndex); ensureGreen(restoredIndex); - assertIndexBlockNotUpdateable(restoredIndex, IndexMetadata.APIBlock.WRITE); // test again on closed index + logger.debug("--> write API block can be removed on a closed index: INDEX_CLOSED_BLOCK already blocks writes"); + updateIndexSettings(restoredIndex, Settings.builder().putNull(IndexMetadata.APIBlock.WRITE.settingName())); + + logger.debug("--> but attempts to re-opening [{}] should fail due to the missing block", restoredIndex); + ex = expectThrows(ResponseException.class, () -> openIndex(restoredIndex)); + assertThat(ex.getMessage(), containsString("must be marked as read-only")); + + addIndexBlock(restoredIndex, IndexMetadata.APIBlock.WRITE); logger.debug("--> re-opening restored index [{}]", restoredIndex); openIndex(restoredIndex); diff --git a/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java index 9a7a77bf77a87..dc4dfc88b2c12 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java @@ -13,6 +13,7 @@ import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.admin.indices.settings.get.GetSettingsResponse; import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.block.ClusterBlockException; import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Priority; @@ -23,6 +24,7 @@ import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.IndexModule; import org.elasticsearch.index.IndexService; +import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.VersionType; import org.elasticsearch.index.engine.VersionConflictEngineException; import org.elasticsearch.indices.IndicesService; @@ -720,4 +722,35 @@ private void assertEqualsAndStringsInterned(List queryFieldsSetting, Set } } + public void testMultipleSettingsUpdateWithMetadataWriteBlock() { + final var indexName = randomIdentifier(); + createIndex(indexName, Settings.builder().put(IndexMetadata.APIBlock.READ_ONLY.settingName(), true).build()); + + // Metadata writes are blocked by the READ_ONLY block + expectThrows( + ClusterBlockException.class, + () -> updateIndexSettings(Settings.builder().put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), "12s"), indexName) + ); + + var randomSetting = randomFrom(IndexMetadata.APIBlock.READ_ONLY, IndexMetadata.APIBlock.READ_ONLY_ALLOW_DELETE).settingName(); + updateIndexSettings( + Settings.builder() + .put(randomSetting, true) // still has the metadata write block... + .put(IndexMetadata.APIBlock.WRITE.settingName(), true) + .put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), "12s"), // should not be allowed + indexName + ); + + assertThat( + indicesAdmin().prepareGetSettings(indexName) + .get() + .getIndexToSettings() + .get(indexName) + .get(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey()), + equalTo("12s") + ); + + // Updating the setting alone should always work + updateIndexSettings(Settings.builder().put(IndexMetadata.APIBlock.READ_ONLY.settingName(), false)); + } } diff --git a/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java b/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java index aa3a6a201eac4..660ce9f4d0b51 100644 --- a/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java +++ b/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java @@ -398,6 +398,10 @@ public boolean hasIndexBlock(String index, ClusterBlock block) { return indices.getOrDefault(index, Set.of()).contains(block); } + public boolean hasIndexBlockLevel(String index, ClusterBlockLevel level) { + return indices.getOrDefault(index, Set.of()).stream().anyMatch(clusterBlock -> clusterBlock.contains(level)); + } + public Builder removeIndexBlock(String index, ClusterBlock block) { if (indices.containsKey(index) == false) { return this; diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index c4643b2727218..2cc00a923909c 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -36,10 +36,10 @@ import org.elasticsearch.common.settings.IndexScopedSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.Nullable; import org.elasticsearch.core.TimeValue; import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexSettings; -import org.elasticsearch.index.IndexVersion; import org.elasticsearch.indices.IndicesService; import org.elasticsearch.indices.ShardLimitValidator; import org.elasticsearch.threadpool.ThreadPool; @@ -52,7 +52,9 @@ import java.util.Objects; import java.util.Set; import java.util.function.BiFunction; +import java.util.function.Function; +import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING; import static org.elasticsearch.index.IndexSettings.same; /** @@ -182,11 +184,14 @@ ClusterState execute(ClusterState currentState) { RoutingTable.Builder routingTableBuilder = null; Metadata.Builder metadataBuilder = Metadata.builder(currentState.metadata()); + final var minSupportedIndexVersion = currentState.nodes().getMinSupportedIndexVersion(); // allow to change any settings to a closed index, and only allow dynamic settings to be changed // on an open index Set openIndices = new HashSet<>(); Set closedIndices = new HashSet<>(); + Set readOnlyIndices = null; + final String[] actualIndices = new String[request.indices().length]; for (int i = 0; i < request.indices().length; i++) { Index index = request.indices()[i]; @@ -198,6 +203,12 @@ ClusterState execute(ClusterState currentState) { } else { closedIndices.add(index); } + if (metadata.getCompatibilityVersion().before(minSupportedIndexVersion)) { + if (readOnlyIndices == null) { + readOnlyIndices = new HashSet<>(); + } + readOnlyIndices.add(index); + } } if (skippedSettings.isEmpty() == false && openIndices.isEmpty() == false) { @@ -328,6 +339,9 @@ ClusterState execute(ClusterState currentState) { } } + final Function verifiedReadOnly = indexName -> VERIFIED_READ_ONLY_SETTING.get( + currentState.metadata().index(indexName).getSettings() + ); final ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks()); boolean changedBlocks = false; for (IndexMetadata.APIBlock block : IndexMetadata.APIBlock.values()) { @@ -338,7 +352,7 @@ ClusterState execute(ClusterState currentState) { block.setting, openSettings, metadataBuilder, - currentState.nodes().getMinSupportedIndexVersion() + verifiedReadOnly ); } changed |= changedBlocks; @@ -368,6 +382,7 @@ ClusterState execute(ClusterState currentState) { // This step is mandatory since we allow to update non-dynamic settings on closed indices. indicesService.verifyIndexMetadata(updatedMetadata, updatedMetadata); } + verifyReadOnlyIndices(readOnlyIndices, updatedState.blocks()); } catch (IOException ex) { throw ExceptionsHelper.convertToElastic(ex); } @@ -426,6 +441,24 @@ public static void updateIndexSettings( } } + /** + * Verifies that read-only compatible indices always have a write block. + * + * @param readOnlyIndices the read-only compatible indices + * @param blocks the updated cluster state blocks + */ + private static void verifyReadOnlyIndices(@Nullable Set readOnlyIndices, ClusterBlocks blocks) { + if (readOnlyIndices != null) { + for (Index readOnlyIndex : readOnlyIndices) { + if (blocks.indexBlocked(ClusterBlockLevel.WRITE, readOnlyIndex.getName()) == false) { + throw new IllegalArgumentException( + String.format(Locale.ROOT, "Can't remove the write block on read-only compatible index %s", readOnlyIndex) + ); + } + } + } + } + /** * Updates the cluster block only iff the setting exists in the given settings */ @@ -436,7 +469,7 @@ private static boolean maybeUpdateClusterBlock( Setting setting, Settings openSettings, Metadata.Builder metadataBuilder, - IndexVersion minSupportedIndexVersion + Function verifiedReadOnlyBeforeBlockChanges ) { boolean changed = false; if (setting.exists(openSettings)) { @@ -446,27 +479,32 @@ private static boolean maybeUpdateClusterBlock( if (blocks.hasIndexBlock(index, block) == false) { blocks.addIndexBlock(index, block); changed = true; + if (block.contains(ClusterBlockLevel.WRITE)) { + var isVerifiedReadOnly = verifiedReadOnlyBeforeBlockChanges.apply(index); + if (isVerifiedReadOnly) { + var indexMetadata = metadataBuilder.get(index); + metadataBuilder.put( + IndexMetadata.builder(indexMetadata) + .settings( + Settings.builder() + .put(indexMetadata.getSettings()) + .put(VERIFIED_READ_ONLY_SETTING.getKey(), true) + ) + ); + } + } } } else { if (blocks.hasIndexBlock(index, block)) { blocks.removeIndexBlock(index, block); changed = true; if (block.contains(ClusterBlockLevel.WRITE)) { - IndexMetadata indexMetadata = metadataBuilder.get(index); - if (indexMetadata.getCompatibilityVersion().before(minSupportedIndexVersion)) { - // Forbid the removal of the block if the index cannot be written by one or more nodes of the cluster - throw new IllegalArgumentException( - String.format( - Locale.ROOT, - "Can't update setting [%s] for read-only compatible index %s", - setting.getKey(), - indexMetadata.getIndex() - ) - ); + if (blocks.hasIndexBlockLevel(index, ClusterBlockLevel.WRITE) == false) { + var indexMetadata = metadataBuilder.get(index); + var indexSettings = Settings.builder().put(indexMetadata.getSettings()); + indexSettings.remove(VERIFIED_READ_ONLY_SETTING.getKey()); + metadataBuilder.put(IndexMetadata.builder(indexMetadata).settings(indexSettings)); } - Settings.Builder indexSettings = Settings.builder().put(indexMetadata.getSettings()); - indexSettings.remove(MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING.getKey()); - metadataBuilder.put(IndexMetadata.builder(indexMetadata).settings(indexSettings)); } } } diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java index 2647e21d34bc5..bedddd4f381f5 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java @@ -1929,7 +1929,7 @@ protected static Map getIndexSettings(String index, boolean incl } @SuppressWarnings("unchecked") - protected Map getIndexSettingsAsMap(String index) throws IOException { + protected static Map getIndexSettingsAsMap(String index) throws IOException { Map indexSettings = getIndexSettings(index); return (Map) ((Map) indexSettings.get(index)).get("settings"); } From de6a0bdb02283850083ed6d633577643dd5f33fb Mon Sep 17 00:00:00 2001 From: tlrx Date: Tue, 28 Jan 2025 14:20:45 +0100 Subject: [PATCH 3/4] feedback --- .../java/org/elasticsearch/cluster/block/ClusterBlocks.java | 4 ++++ .../cluster/metadata/MetadataUpdateSettingsService.java | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java b/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java index 660ce9f4d0b51..659e78f99c21a 100644 --- a/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java +++ b/server/src/main/java/org/elasticsearch/cluster/block/ClusterBlocks.java @@ -146,6 +146,10 @@ public boolean hasIndexBlock(String index, ClusterBlock block) { return indicesBlocks.containsKey(index) && indicesBlocks.get(index).contains(block); } + public boolean hasIndexBlockLevel(String index, ClusterBlockLevel level) { + return blocksForIndex(level, index).isEmpty() == false; + } + public boolean hasIndexBlockWithId(String index, int blockId) { final Set clusterBlocks = indicesBlocks.get(index); if (clusterBlocks != null) { diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index 2cc00a923909c..c11fa06d83c4d 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -450,7 +450,7 @@ public static void updateIndexSettings( private static void verifyReadOnlyIndices(@Nullable Set readOnlyIndices, ClusterBlocks blocks) { if (readOnlyIndices != null) { for (Index readOnlyIndex : readOnlyIndices) { - if (blocks.indexBlocked(ClusterBlockLevel.WRITE, readOnlyIndex.getName()) == false) { + if (blocks.hasIndexBlockLevel(readOnlyIndex.getName(), ClusterBlockLevel.WRITE) == false) { throw new IllegalArgumentException( String.format(Locale.ROOT, "Can't remove the write block on read-only compatible index %s", readOnlyIndex) ); From 59e7acddb8addaeda32172a3ae90ece223f6cde4 Mon Sep 17 00:00:00 2001 From: tlrx Date: Tue, 28 Jan 2025 14:20:48 +0100 Subject: [PATCH 4/4] feedback --- ...gradeLuceneIndexCompatibilityTestCase.java | 114 +++++++++--------- 1 file changed, 59 insertions(+), 55 deletions(-) diff --git a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/RollingUpgradeLuceneIndexCompatibilityTestCase.java b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/RollingUpgradeLuceneIndexCompatibilityTestCase.java index 3c8f41a284262..98054cb4b4f39 100644 --- a/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/RollingUpgradeLuceneIndexCompatibilityTestCase.java +++ b/qa/lucene-index-compatibility/src/javaRestTest/java/org/elasticsearch/lucene/RollingUpgradeLuceneIndexCompatibilityTestCase.java @@ -24,11 +24,14 @@ import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.INDEX_CLOSED_BLOCK; import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.VERIFIED_BEFORE_CLOSE_SETTING; import static org.elasticsearch.cluster.metadata.MetadataIndexStateService.VERIFIED_READ_ONLY_SETTING; +import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.either; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; public class RollingUpgradeLuceneIndexCompatibilityTestCase extends RollingUpgradeIndexCompatibilityTestCase { @@ -75,10 +78,17 @@ public void testIndexUpgrade() throws Exception { closeIndex(index); } - final var block = randomFrom(IndexMetadata.APIBlock.WRITE, IndexMetadata.APIBlock.READ_ONLY); - addIndexBlock(index, block); + final var randomBlocks = randomFrom( + List.of(IndexMetadata.APIBlock.WRITE, IndexMetadata.APIBlock.READ_ONLY), + List.of(IndexMetadata.APIBlock.READ_ONLY), + List.of(IndexMetadata.APIBlock.WRITE) + ); + for (var randomBlock : randomBlocks) { + addIndexBlock(index, randomBlock); + assertThat(indexBlocks(index), hasItem(randomBlock.getBlock())); + } - assertThat(indexBlocks(index), maybeClose ? contains(INDEX_CLOSED_BLOCK, block.getBlock()) : contains(block.getBlock())); + assertThat(indexBlocks(index), maybeClose ? hasItem(INDEX_CLOSED_BLOCK) : not(hasItem(INDEX_CLOSED_BLOCK))); assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(maybeClose)); assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true)); return; @@ -89,18 +99,55 @@ public void testIndexUpgrade() throws Exception { logger.debug("--> upgraded index [{}] is now in [{}] state", index, isClosed ? "closed" : "open"); assertThat( indexBlocks(index), - isClosed - ? either(contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK)).or(contains(INDEX_CLOSED_BLOCK, INDEX_READ_ONLY_BLOCK)) - : either(contains(INDEX_WRITE_BLOCK)).or(contains(INDEX_READ_ONLY_BLOCK)) + allOf( + either(hasItem(INDEX_READ_ONLY_BLOCK)).or(hasItem(INDEX_WRITE_BLOCK)), + isClosed ? hasItem(INDEX_CLOSED_BLOCK) : not(hasItem(INDEX_CLOSED_BLOCK)) + ) ); assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(isClosed)); assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true)); - var block = indexBlocks(index).stream() - .filter(c -> c.equals(INDEX_WRITE_BLOCK) || c.equals(INDEX_READ_ONLY_BLOCK)) - .findFirst() - .orElseThrow(() -> new AssertionError("Block not found")); - if (block.equals(INDEX_READ_ONLY_BLOCK)) { + var blocks = indexBlocks(index).stream().filter(c -> c.equals(INDEX_WRITE_BLOCK) || c.equals(INDEX_READ_ONLY_BLOCK)).toList(); + if (blocks.size() == 2) { + switch (randomInt(2)) { + case 0: + updateIndexSettings( + index, + Settings.builder() + .putNull(IndexMetadata.APIBlock.WRITE.settingName()) + .put(IndexMetadata.APIBlock.READ_ONLY.settingName(), true) + ); + assertThat( + indexBlocks(index), + isClosed ? contains(INDEX_CLOSED_BLOCK, INDEX_READ_ONLY_BLOCK) : contains(INDEX_READ_ONLY_BLOCK) + ); + break; + case 1: + updateIndexSettings( + index, + Settings.builder() + .putNull(IndexMetadata.APIBlock.READ_ONLY.settingName()) + .put(IndexMetadata.APIBlock.WRITE.settingName(), true) + ); + assertThat( + indexBlocks(index), + isClosed ? contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK) : contains(INDEX_WRITE_BLOCK) + ); + break; + case 2: + updateIndexSettings(index, Settings.builder().put(IndexMetadata.APIBlock.READ_ONLY.settingName(), false)); + assertThat( + indexBlocks(index), + isClosed ? contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK) : contains(INDEX_WRITE_BLOCK) + ); + break; + default: + throw new AssertionError(); + } + } + + blocks = indexBlocks(index).stream().filter(c -> c.equals(INDEX_WRITE_BLOCK) || c.equals(INDEX_READ_ONLY_BLOCK)).toList(); + if (blocks.contains(INDEX_READ_ONLY_BLOCK)) { logger.debug("--> read_only API block can be replaced by a write block (required for the remaining tests)"); updateIndexSettings( index, @@ -114,7 +161,7 @@ public void testIndexUpgrade() throws Exception { assertIndexSetting(index, VERIFIED_BEFORE_CLOSE_SETTING, is(isClosed)); assertThat(indexBlocks(index), isClosed ? contains(INDEX_CLOSED_BLOCK, INDEX_WRITE_BLOCK) : contains(INDEX_WRITE_BLOCK)); - if (isIndexClosed(index)) { + if (isClosed) { logger.debug("--> re-opening index [{}] after upgrade", index); openIndex(index); ensureGreen(index); @@ -134,49 +181,6 @@ public void testIndexUpgrade() throws Exception { } } - /** - * Similar to {@link #testIndexUpgrade()} but with a read_only block. - */ - public void testIndexUpgradeReadOnlyBlock() throws Exception { - final String index = suffix("index-"); - final int numDocs = 2573; - - if (isFullyUpgradedTo(VERSION_MINUS_2)) { - logger.debug("--> creating index [{}]", index); - createIndex( - client(), - index, - Settings.builder() - .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) - .put(IndexSettings.INDEX_SOFT_DELETES_SETTING.getKey(), true) - .build() - ); - - logger.debug("--> indexing [{}] docs in [{}]", numDocs, index); - indexDocs(index, numDocs); - return; - } - - ensureGreen(index); - - if (isFullyUpgradedTo(VERSION_MINUS_1)) { - assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); - assertDocCount(client(), index, numDocs); - - addIndexBlock(index, IndexMetadata.APIBlock.READ_ONLY); - return; - } - - if (nodesVersions().values().stream().anyMatch(v -> v.onOrAfter(VERSION_CURRENT))) { - assertThat(indexBlocks(index), contains(INDEX_READ_ONLY_BLOCK)); - assertIndexSetting(index, VERIFIED_READ_ONLY_SETTING, is(true)); - - assertThat(indexVersion(index), equalTo(VERSION_MINUS_2)); - assertDocCount(client(), index, numDocs); - } - } - /** * Creates an index on N-2, marks as read-only on N-1 and creates a snapshot, then restores the snapshot during rolling upgrades to N. */