diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index 8ee41855522..fe97bcea7e6 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -329,6 +329,10 @@ Release 2.0.5-beta - UNRELEASED HADOOP-9437. TestNativeIO#testRenameTo fails on Windows due to assumption that POSIX errno is embedded in NativeIOException. (Chris Nauroth via suresh) + + HADOOP-9488. FileUtil#createJarWithClassPath only substitutes environment + variables from current process environment/does not support overriding + when launching new process (Chris Nauroth via bikas) Release 2.0.4-beta - UNRELEASED diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java index cd220e550f4..b69622d57de 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java @@ -967,15 +967,17 @@ public class FileUtil { * * @param inputClassPath String input classpath to bundle into the jar manifest * @param pwd Path to working directory to save jar + * @param callerEnv Map caller's environment variables to use + * for expansion * @return String absolute path to new jar * @throws IOException if there is an I/O error while writing the jar file */ - public static String createJarWithClassPath(String inputClassPath, Path pwd) - throws IOException { + public static String createJarWithClassPath(String inputClassPath, Path pwd, + Map callerEnv) throws IOException { // Replace environment variables, case-insensitive on Windows @SuppressWarnings("unchecked") - Map env = Shell.WINDOWS ? - new CaseInsensitiveMap(System.getenv()) : System.getenv(); + Map env = Shell.WINDOWS ? new CaseInsensitiveMap(callerEnv) : + callerEnv; String[] classPathEntries = inputClassPath.split(File.pathSeparator); for (int i = 0; i < classPathEntries.length; ++i) { classPathEntries[i] = StringUtils.replaceTokens(classPathEntries[i], @@ -1006,9 +1008,22 @@ public class FileUtil { } } } else { - // Append just this jar - classPathEntryList.add(new File(classPathEntry).toURI().toURL() - .toExternalForm()); + // Append just this entry + String classPathEntryUrl = new File(classPathEntry).toURI().toURL() + .toExternalForm(); + + // File.toURI only appends trailing '/' if it can determine that it is a + // directory that already exists. (See JavaDocs.) If this entry had a + // trailing '/' specified by the caller, then guarantee that the + // classpath entry in the manifest has a trailing '/', and thus refers to + // a directory instead of a file. This can happen if the caller is + // creating a classpath jar referencing a directory that hasn't been + // created yet, but will definitely be created before running. + if (classPathEntry.endsWith(Path.SEPARATOR) && + !classPathEntryUrl.endsWith(Path.SEPARATOR)) { + classPathEntryUrl = classPathEntryUrl + Path.SEPARATOR; + } + classPathEntryList.add(classPathEntryUrl); } } String jarClassPath = StringUtils.join(" ", classPathEntryList); diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index c42114ff74b..97e630b0243 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -585,11 +585,13 @@ public class TestFileUtil { // create classpath jar String wildcardPath = tmp.getCanonicalPath() + File.separator + "*"; + String nonExistentSubdir = tmp.getCanonicalPath() + Path.SEPARATOR + "subdir" + + Path.SEPARATOR; List classPaths = Arrays.asList("cp1.jar", "cp2.jar", wildcardPath, - "cp3.jar"); + "cp3.jar", nonExistentSubdir); String inputClassPath = StringUtils.join(File.pathSeparator, classPaths); String classPathJar = FileUtil.createJarWithClassPath(inputClassPath, - new Path(tmp.getCanonicalPath())); + new Path(tmp.getCanonicalPath()), System.getenv()); // verify classpath by reading manifest from jar file JarFile jarFile = null; @@ -604,15 +606,20 @@ public class TestFileUtil { Assert.assertNotNull(classPathAttr); List expectedClassPaths = new ArrayList(); for (String classPath: classPaths) { - if (!wildcardPath.equals(classPath)) { - expectedClassPaths.add(new File(classPath).toURI().toURL() - .toExternalForm()); - } else { + if (wildcardPath.equals(classPath)) { // add wildcard matches for (File wildcardMatch: wildcardMatches) { expectedClassPaths.add(wildcardMatch.toURI().toURL() .toExternalForm()); } + } else if (nonExistentSubdir.equals(classPath)) { + // expect to maintain trailing path separator if present in input, even + // if directory doesn't exist yet + expectedClassPaths.add(new File(classPath).toURI().toURL() + .toExternalForm() + Path.SEPARATOR); + } else { + expectedClassPaths.add(new File(classPath).toURI().toURL() + .toExternalForm()); } } List actualClassPaths = Arrays.asList(classPathAttr.split(" ")); diff --git a/hadoop-yarn-project/CHANGES.txt b/hadoop-yarn-project/CHANGES.txt index 0ced01e12d2..05d4320b913 100644 --- a/hadoop-yarn-project/CHANGES.txt +++ b/hadoop-yarn-project/CHANGES.txt @@ -382,6 +382,10 @@ Release 2.0.5-beta - UNRELEASED YARN-487. Modify path manipulation in LocalDirsHandlerService to let TestDiskFailures pass on Windows. (Chris Nauroth via vinodkv) + YARN-593. container launch on Windows does not correctly populate + classpath with new process's environment variables and localized resources + (Chris Nauroth via bikas) + Release 2.0.4-alpha - 2013-04-25 INCOMPATIBLE CHANGES diff --git a/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 b/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 index 2a0b338eb29..b83623a6bde 100644 --- a/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 +++ b/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 @@ -28,6 +28,7 @@ import java.io.OutputStream; import java.io.PrintStream; import java.util.ArrayList; import java.util.EnumSet; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -211,7 +212,7 @@ public class ContainerLaunch implements Callable { FINAL_CONTAINER_TOKENS_FILE).toUri().getPath()); // Sanitize the container's environment - sanitizeEnv(environment, containerWorkDir, appDirs); + sanitizeEnv(environment, containerWorkDir, appDirs, localResources); // Write out the environment writeLaunchEnv(containerScriptOutStream, environment, localResources, @@ -506,9 +507,17 @@ public class ContainerLaunch implements Callable { @Override protected void link(Path src, Path dst) throws IOException { - line(String.format("@%s symlink \"%s\" \"%s\"", Shell.WINUTILS, - new File(dst.toString()).getPath(), - new File(src.toUri().getPath()).getPath())); + File srcFile = new File(src.toUri().getPath()); + String srcFileStr = srcFile.getPath(); + String dstFileStr = new File(dst.toString()).getPath(); + // If not on Java7+ on Windows, then copy file instead of symlinking. + // See also FileUtil#symLink for full explanation. + if (!Shell.isJava7OrAbove() && srcFile.isFile()) { + line(String.format("@copy \"%s\" \"%s\"", srcFileStr, dstFileStr)); + } else { + line(String.format("@%s symlink \"%s\" \"%s\"", Shell.WINUTILS, + dstFileStr, srcFileStr)); + } } @Override @@ -532,7 +541,8 @@ public class ContainerLaunch implements Callable { } public void sanitizeEnv(Map environment, - Path pwd, List appDirs) throws IOException { + Path pwd, List appDirs, Map> resources) + throws IOException { /** * Non-modifiable environment variables */ @@ -576,16 +586,6 @@ public class ContainerLaunch implements Callable { environment.put("JVM_PID", "$$"); } - // TODO: Remove Windows check and use this approach on all platforms after - // additional testing. See YARN-358. - if (Shell.WINDOWS) { - String inputClassPath = environment.get(Environment.CLASSPATH.name()); - if (inputClassPath != null && !inputClassPath.isEmpty()) { - environment.put(Environment.CLASSPATH.name(), - FileUtil.createJarWithClassPath(inputClassPath, pwd)); - } - } - /** * Modifiable environment variables */ @@ -604,6 +604,57 @@ public class ContainerLaunch implements Callable { YarnConfiguration.NM_ADMIN_USER_ENV, YarnConfiguration.DEFAULT_NM_ADMIN_USER_ENV) ); + + // TODO: Remove Windows check and use this approach on all platforms after + // additional testing. See YARN-358. + if (Shell.WINDOWS) { + String inputClassPath = environment.get(Environment.CLASSPATH.name()); + if (inputClassPath != null && !inputClassPath.isEmpty()) { + StringBuilder newClassPath = new StringBuilder(inputClassPath); + + // Localized resources do not exist at the desired paths yet, because the + // container launch script has not run to create symlinks yet. This + // means that FileUtil.createJarWithClassPath can't automatically expand + // wildcards to separate classpath entries for each file in the manifest. + // To resolve this, append classpath entries explicitly for each + // resource. + for (Map.Entry> entry : resources.entrySet()) { + boolean targetIsDirectory = new File(entry.getKey().toUri().getPath()) + .isDirectory(); + + for (String linkName : entry.getValue()) { + // Append resource. + newClassPath.append(File.pathSeparator).append(pwd.toString()) + .append(Path.SEPARATOR).append(linkName); + + // FileUtil.createJarWithClassPath must use File.toURI to convert + // each file to a URI to write into the manifest's classpath. For + // directories, the classpath must have a trailing '/', but + // File.toURI only appends the trailing '/' if it is a directory that + // already exists. To resolve this, add the classpath entries with + // explicit trailing '/' here for any localized resource that targets + // a directory. Then, FileUtil.createJarWithClassPath will guarantee + // that the resulting entry in the manifest's classpath will have a + // trailing '/', and thus refer to a directory instead of a file. + if (targetIsDirectory) { + newClassPath.append(Path.SEPARATOR); + } + } + } + + // When the container launches, it takes the parent process's environment + // and then adds/overwrites with the entries from the container launch + // context. Do the same thing here for correct substitution of + // environment variables in the classpath jar manifest. + Map mergedEnv = new HashMap( + System.getenv()); + mergedEnv.putAll(environment); + + String classPathJar = FileUtil.createJarWithClassPath( + newClassPath.toString(), pwd, mergedEnv); + environment.put(Environment.CLASSPATH.name(), classPathJar); + } + } } static void writeLaunchEnv(OutputStream out,