From ee37861abfe1442495cc27efcfe9e0a41023e470 Mon Sep 17 00:00:00 2001 From: Sunil Date: Tue, 21 Jun 2016 21:14:18 +0530 Subject: [PATCH] YARN-5219-branch-2 --- .../yarn/server/nodemanager/ContainerExecutor.java | 33 +++++- .../nodemanager/DefaultContainerExecutor.java | 5 + .../containermanager/launcher/ContainerLaunch.java | 35 ++++++ .../launcher/TestContainerLaunch.java | 123 +++++++++++++++++++++ 4 files changed, 195 insertions(+), 1 deletion(-) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/ContainerExecutor.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/ContainerExecutor.java index 98d45f4..5f67f8b 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/ContainerExecutor.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/ContainerExecutor.java @@ -33,6 +33,8 @@ import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock.ReadLock; import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import com.google.common.annotations.VisibleForTesting; import org.apache.commons.io.FileUtils; @@ -270,9 +272,15 @@ public void writeLaunchEnv(OutputStream out, whitelist.add(ApplicationConstants.Environment.HADOOP_CONF_DIR.name()); whitelist.add(ApplicationConstants.Environment.JAVA_HOME.name()); if (environment != null) { - for (Map.Entry env : environment.entrySet()) { + sb.reportError(); + sb.validatorRoutine(); + for (Map.Entry env : environment.entrySet()) { if (!whitelist.contains(env.getKey())) { sb.env(env.getKey().toString(), env.getValue().toString()); + + // Validate all shell variables explicitly passed for null check + extractAndValidateShellVariables(sb, env.getKey().toString(), + env.getValue().toString()); } else { sb.whitelistedEnv(env.getKey().toString(), env.getValue().toString()); } @@ -318,6 +326,19 @@ public void writeLaunchEnv(OutputStream out, } } + private void extractAndValidateShellVariables( + ContainerLaunch.ShellScriptBuilder sb, String key, String value) { + + // Pattern matcher to find valid shell variables from a string. + // $ is the main delimeter here, and also considering {} () too. + Pattern p = Pattern.compile("\\$[a-zA-Z0-9{(_.]+[)}]*"); + Matcher m = p.matcher(value); + + while (m.find()) { + sb.validateShellVariable(key, m.group(0)); + } + } + public enum ExitCode { SUCCESS(0), FORCE_KILLED(137), @@ -369,6 +390,16 @@ protected void logOutput(String output) { } } + protected void logDebugOutput(String output) { + String shExecOutput = output; + if (shExecOutput != null) { + for (String str : shExecOutput.split("\n")) { + if (LOG.isDebugEnabled()) { + LOG.debug(str); + } + } + } + } /** * Get the pidFile of the container. * @param containerId diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/DefaultContainerExecutor.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/DefaultContainerExecutor.java index f8f19bd..b8c4fc9 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/DefaultContainerExecutor.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/DefaultContainerExecutor.java @@ -236,6 +236,11 @@ public int launchContainer(ContainerStartContext ctx) throws IOException { if (isContainerActive(containerId)) { shExec.execute(); + if (!shExec.getOutput().isEmpty()) { + StringBuilder builder = new StringBuilder(); + builder.append("Shell output: " + shExec.getOutput() + "\n"); + logDebugOutput(builder.toString()); + } } else { LOG.info("Container " + containerIdStr + 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 7e9030c..cf1eea9 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 @@ -824,6 +824,18 @@ protected final void line(String... command) { sb.append(LINE_SEPARATOR); } + public void validatorRoutine() { + // Dummy implementation. + } + + public void reportError() { + // Dummy implementation. + } + + public void validateShellVariable(String key, String value) { + // Dummy implementation. + } + protected abstract void link(Path src, Path dst) throws IOException; protected abstract void mkdir(Path path) throws IOException; @@ -839,6 +851,29 @@ private void errorCheck() { line("fi"); } + @Override + public void reportError() { + line("err_report() {"); + line(" echo \"Error on line ${1} while substituting for ${2}\""); + line("}"); + } + + @Override + public void validatorRoutine() { + line("verify_shell_variable() {"); + line("if [ -z $2 ]"); + line("then"); + line(" echo \"Shell Variable ${1} has empty substitutions. \""); + line("fi"); + line("}"); + } + + @Override + public void validateShellVariable(String key, String value) { + line("toValidate=", value); + line("verify_shell_variable ", key, " ${toValidate}"); + } + public UnixShellScriptBuilder(){ line("#!/bin/bash"); line(); diff --git a/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 b/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 index a558338..69bef2e 100644 --- a/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 +++ b/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 @@ -36,6 +36,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.StringTokenizer; @@ -327,6 +328,128 @@ public void testInvalidEnvSyntaxDiagnostics() throws IOException { } } + /* + * ${foo.version} is substituted to suffix a specific version number + */ + @Test + public void testInvalidEnvVariableSubstitutionType1() throws IOException { + Map env = new HashMap(); + // invalid env + env.put("testVar", "version${foo.version}"); + validateShellExecutorForDifferentEnvs(env); + } + + /* + * Multiple paths are substituted in a path variable + */ + @Test + public void testInvalidEnvVariableSubstitutionType2() throws IOException { + Map env = new HashMap(); + // invalid env + env.put("testPath", "/abc:/${foo.path}:/$bar"); + validateShellExecutorForDifferentEnvs(env); + } + + /* + * Direct assignment of a shell variable. We will fail fast in such cases. + */ + @Test + public void testInvalidEnvVariableSubstitutionType3() throws IOException { + Map env = new HashMap(); + // invalid env + env.put("testVar", "$(foo)"); + validateShellExecutorForDifferentEnvs(env); + } + + /* + * Direct assignment without any curley braces. + */ + @Test + public void testInvalidEnvVariableSubstitutionType4() throws IOException { + Map env = new HashMap(); + // invalid env + env.put("testVar", "$test"); + validateShellExecutorForDifferentEnvs(env); + } + + private void validateShellExecutorForDifferentEnvs(Map env) + throws IOException { + File shellFile = null; + try { + shellFile = Shell.appendScriptExtension(tmpDir, "hello"); + Map> resources = new HashMap>(); + FileOutputStream fos = new FileOutputStream(shellFile); + FileUtil.setExecutable(shellFile, true); + + List commands = new ArrayList(); + new DefaultContainerExecutor().writeLaunchEnv(fos, env, resources, + commands, new Path(localLogDir.getAbsolutePath())); + fos.flush(); + fos.close(); + + // It is supposed that LANG is set as C. + Map cmdEnv = new HashMap(); + cmdEnv.put("LANG", "C"); + Shell.ShellCommandExecutor shexc = new Shell.ShellCommandExecutor( + new String[] { shellFile.getAbsolutePath() }, tmpDir, cmdEnv); + try { + shexc.execute(); + } catch (ExitCodeException e) { + Assert.fail("Should not catch exception"); + } + + // Verify whether the script is logging error message for empty variables + Assert.assertTrue(shexc.getOutput().contains("has empty substitutions")); + Assert.assertTrue(shexc.getExitCode() == 0); + } finally { + // cleanup + if (shellFile != null && shellFile.exists()) { + shellFile.delete(); + } + } + } + + @Test + public void testValidEnvVariableSubstitution() throws IOException { + File shellFile = null; + try { + shellFile = Shell.appendScriptExtension(tmpDir, "hello"); + Map> resources = new HashMap>(); + FileOutputStream fos = new FileOutputStream(shellFile); + FileUtil.setExecutable(shellFile, true); + + Map env = new LinkedHashMap(); + // valid env + env.put("foo", "2.4.6"); + env.put("testVar", "version${foo}"); + List commands = new ArrayList(); + new DefaultContainerExecutor().writeLaunchEnv(fos, env, resources, + commands, new Path(localLogDir.getAbsolutePath())); + fos.flush(); + fos.close(); + + // It is supposed that LANG is set as C. + Map cmdEnv = new HashMap(); + cmdEnv.put("LANG", "C"); + Shell.ShellCommandExecutor shexc = new Shell.ShellCommandExecutor( + new String[] { shellFile.getAbsolutePath() }, tmpDir, cmdEnv); + try { + shexc.execute(); + } catch (ExitCodeException e) { + Assert.fail("Should not catch exception"); + } + + // There should not be any logging as all env variables are valid + Assert.assertFalse(shexc.getOutput().contains("has empty substitutions")); + Assert.assertTrue(shexc.getExitCode() == 0); + } finally { + // cleanup + if (shellFile != null && shellFile.exists()) { + shellFile.delete(); + } + } + } + @Test(timeout = 10000) public void testEnvExpansion() throws IOException { Path logPath = new Path("/nm/container/logs"); -- 2.7.4 (Apple Git-66)