Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-677

Rename failure due to quota results in deletion of src directory

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.20.1, 0.20.2, 0.21.0, 0.22.0
    • Fix Version/s: 0.20.2, 0.21.0
    • Component/s: namenode
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Rename properly considers the case where both source and destination are over quota; operation will fail with error indication.

      Description

      Renaming src to destination where src has exceeded the quota to a dst without sufficent quota fails. During this failure, src is deleted.

      1. hdfs-677.8.patch
        21 kB
        Suresh Srinivas
      2. hdfs-677.9.patch
        21 kB
        Suresh Srinivas
      3. hdfs-677.rel20.patch
        24 kB
        Suresh Srinivas

        Issue Links

          Activity

          Suresh Srinivas created issue -
          Hide
          dhruba borthakur added a comment -

          hi suresh, this seems like a blocker for 0.20 release, isn't it?

          Show
          dhruba borthakur added a comment - hi suresh, this seems like a blocker for 0.20 release, isn't it?
          Hide
          Suresh Srinivas added a comment -

          changed the priority to blocker

          Show
          Suresh Srinivas added a comment - changed the priority to blocker
          Suresh Srinivas made changes -
          Field Original Value New Value
          Fix Version/s 0.20.1 [ 12314048 ]
          Priority Critical [ 2 ] Blocker [ 1 ]
          Hide
          Suresh Srinivas added a comment -

          Here is the approach:

          1. Current rename implementation removes the src and while moving it to dst, if failure is encountered it adds the src back. This change, before removing src from directory tree, ensures the dst has quota and other conditions needed for rename to succeed is met.
          2. During rename after the src is removed, any failure to complete rename is handled by adding src back without checking for quota.
          3. Previous version of the code threw QuotaExceededException from FSDirectory.removeChild(). This was unnecessary since the quota was being freed and the exception would never occur. This method no longer throws exception.
          Show
          Suresh Srinivas added a comment - Here is the approach: Current rename implementation removes the src and while moving it to dst, if failure is encountered it adds the src back. This change, before removing src from directory tree, ensures the dst has quota and other conditions needed for rename to succeed is met. During rename after the src is removed, any failure to complete rename is handled by adding src back without checking for quota. Previous version of the code threw QuotaExceededException from FSDirectory.removeChild(). This was unnecessary since the quota was being freed and the exception would never occur. This method no longer throws exception.
          Suresh Srinivas made changes -
          Attachment hdfs-677.8.patch [ 12421677 ]
          Hide
          Hong Tang added a comment -

          2. During rename after the src is removed, any failure to complete rename is handled by adding src back without checking for quota.

          What kind of failures might we encounter during the rename? How can we guarantee to have no failures adding src back?

          In general, you are trying to make the move operation atomic (either done successfully, or no effect is happening). This typically means that you want to prepare as much change on the side as possible (without changing the state), and effect the state change in one atomic operation.

          Show
          Hong Tang added a comment - 2. During rename after the src is removed, any failure to complete rename is handled by adding src back without checking for quota. What kind of failures might we encounter during the rename? How can we guarantee to have no failures adding src back? In general, you are trying to make the move operation atomic (either done successfully, or no effect is happening). This typically means that you want to prepare as much change on the side as possible (without changing the state), and effect the state change in one atomic operation.
          Hide
          Suresh Srinivas added a comment -

          > This typically means that you want to prepare as much change on the side as possible (without changing the state), and effect the state change in one atomic operation.
          That is what I have captured in bullet 1. All the checks are done prior to removing node. I do not anticipated any failure after that. But in case some exceptions happen, then the src is restored back in a finally block.

          Show
          Suresh Srinivas added a comment - > This typically means that you want to prepare as much change on the side as possible (without changing the state), and effect the state change in one atomic operation. That is what I have captured in bullet 1. All the checks are done prior to removing node. I do not anticipated any failure after that. But in case some exceptions happen, then the src is restored back in a finally block.
          Hide
          Hong Tang added a comment -

          I do not anticipated any failure after that. But in case some exceptions happen, then the src is restored back in a finally block.

          Are you asserting that restoring src back will guarantee to succeed? (Or die miserably, e.g. OOM and the whole NN will crash).

          A typical way of doing this is:

          • lock down both src and dest node.
          • Create src.shadow and dest.shadow and set up the state of src.shadow and dest.shadow as how src and dest would look like after the rename.
          • Support a "guaranteed-to-success" "swap" operation on the node. And "swap" between src.shadow and src, and dest.shadow and dest. This is guaranteed to success.
          Show
          Hong Tang added a comment - I do not anticipated any failure after that. But in case some exceptions happen, then the src is restored back in a finally block. Are you asserting that restoring src back will guarantee to succeed? (Or die miserably, e.g. OOM and the whole NN will crash). A typical way of doing this is: lock down both src and dest node. Create src.shadow and dest.shadow and set up the state of src.shadow and dest.shadow as how src and dest would look like after the rename. Support a "guaranteed-to-success" "swap" operation on the node. And "swap" between src.shadow and src, and dest.shadow and dest. This is guaranteed to success.
          Hide
          Suresh Srinivas added a comment -

          adding the node back is similar operation that is not expected to fail (just like swap in the above case needs to be). BTW any failure such as OOM or NN crash etc. will result in client receiving failure and this operation is not recorded in edits log. When NN restarts, src would still be in the namespace as expected.

          Show
          Suresh Srinivas added a comment - adding the node back is similar operation that is not expected to fail (just like swap in the above case needs to be). BTW any failure such as OOM or NN crash etc. will result in client receiving failure and this operation is not recorded in edits log. When NN restarts, src would still be in the namespace as expected.
          Hide
          Allen Wittenauer added a comment -

          While it was a Good Thing(tm) in this instance, isn't anyone else concerned about the fact that the in memory image and the on disk image are apparently different?

          Show
          Allen Wittenauer added a comment - While it was a Good Thing(tm) in this instance, isn't anyone else concerned about the fact that the in memory image and the on disk image are apparently different?
          Hide
          Nigel Daley added a comment -

          adding the node back is similar operation that is not expected to fail (just like swap in the above case needs to be). BTW any failure such as OOM or NN crash etc. will result in client receiving failure and this operation is not recorded in edits log. When NN restarts, src would still be in the namespace as expected.

          Can these conditions be tested with mocks or injection?

          Show
          Nigel Daley added a comment - adding the node back is similar operation that is not expected to fail (just like swap in the above case needs to be). BTW any failure such as OOM or NN crash etc. will result in client receiving failure and this operation is not recorded in edits log. When NN restarts, src would still be in the namespace as expected. Can these conditions be tested with mocks or injection?
          Hide
          Konstantin Shvachko added a comment -

          The patch looks good. I went through the main logic in unprotectedRenameTo(). Most of conditions in which rename is inapplicable are checked in advance before moving anything anywhere. Everything seems correct. But it will need extensive testing. To make sure all the math is right in verifyQuota(), etc.
          Small things:

          • Check the value returned by getExistingPathINodes(), compare it with the number of path components.
          • verifyQuota() needs JavaDoc - don't want to read code every time I need to know what it does.
          • you addChild() with inheritPermission == true. In the old code it was false. Is that intentional?
          Show
          Konstantin Shvachko added a comment - The patch looks good. I went through the main logic in unprotectedRenameTo() . Most of conditions in which rename is inapplicable are checked in advance before moving anything anywhere. Everything seems correct. But it will need extensive testing. To make sure all the math is right in verifyQuota() , etc. Small things: Check the value returned by getExistingPathINodes(), compare it with the number of path components. verifyQuota() needs JavaDoc - don't want to read code every time I need to know what it does. you addChild() with inheritPermission == true . In the old code it was false . Is that intentional?
          Hide
          Suresh Srinivas added a comment -

          > will need extensive testing...
          Currently TestHDFSCLI, TestQuota and the new test cases added add to a lot of testcases. Let me know if you feel any thing more specific is needed. Also FSDirectory.verifyQuota() is mostly refactored code out of FSDirectory.updateCount().

          > Check the value returned by getExistingPathINodes(), compare it with the number of path components.
          This is what current checks for dstInodes[dstInodes.length-1] != null and dstInodes[dstInodes.length-2] == null does. If you feel it is still good to add additional check for return value, I can add it.

          > you addChild() with inheritPermission == true. In the old code it was false. Is that intentional?
          That was not intentional. Thanks for catching it.

          Attaching new patch with incorporated comments.

          Show
          Suresh Srinivas added a comment - > will need extensive testing... Currently TestHDFSCLI, TestQuota and the new test cases added add to a lot of testcases. Let me know if you feel any thing more specific is needed. Also FSDirectory.verifyQuota() is mostly refactored code out of FSDirectory.updateCount(). > Check the value returned by getExistingPathINodes(), compare it with the number of path components. This is what current checks for dstInodes [dstInodes.length-1] != null and dstInodes [dstInodes.length-2] == null does. If you feel it is still good to add additional check for return value, I can add it. > you addChild() with inheritPermission == true. In the old code it was false. Is that intentional? That was not intentional. Thanks for catching it. Attaching new patch with incorporated comments.
          Suresh Srinivas made changes -
          Attachment hdfs-677.9.patch [ 12421914 ]
          Hide
          Konstantin Shvachko added a comment -

          +1.
          I see you included a test for renames with quotas.

          Show
          Konstantin Shvachko added a comment - +1. I see you included a test for renames with quotas.
          Suresh Srinivas made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Suresh Srinivas added a comment -

          Latest patch passes all the tests with the following exception:

          1. org.apache.hadoop.hdfs.TestFileAppend3 (HDFS-668)
          2. org.apache.hadoop.hdfs.server.namenode.TestBackupNode(HDFS-192)

          Here is the test-patch result:
          [exec] +1 overall.
          [exec]
          [exec] +1 @author. The patch does not contain any @author tags.
          [exec]
          [exec] +1 tests included. The patch appears to include 16 new or modified tests.
          [exec]
          [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
          [exec]
          [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
          [exec]
          [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
          [exec]
          [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

          Show
          Suresh Srinivas added a comment - Latest patch passes all the tests with the following exception: org.apache.hadoop.hdfs.TestFileAppend3 ( HDFS-668 ) org.apache.hadoop.hdfs.server.namenode.TestBackupNode( HDFS-192 ) Here is the test-patch result: [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 16 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
          Hide
          Suresh Srinivas added a comment -

          Committed the patch to both 21 and trunk. Will upload and commit the patch for 20 later.

          Show
          Suresh Srinivas added a comment - Committed the patch to both 21 and trunk. Will upload and commit the patch for 20 later.
          Suresh Srinivas made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Resolution Fixed [ 1 ]
          Suresh Srinivas made changes -
          Link This issue relates to HDFS-709 [ HDFS-709 ]
          Hide
          Suresh Srinivas added a comment -

          Attaching release 20 version of the patch.

          Show
          Suresh Srinivas added a comment - Attaching release 20 version of the patch.
          Suresh Srinivas made changes -
          Attachment hdfs-677.rel20.patch [ 12422371 ]
          Hide
          Suresh Srinivas added a comment -

          I committed this patch to 20 branch.

          Show
          Suresh Srinivas added a comment - I committed this patch to 20 branch.
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #47 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/47/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #47 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/47/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #79 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/79/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #79 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/79/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #120 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/120/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #120 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk/120/ )
          Robert Chansler made changes -
          Release Note Rename properly considers the case where both source and destination are over quota; operation will fail with error indication.
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #78 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/78/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #78 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/78/ )
          dhruba borthakur made changes -
          Link This issue is related to HDFS-761 [ HDFS-761 ]
          Tom White made changes -
          Fix Version/s 0.22.0 [ 12314241 ]
          Tom White made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Suresh Srinivas
              Reporter:
              Suresh Srinivas
            • Votes:
              0 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development