HBase
  1. HBase
  2. HBASE-7814

Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.6
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    1. HBASE-7814-v0.patch
      1 kB
      Sergey Shelukhin
    2. 7814-v2.txt
      1 kB
      Ted Yu
    3. 7814-v3.txt
      1 kB
      Ted Yu
    4. 7814-v4.txt
      2 kB
      Ted Yu

      Issue Links

        Activity

        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security-on-Hadoop-23 #12 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/12/)
        HBASE-7814 Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94 (Revision 1445405)
        HBASE-7814 revert due to hadoop 0.20 compatibility concern (Revision 1445004)
        HBASE-7814 Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94 (Sergey) (Revision 1444978)

        Result = FAILURE
        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java

        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java

        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security-on-Hadoop-23 #12 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/12/ ) HBASE-7814 Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94 (Revision 1445405) HBASE-7814 revert due to hadoop 0.20 compatibility concern (Revision 1445004) HBASE-7814 Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94 (Sergey) (Revision 1444978) Result = FAILURE tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        Hide
        Gary Helmling added a comment -

        IS_SECURE_HADOOP is set only when we can call the isSecurityEnabled() method (it is not security enabled, but rather whether we are using a Hadoop version which has security API). My understanding is that if underlying hadoop does not support security, we will default to using HadoopUser.

        Yes, the User class was added to abstract out the UserGroupInformation API changes between 0.20.x Hadoop (pre-security) and 0.20.203+ (with security). It should be used in cases like this.

        Show
        Gary Helmling added a comment - IS_SECURE_HADOOP is set only when we can call the isSecurityEnabled() method (it is not security enabled, but rather whether we are using a Hadoop version which has security API). My understanding is that if underlying hadoop does not support security, we will default to using HadoopUser. Yes, the User class was added to abstract out the UserGroupInformation API changes between 0.20.x Hadoop (pre-security) and 0.20.203+ (with security). It should be used in cases like this.
        Hide
        Enis Soztutar added a comment -

        This will fail if getShortUserName does not exist

        From my understanding, we will only call getShortName() when we are using SecureHadoopUser, instead of HadoopUser. See getCurrent():

          /**
           * Returns the {@code User} instance within current execution context.
           */
          public static User getCurrent() throws IOException {
            User user;
            if (IS_SECURE_HADOOP) {
              user = new SecureHadoopUser();
            } else {
              user = new HadoopUser();
            }
            if (user.getUGI() == null) {
              return null;
            }
            return user;
          }
        

        SecureHadoopUser is a misnomer. IS_SECURE_HADOOP is set only when we can call the isSecurityEnabled() method (it is not security enabled, but rather whether we are using a Hadoop version which has security API). My understanding is that if underlying hadoop does not support security, we will default to using HadoopUser.

          /**
           * Flag to differentiate between API-incompatible changes to
           * {@link org.apache.hadoop.security.UserGroupInformation} between vanilla
           * Hadoop 0.20.x and secure Hadoop 0.20+.
           */
          private static boolean IS_SECURE_HADOOP = true;
          static {
            try {
              UserGroupInformation.class.getMethod("isSecurityEnabled");
            } catch (NoSuchMethodException nsme) {
              IS_SECURE_HADOOP = false;
            }
          }
        
        Show
        Enis Soztutar added a comment - This will fail if getShortUserName does not exist From my understanding, we will only call getShortName() when we are using SecureHadoopUser, instead of HadoopUser. See getCurrent(): /** * Returns the {@code User} instance within current execution context. */ public static User getCurrent() throws IOException { User user; if (IS_SECURE_HADOOP) { user = new SecureHadoopUser(); } else { user = new HadoopUser(); } if (user.getUGI() == null ) { return null ; } return user; } SecureHadoopUser is a misnomer. IS_SECURE_HADOOP is set only when we can call the isSecurityEnabled() method (it is not security enabled, but rather whether we are using a Hadoop version which has security API). My understanding is that if underlying hadoop does not support security, we will default to using HadoopUser. /** * Flag to differentiate between API-incompatible changes to * {@link org.apache.hadoop.security.UserGroupInformation} between vanilla * Hadoop 0.20.x and secure Hadoop 0.20+. */ private static boolean IS_SECURE_HADOOP = true ; static { try { UserGroupInformation.class.getMethod( "isSecurityEnabled" ); } catch (NoSuchMethodException nsme) { IS_SECURE_HADOOP = false ; } }
        Hide
        Enis Soztutar added a comment -

        Ok, created HBASE-7832 to proceed.

        Show
        Enis Soztutar added a comment - Ok, created HBASE-7832 to proceed.
        Hide
        Lars Hofhansl added a comment -

        This will fail if getShortUserName does not exist, no?

        shortName = (String)call(ugi, "getShortUserName", null, null);
        

        Maybe I'm misreading the code.

        Show
        Lars Hofhansl added a comment - This will fail if getShortUserName does not exist, no? shortName = ( String )call(ugi, "getShortUserName" , null , null ); Maybe I'm misreading the code.
        Hide
        Enis Soztutar added a comment -

        Why do you think it will fail with exception. This is HadoopUser:

        public String getShortName() {
              return ugi != null ? ugi.getUserName() : null;
            }
        

        and this is SecureHadoopUser:

            public String getShortName() {
              if (shortName != null) return shortName;
              try {
                shortName = (String)call(ugi, "getShortUserName", null, null);
              ...
        
        Show
        Enis Soztutar added a comment - Why do you think it will fail with exception. This is HadoopUser: public String getShortName() { return ugi != null ? ugi.getUserName() : null ; } and this is SecureHadoopUser: public String getShortName() { if (shortName != null ) return shortName; try { shortName = ( String )call(ugi, "getShortUserName" , null , null ); ...
        Hide
        Lars Hofhansl added a comment -

        Well... Only that that call would fail with an exception if that method does not exist.

        Show
        Lars Hofhansl added a comment - Well... Only that that call would fail with an exception if that method does not exist.
        Hide
        Lars Hofhansl added a comment -

        Oh. Good call, Enis Soztutar.
        Ted Yu This is the User in the hbase package.

        Show
        Lars Hofhansl added a comment - Oh. Good call, Enis Soztutar . Ted Yu This is the User in the hbase package.
        Hide
        Ted Yu added a comment -

        I checked src/core/org/apache/hadoop/security/User.java under 0.20 hadoop

        This method is not there.

        Show
        Ted Yu added a comment - I checked src/core/org/apache/hadoop/security/User.java under 0.20 hadoop This method is not there.
        Hide
        Enis Soztutar added a comment -

        Sorry to come in late, but doesn't o.a.h.hbase.User.getShortName() solves your problem, instead of this exception catching. Shall I create a new issue to replace that?

        Show
        Enis Soztutar added a comment - Sorry to come in late, but doesn't o.a.h.hbase.User.getShortName() solves your problem, instead of this exception catching. Shall I create a new issue to replace that?
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security #110 (See https://builds.apache.org/job/HBase-0.94-security/110/)
        HBASE-7814 Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94 (Revision 1445405)
        HBASE-7814 revert due to hadoop 0.20 compatibility concern (Revision 1445004)
        HBASE-7814 Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94 (Sergey) (Revision 1444978)

        Result = SUCCESS
        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java

        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java

        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security #110 (See https://builds.apache.org/job/HBase-0.94-security/110/ ) HBASE-7814 Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94 (Revision 1445405) HBASE-7814 revert due to hadoop 0.20 compatibility concern (Revision 1445004) HBASE-7814 Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94 (Sergey) (Revision 1444978) Result = SUCCESS tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #840 (See https://builds.apache.org/job/HBase-0.94/840/)
        HBASE-7814 Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94 (Revision 1445405)

        Result = SUCCESS
        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #840 (See https://builds.apache.org/job/HBase-0.94/840/ ) HBASE-7814 Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94 (Revision 1445405) Result = SUCCESS tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        Hide
        Ted Yu added a comment -

        Integrated to 0.94

        Thanks for the reviews, Jon, Sergey and Lars.

        Show
        Ted Yu added a comment - Integrated to 0.94 Thanks for the reviews, Jon, Sergey and Lars.
        Hide
        Lars Hofhansl added a comment -

        +1

        (Test with older version would be nice, but the change is obvious enough that I would not make that a requirement.)

        Show
        Lars Hofhansl added a comment - +1 (Test with older version would be nice, but the change is obvious enough that I would not make that a requirement.)
        Hide
        Ted Yu added a comment -

        I looked through 0.92 pom.xml and found no profile for hadoop 0.20.x
        I ran the following command with patch and it passed:

        mvn clean test -PlocalTests,security -Dtest=TestHBaseFsck

        Lars Hofhansl:
        Do you want to take a look at patch v4 ?

        Show
        Ted Yu added a comment - I looked through 0.92 pom.xml and found no profile for hadoop 0.20.x I ran the following command with patch and it passed: mvn clean test -PlocalTests,security -Dtest=TestHBaseFsck Lars Hofhansl : Do you want to take a look at patch v4 ?
        Hide
        Jonathan Hsieh added a comment -

        lgtm. ideally this is tested before commit (tweak the pom to use an older hadoop jar?).

        Show
        Jonathan Hsieh added a comment - lgtm. ideally this is tested before commit (tweak the pom to use an older hadoop jar?).
        Hide
        Ted Yu added a comment -

        Patch v4 deals with individual exceptions.

        Show
        Ted Yu added a comment - Patch v4 deals with individual exceptions.
        Hide
        Jonathan Hsieh added a comment -

        I agree with Sergey. The v3 invoke is better. Let's get the specific exception as well.

        Show
        Jonathan Hsieh added a comment - I agree with Sergey. The v3 invoke is better. Let's get the specific exception as well.
        Hide
        Ted Yu added a comment -

        Patch v3 invokes the method from Method object obtained through reflection.

        Show
        Ted Yu added a comment - Patch v3 invokes the method from Method object obtained through reflection.
        Hide
        Sergey Shelukhin added a comment -

        Why not invoke the method via reflection instead to avoid potential compilation issues? Doesn't appear like perf would matter here.
        Also, what if getShortUserName throws some logical exception, +1 to specific exception, or moving the call out of this try-catch

        Show
        Sergey Shelukhin added a comment - Why not invoke the method via reflection instead to avoid potential compilation issues? Doesn't appear like perf would matter here. Also, what if getShortUserName throws some logical exception, +1 to specific exception, or moving the call out of this try-catch
        Hide
        Ted Yu added a comment -

        I followed the same style in existing code:

          private static boolean isInSafeMode(DistributedFileSystem dfs) throws IOException {
            boolean inSafeMode = false;
            try {
              Method m = DistributedFileSystem.class.getMethod("setSafeMode", new Class<?> []{
                  org.apache.hadoop.hdfs.protocol.FSConstants.SafeModeAction.class, boolean.class});
              inSafeMode = (Boolean) m.invoke(dfs,
                org.apache.hadoop.hdfs.protocol.FSConstants.SafeModeAction.SAFEMODE_GET, true);
            } catch (Exception e) {
        
        Show
        Ted Yu added a comment - I followed the same style in existing code: private static boolean isInSafeMode(DistributedFileSystem dfs) throws IOException { boolean inSafeMode = false ; try { Method m = DistributedFileSystem.class.getMethod( "setSafeMode" , new Class <?> []{ org.apache.hadoop.hdfs.protocol.FSConstants.SafeModeAction.class, boolean .class}); inSafeMode = ( Boolean ) m.invoke(dfs, org.apache.hadoop.hdfs.protocol.FSConstants.SafeModeAction.SAFEMODE_GET, true ); } catch (Exception e) {
        Hide
        Jonathan Hsieh added a comment -

        So this requires means this won't compile unless we compile against hadoop 1.0+/.22+ but the binary should work against the older hadoop right? I think I'm ok with that but please add comment and release note about this.

        +    // See HBASE-7814. UserGroupInformation from hadoop 0.20.x may not support getShortUserName().
        +    String username;
        +    try {
        +      UserGroupInformation.class.getMethod("getShortUserName", new Class<?>[]{});
        +      username = ugi.getShortUserName();
        +    } catch (Exception e) {
        

        Can we be more specific about the Exception – maybe figure out what the specific reflection exception is and use it?

        Show
        Jonathan Hsieh added a comment - So this requires means this won't compile unless we compile against hadoop 1.0+/.22+ but the binary should work against the older hadoop right? I think I'm ok with that but please add comment and release note about this. + // See HBASE-7814. UserGroupInformation from hadoop 0.20.x may not support getShortUserName(). + String username; + try { + UserGroupInformation.class.getMethod( "getShortUserName" , new Class <?>[]{}); + username = ugi.getShortUserName(); + } catch (Exception e) { Can we be more specific about the Exception – maybe figure out what the specific reflection exception is and use it?
        Hide
        Ted Yu added a comment -

        Patch v2 uses reflection to see if ugi.getShortUserName() is supported.

        checkAccess() is only called by hbck at the beginning of the execution. So performance impact is negligible.

        Show
        Ted Yu added a comment - Patch v2 uses reflection to see if ugi.getShortUserName() is supported. checkAccess() is only called by hbck at the beginning of the execution. So performance impact is negligible.
        Hide
        Jonathan Hsieh added a comment -

        I'm fine if this is done in a compatible way (and the gymnastics are commented if this is the case).

        Show
        Jonathan Hsieh added a comment - I'm fine if this is done in a compatible way (and the gymnastics are commented if this is the case).
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #837 (See https://builds.apache.org/job/HBase-0.94/837/)
        HBASE-7814 revert due to hadoop 0.20 compatibility concern (Revision 1445004)

        Result = SUCCESS
        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #837 (See https://builds.apache.org/job/HBase-0.94/837/ ) HBASE-7814 revert due to hadoop 0.20 compatibility concern (Revision 1445004) Result = SUCCESS tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        Hide
        Lars Hofhansl added a comment -

        OK... Reopened for 0.94.6 to think about it.

        Show
        Lars Hofhansl added a comment - OK... Reopened for 0.94.6 to think about it.
        Hide
        Ted Yu added a comment -

        I agree.
        We can use reflection to detect whether the new API is supported

        Show
        Ted Yu added a comment - I agree. We can use reflection to detect whether the new API is supported
        Hide
        Lars Hofhansl added a comment -

        Being able to run hbck on secure cluster seems pretty important, though.

        Show
        Lars Hofhansl added a comment - Being able to run hbck on secure cluster seems pretty important, though.
        Hide
        Jimmy Xiang added a comment -

        Oh, I remember now why it is not checked in 0.94 before. I remember the Hadoop API is in hadoop 0.22 as well, not sure about 0.20.

        Show
        Jimmy Xiang added a comment - Oh, I remember now why it is not checked in 0.94 before. I remember the Hadoop API is in hadoop 0.22 as well, not sure about 0.20.
        Hide
        Ted Yu added a comment -

        I didn't find the method in hadoop branch-0.20

        Reverted from 0.94 branch.

        Thanks for the reminder, Jon.

        Show
        Ted Yu added a comment - I didn't find the method in hadoop branch-0.20 Reverted from 0.94 branch. Thanks for the reminder, Jon.
        Hide
        Lars Hofhansl added a comment -

        I agree... I'll revert the patch. This is 0.94.6, so we can just do that. Unless there are objections I'll do that.

        Show
        Lars Hofhansl added a comment - I agree... I'll revert the patch. This is 0.94.6, so we can just do that. Unless there are objections I'll do that.
        Hide
        Jean-Marc Spaggiari added a comment -

        My opinion: I don't think such constraints should be introduced in a minor release like this one...

        Show
        Jean-Marc Spaggiari added a comment - My opinion: I don't think such constraints should be introduced in a minor release like this one...
        Hide
        Jonathan Hsieh added a comment -

        There was a concern on the original HBASE-6963 that this uses a Hadoop API that isn't available until hadoop 1.0. Are we ok with requiring Hadoop 1.0 for 0.94 releases? I thought that 0.96 was supposed to be the first only hadoop 1.0 requiring HBase.

        If Lars Hofhansl is ok with this being a new constraint, we should at the least release note this.

        Show
        Jonathan Hsieh added a comment - There was a concern on the original HBASE-6963 that this uses a Hadoop API that isn't available until hadoop 1.0. Are we ok with requiring Hadoop 1.0 for 0.94 releases? I thought that 0.96 was supposed to be the first only hadoop 1.0 requiring HBase. If Lars Hofhansl is ok with this being a new constraint, we should at the least release note this.
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #836 (See https://builds.apache.org/job/HBase-0.94/836/)
        HBASE-7814 Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94 (Sergey) (Revision 1444978)

        Result = SUCCESS
        tedyu :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #836 (See https://builds.apache.org/job/HBase-0.94/836/ ) HBASE-7814 Port HBASE-6963 'unable to run hbck on a secure cluster' to 0.94 (Sergey) (Revision 1444978) Result = SUCCESS tedyu : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
        Hide
        Ted Yu added a comment -

        Integrated to 0.94

        Thanks for the patch, Sergey.

        Thanks for the review, Jimmy.

        Show
        Ted Yu added a comment - Integrated to 0.94 Thanks for the patch, Sergey. Thanks for the review, Jimmy.
        Hide
        Jimmy Xiang added a comment -

        +1

        Show
        Jimmy Xiang added a comment - +1
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12568876/HBASE-7814-v0.patch
        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 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/4416//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/12568876/HBASE-7814-v0.patch 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 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/4416//console This message is automatically generated.
        Hide
        Sergey Shelukhin added a comment -

        Automatic port; I am running tests

        Show
        Sergey Shelukhin added a comment - Automatic port; I am running tests

          People

          • Assignee:
            Ted Yu
            Reporter:
            Ted Yu
          • Votes:
            0 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development