Merge pull request #13130 from vespa-engine/revert-13129-hakonhall/also-map-optvespalogsvespa

 Revert "Also map /opt/vespa/logs/vespa" and "Mount /etc/machine-id and /var/log/journal"

4 files changed, 42 insertions, 89 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 eb7d3ebe7fe..d7f248250a8 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
@@ -6,32 +6,32 @@ import com.yahoo.config.provision.DockerImage;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.SystemName;
+import com.yahoo.vespa.flags.FlagSource;
 import com.yahoo.vespa.hosted.dockerapi.Container;
 import com.yahoo.vespa.hosted.dockerapi.ContainerResources;
 import com.yahoo.vespa.hosted.dockerapi.ContainerStats;
 import com.yahoo.vespa.hosted.dockerapi.Docker;
 import com.yahoo.vespa.hosted.dockerapi.ProcessResult;
+import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeMembership;
 import com.yahoo.vespa.hosted.node.admin.nodeadmin.ConvergenceException;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.ContainerData;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
-import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath;
 import com.yahoo.vespa.hosted.node.admin.task.util.network.IPAddresses;
 import com.yahoo.vespa.hosted.node.admin.task.util.network.IPVersion;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.CommandResult;
 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.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
 import java.util.OptionalLong;
-import java.util.Random;
 import java.util.Set;
 import java.util.logging.Logger;
-import java.util.stream.Stream;
 
 /**
  * Class that wraps the Docker class and have some tools related to running programs in docker.
@@ -46,20 +46,15 @@ public class DockerOperationsImpl implements DockerOperations {
 
     private static final InetAddress IPV6_NPT_PREFIX = InetAddresses.forString("fd00::");
     private static final InetAddress IPV4_NPT_PREFIX = InetAddresses.forString("172.17.0.0");
-    public static final String ETC_MACHINE_ID = "/etc/machine-id";
-
-    private static final Random random = new Random(System.nanoTime());
 
     private final Docker docker;
     private final Terminal terminal;
     private final IPAddresses ipAddresses;
-    private final FileSystem fileSystem;
 
-    public DockerOperationsImpl(Docker docker, Terminal terminal, IPAddresses ipAddresses, FileSystem fileSystem) {
+    public DockerOperationsImpl(Docker docker, Terminal terminal, IPAddresses ipAddresses, FlagSource flagSource) {
         this.docker = docker;
         this.terminal = terminal;
         this.ipAddresses = ipAddresses;
-        this.fileSystem = fileSystem;
     }
 
     @Override
@@ -124,13 +119,6 @@ public class DockerOperationsImpl implements DockerOperations {
             command.withIpAddress(ipv4Address);
         }
 
-        UnixPath machineIdPath = new UnixPath(context.pathOnHostFromPathInNode(ETC_MACHINE_ID));
-        if (!machineIdPath.exists()) {
-            String machineId = String.format("%16x%16x\n", random.nextLong(), random.nextLong());
-            machineIdPath.createParents().writeUtf8File(machineId);
-            context.log(logger, "Wrote " + machineId + " to " + machineIdPath);
-        }
-
         addMounts(context, command);
 
         logger.info("Creating new container with args: " + command);
@@ -178,7 +166,7 @@ public class DockerOperationsImpl implements DockerOperations {
         ipV6Local.ifPresent(ipv6 -> etcHosts.append(ipv6.getHostAddress()).append('\t').append(hostname).append('\n'));
         ipV4Local.ifPresent(ipv4 -> etcHosts.append(ipv4.getHostAddress()).append('\t').append(hostname).append('\n'));
 
-        containerData.addFile(fileSystem.getPath("/etc/hosts"), etcHosts.toString());
+        containerData.addFile(Paths.get("/etc/hosts"), etcHosts.toString());
     }
 
     @Override
@@ -279,43 +267,46 @@ public class DockerOperationsImpl implements DockerOperations {
         return docker.getContainerStats(context.containerName());
     }
 
-    private void addMounts(NodeAgentContext context, Docker.CreateContainerCommand command) {
-        var volumes = new VolumeHelper(context, fileSystem, command);
+    private static void addMounts(NodeAgentContext context, Docker.CreateContainerCommand command) {
+        Path varLibSia = Paths.get("/var/lib/sia");
 
         // Paths unique to each container
-        volumes.addPrivateVolumes(
-                ETC_MACHINE_ID,  // VESPA-18110, rotation of journal
-                "/etc/vespa/flags", // local file db, to use flags before connection to cfg is established
-                "/etc/yamas-agent", // metrics check configuration
-                "/opt/splunkforwarder/var/log",  // VESPA-14917, thin pool leakage
-                "/var/log",                      // VESPA-14917, thin pool leakage
-                "/var/log/journal",              // VESPA-18110, rotation of journal, must map exact path
-                "/var/spool/postfix/maildrop",
-
-                // Under VESPA_HOME in container
-                "logs/vespa",
-                "logs/ysar",
-                "tmp",
-                "var/crash", // core dumps
-                "var/container-data",
-                "var/db/vespa",
-                "var/jdisc_container",
-                "var/vespa",
-                "var/yca",
-                "var/zookeeper");
+        List<Path> paths = new ArrayList<>(List.of(
+                Paths.get("/etc/vespa/flags"), // local file db, to use flags before connection to cfg is established
+                Paths.get("/etc/yamas-agent"), // metrics check configuration
+                Paths.get("/opt/splunkforwarder/var/log"),  // VESPA-14917, thin pool leakage
+                Paths.get("/var/log"),                      // VESPA-14917, thin pool leakage
+                Paths.get("/var/spool/postfix/maildrop"),   // VESPA-14917, thin pool leakage
+                context.pathInNodeUnderVespaHome("logs/vespa"),
+                context.pathInNodeUnderVespaHome("logs/ysar"),
+                context.pathInNodeUnderVespaHome("tmp"),
+                context.pathInNodeUnderVespaHome("var/crash"), // core dumps
+                context.pathInNodeUnderVespaHome("var/container-data"),
+                context.pathInNodeUnderVespaHome("var/db/vespa"),
+                context.pathInNodeUnderVespaHome("var/jdisc_container"),
+                context.pathInNodeUnderVespaHome("var/vespa"),
+                context.pathInNodeUnderVespaHome("var/yca"),
+                context.pathInNodeUnderVespaHome("var/zookeeper") // Tenant content nodes, config server and controller
+        ));
 
         if (context.nodeType() == NodeType.proxy) {
-            volumes.addPrivateVolumes("logs/nginx", "var/vespa-hosted/routing");
+            paths.add(context.pathInNodeUnderVespaHome("logs/nginx"));
+            paths.add(context.pathInNodeUnderVespaHome("var/vespa-hosted/routing"));
         } else if (context.nodeType() == NodeType.tenant)
-            volumes.addPrivateVolumes("/var/lib/sia");
+            paths.add(varLibSia);
+
+        paths.forEach(path -> command.withVolume(
+                context.pathOnHostFromPathInNode(path),
+                context.rewritePathInNodeForWantedDockerImage(path)));
+
 
         // Shared paths
         if (isInfrastructureHost(context.nodeType()))
-            volumes.addSharedVolumeMap("/var/lib/sia", "/var/lib/sia");
+            command.withSharedVolume(varLibSia, varLibSia);
 
         boolean isMain = context.zone().getSystemName() == SystemName.cd || context.zone().getSystemName() == SystemName.main;
         if (isMain && context.nodeType() == NodeType.tenant)
-            volumes.addSharedVolumeMap("/var/zpe", "var/zpe");
+            command.withSharedVolume(Paths.get("/var/zpe"), context.pathInNodeUnderVespaHome("var/zpe"));
     }
 
     @Override
@@ -335,41 +326,4 @@ public class DockerOperationsImpl implements DockerOperations {
                 nodeType == NodeType.controller;
     }
 
-    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) {
-            this.context = context;
-            this.fileSystem = fileSystem;
-            this.command = command;
-        }
-
-        /**
-         * Resolve each path to an absolute relative the container's vespa home directory.
-         * Mounts the resulting path, under the container's storage directory as path in the container.
-         */
-        public void addPrivateVolumes(String... pathsInNode) {
-            Stream.of(pathsInNode).forEach(pathString -> {
-                Path pathInNode = context.rewritePathInNodeForWantedDockerImage(resolveNodePath(pathString));
-                Path pathOnHost = context.pathOnHostFromPathInNode(pathInNode.toString());
-                command.withVolume(pathOnHost, pathInNode);
-            });
-        }
-
-        /**
-         * Mounts pathOnHost on the host as pathInNode in the container.  Use for paths that
-         * might be shared with other containers.
-         */
-        public void addSharedVolumeMap(String pathOnHost, String pathInNode) {
-            command.withSharedVolume(resolveNodePath(pathOnHost), resolveNodePath(pathInNode));
-        }
-
-        private Path resolveNodePath(String pathString) {
-            Path path = 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/NodeAgentContextImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java
index 9a021b7ade3..4585d72d6de 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
@@ -143,7 +143,7 @@ public class NodeAgentContextImpl implements NodeAgentContext {
         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());
+        return pathToVespaHome.resolve(relativePath);
     }
 
     @Override
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImplTest.java
index bacebcc42c0..1c6a82d0bef 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImplTest.java
@@ -3,6 +3,7 @@ package com.yahoo.vespa.hosted.node.admin.docker;
 
 import com.google.common.net.InetAddresses;
 import com.yahoo.config.provision.DockerImage;
+import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.dockerapi.Container;
 import com.yahoo.vespa.hosted.dockerapi.ContainerName;
 import com.yahoo.vespa.hosted.dockerapi.Docker;
@@ -13,12 +14,11 @@ import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextImpl;
 import com.yahoo.vespa.hosted.node.admin.task.util.network.IPAddresses;
 import com.yahoo.vespa.hosted.node.admin.task.util.network.IPAddressesMock;
 import com.yahoo.vespa.hosted.node.admin.task.util.process.TestTerminal;
-import com.yahoo.vespa.test.file.TestFileSystem;
 import org.junit.Test;
 import org.mockito.InOrder;
 
 import java.net.InetAddress;
-import java.nio.file.FileSystem;
+import java.nio.file.Paths;
 import java.util.Optional;
 import java.util.OptionalLong;
 
@@ -36,9 +36,8 @@ public class DockerOperationsImplTest {
     private final Docker docker = mock(Docker.class);
     private final TestTerminal terminal = new TestTerminal();
     private final IPAddresses ipAddresses = new IPAddressesMock();
-    private final FileSystem fileSystem = TestFileSystem.create();
     private final DockerOperationsImpl dockerOperations = new DockerOperationsImpl(
-            docker, terminal, ipAddresses, fileSystem);
+            docker, terminal, ipAddresses, new InMemoryFlagSource());
 
     @Test
     public void processResultFromNodeProgramWhenSuccess() {
@@ -99,7 +98,7 @@ public class DockerOperationsImplTest {
         dockerOperations.addEtcHosts(containerData, hostname, Optional.empty(), Optional.of(ipV6Local));
 
         verify(containerData, times(1)).addFile(
-                fileSystem.getPath("/etc/hosts"),
+                Paths.get("/etc/hosts"),
                 "# This file was generated by com.yahoo.vespa.hosted.node.admin.docker.DockerOperationsImpl\n" +
                         "127.0.0.1	localhost\n" +
                         "::1	localhost ip6-localhost ip6-loopback\n" +
@@ -112,7 +111,7 @@ public class DockerOperationsImplTest {
         dockerOperations.addEtcHosts(containerData, hostname, Optional.of(ipV4Local), Optional.of(ipV6Local));
 
         verify(containerData, times(1)).addFile(
-                fileSystem.getPath("/etc/hosts"),
+                Paths.get("/etc/hosts"),
                 "# This file was generated by com.yahoo.vespa.hosted.node.admin.docker.DockerOperationsImpl\n" +
                         "127.0.0.1	localhost\n" +
                         "::1	localhost ip6-localhost ip6-loopback\n" +
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..a1ea3558504 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
@@ -81,7 +81,7 @@ public class DockerTester implements AutoCloseable {
         Clock clock = Clock.systemUTC();
         Metrics metrics = new Metrics();
         FileSystem fileSystem = TestFileSystem.create();
-        DockerOperations dockerOperations = new DockerOperationsImpl(docker, terminal, ipAddresses, fileSystem);
+        DockerOperations dockerOperations = new DockerOperationsImpl(docker, terminal, ipAddresses, flagSource);
 
         NodeAgentFactory nodeAgentFactory = (contextSupplier, nodeContext) -> new NodeAgentImpl(
                 contextSupplier, nodeRepository, orchestrator, dockerOperations, storageMaintainer, flagSource,