Guard file settings readiness on file settings support (elastic#109500)

Consistency of file settings is an important invariant. However, when
upgrading from Elasticsearch versions before file settings existed,
cluster state will not yet have the file settings metadata. If the first
node upgraded is not the master node, new nodes will never become ready
while they wait for file settings metadata to exist.

This commit adds a node feature for file settings to guard waiting on
file settings for readiness. Although file settings has existed since
8.4, the feature is not a historical feature because historical features
are not applied to cluster state that readiness checks. In this case it
is not needed since upgrading from 8.4+ will already contain file
settings metadata.

docs/changelog/109500.yaml

@@ -0,0 +1,5 @@
+pr: 109500
+summary: Guard file settings readiness on file settings support
+area: Infra/Settings
+type: bug
+issues: []

server/src/main/java/module-info.java

@@ -430,7 +430,8 @@
             org.elasticsearch.indices.IndicesFeatures,
             org.elasticsearch.action.admin.cluster.allocation.AllocationStatsFeatures,
             org.elasticsearch.index.mapper.MapperFeatures,
-            org.elasticsearch.search.retriever.RetrieversFeatures;
+            org.elasticsearch.search.retriever.RetrieversFeatures,
+            org.elasticsearch.reservedstate.service.FileSettingsFeatures;
 
     uses org.elasticsearch.plugins.internal.SettingsExtension;
     uses RestExtension;

server/src/main/java/org/elasticsearch/readiness/ReadinessService.java

@@ -21,7 +21,9 @@
 import org.elasticsearch.common.settings.Setting;
 import org.elasticsearch.common.transport.BoundTransportAddress;
 import org.elasticsearch.common.transport.TransportAddress;
+import org.elasticsearch.core.SuppressForbidden;
 import org.elasticsearch.env.Environment;
+import org.elasticsearch.reservedstate.service.FileSettingsFeatures;
 import org.elasticsearch.reservedstate.service.FileSettingsService;
 import org.elasticsearch.shutdown.PluginShutdownService;
 import org.elasticsearch.transport.BindTransportException;
@@ -277,7 +279,22 @@ private boolean isMasterElected(ClusterState clusterState) {
     // protected to allow mock service to override
     protected boolean areFileSettingsApplied(ClusterState clusterState) {
         ReservedStateMetadata fileSettingsMetadata = clusterState.metadata().reservedStateMetadata().get(FileSettingsService.NAMESPACE);
-        return fileSettingsMetadata != null && fileSettingsMetadata.version().equals(ReservedStateMetadata.NO_VERSION) == false;
+        if (fileSettingsMetadata == null) {
+            // In order to block readiness on file settings being applied, we need to know that the master node has written an initial
+            // version, or a marker that file settings don't exist. When upgrading from a version that did not have file settings, the
+            // current master node may not be the first node upgraded. To be safe, we wait to consider file settings application for
+            // readiness until the whole cluster supports file settings. Note that this only applies when no reserved state metadata
+            // exists, so either we are starting up a current cluster (and the feature will be found) or we are upgrading from
+            // a version before file settings existed (before 8.4).
+            return supportsFileSettings(clusterState) == false;
+        } else {
+            return fileSettingsMetadata.version().equals(ReservedStateMetadata.NO_VERSION) == false;
+        }
+    }
+
+    @SuppressForbidden(reason = "need to check file settings support on exact cluster state")
+    private static boolean supportsFileSettings(ClusterState clusterState) {
+        return clusterState.clusterFeatures().clusterHasFeature(FileSettingsFeatures.FILE_SETTINGS_SUPPORTED);
     }
 
     private void setReady(boolean ready) {

server/src/main/java/org/elasticsearch/reservedstate/service/FileSettingsFeatures.java

@@ -0,0 +1,26 @@
+/*
+ * 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 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.reservedstate.service;
+
+import org.elasticsearch.features.FeatureSpecification;
+import org.elasticsearch.features.NodeFeature;
+
+import java.util.Set;
+
+public class FileSettingsFeatures implements FeatureSpecification {
+
+    // Although file settings were supported starting in 8.4.0, this is really about whether file settings
+    // are used in readiness.
+    public static final NodeFeature FILE_SETTINGS_SUPPORTED = new NodeFeature("file_settings");
+
+    @Override
+    public Set<NodeFeature> getFeatures() {
+        return Set.of(FILE_SETTINGS_SUPPORTED);
+    }
+}

server/src/main/resources/META-INF/services/org.elasticsearch.features.FeatureSpecification

@@ -15,3 +15,4 @@ org.elasticsearch.indices.IndicesFeatures
 org.elasticsearch.action.admin.cluster.allocation.AllocationStatsFeatures
 org.elasticsearch.index.mapper.MapperFeatures
 org.elasticsearch.search.retriever.RetrieversFeatures
+org.elasticsearch.reservedstate.service.FileSettingsFeatures

server/src/test/java/org/elasticsearch/readiness/ReadinessServiceTests.java

@@ -31,6 +31,7 @@
 import org.elasticsearch.http.HttpInfo;
 import org.elasticsearch.http.HttpServerTransport;
 import org.elasticsearch.http.HttpStats;
+import org.elasticsearch.reservedstate.service.FileSettingsFeatures;
 import org.elasticsearch.reservedstate.service.FileSettingsService;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.MockLog;
@@ -46,6 +47,7 @@
 import java.nio.channels.ServerSocketChannel;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 import static org.elasticsearch.cluster.metadata.ReservedStateErrorMetadata.ErrorKind.TRANSIENT;
@@ -57,6 +59,7 @@ public class ReadinessServiceTests extends ESTestCase implements ReadinessClient
     private ThreadPool threadpool;
     private Environment env;
     private FakeHttpTransport httpTransport;
+    private static final Set<String> nodeFeatures = Set.of(FileSettingsFeatures.FILE_SETTINGS_SUPPORTED.id());
 
     private static Metadata emptyReservedStateMetadata;
     static {
@@ -205,21 +208,8 @@ public void testStatusChange() throws Exception {
         // initially the service isn't ready
         assertFalse(readinessService.ready());
 
-        ClusterState emptyState = ClusterState.builder(new ClusterName("cluster"))
-            .nodes(
-                DiscoveryNodes.builder().add(DiscoveryNodeUtils.create("node2", new TransportAddress(TransportAddress.META_ADDRESS, 9201)))
-            )
-            .build();
-
-        ClusterState noFileSettingsState = ClusterState.builder(emptyState)
-            .nodes(
-                DiscoveryNodes.builder(emptyState.nodes())
-                    .add(httpTransport.node)
-                    .masterNodeId(httpTransport.node.getId())
-                    .localNodeId(httpTransport.node.getId())
-            )
-            .build();
-        ClusterChangedEvent event = new ClusterChangedEvent("test", noFileSettingsState, emptyState);
+        ClusterState noFileSettingsState = noFileSettingsState();
+        ClusterChangedEvent event = new ClusterChangedEvent("test", noFileSettingsState, emptyState());
         readinessService.clusterChanged(event);
 
         // sending a cluster state with active master should not yet bring up the service, file settings still are not applied
@@ -306,14 +296,7 @@ public void testFileSettingsUpdateError() throws Exception {
 
         var fileSettingsState = new ReservedStateMetadata.Builder(FileSettingsService.NAMESPACE).version(21L)
             .errorMetadata(new ReservedStateErrorMetadata(22L, TRANSIENT, List.of("dummy error")));
-        ClusterState state = ClusterState.builder(new ClusterName("cluster"))
-            .nodes(
-                DiscoveryNodes.builder()
-                    .add(DiscoveryNodeUtils.create("node2", new TransportAddress(TransportAddress.META_ADDRESS, 9201)))
-                    .add(httpTransport.node)
-                    .masterNodeId(httpTransport.node.getId())
-                    .localNodeId(httpTransport.node.getId())
-            )
+        ClusterState state = ClusterState.builder(noFileSettingsState())
             .metadata(new Metadata.Builder().put(fileSettingsState.build()))
             .build();
 
@@ -324,4 +307,45 @@ public void testFileSettingsUpdateError() throws Exception {
         readinessService.stop();
         readinessService.close();
     }
+
+    public void testFileSettingsMixedCluster() throws Exception {
+        readinessService.start();
+
+        // initially the service isn't ready because initial cluster state has not been applied yet
+        assertFalse(readinessService.ready());
+
+        ClusterState noFileSettingsState = ClusterState.builder(noFileSettingsState())
+            // the master node is upgraded to support file settings, but existing node2 is not
+            .nodeFeatures(Map.of(httpTransport.node.getId(), nodeFeatures))
+            .build();
+        ClusterChangedEvent event = new ClusterChangedEvent("test", noFileSettingsState, emptyState());
+        readinessService.clusterChanged(event);
+
+        // when upgrading from nodes before file settings exist, readiness should return true once a master is elected
+        assertTrue(readinessService.ready());
+
+        readinessService.stop();
+        readinessService.close();
+    }
+
+    private ClusterState emptyState() {
+        return ClusterState.builder(new ClusterName("cluster"))
+            .nodes(
+                DiscoveryNodes.builder().add(DiscoveryNodeUtils.create("node2", new TransportAddress(TransportAddress.META_ADDRESS, 9201)))
+            )
+            .build();
+    }
+
+    private ClusterState noFileSettingsState() {
+        ClusterState emptyState = emptyState();
+        return ClusterState.builder(emptyState)
+            .nodes(
+                DiscoveryNodes.builder(emptyState.nodes())
+                    .add(httpTransport.node)
+                    .masterNodeId(httpTransport.node.getId())
+                    .localNodeId(httpTransport.node.getId())
+            )
+            .nodeFeatures(Map.of(httpTransport.node.getId(), nodeFeatures, "node2", nodeFeatures))
+            .build();
+    }
 }