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

Varun Vasudev commented on YARN-3853:
-------------------------------------

Thanks for the patch [~sidharta-s]. One question -
Can you explain what the purpose of
{code}
whitelist.add(YarnConfiguration.NM_DOCKER_CONTAINER_EXECUTOR_IMAGE_NAME);
{code}
is?

Some feedback on the patch:
# Can we rename the DefaultLinuxContainerRuntime to ProcessContainerRuntime and 
rename DockerLinuxContainerRuntime to DockerContainerRuntime - both are already 
in the nodemanager.containermanager.linux.runtime package so the Linux seems 
redundant and I think Process is better than Default.
# In LinuxContainerExecutor
{code}
+
+  public LinuxContainerExecutor() {
+  }
+
+  // created primarily for testing
+  public LinuxContainerExecutor(LinuxContainerRuntime linuxContainerRuntime) {
+    this.linuxContainerRuntime = linuxContainerRuntime;
+  }
{code}
Maybe these should be protected? In addition, the VisibleForTesting annotation 
should be used
# In LinuxContainerExecutor
{code}
-      containerSchedPriorityIsSet = true;
-      containerSchedPriorityAdjustment = conf
-          .getInt(YarnConfiguration.NM_CONTAINER_EXECUTOR_SCHED_PRIORITY,
-          YarnConfiguration.DEFAULT_NM_CONTAINER_EXECUTOR_SCHED_PRIORITY);
+     containerSchedPriorityIsSet = true;
+     containerSchedPriorityAdjustment = conf
+         .getInt(YarnConfiguration.NM_CONTAINER_EXECUTOR_SCHED_PRIORITY,
+             YarnConfiguration.DEFAULT_NM_CONTAINER_EXECUTOR_SCHED_PRIORITY);
     }
{code}
Looks like the formatting is messed up.
# In LinuxContainerExecutor, we've removed some debug statements; we should put 
them back in
{code}
-    if (LOG.isDebugEnabled()) {
-      LOG.debug("Output from LinuxContainerExecutor's launchContainer 
follows:");
-      logOutput(shExec.getOutput());
-    }
{code}
and
{code}
-    if (LOG.isDebugEnabled()) {
-      LOG.debug("signalContainer: " + Arrays.toString(command));
-    }
{code}
# In ContainerLaunch.java
{code}
     @Override
+    public void whitelistedEnv(String key, String value) throws IOException {
+      lineWithLenCheck("@set ", key, "=", value);
+      errorCheck();
+    }
{code}
This code is exactly the same as the env() function. Maybe it should just call 
the env() function instead?
# There are some unused imports in DockerLinuxContainerRuntime, DockerClient 
and TestDockerContainerRuntime

> Add docker container runtime support to LinuxContainterExecutor
> ---------------------------------------------------------------
>
>                 Key: YARN-3853
>                 URL: https://issues.apache.org/jira/browse/YARN-3853
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: yarn
>            Reporter: Sidharta Seethana
>            Assignee: Sidharta Seethana
>         Attachments: YARN-3853.001.patch
>
>
> Create a new DockerContainerRuntime that implements support for docker 
> containers via container-executor. LinuxContainerExecutor should default to 
> current behavior when launching containers but switch to docker when 
> requested. 
> Overview
> ===
> The current mechanism of launching/signaling containers is moved to its own 
> (default) container runtime. In order to use docker container runtime a 
> couple of environment variables have to be set. This will have to be 
> revisited when we have a first class client side API to specify different 
> container types and associated parameters. Using ‘pi’ as an example and using 
> a custom docker image, this is how you could use the docker container runtime 
> (LinuxContainerExecutor must be in use and the docker daemon needs to be 
> running) :
> {code}
> export 
> YARN_EXAMPLES_JAR=./share/hadoop/mapreduce/hadoop-mapreduce-examples-*.jar
> bin/yarn jar $YARN_EXAMPLES_JAR pi 
> -Dmapreduce.map.env="YARN_CONTAINER_RUNTIME_TYPE=docker,YARN_CONTAINER_RUNTIME_DOCKER_IMAGE=ashahab/hadoop-trunk"
>  
> -Dyarn.app.mapreduce.am.env="YARN_CONTAINER_RUNTIME_TYPE=docker,YARN_CONTAINER_RUNTIME_DOCKER_IMAGE=ashahab/hadoop-trunk"
>   
> -Dmapreduce.reduce.env="YARN_CONTAINER_RUNTIME_TYPE=docker,YARN_CONTAINER_RUNTIME_DOCKER_IMAGE=ashahab/hadoop-trunk"
>  4 1000
> {code}
>  
> LinuxContainerExecutor can delegate to either runtime on a per container 
> basis. If the docker container type is selected, LinuxContainerExecutor 
> delegates to the DockerContainerRuntime which in turn uses docker support in 
> the container-executor binary to launch/manage docker containers ( see 
> YARN-3852 ) . 



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

Reply via email to