[beam] branch master updated: [BEAM-9645] Fix premature removal of Docker container and logs

[ib...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

ibzib pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/beam.git


The following commit(s) were added to refs/heads/master by this push:
     new c7cfedb  [BEAM-9645] Fix premature removal of Docker container and logs
     new 2c3eb1b  Merge pull request #11303 from mxm/BEAM-9645
c7cfedb is described below

commit c7cfedbd2db41c7ccdf9f95be1e50956a202bf58
Author: Maximilian Michels <mx...@apache.org>
AuthorDate: Fri Apr 3 11:17:01 2020 +0200

    [BEAM-9645] Fix premature removal of Docker container and logs
    
    The default would lead to removing the Docker container on container exit which
    would prevent log retrieval. This was a regression of fbc84b61240a3d83d9c19f7ccc17ba22e5d7e2c9.
---
 .../environment/DockerEnvironmentFactory.java            |  5 +----
 .../environment/DockerEnvironmentFactoryTest.java        | 16 ++++++++++++++--
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/environment/DockerEnvironmentFactory.java b/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/environment/DockerEnvironmentFactory.java
index 39bc8cd..210269b 100644
--- a/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/environment/DockerEnvironmentFactory.java
+++ b/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/environment/DockerEnvironmentFactory.java
@@ -130,11 +130,8 @@ public class DockerEnvironmentFactory implements EnvironmentFactory {
             // host networking on Mac)
             .add("--env=DOCKER_MAC_CONTAINER=" + System.getenv("DOCKER_MAC_CONTAINER"));
 
-    Boolean retainDockerContainer =
+    final boolean retainDockerContainer =
         pipelineOptions.as(ManualDockerEnvironmentOptions.class).getRetainDockerContainers();
-    if (!retainDockerContainer) {
-      dockerOptsBuilder.add("--rm");
-    }
 
     String semiPersistDir = pipelineOptions.as(RemoteEnvironmentOptions.class).getSemiPersistDir();
     ImmutableList.Builder<String> argsBuilder =
diff --git a/runners/java-fn-execution/src/test/java/org/apache/beam/runners/fnexecution/environment/DockerEnvironmentFactoryTest.java b/runners/java-fn-execution/src/test/java/org/apache/beam/runners/fnexecution/environment/DockerEnvironmentFactoryTest.java
index afaada0..3f479a5 100644
--- a/runners/java-fn-execution/src/test/java/org/apache/beam/runners/fnexecution/environment/DockerEnvironmentFactoryTest.java
+++ b/runners/java-fn-execution/src/test/java/org/apache/beam/runners/fnexecution/environment/DockerEnvironmentFactoryTest.java
@@ -17,16 +17,20 @@
  */
 package org.apache.beam.runners.fnexecution.environment;
 
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.is;
-import static org.junit.Assert.assertThat;
-import static org.mockito.Matchers.any;
+import static org.hamcrest.Matchers.not;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyList;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.List;
 import org.apache.beam.model.pipeline.v1.Endpoints.ApiServiceDescriptor;
 import org.apache.beam.model.pipeline.v1.RunnerApi.Environment;
 import org.apache.beam.runners.core.construction.Environments;
@@ -49,6 +53,7 @@ import org.junit.rules.ExpectedException;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 import org.junit.runners.Parameterized.Parameter;
+import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
@@ -135,6 +140,13 @@ public class DockerEnvironmentFactoryTest {
       }
 
       RemoteEnvironment handle = factory.createEnvironment(ENVIRONMENT);
+
+      ArgumentCaptor<List<String>> dockerArgsCaptor = ArgumentCaptor.forClass(List.class);
+      verify(docker).runImage(any(), dockerArgsCaptor.capture(), anyList());
+
+      // Ensure we do not remove the container prematurely which would also remove the logs
+      assertThat(dockerArgsCaptor.getValue(), not(hasItem("--rm")));
+
       handle.close();
 
       verify(docker).killContainer(CONTAINER_ID);