Hadoop Common
  1. Hadoop Common
  2. HADOOP-7089

Fix link resolution logic in hadoop-config.sh

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.22.0, 0.23.0
    • Component/s: scripts
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Updates hadoop-config.sh to always resolve symlinks when determining HADOOP_HOME. Bash built-ins or POSIX:2001 compliant cmds are now required.

      Description

      The link resolution logic in bin/hadoop-config.sh fails when when executed via a symlink, from the root directory. We can replace this logic with cd -P and pwd -P, which should be portable across Linux, Solaris, BSD, and OSX.

      1. hadoop-7089-3.patch
        1 kB
        Eli Collins
      2. hadoop-7089-2.patch
        1.0 kB
        Eli Collins
      3. hadoop-7089-1.patch
        1 kB
        Eli Collins
      4. hadoop-7089-1.patch
        3 kB
        Eli Collins

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #479 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/479/)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #479 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/479/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #576 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/576/)
          HADOOP-7089. Fix link resolution logic in hadoop-config.sh. Contributed by Eli Collins

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #576 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/576/ ) HADOOP-7089 . Fix link resolution logic in hadoop-config.sh. Contributed by Eli Collins
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-22-branch #15 (See https://hudson.apache.org/hudson/job/Hadoop-Common-22-branch/15/)
          HADOOP-7089. svn merge -c 1058881 from trunk

          Show
          Hudson added a comment - Integrated in Hadoop-Common-22-branch #15 (See https://hudson.apache.org/hudson/job/Hadoop-Common-22-branch/15/ ) HADOOP-7089 . svn merge -c 1058881 from trunk
          Hide
          Eli Collins added a comment -

          Committed to trunk and branch 22. Thanks Allen.

          Show
          Eli Collins added a comment - Committed to trunk and branch 22. Thanks Allen.
          Hide
          Eli Collins added a comment -

          Per Allen's suggestion, updated the patch to add a comment indicating -P requires bash built-ins or POSIX:2001 cd and pwd.

          Show
          Eli Collins added a comment - Per Allen's suggestion, updated the patch to add a comment indicating -P requires bash built-ins or POSIX:2001 cd and pwd.
          Hide
          Eli Collins added a comment -

          Thanks for the review Allen.

          According to the following Posix includes -P for cd and pwd so I think this change will work even on systems w/o bash built ins. On my linux host both the built-in and pwd binary support -P.

          IEEE Std 1003.1, 2004 Edition
          http://www.opengroup.org/onlinepubs/000095399/utilities/cd.html
          http://www.opengroup.org/onlinepubs/000095399/utilities/pwd.html

          Show
          Eli Collins added a comment - Thanks for the review Allen. According to the following Posix includes -P for cd and pwd so I think this change will work even on systems w/o bash built ins. On my linux host both the built-in and pwd binary support -P. IEEE Std 1003.1, 2004 Edition http://www.opengroup.org/onlinepubs/000095399/utilities/cd.html http://www.opengroup.org/onlinepubs/000095399/utilities/pwd.html
          Hide
          Allen Wittenauer added a comment -

          Eli asked me to double check it. This is a change in behavior, but I think it will be safe as well as portable so long as the bash built-ins are used. It might be worthwhile adding that in the comments that we're depending upon the built-ins in case someone does something like port Hadoop to busybox. [Think appliance.]

          So, +1 if some changes are made to the comments.

          Show
          Allen Wittenauer added a comment - Eli asked me to double check it. This is a change in behavior, but I think it will be safe as well as portable so long as the bash built-ins are used. It might be worthwhile adding that in the comments that we're depending upon the built-ins in case someone does something like port Hadoop to busybox. [Think appliance.] So, +1 if some changes are made to the comments.
          Hide
          Eli Collins added a comment -

          Some extra explanation of the rationale: the current code only resolves the final symlink in the path, ie only starts if the file that sources hadoop-config.sh is a symlink, where the patch here unconditionally resolves all the symlinks. So the current code sometimes does, and sometimes does not, fully resolve links in the path to HADOOP_HOME. Therefore the current code should be agnostic as to whether you fully resolve HADOOP_HOME (ie it should be OK to always resolve the symlinks).

          I think it makes sense to resolve links (and this probably was the intent of the original symlink resolution code) as that way the logic is uniform, ie the way HADOOP_HOME is determined is the same regardless of whether hadoop-config.sh is sourced from a symlink.

          Show
          Eli Collins added a comment - Some extra explanation of the rationale: the current code only resolves the final symlink in the path, ie only starts if the file that sources hadoop-config.sh is a symlink , where the patch here unconditionally resolves all the symlinks. So the current code sometimes does, and sometimes does not, fully resolve links in the path to HADOOP_HOME. Therefore the current code should be agnostic as to whether you fully resolve HADOOP_HOME (ie it should be OK to always resolve the symlinks). I think it makes sense to resolve links (and this probably was the intent of the original symlink resolution code) as that way the logic is uniform, ie the way HADOOP_HOME is determined is the same regardless of whether hadoop-config.sh is sourced from a symlink.
          Hide
          Eli Collins added a comment -

          Patch attached. Tested running from / via a symlink works and that setting HADOOP_CORE_HOME from HDFS/MR root can start the daemons.

          Show
          Eli Collins added a comment - Patch attached. Tested running from / via a symlink works and that setting HADOOP_CORE_HOME from HDFS/MR root can start the daemons.
          Hide
          Eli Collins added a comment -

          Yea, looks like readlink isn't going to work portably. I think Allen's suggestion of using pwd -P is most portable, should work on Linux, BSD, OSX and Solaris, and is much cleaner than the current code. I'll update the patch.

          The motivation is that there's a bug in the manual symlink resolution code, it fails when the script is called using a symlink from /, which is how Debian runs init scripts.

          / $ ls -al /home/eli/test2
          total 56
          drwxr-xr-x   2 eli eli  4096 2011-01-07 10:07 .
          drwxr-xr-x 136 eli eli 49152 2011-01-07 10:07 ..
          lrwxrwxrwx   1 eli eli    17 2011-01-07 09:58 testl1.sh -> ../test1/test1.sh
          lrwxrwxrwx   1 eli eli    17 2011-01-07 10:00 testl2.sh -> ../test1/test2.sh
          / $ cat /home/eli/test1/test1.sh 
          #!/bin/bash
          set -e
          this="${BASH_SOURCE-$0}"
          while [ -h "$this" ]; do
            ls=`ls -ld "$this"`
            link=`expr "$ls" : '.*-> \(.*\)$'`
            if expr "$link" : '.*/.*' > /dev/null; then
              this="$link"
            else
              this=`dirname "$this"`/"$link"
            fi
          done
          bin=`dirname "$this"`
          script=`basename "$this"`
          bin=`cd "$bin"; pwd`
          this="$bin/$script"
          echo $this
          

          Called w/o a symlink:

          / $ /home/eli/test1/test1.sh
          /home/eli/test1/test1.sh
          

          Called via a symlink:

          / $ /home/eli/test2/testl1.sh 
          /home/eli/test2/testl1.sh: line 15: cd: ../test1: No such file or directory
          //test1.sh
          

          Re-written to use pwd -P

          / $ cat /home/eli/test2/testl2.sh 
          #!/bin/bash
          bin=$(cd -P -- "$(dirname -- "${BASH_SOURCE-$0}")" && pwd -P)
          script="$(basename -- "${BASH_SOURCE-$0}")"
          this="$bin/$script"
          echo $this
          

          Called via a symlink:

          / $ /home/eli/test2/testl2.sh 
          /home/eli/test2/testl2.sh
          
          Show
          Eli Collins added a comment - Yea, looks like readlink isn't going to work portably. I think Allen's suggestion of using pwd -P is most portable, should work on Linux, BSD, OSX and Solaris, and is much cleaner than the current code. I'll update the patch. The motivation is that there's a bug in the manual symlink resolution code, it fails when the script is called using a symlink from /, which is how Debian runs init scripts. / $ ls -al /home/eli/test2 total 56 drwxr-xr-x 2 eli eli 4096 2011-01-07 10:07 . drwxr-xr-x 136 eli eli 49152 2011-01-07 10:07 .. lrwxrwxrwx 1 eli eli 17 2011-01-07 09:58 testl1.sh -> ../test1/test1.sh lrwxrwxrwx 1 eli eli 17 2011-01-07 10:00 testl2.sh -> ../test1/test2.sh / $ cat /home/eli/test1/test1.sh #!/bin/bash set -e this="${BASH_SOURCE-$0}" while [ -h "$this" ]; do ls=`ls -ld "$this"` link=`expr "$ls" : '.*-> \(.*\)$'` if expr "$link" : '.*/.*' > /dev/null; then this="$link" else this=`dirname "$this"`/"$link" fi done bin=`dirname "$this"` script=`basename "$this"` bin=`cd "$bin"; pwd` this="$bin/$script" echo $this Called w/o a symlink: / $ /home/eli/test1/test1.sh /home/eli/test1/test1.sh Called via a symlink: / $ /home/eli/test2/testl1.sh /home/eli/test2/testl1.sh: line 15: cd: ../test1: No such file or directory //test1.sh Re-written to use pwd -P / $ cat /home/eli/test2/testl2.sh #!/bin/bash bin=$(cd -P -- "$(dirname -- "${BASH_SOURCE-$0}")" && pwd -P) script="$(basename -- "${BASH_SOURCE-$0}")" this="$bin/$script" echo $this Called via a symlink: / $ /home/eli/test2/testl2.sh /home/eli/test2/testl2.sh
          Hide
          Allen Wittenauer added a comment -

          readlink is definitely tricky from a portability perspective; it is fairly recent as far as the commercial OSes go. [I think it was added in AIX 7, but maybe Stephen or someone else can confirm since I no longer have access to an AIX machine. On Solaris, GNU readlink was added in either 10 or NV, IIRC.]

          Also, with the change to use BASH_SOURCE, is cd+pwd still super fragile? It may not get it 100% correct, but does it get it 'correct enough'?

          Another option might be to use file -h, but I'm not certain how portable that is either. Since we require bash, there is also the (built-in only!) pwd -P.

          Show
          Allen Wittenauer added a comment - readlink is definitely tricky from a portability perspective; it is fairly recent as far as the commercial OSes go. [I think it was added in AIX 7, but maybe Stephen or someone else can confirm since I no longer have access to an AIX machine. On Solaris, GNU readlink was added in either 10 or NV, IIRC.] Also, with the change to use BASH_SOURCE, is cd+pwd still super fragile? It may not get it 100% correct, but does it get it 'correct enough'? Another option might be to use file -h, but I'm not certain how portable that is either. Since we require bash, there is also the (built-in only!) pwd -P.
          Hide
          Greg Roelofs added a comment -

          ...and looks like older versions of GNU readlink don't accept the -m option. This is the --help output from version 5.2.1:

          -f, --canonicalize canonicalize by following every symlink in every
          component of the given path recursively
          -n, --no-newline do not output the trailing newline
          -q, --quiet,
          -s, --silent suppress most error messages
          -v, --verbose report error messages
          --help display this help and exit
          --version output version information and exit

          Show
          Greg Roelofs added a comment - ...and looks like older versions of GNU readlink don't accept the -m option. This is the --help output from version 5.2.1: -f, --canonicalize canonicalize by following every symlink in every component of the given path recursively -n, --no-newline do not output the trailing newline -q, --quiet, -s, --silent suppress most error messages -v, --verbose report error messages --help display this help and exit --version output version information and exit
          Hide
          Greg Roelofs added a comment -

          Not even all Linux versions use GNU readlink. The (BSD-derived?) version accepts no options.

          Show
          Greg Roelofs added a comment - Not even all Linux versions use GNU readlink. The (BSD-derived?) version accepts no options.
          Hide
          Eli Collins added a comment -

          Does this work on OSX, BSD, and Solaris? I seem to remember this being proposed before, but killed for compatibility reasons.

          I checked that -m and -n were supported on Solaris. Looks like from ZOOKEEPER-303 though that OSX doesn't have this option, think the same workaround is acceptable here?

          Show
          Eli Collins added a comment - Does this work on OSX, BSD, and Solaris? I seem to remember this being proposed before, but killed for compatibility reasons. I checked that -m and -n were supported on Solaris. Looks like from ZOOKEEPER-303 though that OSX doesn't have this option, think the same workaround is acceptable here?
          Hide
          Eli Collins added a comment -

          Right patch this time.

          Show
          Eli Collins added a comment - Right patch this time.
          Hide
          Todd Lipcon added a comment -

          Does this work on OSX, BSD, and Solaris? I seem to remember this being proposed before, but killed for compatibility reasons.

          Show
          Todd Lipcon added a comment - Does this work on OSX, BSD, and Solaris? I seem to remember this being proposed before, but killed for compatibility reasons.
          Hide
          Eli Collins added a comment -

          Patch attached. Tested running a daemon out of the bin dir w/ absolute and relative paths.

          Show
          Eli Collins added a comment - Patch attached. Tested running a daemon out of the bin dir w/ absolute and relative paths.

            People

            • Assignee:
              Eli Collins
              Reporter:
              Eli Collins
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development