Restore API: Fix file settings handling (#137585) (#137878)

* Restore API: Fix file settings handling

When restoring a snapshot including global state, do not remove the
reserved state, but initialise an empty one. This prevents the
readiness service from crashing.
Also, initialising an empty state now correctly removes pre-existing
reserved state settings and file settings.

Fixes #122429

Author: mamazzol

docs/changelog/137585.yaml

@@ -0,0 +1,6 @@
+pr: 137585
+summary: "Restore API: Fix file settings handling"
+area: Infra/Settings
+type: bug
+issues:
+ - 122429

server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java

@@ -27,6 +27,7 @@
 import org.elasticsearch.core.Tuple;
 import org.elasticsearch.reservedstate.action.ReservedClusterSettingsAction;
 import org.elasticsearch.test.ESIntegTestCase;
+import org.elasticsearch.test.InternalTestCluster;
 import org.junit.Before;
 
 import java.io.IOException;
@@ -38,6 +39,7 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
 
+import static org.elasticsearch.cluster.metadata.ReservedStateMetadata.EMPTY_VERSION;
 import static org.elasticsearch.indices.recovery.RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING;
 import static org.elasticsearch.node.Node.INITIAL_STATE_TIMEOUT_SETTING;
 import static org.elasticsearch.test.NodeRoles.dataOnlyNode;
@@ -318,6 +320,68 @@ public void testReservedStatePersistsOnRestart() throws Exception {
         );
     }
 
+    public void testReservedStateClearedWhenFileAbsentAtStartup() throws Exception {
+        internalCluster().setBootstrapMasterNodeIndex(0);
+        logger.info("--> start master node");
+        final String masterNode = internalCluster().startMasterOnlyNode(
+            Settings.builder().put(INITIAL_STATE_TIMEOUT_SETTING.getKey(), "0s").build()
+        );
+        assertMasterNode(internalCluster().masterClient(), masterNode);
+        var savedClusterState = setupClusterStateListener(masterNode);
+
+        FileSettingsService masterFileSettingsService = internalCluster().getInstance(FileSettingsService.class, masterNode);
+
+        assertBusy(() -> assertTrue(masterFileSettingsService.watching()));
+
+        logger.info("--> write some settings");
+        writeJSONFile(masterNode, testJSON, logger, versionCounter.get());
+        assertClusterStateSaveOK(savedClusterState.v1(), savedClusterState.v2(), "50mb");
+
+        logger.info("--> get file path before restarting node");
+        Path watchedFile = masterFileSettingsService.watchedFile();
+        assertTrue("File should exist before deletion", Files.exists(watchedFile));
+
+        logger.info("--> restart master and delete file while node is stopped");
+        internalCluster().restartNode(masterNode, new InternalTestCluster.RestartCallback() {
+            @Override
+            public Settings onNodeStopped(String nodeName) throws Exception {
+                logger.info("--> delete the file settings file while node is stopped");
+                Files.deleteIfExists(watchedFile);
+                assertFalse("File should not exist after deletion", Files.exists(watchedFile));
+                return super.onNodeStopped(nodeName);
+            }
+        });
+
+        logger.info("--> verify reserved state is cleared when missing file is processed at startup");
+        assertBusy(() -> {
+            final ClusterStateResponse clusterStateResponse = clusterAdmin().state(new ClusterStateRequest(TEST_REQUEST_TIMEOUT))
+                .actionGet();
+            ReservedStateMetadata reservedState = clusterStateResponse.getState()
+                .metadata()
+                .reservedStateMetadata()
+                .get(FileSettingsService.NAMESPACE);
+            assertThat(reservedState, notNullValue());
+            assertThat(reservedState.version(), equalTo(EMPTY_VERSION));
+            assertTrue(reservedState.handlers().isEmpty());
+        }, 20, TimeUnit.SECONDS);
+
+        logger.info("--> verify settings are no longer reserved and can be modified");
+        ClusterUpdateSettingsRequest req = new ClusterUpdateSettingsRequest(TEST_REQUEST_TIMEOUT, TEST_REQUEST_TIMEOUT).persistentSettings(
+            Settings.builder().put(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey(), "1234kb")
+        );
+        clusterAdmin().updateSettings(req).get();
+
+        assertThat(
+            clusterAdmin().state(new ClusterStateRequest(TEST_REQUEST_TIMEOUT))
+                .actionGet()
+                .getState()
+                .metadata()
+                .persistentSettings()
+                .get(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey()),
+            equalTo("1234kb")
+        );
+    }
+
     private Tuple<CountDownLatch, AtomicLong> setupClusterStateListenerForError(String node) {
         ClusterService clusterService = internalCluster().clusterService(node);
         CountDownLatch savedClusterState = new CountDownLatch(1);

server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/SnapshotsAndFileSettingsIT.java

@@ -32,8 +32,10 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicLong;
 
+import static org.elasticsearch.cluster.metadata.ReservedStateMetadata.EMPTY_VERSION;
 import static org.elasticsearch.indices.recovery.RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.notNullValue;
 
 /**
  * Tests that snapshot restore behaves correctly when we have file based settings that reserve part of the
@@ -152,8 +154,14 @@ public void testRestoreWithRemovedFileSettings() throws Exception {
         final ClusterStateResponse clusterStateResponse = clusterAdmin().state(new ClusterStateRequest(TEST_REQUEST_TIMEOUT).metadata(true))
             .actionGet();
 
-        // We expect no reserved metadata state for file based settings, the operator file was deleted.
-        assertNull(clusterStateResponse.getState().metadata().reservedStateMetadata().get(FileSettingsService.NAMESPACE));
+        // We expect empty reserved metadata state for file based settings, the operator file was deleted.
+        ReservedStateMetadata reservedState = clusterStateResponse.getState()
+            .metadata()
+            .reservedStateMetadata()
+            .get(FileSettingsService.NAMESPACE);
+        assertThat(reservedState, notNullValue());
+        assertThat(reservedState.version(), equalTo(EMPTY_VERSION));
+        assertTrue(reservedState.handlers().isEmpty());
 
         final ClusterGetSettingsAction.Response getSettingsResponse = clusterAdmin().execute(
             ClusterGetSettingsAction.INSTANCE,
@@ -164,8 +172,8 @@ public void testRestoreWithRemovedFileSettings() throws Exception {
             getSettingsResponse.persistentSettings().get(InternalClusterInfoService.INTERNAL_CLUSTER_INFO_TIMEOUT_SETTING.getKey()),
             equalTo("25s")
         );
-        // We didn't remove the setting set by file settings, we simply removed the reserved (operator) section.
-        assertThat(getSettingsResponse.persistentSettings().get("indices.recovery.max_bytes_per_sec"), equalTo("50mb"));
+        // File setting were re-set as the file was absent, this trumps the snapshot restore.
+        assertNull(getSettingsResponse.persistentSettings().get("indices.recovery.max_bytes_per_sec"));
         // cleanup
         updateClusterSettings(
             Settings.builder()

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

@@ -11,6 +11,7 @@
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.ActionResponse;
 import org.elasticsearch.action.support.PlainActionFuture;
 import org.elasticsearch.cluster.ClusterState;
@@ -84,19 +85,18 @@ public FileSettingsService(ClusterService clusterService, ReservedClusterStateSe
     public void handleSnapshotRestore(ClusterState clusterState, Metadata.Builder mdBuilder) {
         assert clusterState.nodes().isLocalNodeElectedMaster();
 
-        ReservedStateMetadata fileSettingsMetadata = clusterState.metadata().reservedStateMetadata().get(NAMESPACE);
-
         // When we restore from a snapshot we remove the reserved cluster state for file settings,
         // since we don't know the current operator configuration, e.g. file settings could be disabled
         // on the target cluster. If file settings exist and the cluster state has lost it's reserved
         // state for the "file_settings" namespace, we touch our file settings file to cause it to re-process the file.
         if (watching() && filesExists(watchedFile())) {
+            ReservedStateMetadata fileSettingsMetadata = clusterState.metadata().reservedStateMetadata().get(NAMESPACE);
             if (fileSettingsMetadata != null) {
                 ReservedStateMetadata withResetVersion = new ReservedStateMetadata.Builder(fileSettingsMetadata).version(0L).build();
                 mdBuilder.put(withResetVersion);
             }
-        } else if (fileSettingsMetadata != null) {
-            mdBuilder.removeReservedState(fileSettingsMetadata);
+        } else {
+            stateService.initEmpty(NAMESPACE, ActionListener.noop());
         }
     }
 

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

@@ -166,7 +166,7 @@ public void initEmpty(String namespace, ActionListener<ActionResponse.Empty> lis
                 namespace,
                 emptyState,
                 ReservedStateVersionCheck.HIGHER_VERSION_ONLY,
-                Map.of(),
+                handlers,
                 List.of(),
                 // error state should not be possible since there is no metadata being parsed or processed
                 errorState -> { throw new AssertionError(); },

server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java

@@ -62,7 +62,6 @@
 
 import static java.nio.file.StandardCopyOption.ATOMIC_MOVE;
 import static org.elasticsearch.node.Node.NODE_NAME_SETTING;
-import static org.hamcrest.Matchers.anEmptyMap;
 import static org.hamcrest.Matchers.hasEntry;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
@@ -306,7 +305,7 @@ public void testStopWorksInMiddleOfProcessing() throws Exception {
         deadThreadLatch.countDown();
     }
 
-    public void testHandleSnapshotRestoreClearsMetadata() throws Exception {
+    public void testHandleSnapshotRestoreClearsMetadata() {
         ClusterState state = ClusterState.builder(clusterService.state())
             .metadata(
                 Metadata.builder(clusterService.state().metadata())
@@ -318,7 +317,7 @@ public void testHandleSnapshotRestoreClearsMetadata() throws Exception {
         Metadata.Builder metadata = Metadata.builder(state.metadata());
         fileSettingsService.handleSnapshotRestore(state, metadata);
 
-        assertThat(metadata.build().reservedStateMetadata(), anEmptyMap());
+        verify(controller).initEmpty(FileSettingsService.NAMESPACE, ActionListener.noop());
     }
 
     public void testHandleSnapshotRestoreResetsMetadata() throws Exception {