Call maintainer through ProcessExecutor

6 files changed, 65 insertions, 54 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 615eddac645..99a7c5abfa0 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
@@ -25,7 +25,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
-import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Stream;
 
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 de4aacff09f..6cfa5efcc81 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
@@ -6,12 +6,10 @@ import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.yahoo.collections.Pair;
 import com.yahoo.io.IOUtils;
-import com.yahoo.log.LogLevel;
 import com.yahoo.net.HostName;
 import com.yahoo.system.ProcessExecuter;
 import com.yahoo.vespa.hosted.dockerapi.ContainerName;
 import com.yahoo.vespa.hosted.dockerapi.Docker;
-import com.yahoo.vespa.hosted.dockerapi.ProcessResult;
 import com.yahoo.vespa.hosted.dockerapi.metrics.CounterWrapper;
 import com.yahoo.vespa.hosted.dockerapi.metrics.Dimensions;
 import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper;
@@ -30,6 +28,7 @@ import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -38,6 +37,7 @@ import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
+import java.util.stream.Stream;
 
 import static com.yahoo.vespa.defaults.Defaults.getDefaults;
 
@@ -52,14 +52,16 @@ public class StorageMaintainer {
     private final Logger logger = Logger.getLogger(StorageMaintainer.class.getName());
     private final CounterWrapper numberOfNodeAdminMaintenanceFails;
     private final Docker docker;
+    private final ProcessExecuter processExecuter;
     private final Environment environment;
     private final Clock clock;
 
     private Map<ContainerName, MaintenanceThrottler> maintenanceThrottlerByContainerName = new ConcurrentHashMap<>();
 
 
-    public StorageMaintainer(Docker docker, MetricReceiverWrapper metricReceiver, Environment environment, Clock clock) {
+    public StorageMaintainer(Docker docker, ProcessExecuter processExecuter, MetricReceiverWrapper metricReceiver, Environment environment, Clock clock) {
         this.docker = docker;
+        this.processExecuter = processExecuter;
         this.environment = environment;
         this.clock = clock;
 
@@ -157,14 +159,6 @@ public class StorageMaintainer {
         return diskUsageKB * 1024;
     }
 
-    Optional<String> readMeminfo() {
-        try {
-            return Optional.of(new String(Files.readAllBytes(Paths.get("/proc/meminfo"))));
-        } catch (IOException e) {
-            logger.log(LogLevel.WARNING, "Failed to read meminfo", e);
-            return Optional.empty();
-        }
-    }
 
     /**
      * Deletes old log files for vespa, nginx, logstash, etc.
@@ -317,20 +311,33 @@ public class StorageMaintainer {
         return kernelVersion.orElse("unknown");
     }
 
+
+    private String executeMaintainer(String mainClass, String... args) {
+        String[] command = Stream.concat(
+                Stream.of("sudo", "-E", getDefaults().underVespaHome("libexec/vespa/node-admin/maintenance.sh"), mainClass),
+                Stream.of(args))
+                .toArray(String[]::new);
+
+        try {
+            Pair<Integer, String> result = processExecuter.exec(command);
+
+            if (result.getFirst() != 0) {
+                numberOfNodeAdminMaintenanceFails.add();
+                throw new RuntimeException(
+                        String.format("Maintainer failed to execute command: %s, Exit code: %d, Stdout/stderr: %s",
+                                Arrays.toString(command), result.getFirst(), result.getSecond()));
+            }
+            return result.getSecond();
+        } catch (IOException e) {
+            throw new RuntimeException("Failed to execute maintainer", e);
+        }
+    }
+
     /**
      * Wrapper for node-admin-maintenance, queues up maintenances jobs and sends a single request to maintenance JVM
      */
     private class MaintainerExecutor {
         private final List<MaintainerExecutorJob> jobs = new ArrayList<>();
-        private final ContainerName executeIn;
-
-        MaintainerExecutor(ContainerName executeIn) {
-            this.executeIn = executeIn;
-        }
-
-        MaintainerExecutor() {
-            this(NODE_ADMIN);
-        }
 
         MaintainerExecutorJob addJob(String jobName) {
             MaintainerExecutorJob job = new MaintainerExecutorJob(jobName);
@@ -346,16 +353,7 @@ public class StorageMaintainer {
                 throw new RuntimeException("Failed transform list of maintenance jobs to JSON");
             }
 
-            String[] command = {"java",
-                                "-cp", getDefaults().underVespaHome("lib/jars/node-maintainer-jar-with-dependencies.jar"),
-                    "-Dvespa.log.target=file:" + getDefaults().underVespaHome("logs/vespa/maintainer.log"),
-                    "com.yahoo.vespa.hosted.node.maintainer.Maintainer", args};
-            ProcessResult result = docker.executeInContainerAsRoot(executeIn, command);
-
-            if (! result.isSuccess()) {
-                numberOfNodeAdminMaintenanceFails.add();
-                throw new RuntimeException("Failed to run maintenance jobs: " + args + result);
-            }
+            executeMaintainer("com.yahoo.vespa.hosted.node.maintainer.Maintainer", args);
         }
     }
 
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java
index 99c78245b99..56c15804179 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java
@@ -59,13 +59,14 @@ public class ComponentsProviderImpl implements ComponentsProvider {
         }
 
         Clock clock = Clock.systemUTC();
+        ProcessExecuter processExecuter = new ProcessExecuter();
         ConfigServerHttpRequestExecutor requestExecutor = ConfigServerHttpRequestExecutor.create(configServerHosts);
         Orchestrator orchestrator = new OrchestratorImpl(requestExecutor);
         NodeRepository nodeRepository = new NodeRepositoryImpl(requestExecutor, WEB_SERVICE_PORT, baseHostName);
-        DockerOperations dockerOperations = new DockerOperationsImpl(docker, environment, new ProcessExecuter());
+        DockerOperations dockerOperations = new DockerOperationsImpl(docker, environment, processExecuter);
 
         Optional<StorageMaintainer> storageMaintainer = environment.isRunningLocally() ?
-                Optional.empty() : Optional.of(new StorageMaintainer(docker, metricReceiver, environment, clock));
+                Optional.empty() : Optional.of(new StorageMaintainer(docker, processExecuter, metricReceiver, environment, clock));
         Optional<AclMaintainer> aclMaintainer = environment.isRunningLocally() ?
                 Optional.empty() : Optional.of(new AclMaintainer(dockerOperations, nodeRepository, baseHostName));
 
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 15be1209965..735e8a574f7 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
@@ -52,7 +52,7 @@ public class DockerTester implements AutoCloseable {
                 .inetAddressResolver(inetAddressResolver)
                 .pathResolver(new PathResolver(Paths.get("/tmp"), Paths.get("/tmp"))).build();
         Clock clock = Clock.systemUTC();
-        StorageMaintainerMock storageMaintainer = new StorageMaintainerMock(dockerMock, environment, callOrderVerifier, clock);
+        StorageMaintainerMock storageMaintainer = new StorageMaintainerMock(dockerMock, null, environment, callOrderVerifier, clock);
 
 
         MetricReceiverWrapper mr = new MetricReceiverWrapper(MetricReceiver.nullImplementation);
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/StorageMaintainerMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/StorageMaintainerMock.java
index 04dc4010d8a..a7ce706063a 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/StorageMaintainerMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/StorageMaintainerMock.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.node.admin.integrationTests;
 
 import com.yahoo.metrics.simple.MetricReceiver;
+import com.yahoo.system.ProcessExecuter;
 import com.yahoo.vespa.hosted.dockerapi.ContainerName;
 import com.yahoo.vespa.hosted.dockerapi.Docker;
 import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper;
@@ -18,8 +19,8 @@ import java.util.Optional;
 public class StorageMaintainerMock extends StorageMaintainer {
     private final CallOrderVerifier callOrderVerifier;
 
-    public StorageMaintainerMock(Docker docker, Environment environment, CallOrderVerifier callOrderVerifier, Clock clock) {
-        super(docker, new MetricReceiverWrapper(MetricReceiver.nullImplementation), environment, clock);
+    public StorageMaintainerMock(Docker docker, ProcessExecuter processExecuter, Environment environment, CallOrderVerifier callOrderVerifier, Clock clock) {
+        super(docker, processExecuter, new MetricReceiverWrapper(MetricReceiver.nullImplementation), environment, clock);
         this.callOrderVerifier = callOrderVerifier;
     }
 
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 c4b0dfbe153..0324ff285d1 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
@@ -1,11 +1,12 @@
 // 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.yahoo.collections.Pair;
 import com.yahoo.metrics.simple.MetricReceiver;
+import com.yahoo.system.ProcessExecuter;
 import com.yahoo.test.ManualClock;
 import com.yahoo.vespa.hosted.dockerapi.ContainerName;
 import com.yahoo.vespa.hosted.dockerapi.Docker;
-import com.yahoo.vespa.hosted.dockerapi.ProcessResult;
 import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper;
 import com.yahoo.vespa.hosted.node.admin.ContainerNodeSpec;
 import com.yahoo.vespa.hosted.node.admin.util.Environment;
@@ -22,7 +23,6 @@ import java.time.Duration;
 
 import static org.junit.Assert.*;
 import static org.mockito.Matchers.any;
-import static org.mockito.Matchers.anyVararg;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -36,7 +36,8 @@ public class StorageMaintainerTest {
     private final Environment environment = new Environment.Builder()
             .pathResolver(new PathResolver()).build();
     private final Docker docker = mock(Docker.class);
-    private final StorageMaintainer storageMaintainer = new StorageMaintainer(docker,
+    private final ProcessExecuter processExecuter = mock(ProcessExecuter.class);
+    private final StorageMaintainer storageMaintainer = new StorageMaintainer(docker, processExecuter,
             new MetricReceiverWrapper(MetricReceiver.nullImplementation), environment, clock);
 
     @Rule
@@ -62,37 +63,39 @@ public class StorageMaintainerTest {
                 .nodeType("tenants")
                 .nodeFlavor("docker").build();
 
-        when(docker.executeInContainerAsRoot(any(), anyVararg())).thenReturn(new ProcessResult(0, "", ""));
+        try {
+            when(processExecuter.exec(any(String[].class))).thenReturn(new Pair<>(0, ""));
+        } catch (IOException ignored) { }
         storageMaintainer.removeOldFilesFromNode(containerName);
-        verify(docker, times(1)).executeInContainerAsRoot(any(), anyVararg());
+        verifyProcessExecuterCalled(1);
         // Will not actually run maintenance job until an hour passes
         storageMaintainer.removeOldFilesFromNode(containerName);
-        verify(docker, times(1)).executeInContainerAsRoot(any(), anyVararg());
+        verifyProcessExecuterCalled(1);
 
         // Coredump handler has its own throttler
         storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, environment);
-        verify(docker, times(2)).executeInContainerAsRoot(any(), anyVararg());
+        verifyProcessExecuterCalled(2);
 
 
         clock.advance(Duration.ofMinutes(61));
         storageMaintainer.removeOldFilesFromNode(containerName);
-        verify(docker, times(3)).executeInContainerAsRoot(any(), anyVararg());
+        verifyProcessExecuterCalled(3);
 
         storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, environment);
-        verify(docker, times(4)).executeInContainerAsRoot(any(), anyVararg());
+        verifyProcessExecuterCalled(4);
 
         storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, environment);
-        verify(docker, times(4)).executeInContainerAsRoot(any(), anyVararg());
+        verifyProcessExecuterCalled(4);
 
 
         // archiveNodeData is unthrottled and it should reset previous times
         storageMaintainer.archiveNodeData(containerName);
-        verify(docker, times(5)).executeInContainerAsRoot(any(), anyVararg());
+        verifyProcessExecuterCalled(5);
         storageMaintainer.archiveNodeData(containerName);
-        verify(docker, times(6)).executeInContainerAsRoot(any(), anyVararg());
+        verifyProcessExecuterCalled(6);
 
         storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, environment);
-        verify(docker, times(7)).executeInContainerAsRoot(any(), anyVararg());
+        verifyProcessExecuterCalled(7);
     }
 
     @Test
@@ -100,21 +103,30 @@ public class StorageMaintainerTest {
         String hostname = "node-123.us-north-3.test.yahoo.com";
         ContainerName containerName = ContainerName.fromHostname(hostname);
 
-        when(docker.executeInContainerAsRoot(any(), anyVararg()))
-                .thenThrow(new RuntimeException("Something went wrong"))
-                .thenReturn(new ProcessResult(0, "", ""));
+        try {
+            when(processExecuter.exec(any(String[].class)))
+                    .thenThrow(new RuntimeException("Something went wrong"))
+                    .thenReturn(new Pair<>(0, ""));
+        } catch (IOException ignored) { }
+
         try {
             storageMaintainer.removeOldFilesFromNode(containerName);
             fail("Maintenance job should've failed!");
         } catch (RuntimeException ignored) { }
-        verify(docker, times(1)).executeInContainerAsRoot(any(), anyVararg());
+        verifyProcessExecuterCalled(1);
 
         // Maintenance job failed, we should be able to immediately re-run it
         storageMaintainer.removeOldFilesFromNode(containerName);
-        verify(docker, times(2)).executeInContainerAsRoot(any(), anyVararg());
+        verifyProcessExecuterCalled(2);
     }
 
     private static void writeNBytesToFile(File file, int nBytes) throws IOException {
         Files.write(file.toPath(), new byte[nBytes]);
     }
+
+    private void verifyProcessExecuterCalled(int times) {
+        try {
+            verify(processExecuter, times(times)).exec(any(String[].class));
+        } catch (IOException ignored) { }
+    }
 }