Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-3331

setBalancerBandwidth do not checkSuperuserPrivilege

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.23.3, 2.0.2-alpha
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      • setBalancerBandwidth from HDFS-2202 should checkSuperuserPrivilege
      • finalizeUpgrade should acquire the write lock.
      1. h3331_20120427.patch
        3 kB
        Tsz Wo Nicholas Sze
      2. h3331_20120427_0.23.patch
        3 kB
        Tsz Wo Nicholas Sze
      3. h3331_20120426.patch
        2 kB
        Tsz Wo Nicholas Sze

        Issue Links

          Activity

          Hide
          Tsz Wo Nicholas Sze added a comment -

          h3331_20120426.patch: 1st patch.

          Show
          Tsz Wo Nicholas Sze added a comment - h3331_20120426.patch: 1st patch.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12524795/h3331_20120426.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 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 passed unit tests in .

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2341//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2341//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/12524795/h3331_20120426.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 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 passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2341//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2341//console This message is automatically generated.
          Hide
          Robert Joseph Evans added a comment -

          Nicholas, I really would like to see this in as part of 0.23 as well. However, because 0.23 does not have HA the patch is not completely compatible. So, I have created a version for 0.23. Would you prefer that I file a separate JIRA for the port to 0.23, or is it OK with you if I just tack that patch on here too?

          and I am +1 (non-binding) on the fix. I am not an HDFS expert so take it for what it is worth.

          Show
          Robert Joseph Evans added a comment - Nicholas, I really would like to see this in as part of 0.23 as well. However, because 0.23 does not have HA the patch is not completely compatible. So, I have created a version for 0.23. Would you prefer that I file a separate JIRA for the port to 0.23, or is it OK with you if I just tack that patch on here too? and I am +1 (non-binding) on the fix. I am not an HDFS expert so take it for what it is worth.
          Hide
          Daryn Sharp added a comment -

          A few general, and probably trivial, questions: Should checkSuperuserPrivilege be called before checkOperation? It seems more logical that a non-admin should be rejected immediately instead of sometimes seeing an error that an admin would see.

          checkOperation for finalizeUpgrade is moved from NameNodeRpcServer into FSNamesystem. On the surface, there doesn't appear to be consistency between where (rpc server or namesystem) the operation is checked. Is it intentional for finalizeUpgrade to be different than the other methods changed in this patch?

          Also involving FSNameSystem.finalizeUpgrade, I'm curious if there's a reason why the operation and admin checks are performed within the write lock?

          Show
          Daryn Sharp added a comment - A few general, and probably trivial, questions: Should checkSuperuserPrivilege be called before checkOperation ? It seems more logical that a non-admin should be rejected immediately instead of sometimes seeing an error that an admin would see. checkOperation for finalizeUpgrade is moved from NameNodeRpcServer into FSNamesystem . On the surface, there doesn't appear to be consistency between where (rpc server or namesystem) the operation is checked. Is it intentional for finalizeUpgrade to be different than the other methods changed in this patch? Also involving FSNameSystem.finalizeUpgrade , I'm curious if there's a reason why the operation and admin checks are performed within the write lock?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I agree that checkSuperuserPrivilege should be called before checkOperation but the current convention is the other way. I recall one reason to put checkSuperuserPrivilege after safemode check is that checkSuperuserPrivilege is more expensive.

          In the current code, it is inconsistent whether checkOperation is invoked in NameNodeRpcServer or FSNamesystem. checkSuperuserPrivilege is usually invoked in FSNamesystem but an exception is DatanodeManager.refreshNode(Configuration). It seems that FSNamesystem is the logical place to make the call since the checkXxx methods are declared in FSNamesystem.

          Other operations in FSNamesystem such as setQuota and listCorruptFileBlocks call checkXxx within the lock.

          I will post another patch for following the current convention as close as possible.

          Show
          Tsz Wo Nicholas Sze added a comment - I agree that checkSuperuserPrivilege should be called before checkOperation but the current convention is the other way. I recall one reason to put checkSuperuserPrivilege after safemode check is that checkSuperuserPrivilege is more expensive. In the current code, it is inconsistent whether checkOperation is invoked in NameNodeRpcServer or FSNamesystem. checkSuperuserPrivilege is usually invoked in FSNamesystem but an exception is DatanodeManager.refreshNode(Configuration). It seems that FSNamesystem is the logical place to make the call since the checkXxx methods are declared in FSNamesystem. Other operations in FSNamesystem such as setQuota and listCorruptFileBlocks call checkXxx within the lock. I will post another patch for following the current convention as close as possible.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          h3331_20120427.patch: moves the checks to FSNamesystem.

          Show
          Tsz Wo Nicholas Sze added a comment - h3331_20120427.patch: moves the checks to FSNamesystem.
          Hide
          Daryn Sharp added a comment -

          +1 Assuming you meant to remove getFSImage().finalizeUpgrade() from FSNameSystem#refreshNodes()

          Show
          Daryn Sharp added a comment - +1 Assuming you meant to remove getFSImage().finalizeUpgrade() from FSNameSystem#refreshNodes()
          Hide
          Tsz Wo Nicholas Sze added a comment -

          The getFSImage().finalizeUpgrade() was original from FSNamesystem.finalizeUpgrade(). FSNamesystem.refreshNodes() is a new method. "svn diff" mixes the lines up.

          Show
          Tsz Wo Nicholas Sze added a comment - The getFSImage().finalizeUpgrade() was original from FSNamesystem.finalizeUpgrade(). FSNamesystem.refreshNodes() is a new method. "svn diff" mixes the lines up.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12524906/h3331_20120427.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 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 passed unit tests in .

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2344//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2344//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/12524906/h3331_20120427.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 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 passed unit tests in . +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2344//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2344//console This message is automatically generated.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          @Robert, thanks for taking a look of the patch. This is for 0.23.

          h3331_20120427_0.23.patch

          Show
          Tsz Wo Nicholas Sze added a comment - @Robert, thanks for taking a look of the patch. This is for 0.23. h3331_20120427_0.23.patch
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I have committed this.

          Show
          Tsz Wo Nicholas Sze added a comment - I have committed this.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2221 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2221/)
          HDFS-3331. In namenode, check superuser privilege for setBalancerBandwidth and acquire the write lock for finalizeUpgrade. (Revision 1331598)

          Result = SUCCESS
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331598
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2221 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2221/ ) HDFS-3331 . In namenode, check superuser privilege for setBalancerBandwidth and acquire the write lock for finalizeUpgrade. (Revision 1331598) Result = SUCCESS szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331598 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          Hide
          Robert Joseph Evans added a comment -

          Thanks Nicholas!

          Show
          Robert Joseph Evans added a comment - Thanks Nicholas!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2147 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2147/)
          HDFS-3331. In namenode, check superuser privilege for setBalancerBandwidth and acquire the write lock for finalizeUpgrade. (Revision 1331598)

          Result = SUCCESS
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331598
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2147 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2147/ ) HDFS-3331 . In namenode, check superuser privilege for setBalancerBandwidth and acquire the write lock for finalizeUpgrade. (Revision 1331598) Result = SUCCESS szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331598 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2164 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2164/)
          HDFS-3331. In namenode, check superuser privilege for setBalancerBandwidth and acquire the write lock for finalizeUpgrade. (Revision 1331598)

          Result = ABORTED
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331598
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2164 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2164/ ) HDFS-3331 . In namenode, check superuser privilege for setBalancerBandwidth and acquire the write lock for finalizeUpgrade. (Revision 1331598) Result = ABORTED szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331598 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-0.23-Build #241 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/241/)
          HDFS-3331. In namenode, check superuser privilege for setBalancerBandwidth and acquire the write lock for finalizeUpgrade. (Revision 1331600)

          Result = SUCCESS
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331600
          Files :

          • /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
          • /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          • /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Build #241 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/241/ ) HDFS-3331 . In namenode, check superuser privilege for setBalancerBandwidth and acquire the write lock for finalizeUpgrade. (Revision 1331600) Result = SUCCESS szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331600 Files : /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/branches/branch-0.23/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1028 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1028/)
          HDFS-3331. In namenode, check superuser privilege for setBalancerBandwidth and acquire the write lock for finalizeUpgrade. (Revision 1331598)

          Result = FAILURE
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331598
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1028 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1028/ ) HDFS-3331 . In namenode, check superuser privilege for setBalancerBandwidth and acquire the write lock for finalizeUpgrade. (Revision 1331598) Result = FAILURE szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331598 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1063 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1063/)
          HDFS-3331. In namenode, check superuser privilege for setBalancerBandwidth and acquire the write lock for finalizeUpgrade. (Revision 1331598)

          Result = FAILURE
          szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331598
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1063 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1063/ ) HDFS-3331 . In namenode, check superuser privilege for setBalancerBandwidth and acquire the write lock for finalizeUpgrade. (Revision 1331598) Result = FAILURE szetszwo : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331598 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java

            People

            • Assignee:
              Tsz Wo Nicholas Sze
              Reporter:
              Tsz Wo Nicholas Sze
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development