From b868677c5b156233257612ab2121f4a01ca69aed Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Tue, 29 Oct 2024 12:26:27 +0100 Subject: [PATCH] Fix file settings service tests (#115770) This PR addresses some of the failure causes tracked under https://github.com/elastic/elasticsearch/issues/115280 and https://github.com/elastic/elasticsearch/issues/115725: the latch-await setup was rather convoluted and the move command not always correctly invoked in the right order. This PR cleans up latching by separating awaiting the first processing call (on start) from waiting on the subsequent call. Also, it makes writing the file more robust w.r.t. OS'es where `atomic_move` may not be available. This should address failures around the timeout await, and the assertion failures around invoked methods tracked here: https://github.com/elastic/elasticsearch/issues/115725#issuecomment-2441989470 But will likely require another round of changes to address the failures to delete files. Relates: https://github.com/elastic/elasticsearch/issues/115280 Relates: https://github.com/elastic/elasticsearch/issues/115725 --- muted-tests.yml | 3 - .../service/FileSettingsServiceTests.java | 85 ++++++++++++------- 2 files changed, 52 insertions(+), 36 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index 4315f1283a347..419e8fbb68566 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -236,9 +236,6 @@ tests: - class: org.elasticsearch.xpack.inference.DefaultEndPointsIT method: testInferDeploysDefaultE5 issue: https://github.com/elastic/elasticsearch/issues/115361 -- class: org.elasticsearch.reservedstate.service.FileSettingsServiceTests - method: testProcessFileChanges - issue: https://github.com/elastic/elasticsearch/issues/115280 - class: org.elasticsearch.xpack.security.FileSettingsRoleMappingsRestartIT method: testFileSettingsReprocessedOnRestartWithoutVersionChange issue: https://github.com/elastic/elasticsearch/issues/115450 diff --git a/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java index 8af36e2f9677e..f67d7ddcc7550 100644 --- a/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java +++ b/server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java @@ -24,6 +24,8 @@ import org.elasticsearch.common.component.Lifecycle; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.core.IOUtils; +import org.elasticsearch.core.Strings; import org.elasticsearch.core.TimeValue; import org.elasticsearch.env.BuildVersion; import org.elasticsearch.env.Environment; @@ -39,9 +41,10 @@ import org.mockito.stubbing.Answer; import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.AtomicMoveNotSupportedException; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.StandardCopyOption; import java.util.List; import java.util.Map; import java.util.Set; @@ -50,6 +53,8 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; +import static java.nio.file.StandardCopyOption.ATOMIC_MOVE; +import static java.nio.file.StandardCopyOption.REPLACE_EXISTING; import static org.elasticsearch.node.Node.NODE_NAME_SETTING; import static org.hamcrest.Matchers.anEmptyMap; import static org.hamcrest.Matchers.hasEntry; @@ -190,9 +195,7 @@ public void testInitialFileWorks() throws Exception { return null; }).when(controller).process(any(), any(XContentParser.class), any(), any()); - CountDownLatch latch = new CountDownLatch(1); - - fileSettingsService.addFileChangedListener(latch::countDown); + CountDownLatch fileProcessingLatch = new CountDownLatch(1); Files.createDirectories(fileSettingsService.watchedFileDir()); // contents of the JSON don't matter, we just need a file to exist @@ -202,15 +205,14 @@ public void testInitialFileWorks() throws Exception { try { return invocation.callRealMethod(); } finally { - latch.countDown(); + fileProcessingLatch.countDown(); } }).when(fileSettingsService).processFileOnServiceStart(); fileSettingsService.start(); fileSettingsService.clusterChanged(new ClusterChangedEvent("test", clusterService.state(), ClusterState.EMPTY_STATE)); - // wait for listener to be called - assertTrue(latch.await(20, TimeUnit.SECONDS)); + longAwait(fileProcessingLatch); verify(fileSettingsService, times(1)).processFileOnServiceStart(); verify(controller, times(1)).process(any(), any(XContentParser.class), eq(ReservedStateVersionCheck.HIGHER_OR_SAME_VERSION), any()); @@ -223,40 +225,40 @@ public void testProcessFileChanges() throws Exception { return null; }).when(controller).process(any(), any(XContentParser.class), any(), any()); - // we get three events: initial clusterChanged event, first write, second write - CountDownLatch latch = new CountDownLatch(3); - - fileSettingsService.addFileChangedListener(latch::countDown); - - Files.createDirectories(fileSettingsService.watchedFileDir()); - // contents of the JSON don't matter, we just need a file to exist - writeTestFile(fileSettingsService.watchedFile(), "{}"); - + CountDownLatch changesOnStartLatch = new CountDownLatch(1); doAnswer((Answer) invocation -> { try { return invocation.callRealMethod(); } finally { - latch.countDown(); + changesOnStartLatch.countDown(); } }).when(fileSettingsService).processFileOnServiceStart(); + + CountDownLatch changesLatch = new CountDownLatch(1); doAnswer((Answer) invocation -> { try { return invocation.callRealMethod(); } finally { - latch.countDown(); + changesLatch.countDown(); } }).when(fileSettingsService).processFileChanges(); + Files.createDirectories(fileSettingsService.watchedFileDir()); + // contents of the JSON don't matter, we just need a file to exist + writeTestFile(fileSettingsService.watchedFile(), "{}"); + fileSettingsService.start(); fileSettingsService.clusterChanged(new ClusterChangedEvent("test", clusterService.state(), ClusterState.EMPTY_STATE)); - // second file change; contents still don't matter - overwriteTestFile(fileSettingsService.watchedFile(), "{}"); - // wait for listener to be called (once for initial processing, once for subsequent update) - assertTrue(latch.await(20, TimeUnit.SECONDS)); + longAwait(changesOnStartLatch); verify(fileSettingsService, times(1)).processFileOnServiceStart(); verify(controller, times(1)).process(any(), any(XContentParser.class), eq(ReservedStateVersionCheck.HIGHER_OR_SAME_VERSION), any()); + + // second file change; contents still don't matter + writeTestFile(fileSettingsService.watchedFile(), "[]"); + longAwait(changesLatch); + verify(fileSettingsService, times(1)).processFileChanges(); verify(controller, times(1)).process(any(), any(XContentParser.class), eq(ReservedStateVersionCheck.HIGHER_VERSION_ONLY), any()); } @@ -295,9 +297,7 @@ public void testStopWorksInMiddleOfProcessing() throws Exception { // Make some fake settings file to cause the file settings service to process it writeTestFile(fileSettingsService.watchedFile(), "{}"); - // we need to wait a bit, on MacOS it may take up to 10 seconds for the Java watcher service to notice the file, - // on Linux is instantaneous. Windows is instantaneous too. - assertTrue(processFileLatch.await(30, TimeUnit.SECONDS)); + longAwait(processFileLatch); // Stopping the service should interrupt the watcher thread, we should be able to stop fileSettingsService.stop(); @@ -352,15 +352,34 @@ public void testHandleSnapshotRestoreResetsMetadata() throws Exception { } // helpers - private void writeTestFile(Path path, String contents) throws IOException { - Path tempFilePath = createTempFile(); - Files.writeString(tempFilePath, contents); - Files.move(tempFilePath, path, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING); + private static void writeTestFile(Path path, String contents) { + Path tempFile = null; + try { + tempFile = Files.createTempFile(path.getParent(), path.getFileName().toString(), "tmp"); + Files.writeString(tempFile, contents); + + try { + Files.move(tempFile, path, REPLACE_EXISTING, ATOMIC_MOVE); + } catch (AtomicMoveNotSupportedException e) { + Files.move(tempFile, path, REPLACE_EXISTING); + } + } catch (final IOException e) { + throw new UncheckedIOException(Strings.format("could not write file [%s]", path.toAbsolutePath()), e); + } finally { + // we are ignoring exceptions here, so we do not need handle whether or not tempFile was initialized nor if the file exists + IOUtils.deleteFilesIgnoringExceptions(tempFile); + } } - private void overwriteTestFile(Path path, String contents) throws IOException { - Path tempFilePath = createTempFile(); - Files.writeString(tempFilePath, contents); - Files.move(tempFilePath, path, StandardCopyOption.REPLACE_EXISTING); + // this waits for up to 20 seconds to account for watcher service differences between OSes + // on MacOS it may take up to 10 seconds for the Java watcher service to notice the file, + // on Linux is instantaneous. Windows is instantaneous too. + private static void longAwait(CountDownLatch latch) { + try { + assertTrue("longAwait: CountDownLatch did not reach zero within the timeout", latch.await(20, TimeUnit.SECONDS)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + fail(e, "longAwait: interrupted waiting for CountDownLatch to reach zero"); + } } }