Skip to content

Commit

Permalink
Fix file settings service tests (elastic#115770)
Browse files Browse the repository at this point in the history 
This PR addresses some of the failure causes tracked under
elastic#115280 and
elastic#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:

elastic#115725 (comment)

But will likely require another round of changes to address the failures
to delete files.

Relates: elastic#115280 
Relates: elastic#115725
  • Loading branch information
@n1v0lg @jfreden
n1v0lg authored and jfreden committed Nov 4, 2024
1 parent b2346ad commit b88dfd3
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 36 deletions.
3 changes: 0 additions & 3 deletions muted-tests.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
85 changes: 52 additions & 33 deletions server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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());
Expand All @@ -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());
}
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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");
}
}
}

0 comments on commit b88dfd3

Please sign in to comment.