From 4ff1aade130142d0cf9e44cbbcd1591f7c1ee672 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mariusz=20J=C3=B3zala?= <377355+jozala@users.noreply.github.com> Date: Wed, 12 Mar 2025 16:09:55 +0100 Subject: [PATCH] [Tests] Fix copying files for test cluster (#124628) In case when file with `.attach_pid` in name was stored in distribution and then deleted, the exception could stop copying/linking files without any sign of issue. The files were then missing in the cluster used in the test causing them sometimes to fail (depending on which files haven't been copied). When using `Files.walk` it is impossible to catch the IOException and continue walking through files conditionally. It has been replaced with FileVisitor implementation to be able to continue if the exception is caused by files left temporarily by JVM but no longer available. --- .../testclusters/ElasticsearchNode.java | 66 +++++++++------- test/test-clusters/build.gradle | 4 + .../test/cluster/util/IOUtils.java | 55 +++++++------- .../test/cluster/util/IOUtilsTests.java | 75 +++++++++++++++++++ 4 files changed, 147 insertions(+), 53 deletions(-) create mode 100644 test/test-clusters/src/test/java/org/elasticsearch/test/cluster/util/IOUtilsTests.java diff --git a/build-tools/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java b/build-tools/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java index 2f0f85bc6e15..f6170693ce8c 100644 --- a/build-tools/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java +++ b/build-tools/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java @@ -61,11 +61,14 @@ import java.io.StringWriter; import java.io.UncheckedIOException; import java.net.URL; import java.nio.charset.StandardCharsets; +import java.nio.file.FileVisitResult; import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; +import java.nio.file.SimpleFileVisitor; import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; +import java.nio.file.attribute.BasicFileAttributes; import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; @@ -1295,40 +1298,47 @@ public class ElasticsearchNode implements TestClusterConfiguration { private void sync(Path sourceRoot, Path destinationRoot, BiConsumer syncMethod) { assert Files.exists(destinationRoot) == false; - try (Stream stream = Files.walk(sourceRoot)) { - stream.forEach(source -> { - Path relativeDestination = sourceRoot.relativize(source); - if (relativeDestination.getNameCount() <= 1) { - return; + try { + Files.walkFileTree(sourceRoot, new SimpleFileVisitor<>() { + @Override + public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { + Path relativeDestination = sourceRoot.relativize(dir); + if (relativeDestination.getNameCount() <= 1) { + return FileVisitResult.CONTINUE; + } + // Throw away the first name as the archives have everything in a single top level folder we are not interested in + relativeDestination = relativeDestination.subpath(1, relativeDestination.getNameCount()); + Path destination = destinationRoot.resolve(relativeDestination); + Files.createDirectories(destination); + return FileVisitResult.CONTINUE; } - // Throw away the first name as the archives have everything in a single top level folder we are not interested in - relativeDestination = relativeDestination.subpath(1, relativeDestination.getNameCount()); - Path destination = destinationRoot.resolve(relativeDestination); - if (Files.isDirectory(source)) { - try { - Files.createDirectories(destination); - } catch (IOException e) { - throw new UncheckedIOException("Can't create directory " + destination.getParent(), e); - } - } else { - try { - Files.createDirectories(destination.getParent()); - } catch (IOException e) { - throw new UncheckedIOException("Can't create directory " + destination.getParent(), e); + @Override + public FileVisitResult visitFile(Path source, BasicFileAttributes attrs) throws IOException { + Path relativeDestination = sourceRoot.relativize(source); + if (relativeDestination.getNameCount() <= 1) { + return FileVisitResult.CONTINUE; } + // Throw away the first name as the archives have everything in a single top level folder we are not interested in + relativeDestination = relativeDestination.subpath(1, relativeDestination.getNameCount()); + Path destination = destinationRoot.resolve(relativeDestination); + Files.createDirectories(destination.getParent()); syncMethod.accept(destination, source); + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException { + if (exc instanceof NoSuchFileException noFileException) { + // Ignore these files that are sometimes left behind by the JVM + if (noFileException.getFile() != null && noFileException.getFile().contains(".attach_pid")) { + LOGGER.info("Ignoring file left behind by JVM: {}", noFileException.getFile()); + return FileVisitResult.CONTINUE; + } + } + throw exc; } }); - } catch (UncheckedIOException e) { - if (e.getCause() instanceof NoSuchFileException cause) { - // Ignore these files that are sometimes left behind by the JVM - if (cause.getFile() == null || cause.getFile().contains(".attach_pid") == false) { - throw new UncheckedIOException(cause); - } - } else { - throw e; - } } catch (IOException e) { throw new UncheckedIOException("Can't walk source " + sourceRoot, e); } diff --git a/test/test-clusters/build.gradle b/test/test-clusters/build.gradle index d2c7633603f2..da87b75c2d58 100644 --- a/test/test-clusters/build.gradle +++ b/test/test-clusters/build.gradle @@ -9,6 +9,10 @@ dependencies { implementation "com.fasterxml.jackson.core:jackson-annotations:${versions.jackson}" implementation "com.fasterxml.jackson.core:jackson-databind:${versions.jackson}" implementation "org.elasticsearch.gradle:reaper" + + testImplementation "junit:junit:${versions.junit}" + testImplementation "org.hamcrest:hamcrest:${versions.hamcrest}" + testImplementation "org.apache.logging.log4j:log4j-core:${versions.log4j}" } tasks.named("processResources").configure { diff --git a/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/util/IOUtils.java b/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/util/IOUtils.java index b1e217520559..9289e47478e7 100644 --- a/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/util/IOUtils.java +++ b/test/test-clusters/src/main/java/org/elasticsearch/test/cluster/util/IOUtils.java @@ -15,9 +15,12 @@ import org.apache.logging.log4j.Logger; import java.io.File; import java.io.IOException; import java.io.UncheckedIOException; +import java.nio.file.FileVisitResult; import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.attribute.BasicFileAttributes; import java.util.Comparator; import java.util.function.BiConsumer; import java.util.stream.Stream; @@ -118,35 +121,37 @@ public final class IOUtils { private static void sync(Path sourceRoot, Path destinationRoot, BiConsumer syncMethod) { assert Files.exists(destinationRoot) == false; - try (Stream stream = Files.walk(sourceRoot)) { - stream.forEach(source -> { - Path relativeDestination = sourceRoot.relativize(source); + try { + Files.walkFileTree(sourceRoot, new SimpleFileVisitor<>() { + @Override + public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { + Path relativeDestination = sourceRoot.relativize(dir); + Path destination = destinationRoot.resolve(relativeDestination); + Files.createDirectories(destination); + return FileVisitResult.CONTINUE; + } - Path destination = destinationRoot.resolve(relativeDestination); - if (Files.isDirectory(source)) { - try { - Files.createDirectories(destination); - } catch (IOException e) { - throw new UncheckedIOException("Can't create directory " + destination.getParent(), e); - } - } else { - try { - Files.createDirectories(destination.getParent()); - } catch (IOException e) { - throw new UncheckedIOException("Can't create directory " + destination.getParent(), e); - } + @Override + public FileVisitResult visitFile(Path source, BasicFileAttributes attrs) throws IOException { + Path relativeDestination = sourceRoot.relativize(source); + Path destination = destinationRoot.resolve(relativeDestination); + Files.createDirectories(destination.getParent()); syncMethod.accept(destination, source); + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException { + if (exc instanceof NoSuchFileException noFileException) { + // Ignore these files that are sometimes left behind by the JVM + if (noFileException.getFile() != null && noFileException.getFile().contains(".attach_pid")) { + LOGGER.info("Ignoring file left behind by JVM: {}", noFileException.getFile()); + return FileVisitResult.CONTINUE; + } + } + throw exc; } }); - } catch (UncheckedIOException e) { - if (e.getCause() instanceof NoSuchFileException cause) { - // Ignore these files that are sometimes left behind by the JVM - if (cause.getFile() == null || cause.getFile().contains(".attach_pid") == false) { - throw new UncheckedIOException(cause); - } - } else { - throw e; - } } catch (IOException e) { throw new UncheckedIOException("Can't walk source " + sourceRoot, e); } diff --git a/test/test-clusters/src/test/java/org/elasticsearch/test/cluster/util/IOUtilsTests.java b/test/test-clusters/src/test/java/org/elasticsearch/test/cluster/util/IOUtilsTests.java new file mode 100644 index 000000000000..8c0de593fbcc --- /dev/null +++ b/test/test-clusters/src/test/java/org/elasticsearch/test/cluster/util/IOUtilsTests.java @@ -0,0 +1,75 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +package org.elasticsearch.test.cluster.util; + +import org.junit.Test; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.core.Is.is; +import static org.hamcrest.core.Is.isA; +import static org.junit.Assert.assertThrows; + +public class IOUtilsTests { + + @Test + public void testSyncWithLinks() throws IOException { + // given + Path sourceDir = Files.createTempDirectory("sourceDir"); + Files.createFile(sourceDir.resolve("file1.txt")); + Files.createFile(sourceDir.resolve("file2.txt")); + Files.createDirectory(sourceDir.resolve("nestedDir")); + Files.createFile(sourceDir.resolve("nestedDir").resolve("file3.txt")); + + Path baseDestinationDir = Files.createTempDirectory("baseDestinationDir"); + Path destinationDir = baseDestinationDir.resolve("destinationDir"); + + // when + IOUtils.syncWithLinks(sourceDir, destinationDir); + + // then + assertFileExists(destinationDir.resolve("file1.txt")); + assertFileExists(destinationDir.resolve("file2.txt")); + assertFileExists(destinationDir.resolve("nestedDir").resolve("file3.txt")); + } + + private void assertFileExists(Path path) throws IOException { + assertThat("File " + path + " doesn't exist", Files.exists(path), is(true)); + assertThat("File " + path + " is not a regular file", Files.isRegularFile(path), is(true)); + assertThat("File " + path + " is not readable", Files.isReadable(path), is(true)); + if (OS.current() != OS.WINDOWS) { + assertThat("Expected 2 hard links", Files.getAttribute(path, "unix:nlink"), is(2)); + } + } + + @Test + public void testSyncWithLinksThrowExceptionWhenDestinationIsNotWritable() throws IOException { + // given + Path sourceDir = Files.createTempDirectory("sourceDir"); + Files.createFile(sourceDir.resolve("file1.txt")); + + Path baseDestinationDir = Files.createTempDirectory("baseDestinationDir"); + Path destinationDir = baseDestinationDir.resolve("destinationDir"); + + baseDestinationDir.toFile().setWritable(false); + + // when + UncheckedIOException ex = assertThrows(UncheckedIOException.class, () -> IOUtils.syncWithLinks(sourceDir, destinationDir)); + + // then + assertThat(ex.getCause(), isA(IOException.class)); + assertThat(ex.getCause().getMessage(), containsString("destinationDir")); + } +}