Mailing-List: contact yarn-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Date: Thu, 1 Sep 2016 16:33:20 +0000 (UTC)
From: "Varun Vasudev (JIRA)" <jira@apache.org>
To: yarn-issues@hadoop.apache.org
Message-ID: <JIRA.12988950.1468376466000.470522.1472747600697@Atlassian.JIRA>
In-Reply-To: <JIRA.12988950.1468376466000@Atlassian.JIRA>
References: <JIRA.12988950.1468376466000@Atlassian.JIRA> <JIRA.12988950.1468376466098@arcas>
Subject: [jira] [Commented] (YARN-5366) Add support for toggling the removal
 of completed and failed docker containers
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
archived-at: Thu, 01 Sep 2016 16:33:22 -0000


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

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 contain=
er

2)
We should add a configuration option to let admins allow/disallow this beha=
vior - users shouldn=E2=80=99t be allowed to keep containers around because=
 they feel like it

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

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

4)
{code}
+      String msg =3D
+          "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 =3D=3D 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 !=3D null) {
+      status =3D 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 tha=
t you=E2=80=99ve set, but the the code snippet above shouldn=E2=80=99t be i=
n DockerContainerStatusHandler

8)
Does MockContainerExecutorBinary need to be it=E2=80=99s own class - the pa=
ttern 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 =3D new File("./src/test/resources/mock-container-executor");
{code}

The path of the file looks incorrect. It really should be in the target dir=
ectory. You should create a directory in the target directory and create th=
e file in that directory. It also looks like you create the mock executor b=
ut don=E2=80=99t 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 conta=
iners
> -------------------------------------------------------------------------=
-----
>
>                 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 containe=
r-executor. Add a job level environment variable to DockerLinuxContainerRun=
time 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