Ignore files not named by a sha1 in the replication task storage On NFS, deleting or renaming a file that a process on the same client still has open turns it into a hidden ".nfsXXXX" file, which the client removes once the process closes it. If the process dies first, the file stays behind for good. With several primaries sharing the task storage, this can happen to a task file that a primary is reading while it starts or finishes the same task. ReplicationTasksStorage listed such a file as a task, keyed by its file name instead of the sha1 of the task. With the replication distributor enabled this loops forever: every distribution cycle replays the file, the resulting push cannot match the stored task by sha1, so it is pruned as externally completed and scheduled again on the next cycle, on every primary. The push never runs, since starting it renames the file named after the sha1, which does not exist. For an ..all.. task this schedules and cancels a full push of the project every few seconds, for as long as the file exists. Task files are always named after the sha1 of their task, so skip files whose name is not a sha1 when walking the storage. This also covers other stray files, such as editor or rsync leftovers. Release-Notes: Ignore files not named by a sha1, such as NFS ".nfsXXXX" files, in the replication task storage, which the distributor kept rescheduling forever Change-Id: I907be49b01103f11e483f8afdd2b6f2d983432fd
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java index bc0d900..02d50f9 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java +++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorage.java
@@ -69,6 +69,9 @@ * * <p>Tasks are moved atomically via a rename between those directories to indicate the current * state of each task. + * + * <p>Files whose name is not a sha1 are not tasks and are ignored, such as the ".nfsXXXX" files NFS + * keeps for deleted files that are still open. */ @Singleton public class ReplicationTasksStorage { @@ -230,7 +233,7 @@ try { return Files.list(path).flatMap(sub -> walkNonDirs(sub)); } catch (NotDirectoryException e) { - return Stream.of(path); + return isTaskFileName(path) ? Stream.of(path) : Stream.empty(); } catch (Exception e) { String message = "Error while walking directory"; if (isMultiPrimary() && e instanceof NoSuchFileException) { @@ -244,6 +247,11 @@ } } + private static boolean isTaskFileName(Path path) { + Path name = path.getFileName(); + return name != null && ObjectId.isId(name.toString()); + } + @SuppressWarnings("deprecation") private static ObjectId sha1(String s) { return ObjectId.fromRaw(Hashing.sha1().hashString(s, UTF_8).asBytes());
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationDistributorIT.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationDistributorIT.java index 5256bbe..db41376 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationDistributorIT.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationDistributorIT.java
@@ -24,6 +24,8 @@ import com.google.gerrit.entities.Project; import com.google.gerrit.server.git.WorkQueue; import com.googlesource.gerrit.plugins.replication.api.ReplicationConfig.FilterType; +import java.nio.file.Files; +import java.nio.file.Path; import java.time.Duration; import java.util.List; import java.util.Set; @@ -146,6 +148,33 @@ .get(uri); } + @Test + public void distributorIgnoresNfsFiles() throws Exception { + String remote = "foo"; + String replica = "replica"; + Project.NameKey targetProject = createTestProject(project + replica); + setReplicationDestination(remote, replica, ALL_PROJECTS); + reloadConfig(); + + ReplicationTasksStorage.ReplicateRefUpdate update = + ReplicationTasksStorage.ReplicateRefUpdate.create( + project.get(), + Set.of("refs/heads/foo_branch"), + new URIish(getProjectUri(targetProject)), + remote); + // NFS leaves this behind for a task file deleted while open by a process that then died + Path waiting = replicationConfig.getEventsDirectory().resolve("ref-updates").resolve("waiting"); + tasksStorage.create(update); + Files.move(waiting.resolve(update.sha1()), waiting.resolve(".nfs000000004cc412bd000014b6")); + + assertThrows( + InterruptedException.class, + () -> + WaitUtil.waitUntil( + () -> !getProjectTasks().isEmpty(), + Duration.ofSeconds(TEST_DISTRIBUTION_CYCLE_SECONDS * 2))); + } + private List<WorkQueue.Task<?>> getProjectTasks() { return getInstance(WorkQueue.class).getTasks().stream() .filter(t -> t instanceof WorkQueue.ProjectTask)
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java index cb1d541..4ba8545 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
@@ -27,6 +27,7 @@ import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.ReplicateRefUpdate; import java.net.URISyntaxException; import java.nio.file.FileSystem; +import java.nio.file.Files; import java.nio.file.Path; import java.util.Set; import java.util.stream.Collectors; @@ -82,6 +83,23 @@ } @Test + public void doesNotListFilesNotNamedBySha1() throws Exception { + storage.create(REF_UPDATE); + copyWithForeignNames(storageSite.resolve("waiting")); + assertThatStream(storage.streamWaiting()).containsExactly(STORED_REF_UPDATE); + + storage.start(uriUpdates); + copyWithForeignNames(storageSite.resolve("running")); + assertThatStream(storage.streamRunning()).containsExactly(STORED_REF_UPDATE); + } + + private static void copyWithForeignNames(Path dir) throws Exception { + Path task = dir.resolve(REF_UPDATE.sha1()); + Files.copy(task, dir.resolve(".nfs000000004cc412bd000014b6")); + Files.copy(task, dir.resolve(REF_UPDATE.sha1() + ".orig")); + } + + @Test public void canCheckIfUpdateIsWaiting() { storage.create(REF_UPDATE); assertTrue(storage.isWaiting(uriUpdates));