Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix file settings service tests #115770

Merged
merged 12 commits into from
Oct 29, 2024
Merged

Fix file settings service tests #115770

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
213d850
Fix file settings tests
n1v0lg Oct 28, 2024
a2e3f57
Fix comment
n1v0lg Oct 28, 2024
cdea9e2
Move call
n1v0lg Oct 28, 2024
45a7bdb
One more move
n1v0lg Oct 28, 2024
44ce40f
Tweaks
n1v0lg Oct 28, 2024
7736b5c
Merge branch 'main' into fix-file-settings-test
elasticmachine Oct 28, 2024
423e91a
Only catch unsupported ex
n1v0lg Oct 28, 2024
a867fdc
Merge branch 'fix-file-settings-test' of github.com:n1v0lg/elasticsea…
n1v0lg Oct 28, 2024
a5ad4fe
Fewer guess changes
n1v0lg Oct 28, 2024
58fce1a
Undo one more unrelated change
n1v0lg Oct 28, 2024
50fb964
Review feedback
n1v0lg Oct 29, 2024
69b9074
Merge branch 'main' into fix-file-settings-test
elasticmachine Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions muted-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,9 +241,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
66 changes: 38 additions & 28 deletions server/src/test/java/org/elasticsearch/reservedstate/service/FileSettingsServiceTests.java
View file
Open in desktop
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 @@ -192,8 +197,6 @@ public void testInitialFileWorks() throws Exception {

CountDownLatch latch = new CountDownLatch(1);
n1v0lg marked this conversation as resolved.
Show resolved Hide resolved

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(), "{}");
Expand All @@ -209,7 +212,7 @@ public void testInitialFileWorks() throws Exception {
fileSettingsService.start();
fileSettingsService.clusterChanged(new ClusterChangedEvent("test", clusterService.state(), ClusterState.EMPTY_STATE));

// wait for listener to be called
// wait file processing call
assertTrue(latch.await(20, TimeUnit.SECONDS));

verify(fileSettingsService, times(1)).processFileOnServiceStart();
Expand All @@ -223,40 +226,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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not a very robust setup... The two things we really care about is that processFileOnServiceStart gets called, and that processFileChanges and that we wait for these calls. I've restructured this test to use two separate latches, and do the file writes one at a time.

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));
assertTrue(changesOnStartLatch.await(20, TimeUnit.SECONDS));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could use the safeAwait here to follow conventions in other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah it has 10 seconds baked in. it's obnoxious that we'd need to wait longer than that but a comment in this file makes me hesitant:

"// we need to wait a bit, on MacOS it may take up to 10 seconds for the Java watcher service to notice the file"


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());
}
Expand Down Expand Up @@ -352,15 +355,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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tweaking this to:

  • account for missing atomic_move functionality
  • make temp file removal more explicit

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 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);
}
}
}