Remove frozen cache setting leniency (#71013)

We previously allowed but deprecated the ability for the shared cache to
be positively sized on nodes without the frozen role. This is because we
only allocate shared_cache searchable snapshots to nodes with the frozen
role. This commit completes our intention to deprecate/remove this
ability.

docs/reference/migration/migrate_8_0/settings.asciidoc

@@ -167,6 +167,22 @@ Discontinue use of the removed settings. If needed, use
 cluster recovery pending a certain number of data nodes.
 ====
 
+.Setting the searchable snapshots shared cache size on non-frozen nodes is no
+longer permitted.
+[%collapsible]
+====
+*Details* +
+Setting `xpack.searchable.snapshot.shared_cache.size` to be positive on a node
+that does not have the `data_frozen` role was deprecated in {es} 7.12.0 and has
+been removed in {es} 8.0.0.
+
+*Impact* +
+Discontinue use of the removed setting. Note that searchable snapshots mounted
+using the `shared_cache` storage option were only allocated to nodes that had
+the `data_frozen` role, so removing this setting on nodes that do not have the
+`data_frozen` role will have no impact on functionality.
+====
+
 .By default, destructive index actions do not allow wildcards.
 [%collapsible]
 ====

docs/reference/searchable-snapshots/index.asciidoc

@@ -171,9 +171,7 @@ You can configure the setting in `elasticsearch.yml`:
 xpack.searchable.snapshot.shared_cache.size: 4TB
 ----
 
-IMPORTANT: Currently, you can configure
-`xpack.searchable.snapshot.shared_cache.size` on any node. In a future release,
-you will only be able to configure this setting on nodes with the
+IMPORTANT: You can only configure this setting on nodes with the
 <<data-frozen-node,`data_frozen`>> role.
 
 You can set `xpack.searchable.snapshot.shared_cache.size` to any size between a

x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/BaseSearchableSnapshotsIntegTestCase.java

@@ -16,7 +16,8 @@ package org.elasticsearch.xpack.searchablesnapshots;
 
 import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotResponse;
 import org.elasticsearch.action.index.IndexRequestBuilder;
-import org.elasticsearch.xpack.searchablesnapshots.cache.blob.BlobStoreCacheService;
+import org.elasticsearch.cluster.node.DiscoveryNode;
+import org.elasticsearch.cluster.node.DiscoveryNodeRole;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.ByteSizeUnit;
@@ -24,9 +25,11 @@ import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.snapshots.AbstractSnapshotIntegTestCase;
+import org.elasticsearch.test.ESIntegTestCase;
 import org.elasticsearch.xpack.core.searchablesnapshots.MountSearchableSnapshotAction;
 import org.elasticsearch.xpack.core.searchablesnapshots.MountSearchableSnapshotRequest;
 import org.elasticsearch.xpack.core.searchablesnapshots.MountSearchableSnapshotRequest.Storage;
+import org.elasticsearch.xpack.searchablesnapshots.cache.blob.BlobStoreCacheService;
 import org.elasticsearch.xpack.searchablesnapshots.cache.full.CacheService;
 import org.elasticsearch.xpack.searchablesnapshots.cache.shared.FrozenCacheService;
 import org.junit.After;
@@ -35,13 +38,16 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Locale;
+import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
 import static org.elasticsearch.license.LicenseService.SELF_GENERATED_LICENSE_TYPE;
+import static org.elasticsearch.test.NodeRoles.addRoles;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
 import static org.elasticsearch.xpack.searchablesnapshots.cache.shared.SharedBytes.pageAligned;
 import static org.hamcrest.Matchers.equalTo;
 
+@ESIntegTestCase.ClusterScope(supportsDedicatedMasters = false, numClientNodes = 0)
 public abstract class BaseSearchableSnapshotsIntegTestCase extends AbstractSnapshotIntegTestCase {
     @Override
     protected boolean addMockInternalEngine() {
@@ -55,9 +61,16 @@ public abstract class BaseSearchableSnapshotsIntegTestCase extends AbstractSnaps
 
     @Override
     protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) {
-        final Settings.Builder builder = Settings.builder()
-            .put(super.nodeSettings(nodeOrdinal, otherSettings))
-            .put(SELF_GENERATED_LICENSE_TYPE.getKey(), "trial");
+        final Settings settings;
+        {
+            final Settings initialSettings = super.nodeSettings(nodeOrdinal, otherSettings);
+            if (DiscoveryNode.canContainData(otherSettings)) {
+                settings = addRoles(initialSettings, Set.of(DiscoveryNodeRole.DATA_FROZEN_NODE_ROLE));
+            } else {
+                settings = initialSettings;
+            }
+        }
+        final Settings.Builder builder = Settings.builder().put(settings).put(SELF_GENERATED_LICENSE_TYPE.getKey(), "trial");
         if (randomBoolean()) {
             builder.put(
                 CacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(),
@@ -76,14 +89,16 @@ public abstract class BaseSearchableSnapshotsIntegTestCase extends AbstractSnaps
                     : new ByteSizeValue(randomIntBetween(1, 10), ByteSizeUnit.MB)
             );
         }
-        builder.put(
-            FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(),
-            rarely()
-                ? randomBoolean()
-                    ? new ByteSizeValue(randomIntBetween(0, 10), ByteSizeUnit.KB)
-                    : new ByteSizeValue(randomIntBetween(0, 1000), ByteSizeUnit.BYTES)
-                : new ByteSizeValue(randomIntBetween(1, 10), ByteSizeUnit.MB)
-        );
+        if (DiscoveryNode.canContainData(otherSettings)) {
+            builder.put(
+                FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(),
+                rarely()
+                    ? randomBoolean()
+                        ? new ByteSizeValue(randomIntBetween(0, 10), ByteSizeUnit.KB)
+                        : new ByteSizeValue(randomIntBetween(0, 1000), ByteSizeUnit.BYTES)
+                    : new ByteSizeValue(randomIntBetween(1, 10), ByteSizeUnit.MB)
+            );
+        }
         builder.put(
             FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(),
             rarely()

x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsCanMatchOnCoordinatorIntegTests.java

@@ -14,6 +14,7 @@ import org.elasticsearch.action.search.SearchRequest;
 import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.cluster.metadata.DataStream;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
+import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.ByteSizeUnit;
@@ -64,14 +65,19 @@ public class SearchableSnapshotsCanMatchOnCoordinatorIntegTests extends BaseSear
     }
 
     @Override
-    protected Settings nodeSettings(int nodeOrdinal, Settings nodeSettings) {
-        return Settings.builder()
-            .put(super.nodeSettings(nodeOrdinal, nodeSettings))
-            // Use an unbound cache so we can recover the searchable snapshot completely all the times
-            .put(CacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), new ByteSizeValue(Long.MAX_VALUE, ByteSizeUnit.BYTES))
-            // Have a shared cache of reasonable size available on each node because tests randomize over frozen and cold allocation
-            .put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), ByteSizeValue.ofMb(randomLongBetween(1, 10)))
-            .build();
+    protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) {
+        final Settings initialSettings = super.nodeSettings(nodeOrdinal, otherSettings);
+        if (DiscoveryNode.canContainData(otherSettings)) {
+            return Settings.builder()
+                .put(initialSettings)
+                // Use an unbound cache so we can recover the searchable snapshot completely all the times
+                .put(CacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), new ByteSizeValue(Long.MAX_VALUE, ByteSizeUnit.BYTES))
+                // Have a shared cache of reasonable size available on each node because tests randomize over frozen and cold allocation
+                .put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), ByteSizeValue.ofMb(randomLongBetween(1, 10)))
+                .build();
+        } else {
+            return initialSettings;
+        }
     }
 
     public void testSearchableSnapshotShardsAreSkippedWithoutQueryingAnyNodeWhenTheyAreOutsideOfTheQueryRange() throws Exception {

x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/shared/FrozenCacheService.java

@@ -16,7 +16,6 @@ import org.elasticsearch.action.StepListener;
 import org.elasticsearch.cluster.node.DiscoveryNodeRole;
 import org.elasticsearch.common.lease.Releasable;
 import org.elasticsearch.common.lease.Releasables;
-import org.elasticsearch.common.logging.DeprecationCategory;
 import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.settings.Settings;
@@ -111,10 +110,8 @@ public class FrozenCacheService implements Releasable {
                     @SuppressWarnings("unchecked")
                     final List<DiscoveryNodeRole> roles = (List<DiscoveryNodeRole>) settings.get(NodeRoleSettings.NODE_ROLES_SETTING);
                     if (DataTier.isFrozenNode(Set.of(roles.toArray(DiscoveryNodeRole[]::new))) == false) {
-                        deprecationLogger.deprecate(
-                            DeprecationCategory.SETTINGS,
-                            "shared_cache",
-                            "setting [{}] to be positive [{}] on node without the data_frozen role is deprecated, roles are [{}]",
+                        throw new SettingsException(
+                            "setting [{}] to be positive [{}] is only permitted on nodes with the data_frozen role, roles are [{}]",
                             SHARED_CACHE_SETTINGS_PREFIX + "size",
                             value.getStringRep(),
                             roles.stream().map(DiscoveryNodeRole::roleName).collect(Collectors.joining(","))

x-pack/plugin/searchable-snapshots/src/test/java/org/elasticsearch/xpack/searchablesnapshots/cache/shared/FrozenCacheServiceTests.java

@@ -10,6 +10,7 @@ package org.elasticsearch.xpack.searchablesnapshots.cache.shared;
 import org.elasticsearch.cluster.coordination.DeterministicTaskQueue;
 import org.elasticsearch.cluster.node.DiscoveryNodeRole;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.settings.SettingsException;
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.env.NodeEnvironment;
 import org.elasticsearch.env.TestEnvironment;
@@ -24,6 +25,9 @@ import org.elasticsearch.xpack.searchablesnapshots.cache.shared.FrozenCacheServi
 import java.io.IOException;
 
 import static org.elasticsearch.node.Node.NODE_NAME_SETTING;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
 
 public class FrozenCacheServiceTests extends ESTestCase {
 
@@ -194,19 +198,27 @@ public class FrozenCacheServiceTests extends ESTestCase {
         }
     }
 
-    public void testCacheSizeDeprecatedOnNonFrozenNodes() {
+    public void testCacheSizeRejectedOnNonFrozenNodes() {
         final Settings settings = Settings.builder()
             .put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), new ByteSizeValue(size(500)).getStringRep())
             .put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), new ByteSizeValue(size(100)).getStringRep())
             .putList(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), DiscoveryNodeRole.DATA_HOT_NODE_ROLE.roleName())
             .build();
-        FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.get(settings);
-        assertWarnings(
-            "setting ["
-                + FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey()
-                + "] to be positive ["
-                + new ByteSizeValue(size(500)).getStringRep()
-                + "] on node without the data_frozen role is deprecated, roles are [data_hot]"
+        final IllegalArgumentException e = expectThrows(
+            IllegalArgumentException.class,
+            () -> FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.get(settings)
+        );
+        assertThat(e.getCause(), notNullValue());
+        assertThat(e.getCause(), instanceOf(SettingsException.class));
+        assertThat(
+            e.getCause().getMessage(),
+            is(
+                "setting ["
+                    + FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey()
+                    + "] to be positive ["
+                    + new ByteSizeValue(size(500)).getStringRep()
+                    + "] is only permitted on nodes with the data_frozen role, roles are [data_hot]"
+            )
         );
     }