Restore API: Fix file settings handling

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

@@ -28,6 +28,7 @@
 import org.elasticsearch.health.node.FetchHealthInfoCacheAction;
 import org.elasticsearch.reservedstate.action.ReservedClusterSettingsAction;
 import org.elasticsearch.test.ESIntegTestCase;
+import org.elasticsearch.test.InternalTestCluster;
 import org.junit.Before;
 
 import java.io.IOException;
@@ -40,6 +41,7 @@
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.stream.Stream;
 
+import static org.elasticsearch.cluster.metadata.ReservedStateMetadata.EMPTY_VERSION;
 import static org.elasticsearch.health.HealthStatus.YELLOW;
 import static org.elasticsearch.indices.recovery.RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING;
 import static org.elasticsearch.node.Node.INITIAL_STATE_TIMEOUT_SETTING;
@@ -322,6 +324,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()
+        );
+        awaitMasterNode(internalCluster().getMasterName(), masterNode);
+        var savedClusterState = setupClusterStateListener(masterNode, versionCounter.incrementAndGet());
+
+        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

@@ -141,19 +141,18 @@ public void handleSnapshotRestore(
     ) {
         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

@@ -278,7 +278,7 @@ public void initEmpty(String namespace, ActionListener<ActionResponse.Empty> lis
                 namespace,
                 emptyState,
                 ReservedStateVersionCheck.HIGHER_VERSION_ONLY,
-                Map.of(),
+                clusterHandlers,
                 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

@@ -74,7 +74,6 @@
 import static org.elasticsearch.health.HealthStatus.GREEN;
 import static org.elasticsearch.health.HealthStatus.YELLOW;
 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.argThat;
@@ -443,7 +442,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())
@@ -455,7 +454,7 @@ public void testHandleSnapshotRestoreClearsMetadata() throws Exception {
         Metadata.Builder metadata = Metadata.builder(state.metadata());
         fileSettingsService.handleSnapshotRestore(state, ClusterState.builder(state), metadata, ProjectId.DEFAULT);
 
-        assertThat(metadata.build().reservedStateMetadata(), anEmptyMap());
+        verify(controller).initEmpty(FileSettingsService.NAMESPACE, ActionListener.noop());
     }
 
     public void testHandleSnapshotRestoreResetsMetadata() throws Exception {