Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-6395

Skip checking xattr limits for non-user-visible namespaces

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0
    • Fix Version/s: 2.5.0
    • Component/s: namenode
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      It'd be nice to print messages during fsimage and editlog loading if we hit either the # of xattrs per inode or the xattr size limits.

      We should also consider making the # of xattrs limit only apply to the user namespace, or to each namespace separately, to prevent users from locking out access to other namespaces.

      1. HDFS-6395.1.patch
        6 kB
        Yi Liu
      2. HDFS-6395.patch
        4 kB
        Yi Liu

        Issue Links

          Activity

          Hide
          Andrew Wang added a comment -

          Let's also remove the extra import in FSNamesystem that Chris noticed from HDFS-6377:

          +import java.io.UnsupportedEncodingException;
          
          Show
          Andrew Wang added a comment - Let's also remove the extra import in FSNamesystem that Chris noticed from HDFS-6377 : + import java.io.UnsupportedEncodingException;
          Hide
          Yi Liu added a comment -

          Thanks Andrew, I'm handling this.

          Show
          Yi Liu added a comment - Thanks Andrew, I'm handling this.
          Hide
          Uma Maheswara Rao G added a comment -

          Moved as top level Jira as HDFS-2006 branch merged to trunk!

          Show
          Uma Maheswara Rao G added a comment - Moved as top level Jira as HDFS-2006 branch merged to trunk!
          Hide
          Andrew Wang added a comment -

          Hey Yi, I'd like to take a hack at this if you're busy with other things. Is it okay if I assign it to myself?

          Show
          Andrew Wang added a comment - Hey Yi, I'd like to take a hack at this if you're busy with other things. Is it okay if I assign it to myself?
          Hide
          Yi Liu added a comment -

          Thanks Andrew, actually I have finished part of it last week, please let me continue. Thanks again for your voluntary.

          Show
          Yi Liu added a comment - Thanks Andrew, actually I have finished part of it last week, please let me continue. Thanks again for your voluntary.
          Hide
          Andrew Wang added a comment -

          Cool, that's fine with me. I thought a bit more after I posted my HDFS-2006 comment. Basically, I want to avoid the configuration complexity of per-NS confs, ensure that future internal HDFS features (i.e. HDFS-6134) can use XAttrs without depending on XAttrs being enabled, but still give admins flexibility to control user usage of XAttrs. What do you think about the following?

          • XAttr enable/disable config flag and limits configs only affects user-settable namespaces (i.e. user and trusted)
          • System and security namespace have no limits, and are still available even if xattrs are disabled

          Thanks Liu Yi!

          Show
          Andrew Wang added a comment - Cool, that's fine with me. I thought a bit more after I posted my HDFS-2006 comment. Basically, I want to avoid the configuration complexity of per-NS confs, ensure that future internal HDFS features (i.e. HDFS-6134 ) can use XAttrs without depending on XAttrs being enabled, but still give admins flexibility to control user usage of XAttrs. What do you think about the following? XAttr enable/disable config flag and limits configs only affects user-settable namespaces (i.e. user and trusted) System and security namespace have no limits, and are still available even if xattrs are disabled Thanks Liu Yi!
          Hide
          Yi Liu added a comment -

          Thanks Andrew, it makes sense that XAttr enable/disable config flag and limits configs only affects user-settable namespaces. I will update the patch later today.

          Show
          Yi Liu added a comment - Thanks Andrew, it makes sense that XAttr enable/disable config flag and limits configs only affects user-settable namespaces. I will update the patch later today.
          Hide
          Yi Liu added a comment -

          Hi Andrew Wang, I think current implementation already covers what we want.

          • XAttr enable/disable config flag and limits configs only affects user-settable namespaces (i.e. user and trusted)
          • System and security namespace have no limits, and are still available even if xattrs are disabled

          For XAttr enable/disable config flag and size limits configs, we check them in FSNamesystem and RPC calls will be delegated to there. Only user-settable namespaces (user and trusted) are allowed.

          Internal HDFS features (in HDFS core) should use the methods (setXAttr, getXAttrs, removeXAttr) in FSdirectory, and there we don't check the XAttr enable/disable config flag and size limits configs.

          If you agree with this, we can close this JIRA as fixed?

          Show
          Yi Liu added a comment - Hi Andrew Wang , I think current implementation already covers what we want. XAttr enable/disable config flag and limits configs only affects user-settable namespaces (i.e. user and trusted) System and security namespace have no limits, and are still available even if xattrs are disabled For XAttr enable/disable config flag and size limits configs, we check them in FSNamesystem and RPC calls will be delegated to there. Only user-settable namespaces (user and trusted) are allowed. Internal HDFS features (in HDFS core) should use the methods ( setXAttr , getXAttrs , removeXAttr ) in FSdirectory , and there we don't check the XAttr enable/disable config flag and size limits configs. If you agree with this, we can close this JIRA as fixed?
          Hide
          Andrew Wang added a comment -

          Makes sense to me, thanks for investigating Yi. We should still fix up Chris's earlier comments from a prior review though, which were printing messages during metadata loading when things exceed limits, and removing the unnecessary import.

          Show
          Andrew Wang added a comment - Makes sense to me, thanks for investigating Yi. We should still fix up Chris's earlier comments from a prior review though, which were printing messages during metadata loading when things exceed limits, and removing the unnecessary import.
          Hide
          Yi Liu added a comment -

          Thanks Andrew Wang. Right, I will remove the unnecessary import.
          I change the dfs.namenode.fs-limits.max-xattrs-per-inode only affects user-settable namespaces too. Now for xattrs, user's behavior will not affect internal HDFS features at all.

          For printing message when things exceed limits. I add the log in setXAttr and maybe it's better than log during fsimage and editlog loading, it's easier to get all xattrs of inode there and only user-settable namspaces have the limit.

          Show
          Yi Liu added a comment - Thanks Andrew Wang . Right, I will remove the unnecessary import. I change the dfs.namenode.fs-limits.max-xattrs-per-inode only affects user-settable namespaces too. Now for xattrs, user's behavior will not affect internal HDFS features at all. For printing message when things exceed limits. I add the log in setXAttr and maybe it's better than log during fsimage and editlog loading, it's easier to get all xattrs of inode there and only user-settable namspaces have the limit.
          Hide
          Yi Liu added a comment -

          For the boundary test of limits configs, we have existing test testSetXAttr to cover.

          Show
          Yi Liu added a comment - For the boundary test of limits configs, we have existing test testSetXAttr to cover.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12647773/HDFS-6395.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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

          +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 hadoop-hdfs-project/hadoop-hdfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/7024//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/7024//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/12647773/HDFS-6395.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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +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 hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/7024//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/7024//console This message is automatically generated.
          Hide
          Andrew Wang added a comment -

          I chatted with Yi a bit offline with some review feedback, re-posting here:

          I think the idea with the printing was a situation where the admin lowers the xattr limits and restarts the NN. It'd be nice to print out a message if the NN encounters any XAttrs that exceed the new lower limit during metadata loading. Doing this precisely is some work (the edit log might add then remove xattrs, bringing it back under the limit), but I think the rough solution of just printing whenever a limit is exceeded is good enough.

          Show
          Andrew Wang added a comment - I chatted with Yi a bit offline with some review feedback, re-posting here: I think the idea with the printing was a situation where the admin lowers the xattr limits and restarts the NN. It'd be nice to print out a message if the NN encounters any XAttrs that exceed the new lower limit during metadata loading. Doing this precisely is some work (the edit log might add then remove xattrs, bringing it back under the limit), but I think the rough solution of just printing whenever a limit is exceeded is good enough.
          Hide
          Yi Liu added a comment - - edited

          Thanks Andrew.
          When I look into the detail and discussed with Uma, we found some issue.
          We need to get the full inode path for printing, it a bit different to get the full inode path when loading an inode.
          The fsimage protobuf serialization is as following:

          saver.serializeINodeSection(sectionOutputStream);
          saver.serializeINodeDirectorySection(sectionOutputStream);
          saver.serializeFilesUCSection(sectionOutputStream);
          

          So we can't get the inodes tree when loading inode section. We can't simply change the order and it doesn't work.

          An alternative way is only print the current inode name (not including the parents) and the inode id, but I think the user/admin will not know what an inode id is.

          Thoughts?

          Show
          Yi Liu added a comment - - edited Thanks Andrew. When I look into the detail and discussed with Uma, we found some issue. We need to get the full inode path for printing, it a bit different to get the full inode path when loading an inode. The fsimage protobuf serialization is as following: saver.serializeINodeSection(sectionOutputStream); saver.serializeINodeDirectorySection(sectionOutputStream); saver.serializeFilesUCSection(sectionOutputStream); So we can't get the inodes tree when loading inode section. We can't simply change the order and it doesn't work. An alternative way is only print the current inode name (not including the parents) and the inode id, but I think the user/admin will not know what an inode id is. Thoughts?
          Hide
          Andrew Wang added a comment -

          I should have realized this earlier, considering I worked on something pretty similar with the fs-limits and the edit log before. I agree that it's difficult to do this without some serious code gymnastics, so let's just table the entire thing for now. Please resolve this if you agree, thanks again Yi Liu.

          Show
          Andrew Wang added a comment - I should have realized this earlier, considering I worked on something pretty similar with the fs-limits and the edit log before. I agree that it's difficult to do this without some serious code gymnastics, so let's just table the entire thing for now. Please resolve this if you agree, thanks again Yi Liu .
          Hide
          Andrew Wang added a comment -

          Woops, my bad, I forgot that this patch also fixes the # limit to not apply to the non-user namespaces. I had a few comments:

          • Would be nice to test that the system namespace isn't affected by these limits, I guess reach into FSNamesystem or FSDirectory via @VisibleForTesting methods.
          • Let's remove the prints when the limits hit their max, since I think that was a misunderstanding of Chris' comment about printing.

          Thanks Yi!

          Show
          Andrew Wang added a comment - Woops, my bad, I forgot that this patch also fixes the # limit to not apply to the non-user namespaces. I had a few comments: Would be nice to test that the system namespace isn't affected by these limits, I guess reach into FSNamesystem or FSDirectory via @VisibleForTesting methods. Let's remove the prints when the limits hit their max, since I think that was a misunderstanding of Chris' comment about printing. Thanks Yi!
          Hide
          Chris Nauroth added a comment -
              if (xAttr.getNameSpace() == XAttr.NameSpace.USER || 
                  xAttr.getNameSpace() == XAttr.NameSpace.TRUSTED) {
          

          Minor nit-pick: this piece of logic is duplicated. It might be worthwhile to put this in a helper method or possibly add an isUserVisible method on the NameSpace enum to document the intent.

          I agree with Andrew's feedback about removing these prints. If it's too tricky to log warnings during loading right now, then let's proceed with the other valuable changes in this patch. We can always revisit the logging later if needed.

          Thank you for working on this, Yi.

          Show
          Chris Nauroth added a comment - if (xAttr.getNameSpace() == XAttr.NameSpace.USER || xAttr.getNameSpace() == XAttr.NameSpace.TRUSTED) { Minor nit-pick: this piece of logic is duplicated. It might be worthwhile to put this in a helper method or possibly add an isUserVisible method on the NameSpace enum to document the intent. I agree with Andrew's feedback about removing these prints. If it's too tricky to log warnings during loading right now, then let's proceed with the other valuable changes in this patch. We can always revisit the logging later if needed. Thank you for working on this, Yi.
          Hide
          Yi Liu added a comment -

          Thanks Andrew Wang and Chris Nauroth for review
          The new patch includes update for your comments.

          Show
          Yi Liu added a comment - Thanks Andrew Wang and Chris Nauroth for review The new patch includes update for your comments.
          Hide
          Chris Nauroth added a comment -

          +1 for the patch, pending Jenkins run. Thanks again, Yi!

          Show
          Chris Nauroth added a comment - +1 for the patch, pending Jenkins run. Thanks again, Yi!
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12649792/HDFS-6395.1.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 1 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          +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 hadoop-hdfs-project/hadoop-hdfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/7081//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/7081//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/12649792/HDFS-6395.1.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +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 hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/7081//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/7081//console This message is automatically generated.
          Hide
          Chris Nauroth added a comment -

          It's looking good, but I'll hold off committing in case Andrew has additional feedback.

          Show
          Chris Nauroth added a comment - It's looking good, but I'll hold off committing in case Andrew has additional feedback.
          Hide
          Yi Liu added a comment -

          Thanks Chris Nauroth for review. Let's see if Andrew Wang has additional feedback.

          Show
          Yi Liu added a comment - Thanks Chris Nauroth for review. Let's see if Andrew Wang has additional feedback.
          Hide
          Andrew Wang added a comment -

          +1 from me too, will commit shortly. Only nitty thing is that I might have made isUserVisible an instance method of XAttr, but it's fine as is.

          Show
          Andrew Wang added a comment - +1 from me too, will commit shortly. Only nitty thing is that I might have made isUserVisible an instance method of XAttr, but it's fine as is.
          Hide
          Andrew Wang added a comment -

          Committed to trunk and branch-2, thanks Yi for the patch and Chris for reviews!

          Show
          Andrew Wang added a comment - Committed to trunk and branch-2, thanks Yi for the patch and Chris for reviews!
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #5692 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5692/)
          HDFS-6395. Skip checking xattr limits for non-user-visible namespaces. Contributed by Yi Liu. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602288)

          • /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/namenode/FSDirectory.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSDirectory.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #5692 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5692/ ) HDFS-6395 . Skip checking xattr limits for non-user-visible namespaces. Contributed by Yi Liu. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602288 ) /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/namenode/FSDirectory.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSDirectory.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #582 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/582/)
          HDFS-6395. Skip checking xattr limits for non-user-visible namespaces. Contributed by Yi Liu. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602288)

          • /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/namenode/FSDirectory.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSDirectory.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #582 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/582/ ) HDFS-6395 . Skip checking xattr limits for non-user-visible namespaces. Contributed by Yi Liu. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602288 ) /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/namenode/FSDirectory.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSDirectory.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Hdfs-trunk #1773 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1773/)
          HDFS-6395. Skip checking xattr limits for non-user-visible namespaces. Contributed by Yi Liu. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602288)

          • /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/namenode/FSDirectory.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSDirectory.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk #1773 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1773/ ) HDFS-6395 . Skip checking xattr limits for non-user-visible namespaces. Contributed by Yi Liu. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602288 ) /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/namenode/FSDirectory.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSDirectory.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #1800 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1800/)
          HDFS-6395. Skip checking xattr limits for non-user-visible namespaces. Contributed by Yi Liu. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602288)

          • /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/namenode/FSDirectory.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSDirectory.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1800 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1800/ ) HDFS-6395 . Skip checking xattr limits for non-user-visible namespaces. Contributed by Yi Liu. (wang: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1602288 ) /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/namenode/FSDirectory.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSDirectory.java

            People

            • Assignee:
              Yi Liu
              Reporter:
              Andrew Wang
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development