Cleanup TaskContext

 - Removes unnecessary isRunningLocally setting
 - Reduce TaskContext to (basically) recordSystemModification, e.g. removing
   FileSystem as that can be accessed through other means.
 - Remove DockerAdminConfig as it's still unused.
 - Remove unnecessary fields in NodeAdminConfig.
 - Move json configs to /etc/vespa.

16 files changed, 63 insertions, 93 deletions

diff --git a/node-admin/src/main/application/services.xml b/node-admin/src/main/application/services.xml
index da782363508..99733552f76 100644
--- a/node-admin/src/main/application/services.xml
+++ b/node-admin/src/main/application/services.xml
@@ -11,10 +11,6 @@
     <component id="docker-api" class="com.yahoo.vespa.hosted.dockerapi.DockerImpl" bundle="docker-api"/>
     <component id="metrics-wrapper" class="com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper" bundle="docker-api"/>
 
-    <config name="vespa.hosted.dockerapi.docker">
-      <isRunningLocally>false</isRunningLocally>
-    </config>
-
     <nodes type="host"/>
 
     <preprocess:include file="variant.xml" required="false"/>
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/component/TaskContext.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/component/TaskContext.java
index cee2dc9b66b..2d94db3d96e 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/component/TaskContext.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/component/TaskContext.java
@@ -1,26 +1,29 @@
 // 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.component;
 
-import java.nio.file.FileSystem;
-import java.util.EnumSet;
 import java.util.logging.Logger;
 
 public interface TaskContext {
-    enum Cloud { YAHOO, AWS }
-    Cloud cloud();
+    /**
+     * Record a system modification. IdempotentTask is supposed to converge the system (files,
+     * directory permission, iptable rules, etc) to some wanted state. It is especially important
+     * to produce a truthful log of system changes to understand what may or may not be going on.
+     *
+     * All tasks should:
+     *  1. Record any and all modifications to the system
+     *  2. Avoid recording system interactions that does not actually change the system.
+     *  3. Record system modifications as early as possible and preferably before they are
+     *     performed (sometimes this is not possible).
+     *
+     * @param logger Used to log the modification to help locate the source of the modification.
+     * @param description Description of the modification, e.g. "Changing owner of /foo from alice
+     *                    to bob".
+     */
+    void recordSystemModification(Logger logger, String description);
 
-    enum Role { TENANT_DOCKER_HOST, CONFIG_SERVER_DOCKER_HOST }
-    EnumSet<Role> roles();
-    default boolean hasRole(Role role) {
-        return roles().contains(role);
-    }
-
-    FileSystem fileSystem();
-
-    void logSystemModification(Logger logger, String actionDescription);
-    default void logSystemModification(Logger logger, String format, String... args) {
-        logSystemModification(logger, String.format(format, (Object[]) args));
-    }
-
-    default boolean executeSubtask(IdempotentTask task) { return false; }
+    /**
+     * Execute a task as a child of this task, and with its own sub-TaskContext. Please  avoid
+     * excessive task hierarchies.
+     */
+    boolean executeSubtask(IdempotentTask task);
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/DockerAdminConfig.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/DockerAdminConfig.java
deleted file mode 100644
index d1aaefab3dd..00000000000
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/DockerAdminConfig.java
+++ /dev/null
@@ -1,9 +0,0 @@
-// 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.nodeadmin;
-
-import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
-
-@JsonIgnoreProperties(ignoreUnknown = true)
-public class DockerAdminConfig {
-}
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminConfig.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminConfig.java
index df26bc2dee5..c3a1f47dfc5 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminConfig.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminConfig.java
@@ -18,18 +18,6 @@ public class NodeAdminConfig {
     @JsonProperty("main-component")
     public String mainComponent = null;
 
-    public enum Mode {
-        aws_tenant,
-        config_server_host,
-        tenant,
-    }
-
-    @JsonProperty("mode")
-    public Mode mode = Mode.tenant;
-
-    @JsonProperty("docker")
-    public DockerAdminConfig docker = new DockerAdminConfig();
-
     public static NodeAdminConfig fromFile(File file) {
         if (!file.exists()) {
             return new NodeAdminConfig();
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminMain.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminMain.java
index f412ec5ef2f..b0631fc712b 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminMain.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminMain.java
@@ -5,11 +5,10 @@ import com.yahoo.component.ComponentId;
 import com.yahoo.component.provider.ComponentRegistry;
 import com.yahoo.concurrent.classlock.ClassLocking;
 import com.yahoo.log.LogLevel;
-import com.yahoo.vespa.defaults.Defaults;
 import com.yahoo.vespa.hosted.dockerapi.Docker;
 import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper;
-import com.yahoo.vespa.hosted.node.admin.config.ConfigServerConfig;
 import com.yahoo.vespa.hosted.node.admin.component.AdminComponent;
+import com.yahoo.vespa.hosted.node.admin.config.ConfigServerConfig;
 import com.yahoo.vespa.hosted.node.admin.provider.NodeAdminStateUpdater;
 
 import java.io.File;
@@ -49,8 +48,7 @@ public class NodeAdminMain implements AutoCloseable {
     }
 
     public static NodeAdminConfig getConfig() {
-        String path = Defaults.getDefaults().underVespaHome("conf/node-admin.json");
-        return NodeAdminConfig.fromFile(new File(path));
+        return NodeAdminConfig.fromFile(new File("/etc/vespa/node-admin.json"));
     }
 
     public void start() {
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/AttributeSync.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/AttributeSync.java
index a20d30b2bf9..cba3912ce49 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/AttributeSync.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/AttributeSync.java
@@ -111,13 +111,13 @@ public class AttributeSync {
             return false;
         }
 
-        context.logSystemModification(
+        context.recordSystemModification(
                 logger,
-                "Changing %s of %s from %s to %s",
-                attributeName,
-                path.toString(),
-                currentValue,
-                wantedValue.get());
+                String.format("Changing %s of %s from %s to %s",
+                        attributeName,
+                        path.toString(),
+                        currentValue,
+                        wantedValue.get()));
 
         valueSetter.accept(wantedValue.get());
 
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileSync.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileSync.java
index 18d2a2e3aa9..7f11707c1cc 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileSync.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileSync.java
@@ -53,7 +53,7 @@ public class FileSync {
         }
 
         if (!currentAttributes.exists()) {
-            taskContext.logSystemModification(logger, "Creating file " + path);
+            taskContext.recordSystemModification(logger, "Creating file " + path);
             path.createParents();
             path.writeUtf8File(content.get());
             contentCache.updateWith(content.get(), currentAttributes.forceGet().lastModifiedTime());
@@ -63,7 +63,7 @@ public class FileSync {
         if (Objects.equals(content.get(), contentCache.get(currentAttributes.get().lastModifiedTime()))) {
             return false;
         } else {
-            taskContext.logSystemModification(logger, "Patching file " + path);
+            taskContext.recordSystemModification(logger, "Patching file " + path);
             path.writeUtf8File(content.get());
             contentCache.updateWith(content.get(), currentAttributes.forceGet().lastModifiedTime());
             return true;
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectory.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectory.java
index e815ab8bd86..2a88387f8fc 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectory.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/MakeDirectory.java
@@ -54,7 +54,7 @@ public class MakeDirectory {
                 path.createParents();
             }
 
-            context.logSystemModification(logger, "Creating directory " + path);
+            context.recordSystemModification(logger, "Creating directory " + path);
             systemModified = true;
 
             Optional<String> permissions = attributeSync.getPermissions();
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess.java
index 71a4c7c109b..dc2ab07d97c 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess.java
@@ -18,7 +18,7 @@ public interface ChildProcess extends AutoCloseable {
      * Only call this if process was spawned under the assumption the program had no side
      * effects (see Command::spawnProgramWithoutSideEffects).  If it is determined later
      * that the program did in fact have side effects (modified system), this method can
-     * be used to log that fact. Alternatively, call TaskContext::logSystemModification
+     * be used to log that fact. Alternatively, call TaskContext::recordSystemModification
      * directly.
      */
     void logAsModifyingSystemAfterAll(Logger logger);
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessImpl.java
index 98fa18db52d..cabeafd2727 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcessImpl.java
@@ -73,7 +73,7 @@ public class ChildProcessImpl implements ChildProcess {
 
     @Override
     public void logAsModifyingSystemAfterAll(Logger logger) {
-        taskContext.logSystemModification(logger, "Executed command: " + commandLine);
+        taskContext.recordSystemModification(logger, "Executed command: " + commandLine);
     }
 
     @Override
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/Command.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/Command.java
index 275440f7840..97066748b2b 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/Command.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/Command.java
@@ -42,7 +42,7 @@ public class Command {
 
         String commandLine = commandLine();
         if (commandLogger != null) {
-            context.logSystemModification(commandLogger, "Executing command: " + commandLine);
+            context.recordSystemModification(commandLogger, "Executing command: " + commandLine);
         }
 
         // Why isn't this using TaskContext.fileSystem? Because createTempFile assumes
@@ -94,7 +94,7 @@ public class Command {
      *
      * This method can also be used to spawn a process that MAY have side effects
      * to be determined at some later time. The caller is then responsible for calling
-     * TaskContext::logSystemModification afterwards. The caller is encouraged to
+     * TaskContext::recordSystemModification afterwards. The caller is encouraged to
      * call ChildProcess::logAsModifyingSystemAfterAll to do this.
      */
     public ChildProcess spawnProgramWithoutSideEffects() {
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/AddYumRepo.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/AddYumRepo.java
index 2b1cbbed974..9485e5d6728 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/AddYumRepo.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/AddYumRepo.java
@@ -4,6 +4,8 @@ package com.yahoo.vespa.hosted.node.admin.task.util.yum;
 import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.FileWriter;
 
+import java.nio.file.FileSystem;
+import java.nio.file.FileSystems;
 import java.nio.file.Path;
 import java.util.regex.Pattern;
 
@@ -17,20 +19,17 @@ public class AddYumRepo {
     private final String name; // e.g. "Platform RPM Latest Repo"
     private final String baseurl;
     private final boolean enabled;
+    private final FileSystem fileSystem;
 
     public AddYumRepo(String repositoryId,
                       String name,
                       String baseurl,
                       boolean enabled) {
-        this.repositoryId = repositoryId;
-        this.name = name;
-        this.baseurl = baseurl;
-        this.enabled = enabled;
-        validateRepositoryId(repositoryId);
+        this(repositoryId, name, baseurl, enabled, FileSystems.getDefault());
     }
 
     public boolean converge(TaskContext context) {
-        Path path = context.fileSystem().getPath("/etc/yum.repos.d",repositoryId + ".repo");
+        Path path = fileSystem.getPath("/etc/yum.repos.d",repositoryId + ".repo");
 
         FileWriter fileWriter = new FileWriter(path, this::getRepoFileContent)
                 .withOwner("root")
@@ -59,4 +58,18 @@ public class AddYumRepo {
             throw new IllegalArgumentException("Invalid repository ID '" + repositoryId + "'");
         }
     }
+
+    // For testing
+    AddYumRepo(String repositoryId,
+               String name,
+               String baseurl,
+               boolean enabled,
+               FileSystem fileSystem) {
+        this.repositoryId = repositoryId;
+        this.name = name;
+        this.baseurl = baseurl;
+        this.enabled = enabled;
+        this.fileSystem = fileSystem;
+        validateRepositoryId(repositoryId);
+    }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileWriterTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileWriterTest.java
index bb8ca2586c8..1e4c7482aaf 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileWriterTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileWriterTest.java
@@ -37,7 +37,7 @@ public class FileWriterTest {
                 .onlyIfFileDoesNotAlreadyExist();
         TaskContext context = mock(TaskContext.class);
         assertTrue(writer.converge(context));
-        verify(context, times(1)).logSystemModification(any(), eq("Creating file " + path));
+        verify(context, times(1)).recordSystemModification(any(), eq("Creating file " + path));
 
         UnixPath unixPath = new UnixPath(path);
         assertEquals(content, unixPath.readUtf8File());
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/TestTaskContext.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/TestTaskContext.java
index 757f3004683..1fe63e84605 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/TestTaskContext.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/TestTaskContext.java
@@ -5,9 +5,7 @@ package com.yahoo.vespa.hosted.node.admin.task.util.file;
 import com.yahoo.vespa.hosted.node.admin.component.IdempotentTask;
 import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
 
-import java.nio.file.FileSystem;
 import java.util.ArrayList;
-import java.util.EnumSet;
 import java.util.List;
 import java.util.logging.Logger;
 
@@ -15,23 +13,8 @@ public class TestTaskContext implements TaskContext {
     private final List<String> logs = new ArrayList<>();
 
     @Override
-    public Cloud cloud() {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public EnumSet<Role> roles() {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public FileSystem fileSystem() {
-        throw new UnsupportedOperationException();
-    }
-
-    @Override
-    public void logSystemModification(Logger logger, String actionDescription) {
-        logs.add(actionDescription);
+    public void recordSystemModification(Logger logger, String description) {
+        logs.add(description);
     }
 
     public List<String> getSystemModificationLog() {
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandTest.java
index acc27353077..6dad95aeb6a 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandTest.java
@@ -27,7 +27,7 @@ public class CommandTest {
         Path outputFile;
         // Assumes /bin/bash exists on all hosts running this test.
         try (ChildProcess childProcess = command.spawn(logger)) {
-            verify(taskContext).logSystemModification(eq(logger), any());
+            verify(taskContext).recordSystemModification(eq(logger), any());
 
             outputFile = childProcess.getProcessOutputPath();
             int exitValue = childProcess.waitForTermination().exitValue();
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/AddYumRepoTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/AddYumRepoTest.java
index ad1fefe782f..399759c7c0a 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/AddYumRepoTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/yum/AddYumRepoTest.java
@@ -3,8 +3,8 @@
 package com.yahoo.vespa.hosted.node.admin.task.util.yum;
 
 import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
-import com.yahoo.vespa.test.file.TestFileSystem;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath;
+import com.yahoo.vespa.test.file.TestFileSystem;
 import org.junit.Test;
 
 import java.nio.file.FileSystem;
@@ -14,7 +14,6 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
 
 public class AddYumRepoTest {
     @Test
@@ -24,17 +23,16 @@ public class AddYumRepoTest {
         String baseurl = "http://foo.com/bar";
         boolean enabled = true;
 
+        FileSystem fileSystem = TestFileSystem.create();
         AddYumRepo addYumRepo = new AddYumRepo(
                 repositoryId,
                 name,
                 baseurl,
-                enabled);
+                enabled,
+                fileSystem);
 
         TaskContext context = mock(TaskContext.class);
 
-        FileSystem fileSystem = TestFileSystem.create();
-        when(context.fileSystem()).thenReturn(fileSystem);
-
         assertTrue(addYumRepo.converge(context));
 
         UnixPath unixPath = new UnixPath(fileSystem.getPath("/etc/yum.repos.d/" + repositoryId + ".repo"));