Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Not a Problem
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: nodemanager
    • Labels:
    • Environment:

      FreeBSD 8.2 / 64 bit

      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.

      i found 2 cases:

      1. DefaultContainerExecutor.java creates file with /bin/bash hardcoded in writeLocalWrapperScript. (this needs bash in /bin)

      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

        Activity

        Hide
        Radim Kolar added a comment -

        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.

        Show
        Radim Kolar added a comment - 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.
        Hide
        Harsh J added a comment -

        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.

        Show
        Harsh J added a comment - 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.
        Hide
        Harsh J added a comment -

        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.

        Show
        Harsh J added a comment - 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.
        Hide
        Radim Kolar added a comment -

        patch is probably wrong

        Show
        Radim Kolar added a comment - patch is probably wrong
        Hide
        Ahmed Radwan added a comment -

        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.

        Show
        Ahmed Radwan added a comment - 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.
        Hide
        Bikas Saha added a comment -

        I assume you mean to do that when that patch makes its way into trunk from branch-1-win. Otherwise merging branch-1-win into trunk will create a mess.

        Show
        Bikas Saha added a comment - I assume you mean to do that when that patch makes its way into trunk from branch-1-win. Otherwise merging branch-1-win into trunk will create a mess.
        Hide
        Radim Kolar added a comment -

        i had on plan to rework your patch into trunk version with my improvements as i stated in jira.

        Show
        Radim Kolar added a comment - i had on plan to rework your patch into trunk version with my improvements as i stated in jira.
        Hide
        Bikas Saha added a comment -

        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.

        Show
        Bikas Saha added a comment - 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.
        Hide
        Radim Kolar added a comment -

        I will rework this patch but first i need to work on pluggable process tree

        Show
        Radim Kolar added a comment - I will rework this patch but first i need to work on pluggable process tree
        Hide
        Radim Kolar added a comment -

        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?

        Show
        Radim Kolar added a comment - 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?
        Hide
        Radim Kolar added a comment -

        You want configurable shell? I think it is overkill.

        Show
        Radim Kolar added a comment - You want configurable shell? I think it is overkill.
        Hide
        Ahmed Radwan added a comment -

        It think it is cleaner to avoid hardcoding the commands and have it configurable with the default in *-default.xml.

        Show
        Ahmed Radwan added a comment - It think it is cleaner to avoid hardcoding the commands and have it configurable with the default in *-default.xml.
        Hide
        Allen Wittenauer added a comment -

        Given that other parts of the system use $

        {BASH_SOURCE}

        , is it actually worth the effort to change this?

        Show
        Allen Wittenauer added a comment - Given that other parts of the system use $ {BASH_SOURCE} , is it actually worth the effort to change this?
        Hide
        Radim Kolar added a comment -

        I am not sure if shell script can be launched without #! /abs path.

        Show
        Radim Kolar added a comment - I am not sure if shell script can be launched without #! /abs path.
        Hide
        Bikas Saha added a comment -

        so we should work on fixing it and not adding new ones, right?

        Show
        Bikas Saha added a comment - so we should work on fixing it and not adding new ones, right?
        Hide
        Radim Kolar added a comment -

        Nodemanager is creating script with #!/bin/bash, so every node has to have /bin

        Show
        Radim Kolar added a comment - Nodemanager is creating script with #!/bin/bash, so every node has to have /bin
        Hide
        Bikas Saha added a comment -

        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.

        Show
        Bikas Saha added a comment - 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.
        Hide
        Radim Kolar added a comment -

        /bin/sh is always better then plain sh for security purposes

        Show
        Radim Kolar added a comment - /bin/sh is always better then plain sh for security purposes
        Hide
        Bikas Saha added a comment -

        Could you check if the test failure is related to your changes?

        Comments

        +++ b/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/TaskLog.java
        @@ -379,7 +379,7 @@ public class TaskLog {
        }
        }

        • private static final String bashCommand = "bash";
          + private static final String bashCommand = "sh";

        Now you might want to rename this to shellCommand.

        +++ b/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/DefaultContainerExecutor.java
        @@ -169,7 +169,7 @@ public class DefaultContainerExecutor extends ContainerExecutor {
        ContainerExecutor.TASK_LAUNCH_SCRIPT_PERMISSION);

        // Setup command to run

        • String[] command = {"bash", "-c",
          + String[] command = {"/bin/sh", "-c",

        Why the extra /bin/ ?

        Show
        Bikas Saha added a comment - Could you check if the test failure is related to your changes? Comments +++ b/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/TaskLog.java @@ -379,7 +379,7 @@ public class TaskLog { } } private static final String bashCommand = "bash"; + private static final String bashCommand = "sh"; Now you might want to rename this to shellCommand. +++ b/hadoop-mapreduce-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/DefaultContainerExecutor.java @@ -169,7 +169,7 @@ public class DefaultContainerExecutor extends ContainerExecutor { ContainerExecutor.TASK_LAUNCH_SCRIPT_PERMISSION); // Setup command to run String[] command = {"bash", "-c", + String[] command = {"/bin/sh", "-c", Why the extra /bin/ ?
        Hide
        Hadoop QA added a comment -

        -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.

        Show
        Hadoop QA added a comment - -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.
        Hide
        Radim Kolar added a comment -

        patch is against trunk

        Show
        Radim Kolar added a comment - patch is against trunk
        Hide
        Radim Kolar added a comment -

        Replace Bash by standard /bin/sh. Currently used functionality is bash -c "command" which is supported by standard sh too

        Show
        Radim Kolar added a comment - Replace Bash by standard /bin/sh. Currently used functionality is bash -c "command" which is supported by standard sh too
        Hide
        Bikas Saha added a comment -

        I agree on using sh.
        It would be great if you could post a patch to fix it. Thanks!

        Show
        Bikas Saha added a comment - I agree on using sh. It would be great if you could post a patch to fix it. Thanks!

          People

          • Assignee:
            Unassigned
            Reporter:
            Radim Kolar
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development