From e9dba45b41b1ac920e3294ebfec22bbea0956235 Mon Sep 17 00:00:00 2001 From: Jared Rhizor Date: Fri, 21 Jan 2022 08:22:45 -0800 Subject: [PATCH] fix docker process factory test on ci (#9695) * fix docker process factory test on ci * fmt * fix compilation --- .../ContainerOrchestratorApp.java | 3 +- .../java/io/airbyte/workers/WorkerApp.java | 3 +- .../workers/process/DockerProcessFactory.java | 70 +++++++------------ .../process/DockerProcessFactoryTest.java | 25 +++---- 4 files changed, 37 insertions(+), 64 deletions(-) diff --git a/airbyte-container-orchestrator/src/main/java/io/airbyte/container_orchestrator/ContainerOrchestratorApp.java b/airbyte-container-orchestrator/src/main/java/io/airbyte/container_orchestrator/ContainerOrchestratorApp.java index 476f049d608f..62fb1711fd16 100644 --- a/airbyte-container-orchestrator/src/main/java/io/airbyte/container_orchestrator/ContainerOrchestratorApp.java +++ b/airbyte-container-orchestrator/src/main/java/io/airbyte/container_orchestrator/ContainerOrchestratorApp.java @@ -196,8 +196,7 @@ private static ProcessFactory getProcessBuilderFactory(final Configs configs, fi configs.getWorkspaceRoot(), configs.getWorkspaceDockerMount(), configs.getLocalDockerMount(), - configs.getDockerNetwork(), - false); + configs.getDockerNetwork()); } } diff --git a/airbyte-workers/src/main/java/io/airbyte/workers/WorkerApp.java b/airbyte-workers/src/main/java/io/airbyte/workers/WorkerApp.java index bc9e89b01e69..e27dd65e3130 100644 --- a/airbyte-workers/src/main/java/io/airbyte/workers/WorkerApp.java +++ b/airbyte-workers/src/main/java/io/airbyte/workers/WorkerApp.java @@ -269,8 +269,7 @@ private static ProcessFactory getJobProcessFactory(final Configs configs) throws configs.getWorkspaceRoot(), configs.getWorkspaceDockerMount(), configs.getLocalDockerMount(), - configs.getDockerNetwork(), - false); + configs.getDockerNetwork()); } } diff --git a/airbyte-workers/src/main/java/io/airbyte/workers/process/DockerProcessFactory.java b/airbyte-workers/src/main/java/io/airbyte/workers/process/DockerProcessFactory.java index 62ff643d00b8..6184690b5ad1 100644 --- a/airbyte-workers/src/main/java/io/airbyte/workers/process/DockerProcessFactory.java +++ b/airbyte-workers/src/main/java/io/airbyte/workers/process/DockerProcessFactory.java @@ -38,7 +38,6 @@ public class DockerProcessFactory implements ProcessFactory { private final Path workspaceRoot; private final String localMountSource; private final String networkName; - private final boolean isOrchestrator; private final Path imageExistsScriptPath; /** @@ -48,20 +47,17 @@ public class DockerProcessFactory implements ProcessFactory { * @param workspaceMountSource workspace volume * @param localMountSource local volume * @param networkName docker network - * @param isOrchestrator if the process needs to be able to launch containers */ public DockerProcessFactory(final WorkerConfigs workerConfigs, final Path workspaceRoot, final String workspaceMountSource, final String localMountSource, - final String networkName, - final boolean isOrchestrator) { + final String networkName) { this.workerConfigs = workerConfigs; this.workspaceRoot = workspaceRoot; this.workspaceMountSource = workspaceMountSource; this.localMountSource = localMountSource; this.networkName = networkName; - this.isOrchestrator = isOrchestrator; this.imageExistsScriptPath = prepareImageExistsScript(); } @@ -105,46 +101,30 @@ public Process create(final String jobId, IOs.writeFile(jobRoot, file.getKey(), file.getValue()); } - List cmd; - - // todo: add --expose 80 to each - - if (isOrchestrator) { - cmd = Lists.newArrayList( - "docker", - "run", - "--rm", - "--init", - "-i", - "-v", - String.format("%s:%s", workspaceMountSource, workspaceRoot), // real workspace root, not a rebased version - "-v", - String.format("%s:%s", localMountSource, LOCAL_MOUNT_DESTINATION), - "-v", - "/var/run/docker.sock:/var/run/docker.sock", // needs to be able to run docker in docker - "-w", - jobRoot.toString(), // real jobroot, not rebased version - "--network", - networkName, - "--log-driver", - "none"); - } else { - cmd = Lists.newArrayList( - "docker", - "run", - "--rm", - "--init", - "-i", - "-v", - String.format("%s:%s", workspaceMountSource, DATA_MOUNT_DESTINATION), // uses job data mount - "-v", - String.format("%s:%s", localMountSource, LOCAL_MOUNT_DESTINATION), - "-w", - rebasePath(jobRoot).toString(), // rebases the job root on the job data mount - "--network", - networkName, - "--log-driver", - "none"); + final List cmd = Lists.newArrayList( + "docker", + "run", + "--rm", + "--init", + "-i", + "-w", + rebasePath(jobRoot).toString(), // rebases the job root on the job data mount + "--log-driver", + "none"); + + if (networkName != null) { + cmd.add("--network"); + cmd.add(networkName); + } + + if (workspaceMountSource != null) { + cmd.add("-v"); + cmd.add(String.format("%s:%s", workspaceMountSource, DATA_MOUNT_DESTINATION)); + } + + if (localMountSource != null) { + cmd.add("-v"); + cmd.add(String.format("%s:%s", localMountSource, LOCAL_MOUNT_DESTINATION)); } for (final var envEntry : workerConfigs.getEnvMap().entrySet()) { diff --git a/airbyte-workers/src/test/java/io/airbyte/workers/process/DockerProcessFactoryTest.java b/airbyte-workers/src/test/java/io/airbyte/workers/process/DockerProcessFactoryTest.java index 53d33a21955b..4b9d9d51e206 100644 --- a/airbyte-workers/src/test/java/io/airbyte/workers/process/DockerProcessFactoryTest.java +++ b/airbyte-workers/src/test/java/io/airbyte/workers/process/DockerProcessFactoryTest.java @@ -24,8 +24,6 @@ import java.util.Map; import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; // todo (cgardens) - these are not truly "unit" tests as they are check resources on the internet. // we should move them to "integration" tests, when we have facility to do so. @@ -61,7 +59,7 @@ public void testJqExists() throws IOException { public void testImageExists() throws IOException, WorkerException { final Path workspaceRoot = Files.createTempDirectory(Files.createDirectories(TEST_ROOT), "process_factory"); - final DockerProcessFactory processFactory = new DockerProcessFactory(new WorkerConfigs(new EnvConfigs()), workspaceRoot, "", "", "", false); + final DockerProcessFactory processFactory = new DockerProcessFactory(new WorkerConfigs(new EnvConfigs()), workspaceRoot, null, null, null); assertTrue(processFactory.checkImageExists("busybox")); } @@ -69,18 +67,17 @@ public void testImageExists() throws IOException, WorkerException { public void testImageDoesNotExist() throws IOException, WorkerException { final Path workspaceRoot = Files.createTempDirectory(Files.createDirectories(TEST_ROOT), "process_factory"); - final DockerProcessFactory processFactory = new DockerProcessFactory(new WorkerConfigs(new EnvConfigs()), workspaceRoot, "", "", "", false); + final DockerProcessFactory processFactory = new DockerProcessFactory(new WorkerConfigs(new EnvConfigs()), workspaceRoot, null, null, null); assertFalse(processFactory.checkImageExists("airbyte/fake:0.1.2")); } - @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void testFileWriting(boolean isOrchestrator) throws IOException, WorkerException { + @Test + public void testFileWriting() throws IOException, WorkerException { final Path workspaceRoot = Files.createTempDirectory(Files.createDirectories(TEST_ROOT), "process_factory"); final Path jobRoot = workspaceRoot.resolve("job"); final DockerProcessFactory processFactory = - new DockerProcessFactory(new WorkerConfigs(new EnvConfigs()), workspaceRoot, "", "", "", isOrchestrator); + new DockerProcessFactory(new WorkerConfigs(new EnvConfigs()), workspaceRoot, null, null, null); processFactory.create("job_id", 0, jobRoot, "busybox", false, ImmutableMap.of("config.json", "{\"data\": 2}"), "echo hi", new WorkerConfigs(new EnvConfigs()).getResourceRequirements(), Map.of(), Map.of()); @@ -92,9 +89,8 @@ public void testFileWriting(boolean isOrchestrator) throws IOException, WorkerEx /** * Tests that the env var map passed in is accessible within the process. */ - @ParameterizedTest - @ValueSource(booleans = {true, false}) - public void testEnvMapSet(boolean isOrchestrator) throws IOException, WorkerException { + @Test + public void testEnvMapSet() throws IOException, WorkerException { final Path workspaceRoot = Files.createTempDirectory(Files.createDirectories(TEST_ROOT), "process_factory"); final Path jobRoot = workspaceRoot.resolve("job"); @@ -105,10 +101,9 @@ public void testEnvMapSet(boolean isOrchestrator) throws IOException, WorkerExce new DockerProcessFactory( workerConfigs, workspaceRoot, - "", - "", - "host", - isOrchestrator); + null, + null, + "host"); final Process process = processFactory.create( "job_id",