Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.17.2
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Fsck now checks permissions as directories are traversed. Any user can now use fsck, but information is provided only for directories the user has permission to read.

      Description

      Quoting from HADOOP-3222 ("fsck should require superuser privilege"),

      I agree that it makes sense to make fsck do permission checking for the nodes that it traverses. If a user does a fsck on files/directories that he/she has access to (using permissions) then that invocation of fsck should be allowed. Since "/" is usually owned by super-user, only super-user should be allowed to run fsck on "/".

      1. HADOOP-4268-0_20.2.patch
        23 kB
        Jitendra Nath Pandey
      2. 4268_20081230.patch
        23 kB
        Tsz Wo Nicholas Sze
      3. 4268_20081218b.patch
        26 kB
        Tsz Wo Nicholas Sze
      4. 4268_20081218.patch
        18 kB
        Tsz Wo Nicholas Sze
      5. 4268_20081217.patch
        15 kB
        Tsz Wo Nicholas Sze

        Issue Links

          Activity

          Hide
          Tsz Wo Nicholas Sze added a comment -

          Forgot to say: +1 on HADOOP-4268-0_20.2.patch.

          Show
          Tsz Wo Nicholas Sze added a comment - Forgot to say: +1 on HADOOP-4268 -0_20.2.patch.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          HADOOP-4268-0_20.2.patch looks good to me. Thanks, Jitendra!

          Show
          Tsz Wo Nicholas Sze added a comment - HADOOP-4268 -0_20.2.patch looks good to me. Thanks, Jitendra!
          Hide
          Jitendra Nath Pandey added a comment -

          Patch for Hadoop 20 is added.

          Show
          Jitendra Nath Pandey added a comment - Patch for Hadoop 20 is added.
          Hide
          Robert Chansler added a comment -

          Editorial pass over all release notes prior to publication of 0.21.

          Show
          Robert Chansler added a comment - Editorial pass over all release notes prior to publication of 0.21.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk #708 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/708/)
          . Change fsck to use ClientProtocol methods for enforcing permissions. (szetszwo)

          Show
          Hudson added a comment - Integrated in Hadoop-trunk #708 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/708/ ) . Change fsck to use ClientProtocol methods for enforcing permissions. (szetszwo)
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I just committed this.

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

          It passed all tests locally, except for a un-related test TestMapReduceLocal. See HADOOP-4907.

          Show
          Tsz Wo Nicholas Sze added a comment - It passed all tests locally, except for a un-related test TestMapReduceLocal. See HADOOP-4907 .
          Hide
          Tsz Wo Nicholas Sze added a comment -
               [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 6 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 Eclipse classpath. The patch retains Eclipse classpath integrity.
          
          Show
          Tsz Wo Nicholas Sze added a comment - [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 6 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 Eclipse classpath. The patch retains Eclipse classpath integrity.
          Hide
          Konstantin Shvachko added a comment -

          +1. This looks good to me.

          Show
          Konstantin Shvachko added a comment - +1. This looks good to me.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          4268_20081230.patch: moved the new unit test to TestFsck.

          Show
          Tsz Wo Nicholas Sze added a comment - 4268_20081230.patch: moved the new unit test to TestFsck.
          Hide
          Konstantin Shvachko added a comment -
          1. NamenodeFsck constructor has new parameters, which can be obtained from the NameNode nn parameter during the construction. Why do you need the new parameters?
          2. I do not understand the reason for factoring out TestDFSck into a separate class especially if it is in a separate package.
            I would rather place both methods from TestDFSck back into TestFsck.
          3. I don't think introduction of a new package is justified in the case.
          4. In any case TestDFSck, runDFSck anything with DFS are bad names, we should use HDFS or nothing.
          5. testPermission() should have a Javadoc explaining what the test tests and how.
            So that one could see it from the description without going through the code.
          Show
          Konstantin Shvachko added a comment - NamenodeFsck constructor has new parameters, which can be obtained from the NameNode nn parameter during the construction. Why do you need the new parameters? I do not understand the reason for factoring out TestDFSck into a separate class especially if it is in a separate package. I would rather place both methods from TestDFSck back into TestFsck . I don't think introduction of a new package is justified in the case. In any case TestDFSck , runDFSck anything with DFS are bad names, we should use HDFS or nothing. testPermission() should have a Javadoc explaining what the test tests and how. So that one could see it from the description without going through the code.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          4268_20081218b.patch: added a test.

          Show
          Tsz Wo Nicholas Sze added a comment - 4268_20081218b.patch: added a test.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          4268_20081218.patch: forgot to change DFSck in my previous patch. It temporarily includes the codes in HADOOP-4896. Tested this manually.

          Show
          Tsz Wo Nicholas Sze added a comment - 4268_20081218.patch: forgot to change DFSck in my previous patch. It temporarily includes the codes in HADOOP-4896 . Tested this manually.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          4268_20081217.patch: check permissions for fsck

          Show
          Tsz Wo Nicholas Sze added a comment - 4268_20081217.patch: check permissions for fsck

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development