From 3056a27f7c6a4c71434048f79e2e104f5ee493ab Mon Sep 17 00:00:00 2001 From: Deepthi Devaki Akkoorath Date: Wed, 12 Oct 2022 13:37:35 +0200 Subject: [PATCH 1/2] fix(raft): ensure lock files is created and updated atomically This is required to prevent an empty lock files during restart, if the system crashed before the lock content is written to the file. (cherry picked from commit 8322db79364c34ea00aaa3d778bb67917dd3c62f) --- .../io/atomix/raft/storage/RaftStorage.java | 31 ++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/atomix/cluster/src/main/java/io/atomix/raft/storage/RaftStorage.java b/atomix/cluster/src/main/java/io/atomix/raft/storage/RaftStorage.java index 0a56f10c19c2..187cec654392 100644 --- a/atomix/cluster/src/main/java/io/atomix/raft/storage/RaftStorage.java +++ b/atomix/cluster/src/main/java/io/atomix/raft/storage/RaftStorage.java @@ -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; /** @@ -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( + 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); } } From 502e9ec70c64cc77fe50c007c3611a5991d5c1c6 Mon Sep 17 00:00:00 2001 From: Deepthi Devaki Akkoorath Date: Wed, 12 Oct 2022 13:37:57 +0200 Subject: [PATCH 2/2] test(raft): refactor RaftStorageTest (cherry picked from commit 8694c15fa36e2fe779306b04b2ae0339070612df) --- .../atomix/raft/storage/RaftStorageTest.java | 75 +++++++++---------- 1 file changed, 37 insertions(+), 38 deletions(-) diff --git a/atomix/cluster/src/test/java/io/atomix/raft/storage/RaftStorageTest.java b/atomix/cluster/src/test/java/io/atomix/raft/storage/RaftStorageTest.java index 9713d61d8451..7b75342595e9 100644 --- a/atomix/cluster/src/test/java/io/atomix/raft/storage/RaftStorageTest.java +++ b/atomix/cluster/src/test/java/io/atomix/raft/storage/RaftStorageTest.java @@ -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. */ @@ -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") @@ -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() { + // 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); } }