YARN-5714. ContainerExecutor does not order environment map. Contributed by Remi Catherinot and Jim Brennan

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/ContainerExecutor.java

@@ -361,7 +361,8 @@ public void writeLaunchEnv(OutputStream out, Map<String, String> environment,
 
     if (environment != null) {
       sb.echo("Setting up env variables");
-      for (Map.Entry<String, String> env : environment.entrySet()) {
+      for (Map.Entry<String, String> env :
+          sb.orderEnvByDependencies(environment).entrySet()) {
         sb.env(env.getKey(), env.getValue());
       }
     }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java

@@ -30,11 +30,16 @@
 import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.EnumSet;
 import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.atomic.AtomicBoolean;
 
@@ -1121,8 +1126,14 @@ Context getContext() {
 
   public static abstract class ShellScriptBuilder {
     public static ShellScriptBuilder create() {
-      return Shell.WINDOWS ? new WindowsShellScriptBuilder() :
+      return create(Shell.osType);
-        new UnixShellScriptBuilder();
+    }
+
+    @VisibleForTesting
+    public static ShellScriptBuilder create(Shell.OSType osType) {
+      return (osType == Shell.OSType.OS_TYPE_WIN) ?
+          new WindowsShellScriptBuilder() :
+          new UnixShellScriptBuilder();
     }
 
     private static final String LINE_SEPARATOR =
@@ -1248,6 +1259,72 @@ boolean doRedirectStdErr() {
       return redirectStdErr;
     }
 
+    /**
+     * Parse an environment value and returns all environment keys it uses.
+     * @param envVal an environment variable's value
+     * @return all environment variable names used in <code>envVal</code>.
+     */
+    public Set<String> getEnvDependencies(final String envVal) {
+      return Collections.emptySet();
+    }
+
+    /**
+     * Returns a dependency ordered version of <code>envs</code>. Does not alter
+     * input <code>envs</code> map.
+     * @param envs environment map
+     * @return a dependency ordered version of <code>envs</code>
+     */
+    public final Map<String, String> orderEnvByDependencies(
+        Map<String, String> envs) {
+      if (envs == null || envs.size() < 2) {
+        return envs;
+      }
+      final Map<String, String> ordered = new LinkedHashMap<String, String>();
+      class Env {
+        private boolean resolved = false;
+        private final Collection<Env> deps = new ArrayList<>();
+        private final String name;
+        private final String value;
+        Env(String name, String value) {
+          this.name = name;
+          this.value = value;
+        }
+        void resolve() {
+          resolved = true;
+          for (Env dep : deps) {
+            if (!dep.resolved) {
+              dep.resolve();
+            }
+          }
+          ordered.put(name, value);
+        }
+      }
+      final Map<String, Env> singletons = new HashMap<>();
+      for (Map.Entry<String, String> e : envs.entrySet()) {
+        Env env = singletons.get(e.getKey());
+        if (env == null) {
+          env = new Env(e.getKey(), e.getValue());
+          singletons.put(env.name, env);
+        }
+        for (String depStr : getEnvDependencies(env.value)) {
+          if (!envs.containsKey(depStr)) {
+            continue;
+          }
+          Env depEnv = singletons.get(depStr);
+          if (depEnv == null) {
+            depEnv = new Env(depStr, envs.get(depStr));
+            singletons.put(depStr, depEnv);
+          }
+          env.deps.add(depEnv);
+        }
+      }
+      for (Env env : singletons.values()) {
+        if (!env.resolved) {
+          env.resolve();
+        }
+      }
+      return ordered;
+    }
   }
 
   private static final class UnixShellScriptBuilder extends ShellScriptBuilder {
@@ -1339,6 +1416,79 @@ public void listDebugInformation(Path output) throws  IOException {
     public void setExitOnFailure() {
       line("set -o pipefail -e");
     }
+
+    /**
+     * Parse <code>envVal</code> using bash-like syntax to extract env variables
+     * it depends on.
+     */
+    @Override
+    public Set<String> getEnvDependencies(final String envVal) {
+      if (envVal == null || envVal.isEmpty()) {
+        return Collections.emptySet();
+      }
+      final Set<String> deps = new HashSet<>();
+      // env/whitelistedEnv dump values inside double quotes
+      boolean inDoubleQuotes = true;
+      char c;
+      int i = 0;
+      final int len = envVal.length();
+      while (i < len) {
+        c = envVal.charAt(i);
+        if (c == '"') {
+          inDoubleQuotes = !inDoubleQuotes;
+        } else if (c == '\'' && !inDoubleQuotes) {
+          i++;
+          // eat until closing simple quote
+          while (i < len) {
+            c = envVal.charAt(i);
+            if (c == '\\') {
+              i++;
+            }
+            if (c == '\'') {
+              break;
+            }
+            i++;
+          }
+        } else if (c == '\\') {
+          i++;
+        } else if (c == '$') {
+          i++;
+          if (i >= len) {
+            break;
+          }
+          c = envVal.charAt(i);
+          if (c == '{') { // for ${... bash like syntax
+            i++;
+            if (i >= len) {
+              break;
+            }
+            c = envVal.charAt(i);
+            if (c == '#') { // for ${#... bash array syntax
+              i++;
+              if (i >= len) {
+                break;
+              }
+            }
+          }
+          final int start = i;
+          while (i < len) {
+            c = envVal.charAt(i);
+            if (c != '$' && (
+                (i == start && Character.isJavaIdentifierStart(c)) ||
+                    (i > start && Character.isJavaIdentifierPart(c)))) {
+              i++;
+            } else {
+              break;
+            }
+          }
+          if (i > start) {
+            deps.add(envVal.substring(start, i));
+          }
+        }
+        i++;
+      }
+      return deps;
+    }
   }
 
   private static final class WindowsShellScriptBuilder
@@ -1420,6 +1570,46 @@ public void listDebugInformation(Path output) throws IOException {
           String.format("@echo \"dir:\" > \"%s\"", output.toString()));
       lineWithLenCheck(String.format("dir >> \"%s\"", output.toString()));
     }
+
+    /**
+     * Parse <code>envVal</code> using cmd/bat-like syntax to extract env
+     * variables it depends on.
+     */
+    public Set<String> getEnvDependencies(final String envVal) {
+      if (envVal == null || envVal.isEmpty()) {
+        return Collections.emptySet();
+      }
+      final Set<String> deps = new HashSet<>();
+      final int len = envVal.length();
+      int i = 0;
+      while (i < len) {
+        i = envVal.indexOf('%', i); // find beginning of variable
+        if (i < 0 || i == (len - 1)) {
+          break;
+        }
+        i++;
+        // 3 cases: %var%, %var:...% or %%
+        final int j = envVal.indexOf('%', i); // find end of variable
+        if (j == i) {
+          // %% case, just skip it
+          i++;
+          continue;
+        }
+        if (j < 0) {
+          break; // even %var:...% syntax ends with a %, so j cannot be negative
+        }
+        final int k = envVal.indexOf(':', i);
+        if (k >= 0 && k < j) {
+          // %var:...% syntax
+          deps.add(envVal.substring(i, k));
+        } else {
+          // %var% syntax
+          deps.add(envVal.substring(i, j));
+        }
+        i = j + 1;
+      }
+      return deps;
+    }
   }
 
   private static void putEnvIfNotNull(

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/TestContainerLaunch.java

@@ -40,9 +40,12 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.StringTokenizer;
 import java.util.jar.JarFile;
 import java.util.jar.Manifest;
@@ -1824,4 +1827,338 @@ public void testValidEnvVariableSubstitution() throws IOException  {
       }
     }
   }
+
+
+  private static void assertOrderEnvByDependencies(
+      final Map<String, String> env,
+      final ContainerLaunch.ShellScriptBuilder sb) {
+    LinkedHashMap<String, String> copy = new LinkedHashMap<>();
+    copy.putAll(env);
+    Map<String, String> ordered = sb.orderEnvByDependencies(env);
+    // 1st, check that env and copy are the same
+    Assert.assertEquals(
+        "Input env map has been altered because its size changed",
+        copy.size(), env.size()
+    );
+    final Iterator<Map.Entry<String, String>> ai = env.entrySet().iterator();
+    for (Map.Entry<String, String> e : copy.entrySet()) {
+      Map.Entry<String, String> a = ai.next();
+      Assert.assertTrue(
+          "Keys have been reordered in input env map",
+          // env must not be altered at all, so we don't use String.equals
+          // copy and env must use the same String refs
+          e.getKey() == a.getKey()
+      );
+      Assert.assertTrue(
+          "Key "+e.getKey()+" does not longer points to its "
+              +"original value have been reordered in input env map",
+          // env must be altered at all, so we don't use String.equals
+          // copy and env must use the same String refs
+          e.getValue() == a.getValue()
+      );
+    }
+    // 2nd, check the ordered version as the expected ordering
+    // and did not altered values
+    Assert.assertEquals(
+        "Input env map and ordered env map must have the same size, env="+env+
+            ", ordered="+ordered, env.size(), ordered.size()
+    );
+    int iA = -1;
+    int iB = -1;
+    int iC = -1;
+    int iD = -1;
+    int icA = -1;
+    int icB = -1;
+    int icC = -1;
+    int i=0;
+    for (Map.Entry<String, String> e: ordered.entrySet()) {
+      if ("A".equals(e.getKey())) {
+        iA = i++;
+      } else if ("B".equals(e.getKey())) {
+        iB = i++;
+      } else if ("C".equals(e.getKey())) {
+        iC = i++;
+      } else if ("D".equals(e.getKey())) {
+        iD = i++;
+      } else if ("cyclic_A".equals(e.getKey())) {
+        icA = i++;
+      } else if ("cyclic_B".equals(e.getKey())) {
+        icB = i++;
+      } else if ("cyclic_C".equals(e.getKey())) {
+        icC = i++;
+      } else {
+        Assert.fail("Test need to ne fixed, got an unexpected env entry "+
+            e.getKey());
+      }
+    }
+    // expected order : A<B<C<{D,cyclic_A,cyclic_B,cyclic_C}
+    // B depends on A, C depends on B so there are assertion on B>A and C>B
+    // but there is no assertion about C>A because B might be missing in some
+    // broken envs
+    Assert.assertTrue("when reordering "+env+" into "+ordered+
+        ", B should be after A", iA<0 || iB<0 || iA<iB);
+    Assert.assertTrue("when reordering "+env+" into "+ordered+
+        ", C should be after B", iB<0 || iC<0 || iB<iC);
+    Assert.assertTrue("when reordering "+env+" into "+ordered+
+        ", D should be after A", iA<0 || iD<0 || iA<iD);
+    Assert.assertTrue("when reordering "+env+" into "+ordered+
+        ", D should be after B", iB<0 || iD<0 || iB<iD);
+    Assert.assertTrue("when reordering "+env+" into "+ordered+
+        ", cyclic_A should be after C", iC<0 || icA<0 || icB<0 || icC<0 ||
+        iC<icA);
+    Assert.assertTrue("when reordering "+env+" into "+ordered+
+        ", cyclic_B should be after C", iC<0 || icB<0 || icC<0 ||
+        iC<icB);
+    Assert.assertTrue("when reordering "+env+" into "+ordered+
+        ", cyclic_C should be after C", iC<0 || icC<0 || iC<icC);
+    Assert.assertTrue("when reordering "+env+" into "+ordered+
+        ", cyclic_A should be after cyclic_B if no cyclic_C", icC>=0 ||
+        icA<0 || icB<0 || icB<icA);
+    Assert.assertTrue("when reordering "+env+" into "+ordered+
+        ", cyclic_B should be after cyclic_C if no cyclic_A", icA>=0 ||
+        icB<0 || icC<0 || icC<icB);
+    Assert.assertTrue("when reordering "+env+" into "+ordered+
+        ", cyclic_C should be after cyclic_A if no cyclic_B", icA>=0 ||
+        icC<0 || icA<0 || icA<icC);
+  }
+
+  @Test(timeout = 1000)
+  public void testGetEnvDependencies() {
+    final Set<String> expected = new HashSet<>();
+    final ContainerLaunch.ShellScriptBuilder bash =
+        ContainerLaunch.ShellScriptBuilder.create(Shell.OSType.OS_TYPE_LINUX);
+    String s;
+
+    s = null;
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "A";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "\\$A";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "$$";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "$1";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "handle \"'$A'\" simple quotes";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "$ crash test for StringArrayOutOfBoundException";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "${ crash test for StringArrayOutOfBoundException";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "${# crash test for StringArrayOutOfBoundException";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "crash test for StringArrayOutOfBoundException $";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "crash test for StringArrayOutOfBoundException ${";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "crash test for StringArrayOutOfBoundException ${#";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+
+    expected.add("A");
+    s = "$A";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "${A}";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "${#A[*]}";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "in the $A midlle";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+
+    expected.add("B");
+    s = "${A:-$B} var in var";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "${A}$B var outside var";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+
+    expected.add("C");
+    s = "$A:$B:$C:pathlist var";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+    s = "${A}/foo/bar:$B:${C}:pathlist var";
+    Assert.assertEquals("failed to parse " + s, expected,
+        bash.getEnvDependencies(s));
+
+    ContainerLaunch.ShellScriptBuilder win =
+        ContainerLaunch.ShellScriptBuilder.create(Shell.OSType.OS_TYPE_WIN);
+    expected.clear();
+    s = null;
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "A";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "%%%%%%";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "%%A%";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "%A";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "%A:";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+
+    expected.add("A");
+    s = "%A%";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "%%%A%";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "%%C%A%";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "%A:~-1%";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "%A%B%";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "%A%%%%%B%";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+
+    expected.add("B");
+    s = "%A%%B%";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "%A%%%%B%";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+
+    expected.add("C");
+    s = "%A%:%B%:%C%:pathlist var";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+    s = "%A%\\foo\\bar:%B%:%C%:pathlist var";
+    Assert.assertEquals("failed to parse " + s, expected,
+        win.getEnvDependencies(s));
+  }
+
+  private Set<String> asSet(String...str) {
+    final Set<String> set = new HashSet<>();
+    Collections.addAll(set, str);
+    return set;
+  }
+
+  @Test(timeout = 5000)
+  public void testOrderEnvByDependencies() {
+    final Map<String, Set<String>> fakeDeps = new HashMap<>();
+    fakeDeps.put("Aval", Collections.emptySet()); // A has no dependencies
+    fakeDeps.put("Bval", asSet("A")); // B depends on A
+    fakeDeps.put("Cval", asSet("B")); // C depends on B
+    fakeDeps.put("Dval", asSet("A", "B")); // C depends on B
+    fakeDeps.put("cyclic_Aval", asSet("cyclic_B"));
+    fakeDeps.put("cyclic_Bval", asSet("cyclic_C"));
+    fakeDeps.put("cyclic_Cval", asSet("cyclic_A", "C"));
+
+    final ContainerLaunch.ShellScriptBuilder sb =
+        new ContainerLaunch.ShellScriptBuilder() {
+          @Override public Set<String> getEnvDependencies(final String envVal) {
+            return fakeDeps.get(envVal);
+          }
+          @Override protected void mkdir(Path path) throws IOException {}
+          @Override public void listDebugInformation(Path output)
+              throws IOException {}
+          @Override protected void link(Path src, Path dst)
+              throws IOException {}
+          @Override public void env(String key, String value)
+              throws IOException {}
+          @Override public void copyDebugInformation(Path src, Path dst)
+              throws IOException {}
+          @Override public void command(List<String> command)
+              throws IOException {}
+          @Override public void setStdOut(Path stdout)
+              throws IOException {};
+          @Override public void setStdErr(Path stdout)
+              throws IOException {};
+          @Override public void echo(String echoStr)
+              throws IOException {};
+        };
+
+    try {
+      Assert.assertNull("Ordering a null env map must return a null value.",
+          sb.orderEnvByDependencies(null));
+    } catch (Exception e) {
+      Assert.fail("null value is to be supported");
+    }
+
+    try {
+      Assert.assertEquals(
+          "Ordering an empty env map must return an empty map.",
+          0, sb.orderEnvByDependencies(Collections.emptyMap()).size()
+      );
+    } catch (Exception e) {
+      Assert.fail("Empty map is to be supported");
+    }
+
+    final Map<String, String> combination = new LinkedHashMap<>();
+    // to test all possible cases, we create all possible combinations and test
+    // each of them
+    class TestEnv {
+      private final String key;
+      private final String value;
+      private boolean used=false;
+      TestEnv(String key, String value) {
+        this.key = key;
+        this.value = value;
+      }
+      void generateCombinationAndTest(int nbItems,
+                                      final ArrayList<TestEnv> keylist) {
+        used = true;
+        combination.put(key, value);
+        try {
+          if (nbItems == 0) {
+            //LOG.info("Combo : " + combination);
+            assertOrderEnvByDependencies(combination, sb);
+            return;
+          }
+          for (TestEnv localEnv: keylist) {
+            if (!localEnv.used) {
+              localEnv.generateCombinationAndTest(nbItems - 1, keylist);
+            }
+          }
+        } finally {
+          combination.remove(key);
+          used=false;
+        }
+      }
+    }
+    final ArrayList<TestEnv> keys = new ArrayList<>();
+    for (String key : new String[] {"A", "B", "C", "D",
+        "cyclic_A", "cyclic_B", "cyclic_C"}) {
+      keys.add(new TestEnv(key, key+"val"));
+    }
+    for (int count=keys.size(); count > 0; count--) {
+      for (TestEnv env : keys) {
+        env.generateCombinationAndTest(count, keys);
+      }
+    }
+  }
 }