From 213d8505e072d6d27421d3c45d38800e03e73e31 Mon Sep 17 00:00:00 2001 From: Nikolaj Volgushev Date: Mon, 28 Oct 2024 14:45:41 +0100 Subject: [PATCH] Fix file settings tests --- muted-tests.yml | 3 - .../service/FileSettingsServiceTests.java | 56 +++++++++++-------- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index 3a59af6234038..9795e8d49d7a8 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -246,9 +246,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..5c0b74a48d476 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,11 @@ import org.mockito.stubbing.Answer; import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.AccessDeniedException; +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 +54,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; @@ -192,8 +198,6 @@ public void testInitialFileWorks() throws Exception { CountDownLatch latch = new CountDownLatch(1); - 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(), "{}"); @@ -223,10 +227,7 @@ 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); + CountDownLatch changesOnStartLatch = new CountDownLatch(1); Files.createDirectories(fileSettingsService.watchedFileDir()); // contents of the JSON don't matter, we just need a file to exist @@ -236,27 +237,31 @@ public void testProcessFileChanges() throws Exception { 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(); 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)); + assertTrue(changesOnStartLatch.await(20, TimeUnit.SECONDS)); 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(), "[]"); + assertTrue(changesLatch.await(20, TimeUnit.SECONDS)); + verify(fileSettingsService, times(1)).processFileChanges(); verify(controller, times(1)).process(any(), any(XContentParser.class), eq(ReservedStateVersionCheck.HIGHER_VERSION_ONLY), any()); } @@ -352,15 +357,22 @@ 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); - private void overwriteTestFile(Path path, String contents) throws IOException { - Path tempFilePath = createTempFile(); - Files.writeString(tempFilePath, contents); - Files.move(tempFilePath, path, StandardCopyOption.REPLACE_EXISTING); + try { + Files.move(tempFile, path, REPLACE_EXISTING, ATOMIC_MOVE); + } catch (AtomicMoveNotSupportedException | AccessDeniedException 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); + } } }