Details

Description

Currently nodemanager depends on bash shell. It should be well documented for system not having bash installed by default such as FreeBSD. Because only basic functionality of bash is used, probably changing bash to /bin/sh would work enough.

2. yarn-hduser-nodemanager-ponto.amerinoc.com.log:2012-04-03 19:50:10,798 INFO org.apache.hadoop.yarn.server.nodemanager.DefaultContainerExecutor: launchContainer: [bash, -c, /tmp/nm-local-dir/usercache/hduser/appcache/application_1333474251533_0002/container_1333474251533_0002_01_000012/default_container_executor.sh]
this created script is also launched by bash - bash anywhere in path works - in freebsd it is /usr/local/bin/bash

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

Hadoop QA
added a comment - 04/Apr/12 16:00 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12521313/bash-replace-by-sh.txt
against trunk revision .
+1 @author. The patch does not contain any @author tags.
-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed these unit tests:
org.apache.hadoop.yarn.server.nodemanager.containermanager.monitor.TestContainersMonitor
org.apache.hadoop.yarn.server.nodemanager.containermanager.TestContainerManager
org.apache.hadoop.yarn.server.nodemanager.containermanager.launcher.TestContainerLaunch
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2146//testReport/
Console output: https://builds.apache.org/job/PreCommit-MAPREDUCE-Build/2146//console
This message is automatically generated.

Security is a beast by itself
Leaving it as sh lets users setup their installations as long as required stuff is in the PATH. Making it /bin/sh forces installation node to have /bin etc. Some of these things may be difficult, say on Windows.

Bikas Saha
added a comment - 05/Apr/12 22:04 Security is a beast by itself
Leaving it as sh lets users setup their installations as long as required stuff is in the PATH. Making it /bin/sh forces installation node to have /bin etc. Some of these things may be difficult, say on Windows.

Radim Kolar
added a comment - 26/Apr/12 08:42 There is already configurable shell feature - MRJobConfig.MAPRED_ADMIN_USER_SHELL it is just not used for anything except putting into environment in TaskAttemptImpl.
You want to use this setting for every container launch?

Do you have a jira for the pluggable process tree? It would sound like a dup of MAPREDUCE-4204.
Can you please wait until it gets merged back into mainline from branch-1-win? And then make the changes you still think are necessary after that refactoring.

Bikas Saha
added a comment - 03/May/12 19:06 Do you have a jira for the pluggable process tree? It would sound like a dup of MAPREDUCE-4204 .
Can you please wait until it gets merged back into mainline from branch-1-win? And then make the changes you still think are necessary after that refactoring.

Sorry for the late reply. I meant using configurations instead of hardcoding. Most of the patch is changing the hardcoded "bash" to "sh". It will be cleaner to have it as a value for a configuration property and have the default set to "sh" for example.

Ahmed Radwan
added a comment - 04/May/12 19:56 Sorry for the late reply. I meant using configurations instead of hardcoding. Most of the patch is changing the hardcoded "bash" to "sh". It will be cleaner to have it as a value for a configuration property and have the default set to "sh" for example.

It should be well documented for system not having bash installed by default such as FreeBSD.

Why don't we simply document requirements then?

I've recently seen /bin/sh shbanged scripts cause trouble on Ubuntu cause /bin/sh points to Ubuntu's dash (https://wiki.ubuntu.com/DashAsBinSh). You don't wanna run into such a trouble and end up changing things (hadoop or OS side) post-deploy.

Harsh J
added a comment - 23/Sep/12 19:00 It should be well documented for system not having bash installed by default such as FreeBSD.
Why don't we simply document requirements then?
I've recently seen /bin/sh shbanged scripts cause trouble on Ubuntu cause /bin/sh points to Ubuntu's dash ( https://wiki.ubuntu.com/DashAsBinSh ). You don't wanna run into such a trouble and end up changing things (hadoop or OS side) post-deploy.
I'll still vote we stick to one shell (bash) and be clear we need it.

Harsh J
added a comment - 23/Sep/12 19:01 Why don't we simply document requirements then?
We can additionally be clear that we demand bash exists in /bin if thats the whole trouble here? Or rely on env bash , but no idea if thats cross platform properly as well.

Radim Kolar
added a comment - 23/Sep/12 19:24 adding /bin/bash as requirement is fine. I do not have plans to fix this patch because i cant even get "mvn test" working on bsd. It dies with some obscure error.