[jira] [Commented] (YARN-5366) Add support for toggling the removal of completed and failed docker containers

["Varun Vasudev (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/YARN-5366?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15455930#comment-15455930 ] 

Varun Vasudev commented on YARN-5366:
-------------------------------------

Thanks for the patch [~shanekumpf@gmail.com]!

1)
{code}
+      // Validate the configured value
+      if (!keepContainer.equalsIgnoreCase("true") &&
+          !keepContainer.equalsIgnoreCase("false")) {
+        throw new IllegalArgumentException("Only true and false are valid for"
+            + " YARN_CONTAINER_RUNTIME_DOCKER_KEEP_CONTAINER_ON_EXIT");
+      }
{code}

We should validate the value on submission - not when we signal the container

2)
We should add a configuration option to let admins allow/disallow this behavior - users shouldn’t be allowed to keep containers around because they feel like it

3)
{code}
+        LOG.debug("Docker container is not being removed due to user request. "
+            + "ContainerId: " + containerId);
{code}

I think for now this should be logged at info level. What do you think?

4)
{code}
+      String msg =
+          "Liveliness check failed for PID: " + ctx.getExecutionAttribute(PID)
+              + ". Container may have already completed.";
+      LOG.warn(msg);
{code}
I think this ends up double logging the message because the some one in the caller chain also logs the same message?

5)
{code}
+  public enum StatusState {
{code}

Rename to either DockerContainerStatus or DockerContainerState

6)
{code}
+      if (currentContainerStatus == null) {
+        return StatusState.UNKNOWN;
+      } else if (currentContainerStatus.equals(StatusState.RUNNING.getName())) {
+        return StatusState.RUNNING;
+      } else if (currentContainerStatus.equals(StatusState.STOPPED.getName())) {
+        return StatusState.STOPPED;
+      } else if (currentContainerStatus.equals(StatusState.EXITED.getName())) {
+        return StatusState.EXITED;
+      } else {
+        return StatusState.UNKNOWN;
+      }
{code}

Minor nit - but maybe change to switch/case?

7)
{code}
+    // Allow for injecting the container's status for testing.
+    if (statusState != null) {
+      status = statusState.getName();
+    }
{code}

Remove this. What you can do in your testing use cases is to create a class that inherits from DockerContainerStatusHandler and returns the status that you’ve set, but the the code snippet above shouldn’t be in DockerContainerStatusHandler

8)
Does MockContainerExecutorBinary need to be it’s own class - the pattern is used in other places so we should either get everyone else to use this class or move it into TestDockerContainerRuntime.

9)
{code}
+    File f = new File("./src/test/resources/mock-container-executor");
{code}

The path of the file looks incorrect. It really should be in the target directory. You should create a directory in the target directory and create the file in that directory. It also looks like you create the mock executor but don’t clean it up?

10)
Rename PrivilegedOperationCaptor to MockPrivilegedOperationCaptor? I would like the name to reflect that it only works in the testing case.

> Add support for toggling the removal of completed and failed docker containers
> ------------------------------------------------------------------------------
>
>                 Key: YARN-5366
>                 URL: https://issues.apache.org/jira/browse/YARN-5366
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: yarn
>            Reporter: Shane Kumpf
>            Assignee: Shane Kumpf
>         Attachments: YARN-5366.001.patch, YARN-5366.002.patch
>
>
> Currently, completed and failed docker containers are removed by container-executor. Add a job level environment variable to DockerLinuxContainerRuntime to allow the user to toggle whether they want the container deleted or not and remove the logic from container-executor.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org