Use Terminal in StorageMaintainer

2 files changed, 52 insertions, 53 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java
index 2c627e3f6e9..a849cef31ff 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java
@@ -1,23 +1,21 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.node.admin.maintenance;
 
+import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
-import com.google.common.cache.CacheLoader;
-import com.google.common.cache.LoadingCache;
 import com.yahoo.config.provision.NodeType;
-import com.yahoo.io.IOUtils;
 import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.hosted.dockerapi.Container;
+import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec;
 import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.FileFinder;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.Terminal;
 import com.yahoo.vespa.hosted.node.admin.util.SecretAgentCheckConfig;
 import com.yahoo.vespa.hosted.node.admin.maintenance.coredump.CoredumpHandler;
 
-import java.io.IOException;
-import java.io.InputStreamReader;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.time.Duration;
@@ -31,7 +29,6 @@ import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
-import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
@@ -48,21 +45,19 @@ public class StorageMaintainer {
     private static final DateTimeFormatter DATE_TIME_FORMATTER = DateTimeFormatter
             .ofPattern("yyyyMMddHHmmss").withZone(ZoneOffset.UTC);
 
+    private final Terminal terminal;
     private final DockerOperations dockerOperations;
     private final CoredumpHandler coredumpHandler;
     private final Path archiveContainerStoragePath;
 
     // We cache disk usage to avoid doing expensive disk operations so often
-    private LoadingCache<Path, Long> diskUsage = CacheBuilder.newBuilder()
+    private final Cache<Path, Long> diskUsage = CacheBuilder.newBuilder()
             .maximumSize(100)
             .expireAfterWrite(5, TimeUnit.MINUTES)
-            .build(new CacheLoader<Path, Long>() {
-                public Long load(Path containerDir) throws IOException, InterruptedException {
-                    return getDiskUsedInBytes(containerDir);
-                }
-            });
+            .build();
 
-    public StorageMaintainer(DockerOperations dockerOperations, CoredumpHandler coredumpHandler, Path archiveContainerStoragePath) {
+    public StorageMaintainer(Terminal terminal, DockerOperations dockerOperations, CoredumpHandler coredumpHandler, Path archiveContainerStoragePath) {
+        this.terminal = terminal;
         this.dockerOperations = dockerOperations;
         this.coredumpHandler = coredumpHandler;
         this.archiveContainerStoragePath = archiveContainerStoragePath;
@@ -186,37 +181,37 @@ public class StorageMaintainer {
     }
 
     public Optional<Long> getDiskUsageFor(NodeAgentContext context) {
-        Path containerDir = context.pathOnHostFromPathInNode("/");
         try {
-            return Optional.of(getDiskUsageFor(containerDir));
+            Path path = context.pathOnHostFromPathInNode("/");
+
+            Long cachedDiskUsage = diskUsage.getIfPresent(path);
+            if (cachedDiskUsage != null) return Optional.of(cachedDiskUsage);
+
+            long diskUsageBytes = getDiskUsedInBytes(context, path);
+            diskUsage.put(path, diskUsageBytes);
+            return Optional.of(diskUsageBytes);
         } catch (Exception e) {
-            context.log(logger, LogLevel.WARNING, "Problems during disk usage calculations in " + containerDir.toAbsolutePath(), e);
+            context.log(logger, LogLevel.WARNING, "Failed to get disk usage", e);
             return Optional.empty();
         }
     }
 
-    long getDiskUsageFor(Path containerDir) throws ExecutionException {
-        return diskUsage.get(containerDir);
-    }
-
     // Public for testing
-    long getDiskUsedInBytes(Path path) throws IOException, InterruptedException {
+    long getDiskUsedInBytes(TaskContext context, Path path) {
         if (!Files.exists(path)) return 0;
 
-        Process duCommand = new ProcessBuilder().command("du", "-xsk", path.toString()).start();
-        if (!duCommand.waitFor(60, TimeUnit.SECONDS)) {
-            duCommand.destroy();
-            duCommand.waitFor();
-            throw new RuntimeException("Disk usage command timed out, aborting.");
-        }
-        String output = IOUtils.readAll(new InputStreamReader(duCommand.getInputStream()));
+        String output = terminal.newCommandLine(context)
+                .add("du", "-xsk", path.toString())
+                .setTimeout(Duration.ofSeconds(60))
+                .executeSilently()
+                .getOutput();
+
         String[] results = output.split("\t");
         if (results.length != 2) {
             throw new RuntimeException("Result from disk usage command not as expected: " + output);
         }
 
-        long diskUsageKB = Long.valueOf(results[0]);
-        return diskUsageKB * 1024;
+        return 1024 * Long.valueOf(results[0]);
     }
 
 
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 81a0c4a5e20..d276bad9521 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
@@ -13,30 +13,30 @@ import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextImpl;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.FileFinder;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath;
+import com.yahoo.vespa.hosted.node.admin.task.util.process.TestTerminal;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.test.file.TestFileSystem;
-import org.junit.Rule;
+import org.junit.After;
 import org.junit.Test;
 import org.junit.experimental.runners.Enclosed;
-import org.junit.rules.TemporaryFolder;
 import org.junit.runner.RunWith;
 
 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.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
-import java.util.concurrent.ExecutionException;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static com.yahoo.vespa.hosted.node.admin.task.util.file.IOExceptionUtil.uncheck;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 
 /**
@@ -47,7 +47,7 @@ public class StorageMaintainerTest {
     private static final DockerOperations docker = mock(DockerOperations.class);
 
     public static class SecretAgentCheckTests {
-        private final StorageMaintainer storageMaintainer = new StorageMaintainer(docker, null, null);
+        private final StorageMaintainer storageMaintainer = new StorageMaintainer(null, docker, null, null);
 
         @Test
         public void tenant() {
@@ -190,30 +190,34 @@ public class StorageMaintainerTest {
     }
 
     public static class DiskUsageTests {
-        @Rule
-        public TemporaryFolder folder = new TemporaryFolder();
+
+        private final TestTerminal terminal = new TestTerminal();
 
         @Test
-        public void testDiskUsed() throws IOException, ExecutionException {
-            StorageMaintainer storageMaintainer = new StorageMaintainer(docker, null, null);
-            int writeSize = 10000;
-            Files.write(folder.newFile().toPath(), new byte[writeSize]);
-
-            long usedBytes = storageMaintainer.getDiskUsageFor(folder.getRoot().toPath());
-            if (usedBytes * 4 < writeSize || usedBytes > writeSize * 4)
-                fail("Used bytes is " + usedBytes + ", but wrote " + writeSize + " bytes, not even close.");
-
-            // Write another file, since disk usage is cached it should not change
-            Files.write(folder.newFile().toPath(), new byte[writeSize]);
-            assertEquals(usedBytes, storageMaintainer.getDiskUsageFor(folder.getRoot().toPath()));
+        public void testDiskUsed() throws IOException {
+            StorageMaintainer storageMaintainer = new StorageMaintainer(terminal, docker, null, null);
+            FileSystem fileSystem = TestFileSystem.create();
+            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/");
+            assertEquals(Optional.of(328_704L), storageMaintainer.getDiskUsageFor(context));
+
+            // Value should still be cached, no new execution against the terminal
+            assertEquals(Optional.of(328_704L), storageMaintainer.getDiskUsageFor(context));
         }
 
         @Test
-        public void testNonExistingDiskUsed() throws IOException, InterruptedException {
-            StorageMaintainer storageMaintainer = new StorageMaintainer(docker, null, null);
-            long usedBytes = storageMaintainer.getDiskUsedInBytes(folder.getRoot().toPath().resolve("doesn't exist"));
+        public void testNonExistingDiskUsed() {
+            StorageMaintainer storageMaintainer = new StorageMaintainer(terminal, docker, null, null);
+            long usedBytes = storageMaintainer.getDiskUsedInBytes(null, Paths.get("/fake/path"));
             assertEquals(0L, usedBytes);
         }
+
+        @After
+        public void after() {
+            terminal.verifyAllCommandsExecuted();
+        }
     }
 
     public static class ArchiveContainerDataTests {
@@ -237,7 +241,7 @@ public class StorageMaintainerTest {
 
 
             // Archive container-1
-            StorageMaintainer storageMaintainer = new StorageMaintainer(docker, null, pathToArchiveDir);
+            StorageMaintainer storageMaintainer = new StorageMaintainer(null, docker, null, pathToArchiveDir);
             storageMaintainer.archiveNodeStorage(context1);
 
             // container-1 should be gone from container-storage