YARN-7677. Docker image cannot set HADOOP_CONF_DIR. Contributed by Ji…

…m Brennan
apache · Jan 31, 2018 · 12eaae3 · 12eaae3
1 parent d481344
commit 12eaae3
Show file tree

Hide file tree

Showing 8 changed files with 33 additions and 66 deletions.
diff --git a/...odemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/ContainerExecutor.java b/...odemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/ContainerExecutor.java
@@ -346,7 +346,6 @@ public void writeLaunchEnv(OutputStream out, Map<String, String> environment,
   public void writeLaunchEnv(OutputStream out, Map<String, String> environment,
       Map<Path, List<String>> resources, List<String> command, Path logDir,
       String user, String outFilename) throws IOException {
-    updateEnvForWhitelistVars(environment);
 
     ContainerLaunch.ShellScriptBuilder sb =
         ContainerLaunch.ShellScriptBuilder.create();
@@ -364,6 +363,19 @@ public void writeLaunchEnv(OutputStream out, Map<String, String> environment,
       for (Map.Entry<String, String> env : environment.entrySet()) {
         sb.env(env.getKey(), env.getValue());
       }
+      // Whitelist environment variables are treated specially.
+      // Only add them if they are not already defined in the environment.
+      // Add them using special syntax to prevent them from eclipsing
+      // variables that may be set explicitly in the container image (e.g,
+      // in a docker image)
+      for(String var : whitelistVars) {
+        if (!environment.containsKey(var)) {
+          String val = getNMEnvVar(var);
+          if (val != null) {
+            sb.whitelistedEnv(var, val);
+          }
+        }
+      }
     }
 
     if (resources != null) {
@@ -663,23 +675,6 @@ protected boolean isContainerActive(ContainerId containerId) {
     }
   }
 
-  /**
-   * Propagate variables from the nodemanager's environment into the
-   * container's environment if unspecified by the container.
-   * @param env the environment to update
-   * @see org.apache.hadoop.yarn.conf.YarnConfiguration#NM_ENV_WHITELIST
-   */
-  protected void updateEnvForWhitelistVars(Map<String, String> env) {
-    for(String var : whitelistVars) {
-      if (!env.containsKey(var)) {
-        String val = getNMEnvVar(var);
-        if (val != null) {
-          env.put(var, val);
-        }
-      }
-    }
-  }
-
   @VisibleForTesting
   protected String getNMEnvVar(String varname) {
     return System.getenv(varname);

diff --git a/...nager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/LinuxContainerExecutor.java b/...nager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/LinuxContainerExecutor.java
@@ -66,7 +66,6 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
-import java.util.Map;
 import java.util.regex.Pattern;
 
 import static org.apache.hadoop.yarn.server.nodemanager.containermanager.linux.runtime.LinuxContainerRuntimeConstants.*;
@@ -472,13 +471,6 @@ public void prepareContainer(ContainerPrepareContext ctx) throws IOException {
     }
   }
 
-  @Override
-  protected void updateEnvForWhitelistVars(Map<String, String> env) {
-    if (linuxContainerRuntime.useWhitelistEnv(env)) {
-      super.updateEnvForWhitelistVars(env);
-    }
-  }
-
   @Override
   public int launchContainer(ContainerStartContext ctx)
       throws IOException, ConfigurationException {

diff --git a/.../org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java b/.../org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
@@ -1135,6 +1135,9 @@ public final void stderr(Path stderrDir, String stdErrFile) throws IOException {
 
     public abstract void env(String key, String value) throws IOException;
 
+    public abstract void whitelistedEnv(String key, String value)
+        throws IOException;
+
     public abstract void echo(String echoStr) throws IOException;
 
     public final void symlink(Path src, Path dst) throws IOException {
@@ -1254,6 +1257,11 @@ public void env(String key, String value) throws IOException {
       line("export ", key, "=\"", value, "\"");
     }
 
+    @Override
+    public void whitelistedEnv(String key, String value) throws IOException {
+      line("export ", key, "=${", key, ":-", "\"", value, "\"}");
+    }
+
     @Override
     public void echo(final String echoStr) throws IOException {
       line("echo \"" + echoStr + "\"");
@@ -1344,6 +1352,11 @@ public void env(String key, String value) throws IOException {
       errorCheck();
     }
 
+    @Override
+    public void whitelistedEnv(String key, String value) throws IOException {
+      env(key, value);
+    }
+
     @Override
     public void echo(final String echoStr) throws IOException {
       lineWithLenCheck("@echo \"", echoStr, "\"");
@@ -1443,8 +1456,6 @@ public void sanitizeEnv(Map<String, String> environment, Path pwd,
 
     environment.put(Environment.PWD.name(), pwd.toString());
 
-    putEnvIfAbsent(environment, Environment.HADOOP_CONF_DIR.name());
-
     if (!Shell.WINDOWS) {
       environment.put("JVM_PID", "$$");
     }

diff --git a/.../yarn/server/nodemanager/containermanager/linux/runtime/DefaultLinuxContainerRuntime.java b/.../yarn/server/nodemanager/containermanager/linux/runtime/DefaultLinuxContainerRuntime.java
@@ -37,7 +37,6 @@
 import org.slf4j.LoggerFactory;
 
 import java.util.List;
-import java.util.Map;
 
 import static org.apache.hadoop.yarn.server.nodemanager.containermanager.linux.runtime.LinuxContainerRuntimeConstants.*;
 
@@ -73,11 +72,6 @@ public void initialize(Configuration conf, Context nmContext)
     this.conf = conf;
   }
 
-  @Override
-  public boolean useWhitelistEnv(Map<String, String> env) {
-    return true;
-  }
-
   @Override
   public void prepareContainer(ContainerRuntimeContext ctx)
       throws ContainerExecutionException {

diff --git a/...rn/server/nodemanager/containermanager/linux/runtime/DelegatingLinuxContainerRuntime.java b/...rn/server/nodemanager/containermanager/linux/runtime/DelegatingLinuxContainerRuntime.java
@@ -94,17 +94,6 @@ public void initialize(Configuration conf, Context nmContext)
     }
   }
 
-  @Override
-  public boolean useWhitelistEnv(Map<String, String> env) {
-    try {
-      LinuxContainerRuntime runtime = pickContainerRuntime(env);
-      return runtime.useWhitelistEnv(env);
-    } catch (ContainerExecutionException e) {
-      LOG.debug("Unable to determine runtime");
-      return false;
-    }
-  }
-
   @VisibleForTesting
   LinuxContainerRuntime pickContainerRuntime(
       Map<String, String> environment) throws ContainerExecutionException {

diff --git a/...p/yarn/server/nodemanager/containermanager/linux/runtime/DockerLinuxContainerRuntime.java b/...p/yarn/server/nodemanager/containermanager/linux/runtime/DockerLinuxContainerRuntime.java
@@ -366,13 +366,6 @@ public Set<String> getCapabilities() {
     return capabilities;
   }
 
-  @Override
-  public boolean useWhitelistEnv(Map<String, String> env) {
-    // Avoid propagating nodemanager environment variables into the container
-    // so those variables can be picked up from the Docker image instead.
-    return false;
-  }
-
   private String runDockerVolumeCommand(DockerVolumeCommand dockerVolumeCommand,
       Container container) throws ContainerExecutionException {
     try {

diff --git a/.../org/apache/hadoop/yarn/server/nodemanager/containermanager/runtime/ContainerRuntime.java b/.../org/apache/hadoop/yarn/server/nodemanager/containermanager/runtime/ContainerRuntime.java
@@ -24,8 +24,6 @@
 import org.apache.hadoop.classification.InterfaceStability;
 import org.apache.hadoop.yarn.server.nodemanager.containermanager.container.Container;
 
-import java.util.Map;
-
 /**
  * An abstraction for various container runtime implementations. Examples
  * include Process Tree, Docker, Appc runtimes etc. These implementations
@@ -85,13 +83,4 @@ void reapContainer(ContainerRuntimeContext ctx)
    * and hostname
    */
   String[] getIpAndHost(Container container) throws ContainerExecutionException;
-
-  /**
-   * Whether to propagate the whitelist of environment variables from the
-   * nodemanager environment into the container environment.
-   * @param env the container's environment variables
-   * @return true if whitelist variables should be propagated, false otherwise
-   * @see org.apache.hadoop.yarn.conf.YarnConfiguration#NM_ENV_WHITELIST
-   */
-  boolean useWhitelistEnv(Map<String, String> env);
 }
diff --git a/.../apache/hadoop/yarn/server/nodemanager/containermanager/launcher/TestContainerLaunch.java b/.../apache/hadoop/yarn/server/nodemanager/containermanager/launcher/TestContainerLaunch.java
@@ -337,7 +337,8 @@ protected String getNMEnvVar(String varname) {
     Assert.assertFalse(shellContent.contains("HADOOP_HDFS_HOME"));
     // Available in env and in whitelist
     Assert.assertTrue(shellContent.contains(
-        "export HADOOP_YARN_HOME=\"nodemanager_yarn_home\""));
+        "export HADOOP_YARN_HOME=${HADOOP_YARN_HOME:-\"nodemanager_yarn_home\"}"
+      ));
     fos.flush();
     fos.close();
   }
@@ -382,9 +383,12 @@ protected String getNMEnvVar(String varname) {
     // Whitelisted variable overridden by container
     Assert.assertTrue(shellContent.contains(
         "export HADOOP_MAPRED_HOME=\"/opt/hadoopbuild\""));
-    // Verify no whitelisted variables inherited from NM env
+    // Available in env but not in whitelist
     Assert.assertFalse(shellContent.contains("HADOOP_HDFS_HOME"));
-    Assert.assertFalse(shellContent.contains("HADOOP_YARN_HOME"));
+    // Available in env and in whitelist
+    Assert.assertTrue(shellContent.contains(
+        "export HADOOP_YARN_HOME=${HADOOP_YARN_HOME:-\"nodemanager_yarn_home\"}"
+    ));
     fos.flush();
     fos.close();
   }