Hadoop Common
  1. Hadoop Common
  2. HADOOP-9261

S3n filesystem can move a directory under itself -and so lose data

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.1.1, 2.0.2-alpha
    • Fix Version/s: 3.0.0
    • Component/s: fs/s3
    • Labels:
      None
    • Environment:

      Testing against S3 bucket stored on US West (Read after Write consistency; eventual for read-after-delete or write-after-write)

      Description

      The S3N filesystem rename() doesn't make sure that the destination directory is not a child or other descendant of the source directory. The files are copied to the new destination, then the source directory is recursively deleted, so losing data.

      1. HADOOP-9261.patch
        2 kB
        Steve Loughran
      2. HADOOP-9261-2.patch
        5 kB
        Steve Loughran

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1384 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1384/)
          HADOOP-9258 Add stricter tests to FileSystemContractTestBase (includes fixes for production code HADOOP-9261 & HADOOP-9265 and test enhancements HADOOP-9228, HADOOP-9227 & HADOOP-9259) (Revision 1460646)

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/Jets3tFileSystemStore.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/S3FileSystem.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3native/NativeS3FileSystem.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3/InMemoryFileSystemStore.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3native/NativeS3FileSystemContractBaseTest.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1384 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1384/ ) HADOOP-9258 Add stricter tests to FileSystemContractTestBase (includes fixes for production code HADOOP-9261 & HADOOP-9265 and test enhancements HADOOP-9228 , HADOOP-9227 & HADOOP-9259 ) (Revision 1460646) Result = FAILURE stevel : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1460646 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/Jets3tFileSystemStore.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/S3FileSystem.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3native/NativeS3FileSystem.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3/InMemoryFileSystemStore.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3native/NativeS3FileSystemContractBaseTest.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1356 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1356/)
          HADOOP-9258 Add stricter tests to FileSystemContractTestBase (includes fixes for production code HADOOP-9261 & HADOOP-9265 and test enhancements HADOOP-9228, HADOOP-9227 & HADOOP-9259) (Revision 1460646)

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/Jets3tFileSystemStore.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/S3FileSystem.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3native/NativeS3FileSystem.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3/InMemoryFileSystemStore.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3native/NativeS3FileSystemContractBaseTest.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1356 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1356/ ) HADOOP-9258 Add stricter tests to FileSystemContractTestBase (includes fixes for production code HADOOP-9261 & HADOOP-9265 and test enhancements HADOOP-9228 , HADOOP-9227 & HADOOP-9259 ) (Revision 1460646) Result = FAILURE stevel : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1460646 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/Jets3tFileSystemStore.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/S3FileSystem.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3native/NativeS3FileSystem.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3/InMemoryFileSystemStore.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3native/NativeS3FileSystemContractBaseTest.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Yarn-trunk #167 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/167/)
          HADOOP-9258 Add stricter tests to FileSystemContractTestBase (includes fixes for production code HADOOP-9261 & HADOOP-9265 and test enhancements HADOOP-9228, HADOOP-9227 & HADOOP-9259) (Revision 1460646)

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/Jets3tFileSystemStore.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/S3FileSystem.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3native/NativeS3FileSystem.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3/InMemoryFileSystemStore.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3native/NativeS3FileSystemContractBaseTest.java
          Show
          Hudson added a comment - Integrated in Hadoop-Yarn-trunk #167 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/167/ ) HADOOP-9258 Add stricter tests to FileSystemContractTestBase (includes fixes for production code HADOOP-9261 & HADOOP-9265 and test enhancements HADOOP-9228 , HADOOP-9227 & HADOOP-9259 ) (Revision 1460646) Result = SUCCESS stevel : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1460646 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/Jets3tFileSystemStore.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/S3FileSystem.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3native/NativeS3FileSystem.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3/InMemoryFileSystemStore.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3native/NativeS3FileSystemContractBaseTest.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk-Commit #3520 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3520/)
          HADOOP-9258 Add stricter tests to FileSystemContractTestBase (includes fixes for production code HADOOP-9261 & HADOOP-9265 and test enhancements HADOOP-9228, HADOOP-9227 & HADOOP-9259) (Revision 1460646)

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

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/Jets3tFileSystemStore.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/S3FileSystem.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3native/NativeS3FileSystem.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3/InMemoryFileSystemStore.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3native/NativeS3FileSystemContractBaseTest.java
          Show
          Hudson added a comment - Integrated in Hadoop-trunk-Commit #3520 (See https://builds.apache.org/job/Hadoop-trunk-Commit/3520/ ) HADOOP-9258 Add stricter tests to FileSystemContractTestBase (includes fixes for production code HADOOP-9261 & HADOOP-9265 and test enhancements HADOOP-9228 , HADOOP-9227 & HADOOP-9259 ) (Revision 1460646) Result = SUCCESS stevel : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1460646 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/Jets3tFileSystemStore.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3/S3FileSystem.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/s3native/NativeS3FileSystem.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FileSystemContractBaseTest.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3/InMemoryFileSystemStore.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/s3native/NativeS3FileSystemContractBaseTest.java
          Hide
          Steve Loughran added a comment -

          cancelling mv(self, self) behaviours may be not fully in sync w/ posix

          Show
          Steve Loughran added a comment - cancelling mv(self, self) behaviours may be not fully in sync w/ posix
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12567693/HADOOP-9261-2.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. The javadoc tool did not generate any 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-common-project/hadoop-common.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2138//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2138//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/12567693/HADOOP-9261-2.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 . The javadoc tool did not generate any 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-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2138//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2138//console This message is automatically generated.
          Hide
          Steve Loughran added a comment -

          There's enough changes here that we do need rigorous review.
          The previous patch saw copy-onto-self and bailed out early -but these checks were made before the destination path was fully generated -because if you specify a destination directory, the source goes in under there. Which means you could set the dest dir to be the parent of the source file, so generating a rename(src, src) after the equality check had taken place -a rename that would return false.
          Now the value is checked once up front -for fast exit without talking to S3, and then just before the operation actually takes place.
          I also added lots of inline comments to make it clearer what is going on.

          Show
          Steve Loughran added a comment - There's enough changes here that we do need rigorous review. The previous patch saw copy-onto-self and bailed out early -but these checks were made before the destination path was fully generated -because if you specify a destination directory, the source goes in under there. Which means you could set the dest dir to be the parent of the source file, so generating a rename(src, src) after the equality check had taken place -a rename that would return false. Now the value is checked once up front -for fast exit without talking to S3, and then just before the operation actually takes place. I also added lots of inline comments to make it clearer what is going on.
          Hide
          Steve Loughran added a comment -

          This turns out to be harder than expected because of the requirement for POSIX return codes.

          #rename file->file = true
          #rename dir->dir = false
          Also, we need to check the destination after the full destination is calculated, not upfront, so that when the full pathname is calculated as parent+filename, the equality check needs to be made then.

          1. rename (path/file,path) returns true (shortcutting the copy)
          2. rename (path/dir, path) fails
          Show
          Steve Loughran added a comment - This turns out to be harder than expected because of the requirement for POSIX return codes. #rename file->file = true #rename dir->dir = false Also, we need to check the destination after the full destination is calculated, not upfront, so that when the full pathname is calculated as parent+filename, the equality check needs to be made then. rename (path/file,path) returns true (shortcutting the copy) rename (path/dir, path) fails
          Hide
          Steve Loughran added a comment -

          A further test shows that rename-onto-self can play up; need to incorporate this into the patch too with a src.equals(dst) check

          Show
          Steve Loughran added a comment - A further test shows that rename-onto-self can play up; need to incorporate this into the patch too with a src.equals(dst) check
          Hide
          Tom White added a comment -

          +1 looks good to me.

          Show
          Tom White added a comment - +1 looks good to me.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12567188/HADOOP-9261.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. The javadoc tool did not generate any 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-common-project/hadoop-common.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2118//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2118//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/12567188/HADOOP-9261.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 . The javadoc tool did not generate any 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-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2118//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2118//console This message is automatically generated.
          Hide
          Steve Loughran added a comment -

          Tests are in HADOOP-5928, not this patch. Targeting branch-1 and trunk

          Show
          Steve Loughran added a comment - Tests are in HADOOP-5928 , not this patch. Targeting branch-1 and trunk
          Hide
          Steve Loughran added a comment -

          With this patch, s3n passes the extended tests of HADOOP-9528

          Show
          Steve Loughran added a comment - With this patch, s3n passes the extended tests of HADOOP-9528
          Hide
          Steve Loughran added a comment -

          This surfaces in the extended tests of HADOOP-9528

          Show
          Steve Loughran added a comment - This surfaces in the extended tests of HADOOP-9528

            People

            • Assignee:
              Steve Loughran
              Reporter:
              Steve Loughran
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development