From 2fb649a668d3186c0e78d38e6425639f9f2b020d Mon Sep 17 00:00:00 2001 From: Ivan Mitic Date: Sat, 19 Apr 2014 19:00:42 +0000 Subject: [PATCH] YARN-1865 Merging change r1588693 from trunk. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/branch-2@1588696 13f79535-47bb-0310-9956-ffa450edef68 --- .../java/org/apache/hadoop/util/Shell.java | 28 +++ hadoop-yarn-project/CHANGES.txt | 3 + .../launcher/ContainerLaunch.java | 59 ++++-- .../launcher/TestContainerLaunch.java | 175 +++++++++++++++++- 4 files changed, 246 insertions(+), 19 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java index 59c64c63583..61c0e8ddc31 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java @@ -53,6 +53,34 @@ abstract public class Shell { return IS_JAVA7_OR_ABOVE; } + /** + * Maximum command line length in Windows + * KB830473 documents this as 8191 + */ + public static final int WINDOWS_MAX_SHELL_LENGHT = 8191; + + /** + * Checks if a given command (String[]) fits in the Windows maximum command line length + * Note that the input is expected to already include space delimiters, no extra count + * will be added for delimiters. + * + * @param commands command parts, including any space delimiters + */ + public static void checkWindowsCommandLineLength(String...commands) + throws IOException { + int len = 0; + for (String s: commands) { + len += s.length(); + } + if (len > WINDOWS_MAX_SHELL_LENGHT) { + throw new IOException(String.format( + "The command line has a length of %d exceeds maximum allowed length of %d. " + + "Command starts with: %s", + len, WINDOWS_MAX_SHELL_LENGHT, + StringUtils.join("", commands).substring(0, 100))); + } + } + /** a Unix command to get the current user's name */ public final static String USER_NAME_COMMAND = "whoami"; diff --git a/hadoop-yarn-project/CHANGES.txt b/hadoop-yarn-project/CHANGES.txt index 81660d6e483..756c0aa38b4 100644 --- a/hadoop-yarn-project/CHANGES.txt +++ b/hadoop-yarn-project/CHANGES.txt @@ -56,6 +56,9 @@ Release 2.5.0 - UNRELEASED YARN-1940. deleteAsUser() terminates early without deleting more files on error (Rushabh S Shah via jlowe) + YARN-1865. ShellScriptBuilder does not check for some error conditions. + (Remus Rusanu via ivanmi) + Release 2.4.1 - UNRELEASED 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 0fd1200ab30..e252e35dfc8 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 @@ -479,15 +479,20 @@ public class ContainerLaunch implements Callable { + appIdStr; } - private static abstract class ShellScriptBuilder { + @VisibleForTesting + static abstract class ShellScriptBuilder { + public static ShellScriptBuilder create() { + return Shell.WINDOWS ? new WindowsShellScriptBuilder() : + new UnixShellScriptBuilder(); + } private static final String LINE_SEPARATOR = System.getProperty("line.separator"); private final StringBuilder sb = new StringBuilder(); - public abstract void command(List command); + public abstract void command(List command) throws IOException; - public abstract void env(String key, String value); + public abstract void env(String key, String value) throws IOException; public final void symlink(Path src, Path dst) throws IOException { if (!src.isAbsolute()) { @@ -520,11 +525,19 @@ public class ContainerLaunch implements Callable { protected abstract void link(Path src, Path dst) throws IOException; - protected abstract void mkdir(Path path); + protected abstract void mkdir(Path path) throws IOException; } private static final class UnixShellScriptBuilder extends ShellScriptBuilder { + private void errorCheck() { + line("hadoop_shell_errorcode=$?"); + line("if [ $hadoop_shell_errorcode -ne 0 ]"); + line("then"); + line(" exit $hadoop_shell_errorcode"); + line("fi"); + } + public UnixShellScriptBuilder(){ line("#!/bin/bash"); line(); @@ -533,6 +546,7 @@ public class ContainerLaunch implements Callable { @Override public void command(List command) { line("exec /bin/bash -c \"", StringUtils.join(" ", command), "\""); + errorCheck(); } @Override @@ -543,31 +557,43 @@ public class ContainerLaunch implements Callable { @Override protected void link(Path src, Path dst) throws IOException { line("ln -sf \"", src.toUri().getPath(), "\" \"", dst.toString(), "\""); + errorCheck(); } @Override protected void mkdir(Path path) { line("mkdir -p ", path.toString()); + errorCheck(); } } private static final class WindowsShellScriptBuilder extends ShellScriptBuilder { + private void errorCheck() { + line("@if %errorlevel% neq 0 exit /b %errorlevel%"); + } + + private void lineWithLenCheck(String... commands) throws IOException { + Shell.checkWindowsCommandLineLength(commands); + line(commands); + } + public WindowsShellScriptBuilder() { line("@setlocal"); line(); } @Override - public void command(List command) { - line("@call ", StringUtils.join(" ", command)); + public void command(List command) throws IOException { + lineWithLenCheck("@call ", StringUtils.join(" ", command)); + errorCheck(); } @Override - public void env(String key, String value) { - line("@set ", key, "=", value, - "\nif %errorlevel% neq 0 exit /b %errorlevel%"); + public void env(String key, String value) throws IOException { + lineWithLenCheck("@set ", key, "=", value); + errorCheck(); } @Override @@ -578,16 +604,20 @@ public class ContainerLaunch implements Callable { // 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)); + lineWithLenCheck(String.format("@copy \"%s\" \"%s\"", srcFileStr, dstFileStr)); + errorCheck(); } else { - line(String.format("@%s symlink \"%s\" \"%s\"", Shell.WINUTILS, + lineWithLenCheck(String.format("@%s symlink \"%s\" \"%s\"", Shell.WINUTILS, dstFileStr, srcFileStr)); + errorCheck(); } } @Override - protected void mkdir(Path path) { - line("@if not exist ", path.toString(), " mkdir ", path.toString()); + protected void mkdir(Path path) throws IOException { + lineWithLenCheck(String.format("@if not exist \"%s\" mkdir \"%s\"", + path.toString(), path.toString())); + errorCheck(); } } @@ -730,8 +760,7 @@ public class ContainerLaunch implements Callable { Map environment, Map> resources, List command) throws IOException { - ShellScriptBuilder sb = Shell.WINDOWS ? new WindowsShellScriptBuilder() : - new UnixShellScriptBuilder(); + ShellScriptBuilder sb = ShellScriptBuilder.create(); if (environment != null) { for (Map.Entry env : environment.entrySet()) { sb.env(env.getKey().toString(), env.getValue().toString()); 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 247b5d3ee78..c8fc85a7c7a 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 @@ -19,6 +19,9 @@ package org.apache.hadoop.yarn.server.nodemanager.containermanager.launcher; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; +import static org.junit.matchers.JUnitMatchers.*; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -27,6 +30,7 @@ import java.io.File; import java.io.FileOutputStream; import java.io.FileReader; import java.io.IOException; +import java.io.PrintStream; import java.io.PrintWriter; import java.nio.ByteBuffer; import java.util.ArrayList; @@ -37,7 +41,6 @@ import java.util.List; import java.util.Map; import org.junit.Assert; - import org.apache.commons.codec.binary.Base64; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileUtil; @@ -76,6 +79,7 @@ import org.apache.hadoop.yarn.server.nodemanager.containermanager.BaseContainerM import org.apache.hadoop.yarn.server.nodemanager.containermanager.container.Container; import org.apache.hadoop.yarn.server.nodemanager.containermanager.container.ContainerEventType; import org.apache.hadoop.yarn.server.nodemanager.containermanager.container.ContainerExitEvent; +import org.apache.hadoop.yarn.server.nodemanager.containermanager.launcher.ContainerLaunch.ShellScriptBuilder; import org.apache.hadoop.yarn.server.nodemanager.containermanager.localizer.ContainerLocalizer; import org.apache.hadoop.yarn.server.utils.BuilderUtils; import org.apache.hadoop.yarn.util.Apps; @@ -83,6 +87,7 @@ import org.apache.hadoop.yarn.util.AuxiliaryServiceHelper; import org.apache.hadoop.yarn.util.ConverterUtils; import org.apache.hadoop.yarn.util.LinuxResourceCalculatorPlugin; import org.apache.hadoop.yarn.util.ResourceCalculatorPlugin; +import org.junit.Assume; import org.junit.Before; import org.junit.Test; @@ -743,18 +748,18 @@ public class TestContainerLaunch extends BaseContainerManagerTest { } } - @Test + @Test (timeout = 30000) public void testDelayedKill() throws Exception { internalKillTest(true); } - @Test + @Test (timeout = 30000) public void testImmediateKill() throws Exception { internalKillTest(false); } @SuppressWarnings("rawtypes") - @Test + @Test (timeout = 10000) public void testCallFailureWithNullLocalizedResources() { Container container = mock(Container.class); when(container.getContainerId()).thenReturn(ContainerId.newInstance( @@ -792,4 +797,166 @@ public class TestContainerLaunch extends BaseContainerManagerTest { return containerToken; } + /** + * Test that script exists with non-zero exit code when command fails. + * @throws IOException + */ + @Test (timeout = 10000) + public void testShellScriptBuilderNonZeroExitCode() throws IOException { + ShellScriptBuilder builder = ShellScriptBuilder.create(); + builder.command(Arrays.asList(new String[] {"unknownCommand"})); + File shellFile = Shell.appendScriptExtension(tmpDir, "testShellScriptBuilderError"); + PrintStream writer = new PrintStream(new FileOutputStream(shellFile)); + builder.write(writer); + writer.close(); + try { + FileUtil.setExecutable(shellFile, true); + + Shell.ShellCommandExecutor shexc = new Shell.ShellCommandExecutor( + new String[]{shellFile.getAbsolutePath()}, tmpDir); + try { + shexc.execute(); + fail("builder shell command was expected to throw"); + } + catch(IOException e) { + // expected + System.out.println("Received an expected exception: " + e.getMessage()); + } + } + finally { + FileUtil.fullyDelete(shellFile); + } + } + + private static final String expectedMessage = "The command line has a length of"; + + @Test (timeout = 10000) + public void testWindowsShellScriptBuilderCommand() throws IOException { + String callCmd = "@call "; + + // Test is only relevant on Windows + Assume.assumeTrue(Shell.WINDOWS); + + // The tests are built on assuming 8191 max command line length + assertEquals(8191, Shell.WINDOWS_MAX_SHELL_LENGHT); + + ShellScriptBuilder builder = ShellScriptBuilder.create(); + + // Basic tests: less length, exact length, max+1 length + builder.command(Arrays.asList( + org.apache.commons.lang.StringUtils.repeat("A", 1024))); + builder.command(Arrays.asList( + org.apache.commons.lang.StringUtils.repeat( + "E", Shell.WINDOWS_MAX_SHELL_LENGHT - callCmd.length()))); + try { + builder.command(Arrays.asList( + org.apache.commons.lang.StringUtils.repeat( + "X", Shell.WINDOWS_MAX_SHELL_LENGHT -callCmd.length() + 1))); + fail("longCommand was expected to throw"); + } catch(IOException e) { + assertThat(e.getMessage(), containsString(expectedMessage)); + } + + // Composite tests, from parts: less, exact and + + builder.command(Arrays.asList( + org.apache.commons.lang.StringUtils.repeat("A", 1024), + org.apache.commons.lang.StringUtils.repeat("A", 1024), + org.apache.commons.lang.StringUtils.repeat("A", 1024))); + + // buildr.command joins the command parts with an extra space + builder.command(Arrays.asList( + org.apache.commons.lang.StringUtils.repeat("E", 4095), + org.apache.commons.lang.StringUtils.repeat("E", 2047), + org.apache.commons.lang.StringUtils.repeat("E", 2047 - callCmd.length()))); + + try { + builder.command(Arrays.asList( + org.apache.commons.lang.StringUtils.repeat("X", 4095), + org.apache.commons.lang.StringUtils.repeat("X", 2047), + org.apache.commons.lang.StringUtils.repeat("X", 2048 - callCmd.length()))); + fail("long commands was expected to throw"); + } catch(IOException e) { + assertThat(e.getMessage(), containsString(expectedMessage)); + } + } + + @Test (timeout = 10000) + public void testWindowsShellScriptBuilderEnv() throws IOException { + // Test is only relevant on Windows + Assume.assumeTrue(Shell.WINDOWS); + + // The tests are built on assuming 8191 max command line length + assertEquals(8191, Shell.WINDOWS_MAX_SHELL_LENGHT); + + ShellScriptBuilder builder = ShellScriptBuilder.create(); + + // test env + builder.env("somekey", org.apache.commons.lang.StringUtils.repeat("A", 1024)); + builder.env("somekey", org.apache.commons.lang.StringUtils.repeat( + "A", Shell.WINDOWS_MAX_SHELL_LENGHT - ("@set somekey=").length())); + try { + builder.env("somekey", org.apache.commons.lang.StringUtils.repeat( + "A", Shell.WINDOWS_MAX_SHELL_LENGHT - ("@set somekey=").length()) + 1); + fail("long env was expected to throw"); + } catch(IOException e) { + assertThat(e.getMessage(), containsString(expectedMessage)); + } + } + + @Test (timeout = 10000) + public void testWindowsShellScriptBuilderMkdir() throws IOException { + String mkDirCmd = "@if not exist \"\" mkdir \"\""; + + // Test is only relevant on Windows + Assume.assumeTrue(Shell.WINDOWS); + + // The tests are built on assuming 8191 max command line length + assertEquals(8191, Shell.WINDOWS_MAX_SHELL_LENGHT); + + ShellScriptBuilder builder = ShellScriptBuilder.create(); + + // test mkdir + builder.mkdir(new Path(org.apache.commons.lang.StringUtils.repeat("A", 1024))); + builder.mkdir(new Path(org.apache.commons.lang.StringUtils.repeat( + "E", (Shell.WINDOWS_MAX_SHELL_LENGHT - mkDirCmd.length())/2))); + try { + builder.mkdir(new Path(org.apache.commons.lang.StringUtils.repeat( + "X", (Shell.WINDOWS_MAX_SHELL_LENGHT - mkDirCmd.length())/2 +1))); + fail("long mkdir was expected to throw"); + } catch(IOException e) { + assertThat(e.getMessage(), containsString(expectedMessage)); + } + } + + @Test (timeout = 10000) + public void testWindowsShellScriptBuilderLink() throws IOException { + // Test is only relevant on Windows + Assume.assumeTrue(Shell.WINDOWS); + + String linkCmd = "@" +Shell.WINUTILS + " symlink \"\" \"\""; + + // The tests are built on assuming 8191 max command line length + assertEquals(8191, Shell.WINDOWS_MAX_SHELL_LENGHT); + + ShellScriptBuilder builder = ShellScriptBuilder.create(); + + // test link + builder.link(new Path(org.apache.commons.lang.StringUtils.repeat("A", 1024)), + new Path(org.apache.commons.lang.StringUtils.repeat("B", 1024))); + builder.link( + new Path(org.apache.commons.lang.StringUtils.repeat( + "E", (Shell.WINDOWS_MAX_SHELL_LENGHT - linkCmd.length())/2)), + new Path(org.apache.commons.lang.StringUtils.repeat( + "F", (Shell.WINDOWS_MAX_SHELL_LENGHT - linkCmd.length())/2))); + try { + builder.link( + new Path(org.apache.commons.lang.StringUtils.repeat( + "X", (Shell.WINDOWS_MAX_SHELL_LENGHT - linkCmd.length())/2 + 1)), + new Path(org.apache.commons.lang.StringUtils.repeat( + "Y", (Shell.WINDOWS_MAX_SHELL_LENGHT - linkCmd.length())/2) + 1)); + fail("long link was expected to throw"); + } catch(IOException e) { + assertThat(e.getMessage(), containsString(expectedMessage)); + } + } }