From b1e48df5f1c1f9427ec6dcef3fe8091ac241f74e Mon Sep 17 00:00:00 2001 From: Johannes Freden Jansson Date: Fri, 23 May 2025 15:35:06 +0200 Subject: [PATCH 1/3] Fix FileSettingsServiceIT race condition --- .../service/FileSettingsServiceIT.java | 41 +++++++++---------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java b/server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java index 1899b055ed0c9..5778f4ea9f7f0 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/reservedstate/service/FileSettingsServiceIT.java @@ -196,7 +196,7 @@ public void clusterChanged(ClusterChangedEvent event) { return new Tuple<>(savedClusterState, metadataVersion); } - private Tuple setupClusterStateListener(String node) { + private Tuple setupClusterStateListener(String node, long version) { ClusterService clusterService = internalCluster().clusterService(node); CountDownLatch savedClusterState = new CountDownLatch(1); AtomicLong metadataVersion = new AtomicLong(-1); @@ -204,13 +204,10 @@ private Tuple setupClusterStateListener(String node) @Override public void clusterChanged(ClusterChangedEvent event) { ReservedStateMetadata reservedState = event.state().metadata().reservedStateMetadata().get(FileSettingsService.NAMESPACE); - if (reservedState != null) { - ReservedStateHandlerMetadata handlerMetadata = reservedState.handlers().get(ReservedClusterSettingsAction.NAME); - if (handlerMetadata != null && handlerMetadata.keys().contains("indices.recovery.max_bytes_per_sec")) { - clusterService.removeListener(this); - metadataVersion.set(event.state().metadata().version()); - savedClusterState.countDown(); - } + if (reservedState != null && reservedState.version() == version) { + clusterService.removeListener(this); + metadataVersion.set(event.state().metadata().version()); + savedClusterState.countDown(); } } }); @@ -258,14 +255,14 @@ public void testSettingsApplied() throws Exception { logger.info("--> start master node"); final String masterNode = internalCluster().startMasterOnlyNode(); assertMasterNode(internalCluster().nonMasterClient(), masterNode); - var savedClusterState = setupClusterStateListener(masterNode); + var savedClusterState = setupClusterStateListener(masterNode, versionCounter.incrementAndGet()); FileSettingsService masterFileSettingsService = internalCluster().getInstance(FileSettingsService.class, masterNode); assertBusy(() -> assertTrue(masterFileSettingsService.watching())); assertFalse(dataFileSettingsService.watching()); - writeJSONFile(masterNode, testJSON, logger, versionCounter.incrementAndGet()); + writeJSONFile(masterNode, testJSON, logger, versionCounter.get()); assertClusterStateSaveOK(savedClusterState.v1(), savedClusterState.v2(), "50mb"); } @@ -276,11 +273,11 @@ public void testSettingsAppliedOnStart() throws Exception { FileSettingsService dataFileSettingsService = internalCluster().getInstance(FileSettingsService.class, dataNode); assertFalse(dataFileSettingsService.watching()); - var savedClusterState = setupClusterStateListener(dataNode); + var savedClusterState = setupClusterStateListener(dataNode, versionCounter.incrementAndGet()); // In internal cluster tests, the nodes share the config directory, so when we write with the data node path // the master will pick it up on start - writeJSONFile(dataNode, testJSON, logger, versionCounter.incrementAndGet()); + writeJSONFile(dataNode, testJSON, logger, versionCounter.get()); logger.info("--> start master node"); final String masterNode = internalCluster().startMasterOnlyNode(); @@ -301,14 +298,14 @@ public void testReservedStatePersistsOnRestart() throws Exception { Settings.builder().put(INITIAL_STATE_TIMEOUT_SETTING.getKey(), "0s").build() ); assertMasterNode(internalCluster().masterClient(), masterNode); - var savedClusterState = setupClusterStateListener(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.incrementAndGet()); + writeJSONFile(masterNode, testJSON, logger, versionCounter.get()); assertClusterStateSaveOK(savedClusterState.v1(), savedClusterState.v2(), "50mb"); logger.info("--> restart master"); @@ -476,12 +473,12 @@ public void testSettingsAppliedOnMasterReElection() throws Exception { assertFalse(master1FS.watching()); assertFalse(master2FS.watching()); - var savedClusterState = setupClusterStateListener(masterNode); + var savedClusterState = setupClusterStateListener(masterNode, versionCounter.incrementAndGet()); FileSettingsService masterFileSettingsService = internalCluster().getInstance(FileSettingsService.class, masterNode); assertBusy(() -> assertTrue(masterFileSettingsService.watching())); - writeJSONFile(masterNode, testJSON, logger, versionCounter.incrementAndGet()); + writeJSONFile(masterNode, testJSON, logger, versionCounter.get()); assertClusterStateSaveOK(savedClusterState.v1(), savedClusterState.v2(), "50mb"); internalCluster().stopCurrentMasterNode(); @@ -501,8 +498,8 @@ public void testSettingsAppliedOnMasterReElection() throws Exception { boolean awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS); assertTrue(awaitSuccessful); - savedClusterState = setupClusterStateListener(internalCluster().getMasterName()); - writeJSONFile(internalCluster().getMasterName(), testJSON43mb, logger, versionCounter.incrementAndGet()); + savedClusterState = setupClusterStateListener(internalCluster().getMasterName(), versionCounter.incrementAndGet()); + writeJSONFile(internalCluster().getMasterName(), testJSON43mb, logger, versionCounter.get()); assertClusterStateSaveOK(savedClusterState.v1(), savedClusterState.v2(), "43mb"); } @@ -515,21 +512,21 @@ public void testSymlinkUpdateTriggerReload() throws Exception { assertBusy(() -> assertTrue(masterFileSettingsService.watching())); { - var savedClusterState = setupClusterStateListener(masterNode); + var savedClusterState = setupClusterStateListener(masterNode, versionCounter.incrementAndGet()); // Create the settings.json as a symlink to simulate k8 setup // settings.json -> ..data/settings.json // ..data -> ..TIMESTAMP_TEMP_FOLDER_1 var fileDir = Files.createDirectories(baseDir.resolve("..TIMESTAMP_TEMP_FOLDER_1")); - writeJSONFile(masterNode, testJSON, logger, versionCounter.incrementAndGet(), fileDir.resolve("settings.json")); + writeJSONFile(masterNode, testJSON, logger, versionCounter.get(), fileDir.resolve("settings.json")); var dataDir = Files.createSymbolicLink(baseDir.resolve("..data"), fileDir.getFileName()); Files.createSymbolicLink(baseDir.resolve("settings.json"), dataDir.getFileName().resolve("settings.json")); assertClusterStateSaveOK(savedClusterState.v1(), savedClusterState.v2(), "50mb"); } { - var savedClusterState = setupClusterStateListener(masterNode); + var savedClusterState = setupClusterStateListener(masterNode, versionCounter.incrementAndGet()); // Update ..data symlink to ..data -> ..TIMESTAMP_TEMP_FOLDER_2 to simulate kubernetes secret update var fileDir = Files.createDirectories(baseDir.resolve("..TIMESTAMP_TEMP_FOLDER_2")); - writeJSONFile(masterNode, testJSON43mb, logger, versionCounter.incrementAndGet(), fileDir.resolve("settings.json")); + writeJSONFile(masterNode, testJSON43mb, logger, versionCounter.get(), fileDir.resolve("settings.json")); Files.deleteIfExists(baseDir.resolve("..data")); Files.createSymbolicLink(baseDir.resolve("..data"), fileDir.getFileName()); assertClusterStateSaveOK(savedClusterState.v1(), savedClusterState.v2(), "43mb"); From 0b70c21795c8edb2402004a701061623a0990657 Mon Sep 17 00:00:00 2001 From: Johannes Freden Jansson Date: Fri, 23 May 2025 15:41:18 +0200 Subject: [PATCH 2/3] Unmute test --- muted-tests.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index 060f521193aa3..16c106408d132 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -474,9 +474,6 @@ tests: - class: org.elasticsearch.packaging.test.TemporaryDirectoryConfigTests method: test21AcceptsCustomPathInDocker issue: https://github.com/elastic/elasticsearch/issues/128114 -- class: org.elasticsearch.reservedstate.service.FileSettingsServiceIT - method: testSymlinkUpdateTriggerReload - issue: https://github.com/elastic/elasticsearch/issues/128369 # Examples: # From 64bbda461c6b998dfbabaf91c3421f6f52bdcc73 Mon Sep 17 00:00:00 2001 From: Johannes Freden Jansson Date: Mon, 26 May 2025 07:22:31 +0200 Subject: [PATCH 3/3] fixup! unmute test --- muted-tests.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index f8823f477bb60..d415a1c119d32 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -474,9 +474,6 @@ tests: - class: org.elasticsearch.packaging.test.TemporaryDirectoryConfigTests method: test21AcceptsCustomPathInDocker issue: https://github.com/elastic/elasticsearch/issues/128114 -- class: org.elasticsearch.reservedstate.service.FileSettingsServiceIT - method: testSymlinkUpdateTriggerReload - issue: https://github.com/elastic/elasticsearch/issues/128369 - class: org.elasticsearch.xpack.esql.qa.single_node.PushQueriesIT method: testEqualityAndOther {semantic_text} issue: https://github.com/elastic/elasticsearch/issues/128414