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

Jump to bottom

Ensure raft storage lock file is update atomically #10683

Merged
merged 2 commits into from
Oct 13, 2022
Merged

Ensure raft storage lock file is update atomically #10683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8322db7
fix(raft): ensure lock files is created and updated atomically
deepthidevaki Oct 12, 2022
8694c15
test(raft): refactor RaftStorageTest
deepthidevaki Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 23 additions & 8 deletions atomix/cluster/src/main/java/io/atomix/raft/storage/RaftStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@
import java.io.File;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.Files;
import java.nio.file.StandardCopyOption;
import java.nio.file.StandardOpenOption;

/**
Expand Down Expand Up @@ -110,17 +112,30 @@ public String prefix() {
* @return indicates whether the lock was successfully acquired
*/
public boolean lock(final String id) {
final File file = new File(directory, String.format(".%s.lock", prefix));
final File lockFile = new File(directory, String.format(".%s.lock", prefix));
final File tempLockFile = new File(directory, String.format(".%s.lock.tmp", id));
try {
if (file.createNewFile()) {
Files.writeString(file.toPath(), id, StandardOpenOption.WRITE);
return true;
} else {
final String lock = Files.readString(file.toPath());
return lock != null && lock.equals(id);
if (!lockFile.exists()) {
// Create and update the file atomically
Files.writeString(
tempLockFile.toPath(),
id,
StandardOpenOption.CREATE,
StandardOpenOption.TRUNCATE_EXISTING,
StandardOpenOption.WRITE,
StandardOpenOption.SYNC);

// If two nodes tries to acquire lock, move will fail with FileAlreadyExistsException
FileUtil.moveDurably(
Copy link
Member

Choose a reason for hiding this comment

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

❓ I can remember a discussion that this was only supported on some environments wasn't that something? Like only linux or? Is this an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know what exactly was the problem?

Flushing the parent directory does not work in windows. But according to what is documented in FileUtil, this is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using FileUtil#moveDurably in other places as well. So far no problems are reported. So I guess it should be ok. So I will merge this PR. If we see/know any problems later, let's tackle it then.

tempLockFile.toPath(), lockFile.toPath(), StandardCopyOption.ATOMIC_MOVE);
}
// Read the lock file again to ensure that contents matches the local id
final String lock = Files.readString(lockFile.toPath());
return lock != null && lock.equals(id);
} catch (final FileAlreadyExistsException e) {
return false;
} catch (final IOException e) {
throw new StorageException("Failed to acquire storage lock");
throw new StorageException("Failed to acquire storage lock", e);
deepthidevaki marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
75 changes: 37 additions & 38 deletions atomix/cluster/src/test/java/io/atomix/raft/storage/RaftStorageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,14 @@
*/
package io.atomix.raft.storage;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.assertj.core.api.Assertions.assertThat;

import io.camunda.zeebe.util.FileUtil;
import java.io.File;
import java.io.IOException;
import java.nio.file.FileVisitResult;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.SimpleFileVisitor;
import java.nio.file.attribute.BasicFileAttributes;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;

/** Raft storage test. */
Expand All @@ -38,14 +32,14 @@ public class RaftStorageTest {
private static final Path PATH = Paths.get("target/test-logs/");

@Test
public void testDefaultConfiguration() throws Exception {
public void testDefaultConfiguration() {
final RaftStorage storage = RaftStorage.builder().build();
assertEquals("atomix", storage.prefix());
assertEquals(new File(System.getProperty("user.dir")), storage.directory());
assertThat(storage.prefix()).isEqualTo("atomix");
assertThat(storage.directory()).isEqualTo(new File(System.getProperty("user.dir")));
}

@Test
public void testCustomConfiguration() throws Exception {
public void testCustomConfiguration() {
final RaftStorage storage =
RaftStorage.builder()
.withPrefix("foo")
Expand All @@ -54,49 +48,54 @@ public void testCustomConfiguration() throws Exception {
.withFreeDiskSpace(100)
.withFlushExplicitly(false)
.build();
assertEquals("foo", storage.prefix());
assertEquals(new File(PATH.toFile(), "foo"), storage.directory());
assertThat(storage.prefix()).isEqualTo("foo");
assertThat(storage.directory()).isEqualTo(new File(PATH.toFile(), "foo"));
}

@Test
public void testStorageLock() throws Exception {
public void canAcquireLockOnEmptyDirectory() {
// given empty directory in PATH

// when
final RaftStorage storage1 =
RaftStorage.builder().withDirectory(PATH.toFile()).withPrefix("test").build();

assertTrue(storage1.lock("a"));
// then
assertThat(storage1.lock("a")).isTrue();
}

@Test
public void cannotLockAlreadyLockedDirectory() {
// given
final RaftStorage storage1 =
RaftStorage.builder().withDirectory(PATH.toFile()).withPrefix("test").build();
storage1.lock("a");

// when
final RaftStorage storage2 =
RaftStorage.builder().withDirectory(PATH.toFile()).withPrefix("test").build();

assertFalse(storage2.lock("b"));
// then
assertThat(storage2.lock("b")).isFalse();
}

@Test
public void canAcquireLockOnDirectoryLockedBySameNode() {
Copy link
Member

Choose a reason for hiding this comment

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

💭 Just as an idea regarding another test. If we would make the writing to a file injectable (via dependency injection) we could also fail the writing and write a test whether failing write doesn't lock the storage anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 Ya. May be. I will pass it for now. It would be also good if we can inject a mock filesystem in which we can simulate all kinds of failure.

Copy link
Member

Choose a reason for hiding this comment

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

Agree but then we shouldn't longer use Files class :) or at least some wrapper :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think @npepinpe had looked into to similar ideas for testing journal.

// given
final RaftStorage storage1 =
RaftStorage.builder().withDirectory(PATH.toFile()).withPrefix("test").build();
storage1.lock("a");

// when
final RaftStorage storage3 =
RaftStorage.builder().withDirectory(PATH.toFile()).withPrefix("test").build();

assertTrue(storage3.lock("a"));
// then
assertThat(storage3.lock("a")).isTrue();
}

@Before
@After
public void cleanupStorage() throws IOException {
if (Files.exists(PATH)) {
Files.walkFileTree(
PATH,
new SimpleFileVisitor<>() {
@Override
public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs)
throws IOException {
Files.delete(file);
return FileVisitResult.CONTINUE;
}

@Override
public FileVisitResult postVisitDirectory(final Path dir, final IOException exc)
throws IOException {
Files.delete(dir);
return FileVisitResult.CONTINUE;
}
});
}
FileUtil.deleteFolderIfExists(PATH);
}
}