Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.6.0
    • Fix Version/s: 3.0.0-alpha2
    • Component/s: None
    • Labels:
      None

      Description

      TestPermission create a user with the following name and group:

       final private static String USER_NAME = "user" + RAN.nextInt();
       final private static String[] GROUP_NAMES = {"group1", "group2"};
      
         UserGroupInformation userGroupInfo = 
              UserGroupInformation.createUserForTesting(USER_NAME, GROUP_NAMES );
            
            FileSystem userfs = DFSTestUtil.getFileSystemAs(userGroupInfo, conf);
      
            // make sure mkdir of a existing directory that is not owned by 
            // this user does not throw an exception.
            userfs.mkdirs(CHILD_DIR1);
            
      

      Supposedly

       userfs.setOwner(CHILD_FILE3, "foo", "bar");
      

      will be run as the specified user, but it seems to be run as me who run the test.

      Running as the specified user would disallow setOwner, which requires superuser privilege. This is not happening.

      Creating this jira for some investigation to understand whether it's indeed an issue.

      Thanks.

        Activity

        Hide
        yzhangal Yongjun Zhang added a comment -

        Thanks John for offering to look into.

        Show
        yzhangal Yongjun Zhang added a comment - Thanks John for offering to look into.
        Hide
        jzhuge John Zhuge added a comment -

        Got this unexpected exception when debugging TestPermission:

        java.lang.IllegalStateException
        	at com.google.common.base.Preconditions.checkState(Preconditions.java:129)
        	at org.apache.hadoop.ipc.Client.setCallIdAndRetryCount(Client.java:141)
        	at org.apache.hadoop.io.retry.RetryInvocationHandler.invoke(RetryInvocationHandler.java:100)
        	at com.sun.proxy.$Proxy22.setOwner(Unknown Source)
        	at org.apache.hadoop.hdfs.DFSClient.setOwner(DFSClient.java:1825)
        	at org.apache.hadoop.hdfs.DistributedFileSystem$33.doCall(DistributedFileSystem.java:1502)
        	at org.apache.hadoop.hdfs.DistributedFileSystem$33.doCall(DistributedFileSystem.java:1499)
        	at org.apache.hadoop.fs.FileSystemLinkResolver.resolve(FileSystemLinkResolver.java:81)
        	at org.apache.hadoop.hdfs.DistributedFileSystem.setOwner(DistributedFileSystem.java:1499)
        	at org.apache.hadoop.security.TestPermission.testFilePermission(TestPermission.java:277)
        	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        	at java.lang.reflect.Method.invoke(Method.java:606)
        	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
        	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
        	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
        	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
        	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
        	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
        	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
        	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
        	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
        	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
        	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
        	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
        	at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
        	at org.junit.runner.JUnitCore.run(JUnitCore.java:160)
        	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:119)
        	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:42)
        	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:234)
        	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:74)
        
        Show
        jzhuge John Zhuge added a comment - Got this unexpected exception when debugging TestPermission: java.lang.IllegalStateException at com.google.common.base.Preconditions.checkState(Preconditions.java:129) at org.apache.hadoop.ipc.Client.setCallIdAndRetryCount(Client.java:141) at org.apache.hadoop.io.retry.RetryInvocationHandler.invoke(RetryInvocationHandler.java:100) at com.sun.proxy.$Proxy22.setOwner(Unknown Source) at org.apache.hadoop.hdfs.DFSClient.setOwner(DFSClient.java:1825) at org.apache.hadoop.hdfs.DistributedFileSystem$33.doCall(DistributedFileSystem.java:1502) at org.apache.hadoop.hdfs.DistributedFileSystem$33.doCall(DistributedFileSystem.java:1499) at org.apache.hadoop.fs.FileSystemLinkResolver.resolve(FileSystemLinkResolver.java:81) at org.apache.hadoop.hdfs.DistributedFileSystem.setOwner(DistributedFileSystem.java:1499) at org.apache.hadoop.security.TestPermission.testFilePermission(TestPermission.java:277) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:606) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229) at org.junit.runners.ParentRunner.run(ParentRunner.java:309) at org.junit.runner.JUnitCore.run(JUnitCore.java:160) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:119) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:42) at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:234) at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:74)
        Hide
        jzhuge John Zhuge added a comment -

        Ignore my previous comment about IllegalStateException, probably triggered by IDEA step-by-step debugger session. Do not see the problem any more.

        Show
        jzhuge John Zhuge added a comment - Ignore my previous comment about IllegalStateException, probably triggered by IDEA step-by-step debugger session. Do not see the problem any more.
        Hide
        jzhuge John Zhuge added a comment -

        After some debugging, the bogus user is passed to NN for both trunk and 2.6.

        In the trunk, NN.checkOwner checks the existence of the file first before checking permission:

          void checkOwner(FSPermissionChecker pc, INodesInPath iip)
              throws AccessControlException, FileNotFoundException {
            if (iip.getLastINode() == null) {
              throw new FileNotFoundException(
                  "Directory/File does not exist " + iip.getPath());
            }
            checkPermission(pc, iip, true, null, null, null, null);
          }
        

        In branch 2.6, it doesn't:

          private void checkOwner(FSPermissionChecker pc, String path)
              throws AccessControlException, UnresolvedLinkException {
            checkPermission(pc, path, true, null, null, null, null);
          }
        

        The unit test calls setOwner with 2 error conditions: file doesn't exist, user doesn't have permission. What is NN expected to do? Throw FileNotFoundException or AccessControlException? Is there any doc or convention?

              // test permissions on files that do not exist
              assertFalse(userfs.exists(CHILD_FILE3));
              try {
                userfs.setOwner(CHILD_FILE3, "foo", "bar");             <<<<<<<<=======
                fail("setOwner should fail for non-exist file");
              } catch (java.io.FileNotFoundException ignored) {
                LOG.info("GOOD: got " + ignored);
              }
        
        Show
        jzhuge John Zhuge added a comment - After some debugging, the bogus user is passed to NN for both trunk and 2.6. In the trunk, NN.checkOwner checks the existence of the file first before checking permission: void checkOwner(FSPermissionChecker pc, INodesInPath iip) throws AccessControlException, FileNotFoundException { if (iip.getLastINode() == null ) { throw new FileNotFoundException( "Directory/File does not exist " + iip.getPath()); } checkPermission(pc, iip, true , null , null , null , null ); } In branch 2.6, it doesn't: private void checkOwner(FSPermissionChecker pc, String path) throws AccessControlException, UnresolvedLinkException { checkPermission(pc, path, true , null , null , null , null ); } The unit test calls setOwner with 2 error conditions: file doesn't exist, user doesn't have permission. What is NN expected to do? Throw FileNotFoundException or AccessControlException? Is there any doc or convention? // test permissions on files that do not exist assertFalse(userfs.exists(CHILD_FILE3)); try { userfs.setOwner(CHILD_FILE3, "foo" , "bar" ); <<<<<<<<======= fail( "setOwner should fail for non-exist file" ); } catch (java.io.FileNotFoundException ignored) { LOG.info( "GOOD: got " + ignored); }
        Hide
        yzhangal Yongjun Zhang added a comment - - edited

        Hi John Zhuge,

        Thanks a lot for your investigation and nice finds!

        Some thoughts to share:

        1. Since the test intends to see FileNotFoundException thrown for this case, looks like that we should use nnfs.setOwner instead of userfs.setOwner in this test.

        2. We can add a test to do a userfs.setOwner call on a previously created file, and expect AccesControlException

        3. When calling userfs.setOwner on a non-existing file, /a/b/c, I think, if the user has access to /a/b, then FileNotFoundException is appropriate; if the user has no permission to access /a/b, then AccessControlException is appropriate, no matter whether /a/b/c exists or not.

        Thanks.

        Show
        yzhangal Yongjun Zhang added a comment - - edited Hi John Zhuge , Thanks a lot for your investigation and nice finds! Some thoughts to share: 1. Since the test intends to see FileNotFoundException thrown for this case, looks like that we should use nnfs.setOwner instead of userfs.setOwner in this test. 2. We can add a test to do a userfs.setOwner call on a previously created file, and expect AccesControlException 3. When calling userfs.setOwner on a non-existing file, /a/b/c, I think, if the user has access to /a/b, then FileNotFoundException is appropriate; if the user has no permission to access /a/b, then AccessControlException is appropriate, no matter whether /a/b/c exists or not. Thanks.
        Hide
        jzhuge John Zhuge added a comment -

        Patch 001:

        • Add unit test Yongjun Zhang suggested
        • Complete test coverage of setOwner

        Should we add these tests to FileSystemContractTestBase?

        Show
        jzhuge John Zhuge added a comment - Patch 001: Add unit test Yongjun Zhang suggested Complete test coverage of setOwner Should we add these tests to FileSystemContractTestBase ?
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 10s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 6m 34s trunk passed
        +1 compile 0m 43s trunk passed
        +1 checkstyle 0m 26s trunk passed
        +1 mvnsite 0m 51s trunk passed
        +1 mvneclipse 0m 12s trunk passed
        +1 findbugs 1m 39s trunk passed
        +1 javadoc 0m 54s trunk passed
        +1 mvninstall 0m 46s the patch passed
        +1 compile 0m 41s the patch passed
        +1 javac 0m 41s the patch passed
        +1 checkstyle 0m 22s hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 13 unchanged - 1 fixed = 13 total (was 14)
        +1 mvnsite 0m 48s the patch passed
        +1 mvneclipse 0m 9s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 43s the patch passed
        +1 javadoc 0m 54s the patch passed
        -1 unit 57m 38s hadoop-hdfs in the patch failed.
        +1 asflicense 0m 17s The patch does not generate ASF License warnings.
        76m 2s



        Reason Tests
        Failed junit tests hadoop.hdfs.TestDFSShell



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HDFS-10376
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828733/HDFS-10376.001.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 176f13935cfc 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / fcbac00
        Default Java 1.8.0_101
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/16759/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16759/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16759/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 10s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 34s trunk passed +1 compile 0m 43s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 0m 51s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 1m 39s trunk passed +1 javadoc 0m 54s trunk passed +1 mvninstall 0m 46s the patch passed +1 compile 0m 41s the patch passed +1 javac 0m 41s the patch passed +1 checkstyle 0m 22s hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 13 unchanged - 1 fixed = 13 total (was 14) +1 mvnsite 0m 48s the patch passed +1 mvneclipse 0m 9s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 43s the patch passed +1 javadoc 0m 54s the patch passed -1 unit 57m 38s hadoop-hdfs in the patch failed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 76m 2s Reason Tests Failed junit tests hadoop.hdfs.TestDFSShell Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10376 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12828733/HDFS-10376.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 176f13935cfc 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / fcbac00 Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/16759/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16759/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16759/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        yzhangal Yongjun Zhang added a comment -

        Hi John Zhuge,

        Thanks for your thorough work here.

        All looks good, except one suggestion:

          private void testNonSuperCannotChangeOwnerForNonExistentFile()
              throws Exception {
            Path file = new Path(ROOT_PATH,
                "testNonSuperCannotChangeOwnerForNonExistentFile");
            assertFalse(userfs.exists(file));
            try {
              userfs.setOwner(file, NOUSER, null);
              fail("Expect ACE or FNFE when a non-super user tries to change owner " +
                  "for a non-existent file");
            } catch (AccessControlException e) {
              assertThat(e.getMessage(), startsWith(
                  "Non-super user cannot change owner"));
            } catch (FileNotFoundException e) {
            }
          }
        

        In this test, non-existing file can be of two types:
        1. there is one parent component that restrict the access of this non-super user, in this case, we'd expect to see AccessControlException only.
        2. all components in the path of this non-existing file allows access of this non-super user, depending on whether the implementation check file existence first, or check ownership first, we could either see AccessControlException or FileNotFoundException.

        Your test here covers type 2, wonder if we can have a test for type 1?

        Thanks.

        Show
        yzhangal Yongjun Zhang added a comment - Hi John Zhuge , Thanks for your thorough work here. All looks good, except one suggestion: private void testNonSuperCannotChangeOwnerForNonExistentFile() throws Exception { Path file = new Path(ROOT_PATH, "testNonSuperCannotChangeOwnerForNonExistentFile" ); assertFalse(userfs.exists(file)); try { userfs.setOwner(file, NOUSER, null ); fail( "Expect ACE or FNFE when a non- super user tries to change owner " + " for a non-existent file" ); } catch (AccessControlException e) { assertThat(e.getMessage(), startsWith( "Non- super user cannot change owner" )); } catch (FileNotFoundException e) { } } In this test, non-existing file can be of two types: 1. there is one parent component that restrict the access of this non-super user, in this case, we'd expect to see AccessControlException only. 2. all components in the path of this non-existing file allows access of this non-super user, depending on whether the implementation check file existence first, or check ownership first, we could either see AccessControlException or FileNotFoundException. Your test here covers type 2, wonder if we can have a test for type 1? Thanks.
        Hide
        jzhuge John Zhuge added a comment -

        Patch 002:

        • Add 2 more unit tests to cover what Yongjun Zhang described
        • The current setOwner implementation throws FNFE in the new tests because it checks the existence of the last inode in path first. However, it is also reasonable to throw ACE.
          FSDirectory#checkOwner
              if (iip.getLastINode() == null) {
                throw new FileNotFoundException(
                    "Directory/File does not exist " + iip.getPath());
              }
              checkPermission(pc, iip, true, null, null, null, null);
          
        Show
        jzhuge John Zhuge added a comment - Patch 002: Add 2 more unit tests to cover what Yongjun Zhang described The current setOwner implementation throws FNFE in the new tests because it checks the existence of the last inode in path first. However, it is also reasonable to throw ACE. FSDirectory#checkOwner if (iip.getLastINode() == null ) { throw new FileNotFoundException( "Directory/File does not exist " + iip.getPath()); } checkPermission(pc, iip, true , null , null , null , null );
        Hide
        hadoopqa Hadoop QA added a comment -
        -1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 14s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 8m 30s trunk passed
        +1 compile 0m 54s trunk passed
        +1 checkstyle 0m 26s trunk passed
        +1 mvnsite 1m 1s trunk passed
        +1 mvneclipse 0m 14s trunk passed
        +1 findbugs 1m 48s trunk passed
        +1 javadoc 1m 0s trunk passed
        +1 mvninstall 0m 49s the patch passed
        +1 compile 0m 52s the patch passed
        +1 javac 0m 52s the patch passed
        +1 checkstyle 0m 25s hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 13 unchanged - 1 fixed = 13 total (was 14)
        +1 mvnsite 0m 59s the patch passed
        +1 mvneclipse 0m 10s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 1m 58s the patch passed
        +1 javadoc 0m 56s the patch passed
        -1 unit 66m 45s hadoop-hdfs in the patch failed.
        +1 asflicense 0m 24s The patch does not generate ASF License warnings.
        88m 49s



        Reason Tests
        Failed junit tests hadoop.hdfs.server.namenode.TestAddStripedBlockInFBR
          hadoop.hdfs.TestDFSShell
          hadoop.hdfs.server.blockmanagement.TestBlockTokenWithDFS
          hadoop.hdfs.server.datanode.TestDirectoryScanner
          hadoop.hdfs.server.namenode.TestDiskspaceQuotaUpdate



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Issue HDFS-10376
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830112/HDFS-10376.002.patch
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 14776125dc25 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 6eb700e
        Default Java 1.8.0_101
        findbugs v3.0.0
        unit https://builds.apache.org/job/PreCommit-HDFS-Build/16848/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
        Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16848/testReport/
        modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
        Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16848/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 8m 30s trunk passed +1 compile 0m 54s trunk passed +1 checkstyle 0m 26s trunk passed +1 mvnsite 1m 1s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 1m 48s trunk passed +1 javadoc 1m 0s trunk passed +1 mvninstall 0m 49s the patch passed +1 compile 0m 52s the patch passed +1 javac 0m 52s the patch passed +1 checkstyle 0m 25s hadoop-hdfs-project/hadoop-hdfs: The patch generated 0 new + 13 unchanged - 1 fixed = 13 total (was 14) +1 mvnsite 0m 59s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 58s the patch passed +1 javadoc 0m 56s the patch passed -1 unit 66m 45s hadoop-hdfs in the patch failed. +1 asflicense 0m 24s The patch does not generate ASF License warnings. 88m 49s Reason Tests Failed junit tests hadoop.hdfs.server.namenode.TestAddStripedBlockInFBR   hadoop.hdfs.TestDFSShell   hadoop.hdfs.server.blockmanagement.TestBlockTokenWithDFS   hadoop.hdfs.server.datanode.TestDirectoryScanner   hadoop.hdfs.server.namenode.TestDiskspaceQuotaUpdate Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Issue HDFS-10376 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12830112/HDFS-10376.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 14776125dc25 3.13.0-93-generic #140-Ubuntu SMP Mon Jul 18 21:21:05 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 6eb700e Default Java 1.8.0_101 findbugs v3.0.0 unit https://builds.apache.org/job/PreCommit-HDFS-Build/16848/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/16848/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/16848/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        yzhangal Yongjun Zhang added a comment -

        Hi John Zhuge,

        Thanks for the new rev.

        I expect the new tests you added:

        • testNonSuperCannotChangeOwnerForNonExistentFile(); should throw FNF exception
        • testNonSuperCannotChangeOwnerForNonExistentFileInNoAccessDir(); should throw ACE exception
          but I can understand that due to a hole in other places in the code, we can not guarantee that yet.

        Would you please create a separate jira for these issues, and we can fix these tests again when that new jira is fixed.

        I think your new patch looks good, except would you please check the jenkins test failure?

        Thanks.

        Show
        yzhangal Yongjun Zhang added a comment - Hi John Zhuge , Thanks for the new rev. I expect the new tests you added: testNonSuperCannotChangeOwnerForNonExistentFile(); should throw FNF exception testNonSuperCannotChangeOwnerForNonExistentFileInNoAccessDir(); should throw ACE exception but I can understand that due to a hole in other places in the code, we can not guarantee that yet. Would you please create a separate jira for these issues, and we can fix these tests again when that new jira is fixed. I think your new patch looks good, except would you please check the jenkins test failure? Thanks.
        Hide
        jzhuge John Zhuge added a comment -

        Thanks Yongjun Zhang. I will look into it and create a new JIRA if necessary.

        Test failures are unrelated. And the patch only touches test code in TestPermission.java.

        Show
        jzhuge John Zhuge added a comment - Thanks Yongjun Zhang . I will look into it and create a new JIRA if necessary. Test failures are unrelated. And the patch only touches test code in TestPermission.java .
        Hide
        yzhangal Yongjun Zhang added a comment -

        Indeed the tests are unrelated. +1 on patch rev2.

        Let's follow-up with the remaining issue separately.

        Thanks.

        Show
        yzhangal Yongjun Zhang added a comment - Indeed the tests are unrelated. +1 on patch rev2. Let's follow-up with the remaining issue separately. Thanks.
        Hide
        yzhangal Yongjun Zhang added a comment -

        Found that Daryn Sharp is working on the issue reported here

        https://issues.apache.org/jira/browse/HDFS-10376?focusedCommentId=15520417&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15520417

        So I'm +1 on patch rev1, and will commit soon today.

        Hi John Zhuge, would you please remove rev2 to avoid confusion?

        Thanks.

        Show
        yzhangal Yongjun Zhang added a comment - Found that Daryn Sharp is working on the issue reported here https://issues.apache.org/jira/browse/HDFS-10376?focusedCommentId=15520417&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15520417 So I'm +1 on patch rev1, and will commit soon today. Hi John Zhuge , would you please remove rev2 to avoid confusion? Thanks.
        Hide
        jzhuge John Zhuge added a comment -
        Show
        jzhuge John Zhuge added a comment - Done Yongjun Zhang .
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10501 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10501/)
        HDFS-10376. Enhance setOwner testing. (John Zhuge via Yongjun Zhang) (yzhang: rev 2acfb1e1e4355246ef707b7c17964871b5dc7a73)

        • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/security/TestPermission.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10501 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10501/ ) HDFS-10376 . Enhance setOwner testing. (John Zhuge via Yongjun Zhang) (yzhang: rev 2acfb1e1e4355246ef707b7c17964871b5dc7a73) (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/security/TestPermission.java
        Hide
        yzhangal Yongjun Zhang added a comment -

        Committed to trunk. Thanks John Zhuge for the contribution.

        Show
        yzhangal Yongjun Zhang added a comment - Committed to trunk. Thanks John Zhuge for the contribution.
        Hide
        jzhuge John Zhuge added a comment -

        Thanks Yongjun Zhang for the review and commit!

        Show
        jzhuge John Zhuge added a comment - Thanks Yongjun Zhang for the review and commit!
        Hide
        yzhangal Yongjun Zhang added a comment -

        HI John Zhuge,

        Thanks Daryn Sharp for having fixed some relevant issues mentioned here
        https://issues.apache.org/jira/browse/HDFS-10376?focusedCommentId=15527283&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15527283

        Would you please create a new jira to add the other tests that were in rev2 that we left out when commiting the fix here?

        Thanks.

        Show
        yzhangal Yongjun Zhang added a comment - HI John Zhuge , Thanks Daryn Sharp for having fixed some relevant issues mentioned here https://issues.apache.org/jira/browse/HDFS-10376?focusedCommentId=15527283&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15527283 Would you please create a new jira to add the other tests that were in rev2 that we left out when commiting the fix here? Thanks.
        Hide
        jzhuge John Zhuge added a comment -
        Show
        jzhuge John Zhuge added a comment - Sure Yongjun Zhang

          People

          • Assignee:
            jzhuge John Zhuge
            Reporter:
            yzhangal Yongjun Zhang
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development