Support persistent output file for CommandLine

5 files changed, 85 insertions, 13 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess2Impl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess2Impl.java
index 6c8b15b10a6..2854dc55af8 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess2Impl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess2Impl.java
@@ -89,7 +89,8 @@ public class ChildProcess2Impl implements ChildProcess2 {
     @Override
     public void close() {
         try {
-            Files.delete(outputPath);
+            if ( ! commandLine.getOutputFile().isPresent())
+                Files.delete(outputPath);
         } catch (Throwable t) {
             logger.log(LogLevel.WARNING, "Failed to delete " + outputPath, t);
         }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLine.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLine.java
index 940b3255766..8f39c2d257b 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLine.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLine.java
@@ -6,12 +6,14 @@ import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
 
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
 import java.util.function.Predicate;
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
@@ -51,6 +53,7 @@ public class CommandLine {
 
     private boolean redirectStderrToStdoutInsteadOfDiscard = true;
     private boolean executeSilentlyCalled = false;
+    private Optional<Path> outputFile = Optional.empty();
     private Charset outputEncoding = StandardCharsets.UTF_8;
     private Duration timeout = DEFAULT_TIMEOUT;
     private long maxOutputBytes = DEFAULT_MAX_OUTPUT_BYTES;
@@ -198,6 +201,16 @@ public class CommandLine {
     }
 
     /**
+     * By default, the output of the command is piped to a temporary file, which is deleted
+     * when execution ends. This method will cause output to be piped to the given path
+     * instead, and the file will not be removed.
+     */
+    public CommandLine setOutputFile(Path outputFile) {
+        this.outputFile = Optional.of(outputFile);
+        return this;
+    }
+
+    /**
      * By default, the command will be gracefully killed after DEFAULT_TIMEOUT. This method
      * overrides that default.
      */
@@ -235,6 +248,7 @@ public class CommandLine {
     // Accessor fields necessary for classes in this package. Could be public if necessary.
     boolean getRedirectStderrToStdoutInsteadOfDiscard() { return redirectStderrToStdoutInsteadOfDiscard; }
     Predicate<Integer> getSuccessfulExitCodePredicate() { return successfulExitCodePredicate; }
+    Optional<Path> getOutputFile() { return outputFile; }
     Charset getOutputEncoding() { return outputEncoding; }
     Duration getTimeout() { return timeout; }
     long getMaxOutputBytes() { return maxOutputBytes; }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImpl.java
index 78c25897e1d..a5f8e667ff2 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImpl.java
@@ -48,7 +48,7 @@ public class ProcessFactoryImpl implements ProcessFactory {
             processBuilder.redirectError(ProcessBuilder.Redirect.to(DEV_NULL));
         }
 
-        // The output is redirected to a temporary file because:
+        // The output is redirected to a file (temporary or user-defined) because:
         //  - We could read continuously from process.getInputStream, but that may block
         //    indefinitely with a faulty program.
         //  - If we don't read continuously from process.getInputStream, then because
@@ -60,27 +60,36 @@ public class ProcessFactoryImpl implements ProcessFactory {
         // has the benefit of allowing for inspection of the file during execution, and
         // allowing the inspection of the file if it e.g. gets too large to hold in-memory.
 
-        String temporaryFilePrefix =
-                ProcessFactoryImpl.class.getSimpleName() + "-" + commandLine.programName() + "-";
-
         FileAttribute<Set<PosixFilePermission>> fileAttribute = PosixFilePermissions.asFileAttribute(
                 PosixFilePermissions.fromString("rw-------"));
 
-        Path temporaryFile = uncheck(() -> Files.createTempFile(
-                temporaryFilePrefix,
-                ".out",
-                fileAttribute));
+        Path outputFile = commandLine.getOutputFile()
+                                     .map(file -> {
+                                         uncheck(() -> Files.deleteIfExists(file));
+                                         uncheck(() -> Files.createFile(file, fileAttribute));
+                                         return file;
+                                     })
+                                     .orElseGet(() -> {
+                                         String temporaryFilePrefix =
+                                                 ProcessFactoryImpl.class.getSimpleName() + "-" + commandLine.programName() + "-";
+
+                                         return uncheck(() -> Files.createTempFile(
+                                                 temporaryFilePrefix,
+                                                 ".out",
+                                                 fileAttribute));
+                                     });
 
         try {
-            processBuilder.redirectOutput(temporaryFile.toFile());
+            processBuilder.redirectOutput(outputFile.toFile());
             ProcessApi2 process = processStarter.start(processBuilder);
-            return new ChildProcess2Impl(commandLine, process, temporaryFile, timer);
+            return new ChildProcess2Impl(commandLine, process, outputFile, timer);
         } catch (RuntimeException | Error throwable) {
             try {
-                Files.delete(temporaryFile);
+                if ( ! commandLine.getOutputFile().isPresent())
+                    Files.delete(outputFile);
             } catch (IOException ioException) {
                 logger.log(LogLevel.WARNING, "Failed to delete temporary file at " +
-                        temporaryFile, ioException);
+                                             outputFile, ioException);
             }
             throw throwable;
         }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLineTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLineTest.java
index a5eb0ab059b..1531b06070e 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLineTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLineTest.java
@@ -2,12 +2,19 @@
 package com.yahoo.vespa.hosted.node.admin.task.util.process;
 
 import com.yahoo.vespa.hosted.node.admin.component.TestTaskContext;
+import com.yahoo.vespa.hosted.node.admin.task.util.time.TestTimer;
+import com.yahoo.vespa.test.file.TestFileSystem;
 import org.junit.After;
 import org.junit.Test;
 
+import java.io.IOException;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.FileSystem;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Optional;
 import java.util.function.Predicate;
 
 import static org.junit.Assert.assertEquals;
@@ -61,6 +68,7 @@ public class CommandLineTest {
         assertEquals(CommandLine.DEFAULT_SIGTERM_GRACE_PERIOD, commandLine.getSigTermGracePeriod());
         assertEquals(CommandLine.DEFAULT_SIGKILL_GRACE_PERIOD, commandLine.getSigKillGracePeriod());
         assertEquals(0, commandLine.getArguments().size());
+        assertEquals(Optional.empty(), commandLine.getOutputFile());
         assertEquals(StandardCharsets.UTF_8, commandLine.getOutputEncoding());
         assertTrue(commandLine.getRedirectStderrToStdoutInsteadOfDiscard());
         Predicate<Integer> defaultExitCodePredicate = commandLine.getSuccessfulExitCodePredicate();
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImplTest.java
index 5a32b4c68b1..88d26103777 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImplTest.java
@@ -1,15 +1,24 @@
 // Copyright 2018 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.task.util.process;
 
+import com.yahoo.vespa.hosted.node.admin.task.util.file.FileAttributes;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath;
 import com.yahoo.vespa.hosted.node.admin.task.util.time.TestTimer;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
 
+import java.io.IOException;
+import java.nio.file.FileSystems;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
 import java.util.Arrays;
+import java.util.Optional;
+import java.util.Set;
 
+import static com.yahoo.yolean.Exceptions.uncheck;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -45,4 +54,35 @@ public class ProcessFactoryImplTest {
 
         assertFalse(Files.exists(outputPath));
     }
+
+    @Test
+    public void testSpawnWithPersistentOutputFile() {
+
+        class TemporaryFile implements AutoCloseable {
+            Path path;
+            TemporaryFile() {
+                String outputFileName = ProcessFactoryImplTest.class.getSimpleName() + "-temporary-test-file.out";
+                FileAttribute<Set<PosixFilePermission>> fileAttribute = PosixFilePermissions.asFileAttribute(
+                        PosixFilePermissions.fromString("rw-------"));
+                path = uncheck(() -> Files.createTempFile(outputFileName, ".out", fileAttribute));
+            }
+            @Override public void close() { uncheck(() -> Files.deleteIfExists(path)); }
+        }
+
+        try (TemporaryFile outputPath = new TemporaryFile()) {
+            CommandLine commandLine = mock(CommandLine.class);
+            when(commandLine.getArguments()).thenReturn(Arrays.asList("program"));
+            when(commandLine.programName()).thenReturn("program");
+            when(commandLine.getOutputFile()).thenReturn(Optional.of(outputPath.path));
+            try (ChildProcess2Impl child = processFactory.spawn(commandLine)) {
+                assertEquals(outputPath.path, child.getOutputPath());
+                assertTrue(Files.exists(outputPath.path));
+                assertEquals("rw-------", new UnixPath(outputPath.path).getPermissions());
+            }
+
+            assertTrue(Files.exists(outputPath.path));
+        }
+
+    }
+
 }
\ No newline at end of file