Merge pull request #13140 from vespa-engine/hakonhall/require-the-same-filesystem-provider

Require the same FileSystem provider

7 files changed, 79 insertions, 80 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..9f0c8d47d64 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,7 @@ public class NodeAgentContextImpl implements NodeAgentContext {
     private final AthenzIdentity identity;
     private final DockerNetworking dockerNetworking;
     private final ZoneApi zone;
+    private final FileSystem fileSystem;
     private final Path pathToNodeRootOnHost;
     private final Path pathToVespaHome;
     private final String vespaUser;
@@ -43,6 +43,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 +55,9 @@ 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.pathToNodeRootOnHost = requireValidPath(pathToContainerStorage).resolve(containerName.asString());
+        this.pathToVespaHome = requireValidPath(pathToVespaHome);
         this.logPrefix = containerName.asString() + ": ";
         this.vespaUser = vespaUser;
         this.vespaUserOnHost = vespaUserOnHost;
@@ -108,48 +110,41 @@ public class NodeAgentContextImpl implements NodeAgentContext {
     }
 
     @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));
     }
 
     @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 pathOnHost.getRoot().resolve(pathToNodeRootOnHost.relativize(pathOnHost));
     }
 
     @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));
+        return pathToVespaHome.resolve(relativePath);
     }
 
     @Override
@@ -168,6 +163,18 @@ public class NodeAgentContextImpl implements NodeAgentContext {
     }
 
     @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{" +
                 "node=" + node +
@@ -183,6 +190,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 +209,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 +252,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 +262,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 +300,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/container-storage"),
+                    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..cb615a94615 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
@@ -52,7 +52,7 @@ public class StorageMaintainerTest {
             NodeAgentContext context = new NodeAgentContextImpl.Builder("host-1.domain.tld").fileSystem(fileSystem).build();
             Files.createDirectories(context.pathOnHostFromPathInNode("/"));
 
-            terminal.expectCommand("du -xsk /home/docker/host-1 2>&1", 0, "321\t/home/docker/host-1/");
+            terminal.expectCommand("du -xsk /home/docker/container-storage/host-1 2>&1", 0, "321\t/home/docker/container-storage/host-1/");
             assertEquals(Optional.of(DiskSize.of(328_704)), storageMaintainer.diskUsageFor(context));
 
             // Value should still be cached, no new execution against the terminal
@@ -80,7 +80,7 @@ public class StorageMaintainerTest {
             NodeAgentContext context1 = createNodeAgentContextAndContainerStorage(fileSystem, "container-1");
             createNodeAgentContextAndContainerStorage(fileSystem, "container-2");
 
-            Path pathToArchiveDir = fileSystem.getPath("/home/docker/container-archive");
+            Path pathToArchiveDir = fileSystem.getPath("/home/docker/container-storage/container-archive");
             Files.createDirectories(pathToArchiveDir);
 
             Path containerStorageRoot = context1.pathOnHostFromPathInNode("/").getParent();
@@ -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 {
@@ -181,7 +181,7 @@ public class StorageMaintainerTest {
         }
 
         private void mockDiskUsage(long kBytes) {
-            terminal.expectCommand("du -xsk /home/docker/h123a 2>&1", 0, kBytes + "\t/path");
+            terminal.expectCommand("du -xsk /home/docker/container-storage/h123a 2>&1", 0, kBytes + "\t/path");
         }
     }
 }
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..9bcbce849af 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,16 +17,16 @@ 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() {
         assertEquals(
-                "/home/docker/container-1",
+                "/home/docker/container-storage/container-1",
                 context.pathOnHostFromPathInNode("/").toString());
 
         assertEquals(
-                "/home/docker/container-1/dev/null",
+                "/home/docker/container-storage/container-1/dev/null",
                 context.pathOnHostFromPathInNode("/dev/null").toString());
     }
 
@@ -39,7 +39,7 @@ public class NodeAgentContextImplTest {
     public void path_in_node_from_path_on_host_test() {
         assertEquals(
                 "/dev/null",
-                context.pathInNodeFromPathOnHost(fileSystem.getPath("/home/docker/container-1/dev/null")).toString());
+                context.pathInNodeFromPathOnHost(fileSystem.getPath("/home/docker/container-storage/container-1/dev/null")).toString());
     }
 
     @Test(expected=IllegalArgumentException.class)
@@ -49,7 +49,7 @@ public class NodeAgentContextImplTest {
 
     @Test(expected=IllegalArgumentException.class)
     public void path_on_host_must_be_inside_container_storage_of_context() {
-        context.pathInNodeFromPathOnHost(fileSystem.getPath("/home/docker/container-2/dev/null"));
+        context.pathInNodeFromPathOnHost(fileSystem.getPath("/home/docker/container-storage/container-2/dev/null"));
     }
 
     @Test(expected=IllegalArgumentException.class)