You are viewing a plain text version of this content. The canonical link for it is here.
Posted to yarn-commits@hadoop.apache.org by iv...@apache.org on 2014/04/19 20:55:07 UTC
svn commit: r1588693 - in /hadoop/common/trunk/hadoop-yarn-project: ./
hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/
hadoop-yarn/hadoop-yarn-server/hadoo...
Author: ivanmi
Date: Sat Apr 19 18:55:07 2014
New Revision: 1588693
URL: http://svn.apache.org/r1588693
Log:
YARN-1865. ShellScriptBuilder does not check for some error conditions. Contributed by Remus Rusanu.
Modified:
hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
hadoop/common/trunk/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
hadoop/common/trunk/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
Modified: hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
URL: http://svn.apache.org/viewvc/hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt?rev=1588693&r1=1588692&r2=1588693&view=diff
==============================================================================
--- hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt (original)
+++ hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt Sat Apr 19 18:55:07 2014
@@ -71,6 +71,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
Modified: hadoop/common/trunk/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
URL: http://svn.apache.org/viewvc/hadoop/common/trunk/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?rev=1588693&r1=1588692&r2=1588693&view=diff
==============================================================================
--- hadoop/common/trunk/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 (original)
+++ hadoop/common/trunk/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 Sat Apr 19 18:55:07 2014
@@ -479,15 +479,20 @@ public class ContainerLaunch implements
+ 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<String> command);
+ public abstract void command(List<String> 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
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
@Override
public void command(List<String> command) {
line("exec /bin/bash -c \"", StringUtils.join(" ", command), "\"");
+ errorCheck();
}
@Override
@@ -543,31 +557,43 @@ public class ContainerLaunch implements
@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<String> command) {
- line("@call ", StringUtils.join(" ", command));
+ public void command(List<String> 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
// 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
Map<String,String> environment, Map<Path,List<String>> resources,
List<String> command)
throws IOException {
- ShellScriptBuilder sb = Shell.WINDOWS ? new WindowsShellScriptBuilder() :
- new UnixShellScriptBuilder();
+ ShellScriptBuilder sb = ShellScriptBuilder.create();
if (environment != null) {
for (Map.Entry<String,String> env : environment.entrySet()) {
sb.env(env.getKey().toString(), env.getValue().toString());
Modified: hadoop/common/trunk/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
URL: http://svn.apache.org/viewvc/hadoop/common/trunk/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?rev=1588693&r1=1588692&r2=1588693&view=diff
==============================================================================
--- hadoop/common/trunk/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 (original)
+++ hadoop/common/trunk/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 Sat Apr 19 18:55:07 2014
@@ -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.nod
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.Auxil
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
}
}
- @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
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));
+ }
+ }
}