Fix review defects

engine/README.md

@@ -25,7 +25,7 @@ For using Docker execution environments:
     -e DOCKER_IMAGE_DEFAULT=odysseusinc/r-hades:2023q3v3             // Default image to use for running executions 
     -e ANALYSIS_MOUNT=/etc/ee                                        // Provide container location of the host directory for executions to allow mounting it spawn Docker containers
     -e DOCKER_REGISTRY_URL=...                                       // (Optional) url to Docker registry for pulling image runtime files
-    -e DOCKER_REGISTRY_USERNAME=...                                  // (Optional) username to connect to Docker registry
+    -e **DOCKER**_REGISTRY_USERNAME=...                                  // (Optional) username to connect to Docker registry
     -e DOCKER_REGISTRY_PASSWORD=...                                  // (Optional) password to connect to Docker registry
 
 ## Build with Impala JDBC driver

engine/pom.xml

@@ -195,19 +195,6 @@
             <version>${docker-java.version}</version>
         </dependency>
 
-        <!-- Docker -->
-        <dependency>
-            <groupId>com.github.docker-java</groupId>
-            <artifactId>docker-java</artifactId>
-            <version>${docker-java.version}</version>
-        </dependency>
-
-        <dependency>
-            <groupId>com.github.docker-java</groupId>
-            <artifactId>docker-java-transport-httpclient5</artifactId>
-            <version>${docker-java.version}</version>
-        </dependency>
-
     </dependencies>
 
     <profiles>

engine/src/main/java/com/odysseusinc/arachne/executionengine/config/properties/DockerRegistryProperties.java

@@ -4,7 +4,6 @@
 import lombok.Setter;
 import org.springframework.boot.context.properties.ConfigurationProperties;
 import org.springframework.context.annotation.Configuration;
-import org.springframework.stereotype.Component;
 
 @Setter
 @Getter

engine/src/main/java/com/odysseusinc/arachne/executionengine/execution/AbstractOverseer.java

@@ -7,6 +7,9 @@
 import lombok.Getter;
 
 public abstract class AbstractOverseer implements Overseer {
+    /**
+     * Execution id for logging.
+     */
     protected final long id;
     protected final StringBuffer stdout;
     protected final BiConsumer<String, String> callback;
@@ -18,19 +21,22 @@ public abstract class AbstractOverseer implements Overseer {
     protected final Instant started;
     @Getter
     protected final String environment;
-    protected final int killTimeout;
+    /**
+     * Timeout to wait for kill command to complete, in seconds
+     */
+    protected final int killTimeoutSec;
     /**
      * Execution result after applying post-processing
      */
     @Getter
     protected volatile CompletableFuture<ExecutionOutcome> result;
 
-    public AbstractOverseer(long id, BiConsumer<String, String> callback, Instant started, String environment, int killTimeout, StringBuffer stdout, CompletableFuture<ExecutionOutcome> outcome) {
+    public AbstractOverseer(long id, BiConsumer<String, String> callback, Instant started, String environment, int killTimeoutSec, StringBuffer stdout, CompletableFuture<ExecutionOutcome> outcome) {
         this.id = id;
         this.callback = callback;
         this.started = started;
         this.environment = environment;
-        this.killTimeout = killTimeout;
+        this.killTimeoutSec = killTimeoutSec;
         result = this.outcome = outcome;
         this.stdout = stdout;
     }

engine/src/main/java/com/odysseusinc/arachne/executionengine/execution/AnalysisService.java

@@ -38,7 +38,9 @@
 import com.odysseusinc.arachne.executionengine.aspect.FileDescriptorCount;
 import com.odysseusinc.arachne.executionengine.service.CdmMetadataService;
 import com.odysseusinc.arachne.executionengine.util.AutoCloseWrapper;
+import java.io.File;
 import java.util.Date;
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
@@ -49,6 +51,8 @@
 import java.util.function.BiConsumer;
 import java.util.function.Consumer;
 import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import lombok.extern.slf4j.Slf4j;
 import net.lingala.zip4j.exception.ZipException;
 import org.apache.commons.lang3.StringUtils;
@@ -60,11 +64,6 @@
 import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor;
 import org.springframework.stereotype.Service;
 
-import java.io.File;
-import java.util.List;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
-
 @Slf4j
 @Service
 public class AnalysisService {
@@ -137,10 +136,10 @@ private ExecutionOutcome abort(Long id, Overseer overseer) {
             return new ExecutionOutcome(Stage.ABORT, "Waiting for abort interrupted", overseer.getStdout());
         } catch (ExecutionException e) {
             log.info("Execution [{}] abort attempt failed", id, e);
-            return new ExecutionOutcome(Stage.ABORT, e.getMessage(), overseer.getStdout() + "\n" + getStackTrace(e.getCause()));
+            return new ExecutionOutcome(Stage.ABORT, e.getMessage(), overseer.getStdout() + "\r\n" + getStackTrace(e.getCause()));
         } catch (TimeoutException e) {
             log.info("Execution [{}] waiting for abort timed out", id);
-            return new ExecutionOutcome(Stage.ABORT, e.getMessage(), overseer.getStdout() + "\n" + getStackTrace(e.getCause()));
+            return new ExecutionOutcome(Stage.ABORT, e.getMessage(), overseer.getStdout() + "\r\n" + getStackTrace(e.getCause()));
         }
     }
 

engine/src/main/java/com/odysseusinc/arachne/executionengine/execution/r/DockerOverseer.java

@@ -31,21 +31,24 @@ public class DockerOverseer extends AbstractOverseer {
     }};
     private final CompletableFuture<String> init;
 
+    /**
+     * Last position in stdout that was submitted to callback.
+     */
     private volatile int pos;
     private final DockerClient client;
 
     public DockerOverseer(
-            long id, DockerClient client, Instant started, int timeout, StringBuffer stdout, CompletableFuture<String> init,
-            int updateInterval, BiConsumer<String, String> callback, String image, int killTimeout
+            long id, DockerClient client, Instant started, int timeoutSec, StringBuffer stdout, CompletableFuture<String> init,
+            int updateInterval, BiConsumer<String, String> callback, String image, int killTimeoutSec
     ) {
-        super(id, callback, started, image, killTimeout, new StringBuffer(), init.handle((containerId, throwable) -> {
+        super(id, callback, started, image, killTimeoutSec, stdout, init.handle((containerId, throwable) -> {
             if (throwable != null) {
-                String out = stdout.append("\n").append(ExceptionUtils.getStackTrace(throwable)).toString();
+                String out = stdout.append("\r\n").append(ExceptionUtils.getStackTrace(throwable)).toString();
                 return new ExecutionOutcome(Stage.INITIALIZE, throwable.getMessage(), out);
             } else {
                 LogContainerCmd cmd = client.logContainerCmd(containerId).withStdOut(true).withStdErr(true).withFollowStream(true);
                 cmd.exec(logAdapter(id, stdout));
-                Integer exitCode = client.waitContainerCmd(containerId).exec(new WaitContainerResultCallback()).awaitStatusCode(timeout, TimeUnit.SECONDS);
+                Integer exitCode = client.waitContainerCmd(containerId).exec(new WaitContainerResultCallback()).awaitStatusCode(timeoutSec, TimeUnit.SECONDS);
                 log.info("Execution [{}] Rscript exit code {}", id, exitCode);
                 String out = stdout.toString();
                 return (exitCode == 0)
@@ -54,7 +57,6 @@ public DockerOverseer(
             }
         }));
         pos = stdout.length();
-        this.stdout.append(stdout);
         this.client = client;
         init.thenAccept(containerId ->
                 executor.scheduleWithFixedDelay(this::writeLogs, updateInterval, updateInterval, TimeUnit.MILLISECONDS)

engine/src/main/java/com/odysseusinc/arachne/executionengine/execution/r/DockerService.java

@@ -48,6 +48,9 @@ public class DockerService extends RService implements AutoCloseable {
     @Value("${docker.host:#{null}}")
     private String host;
 
+    @Value("docker.certPath:#{null}")
+    private String certPath;
+
     @Value("${docker.image.default}")
     private String defaultImage;
 
@@ -57,10 +60,11 @@ public class DockerService extends RService implements AutoCloseable {
 
     public DockerService(DockerRegistryProperties properties) {
         DefaultDockerClientConfig.Builder builder = DefaultDockerClientConfig.createDefaultConfigBuilder()
-                .withDockerTlsVerify(false)
+                .withDockerTlsVerify(certPath != null)
+                .withDockerCertPath(certPath)
                 .withRegistryUrl(properties.getUrl())
-                .withRegistryPassword(properties.getPasword())
-                .withRegistryUsername(properties.getUsername());
+                .withRegistryUsername(properties.getUsername())
+                .withRegistryPassword(properties.getPasword());
         Optional.ofNullable(host).ifPresent(builder::withDockerHost);
         DefaultDockerClientConfig config = builder.build();
 
@@ -71,7 +75,7 @@ public DockerService(DockerRegistryProperties properties) {
                 .maxConnections(50)
                 .build();
         client = DockerClientImpl.getInstance(config, dockerHttpClient);
-        log.info("Initialized docker interface [{}]", host);
+        log.info("Initialized Docker interface [{}]", host);
     }
 
     @Override
@@ -94,13 +98,13 @@ protected Overseer analyze(
 
         CompletableFuture<String> init = CompletableFuture.supplyAsync(
                 () -> {
-                    log.info("Execution [{}] use docker image [{}]", id, image);
+                    log.info("Execution [{}] use Docker image [{}]", id, image);
                     pullImage(callback, id, image, stdout, client);
-                    callback.accept(Stage.INITIALIZE, "Pull complete, creating container\n");
+                    callback.accept(Stage.INITIALIZE, "Pull complete, creating container\r\n");
                     log.info("Execution [{}] creating container with image [{}]", id, image);
                     String containerId = createContainer(analysisDir, env, image, script);
                     log.info("Execution [{}] created container [{}]", id, containerId);
-                    callback.accept(Stage.INITIALIZE, "Container [" + containerId + "] created with [" + analysisDir.getPath() + "] mounted for execution\n");
+                    callback.accept(Stage.INITIALIZE, "Container [" + containerId + "] created with [" + analysisDir.getPath() + "] mounted for execution\r\n");
                     client.startContainerCmd(containerId).exec();
                     return containerId;
                 },
@@ -112,7 +116,7 @@ protected Overseer analyze(
 
     private void pullImage(BiConsumer<String, String> callback, Long id, String image, StringBuffer stdout, DockerClient client) {
         try {
-            callback.accept(Stage.INITIALIZE, "Pulling image [" + image + "]\n");
+            callback.accept(Stage.INITIALIZE, "Pulling image [" + image + "]\r\n");
             client.pullImageCmd(image).exec(new PullImageResultCallback() {
                 @Override
                 public void onNext(PullResponseItem item) {

engine/src/main/java/com/odysseusinc/arachne/executionengine/execution/r/RService.java

@@ -9,7 +9,6 @@
 import com.odysseusinc.arachne.executionengine.execution.ExecutionService;
 import com.odysseusinc.arachne.executionengine.execution.KerberosSupport;
 import com.odysseusinc.arachne.executionengine.execution.Overseer;
-import com.odysseusinc.arachne.executionengine.service.DescriptorService;
 import com.odysseusinc.datasourcemanager.krblogin.KrbConfig;
 import java.io.File;
 import java.util.HashMap;

engine/src/main/java/com/odysseusinc/arachne/executionengine/execution/r/TarballROverseer.java

@@ -134,7 +134,7 @@ private boolean waitForKill() {
             // Will not kill whole process tree on windows. The fundamental problem here is that (unlike Unix)
             // Windows doesn't maintain parent-child relationships between processes. A process can kill
             // its own immediate children, but not 'grand-children' because it has no way of finding them.
-            return process.destroyForcibly().waitFor(killTimeout, TimeUnit.SECONDS);
+            return process.destroyForcibly().waitFor(killTimeoutSec, TimeUnit.SECONDS);
         } catch (InterruptedException e) {
             log.info("Overseer [{}] interrupted waiting for process termination", id);
             return false;