From 2de4490282f123989a2ee8fbb7e5c92968d876c1 Mon Sep 17 00:00:00 2001 From: Håkon Hallingstad Date: Sun, 3 May 2020 21:52:53 +0200 Subject: Require the same FileSystem provider --- .../node/admin/docker/DockerOperationsImpl.java | 13 ++-- .../node/admin/nodeagent/NodeAgentContext.java | 30 ++++--- .../node/admin/nodeagent/NodeAgentContextImpl.java | 91 ++++++++++++---------- .../node/admin/integrationTests/DockerTester.java | 2 +- .../admin/maintenance/StorageMaintainerTest.java | 4 +- .../maintenance/coredump/CoredumpHandlerTest.java | 3 +- .../admin/nodeagent/NodeAgentContextImplTest.java | 2 +- 7 files changed, 73 insertions(+), 72 deletions(-) diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java index 7294018c24a..ae8fdfb9d2e 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java @@ -23,7 +23,6 @@ import com.yahoo.vespa.hosted.node.admin.task.util.process.Terminal; import java.net.InetAddress; import java.nio.file.FileSystem; import java.nio.file.Path; -import java.nio.file.Paths; import java.time.Duration; import java.util.Arrays; import java.util.List; @@ -281,7 +280,7 @@ public class DockerOperationsImpl implements DockerOperations { } private void addMounts(NodeAgentContext context, Docker.CreateContainerCommand command) { - var volumes = new VolumeHelper(context, fileSystem, command); + var volumes = new VolumeHelper(context, command); // Paths unique to each container volumes.addPrivateVolumes( @@ -338,12 +337,10 @@ public class DockerOperationsImpl implements DockerOperations { private static class VolumeHelper { private final NodeAgentContext context; - private final FileSystem fileSystem; private final Docker.CreateContainerCommand command; - public VolumeHelper(NodeAgentContext context, FileSystem fileSystem, Docker.CreateContainerCommand command) { + public VolumeHelper(NodeAgentContext context, Docker.CreateContainerCommand command) { this.context = context; - this.fileSystem = fileSystem; this.command = command; } @@ -353,8 +350,8 @@ public class DockerOperationsImpl implements DockerOperations { */ public void addPrivateVolumes(String... pathsInNode) { Stream.of(pathsInNode).forEach(pathString -> { - Path absolutePathInNode = Paths.get(resolveNodePath(pathString).toString()); - Path pathOnHost = context.pathOnHostFromPathInNode(absolutePathInNode.toString()); + Path absolutePathInNode = resolveNodePath(pathString); + Path pathOnHost = context.pathOnHostFromPathInNode(absolutePathInNode); Path pathInNode = context.rewritePathInNodeForWantedDockerImage(absolutePathInNode); command.withVolume(pathOnHost, pathInNode); }); @@ -369,7 +366,7 @@ public class DockerOperationsImpl implements DockerOperations { } private Path resolveNodePath(String pathString) { - Path path = fileSystem.getPath(pathString); + Path path = context.fileSystem().getPath(pathString); return path.isAbsolute() ? path : context.pathInNodeUnderVespaHome(path); } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java index 2820fb2fa70..d589000c07e 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java @@ -11,8 +11,8 @@ import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; import com.yahoo.vespa.hosted.node.admin.docker.DockerNetworking; +import java.nio.file.FileSystem; import java.nio.file.Path; -import java.nio.file.Paths; public interface NodeAgentContext extends TaskContext { @@ -52,6 +52,9 @@ public interface NodeAgentContext extends TaskContext { */ double unscaledVcpu(); + /** The file system used by the NodeAgentContext. All paths must have the same provider. */ + FileSystem fileSystem(); + /** * This method is the inverse of {@link #pathInNodeFromPathOnHost(Path)}} * @@ -60,8 +63,9 @@ public interface NodeAgentContext extends TaskContext { */ Path pathOnHostFromPathInNode(Path pathInNode); - /** @see #pathOnHostFromPathInNode(Path) */ - Path pathOnHostFromPathInNode(String pathInNode); + default Path pathOnHostFromPathInNode(String pathInNode) { + return pathOnHostFromPathInNode(fileSystem().getPath(pathInNode)); + } /** * This method is the inverse of {@link #pathOnHostFromPathInNode(Path)} @@ -71,9 +75,9 @@ public interface NodeAgentContext extends TaskContext { */ Path pathInNodeFromPathOnHost(Path pathOnHost); - /** @see #pathOnHostFromPathInNode(Path) */ - Path pathInNodeFromPathOnHost(String pathOnHost); - + default Path pathInNodeFromPathOnHost(String pathOnHost) { + return pathInNodeFromPathOnHost(fileSystem().getPath(pathOnHost)); + } /** * @param relativePath relative path under Vespa home in container @@ -81,8 +85,9 @@ public interface NodeAgentContext extends TaskContext { */ Path pathInNodeUnderVespaHome(Path relativePath); - /** @see #pathInNodeUnderVespaHome(Path) */ - Path pathInNodeUnderVespaHome(String relativePath); + default Path pathInNodeUnderVespaHome(String relativePath) { + return pathInNodeUnderVespaHome(fileSystem().getPath(relativePath)); + } /** * Rewrite the given path in node to a path required by the image. @@ -90,12 +95,5 @@ public interface NodeAgentContext extends TaskContext { * configuring mounts. * TODO: Remove when everyone has migrated of vespa/ci image */ - default Path rewritePathInNodeForWantedDockerImage(Path path) { - if (!node().wantedDockerImage().get().repository().endsWith("/vespa/ci")) return path; - - Path originalVespaHome = pathInNodeUnderVespaHome(""); - if (!path.startsWith(originalVespaHome)) return path; - - return Paths.get("/home/y").resolve(originalVespaHome.relativize(path)); - } + Path rewritePathInNodeForWantedDockerImage(Path path); } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java index 9a021b7ade3..b12a6b93801 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java @@ -15,7 +15,7 @@ import com.yahoo.vespa.hosted.node.admin.docker.DockerNetworking; import java.nio.file.FileSystem; import java.nio.file.FileSystems; import java.nio.file.Path; -import java.nio.file.Paths; +import java.nio.file.ProviderMismatchException; import java.util.Objects; import java.util.Optional; import java.util.function.Function; @@ -26,7 +26,6 @@ import java.util.logging.Logger; * @author freva */ public class NodeAgentContextImpl implements NodeAgentContext { - private static final Path ROOT = Paths.get("/"); private final String logPrefix; private final NodeSpec node; @@ -35,6 +34,8 @@ public class NodeAgentContextImpl implements NodeAgentContext { private final AthenzIdentity identity; private final DockerNetworking dockerNetworking; private final ZoneApi zone; + private final FileSystem fileSystem; + private final Path root; private final Path pathToNodeRootOnHost; private final Path pathToVespaHome; private final String vespaUser; @@ -43,6 +44,7 @@ public class NodeAgentContextImpl implements NodeAgentContext { public NodeAgentContextImpl(NodeSpec node, Acl acl, AthenzIdentity identity, DockerNetworking dockerNetworking, ZoneApi zone, + FileSystem fileSystem, Path pathToContainerStorage, Path pathToVespaHome, String vespaUser, String vespaUserOnHost, double cpuSpeedup) { if (cpuSpeedup <= 0) @@ -54,8 +56,10 @@ public class NodeAgentContextImpl implements NodeAgentContext { this.identity = Objects.requireNonNull(identity); this.dockerNetworking = Objects.requireNonNull(dockerNetworking); this.zone = Objects.requireNonNull(zone); - this.pathToNodeRootOnHost = Objects.requireNonNull(pathToContainerStorage).resolve(containerName.asString()); - this.pathToVespaHome = Objects.requireNonNull(pathToVespaHome); + this.fileSystem = fileSystem; + this.root = fileSystem.getPath("/"); + this.pathToNodeRootOnHost = requireValidPath(pathToContainerStorage).resolve(containerName.asString()); + this.pathToVespaHome = requireValidPath(pathToVespaHome); this.logPrefix = containerName.asString() + ": "; this.vespaUser = vespaUser; this.vespaUserOnHost = vespaUserOnHost; @@ -107,51 +111,44 @@ public class NodeAgentContextImpl implements NodeAgentContext { return node.vcpu() / cpuSpeedup; } + @Override + public FileSystem fileSystem() { + return fileSystem; + } + @Override public Path pathOnHostFromPathInNode(Path pathInNode) { + requireValidPath(pathInNode); + if (! pathInNode.isAbsolute()) throw new IllegalArgumentException("Expected an absolute path in the container, got: " + pathInNode); - return pathToNodeRootOnHost.resolve(ROOT.relativize(pathInNode).toString()); - } - - @Override - public Path pathOnHostFromPathInNode(String pathInNode) { - // Ensure the path is on the proper FileSystem - return pathOnHostFromPathInNode(ROOT.getFileSystem().getPath(pathInNode)); + return pathToNodeRootOnHost.resolve(pathInNode.getRoot().relativize(pathInNode).toString()); } @Override public Path pathInNodeFromPathOnHost(Path pathOnHost) { + requireValidPath(pathOnHost); + if (! pathOnHost.isAbsolute()) throw new IllegalArgumentException("Expected an absolute path on the host, got: " + pathOnHost); if (!pathOnHost.startsWith(pathToNodeRootOnHost)) throw new IllegalArgumentException("Path " + pathOnHost + " does not exist in the container"); - return ROOT.resolve(pathToNodeRootOnHost.relativize(pathOnHost).toString()); - } - - @Override - public Path pathInNodeFromPathOnHost(String pathOnHost) { - // Ensure the path is on the proper FileSystem - return pathInNodeFromPathOnHost(pathToNodeRootOnHost.getFileSystem().getPath(pathOnHost)); + return root.resolve(pathToNodeRootOnHost.relativize(pathOnHost).toString()); } @Override public Path pathInNodeUnderVespaHome(Path relativePath) { + requireValidPath(relativePath); + if (relativePath.isAbsolute()) throw new IllegalArgumentException("Expected a relative path to the Vespa home, got: " + relativePath); return relativePath.getFileSystem().getPath(pathToVespaHome.resolve(relativePath.toString()).toString()); } - @Override - public Path pathInNodeUnderVespaHome(String relativePath) { - // Ensure the path is on the proper FileSystem - return pathInNodeUnderVespaHome(pathToVespaHome.getFileSystem().getPath(relativePath)); - } - @Override public void recordSystemModification(Logger logger, String message) { log(logger, message); @@ -167,6 +164,18 @@ public class NodeAgentContextImpl implements NodeAgentContext { logger.log(level, logPrefix + message, throwable); } + @Override + public Path rewritePathInNodeForWantedDockerImage(Path path) { + requireValidPath(path); + + if (!node().wantedDockerImage().get().repository().endsWith("/vespa/ci")) return path; + + Path originalVespaHome = pathInNodeUnderVespaHome(""); + if (!path.startsWith(originalVespaHome)) return path; + + return fileSystem.getPath("/home/y").resolve(originalVespaHome.relativize(path)); + } + @Override public String toString() { return "NodeAgentContextImpl{" + @@ -183,6 +192,18 @@ public class NodeAgentContextImpl implements NodeAgentContext { '}'; } + private Path requireValidPath(Path path) { + Objects.requireNonNull(path); + + Objects.requireNonNull(fileSystem); // to allow this method to be used in constructor. + if (!path.getFileSystem().provider().equals(fileSystem.provider())) { + throw new ProviderMismatchException("Expected file system provider " + fileSystem.provider() + + " but " + path + " had " + path.getFileSystem().provider()); + } + + return path; + } + /** For testing only! */ public static class Builder { private NodeSpec.Builder nodeSpecBuilder; @@ -190,8 +211,6 @@ public class NodeAgentContextImpl implements NodeAgentContext { private AthenzIdentity identity; private DockerNetworking dockerNetworking; private ZoneApi zone; - private Path pathToContainerStorage; - private Path pathToVespaHome; private String vespaUser; private String vespaUserOnHost; private FileSystem fileSystem = FileSystems.getDefault(); @@ -235,16 +254,6 @@ public class NodeAgentContextImpl implements NodeAgentContext { return this; } - public Builder pathToContainerStorageFromFileSystem(FileSystem fileSystem) { - this.pathToContainerStorage = fileSystem.getPath("/home/docker"); - return this; - } - - public Builder pathToVespaHome(Path pathToVespaHome) { - this.pathToVespaHome = pathToVespaHome; - return this; - } - public Builder vespaUser(String vespaUser) { this.vespaUser = vespaUser; return this; @@ -255,10 +264,7 @@ public class NodeAgentContextImpl implements NodeAgentContext { return this; } - /** - * Sets the default file system to use for paths. May be overridden for each path, - * e.g. {@link #pathToVespaHome(Path)} pathToVespaHome()}. - */ + /** Sets the file system to use for paths. */ public Builder fileSystem(FileSystem fileSystem) { this.fileSystem = fileSystem; return this; @@ -296,8 +302,9 @@ public class NodeAgentContextImpl implements NodeAgentContext { return getId().region().value(); } }), - Optional.ofNullable(pathToContainerStorage).orElseGet(() -> fileSystem.getPath("/home/docker")), - Optional.ofNullable(pathToVespaHome).orElseGet(() -> fileSystem.getPath("/opt/vespa")), + fileSystem, + fileSystem.getPath("/home/docker"), + fileSystem.getPath("/opt/vespa"), Optional.ofNullable(vespaUser).orElse("vespa"), Optional.ofNullable(vespaUserOnHost).orElse("container_vespa"), cpuSpeedUp); diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java index 19f991b2ce8..bf14ac2ce5e 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java @@ -88,7 +88,7 @@ public class DockerTester implements AutoCloseable { Optional.empty(), Optional.empty(), Optional.empty(), clock, Duration.ofSeconds(-1)); nodeAdmin = new NodeAdminImpl(nodeAgentFactory, metrics, clock, Duration.ofMillis(10), Duration.ZERO); NodeAgentContextFactory nodeAgentContextFactory = (nodeSpec, acl) -> - new NodeAgentContextImpl.Builder(nodeSpec).acl(acl).pathToContainerStorageFromFileSystem(fileSystem).build(); + new NodeAgentContextImpl.Builder(nodeSpec).acl(acl).fileSystem(fileSystem).build(); nodeAdminStateUpdater = new NodeAdminStateUpdater(nodeAgentContextFactory, nodeRepository, orchestrator, nodeAdmin, HOST_HOSTNAME, clock); diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainerTest.java index 3a169795df2..3a3d087e644 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainerTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainerTest.java @@ -122,7 +122,7 @@ public class StorageMaintainerTest { private NodeAgentContext createNodeAgentContextAndContainerStorage(FileSystem fileSystem, String containerName) throws IOException { NodeAgentContext context = new NodeAgentContextImpl.Builder(containerName + ".domain.tld") - .pathToContainerStorageFromFileSystem(fileSystem).build(); + .fileSystem(fileSystem).build(); Path containerVespaHomeOnHost = context.pathOnHostFromPathInNode(context.pathInNodeUnderVespaHome("")); Files.createDirectories(context.pathOnHostFromPathInNode("/etc/something")); @@ -159,7 +159,7 @@ public class StorageMaintainerTest { private final FileSystem fileSystem = TestFileSystem.create(); private final NodeAgentContext context = new NodeAgentContextImpl .Builder(NodeSpec.Builder.testSpec("h123a.domain.tld").resources(new NodeResources(1, 1, 1, 1)).build()) - .pathToContainerStorageFromFileSystem(fileSystem).build(); + .fileSystem(fileSystem).build(); @Test public void not_run_if_not_enough_used() throws IOException { diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java index 8aa4fb9131b..30a08483cf2 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java @@ -17,7 +17,6 @@ import java.io.IOException; import java.nio.file.FileSystem; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.Paths; import java.nio.file.attribute.FileTime; import java.time.Duration; import java.time.Instant; @@ -164,7 +163,7 @@ public class CoredumpHandlerTest { "}}"; - Path coredumpDirectoryInContainer = Paths.get("/var/crash/id-123"); + Path coredumpDirectoryInContainer = fileSystem.getPath("/var/crash/id-123"); Path coredumpDirectory = context.pathOnHostFromPathInNode(coredumpDirectoryInContainer); Files.createDirectories(coredumpDirectory); Files.createFile(coredumpDirectory.resolve("dump_core.456")); diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImplTest.java index 250c005566b..d8e97ee47b7 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImplTest.java @@ -17,7 +17,7 @@ import static org.junit.Assert.assertEquals; public class NodeAgentContextImplTest { private final FileSystem fileSystem = TestFileSystem.create(); private final NodeAgentContext context = new NodeAgentContextImpl.Builder("container-1.domain.tld") - .pathToContainerStorageFromFileSystem(fileSystem).build(); + .fileSystem(fileSystem).build(); @Test public void path_on_host_from_path_in_node_test() { -- cgit v1.2.3